Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create converter to/from numerical and string properties #34

Open
lucasvo opened this issue Dec 4, 2018 · 22 comments
Open

Create converter to/from numerical and string properties #34

lucasvo opened this issue Dec 4, 2018 · 22 comments

Comments

@lucasvo
Copy link
Member

lucasvo commented Dec 4, 2018 •
edited
Loading

We should create a helper that can take a binary property and convert it to a literal property without having an instance of the message available but only the type. The converter needs to be smart enough to concatenate nested fields and repeated fields.

Acceptance Criteria:

  1. A method is added to the proofs package that accepts a literal property string, a protobuf autogenerated struct type and returns the compact property as []byte
  2. A method is added to the proofs package that accepts a compact property []byte, a protobuf autogenerated struct type and returns a literal property as string
  3. There is adequate test coverage for the above two methods
  4. The example is extended with an example usage (https://github.com/centrifuge/precise-proofs/blob/master/proofs/tree_test.go#L1351)

For more info on literal types and it's implementation see
#29
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 280.0 DAI (280.0 USD @ $1.0/DAI) attached to it as part of the Centrifuge fund.

@gitcoinbot
Copy link

gitcoinbot commented Dec 25, 2018 •
edited
Loading

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 3 weeks, 3 days ago.
Please review their action plans below:

1) stefanhans has been approved to start work.

I've forked it and tested it. Next, I will analyze and play around a bit to get a deeper understanding especially from already implemented protobuf types etc. Then, I can start the first implementation or I have questions. Let's see. Currently, I've scheduled to work at two days a week on it. But, I can change that if needed.

Learn more on the Gitcoin Issue Details page.

@stefanhans
Copy link

I have analyzed and played around a bit...

// GetCompactPropertyFromType returns a compact property 
// matching the specified protobuf type's fieldname
func GetCompactPropertyFromType(i interface{}, prop string) ([]byte, error) {

	// Get type and check it is a struct
	t := reflect.TypeOf(i)
	if t.Kind() != reflect.Struct {
		return nil, fmt.Errorf("%v is not a struct\n", t.Kind())
	}

	// Loop over all fields and returns compact property if protobuf name does match
	for i := 0; i < t.NumField(); i++ {

		if s, ok := t.Field(i).Tag.Lookup("protobuf"); ok {
			protoFieldName := strings.Split(strings.Split(s, "name=")[1], ",")[0]

			if protoFieldName == prop {
				return proofs.Empty.FieldProp(prop, proofs.FieldNum(i)).Compact, nil
			}
		}
	}
	return nil, fmt.Errorf("%v not found\n", prop)
}

// GetPropertyStringFromType returns a protobuf type's fieldname 
// matching the specified compact property
func GetPropertyStringFromType(i interface{}, prop []byte) (string, error) {

	// Get type and check it is type struct
	t := reflect.TypeOf(i)
	if t.Kind() != reflect.Struct {
		return "", fmt.Errorf("%v is not a struct\n", t.Kind())
	}

	// Get field with index of last byte of compact property
	field := t.Field(int(prop[len(prop)-1]))

	// Look for protobuf first name in field tag and returns it if found
	if s, ok := field.Tag.Lookup("protobuf"); ok {
		return strings.Split(strings.Split(s, "name=")[1], ",")[0], nil
	}

	return "", fmt.Errorf("%v not found\n", prop)
}

Calling

propStr := "value1"

b, err := GetCompactPropertyFromType(documentspb.ExampleDocument{}, propStr)
checkErr(err)
fmt.Printf("GetCompactPropertyFromType(documentspb.ExampleDocument{}, %q): %v\n", propStr, b)

str, err := GetPropertyStringFromType(documentspb.ExampleDocument{}, b)
checkErr(err)
fmt.Printf("GetPropertyStringFromType(documentspb.ExampleDocument{}, %v: %q\n", b, str)

returns

GetCompactPropertyFromType(documentspb.ExampleDocument{}, "value1"): [0 0 0 0 0 0 0 2]
GetPropertyStringFromType(documentspb.ExampleDocument{}, [0 0 0 0 0 0 0 2]: "value1"

Am I on the right track?

@lucasvo
Copy link
Member Author

lucasvo commented Jan 9, 2019

Yes, this looks good. Now the only thing that is missing are all the other edge cases, like nesting & repeated fields ;)

@stefanhans
Copy link

  • Let's say we have this structure
Address *ExampleConverter_Address
        Country string
        City    *ExampleConverter_Address_City
                Name    string
                Code    int32
        Street  string

valid conversions?

address <-> [0 0 0 0 0 0 0 13]
address.city <-> [0 0 0 0 0 0 0 13 0 0 0 0 0 0 0 1]
address.city.name <-> [0 0 0 0 0 0 0 13 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 0]
  • Let's say we have that structure
Phone   []*ExampleConverter_PhoneNumber
        Number  string
        Type    string

valid conversions?

phone <-> [0 0 0 0 0 0 0 12]
phone.type <-> [0 0 0 0 0 0 0 12 0 0 0 0 0 0 0 1]

