feat: adding authz support for ICS-20 `MsgTransfer` #2795

zmanian · 2022-11-20T02:56:17Z

Authz grant for ics-20 transfers

This PR introduces an authz grant for ICS-20 transfers.

it enables restricting transfers to only specified channels and addresses.

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.
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.

Create an authz grant that resricts to transfers to specific addresses and channels.

crodriguezvega

Thanks a lot for adding, @zmanian! This actually will close #2431, which was requested some time ago.

Even though the unit tests look great, I think it would be nice to add some integration/e2e tests.

Please let us know if you would be interested in collaborating on the PR with us. Maybe @jonathanjmak would also be interested. We can then discuss a way to work together and take this to the finish line.

And we need also a changelog entry :) (I would say under Features).

crodriguezvega · 2022-11-21T08:52:40Z

proto/ibc/applications/transfer/v2/authz.proto

+  repeated cosmos.base.v1beta1.Coin spend_limit = 3
+      [(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"];
+  // allowed addresses to be sent via transfer message
+  repeated string allowed_addresses = 4;


This is a nit, but maybe we could use allow_list, which is the same name the SDK uses in send authorization? Just to keep terminology a bit consistent...

Agree, allow_list is probably more consistent terminology. It would be useful to mention in the protodoc that this is an allow list of receiver addresses.

modules/apps/transfer/types/transfer_authz.go

proto/ibc/applications/transfer/v2/authz.proto

crodriguezvega · 2022-11-21T09:15:55Z

modules/apps/transfer/types/transfer_authz.go

+	return sdk.MsgTypeURL(&MsgTransfer{})
+}
+
+func IsAllowedAddress(receiver string, allowedAddrs []string) bool {


I guess this function can be made private.

crodriguezvega · 2022-11-21T09:23:06Z

proto/ibc/applications/transfer/v2/authz.proto

+import "gogoproto/gogo.proto";
+import "cosmos/base/v1beta1/coin.proto";
+
+message PortChannelAmount {


Just thinking of some alternative names... Maybe Allocation (since that's the field name used in TransferAuthorization) or Allowance?

Agree, Allocation works!

modules/apps/transfer/types/transfer_authz.go

crodriguezvega · 2022-11-21T09:28:28Z

modules/apps/transfer/types/transfer_authz_test.go

+	toAddr         = sdk.AccAddress("_______to________")
+)
+
+func TestTransferAuthorization(t *testing.T) {


I think it would be nice to create a set of table tests for the different cases tested here. What do you think, @colin-axner?

crodriguezvega · 2022-11-21T09:34:18Z

modules/apps/transfer/types/transfer_authz.go

+)
+
+// NewTransferAuthorization creates a new TransferAuthorization object.
+func NewTransferAuthorization(sourcePorts, sourceChannels []string, spendLimits []sdk.Coins, allowedAddrs [][]string) *TransferAuthorization {


Passing each input as an individual slice opens up the possibility of mismatch in the length of the slices. We should handle that situation... or maybe something better would be to accept as input a slice of PortChannelAmount that the caller has already initialized with the desired values (I guess in this case we would need to make a copy of the slice, since it might be modified in Accept).

I'd like to suggest using varargs. I think it makes a flexible/clean API.

func NewTransferAuthorization(allocations ...Allocation) *TransferAuthorization { return &TransferAuthorization{ Allocations: allocations, } }

I don't understand why we would need to make any copies? If TransferAuthorization is modified in Accept, an Update is returned to authz in the AcceptResponse, right?

jonathanjmak

just a few small comments

context: #2431 (comment)

jonathanjmak · 2022-11-21T19:22:34Z

modules/apps/transfer/types/transfer_authz.go

@@ -0,0 +1,105 @@
+package types


small nit here as well, but can we name it transfer_authorization.go so it's similar to this?

modules/apps/transfer/types/transfer_authz.go

…to ibc_transfer_authz

damiannolan

Awesome work! Thanks for getting this started and apologies for the delay getting around to review.

Left some nits and suggestions! It looks like the tests should be updated to include the memo field arg in NewMsgTransfer. I imagine the fork was likely out of date.

Would be great to see an e2e test with this, I can add one in a follow up PR.
Also happy to help out in pushing this forward!

edit: I've renamed the PR title as it's used by default when merging a squash commit, we use this to parse changelog entries using conventional commits.

damiannolan · 2022-12-15T21:30:24Z

modules/apps/transfer/types/errors.go

@@ -14,4 +14,5 @@ var (
 	ErrSendDisabled            = sdkerrors.Register(ModuleName, 7, "fungible token transfers from this chain are disabled")
 	ErrReceiveDisabled         = sdkerrors.Register(ModuleName, 8, "fungible token transfers to this chain are disabled")
 	ErrMaxTransferChannels     = sdkerrors.Register(ModuleName, 9, "max transfer channels")
+	ErrDuplicateEntry          = sdkerrors.Register(ModuleName, 10, "duplicated entry")


At the moment this error is being returned directly from ValidateBasic with no additional information.
As a user I would see something like: failed to execute message; message types.MsgTransfer: duplicated entry returned from authz

Since this error is specific to x/authz functionality should we consider renaming to something like ErrInvalidAuthorization and then we can reuse and wrap as necessary?

Suggested change

ErrDuplicateEntry = sdkerrors.Register(ModuleName, 10, "duplicated entry")

ErrInvalidAuthorization = sdkerrors.Register(ModuleName, 10, "invalid transfer authorization")

We could then return the following and the user would see: failed to execute message; message types.MsgTransfer: invalid transfer authorization: duplicate entry in allow list cosmos1...

return sdkerrors.Wrapf(ErrInvalidAuthorization, "duplicate entry in allow list %s")

modules/apps/transfer/types/transfer_authz.go

damiannolan · 2022-12-15T21:52:07Z

modules/apps/transfer/types/transfer_authz.go

+func (a TransferAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.AcceptResponse, error) {
+	mTransfer, ok := msg.(*MsgTransfer)
+	if !ok {
+		return authz.AcceptResponse{}, sdkerrors.ErrInvalidType.Wrap("type mismatch")


nit: in ibc-go we normally structure sdk error returns like below. see example

Suggested change

return authz.AcceptResponse{}, sdkerrors.ErrInvalidType.Wrap("type mismatch")

return authz.AcceptResponse{}, sdkerrors.Wrap(sdkerrors.ErrInvalidType, "type mismatch")

modules/apps/transfer/types/transfer_authz.go

damiannolan · 2022-12-15T22:18:13Z

modules/apps/transfer/types/transfer_authz_test.go

+	toAddr         = sdk.AccAddress("_______to________")
+)
+
+func TestTransferAuthorization(t *testing.T) {


damiannolan · 2022-12-15T22:20:59Z

proto/ibc/applications/transfer/v2/authz.proto

+import "gogoproto/gogo.proto";
+import "cosmos/base/v1beta1/coin.proto";
+
+message PortChannelAmount {


Agree, Allocation works!

damiannolan · 2022-12-15T22:23:23Z

proto/ibc/applications/transfer/v2/authz.proto

@@ -0,0 +1,30 @@
+syntax = "proto3";
+
+package ibc.applications.transfer.v2;


This proto file could live in ibc.applications.transfer.v1 but its not required.
The v2 protos were added to support an updated FungibleTokenPacketData. Happy to leave it here.

damiannolan · 2022-12-15T22:24:44Z

modules/apps/transfer/types/transfer_authz.go

+	"golang.org/x/exp/slices"
+)
+
+const gasCostPerIteration = uint64(10)


Surprising that x/authz doesn't provide a default const out of the box!

Aha... 🤔

damiannolan · 2022-12-15T22:34:41Z

modules/apps/transfer/types/transfer_authz.go

+)
+
+// NewTransferAuthorization creates a new TransferAuthorization object.
+func NewTransferAuthorization(sourcePorts, sourceChannels []string, spendLimits []sdk.Coins, allowedAddrs [][]string) *TransferAuthorization {


I'd like to suggest using varargs. I think it makes a flexible/clean API.

func NewTransferAuthorization(allocations ...Allocation) *TransferAuthorization { return &TransferAuthorization{ Allocations: allocations, } }

I don't understand why we would need to make any copies? If TransferAuthorization is modified in Accept, an Update is returned to authz in the AcceptResponse, right?

* update README to reflect new website * pr comment

… solo machine client state definition, removes all solo machine consensus states and removes the localhost client. (cosmos#2824)

…nctions (cosmos#2864)

…ate release line to ibc-go >= v1.x.x. (cosmos#2897)

…os#2903)

…#2899)

…beta.1 (cosmos#2870)

* update chainconfig * update to commit fixing broadcastTx * update to use SDK default cointype 118 * update so relayer tag isn't required

…osmos#2905)

…ap. (cosmos#2829)

* add test for automatic migration of solomachine clientstate version * add clientID generation functions * update godoc * update sprintf message

…onsensus state via the client state `Initialize` method (cosmos#2936) * set the initial client and consensus state via the client state Initialize method. update godocs * updating godocs * updating migration doc * updating light client guide docs for Initialize method * updating migration doc * Apply suggestions from code review Co-authored-by: Charly <charly@interchain.io> * Update docs/ibc/light-clients/client-state.md Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * adding tests to lightclients * updating 02-client tests Co-authored-by: Charly <charly@interchain.io> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

* update compatibility tests * compatibility tests for v4.3.x * adding tags to tests * Skip e2e if test matrix does not exist (cosmos#2949) Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Cian Hatton <cian@interchain.io>

Bumps [goreleaser/goreleaser-action](https://github.com/goreleaser/goreleaser-action) from 3 to 4. - [Release notes](https://github.com/goreleaser/goreleaser-action/releases) - [Commits](goreleaser/goreleaser-action@v3...v4) --- updated-dependencies: - dependency-name: goreleaser/goreleaser-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

* removing solomachine header sequence * removing commented out code in validate basic

…mos#2933)

crodriguezvega · 2022-12-19T20:38:00Z

Thank yo very much for starting this feature, @zmanian. We will continue now opening new PRs against the feature branch.

icafa added 4 commits September 27, 2022 09:14

ibc transfer authz work

8a7f2bf

add allowed address restriction for ibc transfer authz

f84bb92

expect error when transferring to not allowed address

2b5ea19

feat: authz grants for ics-20 transfers

805afdf

Create an authz grant that resricts to transfers to specific addresses and channels.

zmanian requested review from colin-axner, AdityaSripal, damiannolan, crodriguezvega, charleenfei and chatton as code owners November 20, 2022 02:56

crodriguezvega reviewed Nov 21, 2022

View reviewed changes

crodriguezvega mentioned this pull request Nov 21, 2022

Adding in a new Authorization Type for IBC Transfers in Authz #2431

Closed

3 tasks

jonathanjmak reviewed Nov 21, 2022

View reviewed changes

icafa and others added 5 commits December 5, 2022 21:37

convert IsAllPositive to Validate

0bcb95e

Merge branch 'ibc_transfer_authz' of github.com:iqlusioninc/ibc-go in…

d09caaf

…to ibc_transfer_authz

add gas cost per interation

3290c18

duplicated entry check

288f9c0

Merge branch 'main' into ibc_transfer_authz

2d33a9f

damiannolan reviewed Dec 15, 2022

View reviewed changes

damiannolan changed the title ~~Authz grant for ICS-20 IBC transfers~~ feat: adding authz support for ICS-20 MsgTransfer Dec 15, 2022

charleenfei and others added 10 commits December 19, 2022 09:47

chore: scaffold custom IBC light client development guide (cosmos#2860)

1742c54

chore: changing ibcprotocol.org to ibcprotocol.dev (cosmos#2884)

412e213

chore: update README to include link to IDA (cosmos#2887)

72fe693

* update README to reflect new website * pr comment

feat: Add genesis migrations for v6 to v7. The migration migrates the…

85ebc84

… solo machine client state definition, removes all solo machine consensus states and removes the localhost client. (cosmos#2824)

refactor: simplify automatic migration code by using client keeper fu…

19c79ae

…nctions (cosmos#2864)

imp(api)!: remove legacy migrations required for upgrading from Starg…

83854f4

…ate release line to ibc-go >= v1.x.x. (cosmos#2897)

chore: remove unnecessary file (cosmos#2898)

6bb1543

Update release-tracker.md

ab2e995

docs: v6 ICA migration docs improvements

f5e0233

e2e: changing default relayer tag to be v2.1.2 instead of main. (cosm…

2728e7d

…os#2903)

Carlos Rodriguez and others added 25 commits December 19, 2022 09:47

docs: add legacy docs for ICA

8ad5932

docs: added documentation for client state methods. (cosmos#2886)

04cbd60

docs: improvements to ICA docs

a25a9d0

docs: move solomachine docs into the docs directory. (cosmos#2908)

9d7922f

deps: bump technote-space/get-diff-action from 6.1.1 to 6.1.2 (cosmos…

514ad39

…#2899)

deps: bump github.com/cosmos/cosmos-proto from 1.0.0-alpha8 to 1.0.0-…

d1fbdbd

…beta.1 (cosmos#2870)

update README (cosmos#2910)

e1c29e5

Update CHANGELOG.md

267359b

chore: update ibctest to latest commit (cosmos#2909)

a5ff864

* update chainconfig * update to commit fixing broadcastTx * update to use SDK default cointype 118 * update so relayer tag isn't required

Fixing markdown link (cosmos#2924)

bfe7ee8

post v6.0.0 release chores (cosmos#2919)

a322103

docs: update integration docs to include light client registration. (c…

bb08f5e

…osmos#2905)

imp: Add AssertEvents which asserts events against expected event m…

2220957

…ap. (cosmos#2829)

e2e: adding e2e upgrade test for ibc-go v7 (cosmos#2902)

6afa1b6

* add test for automatic migration of solomachine clientstate version * add clientID generation functions * update godoc * update sprintf message

e2e: update tags in e2e upgrade test

cd51cc9

chore: use diffs to make registration more clear (cosmos#2927)

1078875

nit: fix typo in a comment

2ea8655

chore: add backports for v4.3.x and v6.1.x

e8f6eaf

fix(statemachine)!: check x/bank send enabled before escrowing fees

d3be95a

chore(api)!: removing solomachine header sequence (cosmos#2941)

ddc4f2d

* removing solomachine header sequence * removing commented out code in validate basic

chore(deps): bump bufbuild/buf-setup-action from 1.9.0 to 1.10.0 (cos…

0deb8ad

…mos#2933)

Resolve some code review comments

8f2bbd2

zmanian requested a review from tmsdkeys as a code owner December 19, 2022 17:56

zmanian changed the base branch from main to feat/authz-ics20 December 19, 2022 17:57

crodriguezvega merged commit d1b8a69 into cosmos:feat/authz-ics20 Dec 19, 2022

crodriguezvega mentioned this pull request Dec 20, 2022

authz/ics20: review comments for initial PR #2960

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding authz support for ICS-20 `MsgTransfer` #2795

feat: adding authz support for ICS-20 `MsgTransfer` #2795

zmanian commented Nov 20, 2022

crodriguezvega left a comment

crodriguezvega Nov 21, 2022

jonathanjmak Nov 21, 2022

damiannolan Dec 15, 2022

crodriguezvega Nov 21, 2022

crodriguezvega Nov 21, 2022

damiannolan Dec 15, 2022

crodriguezvega Nov 21, 2022

damiannolan Dec 15, 2022

crodriguezvega Nov 21, 2022

damiannolan Dec 15, 2022

jonathanjmak left a comment

jonathanjmak Nov 21, 2022

damiannolan left a comment •

edited

Loading

damiannolan Dec 15, 2022 •

edited

Loading

damiannolan Dec 15, 2022

damiannolan Dec 15, 2022

damiannolan Dec 15, 2022

damiannolan Dec 15, 2022

damiannolan Dec 15, 2022

damiannolan Dec 16, 2022

damiannolan Dec 15, 2022

crodriguezvega commented Dec 19, 2022

	ErrDuplicateEntry = sdkerrors.Register(ModuleName, 10, "duplicated entry")
	ErrInvalidAuthorization = sdkerrors.Register(ModuleName, 10, "invalid transfer authorization")

	return authz.AcceptResponse{}, sdkerrors.ErrInvalidType.Wrap("type mismatch")
	return authz.AcceptResponse{}, sdkerrors.Wrap(sdkerrors.ErrInvalidType, "type mismatch")

		@@ -0,0 +1,30 @@
		syntax = "proto3";

		package ibc.applications.transfer.v2;

feat: adding authz support for ICS-20 MsgTransfer #2795

feat: adding authz support for ICS-20 MsgTransfer #2795

Conversation

zmanian commented Nov 20, 2022

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanjmak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan left a comment • edited Loading

Choose a reason for hiding this comment

damiannolan Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega commented Dec 19, 2022

feat: adding authz support for ICS-20 `MsgTransfer` #2795

feat: adding authz support for ICS-20 `MsgTransfer` #2795

damiannolan left a comment •

edited

Loading

damiannolan Dec 15, 2022 •

edited

Loading