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

Allow authz granters to specify forwarding information for token transfers #6589

Closed
crodriguezvega opened this issue Jun 13, 2024 · 5 comments
Assignees
Labels
20-transfer type: feature New features, sub-features or integrations

Comments

@crodriguezvega
Copy link
Contributor

crodriguezvega commented Jun 13, 2024

With the addition of forwarding to MsgTransfer, we should allow authz granters to specify if all or a subset of forwarding routes are allowed when grantees send IBC transfers on their behalf.

As discussed, we can add a new field to the Allocation type:

@@ -21,7 +22,12 @@ message Allocation {
   repeated string allow_list = 4;
   // allow list of memo strings, an empty list prohibits all memo strings;
   // a list only with "*" permits any memo string
-  repeated string allowed_packet_data = 5;
+  repeated string allowed_packet_data     = 5;
+  repeated Hops   allowed_forwarding_hops = 6;
+}
+
+message Hops {
+  repeated Hop hops = 1 [(gogoproto.nullable) = false];
 }

So this field is a slice of Hops, which is itself a slice of Hop, which is a port ID/channel ID pair.

The granter can specify different combinations of hops that the grantee can use in their MsgTransfer. It's bit more problematic how to specify that all routes are allowed, though, since it's not straightforward to use a wildcard * character in the allowed_forwarding_hops field. I am open to suggestions from the rest of the team.

Then in Accept we need to validate the hops in MsgTransfer against theallowed_forwarding_hops slices in the Allocation.

Let's discuss her in the issue is people agree with this proposal and in the naming of things. If we end up reusing Trace instead of Hop, that should be reflected here as well.

@crodriguezvega crodriguezvega added 20-transfer needs discussion Issues that need discussion before they can be worked on type: feature New features, sub-features or integrations labels Jun 13, 2024
@crodriguezvega crodriguezvega moved this to Todo 🏃 in ibc-go Jun 13, 2024
@chatton
Copy link
Contributor

chatton commented Jun 13, 2024

@bznein gonna assign this one to you, I think it will be a nice one we can hop on a call to discuss and/or pair on it 💪

@crodriguezvega
Copy link
Contributor Author

We decided to not use any wildcard to allow all.

@crodriguezvega crodriguezvega removed the needs discussion Issues that need discussion before they can be worked on label Jun 17, 2024
@bznein
Copy link
Contributor

bznein commented Jun 17, 2024

@crodriguezvega

+  repeated Hops   allowed_forwarding_hops = 6;
+}
+
+message Hops {
+  repeated Hop hops = 1 [(gogoproto.nullable) = false];

What's the meaning of having both as repeated fields?

Does it mean each "allowed_forwarding_hops" is a list of allowed Hops that can be specified in a MsgTransfer?

Let's say we have two entries in "allowed_forwarding_hops":

  • one contains two Hops:

    • port1, channel1
    • port2, channel2
  • The other contains one Hop:

    • port3, channel3

Does this mean that the following MsgTransfers are correct:

Hops: 
    port1, channel1
Hops: 
    port2, channel2
Hops: 
    port3, channel3
Hops: 
    port1, channel1
    port2, channel2

But this one is invalid?

Hops: 
    port1, channel1
    port3, channel3

@bznein
Copy link
Contributor

bznein commented Jun 17, 2024

We decided to not use any wildcard to allow all.

I might have missed part of the conversation here.
Does this also mean that allowed_forwarding_hops is a mandatory field?

@crodriguezvega
Copy link
Contributor Author

@crodriguezvega

+  repeated Hops   allowed_forwarding_hops = 6;
+}
+
+message Hops {
+  repeated Hop hops = 1 [(gogoproto.nullable) = false];

What's the meaning of having both as repeated fields?

Does it mean each "allowed_forwarding_hops" is a list of allowed Hops that can be specified in a MsgTransfer?

Let's say we have two entries in "allowed_forwarding_hops":

  • one contains two Hops:

    • port1, channel1
    • port2, channel2
  • The other contains one Hop:

    • port3, channel3

Does this mean that the following MsgTransfers are correct:

Hops: 
    port1, channel1
Hops: 
    port2, channel2
Hops: 
   port3, channel3
Hops: 
   port1, channel1
   port2, channel2

But this one is invalid?

Hops: 
    port1, channel1
    port3, channel3

Sorry for the late replies, @bznein.

The validation should check for exact equality, therefore MSgTransfer should only be valid for these two forwarding hops:

Hops: 
  port3, channel3
Hops: 
  port1, channel1
  port2, channel2

I might have missed part of the conversation here.
Does this also mean that allowed_forwarding_hops is a mandatory field?

No, it's not mandatory. If it's not set, then no forwarding should be allowed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
20-transfer type: feature New features, sub-features or integrations
Projects
Archived in project
Development

No branches or pull requests

3 participants