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.

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

Proto encoding of registered types #276

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from
Draft

Proto encoding of registered types #276

wants to merge 21 commits into from

Conversation

liamsi
Copy link
Contributor

@liamsi liamsi commented Jun 5, 2019

ref: #267

@alexanderbez @jackzampolin this could do with more testing but I'd appreciate a first peek.

liamsi added 10 commits June 5, 2019 11:33
amino should encode registered types as this protobuf message

Signed-off-by: Ismail Khoffi <[email protected]>
Signed-off-by: Ismail Khoffi <[email protected]>
Signed-off-by: Ismail Khoffi <[email protected]>
…ompat.

tests in make file

Signed-off-by: Ismail Khoffi <[email protected]>
struct instead of operating directly on the disfix bytes

Signed-off-by: Ismail Khoffi <[email protected]>
TestCodecRoundtripUnarshalOnConcreteNonNilRegisteredTypeDef
TestCodecRoundtripMarshalOnConcreteNonNilRegisteredTypeDef
TestCodecRoundtripUnarshalOnConcreteNonNilRegisteredTypeDef
TestCodecRoundtripMarshalOnConcreteNonNilRegisteredTypeDef
Example

Signed-off-by: Ismail Khoffi <[email protected]>
Still need much more testing though

Signed-off-by: Ismail Khoffi <[email protected]>
Signed-off-by: Ismail Khoffi <[email protected]>
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

@tac0turtle
Copy link
Contributor

tac0turtle commented Aug 7, 2019

TODO:

  • remove debug output
  • Docs & Readme updates
  • test with sdk sim
  • add more tests
  • intense review

comments & todos for @marbar3778 to catch up

Signed-off-by: Ismail Khoffi <[email protected]>
@liamsi
Copy link
Contributor Author

liamsi commented Aug 14, 2019

As discussed with @marbar3778 the changes here pose a problem when bechifying the consensus pubkey in the SDK: https://github.com/cosmos/cosmos-sdk/blob/f1adb7afd974ee50ac9ac83a315cde932f47d7ef/x/staking/types/validator.go#L181-L182
creates a too large string (92 bytes) which can not be decoded later via:
https://github.com/cosmos/cosmos-sdk/blob/f1adb7afd974ee50ac9ac83a315cde932f47d7ef/x/staking/types/validator.go#L203-L208

This increase is due to the overhead introduced here (prefix and value are encoded as fields now). The simplest way to understand and reproduce this is via running TestRandBech32PubkeyConsistency in the sdk. The simulation will also fail (the simulation is where Marko noticed the problem).

@liamsi
Copy link
Contributor Author

liamsi commented Aug 14, 2019

There seems to be another bug in the changes here. Add this test (in types/address_test.go in the sdk for instance):

func TestValAddrMustMarshal(t *testing.T) {
	var pub ed25519.PubKeyEd25519

	tSlice := make([]types.ValAddress, 20)
	for i := 0; i < 20; i++ {
		rand.Read(pub[:])
		acc := types.ValAddress(pub.Address())
		tSlice[i] = acc		
	}
	bz := codec.Cdc.MustMarshalBinaryLengthPrefixed(tSlice)
	timeslice := []types.ValAddress{}
	codec.Cdc.MustUnmarshalBinaryLengthPrefixed(bz, &timeslice)
}

@liamsi
Copy link
Contributor Author

liamsi commented Aug 14, 2019

The latter might be somewhat related to: #272

In UnbondAllMatureValidatorQueue the timeslice := []sdk.ValAddress{} is implicitly of type
[][]byte (ValAddress is just bytes). But this should (and previously did) work just fine. Also, repeated bytes are allowed in protobuf.

binary-encode.go Outdated Show resolved Hide resolved
@zmanian zmanian self-requested a review August 31, 2019 01:38
amino.go Show resolved Hide resolved
amino.go Show resolved Hide resolved
amino.go Show resolved Hide resolved
amino.go Show resolved Hide resolved
binary-decode.go Show resolved Hide resolved
binary-decode.go Show resolved Hide resolved
binary-decode.go Show resolved Hide resolved
binary-decode.go Show resolved Hide resolved
binary-encode.go Show resolved Hide resolved
@tessr
Copy link
Contributor

tessr commented Nov 11, 2019

This linter is INTENSE!

@tac0turtle
Copy link
Contributor

ignore these linters will fix it in a coming pr

amino.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants