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

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

Merged
merged 10 commits into from
Sep 26, 2023
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2d638cc
remove pretty and string custom implementations for merkle path
Aug 24, 2023
ed33e46
Merge branch 'main' into carlos/remove-string-functions-merklepath
Aug 28, 2023
57150bd
address review feedback
Aug 28, 2023
34c70b1
Merge branch 'main' into carlos/remove-string-functions-merklepath
Sep 12, 2023
3a118d0
Merge branch 'main' into carlos/remove-string-functions-merklepath
Sep 18, 2023
bc03ca2
Update modules/core/23-commitment/types/merkle_test.go
Sep 19, 2023
3c87b0e
review comment
Sep 19, 2023
f4a951d
Merge branch 'main' into carlos/remove-string-functions-merklepath
Sep 22, 2023
e011ab3
Merge branch 'main' into carlos/remove-string-functions-merklepath
Sep 25, 2023
c5f1ea7
Merge branch 'main' into carlos/remove-string-functions-merklepath
mergify[bot] Sep 26, 2023
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
47 changes: 24 additions & 23 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.

38 changes: 0 additions & 38 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 @@ -3,7 +3,6 @@ package types
import (
"bytes"
"fmt"
"net/url"

"github.com/cosmos/gogoproto/proto"
ics23 "github.com/cosmos/ics23/go"
Expand Down Expand Up @@ -77,44 +76,7 @@ func NewMerklePath(keyPath ...string) MerklePath {
}
}

// String implements fmt.Stringer.
// 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
// in a multistore should be represented as a string that are not standardized.
// The decision on how to represent the merkle path as a string will be deferred
// to upstream users of the type.
// This function will be removed in a next release.
func (mp MerklePath) String() string {
pathStr := ""
for _, k := range mp.KeyPath {
pathStr += "/" + url.PathEscape(k)
}
return pathStr
}

// Pretty returns the unescaped path of the URL string.
// This function will unescape any backslash within a particular store key.
// This makes the keypath more human-readable while removing information
// about the exact partitions in the key path.
//
// Deprecated: This function makes assumptions about the way a merkle path
// in a multistore should be represented as a string that are not standardized.
// The decision on how to represent the merkle path as a string will be deferred
// to upstream users of the type.
// This function will be removed in a next release.
func (mp MerklePath) Pretty() string {
path, err := url.PathUnescape(mp.String())
if err != nil {
panic(err)
}
return path
}

// GetKey will return a byte representation of the key
// after URL escaping the key element
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))
Expand Down
21 changes: 1 addition & 20 deletions 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 @@ -150,24 +150,5 @@ func TestApplyPrefix(t *testing.T) {

prefixedPath, err := types.ApplyPrefix(prefix, path)
require.NoError(t, err, "valid prefix returns error")

require.Equal(t, "/storePrefixKey/"+pathStr, prefixedPath.Pretty(), "Prefixed path incorrect")
require.Equal(t, "/storePrefixKey/pathone%2Fpathtwo%2Fpaththree%2Fkey", prefixedPath.String(), "Prefixed escaped path incorrect")
}

func TestString(t *testing.T) {
path := types.NewMerklePath("rootKey", "storeKey", "path/to/leaf")

require.Equal(t, "/rootKey/storeKey/path%2Fto%2Fleaf", path.String(), "path String returns unxpected value")
require.Equal(t, "/rootKey/storeKey/path/to/leaf", path.Pretty(), "path's pretty string representation is incorrect")

onePath := types.NewMerklePath("path/to/leaf")

require.Equal(t, "/path%2Fto%2Fleaf", onePath.String(), "one element path does not have correct string representation")
require.Equal(t, "/path/to/leaf", onePath.Pretty(), "one element path has incorrect pretty string representation")

zeroPath := types.NewMerklePath()

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
1 change: 0 additions & 1 deletion modules/core/exported/commitment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

String() string // deprecated
Empty() bool
}

Expand Down
16 changes: 7 additions & 9 deletions modules/light-clients/06-solomachine/client_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
clientStateBz, err := suite.chainA.Codec.Marshal(clientState)
suite.Require().NoError(err)

