Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imp: represent unlimited approvals with MaxUint256 value #3454

Merged
merged 31 commits into from
May 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
060cdf6
imp: represent unlimited approvals with a nil value
Vvaradinov Apr 13, 2023
201ea0f
CHANGELOG
Vvaradinov Apr 13, 2023
10a8df5
Update CHANGELOG.md
crodriguezvega Apr 14, 2023
98d9d3c
fix: updated unlimited spending limit to be represented with the MaxI…
Vvaradinov Apr 25, 2023
01fa2d5
Merge branch 'Vvaradinov/ics20-unlimited-authz' of https://github.com…
Vvaradinov Apr 25, 2023
87daf70
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
Vvaradinov Apr 25, 2023
bdb9fc6
Update modules/apps/transfer/types/transfer_authorization.go
Vvaradinov Apr 26, 2023
05719c1
Update CHANGELOG.md
Vvaradinov Apr 26, 2023
7dc4640
fix: lint
Vvaradinov Apr 26, 2023
b33dba6
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
fedekunze Apr 30, 2023
633c254
Update modules/apps/transfer/types/transfer_authorization.go
fedekunze Apr 30, 2023
f80ada0
fix: update failing test and add test case suggested in review
Vvaradinov May 4, 2023
ff05f63
fix: moved isAllowedAddress check before coin loop
Vvaradinov May 4, 2023
005a537
fix: use maxUint256 case so it aligns with what's being passed from t…
Vvaradinov May 8, 2023
21cd1f6
refactor transfer authz to remove coins iteration in favour of explic…
damiannolan May 9, 2023
b3abb10
make format
damiannolan May 9, 2023
edc031b
Update modules/apps/transfer/types/transfer_authorization.go
Vvaradinov May 9, 2023
a55efb7
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
damiannolan May 9, 2023
05cfa35
fix: add the Authorization to Updated.
Vvaradinov May 9, 2023
e8b2fed
Merge branch 'Vvaradinov/ics20-unlimited-authz' of https://github.com…
Vvaradinov May 9, 2023
7914b62
moving allowlist check to before spend limit logic
damiannolan May 9, 2023
1d4260d
Apply suggestions from code review
fedekunze May 10, 2023
c85fbb3
fix: add comment to spend limit check
Vvaradinov May 10, 2023
715a082
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
Vvaradinov May 11, 2023
30293f6
review feedback
crodriguezvega May 12, 2023
b7f81c6
Merge pull request #1 from cosmos/Vvaradinov/ics20-unlimited-authz
Vvaradinov May 15, 2023
464bccd
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
Vvaradinov May 15, 2023
4eb96d3
Update modules/apps/transfer/types/transfer_authorization.go
Vvaradinov May 15, 2023
cc9cf3f
Update docs/apps/transfer/authorizations.md
damiannolan May 15, 2023
3552971
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
crodriguezvega May 15, 2023
aab8b18
fix: changed to new sentinel value name
Vvaradinov May 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Improvements

