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

refactor!: adapt merkle path to use repeated bytes in favour of strings #6633

Merged
merged 4 commits into from
Jun 19, 2024
Merged

refactor!: adapt merkle path to use repeated bytes in favour of strings #6633

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0d2789
refactor: adapt merkle path to use repeated bytes in favour of strings
damiannolan Jun 18, 2024
a82bdbb
Merge branch 'main' into damian/6496-merklepath-bz
damiannolan Jun 18, 2024
1ad869c
Merge branch 'main' into damian/6496-merklepath-bz
damiannolan Jun 19, 2024
98e1b7a
chore: add changelog
damiannolan Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 31 additions & 31 deletions modules/core/23-commitment/types/commitment.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 11 additions & 3 deletions modules/core/23-commitment/types/merkle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,13 @@ var _ exported.Path = (*MerklePath)(nil)
// NewMerklePath creates a new MerklePath instance
// The keys must be passed in from root-to-leaf order
func NewMerklePath(keyPath ...string) MerklePath {
var path [][]byte
for _, key := range keyPath {
path = append(path, []byte(key))
}

return MerklePath{
KeyPath: keyPath,
KeyPath: path,
}
}

Expand All @@ -81,7 +86,7 @@ func (mp MerklePath) GetKey(i uint64) ([]byte, error) {
if i >= uint64(len(mp.KeyPath)) {
return nil, fmt.Errorf("index out of range. %d (index) >= %d (len)", i, len(mp.KeyPath))
}
return []byte(mp.KeyPath[i]), nil
return mp.KeyPath[i], nil
}

// Empty returns true if the path is empty
Expand All @@ -95,7 +100,10 @@ func ApplyPrefix(prefix exported.Prefix, path MerklePath) (MerklePath, error) {
if prefix == nil || prefix.Empty() {
return MerklePath{}, errorsmod.Wrap(ErrInvalidPrefix, "prefix can't be empty")
}
return NewMerklePath(append([]string{string(prefix.Bytes())}, path.KeyPath...)...), nil

return MerklePath{
KeyPath: append([][]byte{prefix.Bytes()}, path.KeyPath...),
}, nil
}

// VerifyMembership verifies the membership of a merkle proof against the given root, path, and value.
Expand Down
2 changes: 1 addition & 1 deletion modules/core/23-commitment/types/merkle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func TestApplyPrefix(t *testing.T) {

pathStr := "pathone/pathtwo/paththree/key"
path := types.MerklePath{
KeyPath: []string{pathStr},
KeyPath: [][]byte{[]byte(pathStr)},
}

prefixedPath, err := types.ApplyPrefix(prefix, path)
Expand Down
2 changes: 1 addition & 1 deletion proto/ibc/core/commitment/v1/commitment.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ message MerklePrefix {
// arbitrary structured object (defined by a commitment type).
// MerklePath is represented from root-to-leaf
message MerklePath {
repeated string key_path = 1;
repeated bytes key_path = 1;
Copy link

Choose a reason for hiding this comment

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

This is a proto breaking change, what's the tradeoff here vs. field deprecation?

Copy link
Member Author

@damiannolan damiannolan Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @erwanor! Does this raise any concern in particular for you and do you depend on the message type?

This is a bit of a protobuf grey area. Myself and @colin-axner looked into this quite a bit to ensure it was okay to make this change in order to support proving kvs stored under non utf8 encoded keys (as requested by union).

Thus far we've considered this Go api breaking but not actually proto encoding breaking. Backwards compatibility should be maintained with this change. I wrote a quick test to validate this, introducing a temp LegacyMerklePath type with repeated string (old diff). I also tested for equality of bytes when both types are marshalled using proto codec.

func (suite *MerkleTestSuite) TestMerklePathEncoding() {
	encodingCfg := moduletestutil.MakeTestEncodingConfig()

	keyPath := []string{"ibc", "commitments/ports/transfer/channels/channel-0/sequences/1"}
	merklePath := types.NewMerklePath(keyPath...)

	bz, err := encodingCfg.Codec.Marshal(&merklePath)
	suite.Require().NoError(err)

	var legacy types.LegacyMerklePath
	err = encodingCfg.Codec.Unmarshal(bz, &legacy)
	suite.Require().NoError(err)

	suite.Require().Equal(keyPath, legacy.KeyPath)
}

Some of the resources which helped us come to this conclusion:

https://protobuf.dev/programming-guides/encoding/#structure

The binary version of a message just uses the field’s number as the key – the name and declared type for each field can only be determined on the decoding end by referencing the message type’s definition (i.e. the .proto file).

https://protobuf.dev/programming-guides/proto2/#updating

string and bytes are compatible as long as the bytes are valid UTF-8.

It's possible that we may have overlooked something wrt to rust users/tooling, please let us know if you have any concerns!

Copy link
Member Author

Choose a reason for hiding this comment

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

The biggest concern for me here was that when encoding to JSON you now end up with a key_path field which is a base64 encoded byte string.

Copy link
Member Author

@damiannolan damiannolan Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also happy to introduce a v2 proto pkg and adjust ibc-go code as necessary. If it makes things easier to keep the old type around! Let me know what you think!

}

// MerkleProof is a wrapper type over a chain of CommitmentProofs.
Expand Down
Loading