fix(statemachine)!: use path within IBC store without escaping characters in solomachine #4429

crodriguezvega · 2023-08-22T22:23:12Z

Description

As discussed with @colin-axner, I implemented changes so that:

the bytes the solo machine signs over are in a key that does not include the IBC store prefix, since the IBC store prefix only makes send in a multistore, and the diversifier already satisfy the function of namespacing the data.
both String and Pretty functions become unused and can be removed later on. They make assumptions about formatting a merkle path in a multi store into a string that we shouldn't be making.

Commit Message / Changelog Entry

type: commit message

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.

codecov-commenter · 2023-08-22T22:28:34Z

Codecov Report

Attention: Patch coverage is 55.55556% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.47%. Comparing base (cef7ee2) to head (6a74acb).
Report is 1117 commits behind head on main.

Files with missing lines	Patch %	Lines
...dules/light-clients/06-solomachine/client_state.go	46.66%	5 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4429      +/-   ##
==========================================
- Coverage   79.50%   79.47%   -0.04%     
==========================================
  Files         188      188              
  Lines       13038    13045       +7     
==========================================
+ Hits        10366    10367       +1     
- Misses       2241     2245       +4     
- Partials      431      433       +2

Files with missing lines	Coverage Δ
modules/core/23-commitment/types/merkle.go	`69.09% <100.00%> (+1.04%)`	⬆️
modules/light-clients/07-tendermint/upgrade.go	`73.86% <100.00%> (ø)`
...dules/light-clients/06-solomachine/client_state.go	`82.73% <46.66%> (-5.55%)`	⬇️

crodriguezvega · 2023-08-22T22:24:44Z

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

