-
Notifications
You must be signed in to change notification settings - Fork 623
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
fedekunze
merged 31 commits into
cosmos:main
from
Vvaradinov:Vvaradinov/ics20-unlimited-authz
May 15, 2023
Merged
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 201ea0f
CHANGELOG
Vvaradinov 10a8df5
Update CHANGELOG.md
98d9d3c
fix: updated unlimited spending limit to be represented with the MaxI…
Vvaradinov 01fa2d5
Merge branch 'Vvaradinov/ics20-unlimited-authz' of https://github.com…
Vvaradinov 87daf70
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
Vvaradinov bdb9fc6
Update modules/apps/transfer/types/transfer_authorization.go
Vvaradinov 05719c1
Update CHANGELOG.md
Vvaradinov 7dc4640
fix: lint
Vvaradinov b33dba6
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
fedekunze 633c254
Update modules/apps/transfer/types/transfer_authorization.go
fedekunze f80ada0
fix: update failing test and add test case suggested in review
Vvaradinov ff05f63
fix: moved isAllowedAddress check before coin loop
Vvaradinov 005a537
fix: use maxUint256 case so it aligns with what's being passed from t…
Vvaradinov 21cd1f6
refactor transfer authz to remove coins iteration in favour of explic…
damiannolan b3abb10
make format
damiannolan edc031b
Update modules/apps/transfer/types/transfer_authorization.go
Vvaradinov a55efb7
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
damiannolan 05cfa35
fix: add the Authorization to Updated.
Vvaradinov e8b2fed
Merge branch 'Vvaradinov/ics20-unlimited-authz' of https://github.com…
Vvaradinov 7914b62
moving allowlist check to before spend limit logic
damiannolan 1d4260d
Apply suggestions from code review
fedekunze c85fbb3
fix: add comment to spend limit check
Vvaradinov 715a082
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
Vvaradinov 30293f6
review feedback
b7f81c6
Merge pull request #1 from cosmos/Vvaradinov/ics20-unlimited-authz
Vvaradinov 464bccd
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
Vvaradinov 4eb96d3
Update modules/apps/transfer/types/transfer_authorization.go
Vvaradinov cc9cf3f
Update docs/apps/transfer/authorizations.md
damiannolan 3552971
Merge branch 'main' into Vvaradinov/ics20-unlimited-authz
aab8b18
fix: changed to new sentinel value name
Vvaradinov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,6 @@ package types_test | |
import ( | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/cosmos/cosmos-sdk/x/authz" | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
@@ -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() { | ||
|
@@ -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() { | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here
Updated
can also benil
but I'm happy to keep it explicit. refThere was a problem hiding this comment.
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
.There was a problem hiding this comment.
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:
Is it not possible to update the tests?
There was a problem hiding this comment.
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!