Skip to content

Commit

Permalink
fix: compute ibc/xxx denom instead of base denom for comparison (#486)
Browse files Browse the repository at this point in the history 
* fix: compute ibc/xxx denom instead of base denom for comparison

* fix tests

* fix hardcoded test values 🤦

* fix: dont treat all failed MsgSend as unbond messages [QCK-509]
  • Loading branch information
Joe Bowman authored Jul 6, 2023
1 parent 85ed48b commit bf94d79
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 28 deletions.
17 changes: 17 additions & 0 deletions utils/coins.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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(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
}
50 changes: 43 additions & 7 deletions x/interchainstaking/keeper/ibc_packet_handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,11 @@ func (k *Keeper) HandleMsgTransfer(ctx sdk.Context, msg sdk.Msg) error {

zone, found := k.GetZoneForWithdrawalAccount(ctx, sMsg.Sender)

if found && receivedCoin.Denom != zone.BaseDenom {
denomTrace := utils.DeriveIbcDenomTrace(sMsg.SourcePort, sMsg.SourceChannel, receivedCoin.Denom)
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, "")
Expand Down Expand Up @@ -355,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)
Expand Down Expand Up @@ -723,15 +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 {
txHash, err := types.ParseTxMsgMemo(memo, types.MsgTypeUnbondSend)
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
}

// 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)
Expand Down Expand Up @@ -1174,6 +1206,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(),
Expand Down
46 changes: 25 additions & 21 deletions x/interchainstaking/keeper/ibc_packet_handlers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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%
},
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
})
}
}
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions x/interchainstaking/types/zones.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit bf94d79

Please sign in to comment.