fix(api)!: remove `Pretty` and `String` custom implementations of `MerklePath` #4459

crodriguezvega · 2023-08-24T19:35:44Z

Description

Second part of #4128. First part was #4429.

closes: #4128

Commit Message / Changelog Entry

fix(api)!: remove `Pretty` and `String` custom implementations of `MerklePath`

see the guidelines for commit messages. (view raw markdown for examples)

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 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/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

crodriguezvega

The API breaking changes:

Removing String and Pretty.
Removing String from Path interface.

In which section of the migration docs should we mention them?

crodriguezvega · 2023-08-24T19:38:15Z

modules/core/exported/commitment.go

@@ -28,7 +28,6 @@ type Prefix interface {
 // Path implements spec:CommitmentPath.
 // A path is the additional information provided to the verification function.
 type Path interface {


I had to keep the interface because there was an import cycle between 02-client and 23-commitment, if I tried to use commitmenttypes.MerklePath as the type for path in the Verify(Non)Membership functions of the client state interface.

crodriguezvega · 2023-08-24T19:39:54Z

proto/ibc/core/commitment/v1/commitment.proto

@@ -26,8 +26,6 @@ message MerklePrefix {
 // arbitrary structured object (defined by a commitment type).
 // MerklePath is represented from root-to-leaf
 message MerklePath {
-  option (gogoproto.goproto_stringer) = false;


I removed this so that the proto compiler generated an implementation of String because that is needed when we do this.

I think the linked test should be updated to use a valid path for misbehaviour. A marshaled merkle path is not an expected singing path for solomachines

It turns out that we might need to remove this because of this and this (why would we require the path to be proto-marshalled when submitting misbehaviour?).

Allowing it to be proto marshaled makes sense to me, but solo machine should not be proto marshaling the path when submitting misbeahviour, I think that might be a bug

colin-axner

Looks good! But we should not be referencing merkle path in solo machines anymore for testing. We should directly use the 24-host path

modules/light-clients/06-solomachine/client_state_test.go

colin-axner · 2023-08-28T11:04:16Z

proto/ibc/core/commitment/v1/commitment.proto

@@ -26,8 +26,6 @@ message MerklePrefix {
 // arbitrary structured object (defined by a commitment type).
 // MerklePath is represented from root-to-leaf
 message MerklePath {
-  option (gogoproto.goproto_stringer) = false;


I think the linked test should be updated to use a valid path for misbehaviour. A marshaled merkle path is not an expected singing path for solomachines

colin-axner · 2023-08-28T11:05:02Z

testing/solomachine.go

+	merklePath := commitmenttypes.NewMerklePath(host.FullClientStatePath("counterparty"))
 	path, err := solo.cdc.Marshal(&merklePath)
 	require.NoError(solo.t, err)


we should be removing merkle path usage in solo machines

True! I will do that!

Is this going to be fixed in a followup?

I cannot change this one because we require at the moment the path to be a proto-marshalled MerklePath when submitting misbehaviour. I will open an issue for this.

crodriguezvega · 2023-08-28T17:56:11Z

@colin-axner I implemented the feedback, but there's one thing I couldn't change and left a question in one of your comments

# Conflicts: # modules/core/23-commitment/types/commitment.pb.go

modules/core/23-commitment/types/merkle_test.go

testing/solomachine.go

Co-authored-by: Damian Nolan <[email protected]>

colin-axner

LGTM, is there an issue for the misbehaviour unmarshaling fix?

colin-axner · 2023-09-26T10:31:49Z

modules/core/23-commitment/types/merkle_test.go

-
-	require.Equal(t, "", zeroPath.String(), "zero element path does not have correct string representation")
-	require.Equal(t, "", zeroPath.Pretty(), "zero element path does not have correct pretty string representation")
+	require.Len(t, prefixedPath.GetKeyPath(), 2, "unexpected key path length")


should this test be asserting what each element is in the key path?

colin-axner · 2023-09-26T10:33:47Z

proto/ibc/core/commitment/v1/commitment.proto

@@ -26,8 +26,6 @@ message MerklePrefix {
 // arbitrary structured object (defined by a commitment type).
 // MerklePath is represented from root-to-leaf
 message MerklePath {
-  option (gogoproto.goproto_stringer) = false;


Allowing it to be proto marshaled makes sense to me, but solo machine should not be proto marshaling the path when submitting misbeahviour, I think that might be a bug

colin-axner · 2023-09-26T10:34:39Z

testing/solomachine.go

+	merklePath := commitmenttypes.NewMerklePath(host.FullClientStatePath("counterparty"))
 	path, err := solo.cdc.Marshal(&merklePath)
 	require.NoError(solo.t, err)


Is this going to be fixed in a followup?

remove pretty and string custom implementations for merkle path

2d638cc

crodriguezvega commented Aug 24, 2023

View reviewed changes

crodriguezvega marked this pull request as ready for review August 28, 2023 07:33

crodriguezvega requested review from colin-axner, AdityaSripal, damiannolan, charleenfei, chatton and DimitrisJim as code owners August 28, 2023 07:33

colin-axner suggested changes Aug 28, 2023

View reviewed changes

Carlos Rodriguez added 2 commits August 28, 2023 19:15

Merge branch 'main' into carlos/remove-string-functions-merklepath

ed33e46

address review feedback

57150bd

Merge branch 'main' into carlos/remove-string-functions-merklepath

34c70b1

# Conflicts: # modules/core/23-commitment/types/commitment.pb.go

crodriguezvega assigned colin-axner and damiannolan Sep 18, 2023

Merge branch 'main' into carlos/remove-string-functions-merklepath

3a118d0

damiannolan approved these changes Sep 19, 2023

View reviewed changes

modules/core/23-commitment/types/merkle_test.go Outdated Show resolved Hide resolved

testing/solomachine.go Outdated Show resolved Hide resolved

Carlos Rodriguez and others added 2 commits September 19, 2023 13:27

Update modules/core/23-commitment/types/merkle_test.go

bc03ca2

Co-authored-by: Damian Nolan <[email protected]>

review comment

3c87b0e

crodriguezvega added the automerge label Sep 19, 2023

Carlos Rodriguez added 2 commits September 22, 2023 10:24

Merge branch 'main' into carlos/remove-string-functions-merklepath

f4a951d

Merge branch 'main' into carlos/remove-string-functions-merklepath

e011ab3

colin-axner approved these changes Sep 26, 2023

View reviewed changes

Merge branch 'main' into carlos/remove-string-functions-merklepath

c5f1ea7

mergify bot merged commit 26e19db into main Sep 26, 2023
60 of 61 checks passed

mergify bot deleted the carlos/remove-string-functions-merklepath branch September 26, 2023 10:45

crodriguezvega mentioned this pull request Sep 26, 2023

test extra conditions for ApplyPrefix #4764

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api)!: remove `Pretty` and `String` custom implementations of `MerklePath` #4459

fix(api)!: remove `Pretty` and `String` custom implementations of `MerklePath` #4459

crodriguezvega commented Aug 24, 2023 •

edited

Loading

crodriguezvega left a comment

crodriguezvega Aug 24, 2023

crodriguezvega Aug 24, 2023

colin-axner Aug 28, 2023

crodriguezvega Aug 28, 2023

colin-axner Sep 26, 2023

colin-axner left a comment

colin-axner Aug 28, 2023

colin-axner Aug 28, 2023

crodriguezvega Aug 28, 2023

colin-axner Sep 26, 2023

crodriguezvega Sep 26, 2023 •

edited

Loading

crodriguezvega commented Aug 28, 2023

colin-axner left a comment

colin-axner Sep 26, 2023

colin-axner Sep 26, 2023

colin-axner Sep 26, 2023

fix(api)!: remove Pretty and String custom implementations of MerklePath #4459

fix(api)!: remove Pretty and String custom implementations of MerklePath #4459

Conversation

crodriguezvega commented Aug 24, 2023 • edited Loading

Description

Commit Message / Changelog Entry

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

crodriguezvega commented Aug 28, 2023

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix(api)!: remove `Pretty` and `String` custom implementations of `MerklePath` #4459

fix(api)!: remove `Pretty` and `String` custom implementations of `MerklePath` #4459

crodriguezvega commented Aug 24, 2023 •

edited

Loading

crodriguezvega Sep 26, 2023 •

edited

Loading