imp: represent unlimited approvals with MaxUint256 value #3454

Vvaradinov · 2023-04-13T13:34:39Z

Description

This PR implements the changes proposed in #3452

closes: #3452

Commit Message / Changelog Entry

imp(statemachine)!: represent unlimited approvals with a the MaxInt256 value

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

chatton

Thanks for the PR @Vvaradinov!

My concern about giving a special value to nil here means that it because easier to accidentally configure unlimited spending if the field is omitted.

I think I would lean towards creating a named sentinel value to pass in explicitly as the SpendLimit.

e.g.

allocation.SpendLimit = UnlimitedSpending()

What do you think?

crodriguezvega · 2023-04-18T11:25:04Z

I agree with the risk described above by @chatton.

And besides that, and after discussing also with @womensrights, we think that it is risky to not specify the unlimited spending for a particular denomination. With the approach of using nil (or even the sentinel value) the authorisation would grant the grantee the possibility to spend without any limits any denomination. We think that it would be safer to follow an approach closer to what Ethereum does and use the maximum Int value for Amount in sdk.Coin to indicate unlimited spending for a particular denomination. What do you think?

…nt64

…/Vvaradinov/ibc-go into Vvaradinov/ics20-unlimited-authz

codecov-commenter · 2023-04-25T22:35:38Z

Codecov Report

Merging #3454 (aab8b18) into main (3882830) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3454   +/-   ##
=======================================
  Coverage   78.79%   78.79%           
=======================================
  Files         182      182           
  Lines       12683    12688    +5     
=======================================
+ Hits         9993     9998    +5     
  Misses       2258     2258           
  Partials      432      432

Impacted Files	Coverage Δ
...ules/apps/transfer/types/transfer_authorization.go	`92.04% <100.00%> (+0.47%)`	⬆️

modules/apps/transfer/types/transfer_authorization.go

CHANGELOG.md

Co-authored-by: Federico Kunze Küllmer <[email protected]>

modules/apps/transfer/types/transfer_authorization.go

damiannolan · 2023-05-04T09:09:00Z

Looks like some unit test is failing 👀 Will take a closer look later today

fedekunze

ACK! Great work @Vvaradinov

modules/apps/transfer/types/transfer_authorization.go

CHANGELOG.md

crodriguezvega · 2023-05-12T14:14:11Z

Thanks for all the work on this one, @Vvaradinov! I opened a PR with an idea suggested by @damiannolan and @colin-axner (making maxUint256 un-exported and adding an exported function to retrieve it) and updating the documentation.

review feedback

damiannolan

ACK, thanks @crodriguezvega

modules/apps/transfer/types/transfer_authorization.go

Co-authored-by: Damian Nolan <[email protected]>

docs/apps/transfer/authorizations.md

crodriguezvega

ACK. Thanks a lot for all this work, @Vvaradinov!

modules/apps/transfer/types/transfer_authorization.go

crodriguezvega

ACK. Thanks a lot for all this work, @Vvaradinov!

* imp: represent unlimited approvals with a nil value * CHANGELOG * Update CHANGELOG.md * fix: updated unlimited spending limit to be represented with the MaxInt64 * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Federico Kunze Küllmer <[email protected]> * Update CHANGELOG.md Co-authored-by: Federico Kunze Küllmer <[email protected]> * fix: lint * Update modules/apps/transfer/types/transfer_authorization.go * fix: update failing test and add test case suggested in review * fix: moved isAllowedAddress check before coin loop * fix: use maxUint256 case so it aligns with what's being passed from the EVM extension * refactor transfer authz to remove coins iteration in favour of explicit amount checking * make format * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Damian Nolan <[email protected]> * fix: add the Authorization to Updated. * moving allowlist check to before spend limit logic * Apply suggestions from code review * fix: add comment to spend limit check * review feedback * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Damian Nolan <[email protected]> * Update docs/apps/transfer/authorizations.md * fix: changed to new sentinel value name --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Federico Kunze Küllmer <[email protected]> Co-authored-by: Damian Nolan <[email protected]> (cherry picked from commit 7e6eb4c) # Conflicts: # CHANGELOG.md # e2e/tests/core/client_test.go # e2e/testsuite/grpc_query.go # modules/apps/transfer/types/transfer_authorization.go # modules/apps/transfer/types/transfer_authorization_test.go

