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

test: forwarding test that verifies forwarded memo #6707

140 changes: 140 additions & 0 deletions modules/apps/transfer/keeper/relay_forwarding_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,146 @@ func (suite *KeeperTestSuite) TestSuccessfulForward() {
suite.Require().NoError(err)
}

// TestSuccessfulPathForwarding tests a successful transfer from A to C through B with a memo that should arrive at C.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like godoc needs updating to align :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I had already set it for merging, will update this and other review comments you made 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

func (suite *KeeperTestSuite) TestSuccessfulForwardWithMemo() {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto reviews in #6709 I suppose 😅

/*
Given the following topology:
Copy link
Member

Choose a reason for hiding this comment

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

Nice comment, could optionally move to godoc on test func as well. But fine if it stays here 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy-past detected 🙈
I'll sort the godocs for both cases :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chain A (channel 0) -> (channel-0) chain B (channel-1) -> (channel-0) chain C
stake transfer/channel-0/stake transfer/channel-0/transfer/channel-0/stake
We want to trigger:
1. A sends B over channel-0.
2. Receive on B.
2.1 B sends C over channel-1
3. Receive on C.
At this point we want to assert:
A: escrowA = amount,stake
B: escrowB = amount,transfer/channel-0/denom
C: finalReceiver = amount,transfer/channel-0/transfer/channel-0/denom,memo
*/

amount := sdkmath.NewInt(100)
testMemo := "test forwarding memo"

pathAtoB, pathBtoC := suite.setupForwardingPaths()

coinOnA := ibctesting.TestCoin
sender := suite.chainA.SenderAccounts[0].SenderAccount
receiver := suite.chainC.SenderAccounts[0].SenderAccount
forwarding := types.NewForwarding(false, types.Hop{
PortId: pathBtoC.EndpointA.ChannelConfig.PortID,
ChannelId: pathBtoC.EndpointA.ChannelID,
})

transferMsg := types.NewMsgTransfer(
pathAtoB.EndpointA.ChannelConfig.PortID,
pathAtoB.EndpointA.ChannelID,
sdk.NewCoins(coinOnA),
sender.GetAddress().String(),
receiver.GetAddress().String(),
clienttypes.ZeroHeight(),
suite.chainA.GetTimeoutTimestamp(),
testMemo,
forwarding,
)

result, err := suite.chainA.SendMsgs(transferMsg)
suite.Require().NoError(err) // message committed

// parse the packet from result events and recv packet on chainB
packetFromAtoB, err := ibctesting.ParsePacketFromEvents(result.Events)
suite.Require().NoError(err)
suite.Require().NotNil(packetFromAtoB)

err = pathAtoB.EndpointB.UpdateClient()
suite.Require().NoError(err)

// Check that the memo is stored correctly in the packet sent from A
var tokenPacketOnA types.FungibleTokenPacketDataV2
err = suite.chainA.Codec.UnmarshalJSON(packetFromAtoB.Data, &tokenPacketOnA)
suite.Require().NoError(err)
suite.Require().Equal("", tokenPacketOnA.Memo)
suite.Require().Equal(testMemo, tokenPacketOnA.Forwarding.DestinationMemo)

result, err = pathAtoB.EndpointB.RecvPacketWithResult(packetFromAtoB)
suite.Require().NoError(err)
suite.Require().NotNil(result)

// Check that Escrow A has amount
suite.assertAmountOnChain(suite.chainA, escrow, amount, sdk.DefaultBondDenom)

// denom path: transfer/channel-0
denom := types.NewDenom(sdk.DefaultBondDenom, types.NewTrace(pathAtoB.EndpointB.ChannelConfig.PortID, pathAtoB.EndpointB.ChannelID))
suite.assertAmountOnChain(suite.chainB, escrow, amount, denom.IBCDenom())

packetFromBtoC, err := ibctesting.ParsePacketFromEvents(result.Events)
suite.Require().NoError(err)
suite.Require().NotNil(packetFromBtoC)

// Check that the memo is stored correctly in the packet sent from B
var tokenPacketOnB types.FungibleTokenPacketDataV2
err = suite.chainB.Codec.UnmarshalJSON(packetFromBtoC.Data, &tokenPacketOnB)
suite.Require().NoError(err)
suite.Require().Equal(testMemo, tokenPacketOnB.Memo)
suite.Require().Equal("", tokenPacketOnB.Forwarding.DestinationMemo)

err = pathBtoC.EndpointA.UpdateClient()
suite.Require().NoError(err)

err = pathBtoC.EndpointB.UpdateClient()
suite.Require().NoError(err)

// B should have stored the forwarded packet.
_, found := suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), packetFromBtoC.SourcePort, packetFromBtoC.SourceChannel, packetFromBtoC.Sequence)
suite.Require().True(found, "Chain B should have stored the forwarded packet")

result, err = pathBtoC.EndpointB.RecvPacketWithResult(packetFromBtoC)
suite.Require().NoError(err)
suite.Require().NotNil(result)

packetOnC, err := ibctesting.ParseRecvPacketFromEvents(result.Events)
suite.Require().NoError(err)
suite.Require().NotNil(packetOnC)

