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 and simplify reverts of forwarding state #6574

Merged
merged 16 commits into from
Jun 13, 2024
Merged

Fix and simplify reverts of forwarding state #6574

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6305f3f
refactor: initial simplification attempt
srdtrk Jun 11, 2024
b13dae4
refactor: further organize
srdtrk Jun 11, 2024
f25c918
fix: all tests fixed
srdtrk Jun 11, 2024
492ecac
docs: improved godocs
srdtrk Jun 12, 2024
7770197
fix: logic and testing error
srdtrk Jun 12, 2024
bcf359d
style: self suggestion
srdtrk Jun 12, 2024
d867a5e
docs: suggestion
srdtrk Jun 12, 2024
a092154
docs: spellcheck
srdtrk Jun 12, 2024
0f4c22c
style: suggestions
srdtrk Jun 12, 2024
13270d8
style: renamed to revertForwardedPacket
srdtrk Jun 12, 2024
e5b9d78
style: suggestion
srdtrk Jun 12, 2024
e75c29e
docs: remove docs
srdtrk Jun 12, 2024
3123d67
docs: godoc suggestion
srdtrk Jun 13, 2024
cbad515
style: suggestion
srdtrk Jun 13, 2024
d3a60fc
Merge branch 'feat/ics20-v2-path-forwarding' into serdar/6558-refacto…
srdtrk Jun 13, 2024
45c7efa
docs: colin suggestions
srdtrk Jun 13, 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
96 changes: 96 additions & 0 deletions modules/apps/transfer/keeper/forward.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package keeper

import (
"errors"

errorsmod "cosmossdk.io/errors"
sdkmath "cosmossdk.io/math"

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

"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
)

// reverts the receive packet logic that occurs in the middle chain and asyncronously acknowledges the prevPacket
func (k Keeper) onForwardedPacketErrorAck(ctx sdk.Context, prevPacket channeltypes.Packet, failedPacketData types.FungibleTokenPacketDataV2) error {
// the forwarded packet has failed, thus the funds have been refunded to the intermediate address.
// we must revert the changes that came from successfully receiving the tokens on our chain
// before propogating the error acknowledgement back to original sender chain
if err := k.revertInFlightChanges(ctx, prevPacket, failedPacketData); err != nil {
return err
}

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

// asyncronously acknowledges the prevPacket
func (k Keeper) onForwardedPacketResultAck(ctx sdk.Context, prevPacket channeltypes.Packet) error {
forwardAck := channeltypes.NewResultAcknowledgement([]byte("forwarded packet succeeded"))
return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardAck)
}

// reverts the receive packet logic that occurs in the middle chain and asyncronously acknowledges the prevPacket
func (k Keeper) onForwardedPacketTimeout(ctx sdk.Context, prevPacket channeltypes.Packet, timeoutPacketData types.FungibleTokenPacketDataV2) error {
if err := k.revertInFlightChanges(ctx, prevPacket, timeoutPacketData); err != nil {
return err
}

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

// writes acknowledgement for a forwarded packet asyncronously
func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet channeltypes.Packet, ack channeltypes.Acknowledgement) error {
capability, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(packet.DestinationPort, packet.DestinationChannel))
if !ok {
return errorsmod.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability")
}

return k.ics4Wrapper.WriteAcknowledgement(ctx, capability, packet, ack)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we can structure the code in a different way to have fewer helper functions, since some of them are pretty thin and only called once, so not sure if they really add a lot of value... I was thinking of having one helper function like this (naming, as always is up for debate):

func (k Keeper) acknowledgeForwardedErrorAck(ctx sdk.Context, prevPacket channeltypes.Packet, nextPacketData types.FungibleTokenPacketDataV2, prevPacketAck channeltypes.Acknowledgement_Error) error {
  // the forwarded packet has failed, thus the funds have been refunded to the intermediate address.
  // we must revert the changes that came from successfully receiving the tokens on our chain
  // before propogating the error acknowledgement back to original sender chain
  if err := k.revertInFlightChanges(ctx, prevPacket, nextPacketData); err != nil {
    return err
  }

  capability, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(prevPacket.DestinationPort, prevPacket.DestinationChannel))
  if !ok {
    return errorsmod.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability")
  }

  return k.ics4Wrapper.WriteAcknowledgement(ctx, capability, prevPacket, prevPacketAck)
}

And then wherever is called, either on error ack or timeout, we do something like this (adjusting the message in the ack accordingly):

forwardAck := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet failed"))
return acknowledgeForwardedErrorAck(ctx, prevPacket, data, forwardAck)

