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

refactor!: add v2 MerklePath with bytes in favour of strings #6644

Merged
merged 21 commits into from
Jul 3, 2024

Conversation

damiannolan
Copy link
Member

@damiannolan damiannolan commented Jun 19, 2024

Description

Follow up to #6633.

  • Introduces proto pkg v2 for ibc commitment
  • Deprecate and reserve QueryVerifyMembership merkle path field
  • Extend 08-wasm VerifyMembershipMsg and VerifyNonMembershipMsg with optional field for non utf8 encoded merkle paths

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.

Base automatically changed from damian/6496-merklepath-bz to main June 19, 2024 10:41
@damiannolan damiannolan marked this pull request as ready for review June 19, 2024 10:58
@damiannolan damiannolan added the priority PRs that need prompt reviews label Jun 19, 2024
Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

LGTM, happy enough with it as is!

modules/light-clients/07-tendermint/upgrade.go Outdated Show resolved Hide resolved
Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for the split-up 🙏

modules/core/23-commitment/types/merkle.go Outdated Show resolved Hide resolved
modules/light-clients/07-tendermint/upgrade.go Outdated Show resolved Hide resolved
Copy link
Contributor

@gjermundgaraba gjermundgaraba left a comment

Choose a reason for hiding this comment

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

Clean change! 🚀

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Maybe you are already planning to do this in a follow-up PR, but looks like many (if not all) of the ...Path functions could be made unexported (or even completely removed and move their code to the corresponding ...Key function.

It would nice also to have changelog :) And document also in the migration docs the change in NewMerklePath.

@damiannolan
Copy link
Member Author

Maybe you are already planning to do this in a follow-up PR, but looks like many (if not all) of the ...Path functions could be made unexported (or even completely removed and move their code to the corresponding ...Key function.

It would nice also to have changelog :) And document also in the migration docs the change in NewMerklePath.

Yep! I mentioned it on the issue: #6496 (comment)

Adding changelog now!

@damiannolan damiannolan mentioned this pull request Jun 20, 2024
10 tasks
@damiannolan
Copy link
Member Author

Currently been holding off on merging this until coming to a resolution for wasm e2e test failure.

It looks like the change to merkle path means that VerifyMembership msgs in SudoMsg for client contract execution will encode the field as a base64 encoded string. See #6660 (comment)

@colin-axner colin-axner self-requested a review June 24, 2024 09:47
@damiannolan damiannolan marked this pull request as ready for review June 25, 2024 14:34
@damiannolan
Copy link
Member Author

Wasm e2e tests got past channel creation which proves the changes are working as expected, unfortunately they are failing for other reasons still. Would be nice to resolve them somehow or turn them off altogether and run when needed...

Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

I've verified that this fix works e2e. I've used it verify a non-string cosmwasm state here.

func IsValidUTF8(keyPath [][]byte) bool {
var key []byte
for _, bz := range keyPath {
key = append(key, bz...)
Copy link
Contributor

Choose a reason for hiding this comment

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

each consituent path should be valid utf8 no? This could return early on first non-utf8 valid key path?

Copy link
Member Author

@damiannolan damiannolan Jun 26, 2024

Choose a reason for hiding this comment

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

Indeed, I think you're right! We can make that optimisation here 👍🏻 I'll push the change later today.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in: ba58f37 and added more tests!

modules/light-clients/08-wasm/types/contract_api.go Outdated Show resolved Hide resolved
Comment on lines 74 to 75
MerklePath *commitmenttypesv2.MerklePath `json:"merkle_path,omitempty"`
Path commitmenttypes.MerklePath `json:"path"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess tradeoff here is pushing a potential migration to the future as opposed to right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, as I mentioned here #6496 (comment), I'm not too happy about it.

Changing it forces a migration on existing contracts. But I don't see any clear path forward on when you could switch over and fully deprecate the Path field. In fact, I think the longer it stays like this the more disruption it would cause to client contract users, as right now I don't think there is a large amount deployed and in use in prod, whereas if this continues to exist and more are deployed then we are forcing a larger amount of downtime in the event that we actually do change it at some point.
Alternatively, it stays like this forever!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we ever had a discussion on versioning the contract api for these sort of situations. Agree that I think this will stay forever or force even more clients to eventually need to migrate.

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @damiannolan! The changes are clean. Agreed the addition of a new field is not ideal, but I think works okay in the short term. At the very least, I don't feel it added much complexity on our side

modules/light-clients/08-wasm/light_client_module.go Outdated Show resolved Hide resolved
modules/light-clients/08-wasm/types/contract_api_test.go Outdated Show resolved Hide resolved
@crodriguezvega
Copy link
Contributor

crodriguezvega commented Jul 3, 2024

@damiannolan could you please also write some explanation here of the migration needed for contract developers?

CHANGELOG.md Outdated Show resolved Hide resolved
@damiannolan
Copy link
Member Author

@damiannolan could you please also write some explanation here of the migration needed for contract developers?

Yes! Can add in a follow up PR.

@damiannolan damiannolan disabled auto-merge July 3, 2024 12:48
@damiannolan damiannolan added this pull request to the merge queue Jul 3, 2024
Copy link

sonarcloud bot commented Jul 3, 2024

Merged via the queue into main with commit 2bd5920 Jul 3, 2024
89 of 94 checks passed
@damiannolan damiannolan deleted the damian/6496-merkle-path-constructor-breakage branch July 3, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority PRs that need prompt reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants