From 2559e0e22583c2178dc15e29302bdf27bd5d8df8 Mon Sep 17 00:00:00 2001 From: Joe Bowman Date: Wed, 5 Jul 2023 12:55:16 +0100 Subject: [PATCH 1/4] fix: compute ibc/xxx denom instead of base denom for comparison --- .../keeper/ibc_packet_handlers.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/x/interchainstaking/keeper/ibc_packet_handlers.go b/x/interchainstaking/keeper/ibc_packet_handlers.go index 4d211f39d..6f8251a2b 100644 --- a/x/interchainstaking/keeper/ibc_packet_handlers.go +++ b/x/interchainstaking/keeper/ibc_packet_handlers.go @@ -315,7 +315,17 @@ func (k *Keeper) HandleMsgTransfer(ctx sdk.Context, msg sdk.Msg) error { zone, found := k.GetZoneForWithdrawalAccount(ctx, sMsg.Sender) - if found && receivedCoin.Denom != zone.BaseDenom { + // since SendPacket did not prefix the denomination, we must prefix denomination here + sourcePrefix := ibctransfertypes.GetDenomPrefix(sMsg.SourcePort, sMsg.SourceChannel) + // NOTE: sourcePrefix contains the trailing "/" + prefixedDenom := sourcePrefix + receivedCoin.Denom + + // construct the denomination trace from the full raw denomination + denomTrace := ibctransfertypes.ParseDenomTrace(prefixedDenom) + + receivedCoin.Denom = denomTrace.IBCDenom() + + if found && denomTrace.BaseDenom != zone.BaseDenom { feeAmount := sdk.NewDecFromInt(receivedCoin.Amount).Mul(k.GetCommissionRate(ctx)).TruncateInt() rewardCoin := receivedCoin.SubAmount(feeAmount) zoneAddress, err := addressutils.AccAddressFromBech32(zone.WithdrawalAddress.Address, "") @@ -723,6 +733,9 @@ func (k *Keeper) HandleUndelegate(ctx sdk.Context, msg sdk.Msg, completion time. } func (k *Keeper) HandleFailedBankSend(ctx sdk.Context, msg sdk.Msg, memo string) error { + // this might not be a unbond message - depends on the recipeint... + k.Logger(ctx).Error("failed msgSend", "msg", msg) + txHash, err := types.ParseTxMsgMemo(memo, types.MsgTypeUnbondSend) if err != nil { return err @@ -1174,6 +1187,10 @@ func DistributeRewardsFromWithdrawAccount(k *Keeper, ctx sdk.Context, args []byt } func (k *Keeper) prepareRewardsDistributionMsgs(zone types.Zone, rewards sdkmath.Int) sdk.Msg { + if !rewards.IsPositive() { + return &banktypes.MsgSend{} + } + return &banktypes.MsgSend{ FromAddress: zone.WithdrawalAddress.GetAddress(), ToAddress: zone.DelegationAddress.GetAddress(), From 23909f0986bdea4f0bbb8e300f9fd44683942267 Mon Sep 17 00:00:00 2001 From: Joe Bowman Date: Wed, 5 Jul 2023 21:29:26 +0100 Subject: [PATCH 2/4] fix tests --- utils/coins.go | 17 +++++++ .../keeper/ibc_packet_handlers.go | 9 +--- .../keeper/ibc_packet_handlers_test.go | 46 ++++++++++--------- 3 files changed, 43 insertions(+), 29 deletions(-) diff --git a/utils/coins.go b/utils/coins.go index 5755584a5..0a1d2253a 100644 --- a/utils/coins.go +++ b/utils/coins.go @@ -5,6 +5,7 @@ import ( 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" ) func DenomFromRequestKey(query []byte, accAddr sdk.AccAddress) (string, error) { @@ -24,3 +25,19 @@ 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("transfer", "channel-0") + // NOTE: sourcePrefix contains the trailing "/" + prefixedDenom := sourcePrefix + denom + + // construct the denomination trace from the full raw denomination + denomTrace := ibctransfertypes.ParseDenomTrace(prefixedDenom) + + return denomTrace +} diff --git a/x/interchainstaking/keeper/ibc_packet_handlers.go b/x/interchainstaking/keeper/ibc_packet_handlers.go index 6f8251a2b..77a9b2a9b 100644 --- a/x/interchainstaking/keeper/ibc_packet_handlers.go +++ b/x/interchainstaking/keeper/ibc_packet_handlers.go @@ -315,14 +315,7 @@ func (k *Keeper) HandleMsgTransfer(ctx sdk.Context, msg sdk.Msg) error { zone, found := k.GetZoneForWithdrawalAccount(ctx, sMsg.Sender) - // since SendPacket did not prefix the denomination, we must prefix denomination here - sourcePrefix := ibctransfertypes.GetDenomPrefix(sMsg.SourcePort, sMsg.SourceChannel) - // NOTE: sourcePrefix contains the trailing "/" - prefixedDenom := sourcePrefix + receivedCoin.Denom - - // construct the denomination trace from the full raw denomination - denomTrace := ibctransfertypes.ParseDenomTrace(prefixedDenom) - + denomTrace := utils.DeriveIbcDenomTrace(sMsg.SourcePort, sMsg.SourceChannel, receivedCoin.Denom) receivedCoin.Denom = denomTrace.IBCDenom() if found && denomTrace.BaseDenom != zone.BaseDenom { diff --git a/x/interchainstaking/keeper/ibc_packet_handlers_test.go b/x/interchainstaking/keeper/ibc_packet_handlers_test.go index 934d1a2a5..22e78925e 100644 --- a/x/interchainstaking/keeper/ibc_packet_handlers_test.go +++ b/x/interchainstaking/keeper/ibc_packet_handlers_test.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/require" "github.com/ingenuity-build/quicksilver/app" + "github.com/ingenuity-build/quicksilver/utils" "github.com/ingenuity-build/quicksilver/utils/addressutils" "github.com/ingenuity-build/quicksilver/utils/randomutils" icstypes "github.com/ingenuity-build/quicksilver/x/interchainstaking/types" @@ -29,27 +30,27 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() { tcs := []struct { name string amount sdk.Coin - fcAmount sdk.Coin - withdrawalAmount sdk.Coin + fcAmount math.Int + withdrawalAmount math.Int feeAmount *sdk.Dec }{ { name: "staking denom - all goes to fc", amount: sdk.NewCoin("uatom", math.NewInt(100)), - fcAmount: sdk.NewCoin("uatom", math.NewInt(100)), - withdrawalAmount: sdk.Coin{}, + fcAmount: math.NewInt(100), + withdrawalAmount: math.ZeroInt(), }, { name: "non staking denom - default (2.5%) to fc, remainder to withdrawal", amount: sdk.NewCoin("ujuno", math.NewInt(100)), - fcAmount: sdk.NewCoin("ujuno", math.NewInt(2)), - withdrawalAmount: sdk.NewCoin("ujuno", math.NewInt(98)), + fcAmount: math.NewInt(2), + withdrawalAmount: math.NewInt(98), }, { name: "non staking denom - non-default (9%) to fc, remainder to withdrawal", - amount: sdk.NewCoin("ibc/01234567890123456789012345678901", math.NewInt(100)), - fcAmount: sdk.NewCoin("ibc/01234567890123456789012345678901", math.NewInt(9)), - withdrawalAmount: sdk.NewCoin("ibc/01234567890123456789012345678901", math.NewInt(91)), + amount: sdk.NewCoin("uakt", math.NewInt(100)), + fcAmount: math.NewInt(9), + withdrawalAmount: math.NewInt(91), feeAmount: &nineDec, // 0.09 = 9% }, } @@ -61,7 +62,9 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() { quicksilver := suite.GetQuicksilverApp(suite.chainA) ctx := suite.chainA.GetContext() - err := quicksilver.BankKeeper.MintCoins(ctx, icstypes.ModuleName, sdk.NewCoins(tc.amount)) + ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", tc.amount.Denom) + + err := quicksilver.BankKeeper.MintCoins(ctx, icstypes.ModuleName, sdk.NewCoins(sdk.NewCoin(ibcDenom, tc.amount.Amount))) suite.Require().NoError(err) if tc.feeAmount != nil { @@ -99,12 +102,10 @@ func (suite *KeeperTestSuite) TestHandleMsgTransferGood() { suite.Require().Equal(sdk.Coins{}, txMaccBalance) // assert that fee collector module balance is the expected value - suite.Require().Contains(feeMaccBalance, tc.fcAmount) + suite.Require().Equal(feeMaccBalance.AmountOf(ibcDenom), tc.fcAmount) - if !tc.withdrawalAmount.IsNil() { - // assert that zone withdrawal address balance (local chain) is the expected value - suite.Require().Contains(wdAccountBalance, tc.withdrawalAmount) - } + // assert that zone withdrawal address balance (local chain) is the expected value + suite.Require().Equal(wdAccountBalance.AmountOf(ibcDenom), tc.withdrawalAmount) }) } } @@ -1451,7 +1452,8 @@ func (suite *KeeperTestSuite) Test_v045Callback() { { name: "msg response with some data", setStatements: func(ctx sdk.Context, quicksilver *app.Quicksilver) ([]sdk.Msg, []byte) { - err := quicksilver.BankKeeper.MintCoins(ctx, icstypes.ModuleName, sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(100)))) + ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", "denom") + err := quicksilver.BankKeeper.MintCoins(ctx, icstypes.ModuleName, sdk.NewCoins(sdk.NewCoin(ibcDenom, sdk.NewInt(100)))) suite.Require().NoError(err) sender := addressutils.GenerateAccAddressForTest() senderAddr, err := sdk.Bech32ifyAddressBytes("cosmos", sender) @@ -1478,8 +1480,8 @@ func (suite *KeeperTestSuite) Test_v045Callback() { feeMaccBalance2 := quicksilver.BankKeeper.GetAllBalances(ctx, feeMacc) // assert that ics module balance is now 100denom less than before HandleMsgTransfer() - - if txMaccBalance2.AmountOf("denom").Equal(sdk.ZeroInt()) && feeMaccBalance2.AmountOf("denom").Equal(sdk.NewInt(100)) { + ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", "denom") + if txMaccBalance2.AmountOf(ibcDenom).Equal(sdk.ZeroInt()) && feeMaccBalance2.AmountOf(ibcDenom).Equal(sdk.NewInt(100)) { return true } return false @@ -1571,7 +1573,9 @@ func (suite *KeeperTestSuite) Test_v046Callback() { { name: "msg response with some data", setStatements: func(ctx sdk.Context, quicksilver *app.Quicksilver) ([]sdk.Msg, *codectypes.Any) { - err := quicksilver.BankKeeper.MintCoins(ctx, icstypes.ModuleName, sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(100)))) + // since SendPacket did not prefix the denomination, we must prefix denomination here + ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", "denom") + err := quicksilver.BankKeeper.MintCoins(ctx, icstypes.ModuleName, sdk.NewCoins(sdk.NewCoin(ibcDenom, sdk.NewInt(100)))) suite.Require().NoError(err) sender := addressutils.GenerateAccAddressForTest() senderAddr, err := sdk.Bech32ifyAddressBytes("cosmos", sender) @@ -1599,8 +1603,8 @@ func (suite *KeeperTestSuite) Test_v046Callback() { feeMaccBalance2 := quicksilver.BankKeeper.GetAllBalances(ctx, feeMacc) // assert that ics module balance is now 100denom less than before HandleMsgTransfer() - - if txMaccBalance2.AmountOf("denom").Equal(sdk.ZeroInt()) && feeMaccBalance2.AmountOf("denom").Equal(sdk.NewInt(100)) { + ibcDenom := utils.DeriveIbcDenom("transfer", "channel-0", "denom") + if txMaccBalance2.AmountOf(ibcDenom).Equal(sdk.ZeroInt()) && feeMaccBalance2.AmountOf(ibcDenom).Equal(sdk.NewInt(100)) { return true } return false From 05dd842df58b0fe5d55e9d6b30b6738ffec755e3 Mon Sep 17 00:00:00 2001 From: Joe Bowman Date: Wed, 5 Jul 2023 21:39:28 +0100 Subject: [PATCH 3/4] fix hardcoded test values :facepalm: --- utils/coins.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/coins.go b/utils/coins.go index 0a1d2253a..7da6fa226 100644 --- a/utils/coins.go +++ b/utils/coins.go @@ -32,7 +32,7 @@ func DeriveIbcDenom(port, channel, denom string) string { func DeriveIbcDenomTrace(port, channel, denom string) ibctransfertypes.DenomTrace { // generate denomination prefix - sourcePrefix := ibctransfertypes.GetDenomPrefix("transfer", "channel-0") + sourcePrefix := ibctransfertypes.GetDenomPrefix(port, channel) // NOTE: sourcePrefix contains the trailing "/" prefixedDenom := sourcePrefix + denom From df51c22587393ca5c2771660a2e6f59d5c1eb84a Mon Sep 17 00:00:00 2001 From: Joe Bowman Date: Wed, 5 Jul 2023 21:40:18 +0100 Subject: [PATCH 4/4] fix: dont treat all failed MsgSend as unbond messages [QCK-509] --- .../keeper/ibc_packet_handlers.go | 42 +++++++++++++++---- x/interchainstaking/types/zones.go | 4 ++ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/x/interchainstaking/keeper/ibc_packet_handlers.go b/x/interchainstaking/keeper/ibc_packet_handlers.go index 77a9b2a9b..1ad9a0534 100644 --- a/x/interchainstaking/keeper/ibc_packet_handlers.go +++ b/x/interchainstaking/keeper/ibc_packet_handlers.go @@ -319,6 +319,7 @@ func (k *Keeper) HandleMsgTransfer(ctx sdk.Context, msg sdk.Msg) error { receivedCoin.Denom = denomTrace.IBCDenom() if found && denomTrace.BaseDenom != 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) zoneAddress, err := addressutils.AccAddressFromBech32(zone.WithdrawalAddress.Address, "") @@ -358,7 +359,7 @@ func (k *Keeper) HandleCompleteSend(ctx sdk.Context, msg sdk.Msg, memo string) e // checks here are specific to ensure future extensibility; switch { - case sMsg.FromAddress == zone.WithdrawalAddress.GetAddress(): + case zone.IsWithdrawalAddress(sMsg.FromAddress): // WithdrawalAddress (for rewards) only send to DelegationAddresses. // Target here is the DelegationAddresses. return k.handleRewardsDelegation(ctx, *zone, sMsg) @@ -726,18 +727,43 @@ func (k *Keeper) HandleUndelegate(ctx sdk.Context, msg sdk.Msg, completion time. } func (k *Keeper) HandleFailedBankSend(ctx sdk.Context, msg sdk.Msg, memo string) error { - // this might not be a unbond message - depends on the recipeint... - k.Logger(ctx).Error("failed msgSend", "msg", msg) + sMsg, ok := msg.(*banktypes.MsgSend) + if !ok { + err := errors.New("unable to cast source message to MsgSend") + k.Logger(ctx).Error(err.Error()) + return err + } - txHash, err := types.ParseTxMsgMemo(memo, types.MsgTypeUnbondSend) + // get zone + zone, err := k.GetZoneFromContext(ctx) if err != nil { + k.Logger(ctx).Error(err.Error()) return err } - // valid msg type bank send - sendMsg, ok := msg.(*banktypes.MsgSend) - if !ok { - return errors.New("unable to unmarshal MsgSend") + // checks here are specific to ensure future extensibility; + switch { + case zone.IsWithdrawalAddress(sMsg.FromAddress): + // MsgSend from Withdrawal account to delegate account was not completed. We can ignore this. + k.Logger(ctx).Error("MsgSend from withdrawal account to delegate account failed") + case zone.IsDelegateAddress(sMsg.FromAddress): + return k.HandleFailedUnbondSend(ctx, sMsg, memo) + case zone.IsDelegateAddress(sMsg.ToAddress) && zone.DepositAddress.Address == sMsg.FromAddress: + // MsgSend from deposit account to delegate account for deposit. + k.Logger(ctx).Error("MsgSend from deposit account to delegate account failed") + default: + err = errors.New("unexpected completed send") + k.Logger(ctx).Error(err.Error()) + return err + } + + return nil +} + +func (k *Keeper) HandleFailedUnbondSend(ctx sdk.Context, sendMsg *banktypes.MsgSend, memo string) error { + txHash, err := types.ParseTxMsgMemo(memo, types.MsgTypeUnbondSend) + if err != nil { + return err } // get chainID for the remote zone using msg addresses (ICA acc) diff --git a/x/interchainstaking/types/zones.go b/x/interchainstaking/types/zones.go index d33bb8886..923b90e47 100644 --- a/x/interchainstaking/types/zones.go +++ b/x/interchainstaking/types/zones.go @@ -26,6 +26,10 @@ func (z Zone) IsDelegateAddress(addr string) bool { return z.DelegationAddress != nil && z.DelegationAddress.Address == addr } +func (z Zone) IsWithdrawalAddress(addr string) bool { + return z.WithdrawalAddress != nil && z.WithdrawalAddress.Address == addr +} + func (z *Zone) GetDelegationAccount() (*ICAAccount, error) { if z.DelegationAddress == nil { return nil, fmt.Errorf("no delegation account set: %v", z)