@@ -81,6 +81,12 @@ func NewMerklePath(keyPath ...string) MerklePath {
 // This represents the path in the same way the tendermint KeyPath will
 // represent a key path. The backslashes partition the key path into
 // the respective stores they belong to.
+//
+// Deprecated: This function makes assumptions about the way a merkle path


I added a deprecated notice to these functions instead of removing them completely because I think removing them is API breaking and if I remove them in this PR the backport to v7.3.x will be more difficult. I will remove them in a follow up PR once this one gets merged.

crodriguezvega · 2023-08-22T22:25:08Z

modules/core/exported/commitment.go

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


If we remove this function, I am not sure if this interface makes much sense anymore...

crodriguezvega · 2023-08-23T06:02:38Z

testing/solomachine.go

@@ -516,11 +516,14 @@ func (solo *Solomachine) GenerateClientStateProof(clientState exported.ClientSta
 	data, err := clienttypes.MarshalClientState(solo.cdc, clientState)
 	require.NoError(solo.t, err)

+	merklePath := solo.GetClientStatePath(clientIDSolomachine)


These testing functions in this file return a merle path with 2 elements (first one for the IBC store), I could also change those to return only the second element. So something like this for GetClientStatePath:

func (solo *Solomachine) GetClientStatePath(counterpartyClientIdentifier string) commitmenttypes.MerklePath { return commitmenttypes.NewMerklePath(host.FullClientStatePath(counterpartyClientIdentifier)) }

Although the function the becomes a one-liner, so maybe we don't even to have a separate function to construct the path. Then in the test I would get key 0, instead of 1 as the path over which the solo machine signs the bytes.

Let me know what you think.

I think we should just call the 24-host paths directly in our code and remove these functions:

key := host.FullClientStatePath(counterpartyClientIdentififer)

I'm okay if you want to do it in a followup so as to avoid breaking changes in backport

Yes, I will do that better in a follow-up PR, otherwise if I remove these functions now that's also API breaking.

colin-axner

LGTM

colin-axner · 2023-08-23T09:54:15Z

modules/core/exported/commitment.go

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


colin-axner · 2023-08-23T09:58:32Z

testing/solomachine.go

@@ -516,11 +516,14 @@ func (solo *Solomachine) GenerateClientStateProof(clientState exported.ClientSta
 	data, err := clienttypes.MarshalClientState(solo.cdc, clientState)
 	require.NoError(solo.t, err)

+	merklePath := solo.GetClientStatePath(clientIDSolomachine)


I think we should just call the 24-host paths directly in our code and remove these functions:

key := host.FullClientStatePath(counterpartyClientIdentififer)

I'm okay if you want to do it in a followup so as to avoid breaking changes in backport

…olomachine-client

damiannolan

Looks good to me! LGTM, left one question about the GetKey method

damiannolan · 2023-08-24T10:19:19Z

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

+	}
+
+	// in a multistore context: index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
+	key, err := merklePath.GetKey(1)


GetKey seems to do some unescaping on the string also, is this okay here?

from GetKey:

key, err := url.PathUnescape(mp.KeyPath[i])

Maybe we should just use merklePath.KeyPath[1]?

wouldn't this be fine considering escaping calls have been removed? It seems like it would just be best to tweak GetKey.

That's a good observation @damiannolan!

I think it is still fine, since it is doing unescaping. And also according to ICS 24 % is not a valid character in identifiers and paths, so it should not happen that a path contains an escaped character and then GetKey would return it unescaped.

What do you guys prefer to do?

Leave as is.

Use merklePath.KeyPath[1]

Tweak GetKey. I would prefer to do this in a separate PR where I remove the String and Pretty functions.

Happy with opt 2 or 3 I think!

if we're having a vote I'd go towards 3.

~~Ok, I will go for 2 and implement 3 in the next PR.~~ Nah, I changed my mind: I will go for 3, will make working on the follow-up PR easier.

Done. I will merge when tests pass.

…olomachine-client

…ers in solomachine (#4429) (cherry picked from commit 98b0c99) # Conflicts: # modules/light-clients/06-solomachine/client_state.go # modules/light-clients/07-tendermint/upgrade.go

…ters in solomachine (backport #4429) (#4453) * fix(statemachine)!: use key within IBC store without escaping characters in solomachine (#4429) (cherry picked from commit 98b0c99) # Conflicts: # modules/light-clients/06-solomachine/client_state.go # modules/light-clients/07-tendermint/upgrade.go * fix conflicts * fix package usage * use sdkerrors * add changelog * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md --------- Co-authored-by: Carlos Rodriguez <[email protected]>

…rklePath` (#4459) ## Description Second part of #4128. First part was #4429. closes: #4128 ### Commit Message / Changelog Entry ```bash fix(api)!: remove `Pretty` and `String` custom implementations of `MerklePath` ``` see the [guidelines](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) 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. - [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#pull-request-targeting)). - [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/11-structure.md) and [Go style guide](../docs/dev/go-style-guide.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/main/testing/README.md#ibc-testing-package). - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`). - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Provide a [commit message](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) to be used for the changelog entry in the PR description for review. - [x] Re-reviewed `Files changed` in the Github PR explorer. - [ ] Review `Codecov Report` in the comment section below once CI passes.

use path in IBC store without escaping characters for solo machine

8a59dbb

crodriguezvega added the backport-to-v7.3.x label Aug 22, 2023

crodriguezvega linked an issue Aug 22, 2023 that may be closed by this pull request

Wrong URL encoded path used in solomachine client. #4128

Closed

3 tasks

comment

f946251

crodriguezvega commented Aug 22, 2023

View reviewed changes

crodriguezvega commented Aug 23, 2023

View reviewed changes

colin-axner approved these changes Aug 23, 2023

View reviewed changes

crodriguezvega marked this pull request as ready for review August 24, 2023 10:06

crodriguezvega requested review from AdityaSripal, damiannolan, charleenfei, chatton and DimitrisJim as code owners August 24, 2023 10:06

Merge branch 'main' into carlos/4128-wrong-url-encoded-path-used-in-s…

de738a8

…olomachine-client

damiannolan approved these changes Aug 24, 2023

View reviewed changes

DimitrisJim approved these changes Aug 24, 2023

View reviewed changes

Carlos Rodriguez added 2 commits August 24, 2023 14:13

remove character unescaping in GetKey

7424f5c

Merge branch 'main' into carlos/4128-wrong-url-encoded-path-used-in-s…

6a74acb

…olomachine-client

crodriguezvega mentioned this pull request Aug 24, 2023

Wrong URL encoded path used in solomachine client. #4128

Closed

3 tasks

crodriguezvega merged commit 98b0c99 into main Aug 24, 2023

crodriguezvega deleted the carlos/4128-wrong-url-encoded-path-used-in-solomachine-client branch August 24, 2023 12:49

mergify bot mentioned this pull request Aug 24, 2023

fix(statemachine)!: use path within IBC store without escaping characters in solomachine (backport #4429) #4453

Merged

crodriguezvega mentioned this pull request Aug 24, 2023

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

Merged

9 tasks

crodriguezvega pushed a commit to cosmos/ibc that referenced this pull request Sep 14, 2023

ICS 06: updates for cosmos/ibc-go#4429

1179885

crodriguezvega mentioned this pull request Sep 14, 2023

ICS 06: updates for using unprefixed path in sign bytes cosmos/ibc#1017

Merged

AdityaSripal pushed a commit to cosmos/ibc that referenced this pull request Sep 18, 2023

ICS 06: updates for cosmos/ibc-go#4429 (#1017)

2c557ec

asalzmann mentioned this pull request Jan 11, 2024

remove url escape icq Stride-Labs/stride#1068

Merged

kruspy mentioned this pull request Apr 18, 2024

fix: remove merkle path escaping persistenceOne/persistence-sdk#411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(statemachine)!: use path within IBC store without escaping characters in solomachine #4429

fix(statemachine)!: use path within IBC store without escaping characters in solomachine #4429

crodriguezvega commented Aug 22, 2023 •

edited

Loading

codecov-commenter commented Aug 22, 2023 •

edited

Loading

crodriguezvega Aug 22, 2023

crodriguezvega Aug 22, 2023

colin-axner Aug 23, 2023

crodriguezvega Aug 23, 2023 •

edited

Loading

colin-axner Aug 23, 2023

crodriguezvega Aug 24, 2023

colin-axner left a comment

colin-axner Aug 23, 2023

colin-axner Aug 23, 2023

damiannolan left a comment

damiannolan Aug 24, 2023

colin-axner Aug 24, 2023

DimitrisJim Aug 24, 2023

crodriguezvega Aug 24, 2023

crodriguezvega Aug 24, 2023 •

edited

Loading

damiannolan Aug 24, 2023

DimitrisJim Aug 24, 2023

crodriguezvega Aug 24, 2023 •

edited

Loading

crodriguezvega Aug 24, 2023

fix(statemachine)!: use path within IBC store without escaping characters in solomachine #4429

fix(statemachine)!: use path within IBC store without escaping characters in solomachine #4429

Conversation

crodriguezvega commented Aug 22, 2023 • edited Loading

Description

Commit Message / Changelog Entry

codecov-commenter commented Aug 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega Aug 23, 2023 • edited Loading

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

damiannolan 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 Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega commented Aug 22, 2023 •

edited

Loading

codecov-commenter commented Aug 22, 2023 •

edited

Loading

crodriguezvega Aug 23, 2023 •

edited

Loading

crodriguezvega Aug 24, 2023 •

edited

Loading

crodriguezvega Aug 24, 2023 •

edited

Loading