Skip to content

feat(transfer): use registered error code for error acks in token forwarding #6648

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

Merged
gjermundgaraba merged 7 commits into from
Jun 19, 2024
Merged

feat(transfer): use registered error code for error acks in token forwarding #6648

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aec52a4
Add typed errors to packet forwarding
gjermundgaraba Jun 14, 2024
2d1575b
Merge branch 'feat/ics20-v2-path-forwarding' into gjermund/6586-use-r…
gjermundgaraba Jun 19, 2024
a35cb6d
Use forward errors in tests
gjermundgaraba Jun 19, 2024
69155ad
Make ack construction consistent
gjermundgaraba Jun 19, 2024
5965c83
Merge branch 'feat/ics20-v2-path-forwarding' into gjermund/6586-use-r…
gjermundgaraba Jun 19, 2024
aeb5162
Merge branch 'feat/ics20-v2-path-forwarding' into gjermund/6586-use-r…
gjermundgaraba Jun 19, 2024
ab3149a
Merge branch 'feat/ics20-v2-path-forwarding' into gjermund/6586-use-r…
gjermundgaraba Jun 19, 2024
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
6 changes: 2 additions & 4 deletions modules/apps/transfer/keeper/forwarding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package keeper

import (
"errors"

errorsmod "cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -63,7 +61,7 @@ func (k Keeper) ackForwardPacketError(ctx sdk.Context, prevPacket channeltypes.P
return err
}

forwardAck := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet failed"))
forwardAck := channeltypes.NewErrorAcknowledgement(types.ErrForwardedPacketFailed)
return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardAck)
}

Expand All @@ -73,7 +71,7 @@ func (k Keeper) ackForwardPacketTimeout(ctx sdk.Context, prevPacket channeltypes
return err
}

forwardAck := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet timed out"))
forwardAck := channeltypes.NewErrorAcknowledgement(types.ErrForwardedPacketTimedOut)
return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardAck)
}

Expand Down
84 changes: 38 additions & 46 deletions modules/apps/transfer/keeper/relay_forwarding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
package keeper_test
Copy link
Copy Markdown
Contributor

@bznein bznein Jun 19, 2024

Choose a reason for hiding this comment

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

The changes on this file seem to mix the purpose of the PR (use registered error code) with some other refactoring. WDYT about moving them into a separate PR?

Copy link
Copy Markdown
Contributor Author

@gjermundgaraba gjermundgaraba Jun 19, 2024

Choose a reason for hiding this comment

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

It does to some extent, but a good chunk of the refactor was to make use of the added error code in a test. Some of it I could've potentially skipped, but it would only be a relatively small portion. I did expect the refactor to be slightly smaller, though. If there are concerns about the refactor itself, I could take it out and not test that error message. Wdyt?

Copy link
Copy Markdown
Contributor

@bznein bznein Jun 19, 2024

Choose a reason for hiding this comment

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

It's fine by me to let it like this. But since my understanding of the test code is still fuzzy, I'll just refrain from LGTMing it :)

Copy link
Copy Markdown
Contributor

@colin-axner colin-axner Jun 19, 2024

Choose a reason for hiding this comment

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

In general, less diffs are always appreciated. But this felt like an okay amount.


import (
"errors"
"fmt"
"time"

sdkmath "cosmossdk.io/math"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v8/modules/apps/transfer/internal"
"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
Expand Down Expand Up @@ -403,14 +401,14 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() {
suite.Require().NoError(err) // message committed

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

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

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

Expand Down Expand Up @@ -447,14 +445,14 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() {
suite.Require().NoError(err) // message committed

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

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

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

Expand Down Expand Up @@ -503,22 +501,22 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() {
suite.Require().Equal(sdkmath.NewInt(0), postCoinOnC.Amount, "Vouchers have not been burned")

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

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

result, err = path2.EndpointA.RecvPacketWithResult(packet)
result, err = path2.EndpointA.RecvPacketWithResult(packetFromCtoB)
suite.Require().NoError(err)
suite.Require().NotNil(result)

// We have successfully received the packet on B and forwarded it to A.
// Lets try to retrieve it in order to save it
forwardedPacket, found := suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID, packet.Sequence)
forwardedPacket, found := suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID, packetFromCtoB.Sequence)
suite.Require().True(found)
suite.Require().Equal(packet, forwardedPacket)
suite.Require().Equal(packetFromCtoB, forwardedPacket)

// Voucher have been burned on chain B
coin = sdk.NewCoin(denomAB.IBCDenom(), amount)
Expand All @@ -530,68 +528,62 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() {
// of denom

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

// manipulate escrow account for denom on chain A
coin = sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(99))
suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), coin)
totalEscrowChainA = suite.chainA.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainA.GetContext(), coin.GetDenom())
suite.Require().Equal(sdkmath.NewInt(99), totalEscrowChainA.Amount)
// turn off receive on chain A to trigger an error
suite.chainA.GetSimApp().TransferKeeper.SetParams(suite.chainA.GetContext(), types.Params{
SendEnabled: true,
ReceiveEnabled: false,
})

