Skip to content

Commit

Permalink
Fix MsgTransfer acknowledgement for non-native tokens (#1056)
Browse files Browse the repository at this point in the history 
* move transfer handler to ics middleware to properly handle multihop tokens

* gofumpt

* lint

* rename file, because we dont implement ics4

* remove debug log; add attribution in comment
  • Loading branch information
Joe Bowman authored Jan 21, 2024
1 parent 7971a28 commit 58e1d5c
Show file tree
Hide file tree
Showing 5 changed files with 189 additions and 143 deletions.
1 change: 1 addition & 0 deletions app/keepers/keepers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,7 @@ func (appKeepers *AppKeepers) InitKeepers(
packetforwardkeeper.DefaultForwardTransferPacketTimeoutTimestamp,
packetforwardkeeper.DefaultRefundTransferPacketTimeoutTimestamp,
)
ibcStack = interchainstaking.NewTransferMiddleware(ibcStack, appKeepers.InterchainstakingKeeper)

// Create static IBC router, add transfer route, then set and seal it
ibcRouter := porttypes.NewRouter()
Expand Down
31 changes: 16 additions & 15 deletions utils/coins.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ package utils

import (
"fmt"
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

ibctransfertypes "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types"
transfertypes "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types"
)

func DenomFromRequestKey(query []byte, accAddr sdk.AccAddress) (string, error) {
Expand All @@ -27,18 +28,18 @@ func DenomFromRequestKey(query []byte, accAddr sdk.AccAddress) (string, error) {
return denom, nil
}

func DeriveIbcDenom(port, channel, denom string) string {
return DeriveIbcDenomTrace(port, channel, denom).IBCDenom()
}

func DeriveIbcDenomTrace(port, channel, denom string) ibctransfertypes.DenomTrace {
// generate denomination prefix
sourcePrefix := ibctransfertypes.GetDenomPrefix(port, channel)
// NOTE: sourcePrefix contains the trailing "/"
prefixedDenom := sourcePrefix + denom

// construct the denomination trace from the full raw denomination
denomTrace := ibctransfertypes.ParseDenomTrace(prefixedDenom)

return denomTrace
// DeriveIbcDenom mirrors getDenomForThisChain from the packet-forward-middleware/v5, used under MIT License.
// See: https://github.com/strangelove-ventures/packet-forward-middleware/blob/86f045c12cc48ffc1f016ff122b89a9f6ac8ed63/router/ibc_middleware.go#L104
func DeriveIbcDenom(port, channel, counterpartyPort, counterpartyChannel, denom string) string {
counterpartyPrefix := transfertypes.GetDenomPrefix(counterpartyPort, counterpartyChannel)
if strings.HasPrefix(denom, counterpartyPrefix) {
unwoundDenom := denom[len(counterpartyPrefix):]
denomTrace := transfertypes.ParseDenomTrace(unwoundDenom)
if denomTrace.Path == "" {
return unwoundDenom
}
return denomTrace.IBCDenom()
}
prefixedDenom := transfertypes.GetDenomPrefix(port, channel) + denom
return transfertypes.ParseDenomTrace(prefixedDenom).IBCDenom()
}
54 changes: 12 additions & 42 deletions x/interchainstaking/keeper/ibc_packet_handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,21 +274,9 @@ func (k *Keeper) HandleAcknowledgement(ctx sdk.Context, packet channeltypes.Pack
return err
}
case "/ibc.applications.transfer.v1.MsgTransfer":
// this should be okay to fail; we'll pick it up next time around.
if !success {
return nil
}
response := ibctransfertypes.MsgTransferResponse{}
err = proto.Unmarshal(msgResponse, &response)
if err != nil {
k.Logger(ctx).Error("unable to unpack MsgTransfer response", "error", err)
return err
}
k.Logger(ctx).Debug("Received MsgTransfer acknowledgement; no action")
return nil

k.Logger(ctx).Info("MsgTranfer acknowledgement received")
if err := k.HandleMsgTransfer(ctx, msg.Msg); err != nil {
return err
}
default:
k.Logger(ctx).Error("unhandled acknowledgement packet", "type", reflect.TypeOf(msg.Msg).Name())
}
Expand All @@ -303,46 +291,28 @@ func (*Keeper) HandleTimeout(_ sdk.Context, _ channeltypes.Packet) error {

// ----------------------------------------------------------------

func (k *Keeper) HandleMsgTransfer(ctx sdk.Context, msg sdk.Msg) error {
func (k *Keeper) HandleMsgTransfer(ctx sdk.Context, msg ibctransfertypes.FungibleTokenPacketData, ibcDenom string) error {
k.Logger(ctx).Info("Received MsgTransfer acknowledgement")
// first, type assertion. we should have ibctransfertypes.MsgTransfer
sMsg, ok := msg.(*ibctransfertypes.MsgTransfer)
if !ok {
k.Logger(ctx).Error("unable to cast source message to MsgTransfer")
return errors.New("unable to cast source message to MsgTransfer")
}

// check if destination is interchainstaking module account (spoiler: it was)
if sMsg.Receiver != k.AccountKeeper.GetModuleAddress(types.ModuleName).String() {
if msg.Receiver != k.AccountKeeper.GetModuleAddress(types.ModuleName).String() {
k.Logger(ctx).Error("msgTransfer to unknown account!")
return errors.New("unexpected recipient")
}

receivedCoin := sMsg.Token

zone, found := k.GetZoneForWithdrawalAccount(ctx, sMsg.Sender)
if !found {
return fmt.Errorf("zone not found for withdrawal account %s", sMsg.Sender)
receivedAmount, ok := sdkmath.NewIntFromString(msg.Amount)
if !ok {
return fmt.Errorf("unable to marshal amount into math.Int: %s", msg.Amount)
}
receivedCoin := sdk.NewCoin(ibcDenom, receivedAmount)

var channel *channeltypes.IdentifiedChannel
k.IBCKeeper.ChannelKeeper.IterateChannels(ctx, func(ic channeltypes.IdentifiedChannel) bool {
if ic.Counterparty.ChannelId == sMsg.SourceChannel && ic.Counterparty.PortId == sMsg.SourcePort && len(ic.ConnectionHops) == 1 && ic.ConnectionHops[0] == zone.ConnectionId && ic.State == channeltypes.OPEN {
channel = &ic
return true
}
return false
})

if channel == nil {
k.Logger(ctx).Error("channel not found for the packet", "port", sMsg.SourcePort, "channel", sMsg.SourceChannel)
return errors.New("channel not found for the packet")
zone, found := k.GetZoneForWithdrawalAccount(ctx, msg.Sender)
if !found {
return fmt.Errorf("zone not found for withdrawal account %s", msg.Sender)
}

denomTrace := utils.DeriveIbcDenomTrace(channel.PortId, channel.ChannelId, receivedCoin.Denom)
receivedCoin.Denom = denomTrace.IBCDenom()

if found && denomTrace.BaseDenom != zone.BaseDenom {
if found && msg.Denom != zone.BaseDenom {
// k.Logger(ctx).Error("got withdrawal account and NOT staking denom", "rx", receivedCoin.Denom, "trace_base_denom", denomTrace.BaseDenom, "zone_base_denom", zone.BaseDenom)
feeAmount := sdk.NewDecFromInt(receivedCoin.Amount).Mul(k.GetCommissionRate(ctx)).TruncateInt()
rewardCoin := receivedCoin.SubAmount(feeAmount)
Expand Down
123 changes: 37 additions & 86 deletions x/interchainstaking/keeper/ibc_packet_handlers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() {
fcAmount: math.NewInt(100),
withdrawalAmount: math.ZeroInt(),
},
{
name: "ibc denom denom - all goes to fc",
amount: sdk.NewCoin("transfer/channel-569/untrn", math.NewInt(100)),
fcAmount: math.NewInt(2),
withdrawalAmount: math.NewInt(98),
},
{
name: "non staking denom - default (2.5%) to fc, remainder to withdrawal",
amount: sdk.NewCoin("ujuno", math.NewInt(100)),
Expand All @@ -81,7 +87,7 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() {
channel, cfound := quicksilver.InterchainstakingKeeper.IBCKeeper.ChannelKeeper.GetChannel(ctx, "transfer", "channel-0")
suite.True(cfound)

ibcDenom := utils.DeriveIbcDenom(channel.Counterparty.PortId, channel.Counterparty.ChannelId, tc.amount.Denom)
ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", channel.Counterparty.PortId, channel.Counterparty.ChannelId, tc.amount.Denom)

err := quicksilver.BankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(ibcDenom, tc.amount.Amount)))
suite.NoError(err)
Expand All @@ -101,14 +107,14 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() {
txMacc := quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName)
feeMacc := quicksilver.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName)

transferMsg := ibctransfertypes.MsgTransfer{
SourcePort: "transfer",
SourceChannel: "channel-0",
Token: tc.amount,
Sender: sender,
Receiver: quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName).String(),
transferPacket := ibctransfertypes.FungibleTokenPacketData{
Amount: tc.amount.Amount.String(),
Denom: tc.amount.Denom,
Sender: sender,
Receiver: quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName).String(),
}
suite.NoError(quicksilver.InterchainstakingKeeper.HandleMsgTransfer(ctx, &transferMsg))

suite.NoError(quicksilver.InterchainstakingKeeper.HandleMsgTransfer(ctx, transferPacket, utils.DeriveIbcDenom("transfer", "channel-0", channel.Counterparty.PortId, channel.Counterparty.ChannelId, tc.amount.Denom)))

txMaccBalance := quicksilver.BankKeeper.GetAllBalances(ctx, txMacc)
feeMaccBalance := quicksilver.BankKeeper.GetAllBalances(ctx, feeMacc)
Expand All @@ -128,15 +134,6 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() {
}
}

func TestHandleMsgTransferBadType(t *testing.T) {
quicksilver, ctx := app.GetAppWithContext(t, true)
err := quicksilver.BankKeeper.MintCoins(ctx, ibctransfertypes.ModuleName, sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(100))))
require.NoError(t, err)

transferMsg := banktypes.MsgSend{}
require.Error(t, quicksilver.InterchainstakingKeeper.HandleMsgTransfer(ctx, &transferMsg))
}

func TestHandleMsgTransferBadRecipient(t *testing.T) {
recipient := addressutils.GenerateAccAddressForTest()
quicksilver, ctx := app.GetAppWithContext(t, true)
Expand All @@ -145,14 +142,13 @@ func TestHandleMsgTransferBadRecipient(t *testing.T) {
senderAddr, err := sdk.Bech32ifyAddressBytes("cosmos", sender)
require.NoError(t, err)

transferMsg := ibctransfertypes.MsgTransfer{
SourcePort: "transfer",
SourceChannel: "channel-0",
Token: sdk.NewCoin("denom", sdk.NewInt(100)),
Sender: senderAddr,
Receiver: recipient.String(),
transferMsg := ibctransfertypes.FungibleTokenPacketData{
Denom: "denom",
Amount: "100",
Sender: senderAddr,
Receiver: recipient.String(),
}
require.Error(t, quicksilver.InterchainstakingKeeper.HandleMsgTransfer(ctx, &transferMsg))
require.Error(t, quicksilver.InterchainstakingKeeper.HandleMsgTransfer(ctx, transferMsg, "raa"))
}

func (suite *KeeperTestSuite) TestHandleQueuedUnbondings() {
Expand Down Expand Up @@ -1806,44 +1802,21 @@ func (suite *KeeperTestSuite) Test_v045Callback() {
if !found {
suite.Fail("unable to retrieve zone for test")
}
sender := zone.WithdrawalAddress.Address

quicksilver.InterchainstakingKeeper.IBCKeeper.ChannelKeeper.SetChannel(ctx, "transfer", "channel-0", TestChannel)
val := quicksilver.InterchainstakingKeeper.GetValidatorAddresses(ctx, zone.ChainId)[0]

ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", zone.BaseDenom)
err := quicksilver.BankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(ibcDenom, sdk.NewInt(100))))
suite.NoError(err)

transferMsg := ibctransfertypes.MsgTransfer{
SourcePort: "transfer",
SourceChannel: "channel-0",
Token: sdk.NewCoin(zone.BaseDenom, sdk.NewInt(100)),
Sender: sender,
Receiver: quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName).String(),
}
response := ibctransfertypes.MsgTransferResponse{
Sequence: 1,
sendMsg := stakingtypes.MsgDelegate{
Amount: sdk.NewCoin(zone.BaseDenom, sdk.NewInt(100)),
DelegatorAddress: zone.PerformanceAddress.Address,
ValidatorAddress: val,
}
response := stakingtypes.MsgDelegateResponse{}

respBytes := icatypes.ModuleCdc.MustMarshal(&response)
return []sdk.Msg{&transferMsg}, respBytes
return []sdk.Msg{&sendMsg}, respBytes
},
assertStatements: func(ctx sdk.Context, quicksilver *app.Quicksilver) bool {
zone, found := quicksilver.InterchainstakingKeeper.GetZone(ctx, suite.chainB.ChainID)
if !found {
suite.Fail("unable to retrieve zone for test")
}

txMacc := quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName)
feeMacc := quicksilver.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName)
txMaccBalance2 := quicksilver.BankKeeper.GetAllBalances(ctx, txMacc)
feeMaccBalance2 := quicksilver.BankKeeper.GetAllBalances(ctx, feeMacc)

ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", zone.BaseDenom)
if txMaccBalance2.AmountOf(ibcDenom).Equal(sdk.ZeroInt()) && feeMaccBalance2.AmountOf(ibcDenom).Equal(sdk.NewInt(100)) {
return true
}
return false
return true
},
},
{
Expand Down Expand Up @@ -1936,45 +1909,22 @@ func (suite *KeeperTestSuite) Test_v046Callback() {
if !found {
suite.Fail("unable to retrieve zone for test")
}
sender := zone.WithdrawalAddress.Address

quicksilver.InterchainstakingKeeper.IBCKeeper.ChannelKeeper.SetChannel(ctx, "transfer", "channel-0", TestChannel)
val := quicksilver.InterchainstakingKeeper.GetValidatorAddresses(ctx, zone.ChainId)[0]

ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", zone.BaseDenom)
err := quicksilver.BankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(ibcDenom, sdk.NewInt(100))))
suite.NoError(err)

transferMsg := ibctransfertypes.MsgTransfer{
SourcePort: "transfer",
SourceChannel: "channel-0",
Token: sdk.NewCoin(zone.BaseDenom, sdk.NewInt(100)),
Sender: sender,
Receiver: quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName).String(),
}
response := ibctransfertypes.MsgTransferResponse{
Sequence: 1,
sendMsg := stakingtypes.MsgDelegate{
Amount: sdk.NewCoin(zone.BaseDenom, sdk.NewInt(100)),
DelegatorAddress: zone.PerformanceAddress.Address,
ValidatorAddress: val,
}
response := stakingtypes.MsgDelegateResponse{}

anyResponse, err := codectypes.NewAnyWithValue(&response)
suite.NoError(err)
return []sdk.Msg{&transferMsg}, anyResponse
return []sdk.Msg{&sendMsg}, anyResponse
},
assertStatements: func(ctx sdk.Context, quicksilver *app.Quicksilver) bool {
zone, found := quicksilver.InterchainstakingKeeper.GetZone(ctx, suite.chainB.ChainID)
if !found {
suite.Fail("unable to retrieve zone for test")
}

txMacc := quicksilver.AccountKeeper.GetModuleAddress(types.ModuleName)
feeMacc := quicksilver.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName)
txMaccBalance2 := quicksilver.BankKeeper.GetAllBalances(ctx, txMacc)
feeMaccBalance2 := quicksilver.BankKeeper.GetAllBalances(ctx, feeMacc)

ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", zone.BaseDenom)
if txMaccBalance2.AmountOf(ibcDenom).Equal(sdk.ZeroInt()) && feeMaccBalance2.AmountOf(ibcDenom).Equal(sdk.NewInt(100)) {
return true
}
return false
return true
},
},
{
Expand Down Expand Up @@ -2047,6 +1997,7 @@ func (suite *KeeperTestSuite) Test_v046Callback() {
Data: packetBytes,
}

ctx = ctx.WithContext(context.WithValue(ctx.Context(), utils.ContextKey("connectionID"), "connection-0"))
suite.NoError(quicksilver.InterchainstakingKeeper.HandleAcknowledgement(ctx, packet, icatypes.ModuleCdc.MustMarshalJSON(&acknowledgement)))

suite.True(test.assertStatements(ctx, quicksilver))
Expand Down
Loading

0 comments on commit 58e1d5c

Please sign in to comment.