// Check that the memo is stored directly in the memo field on C
var tokenPacketOnC types.FungibleTokenPacketDataV2
err = suite.chainC.Codec.UnmarshalJSON(packetOnC.Data, &tokenPacketOnC)
suite.Require().NoError(err)
suite.Require().Equal("", tokenPacketOnC.Forwarding.DestinationMemo)
suite.Require().Equal(testMemo, tokenPacketOnC.Memo)

// transfer/channel-0/transfer/channel-0/denom
// Check that the final receiver has received the expected tokens.
denomABC := types.NewDenom(sdk.DefaultBondDenom, types.NewTrace(pathBtoC.EndpointB.ChannelConfig.PortID, pathBtoC.EndpointB.ChannelID), types.NewTrace(pathAtoB.EndpointB.ChannelConfig.PortID, pathAtoB.EndpointB.ChannelID))
// Check that the final receiver has received the expected tokens.
suite.assertAmountOnChain(suite.chainC, balance, amount, denomABC.IBCDenom())

successAck := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
successAckBz := channeltypes.CommitAcknowledgement(successAck.Acknowledgement())
ackOnC := suite.chainC.GetAcknowledgement(packetFromBtoC)
suite.Require().Equal(successAckBz, ackOnC)

// Ack back to B
err = pathBtoC.EndpointB.UpdateClient()
suite.Require().NoError(err)

err = pathBtoC.EndpointA.AcknowledgePacket(packetFromBtoC, successAck.Acknowledgement())
suite.Require().NoError(err)

ackOnB := suite.chainB.GetAcknowledgement(packetFromAtoB)
suite.Require().Equal(successAckBz, ackOnB)

// B should now have deleted the forwarded packet.
_, found = suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), packetFromBtoC.SourcePort, packetFromBtoC.SourceChannel, packetFromBtoC.Sequence)
suite.Require().False(found, "Chain B should have deleted the forwarded packet")

// Ack back to A
err = pathAtoB.EndpointA.UpdateClient()
suite.Require().NoError(err)

err = pathAtoB.EndpointA.AcknowledgePacket(packetFromAtoB, successAck.Acknowledgement())
suite.Require().NoError(err)
}

// TestSuccessfulPathForwarding tests that a packet is successfully forwarded with a non-Cosmos account address.
// The test stops before verifying the final receive, because we don't have a non-cosmos chain to test with.
func (suite *KeeperTestSuite) TestSuccessfulForwardWithNonCosmosAccAddress() {
Expand Down
19 changes: 15 additions & 4 deletions testing/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,21 @@ func ParseChannelIDFromEvents(events []abci.Event) (string, error) {
}

// ParsePacketFromEvents parses events emitted from a MsgRecvPacket and returns
// the first packet found.
// the first EventTypeSendPacket packet found.
// Returns an error if no packet is found.
func ParsePacketFromEvents(events []abci.Event) (channeltypes.Packet, error) {
packets, err := ParsePacketsFromEvents(events)
packets, err := ParsePacketsFromEvents(channeltypes.EventTypeSendPacket, events)
if err != nil {
return channeltypes.Packet{}, err
}
return packets[0], nil
}

// ParseRecvPacketFromEvents parses events emitted from a MsgRecvPacket and returns
// the first EventTypeRecvPacket packet found.
// Returns an error if no packet is found.
func ParseRecvPacketFromEvents(events []abci.Event) (channeltypes.Packet, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just a single function with event type as argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, just because of the huge PR it would be. I can refactor in a separate PR, perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds like a plan to me

packets, err := ParsePacketsFromEvents(channeltypes.EventTypeRecvPacket, events)
if err != nil {
return channeltypes.Packet{}, err
}
Expand All @@ -75,13 +86,13 @@ func ParsePacketFromEvents(events []abci.Event) (channeltypes.Packet, error) {
// ParsePacketsFromEvents parses events emitted from a MsgRecvPacket and returns
// all the packets found.
// Returns an error if no packet is found.
func ParsePacketsFromEvents(events []abci.Event) ([]channeltypes.Packet, error) {
func ParsePacketsFromEvents(eventType string, events []abci.Event) ([]channeltypes.Packet, error) {
ferr := func(err error) ([]channeltypes.Packet, error) {
return nil, fmt.Errorf("ibctesting.ParsePacketsFromEvents: %w", err)
}
var packets []channeltypes.Packet
for _, ev := range events {
if ev.Type == channeltypes.EventTypeSendPacket {
Copy link
Member

Choose a reason for hiding this comment

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

couldn't we have just done logical or || on event type channeltypes.EventTypeSendPacket || channeltypes.EventTypeRecvPacket?

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the result.Events from a recv packet which does a send internally include send packet events which are parsable here too?

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 tried that, but it failed on some tests, so decided to keep functionality as it was.
I guess it picked up a received packet first, and it had not been modified as expected yet (for forwarding, I would assume).

if ev.Type == eventType {
var packet channeltypes.Packet
for _, attr := range ev.Attributes {
switch attr.Key {
Expand Down
2 changes: 1 addition & 1 deletion testing/events_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ func TestParsePacketsFromEvents(t *testing.T) {
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
allPackets, err := ibctesting.ParsePacketsFromEvents(tc.events)
allPackets, err := ibctesting.ParsePacketsFromEvents(channeltypes.EventTypeSendPacket, tc.events)

if tc.expectedError == "" {
require.NoError(t, err)
Expand Down
Loading