err = path1.EndpointA.UpdateClient()
suite.Require().NoError(err)
// suite.Require().Equal(packet, forwardedPacket)

result, err = path1.EndpointA.RecvPacketWithResult(packet)
suite.Require().Error(err)
suite.Require().Nil(result)
// In theory now an error ack should have been written on chain A
// NOW WE HAVE TO SEND ACK TO B, PROPAGTE ACK TO C, CHECK FINAL RESULTS
result, err = path1.EndpointA.RecvPacketWithResult(packetFromBtoA)
suite.Require().NoError(err)

// Reconstruct packet data
data, err := internal.UnmarshalPacketData(packet.Data, types.V2)
// An error ack has been written on chainA
// Now we need to propagate it back to chainB and chainC
packetSequenceOnA, err := ibctesting.ParsePacketSequenceFromEvents(result.Events)
suite.Require().NoError(err)

errorAckOnA := channeltypes.NewErrorAcknowledgement(types.ErrReceiveDisabled)
errorAckCommitmentOnA := channeltypes.CommitAcknowledgement(errorAckOnA.Acknowledgement())
ackOnC, found := suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.GetPacketAcknowledgement(suite.chainA.GetContext(), path1.EndpointA.ChannelConfig.PortID, path1.EndpointA.ChannelID, packetSequenceOnA)
suite.Require().True(found)
suite.Require().Equal(errorAckCommitmentOnA, ackOnC)

err = path1.EndpointB.UpdateClient()
suite.Require().NoError(err)
ack := channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed packet transfer"))

// err = path1.EndpointA.AcknowledgePacket(packetRecv, ack.Acknowledgement())
err = suite.chainB.GetSimApp().TransferKeeper.OnAcknowledgementPacket(suite.chainB.GetContext(), packet, data, ack)
err = path1.EndpointB.AcknowledgePacket(packetFromBtoA, errorAckOnA.Acknowledgement())
suite.Require().NoError(err)

// Check that Escrow B has been refunded amount
coin = sdk.NewCoin(denomAB.IBCDenom(), amount)
totalEscrowChainB = suite.chainB.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainB.GetContext(), coin.GetDenom())
suite.Require().Equal(sdkmath.NewInt(100), totalEscrowChainB.Amount)

denom := types.ExtractDenomFromPath(denomABC.Path())
data = types.NewFungibleTokenPacketDataV2(
[]types.Token{
{
Denom: denom,
Amount: amount.String(),
},
}, suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String(), suite.chainA.SenderAccounts[0].SenderAccount.GetAddress().String(), "", types.Forwarding{})
// suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String() This should be forward account of B
packet = channeltypes.NewPacket(data.GetBytes(), 3, path2.EndpointB.ChannelConfig.PortID, path2.EndpointB.ChannelID, path2.EndpointA.ChannelConfig.PortID, path2.EndpointA.ChannelID, clienttypes.NewHeight(1, 100), 0)

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

errorAckOnB := channeltypes.NewErrorAcknowledgement(types.ErrForwardedPacketFailed)
errorAckCommitmentOnB := channeltypes.CommitAcknowledgement(errorAckOnB.Acknowledgement())
ackOnB := suite.chainB.GetAcknowledgement(packetFromCtoB)
suite.Require().Equal(errorAckCommitmentOnB, ackOnB)

// Check the status of account on chain C before executing ack.
coin = sdk.NewCoin(denomABC.IBCDenom(), amount)
postCoinOnC = suite.chainC.GetSimApp().BankKeeper.GetBalance(suite.chainC.GetContext(), suite.chainC.SenderAccounts[0].SenderAccount.GetAddress(), coin.GetDenom())
suite.Require().Equal(sdkmath.NewInt(0), postCoinOnC.Amount, "Final Hop balance has been refunded before Ack execution")

// Execute ack
err = suite.chainC.GetSimApp().TransferKeeper.OnAcknowledgementPacket(suite.chainC.GetContext(), packet, data, ack)
// err = path2.EndpointB.AcknowledgePacket(packet, ack.Acknowledgement())
err = path2.EndpointB.AcknowledgePacket(packetFromCtoB, errorAckOnB.Acknowledgement())
suite.Require().NoError(err)