* (tests) [\#3138](https://github.com/cosmos/ibc-go/pull/3138) Support benchmarks and fuzz tests through `testing.TB`.
* (apps/transfer) [\#3454](https://github.com/cosmos/ibc-go/pull/3454) Support transfer authorization unlimited spending when the max `uint256` value is provided as limit.

### Features

Expand Down
4 changes: 2 additions & 2 deletions docs/apps/transfer/authorizations.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# `TransferAuthorization`

`TransferAuthorization` implements the `Authorization` interface for `ibc.applications.transfer.v1.MsgTransfer`. It allows a granter to grant a grantee the privilege to submit MsgTransfer on its behalf. Please see the [Cosmos SDK docs](https://docs.cosmos.network/v0.47/modules/authz) for more details on granting privileges via the `x/authz` module.
`TransferAuthorization` implements the `Authorization` interface for `ibc.applications.transfer.v1.MsgTransfer`. It allows a granter to grant a grantee the privilege to submit `MsgTransfer` on its behalf. Please see the [Cosmos SDK docs](https://docs.cosmos.network/v0.47/modules/authz) for more details on granting privileges via the `x/authz` module.

More specifically, the granter allows the grantee to transfer funds that belong to the granter over a specified channel.

Expand All @@ -12,7 +12,7 @@ It takes:

- a `SourcePort` and a `SourceChannel` which together comprise the unique transfer channel identifier over which authorized funds can be transferred.

- a `SpendLimit` that specifies the maximum amount of tokens the grantee can spend. The `SpendLimit` is updated as the tokens are spent. This `SpendLimit` may also be updated to increase or decrease the limit as the granter wishes.
- a `SpendLimit` that specifies the maximum amount of tokens the grantee can transfer. The `SpendLimit` is updated as the tokens are transfered, unless the sentinel value of the maximum value for a 256-bit unsigned integer (i.e. 2^256 - 1) is used for the amount, in which case the `SpendLimit` will not be updated (please be aware that using this sentinel value will grant the grantee the privilege to transfer **all** the tokens of a given denomination available at the granter's account). The helper function `UnboundedSpendLimit` in the `types` package of the `transfer` module provides the sentinel value that can be used. This `SpendLimit` may also be updated to increase or decrease the limit as the granter wishes.

- an `AllowList` list that specifies the list of addresses that are allowed to receive funds. If this list is empty, then all addresses are allowed to receive funds from the `TransferAuthorization`.

Expand Down
2 changes: 1 addition & 1 deletion e2e/tests/core/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ func (s *ClientTestSuite) extractChainPrivateKeys(ctx context.Context, chain *co
}

// createMaliciousTMHeader creates a header with the provided trusted height with an invalid app hash.
func createMaliciousTMHeader(chainID string, blockHeight int64, trustedHeight clienttypes.Height, timestamp time.Time, tmValSet, tmTrustedVals *tmtypes.ValidatorSet, signers []tmtypes.PrivValidator, oldHeader testsuite.Header ) (*ibctm.Header, error) {
func createMaliciousTMHeader(chainID string, blockHeight int64, trustedHeight clienttypes.Height, timestamp time.Time, tmValSet, tmTrustedVals *tmtypes.ValidatorSet, signers []tmtypes.PrivValidator, oldHeader testsuite.Header) (*ibctm.Header, error) {
tmHeader := tmtypes.Header{
Version: tmprotoversion.Consensus{Block: tmversion.BlockProtocol, App: 2},
ChainID: chainID,
Expand Down
1 change: 0 additions & 1 deletion e2e/testsuite/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,6 @@ func (s *E2ETestSuite) QueryTotalEscrowForDenom(ctx context.Context, chain ibc.C
res, err := queryClient.TotalEscrowForDenom(ctx, &transfertypes.QueryTotalEscrowForDenomRequest{
Denom: denom,
})

if err != nil {
return sdk.Coin{}, err
}
Expand Down
30 changes: 25 additions & 5 deletions modules/apps/transfer/types/transfer_authorization.go
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
package types

import (
"math/big"

errorsmod "cosmossdk.io/errors"
sdkmath "cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/authz"

ibcerrors "github.com/cosmos/ibc-go/v7/internal/errors"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v7/modules/core/24-host"
)

var _ authz.Authorization = (*TransferAuthorization)(nil)

// 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))

// NewTransferAuthorization creates a new TransferAuthorization object.
func NewTransferAuthorization(allocations ...Allocation) *TransferAuthorization {
return &TransferAuthorization{
Expand All @@ -36,15 +41,20 @@ func (a TransferAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.Accep
continue
}

if !isAllowedAddress(ctx, msgTransfer.Receiver, allocation.AllowList) {
return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "not allowed receiver address for transfer")
}

// If the spend limit is set to the MaxUint256 sentinel value, do not subtract the amount from the spend limit.
if allocation.SpendLimit.AmountOf(msgTransfer.Token.Denom).Equal(UnboundedSpendLimit()) {
return authz.AcceptResponse{Accept: true, Delete: false, Updated: &a}, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here Updated can also be nil but I'm happy to keep it explicit. ref

Suggested change
return authz.AcceptResponse{Accept: true, Delete: false, Updated: &a}, nil
return authz.AcceptResponse{Accept: true, Delete: false, Updated: nil}, nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually let's keep it since the tests are checking for the Updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think updated being nil is useful for two reasons:

  • readability, in this situation no update should occur to the allocations (no undesired sideaffects, removes possibility of buggy code from causing issues)
  • less gas costs (returning an updated value causes kv store read/writes)

Is it not possible to update the tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make quick PR to adapt it. Your reasoning seems good enough for me!

}

limitLeft, isNegative := allocation.SpendLimit.SafeSub(msgTransfer.Token)
Vvaradinov marked this conversation as resolved.
Show resolved Hide resolved
if isNegative {
return authz.AcceptResponse{}, errorsmod.Wrapf(ibcerrors.ErrInsufficientFunds, "requested amount is more than spend limit")
}

if !isAllowedAddress(ctx, msgTransfer.Receiver, allocation.AllowList) {
return authz.AcceptResponse{}, errorsmod.Wrap(ibcerrors.ErrInvalidAddress, "not allowed address for transfer")
}

if limitLeft.IsZero() {
a.Allocations = append(a.Allocations[:index], a.Allocations[index+1:]...)
if len(a.Allocations) == 0 {
Expand All @@ -65,6 +75,7 @@ func (a TransferAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.Accep
Allocations: a.Allocations,
}}, nil
}

return authz.AcceptResponse{}, errorsmod.Wrapf(ibcerrors.ErrNotFound, "requested port and channel allocation does not exist")
}

Expand Down Expand Up @@ -128,3 +139,12 @@ func isAllowedAddress(ctx sdk.Context, receiver string, allowedAddrs []string) b
}
return false
}

// UnboundedSpendLimit returns the sentinel value that can be used
// as the amount for a denomination's spend limit for which spend limit updating
// should be disabled. Please note that using this sentinel value means that a grantee
// will be granted the privilege to do ICS20 token transfers for the total amount
// of the denomination available at the granter's account.
func UnboundedSpendLimit() sdkmath.Int {
return sdk.NewIntFromBigInt(maxUint256)
}
50 changes: 49 additions & 1 deletion modules/apps/transfer/types/transfer_authorization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package types_test
import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/authz"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: import formatting

"github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
ibctesting "github.com/cosmos/ibc-go/v7/testing"
"github.com/cosmos/ibc-go/v7/testing/mock"
Expand Down Expand Up @@ -86,6 +85,48 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
suite.Require().Len(updatedAuthz.Allocations, 1)
},
},
{
"success: with unlimited spend limit of max uint256",
func() {
transferAuthz.Allocations[0].SpendLimit = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, types.UnboundedSpendLimit()))
},
func(res authz.AcceptResponse, err error) {
suite.Require().NoError(err)

updatedTransferAuthz, ok := res.Updated.(*types.TransferAuthorization)
suite.Require().True(ok)

remainder := updatedTransferAuthz.Allocations[0].SpendLimit.AmountOf(sdk.DefaultBondDenom)
suite.Require().True(types.UnboundedSpendLimit().Equal(remainder))
},
},
{
"test multiple coins does not overspend",
func() {
transferAuthz.Allocations[0].SpendLimit = transferAuthz.Allocations[0].SpendLimit.Add(
sdk.NewCoins(
sdk.NewCoin("test-denom", sdk.NewInt(100)),
sdk.NewCoin("test-denom2", sdk.NewInt(100)),
)...,
)
msgTransfer.Token = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(50))
},
func(res authz.AcceptResponse, err error) {
suite.Require().NoError(err)

updatedTransferAuthz, ok := res.Updated.(*types.TransferAuthorization)
suite.Require().True(ok)

remainder := updatedTransferAuthz.Allocations[0].SpendLimit.AmountOf(sdk.DefaultBondDenom)
suite.Require().True(sdk.NewInt(50).Equal(remainder))

remainder = updatedTransferAuthz.Allocations[0].SpendLimit.AmountOf("test-denom")
suite.Require().True(sdk.NewInt(100).Equal(remainder))

remainder = updatedTransferAuthz.Allocations[0].SpendLimit.AmountOf("test-denom2")
suite.Require().True(sdk.NewInt(100).Equal(remainder))
},
},
{
"no spend limit set for MsgTransfer port/channel",
func() {
Expand Down Expand Up @@ -190,6 +231,13 @@ func (suite *TypesTestSuite) TestTransferAuthorizationValidateBasic() {
},
true,
},
{
"success: with unlimited spend limit of max uint256",
func() {
transferAuthz.Allocations[0].SpendLimit = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, types.UnboundedSpendLimit()))
},
true,
},
{
"empty allocations",
func() {
Expand Down