path = suite.solomachine.GetClientStatePath(counterpartyClientIdentifier)
path = sm.GetClientStatePath(counterpartyClientIdentifier)
merklePath, ok := path.(commitmenttypes.MerklePath)
suite.Require().True(ok)
key, err := merklePath.GetKey(1) // 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
Expand Down Expand Up @@ -474,7 +474,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
{
"malformed proof fails to unmarshal",
func() {
path = suite.solomachine.GetClientStatePath(counterpartyClientIdentifier)
path = sm.GetClientStatePath(counterpartyClientIdentifier)
proof = []byte("invalid proof")
},
false,
Expand Down Expand Up @@ -609,22 +609,20 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {

func (suite *SoloMachineTestSuite) TestSignBytesMarshalling() {
sm := suite.solomachine
merklePath := commitmenttypes.NewMerklePath("ibc", "solomachine")
key, err := merklePath.GetKey(1) // 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
suite.Require().NoError(err)
path := []byte("solomachine")
signBytesNilData := solomachine.SignBytes{
Sequence: sm.GetHeight().GetRevisionHeight(),
Timestamp: sm.Time,
Diversifier: sm.Diversifier,
Path: key,
Path: path,
Data: nil,
}

signBytesEmptyArray := solomachine.SignBytes{
Sequence: sm.GetHeight().GetRevisionHeight(),
Timestamp: sm.Time,
Diversifier: sm.Diversifier,
Path: key,
Path: path,
Data: []byte{},
}

Expand Down Expand Up @@ -662,7 +660,7 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
{
"success: packet receipt absence verification",
func() {
path = suite.solomachine.GetPacketReceiptPath(ibctesting.MockPort, ibctesting.FirstChannelID, 1)
path = sm.GetPacketReceiptPath(ibctesting.MockPort, ibctesting.FirstChannelID, 1)
merklePath, ok := path.(commitmenttypes.MerklePath)
suite.Require().True(ok)
key, err := merklePath.GetKey(1) // 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
Expand Down Expand Up @@ -700,7 +698,7 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
{
"malformed proof fails to unmarshal",
func() {
path = suite.solomachine.GetClientStatePath(counterpartyClientIdentifier)
path = sm.GetClientStatePath(counterpartyClientIdentifier)
proof = []byte("invalid proof")
},
false,
Expand Down
2 changes: 0 additions & 2 deletions 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,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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?).

Copy link
Contributor

Choose a reason for hiding this comment

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

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


repeated string key_path = 1;
}

Expand Down
52 changes: 18 additions & 34 deletions testing/solomachine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func (solo *Solomachine) CreateHeader(newDiversifier string) *solomachine.Header
// CreateMisbehaviour constructs testing misbehaviour for the solo machine client
// by signing over two different data bytes at the same sequence.
func (solo *Solomachine) CreateMisbehaviour() *solomachine.Misbehaviour {
merklePath := solo.GetClientStatePath("counterparty")
merklePath := commitmenttypes.NewMerklePath(host.FullClientStatePath("counterparty"))
path, err := solo.cdc.Marshal(&merklePath)
require.NoError(solo.t, err)
Comment on lines +205 to 207
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be removing merkle path usage in solo machines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True! I will do that!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to be fixed in a followup?

Copy link
Contributor Author

@crodriguezvega crodriguezvega Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


Expand Down Expand Up @@ -231,7 +231,7 @@ func (solo *Solomachine) CreateMisbehaviour() *solomachine.Misbehaviour {
// misbehaviour signaturess can have different timestamps
solo.Time++

merklePath = solo.GetConsensusStatePath("counterparty", clienttypes.NewHeight(0, 1))
merklePath = commitmenttypes.NewMerklePath(host.FullConsensusStatePath("counterparty", clienttypes.NewHeight(0, 1)))
path, err = solo.cdc.Marshal(&merklePath)
require.NoError(solo.t, err)

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

merklePath := solo.GetClientStatePath(clientIDSolomachine)
key, err := merklePath.GetKey(1) // index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
require.NoError(solo.t, err)
path := host.FullClientStateKey(clientIDSolomachine)
signBytes := &solomachine.SignBytes{
Sequence: solo.Sequence,
Timestamp: solo.Time,
Diversifier: solo.Diversifier,
Path: key,
Path: path,
Data: data,
}

Expand All @@ -536,14 +534,12 @@ func (solo *Solomachine) GenerateConsensusStateProof(consensusState exported.Con
data, err := clienttypes.MarshalConsensusState(solo.cdc, consensusState)
require.NoError(solo.t, err)

merklePath := solo.GetConsensusStatePath(clientIDSolomachine, consensusHeight)
key, err := merklePath.GetKey(1) // index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
require.NoError(solo.t, err)
path := host.FullConsensusStateKey(clientIDSolomachine, consensusHeight)
signBytes := &solomachine.SignBytes{
Sequence: solo.Sequence,
Timestamp: solo.Time,
Diversifier: solo.Diversifier,
Path: key,
Path: path,
Data: data,
}

Expand All @@ -559,14 +555,12 @@ func (solo *Solomachine) GenerateConnOpenTryProof(counterpartyClientID, counterp
data, err := solo.cdc.Marshal(&connection)
require.NoError(solo.t, err)

merklePath := solo.GetConnectionStatePath(connectionIDSolomachine)
key, err := merklePath.GetKey(1) // index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
require.NoError(solo.t, err)
path := host.ConnectionKey(connectionIDSolomachine)
signBytes := &solomachine.SignBytes{
Sequence: solo.Sequence,
Timestamp: solo.Time,
Diversifier: solo.Diversifier,
Path: key,
Path: path,
Data: data,
}

Expand All @@ -582,14 +576,12 @@ func (solo *Solomachine) GenerateChanOpenTryProof(portID, version, counterpartyC
data, err := solo.cdc.Marshal(&channel)
require.NoError(solo.t, err)

merklePath := solo.GetChannelStatePath(portID, channelIDSolomachine)
key, err := merklePath.GetKey(1) // index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
require.NoError(solo.t, err)
path := host.ChannelKey(portID, channelIDSolomachine)
signBytes := &solomachine.SignBytes{
Sequence: solo.Sequence,
Timestamp: solo.Time,
Diversifier: solo.Diversifier,
Path: key,
Path: path,
Data: data,
}

Expand All @@ -605,14 +597,12 @@ func (solo *Solomachine) GenerateChanClosedProof(portID, version, counterpartyCh
data, err := solo.cdc.Marshal(&channel)
require.NoError(solo.t, err)

merklePath := solo.GetChannelStatePath(portID, channelIDSolomachine)
key, err := merklePath.GetKey(1) // index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
require.NoError(solo.t, err)
path := host.ChannelKey(portID, channelIDSolomachine)
signBytes := &solomachine.SignBytes{
Sequence: solo.Sequence,
Timestamp: solo.Time,
Diversifier: solo.Diversifier,
Path: key,
Path: path,
Data: data,
}

Expand All @@ -623,14 +613,12 @@ func (solo *Solomachine) GenerateChanClosedProof(portID, version, counterpartyCh
func (solo *Solomachine) GenerateCommitmentProof(packet channeltypes.Packet) []byte {
commitment := channeltypes.CommitPacket(solo.cdc, packet)

merklePath := solo.GetPacketCommitmentPath(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
key, err := merklePath.GetKey(1) // index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
require.NoError(solo.t, err)
path := host.PacketCommitmentKey(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
signBytes := &solomachine.SignBytes{
Sequence: solo.Sequence,
Timestamp: solo.Time,
Diversifier: solo.Diversifier,
Path: key,
Path: path,
Data: commitment,
}

Expand All @@ -641,14 +629,12 @@ func (solo *Solomachine) GenerateCommitmentProof(packet channeltypes.Packet) []b
func (solo *Solomachine) GenerateAcknowledgementProof(packet channeltypes.Packet) []byte {
transferAck := channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement()

merklePath := solo.GetPacketAcknowledgementPath(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
key, err := merklePath.GetKey(1) // index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
require.NoError(solo.t, err)
path := host.PacketAcknowledgementKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
signBytes := &solomachine.SignBytes{
Sequence: solo.Sequence,
Timestamp: solo.Time,
Diversifier: solo.Diversifier,
Path: key,
Path: path,
Data: channeltypes.CommitAcknowledgement(transferAck),
}

Expand All @@ -657,14 +643,12 @@ func (solo *Solomachine) GenerateAcknowledgementProof(packet channeltypes.Packet

// GenerateReceiptAbsenceProof generates a receipt absence proof for the provided packet.
func (solo *Solomachine) GenerateReceiptAbsenceProof(packet channeltypes.Packet) []byte {
merklePath := solo.GetPacketReceiptPath(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
key, err := merklePath.GetKey(1) // index 0 is the key for the IBC store in the multistore, index 1 is the key in the IBC store
require.NoError(solo.t, err)
path := host.PacketReceiptKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
signBytes := &solomachine.SignBytes{
Sequence: solo.Sequence,
Timestamp: solo.Time,
Diversifier: solo.Diversifier,
Path: key,
Path: path,
Data: nil,
}
return solo.GenerateProof(signBytes)
Expand Down
Loading