// Check that everythig has been reverted
// Check that everything has been reverted
//
// Check the vouchers transfer/channel-1/transfer/channel-0/denom have been refunded on C
coin = sdk.NewCoin(denomABC.IBCDenom(), amount)
Expand Down Expand Up @@ -748,7 +740,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() {
suite.Require().True(found, "chainB does not have an ack")

// And that this ack is of the type we expect (Error due to time out)
ack := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet timed out"))
ack := channeltypes.NewErrorAcknowledgement(types.ErrForwardedPacketTimedOut)
ackbytes := channeltypes.CommitAcknowledgement(ack.Acknowledgement())
suite.Require().Equal(ackbytes, storedAck)

Expand Down
2 changes: 2 additions & 0 deletions modules/apps/transfer/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,6 @@ var (
ErrInvalidAuthorization = errorsmod.Register(ModuleName, 10, "invalid transfer authorization")
ErrInvalidMemo = errorsmod.Register(ModuleName, 11, "invalid memo")
ErrInvalidForwarding = errorsmod.Register(ModuleName, 12, "invalid token forwarding")
ErrForwardedPacketTimedOut = errorsmod.Register(ModuleName, 13, "forwarded packet timed out")
ErrForwardedPacketFailed = errorsmod.Register(ModuleName, 14, "forwarded packet failed")
)
13 changes: 13 additions & 0 deletions testing/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,19 @@ func ParseProposalIDFromEvents(events []abci.Event) (uint64, error) {
return 0, fmt.Errorf("proposalID event attribute not found")
}

// ParsePacketSequenceFromEvents parses events emitted from MsgRecvPacket and returns the packet sequence
func ParsePacketSequenceFromEvents(events []abci.Event) (uint64, error) {
for _, event := range events {
for _, attribute := range event.Attributes {
if attribute.Key == "packet_sequence" {
return strconv.ParseUint(attribute.Value, 10, 64)
}
Comment on lines +180 to +183
Copy link
Copy Markdown
Contributor

@bznein bznein Jun 19, 2024

Choose a reason for hiding this comment

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

[nit] Depending on how much you (dis)like nested loops, you could also go with something like:

	for _, event := range events {
		if idx := slices.IndexFunc(event.Attributes,func(a abci.EventAttribute) bool {return a.Key == "packet_sequence"}); idx != -1 {
				return strconv.ParseUint(event.Attributes[idx].Value, 10, 64)
			}
	}

	return 0, fmt.Errorf("packet sequence event attribute not found")

Copy link
Copy Markdown
Contributor Author

@gjermundgaraba gjermundgaraba Jun 19, 2024

Choose a reason for hiding this comment

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

True. To be honest I copied the function above which is very similar, but I could change to this. I'm not a fan of nested loops, but I am not sure if this reads a lot better, though?

Copy link
Copy Markdown
Contributor

@bznein bznein Jun 19, 2024

Choose a reason for hiding this comment

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

I tend to prefer structures like this but this is personal preference. If it is used more than once (I see also at least 3 more usages at the top of the file) we could extract it to an helper fn:

func attributeByKey(key string, attributes []abci.EventAttribute) (Attribute, bool) {
    if idx :=  slices.IndexFunc(event.Attributes,func(a abci.EventAttribute) bool {return a.Key == key}); idx == -1 {
             return abci.Attribute{}, false
     }
     return attributes[idx], true
}

So that we can later just do:

for _, event := range events {
		if attribute, found := attributeByKey("packet_sequence", event.Attributes); found {
				return strconv.ParseUint(attribute.Value, 10, 64)
			}
	}

Which I think makes things easier to read.
(now that I think about it, even if we do not use the slices.IndexFunc method, I think we could still wrap the loop over event.Attributes in an helper function)

Copy link
Copy Markdown
Contributor Author

@gjermundgaraba gjermundgaraba Jun 19, 2024

Choose a reason for hiding this comment

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

That does make sense! I'll look into that :)

Copy link
Copy Markdown
Contributor

@colin-axner colin-axner Jun 19, 2024

Choose a reason for hiding this comment

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

I'd be interested in a generic fn for parsing event attributes so we don't need to add a helper fn for each attribute, but maybe lets take this discussion to a separate issue?

Copy link
Copy Markdown
Contributor Author

@gjermundgaraba gjermundgaraba Jun 19, 2024

Choose a reason for hiding this comment

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

#6651

}
}

return 0, fmt.Errorf("packet sequence event attribute not found")
}

// AssertEvents asserts that expected events are present in the actual events.
func AssertEvents(
suite *testifysuite.Suite,
Expand Down