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

[damiannolan]

Description

Initial PR. Will break the API of NewMerklePath in a following PR.

• Change MerklePath to use repeated bytes in favour of repeated string.

ref: #6496

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against the correct branch (see CONTRIBUTING.md).
• Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
• Code follows the module structure standards and Go style guide.
• Wrote unit and integration tests.
• Updated relevant documentation (docs/).
• Added relevant godoc comments.
• Provide a conventional commit message to follow the repository standards.
• Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
• Re-reviewed Files changed in the GitHub PR explorer.
• Review SonarCloud Report in the comment section below once CI passes.

[colin-axner]

Excellent! ❤️

[crodriguezvega]

Nice this refactor is started, @damiannolan!

I am wondering if it would help readability to have a type alias for [][]byte?

Should we also mention in migration docs that KeyPath type is changed from string to [][]byte?

[damiannolan]

I am wondering if it would help readability to have a type alias for [][]byte?

I mentioned something similar to @colin-axner yesterday in dms. I can explore that when the bulk of the code is updated.

Should we also mention in migration docs that KeyPath type is changed from string to [][]byte?

Yes! I'll add it to the issue 👍🏻

[damiannolan]

I'll add a changelog to this before merging

[sonarqubecloud]

Quality Gate passed for 'ibc-go'

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[erwanor]

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

[damiannolan]

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!

[damiannolan]

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.

[damiannolan]

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!