From 9bbfa1a5f1cfae05889ca832f22271352d49d429 Mon Sep 17 00:00:00 2001 From: Charly Date: Mon, 6 May 2024 09:51:01 +0200 Subject: [PATCH] imp: update transfer authz implementation to account for multi denom transfers (#6252) * add transfer authz code + tests * linter * address review comments --------- Co-authored-by: Carlos Rodriguez --- .../transfer/types/transfer_authorization.go | 84 ++++++++++++------- .../types/transfer_authorization_test.go | 27 ++++-- 2 files changed, 76 insertions(+), 35 deletions(-) diff --git a/modules/apps/transfer/types/transfer_authorization.go b/modules/apps/transfer/types/transfer_authorization.go index b38702f980a..e5612c6442e 100644 --- a/modules/apps/transfer/types/transfer_authorization.go +++ b/modules/apps/transfer/types/transfer_authorization.go @@ -22,6 +22,10 @@ import ( var _ authz.Authorization = (*TransferAuthorization)(nil) +const ( + allocationNotFound = -1 +) + // maxUint256 is the maximum value for a 256 bit unsigned integer. var maxUint256 = new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 256), big.NewInt(1)) @@ -38,48 +42,47 @@ func (TransferAuthorization) MsgTypeURL() string { } // Accept implements Authorization.Accept. -func (a TransferAuthorization) Accept(ctx context.Context, msg proto.Message) (authz.AcceptResponse, error) { +func (a TransferAuthorization) Accept(goCtx context.Context, msg proto.Message) (authz.AcceptResponse, error) { msgTransfer, ok := msg.(*MsgTransfer) if !ok { return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidType, "type mismatch") } - // TODO: replace with correct usage in https://github.com/cosmos/ibc-go/issues/5802 - token := msgTransfer.GetTokens()[0] + index := getAllocationIndex(*msgTransfer, a.Allocations) + if index == allocationNotFound { + return authz.AcceptResponse{}, errorsmod.Wrapf(ibcerrors.ErrNotFound, "requested port and channel allocation does not exist") + } - for index, allocation := range a.Allocations { - if !(allocation.SourceChannel == msgTransfer.SourceChannel && allocation.SourcePort == msgTransfer.SourcePort) { - continue - } + allocation := a.Allocations[index] + ctx := sdk.UnwrapSDKContext(goCtx) - if !isAllowedAddress(sdk.UnwrapSDKContext(ctx), msgTransfer.Receiver, allocation.AllowList) { - return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "not allowed receiver address for transfer") - } + if !isAllowedAddress(ctx, msgTransfer.Receiver, allocation.AllowList) { + return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "not allowed receiver address for transfer") + } - err := validateMemo(sdk.UnwrapSDKContext(ctx), msgTransfer.Memo, allocation.AllowedPacketData) - if err != nil { - return authz.AcceptResponse{}, err - } + err := validateMemo(ctx, msgTransfer.Memo, allocation.AllowedPacketData) + if err != nil { + return authz.AcceptResponse{}, err + } + // bool flag to see if we have updated any of the allocations + allocationModified := false + + // update spend limit for each token in the MsgTransfer + for _, token := range msgTransfer.GetTokens() { // If the spend limit is set to the MaxUint256 sentinel value, do not subtract the amount from the spend limit. + // if there is no unlimited spend, then we need to subtract the amount from the spend limit to get the limit left if allocation.SpendLimit.AmountOf(token.Denom).Equal(UnboundedSpendLimit()) { - return authz.AcceptResponse{Accept: true, Delete: false, Updated: nil}, nil + continue } - limitLeft, isNegative := allocation.SpendLimit.SafeSub(token) + limitLeft, isNegative := a.Allocations[index].SpendLimit.SafeSub(token) if isNegative { - return authz.AcceptResponse{}, errorsmod.Wrapf(ibcerrors.ErrInsufficientFunds, "requested amount is more than spend limit") + return authz.AcceptResponse{}, errorsmod.Wrapf(ibcerrors.ErrInsufficientFunds, "requested amount of token %s is more than spend limit", token.Denom) } - if limitLeft.IsZero() { - a.Allocations = append(a.Allocations[:index], a.Allocations[index+1:]...) - if len(a.Allocations) == 0 { - return authz.AcceptResponse{Accept: true, Delete: true}, nil - } - return authz.AcceptResponse{Accept: true, Delete: false, Updated: &TransferAuthorization{ - Allocations: a.Allocations, - }}, nil - } + allocationModified = true + a.Allocations[index] = Allocation{ SourcePort: allocation.SourcePort, SourceChannel: allocation.SourceChannel, @@ -87,13 +90,24 @@ func (a TransferAuthorization) Accept(ctx context.Context, msg proto.Message) (a AllowList: allocation.AllowList, AllowedPacketData: allocation.AllowedPacketData, } + } + + // if the spend limit is zero of the associated allocation then we delete it. + if a.Allocations[index].SpendLimit.IsZero() { + a.Allocations = append(a.Allocations[:index], a.Allocations[index+1:]...) + } + + if len(a.Allocations) == 0 { + return authz.AcceptResponse{Accept: true, Delete: true}, nil + } - return authz.AcceptResponse{Accept: true, Delete: false, Updated: &TransferAuthorization{ - Allocations: a.Allocations, - }}, nil + if !allocationModified { + return authz.AcceptResponse{Accept: true, Delete: false, Updated: nil}, nil } - return authz.AcceptResponse{}, errorsmod.Wrapf(ibcerrors.ErrNotFound, "requested port and channel allocation does not exist") + return authz.AcceptResponse{Accept: true, Delete: false, Updated: &TransferAuthorization{ + Allocations: a.Allocations, + }}, nil } // ValidateBasic implements Authorization.ValidateBasic. @@ -212,3 +226,13 @@ func validateMemo(ctx sdk.Context, memo string, allowedPacketDataList []string) func UnboundedSpendLimit() sdkmath.Int { return sdkmath.NewIntFromBigInt(maxUint256) } + +// getAllocationIndex ranges through a set of allocations, and returns the index of the allocation if found. If not, returns -1. +func getAllocationIndex(msg MsgTransfer, allocations []Allocation) int { + for index, allocation := range allocations { + if allocation.SourceChannel == msg.SourceChannel && allocation.SourcePort == msg.SourcePort { + return index + } + } + return allocationNotFound +} diff --git a/modules/apps/transfer/types/transfer_authorization_test.go b/modules/apps/transfer/types/transfer_authorization_test.go index d86c396d335..063200c9d92 100644 --- a/modules/apps/transfer/types/transfer_authorization_test.go +++ b/modules/apps/transfer/types/transfer_authorization_test.go @@ -67,15 +67,32 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() { }, }, { - "success: with multiple allocations", + "success: with multiple allocations and multidenom transfer", func() { - alloc := types.Allocation{ + coins := sdk.NewCoins( + ibctesting.TestCoin, + sdk.NewCoin("atom", sdkmath.NewInt(100)), + sdk.NewCoin("osmo", sdkmath.NewInt(100)), + ) + + allocation := types.Allocation{ SourcePort: ibctesting.MockPort, SourceChannel: "channel-9", - SpendLimit: ibctesting.TestCoins, + SpendLimit: coins, } - transferAuthz.Allocations = append(transferAuthz.Allocations, alloc) + transferAuthz.Allocations = append(transferAuthz.Allocations, allocation) + + msgTransfer = types.NewMsgTransfer( + ibctesting.MockPort, + "channel-9", + coins, + suite.chainA.SenderAccount.GetAddress().String(), + ibctesting.TestAccAddress, + suite.chainB.GetTimeoutHeight(), + 0, + "", + ) }, func(res authz.AcceptResponse, err error) { suite.Require().NoError(err) @@ -86,7 +103,7 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() { updatedAuthz, ok := res.Updated.(*types.TransferAuthorization) suite.Require().True(ok) - // assert spent spendlimit is removed from the list + // assert spent spendlimits are removed from the list suite.Require().Len(updatedAuthz.Allocations, 1) }, },