From 5793c9f02b8850e68b1b2651f96bf5470c8ac189 Mon Sep 17 00:00:00 2001 From: Charly Date: Thu, 2 May 2024 15:54:47 +0200 Subject: [PATCH 1/3] add transfer authz code + tests --- .../transfer/types/transfer_authorization.go | 79 ++++++++++++------- .../types/transfer_authorization_test.go | 19 ++++- 2 files changed, 67 insertions(+), 31 deletions(-) diff --git a/modules/apps/transfer/types/transfer_authorization.go b/modules/apps/transfer/types/transfer_authorization.go index b38702f980a..ee8bde6826e 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)) @@ -44,42 +48,40 @@ func (a TransferAuthorization) Accept(ctx context.Context, msg proto.Message) (a 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] - if !isAllowedAddress(sdk.UnwrapSDKContext(ctx), msgTransfer.Receiver, allocation.AllowList) { - return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "not allowed receiver address for transfer") - } + // bool flag to see if we have updated any of the allocations + modificationMade := false - err := validateMemo(sdk.UnwrapSDKContext(ctx), msgTransfer.Memo, allocation.AllowedPacketData) - if err != nil { - return authz.AcceptResponse{}, err - } + if !isAllowedAddress(sdk.UnwrapSDKContext(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 + } + // 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) 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 - } + modificationMade = true + a.Allocations[index] = Allocation{ SourcePort: allocation.SourcePort, SourceChannel: allocation.SourceChannel, @@ -87,13 +89,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 !modificationMade { + 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 +225,13 @@ func validateMemo(ctx sdk.Context, memo string, allowedPacketDataList []string) func UnboundedSpendLimit() sdkmath.Int { return sdkmath.NewIntFromBigInt(maxUint256) } + +// getAllocationIndex ranges through 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..eb147a51980 100644 --- a/modules/apps/transfer/types/transfer_authorization_test.go +++ b/modules/apps/transfer/types/transfer_authorization_test.go @@ -69,13 +69,26 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() { { "success: with multiple allocations", func() { + + testMultiDenom := sdk.NewCoins(ibctesting.TestCoin, sdk.NewCoin("atom", sdkmath.NewInt(100)), sdk.NewCoin("osmo", sdkmath.NewInt(100))) + alloc := types.Allocation{ SourcePort: ibctesting.MockPort, SourceChannel: "channel-9", - SpendLimit: ibctesting.TestCoins, + SpendLimit: testMultiDenom, } transferAuthz.Allocations = append(transferAuthz.Allocations, alloc) + msgTransfer = types.NewMsgTransfer( + ibctesting.MockPort, + "channel-9", + sdk.NewCoins(ibctesting.TestCoin, sdk.NewCoin("atom", sdkmath.NewInt(100))), + suite.chainA.SenderAccount.GetAddress().String(), + ibctesting.TestAccAddress, + suite.chainB.GetTimeoutHeight(), + 0, + "", + ) }, func(res authz.AcceptResponse, err error) { suite.Require().NoError(err) @@ -86,8 +99,8 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() { updatedAuthz, ok := res.Updated.(*types.TransferAuthorization) suite.Require().True(ok) - // assert spent spendlimit is removed from the list - suite.Require().Len(updatedAuthz.Allocations, 1) + // assert spent spendlimits are removed from the list + suite.Require().Len(updatedAuthz.Allocations, 2) }, }, { From ebfd82713008a36d2aa5e053d4a1da4e1ce689aa Mon Sep 17 00:00:00 2001 From: Charly Date: Thu, 2 May 2024 15:57:00 +0200 Subject: [PATCH 2/3] linter --- modules/apps/transfer/types/transfer_authorization_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/apps/transfer/types/transfer_authorization_test.go b/modules/apps/transfer/types/transfer_authorization_test.go index eb147a51980..ad4fcd8df69 100644 --- a/modules/apps/transfer/types/transfer_authorization_test.go +++ b/modules/apps/transfer/types/transfer_authorization_test.go @@ -69,7 +69,6 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() { { "success: with multiple allocations", func() { - testMultiDenom := sdk.NewCoins(ibctesting.TestCoin, sdk.NewCoin("atom", sdkmath.NewInt(100)), sdk.NewCoin("osmo", sdkmath.NewInt(100))) alloc := types.Allocation{ From 38a42dba243594a574eb55bb7bde8234ace3435e Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Fri, 3 May 2024 12:23:52 +0200 Subject: [PATCH 3/3] address review comments --- .../transfer/types/transfer_authorization.go | 21 ++++++++++--------- .../types/transfer_authorization_test.go | 19 ++++++++++------- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/modules/apps/transfer/types/transfer_authorization.go b/modules/apps/transfer/types/transfer_authorization.go index ee8bde6826e..e5612c6442e 100644 --- a/modules/apps/transfer/types/transfer_authorization.go +++ b/modules/apps/transfer/types/transfer_authorization.go @@ -42,7 +42,7 @@ 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") @@ -54,19 +54,20 @@ func (a TransferAuthorization) Accept(ctx context.Context, msg proto.Message) (a } allocation := a.Allocations[index] + ctx := sdk.UnwrapSDKContext(goCtx) - // bool flag to see if we have updated any of the allocations - modificationMade := false - - if !isAllowedAddress(sdk.UnwrapSDKContext(ctx), msgTransfer.Receiver, allocation.AllowList) { + 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) + 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. @@ -75,12 +76,12 @@ func (a TransferAuthorization) Accept(ctx context.Context, msg proto.Message) (a 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 of token %s is more than spend limit", token.Denom) } - modificationMade = true + allocationModified = true a.Allocations[index] = Allocation{ SourcePort: allocation.SourcePort, @@ -100,7 +101,7 @@ func (a TransferAuthorization) Accept(ctx context.Context, msg proto.Message) (a return authz.AcceptResponse{Accept: true, Delete: true}, nil } - if !modificationMade { + if !allocationModified { return authz.AcceptResponse{Accept: true, Delete: false, Updated: nil}, nil } @@ -226,7 +227,7 @@ func UnboundedSpendLimit() sdkmath.Int { return sdkmath.NewIntFromBigInt(maxUint256) } -// getAllocationIndex ranges through set of allocations, and returns the index of the allocation if found. If not, returns -1. +// 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 { diff --git a/modules/apps/transfer/types/transfer_authorization_test.go b/modules/apps/transfer/types/transfer_authorization_test.go index ad4fcd8df69..063200c9d92 100644 --- a/modules/apps/transfer/types/transfer_authorization_test.go +++ b/modules/apps/transfer/types/transfer_authorization_test.go @@ -67,21 +67,26 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() { }, }, { - "success: with multiple allocations", + "success: with multiple allocations and multidenom transfer", func() { - testMultiDenom := sdk.NewCoins(ibctesting.TestCoin, sdk.NewCoin("atom", sdkmath.NewInt(100)), sdk.NewCoin("osmo", sdkmath.NewInt(100))) + coins := sdk.NewCoins( + ibctesting.TestCoin, + sdk.NewCoin("atom", sdkmath.NewInt(100)), + sdk.NewCoin("osmo", sdkmath.NewInt(100)), + ) - alloc := types.Allocation{ + allocation := types.Allocation{ SourcePort: ibctesting.MockPort, SourceChannel: "channel-9", - SpendLimit: testMultiDenom, + SpendLimit: coins, } - transferAuthz.Allocations = append(transferAuthz.Allocations, alloc) + transferAuthz.Allocations = append(transferAuthz.Allocations, allocation) + msgTransfer = types.NewMsgTransfer( ibctesting.MockPort, "channel-9", - sdk.NewCoins(ibctesting.TestCoin, sdk.NewCoin("atom", sdkmath.NewInt(100))), + coins, suite.chainA.SenderAccount.GetAddress().String(), ibctesting.TestAccAddress, suite.chainB.GetTimeoutHeight(), @@ -99,7 +104,7 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() { suite.Require().True(ok) // assert spent spendlimits are removed from the list - suite.Require().Len(updatedAuthz.Allocations, 2) + suite.Require().Len(updatedAuthz.Allocations, 1) }, }, {