or

phone.type <-> [0 0 0 0 0 0 0 12 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1]

Details

@lucasvo
Copy link
Member Author

lucasvo commented Jan 12, 2019

For starters, I think you are using uint64 instead of uint32 for protobuf tag numbers. Additionally, phone is a repeated field in your example. This means that phone.type is actually an invalid tag. A valid tag would be: phone[0].type. phone itself would never be used either as it would always have to be phone[0].XXX

Additionally it looks like you are off by one. Phone's tag number is 13, not 12. For repeated fields, we are using a uint64 as an array index. So valid tags are:

phone[0] -> [
    0, 0, 0, 13, # Tag phone
    0, 0, 0, 0, 0, 0, 0,  0,  # uint64(0)
    0, 0, 0, 1]

I would recommend you check out the javascript implementation of this, it shows you all the different cases and rules:

https://github.com/centrifuge/precise-proofs-js/blob/master/src/PropertyTransformHelper.js

@stefanhans
Copy link

There is

type Property struct {
	Parent     *Property
	Text       string
	Compact    []byte
	NameFormat string
}

at the core of the converter, isn't it?

We walk through the protobuffer message type via reflect to convert the path of the structure to the matching node, right?

This leads us to the bijective function of the converter. But this is not sufficient. There are other pieces of code using the converter output, right?

@lucasvo
Copy link
Member Author

lucasvo commented Jan 13, 2019

These properties are built by iterating over all of the fields in an instance of the struct. So this works only when the exact instance of the struct is available. The converter must work solely by having access to the Protobuf struct, not an instance of it. This should example should make it clear:

field[0].subfield
field[1].subfield
map_field[blah].otherfield

Without an instance of the struct, you wouldn't know what the length of field is (if it has 0, 1, 2, 3 items in the list for example). Likewise with mappings, you couldn't do a simple lookup in properties because not all map entries are. So the converter needs to work solely on the schema independent of the existing code.

@stefanhans
Copy link

Thx, I'll work on tomorrow.

@stefanhans
Copy link

Test for all used types and nested combinations finished (100% coverage)

Generalized test "only a message type needed" mostly written - during that I've found combinations not covered

Problems with the dev environment and protobuffer

i.e. under examples/documents: protoc -I $GOPATH/src/github.com/stefanhans/precise-proofs -I . --go_out=. example.proto
compiles
under examples
go build says

build github.com/stefanhans/precise-proofs/examples: cannot find module for path proofs/proto
I do not need proofs/proto, so
//import _ "proofs/proto"
solves the problem for the time being, but ...

I will reset the whole environment and try to fix it first - next week

@stefanhans
Copy link

details

@gitcoinbot
Copy link

@stefanhans Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@stefanhans
Copy link

Hi there,

I will continue my work tomorrow.

@lucasvo Are you online at slack tomorrow afternoon?

@stefanhans
Copy link

I had found a bug in the converter and I've decided to change to TDD.

So, I've worked on a testing environment, i.e. tests which

  • accept any protobuf message type
  • create a slice of strings containing any literal property
  • do a forward-backward test for each string literal

It is a bit frustrating to find some missing cases (or little bugs), create or edit message types, add test cases, and to change already tested functions. Imho, without appropriate testing it would be very likely to have a buggy converter - if the converter must be able to work with every possible message type.

Today is the last day of the bounty. Please give me feedback if you agree on my approach and if I should go on.

@lucasvo
Copy link
Member Author

lucasvo commented Jan 23, 2019

I think the approach is good, the one thing that would be great is if you could use the existing example.proto messages to build the test cases. The rest of the library is built with it, those should be used as well for the converter of creating new ones.

To generate the go bindings, check out the go:generate instruction:
https://github.com/centrifuge/precise-proofs/blob/master/examples/documents/documents.go#L3

@stefanhans
Copy link

stefanhans commented Jan 24, 2019 via email

@gitcoinbot
Copy link

@stefanhans Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@stefanhans
Copy link

Currently, I'm sick, but I will continue to work on Friday, latest.

@stefanhans
Copy link

I did some work today. I'm still a bit sick. It was not very effective 😞

Concerning the message type NestedMap, are this all literal properties?

"value[0]"
"value[0].value[0]"
"value[0].value[0].value"

@stefanhans
Copy link

I do exclude recursion and return an error, e.g.

message ExampleOfRecursiveStruct
{
  string RSstring = 1;
  ExampleOfRecursiveStruct RSstruct = 2;
}

error: documentspb.ExampleOfRecursiveStruct is a recursive struct

@lucasvo
Copy link
Member Author

lucasvo commented Feb 4, 2019

Did you already submit a "WiP PR"? That's usually easiest to see what the status is so far.

@stefanhans
Copy link

Not yet. I did not want to spam with my code before I'm sure how to design the converter.

Actually, I did a fresh start after ending up with a too much confusing design. Maybe I'm overdoing it. But if the code is not maintainable before it's done, I hesitate to finish it anyhow. Especially, if I do not know how far I have to go still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@lucasvo @stefanhans @gitcoinbot