From 3d2d4c90931b299e4908b60f1e99c7cd0d87dca2 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Tue, 21 Jan 2025 16:24:51 +0800 Subject: [PATCH 1/3] imp: simplified BuildMerklePath --- modules/core/04-channel/v2/keeper/packet.go | 10 ++---- modules/core/04-channel/v2/types/merkle.go | 27 +++++---------- .../core/04-channel/v2/types/merkle_test.go | 34 +++++++++++-------- 3 files changed, 31 insertions(+), 40 deletions(-) diff --git a/modules/core/04-channel/v2/keeper/packet.go b/modules/core/04-channel/v2/keeper/packet.go index a0254d9ca64..f760f3fe5b5 100644 --- a/modules/core/04-channel/v2/keeper/packet.go +++ b/modules/core/04-channel/v2/keeper/packet.go @@ -12,7 +12,6 @@ import ( clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types" "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types" - commitmenttypes "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types" hostv2 "github.com/cosmos/ibc-go/v9/modules/core/24-host/v2" "github.com/cosmos/ibc-go/v9/modules/core/exported" ) @@ -121,8 +120,7 @@ func (k *Keeper) recvPacket( } path := hostv2.PacketCommitmentKey(packet.SourceClient, packet.Sequence) - merklePrefix := commitmenttypes.NewMerklePath(counterparty.MerklePrefix...) - merklePath := types.BuildMerklePath(merklePrefix, path) + merklePath := types.BuildMerklePath(counterparty.MerklePrefix, path) commitment := types.CommitPacket(packet) @@ -219,8 +217,7 @@ func (k *Keeper) acknowledgePacket(ctx context.Context, packet types.Packet, ack } path := hostv2.PacketAcknowledgementKey(packet.DestinationClient, packet.Sequence) - merklePrefix := commitmenttypes.NewMerklePath(counterparty.MerklePrefix...) - merklePath := types.BuildMerklePath(merklePrefix, path) + merklePath := types.BuildMerklePath(counterparty.MerklePrefix, path) if err := k.ClientKeeper.VerifyMembership( ctx, @@ -298,8 +295,7 @@ func (k *Keeper) timeoutPacket( // verify packet receipt absence path := hostv2.PacketReceiptKey(packet.DestinationClient, packet.Sequence) - merklePrefix := commitmenttypes.NewMerklePath(counterparty.MerklePrefix...) - merklePath := types.BuildMerklePath(merklePrefix, path) + merklePath := types.BuildMerklePath(counterparty.MerklePrefix, path) if err := k.ClientKeeper.VerifyNonMembership( ctx, diff --git a/modules/core/04-channel/v2/types/merkle.go b/modules/core/04-channel/v2/types/merkle.go index 59be153c960..cc99fad0369 100644 --- a/modules/core/04-channel/v2/types/merkle.go +++ b/modules/core/04-channel/v2/types/merkle.go @@ -1,31 +1,20 @@ package types import ( - commitmenttypes "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types" commitmenttypesv2 "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" ) // BuildMerklePath takes the merkle path prefix and an ICS24 path // and builds a new path by appending the ICS24 path to the last element of the merkle path prefix. -func BuildMerklePath(prefix commitmenttypesv2.MerklePath, path []byte) commitmenttypesv2.MerklePath { - if prefix.Empty() { - return commitmenttypes.NewMerklePath(path) +func BuildMerklePath(prefix [][]byte, path []byte) commitmenttypesv2.MerklePath { + prefixLength := len(prefix) + if prefixLength == 0 { + panic("cannot build merkle path with empty prefix") } - // avoid mutating the provided prefix - prefixKeys := make([][]byte, len(prefix.KeyPath)) - copy(prefixKeys, prefix.KeyPath) - - lastElement := prefixKeys[len(prefixKeys)-1] + // copy prefix to avoid modifying the original slice + prefix = append([][]byte(nil), prefix...) // append path to last element - newLastElement := cloneAppend(lastElement, path) - prefixKeys[len(prefixKeys)-1] = newLastElement - return commitmenttypes.NewMerklePath(prefixKeys...) -} - -func cloneAppend(bz []byte, tail []byte) []byte { - res := make([]byte, len(bz)+len(tail)) - copy(res, bz) - copy(res[len(bz):], tail) - return res + prefix[prefixLength-1] = append(prefix[prefixLength-1], path...) + return commitmenttypesv2.NewMerklePath(prefix...) } diff --git a/modules/core/04-channel/v2/types/merkle_test.go b/modules/core/04-channel/v2/types/merkle_test.go index df239bc17d7..6f5886981b7 100644 --- a/modules/core/04-channel/v2/types/merkle_test.go +++ b/modules/core/04-channel/v2/types/merkle_test.go @@ -2,7 +2,6 @@ package types_test import ( "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types" - commitmenttypes "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types" commitmenttypesv2 "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types/v2" host "github.com/cosmos/ibc-go/v9/modules/core/24-host" ibctesting "github.com/cosmos/ibc-go/v9/testing" @@ -12,14 +11,15 @@ func (s *TypesTestSuite) TestBuildMerklePath() { path := ibctesting.NewPath(s.chainA, s.chainB) path.SetupV2() - prefixPath := commitmenttypes.NewMerklePath([]byte("ibc"), []byte("")) + prefixPath := [][]byte{[]byte("ibc"), []byte("")} packetCommitmentKey := host.PacketCommitmentKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, 1) testCases := []struct { - name string - prefix commitmenttypesv2.MerklePath - path []byte - expPath commitmenttypesv2.MerklePath + name string + prefix [][]byte + path []byte + expPath commitmenttypesv2.MerklePath + expPanics bool }{ { name: "standard ibc path", @@ -29,21 +29,21 @@ func (s *TypesTestSuite) TestBuildMerklePath() { }, { name: "non-empty last element prefix path", - prefix: commitmenttypes.NewMerklePath([]byte("ibc"), []byte("abc")), + prefix: [][]byte{[]byte("ibc"), []byte("abc")}, path: packetCommitmentKey, expPath: commitmenttypesv2.NewMerklePath([]byte("ibc"), append([]byte("abc"), packetCommitmentKey...)), }, { name: "many elements in prefix path", - prefix: commitmenttypes.NewMerklePath([]byte("ibc"), []byte("a"), []byte("b"), []byte("c"), []byte("d")), + prefix: [][]byte{[]byte("ibc"), []byte("a"), []byte("b"), []byte("c"), []byte("d")}, path: packetCommitmentKey, expPath: commitmenttypesv2.NewMerklePath([]byte("ibc"), []byte("a"), []byte("b"), []byte("c"), append([]byte("d"), packetCommitmentKey...)), }, { - name: "empty prefix", - prefix: commitmenttypesv2.MerklePath{}, - path: packetCommitmentKey, - expPath: commitmenttypesv2.NewMerklePath(packetCommitmentKey), + name: "empty prefix", + prefix: [][]byte{}, + path: packetCommitmentKey, + expPanics: true, }, { name: "empty path", @@ -57,8 +57,14 @@ func (s *TypesTestSuite) TestBuildMerklePath() { tc := tc s.Run(tc.name, func() { - merklePath := types.BuildMerklePath(tc.prefix, tc.path) - s.Require().Equal(tc.expPath, merklePath) + if tc.expPanics { + s.Require().PanicsWithValue("cannot build merkle path with empty prefix", func() { + _ = types.BuildMerklePath(tc.prefix, tc.path) + }) + } else { + merklePath := types.BuildMerklePath(tc.prefix, tc.path) + s.Require().Equal(tc.expPath, merklePath) + } }) } } From c0721348527315be1509637ef8d6453e4ceba1c6 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Tue, 21 Jan 2025 18:53:21 +0800 Subject: [PATCH 2/3] imp: review item --- modules/core/04-channel/v2/types/merkle.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/core/04-channel/v2/types/merkle.go b/modules/core/04-channel/v2/types/merkle.go index cc99fad0369..30d6cbfc000 100644 --- a/modules/core/04-channel/v2/types/merkle.go +++ b/modules/core/04-channel/v2/types/merkle.go @@ -13,8 +13,8 @@ func BuildMerklePath(prefix [][]byte, path []byte) commitmenttypesv2.MerklePath } // copy prefix to avoid modifying the original slice - prefix = append([][]byte(nil), prefix...) + fullPath := append([][]byte(nil), prefix...) // append path to last element - prefix[prefixLength-1] = append(prefix[prefixLength-1], path...) - return commitmenttypesv2.NewMerklePath(prefix...) + fullPath[prefixLength-1] = append(fullPath[prefixLength-1], path...) + return commitmenttypesv2.NewMerklePath(fullPath...) } From 94444631a2d5af681f186076c7a55eba68e6c786 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Tue, 21 Jan 2025 18:58:22 +0800 Subject: [PATCH 3/3] test: improved test structure --- modules/core/04-channel/v2/types/merkle_test.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/modules/core/04-channel/v2/types/merkle_test.go b/modules/core/04-channel/v2/types/merkle_test.go index 6f5886981b7..0eac1e68428 100644 --- a/modules/core/04-channel/v2/types/merkle_test.go +++ b/modules/core/04-channel/v2/types/merkle_test.go @@ -13,13 +13,14 @@ func (s *TypesTestSuite) TestBuildMerklePath() { prefixPath := [][]byte{[]byte("ibc"), []byte("")} packetCommitmentKey := host.PacketCommitmentKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, 1) + emptyPrefixPanicMsg := "cannot build merkle path with empty prefix" testCases := []struct { name string prefix [][]byte path []byte expPath commitmenttypesv2.MerklePath - expPanics bool + expPanics *string }{ { name: "standard ibc path", @@ -43,7 +44,7 @@ func (s *TypesTestSuite) TestBuildMerklePath() { name: "empty prefix", prefix: [][]byte{}, path: packetCommitmentKey, - expPanics: true, + expPanics: &emptyPrefixPanicMsg, }, { name: "empty path", @@ -57,13 +58,13 @@ func (s *TypesTestSuite) TestBuildMerklePath() { tc := tc s.Run(tc.name, func() { - if tc.expPanics { - s.Require().PanicsWithValue("cannot build merkle path with empty prefix", func() { - _ = types.BuildMerklePath(tc.prefix, tc.path) - }) - } else { + if tc.expPanics == nil { merklePath := types.BuildMerklePath(tc.prefix, tc.path) s.Require().Equal(tc.expPath, merklePath) + } else { + s.Require().PanicsWithValue(*tc.expPanics, func() { + _ = types.BuildMerklePath(tc.prefix, tc.path) + }) } }) }