* imp: represent unlimited approvals with a nil value * CHANGELOG * Update CHANGELOG.md * fix: updated unlimited spending limit to be represented with the MaxInt64 * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Federico Kunze Küllmer <[email protected]> * Update CHANGELOG.md Co-authored-by: Federico Kunze Küllmer <[email protected]> * fix: lint * Update modules/apps/transfer/types/transfer_authorization.go * fix: update failing test and add test case suggested in review * fix: moved isAllowedAddress check before coin loop * fix: use maxUint256 case so it aligns with what's being passed from the EVM extension * refactor transfer authz to remove coins iteration in favour of explicit amount checking * make format * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Damian Nolan <[email protected]> * fix: add the Authorization to Updated. * moving allowlist check to before spend limit logic * Apply suggestions from code review * fix: add comment to spend limit check * review feedback * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Damian Nolan <[email protected]> * Update docs/apps/transfer/authorizations.md * fix: changed to new sentinel value name --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Federico Kunze Küllmer <[email protected]> Co-authored-by: Damian Nolan <[email protected]> (cherry picked from commit 7e6eb4c) # Conflicts: # CHANGELOG.md # e2e/tests/core/client_test.go # e2e/testsuite/grpc_query.go # modules/apps/transfer/types/transfer_authorization.go

) (#3580) * imp: represent unlimited approvals with MaxUint256 value (#3454) * imp: represent unlimited approvals with a nil value * CHANGELOG * Update CHANGELOG.md * fix: updated unlimited spending limit to be represented with the MaxInt64 * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Federico Kunze Küllmer <[email protected]> * Update CHANGELOG.md Co-authored-by: Federico Kunze Küllmer <[email protected]> * fix: lint * Update modules/apps/transfer/types/transfer_authorization.go * fix: update failing test and add test case suggested in review * fix: moved isAllowedAddress check before coin loop * fix: use maxUint256 case so it aligns with what's being passed from the EVM extension * refactor transfer authz to remove coins iteration in favour of explicit amount checking * make format * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Damian Nolan <[email protected]> * fix: add the Authorization to Updated. * moving allowlist check to before spend limit logic * Apply suggestions from code review * fix: add comment to spend limit check * review feedback * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Damian Nolan <[email protected]> * Update docs/apps/transfer/authorizations.md * fix: changed to new sentinel value name --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Federico Kunze Küllmer <[email protected]> Co-authored-by: Damian Nolan <[email protected]> (cherry picked from commit 7e6eb4c) # Conflicts: # CHANGELOG.md # e2e/tests/core/client_test.go # e2e/testsuite/grpc_query.go # modules/apps/transfer/types/transfer_authorization.go # modules/apps/transfer/types/transfer_authorization_test.go * resolving conflicts --------- Co-authored-by: Vladislav Varadinov <[email protected]> Co-authored-by: Damian Nolan <[email protected]>

) (#3581) * imp: represent unlimited approvals with MaxUint256 value (#3454) * imp: represent unlimited approvals with a nil value * CHANGELOG * Update CHANGELOG.md * fix: updated unlimited spending limit to be represented with the MaxInt64 * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Federico Kunze Küllmer <[email protected]> * Update CHANGELOG.md Co-authored-by: Federico Kunze Küllmer <[email protected]> * fix: lint * Update modules/apps/transfer/types/transfer_authorization.go * fix: update failing test and add test case suggested in review * fix: moved isAllowedAddress check before coin loop * fix: use maxUint256 case so it aligns with what's being passed from the EVM extension * refactor transfer authz to remove coins iteration in favour of explicit amount checking * make format * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Damian Nolan <[email protected]> * fix: add the Authorization to Updated. * moving allowlist check to before spend limit logic * Apply suggestions from code review * fix: add comment to spend limit check * review feedback * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Damian Nolan <[email protected]> * Update docs/apps/transfer/authorizations.md * fix: changed to new sentinel value name --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Federico Kunze Küllmer <[email protected]> Co-authored-by: Damian Nolan <[email protected]> (cherry picked from commit 7e6eb4c) # Conflicts: # CHANGELOG.md # e2e/tests/core/client_test.go # e2e/testsuite/grpc_query.go # modules/apps/transfer/types/transfer_authorization.go * resolving conflicts --------- Co-authored-by: Vladislav Varadinov <[email protected]> Co-authored-by: Damian Nolan <[email protected]>

colin-axner

Nice work :) just left a small note on the updated return value