What do people think?

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 I prefer this suggestion to reduce the number of small helper fns

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, also wouldn't mind in-lining these smaller fns in the OnAck/OnTimeout methods but see the logic of keeping most forwarding logic in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, it seems this would only get rid of acknowledgeForwardedPacket function. I'd rather not inline this because GetCapability hurts readability a lot in my opinion. I want to see GetCapability in as few places as possible. Could you reconsider this?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably handle success/error/timeout all in one function. Something like this:

func (k Keeper) acknowledgeForwardedAck(ctx sdk.Context, prevPacket channeltypes.Packet, nextPacketData types.FungibleTokenPacketDataV2, prevPacketAck channeltypes.Acknowledgement) error {
  if prevPacketAck.Response.(*channeltypes.Acknowledgement_Error) != nil {
    if err := k.revertInFlightChanges(ctx, prevPacket, nextPacketData); err != nil {
      return err
    }
  }

  capability, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(prevPacket.DestinationPort, prevPacket.DestinationChannel))
  if !ok {
    return errorsmod.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability")
  }

  return k.ics4Wrapper.WriteAcknowledgement(ctx, capability, prevPacket, prevPacketAck)
}

And then getting the capability is in a single place.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't address the timeout case. I am strongly against this suggestion because coupling acknowledgement, error handling, and reversion into a single function overcomplicates the code imo. I'd rather have multiple easy to understand helper functions.

Copy link
Contributor

@crodriguezvega crodriguezvega Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I might be wrong, but I think it would cover it. In OnTimeoutPacket You would do something like:

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

Copy link
Contributor

Choose a reason for hiding this comment

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

think its fine if we leave as is for now and kick this can for down the road. These are all private fns and can be changed after the fact without issue. Lets not block on it for time being?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok, it does handle it. Yeah, lets kick this down the road, I do like functions with simple responsibilities.

Although it may seem ok to merge them now, we might want to perform additional logic in the future depending on whether it was a timeout or not, such as emitting events. Then having these separated could be helpful.


// revertInFlightChanges reverts the logic of receive packet that occurs in the middle chains during a packet forwarding.
// If an error occurs further down the line, the state changes on this chain must be reverted before sending back the error
// acknowledgement to ensure atomic packet forwarding.
func (k Keeper) revertInFlightChanges(ctx sdk.Context, prevPacket channeltypes.Packet, failedPacketData types.FungibleTokenPacketDataV2) error {
/*
Recall that RecvPacket handles an incoming packet depending on the denom of the received funds:
1. If the funds are native, then the amount is sent to the receiver from the escrow.
2. If the funds are foreign, then a voucher token is minted.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth for clarity's sake to mention that the receiver is the forwarding account and that the vouchers are minted also on the forwarding account.

We revert it in this function by:
1. Sending funds back to escrow if the funds are native.
2. Burning voucher tokens if the funds are foreign
*/

intermediateSenderAddr := types.GetForwardAddress(prevPacket.DestinationPort, prevPacket.DestinationChannel)
escrow := types.GetEscrowAddress(prevPacket.DestinationPort, prevPacket.DestinationChannel)

// we can iterate over the received tokens of prevPacket by iterating over the sent tokens of failedPacketData
for _, token := range failedPacketData.Tokens {
// parse the transfer amount
transferAmount, ok := sdkmath.NewIntFromString(token.Amount)
if !ok {
return errorsmod.Wrapf(types.ErrInvalidAmount, "unable to parse transfer amount (%s) into math.Int", transferAmount)
}
coin := sdk.NewCoin(token.Denom.IBCDenom(), transferAmount)

// check if the packet we received was a native token
if token.Denom.IsNative() {
// then send it back to the escrow address
if err := k.escrowCoin(ctx, intermediateSenderAddr, escrow, coin); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good one to call escrowCoin here to update the total amount in escrow for the denom. 🥇

return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to wrap the errors returned from this function in some error like ErrRevert...?

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't really do any additional wrapping with errors returned from refundPacketTokens so I don't think it's strictly necessary to add an additional error type here

}

continue
}

// otherwise burn it
if err := k.burnCoin(ctx, intermediateSenderAddr, coin); err != nil {
return err
}
}
return nil
}
146 changes: 42 additions & 104 deletions modules/apps/transfer/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package keeper

import (
"errors"
"fmt"
"strings"

Expand Down Expand Up @@ -320,67 +319,47 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
// acknowledgement was a success then nothing occurs. If the acknowledgement failed,
// then the sender is refunded their tokens using the refundPacketToken function.
func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2, ack channeltypes.Acknowledgement) error {
prevPacket, found := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence)
if found {
channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(packet.SourcePort, packet.SourceChannel))
if !ok {
return errorsmod.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability")
}
prevPacket, isForwarded := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence)

switch ack.Response.(type) {
case *channeltypes.Acknowledgement_Result:
// the acknowledgement succeeded on the receiving chain so
// we write the asynchronous acknowledgement for the sender
// of the previous packet.
fungibleTokenPacketAcknowledgement := channeltypes.NewResultAcknowledgement([]byte("forwarded packet succeeded"))
return k.ics4Wrapper.WriteAcknowledgement(ctx, channelCap, prevPacket, fungibleTokenPacketAcknowledgement)
case *channeltypes.Acknowledgement_Error:
// the forwarded packet has failed, thus the funds have been refunded to the forwarding address.
// we must revert the changes that came from successfully receiving the tokens on our chain
// before propogating the error acknowledgement back to original sender chain
if err := k.revertInFlightChanges(ctx, packet, prevPacket, data); err != nil {
return err
}

fungibleTokenPacketAcknowledgement := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet failed"))
return k.ics4Wrapper.WriteAcknowledgement(ctx, channelCap, prevPacket, fungibleTokenPacketAcknowledgement)
default:
return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected one of [%T, %T], got %T", channeltypes.Acknowledgement_Result{}, channeltypes.Acknowledgement_Error{}, ack.Response)
}
} else {
switch ack.Response.(type) {
case *channeltypes.Acknowledgement_Result:
switch ack.Response.(type) {
case *channeltypes.Acknowledgement_Result:
if !isForwarded {
// the acknowledgement succeeded on the receiving chain so nothing
// needs to be executed and no error needs to be returned
return nil
case *channeltypes.Acknowledgement_Error:
return k.refundPacketTokens(ctx, packet, data)
default:
return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected one of [%T, %T], got %T", channeltypes.Acknowledgement_Result{}, channeltypes.Acknowledgement_Error{}, ack.Response)
}

return k.onForwardedPacketResultAck(ctx, prevPacket)
case *channeltypes.Acknowledgement_Error:
// We refund the tokens from the escrow address to the sender
if err := k.refundPacketTokens(ctx, packet, data); err != nil {
return err
}
if !isForwarded {
return nil
}

return k.onForwardedPacketErrorAck(ctx, prevPacket, data)
default:
return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected one of [%T, %T], got %T", channeltypes.Acknowledgement_Result{}, channeltypes.Acknowledgement_Error{}, ack.Response)
}
}

// OnTimeoutPacket either reverts the state changes executed in receive and send
// packet if the chain acted as a middle hop on a multihop transfer; or refunds
// the sender if the original packet sent was never received and has been timed out.
func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2) error {
prevPacket, found := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence)
if found {
channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(packet.SourcePort, packet.SourceChannel))
if !ok {
return errorsmod.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability")
}

if err := k.revertInFlightChanges(ctx, packet, prevPacket, data); err != nil {
return err
}
if err := k.refundPacketTokens(ctx, packet, data); err != nil {
return err
}

fungibleTokenPacketAcknowledgement := channeltypes.NewErrorAcknowledgement(fmt.Errorf("forwarded packet timed out"))
return k.ics4Wrapper.WriteAcknowledgement(ctx, channelCap, prevPacket, fungibleTokenPacketAcknowledgement)
prevPacket, isForwarded := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence)
if !isForwarded {
// return if not a forwarded packet
return nil
}

return k.refundPacketTokens(ctx, packet, data)
return k.onForwardedPacketTimeout(ctx, prevPacket, data)
}

// refundPacketTokens will unescrow and send back the tokens back to sender
Expand Down Expand Up @@ -429,63 +408,6 @@ func (k Keeper) refundPacketTokens(ctx sdk.Context, packet channeltypes.Packet,
return nil
}

// revertInFlightChanges reverts the logic of receive packet and send packet
// that occurs in the middle chains during a packet forwarding. If an error
// occurs further down the line, the state changes on this chain must be
// reverted before sending back the error acknowledgement to ensure atomic packet forwarding.
func (k Keeper) revertInFlightChanges(ctx sdk.Context, sentPacket channeltypes.Packet, receivedPacket channeltypes.Packet, sentPacketData types.FungibleTokenPacketDataV2) error {
forwardEscrow := types.GetEscrowAddress(sentPacket.SourcePort, sentPacket.SourceChannel)
reverseEscrow := types.GetEscrowAddress(receivedPacket.DestinationPort, receivedPacket.DestinationChannel)

// the token on our chain is the token in the sentPacket
for _, token := range sentPacketData.Tokens {
// parse the transfer amount
transferAmount, ok := sdkmath.NewIntFromString(token.Amount)
if !ok {
return errorsmod.Wrapf(types.ErrInvalidAmount, "unable to parse transfer amount (%s) into math.Int", transferAmount)
}
coin := sdk.NewCoin(token.Denom.IBCDenom(), transferAmount)

// check if the packet we sent out was sending as source or not
// if it is source, then we escrowed the outgoing tokens
if token.Denom.SenderChainIsSource(sentPacket.SourcePort, sentPacket.SourceChannel) {
// check if the packet we received was a source token for our chain
// check if here should be ReceiverChainIsSource
if token.Denom.SenderChainIsSource(receivedPacket.DestinationPort, receivedPacket.DestinationChannel) {
// receive sent tokens from the received escrow to the forward escrow account
// so we must send the tokens back from the forward escrow to the original received escrow account
return k.unescrowCoin(ctx, forwardEscrow, reverseEscrow, coin)
}

// receive minted vouchers and sent to the forward escrow account
// so we must remove the vouchers from the forward escrow account and burn them
if err := k.bankKeeper.BurnCoins(
ctx, types.ModuleName, sdk.NewCoins(coin),
); err != nil {
return err
}
} else { //nolint:gocritic
// in this case we burned the vouchers of the outgoing packets
// check if the packet we received was a source token for our chain
// in this case, the tokens were unescrowed from the reverse escrow account
if token.Denom.SenderChainIsSource(receivedPacket.DestinationPort, receivedPacket.DestinationChannel) {
// in this case we must mint the burned vouchers and send them back to the escrow account
if err := k.bankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(coin)); err != nil {
return err
}

if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, reverseEscrow, sdk.NewCoins(coin)); err != nil {
panic(fmt.Errorf("unable to send coins from module to account despite previously minting coins to module account: %v", err))
}
}

// if it wasn't a source token on receive, then we simply had minted vouchers and burned them in the receive.
// So no state changes were made, and thus no reversion is necessary
}
}
return nil
}

// escrowCoin will send the given coin from the provided sender to the escrow address. It will also
// update the total escrowed amount by adding the escrowed coin's amount to the current total escrow.
func (k Keeper) escrowCoin(ctx sdk.Context, sender, escrowAddress sdk.AccAddress, coin sdk.Coin) error {
Expand Down Expand Up @@ -572,3 +494,19 @@ func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string,

return packetDataBytes
}

// burnCoin only works with a module account in SDK v0.50, the next version of the SDK will allow burning coins from any account.
// This function will send coins from the account to the module account and then burn them.
// TODO: remove this function once we switch forwarding address to a module account (#6561)
func (k Keeper) burnCoin(ctx sdk.Context, account sdk.AccAddress, coin sdk.Coin) error {
coins := sdk.NewCoins(coin)
if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, account, types.ModuleName, coins); err != nil {
return err
}

if err := k.bankKeeper.BurnCoins(ctx, types.ModuleName, coins); err != nil {
return err
}

return nil
}
11 changes: 4 additions & 7 deletions modules/apps/transfer/keeper/relay_forwarding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() {

// Now we want to trigger C -> B -> A
// The coin we want to send out is exactly the one we received on C
// coin = sdk.NewCoin(denomTraceBC.IBCDenom(), amount)
coin = sdk.NewCoin(denomABC.IBCDenom(), amount)

sender = suite.chainC.SenderAccounts[0].SenderAccount
receiver = suite.chainA.SenderAccounts[0].SenderAccount // Receiver is the A chain account
Expand Down Expand Up @@ -572,13 +572,10 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() {
err = suite.chainB.GetSimApp().TransferKeeper.OnAcknowledgementPacket(suite.chainB.GetContext(), packetRecv, data, ack)
suite.Require().NoError(err)

// Check that Escrow B has been refunded amount
// NOTE This is failing. The revertInFlightsChanges sohuld mint back voucher to chainBescrow
// but this is not happening. It may be a problem related with how we're writing async acks.
//
// Check that Escrow B is empty the revertInFlightsChanges should burn the entire token since it is not native to Chain B.
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)
allEscrowChainB := suite.chainB.GetSimApp().TransferKeeper.GetAllTotalEscrowed(suite.chainB.GetContext())
suite.Require().Empty(allEscrowChainB)

denom = types.ExtractDenomFromPath(denomABC.Path())
data = types.NewFungibleTokenPacketDataV2(
Expand Down
Loading