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 15 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
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
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
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
### State Machine Breaking

### Improvements

* (tests) [\#3138](https://github.com/cosmos/ibc-go/pull/3138) Support benchmarks and fuzz tests through `testing.TB`.
* (ics20) [\#3454](https://github.com/cosmos/ibc-go/pull/3454) Represent authz unlimited spending limit as `math.MaxInt64` value.
Vvaradinov marked this conversation as resolved.
Show resolved Hide resolved
fedekunze marked this conversation as resolved.
Show resolved Hide resolved

### Features

Expand Down
11 changes: 10 additions & 1 deletion modules/apps/transfer/types/transfer_authorization.go
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
package types

import (
"math/big"

errorsmod "cosmossdk.io/errors"
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,6 +40,10 @@ func (a TransferAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.Accep
continue
}

if allocation.SpendLimit.AmountOf(msgTransfer.Token.Denom).Equal(sdk.NewIntFromBigInt(MaxUint256)) {
Vvaradinov marked this conversation as resolved.
Show resolved Hide resolved
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")
Expand Down Expand Up @@ -65,6 +73,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
44 changes: 43 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,22 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
suite.Require().Len(updatedAuthz.Allocations, 1)
},
},
{
"success: with unlimted spend limit uint256 max",
func() {
transferAuthz.Allocations[0].SpendLimit = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewIntFromBigInt(types.MaxUint256)))
},
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.NewIntFromBigInt(types.MaxUint256).Equal(remainder))

},
},
{
"no spend limit set for MsgTransfer port/channel",
func() {
Expand Down Expand Up @@ -114,6 +129,26 @@ func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
suite.Require().Error(err)
},
},
{
"test multiple coins does not overspend",
func() {
transferAuthz.Allocations[0].SpendLimit = sdk.NewCoins(
sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)),
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))
},
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -190,6 +225,13 @@ func (suite *TypesTestSuite) TestTransferAuthorizationValidateBasic() {
},
true,
},
{
"success: with unlimited spend limit uint256 max",
func() {
transferAuthz.Allocations[0].SpendLimit = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewIntFromBigInt(types.MaxUint256)))
},
true,
},
{
"empty allocations",
func() {
Expand Down