colin-axner · 2023-05-16T09:22:55Z

modules/apps/transfer/types/transfer_authorization.go

@@ -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)) {
+			return authz.AcceptResponse{Accept: true, Delete: false, Updated: &a}, nil


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?

colin-axner · 2023-05-16T09:26:18Z

modules/apps/transfer/types/transfer_authorization_test.go

@@ -3,7 +3,6 @@ package types_test
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/authz"
-


nit: import formatting

Vvaradinov and others added 3 commits April 13, 2023 16:33

imp: represent unlimited approvals with a nil value

060cdf6

CHANGELOG

201ea0f

Update CHANGELOG.md

10a8df5

chatton reviewed Apr 17, 2023

View reviewed changes

Vvaradinov added 2 commits April 25, 2023 14:03

fix: updated unlimited spending limit to be represented with the MaxI…

98d9d3c

…nt64

Merge branch 'Vvaradinov/ics20-unlimited-authz' of https://github.com…

01fa2d5

…/Vvaradinov/ibc-go into Vvaradinov/ics20-unlimited-authz

Vvaradinov changed the title ~~imp: represent unlimited approvals with a nil value~~ imp: represent unlimited approvals with MaxInt64 value Apr 25, 2023

Merge branch 'main' into Vvaradinov/ics20-unlimited-authz

87daf70

Vvaradinov marked this pull request as ready for review April 25, 2023 12:04

Vvaradinov requested review from colin-axner, AdityaSripal, damiannolan, crodriguezvega, charleenfei and DimitrisJim as code owners April 25, 2023 12:04

crodriguezvega added backport-to-v6.2.x labels Apr 25, 2023

fedekunze reviewed Apr 25, 2023

View reviewed changes

modules/apps/transfer/types/transfer_authorization.go Outdated Show resolved Hide resolved

modules/apps/transfer/types/transfer_authorization.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

Vvaradinov and others added 4 commits April 26, 2023 09:37

Update modules/apps/transfer/types/transfer_authorization.go

bdb9fc6

Co-authored-by: Federico Kunze Küllmer <[email protected]>

Update CHANGELOG.md

05719c1

Co-authored-by: Federico Kunze Küllmer <[email protected]>

fix: lint

7dc4640

Merge branch 'main' into Vvaradinov/ics20-unlimited-authz

b33dba6

fedekunze reviewed Apr 30, 2023

View reviewed changes

modules/apps/transfer/types/transfer_authorization.go Outdated Show resolved Hide resolved

Update modules/apps/transfer/types/transfer_authorization.go

633c254

fedekunze added 20-transfer type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. labels Apr 30, 2023

damiannolan reviewed May 4, 2023

View reviewed changes

modules/apps/transfer/types/transfer_authorization.go Outdated Show resolved Hide resolved

moving allowlist check to before spend limit logic

7914b62

fedekunze approved these changes May 10, 2023

View reviewed changes

modules/apps/transfer/types/transfer_authorization.go Outdated Show resolved Hide resolved

modules/apps/transfer/types/transfer_authorization.go Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

fedekunze and others added 4 commits May 10, 2023 14:45

Apply suggestions from code review

1d4260d

fix: add comment to spend limit check

c85fbb3

Merge branch 'main' into Vvaradinov/ics20-unlimited-authz

715a082

review feedback

30293f6

crodriguezvega removed 20-transfer type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. labels May 12, 2023

Merge pull request #1 from cosmos/Vvaradinov/ics20-unlimited-authz

b7f81c6

review feedback

Vvaradinov requested a review from srdtrk as a code owner May 15, 2023 07:10

Merge branch 'main' into Vvaradinov/ics20-unlimited-authz

464bccd

damiannolan approved these changes May 15, 2023

View reviewed changes

modules/apps/transfer/types/transfer_authorization.go Outdated Show resolved Hide resolved

Update modules/apps/transfer/types/transfer_authorization.go

4eb96d3

Co-authored-by: Damian Nolan <[email protected]>

damiannolan reviewed May 15, 2023

View reviewed changes

docs/apps/transfer/authorizations.md Outdated Show resolved Hide resolved

damiannolan and others added 3 commits May 15, 2023 11:48

Update docs/apps/transfer/authorizations.md

cc9cf3f

Merge branch 'main' into Vvaradinov/ics20-unlimited-authz

3552971

fix: changed to new sentinel value name

aab8b18

crodriguezvega approved these changes May 15, 2023

View reviewed changes

modules/apps/transfer/types/transfer_authorization.go Outdated Show resolved Hide resolved

crodriguezvega approved these changes May 15, 2023

View reviewed changes

fedekunze merged commit 7e6eb4c into cosmos:main May 15, 2023

This was referenced May 15, 2023

imp: represent unlimited approvals with MaxUint256 value (backport #3454) #3580

Merged

imp: represent unlimited approvals with MaxUint256 value (backport #3454) #3581

Merged

colin-axner reviewed May 16, 2023

View reviewed changes

damiannolan mentioned this pull request May 16, 2023

chore: transfer authz nits #3586

Merged

9 tasks

faddat mentioned this pull request Jun 23, 2023

v7.2.0 for sdk 50 #3948

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imp: represent unlimited approvals with MaxUint256 value #3454

imp: represent unlimited approvals with MaxUint256 value #3454

Vvaradinov commented Apr 13, 2023 •

edited

Loading

chatton left a comment

crodriguezvega commented Apr 18, 2023

codecov-commenter commented Apr 25, 2023 •

edited

Loading

damiannolan commented May 4, 2023

fedekunze left a comment •

edited

Loading

crodriguezvega commented May 12, 2023

damiannolan left a comment

crodriguezvega left a comment

crodriguezvega left a comment

colin-axner left a comment

colin-axner May 16, 2023

colin-axner May 16, 2023

imp: represent unlimited approvals with MaxUint256 value #3454

imp: represent unlimited approvals with MaxUint256 value #3454

Conversation

Vvaradinov commented Apr 13, 2023 • edited Loading

Description

Commit Message / Changelog Entry

chatton left a comment

Choose a reason for hiding this comment

crodriguezvega commented Apr 18, 2023

codecov-commenter commented Apr 25, 2023 • edited Loading

Codecov Report

damiannolan commented May 4, 2023

fedekunze left a comment • edited Loading

Choose a reason for hiding this comment

crodriguezvega commented May 12, 2023

damiannolan left a comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

colin-axner May 16, 2023

Choose a reason for hiding this comment

colin-axner May 16, 2023

Choose a reason for hiding this comment

Vvaradinov commented Apr 13, 2023 •

edited

Loading

codecov-commenter commented Apr 25, 2023 •

edited

Loading

fedekunze left a comment •

edited

Loading