-
Notifications
You must be signed in to change notification settings - Fork 379
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
ICS20-2: Add Token Forwarding ability to ICS20-2 #1090
base: main
Are you sure you want to change the base?
Conversation
One important aspect of PFM is the ability to compose more complex user experiences by integrating multiple middleware in the stack together, so that you can essentially receive an ICS-20 transfer, perform some action on the intermediate chain, and then forward tokens to the destination Example: A user wants to send tokens from a source chain to an intermediate chain. On the intermediate chain the funds will be received via With middleware this flow would involve wiring up the stack like this:
Then when a packet comes in for an ICS-20 transfer, the swap middleware would call into the underlying app and that call would fall through to the transfer module so that the appropriate bookkeeping could take place to get funds into the user controlled account. Control would then be passed back up to the swap middleware where the swap could take place, the middleware would mutate the Bringing token forwarding into the ICS-20 protocol is amazing to hear, but I think preserving this type of composability for developers to build these more complicated experiences is an important aspect; otherwise, it becomes a bit less appealing. |
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.
Thank you for the quick turnaround with the changes, @AdityaSripal.
I did a first pass and left a few comments. I think my biggest gap of understanding at the moment is in the usage of channelForwardingAddresses
(I think there might be some logic errors with that).
To help with the reasoning I tried to make a coupe of diagrams. Both go through the scenario where and error ack is written on the destination chain, but in one of them the middle hop is the source of the token and in the other one it is not.
The diagram is not completely correct, because I think the logic to transfer/burn token on the middle hop needs to be adjusted a bit.
@@ -51,6 +51,7 @@ interface FungibleTokenPacketDataV2 { | |||
sender: string | |||
receiver: string | |||
memo: string | |||
forwardingPath: []string |
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 it would be nice to add some context and sample usages for this field in a paragraph below? I understand from the code below that this is a list of elements of the form {source port ID}/{source channel ID}
, right?
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.
It would also be good to update the sentence "and receiving account or FungibleTokenPacketDataV2
which specifies multiple tokens being sent between sender and receiver" on line 38 that v2 also support the forwarding path.
@@ -92,6 +93,7 @@ The fungible token transfer bridge module tracks escrow addresses associated wit | |||
```typescript | |||
interface ModuleState { | |||
channelEscrowAddresses: Map<Identifier, string> | |||
channelForwardingAddresses: Map<Identifier, string> |
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.
Is this actually needed? It seems like we could manage by using channelEscrowAddresses
only?
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.
Unless I am missing something about the reasoning behind the separation
channelEscrowAddresses: Map<Identifier, string>
channelForwardingAddresses: Map<Identifier, string>
I agree we could use directly channelEscrowAddresses
. The channelEscrowAddresses
mapping is populated during onChanOpenInit
at line 157 or onChainOpenTry
at line 181. That means we could potentially access this mapping at line 331 using the packet.destintionChannel
as identifier.
Instead the channelForwardingAddresses
is never set. Thus the read we do at line 331 may be illegal.
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.
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.
If the only reason for using this is a logical separation, I think I would be in favour of simply using the escrow address. Having this other set of addresses adds to the overall complexity without adding a huge amount of value IMO.
And like @damiannolan is linking, the existing escrow address has already been reasoned about in terms of collisions.
// send the tokens we received above to the next port and channel | ||
// on the forwarding path | ||
// and reduce the forwardingPath by the first element | ||
nextSequence = sendFungibleTokens( |
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.
nextSequence = sendFungibleTokens( | |
nextPacketSequence = sendFungibleTokens( |
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.
Hello guys and thank you for the great work!
I have done a first review pass and left some comments and suggestions.
Overall, I found a bit complicated reasoning by hand on certain specific scenarios, thus I agree having quint spec would be much helpful to ensure certain properties are respected.
To analyse the function flows I derived with the help of GPT4 and this app.code2flow website some flowcharts diagrams that I repost here for reference in case they can be useful for others.
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.
These diagrams helped me personally to reason through the flow of an error acknowledgement with a middle hop for the 4 scenarios that are handled in revertInFlightChanges
.
When doing this exercise, I noticed a couple of things that I left as comments.
Co-authored-by: Carlos Rodriguez <[email protected]>
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.
Did another review, and left a couple of small comments, but this looks good to me now. Thank you @AdityaSripal for the specification and @sangier for reviewing and all the work on the quint spec.
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 have updated the quint spec with multidenoms logic + tests and a couple of invariants. Note that timeout tests are skipped in quint as they would trigger the same refund/revert logic as the error acknowledgements tests.
After testing spec looks good to me!
Thanks @AdityaSripal and @crodriguezvega for all the work!
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.
Hey guys, I have done another review pass for the latest changes and left some comments.
sourcePort: string, | ||
sourceChannel: string, | ||
timeoutHeight: Height, | ||
timeoutTimestamp: uint64, // in unix nanoseconds | ||
): uint64 { | ||
// memo and forwardingPath cannot both be non-empty | ||
abortTransactionUnless(memo != "" && forwardingPath != nil) |
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.
If my understanding is correct, with this check we want to ensure that the memo
is not set if a forwardingPath exist.
However, if want to use ics20v2 with a multidenom packet from A to B without further actions required, kind like:
FungibleTokenPacketDataV2 {
tokens: [
Token{
denom: uatom,
amount: 500,
trace: ["transfer/channel-1", "transfer/channel-4"],
},
Token{
denom: btc,
amount: 7,
trace: ["transfer/channel-3"],
}
],
sender: cosmosexampleaddr1A,
receiver: cosmosexampleaddr2B,
memo: "",
forwardingPath: {nil, ""}
This packet wouldn't pass this check, right?
Given:
abortUnless(True) = Pass
abortUnless(False) = Fail
The truth table, where 0 means empty, should be like this:
Memo | ForwardingPath | Result | Case |
---|---|---|---|
0 | 0 | Pass | No Memo and No Forwarding |
0 | 1 | Pass | Final Hop Memo eventually indicated in the Forwarding |
1 | 0 | Pass | Memo indicated in the First Hop and No Forwarding |
1 | 1 | Fail | Memo indicated in both First Hop and Forwarding |
Shouldn't the expression be like:
abortTransactionUnless(memo != "" NAND forwardingPath.Hops != nil)
?
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.
Yes, I think you're right.
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.
Actually, couldn't we get rid of one of of the two memos? If the memo supposed to trigger an action on the final destination of the tokens, why cannot we have something like this?
interface FungibleTokenPacketDataV2 {
tokens: []Token
sender: string
receiver: string
memo: string
forwardingPath: ForwardingInfo
}
interface ForwardingInfo {
hops: []Hop
}
But then it would not be possible to trigger any action on intermediary hops. Is that desired behaviour?
//check that next channel supports token forwarding | ||
channel = publicStore.get(forwardingPath[0].portID, forwardingPath[0].channelID) | ||
if channel.version != "ics20-2" && len(forwardingPath) > 1 { | ||
channel = publicStore.get(forwardingPath.hops[0].portID, forwardingPath.hops[0].channelID) |
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.
Should this line be channel = provableStore.get(channelPath(forwardingPath.hops[0].portID, forwardingPath.hops[0].channelID))
?
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 I don't understand yet the reason for having two memo
fields, one in FungibleTokenPacketDataV2
and one in ForwardingInfo
, if the memo
is only going to trigger an action on the receiving chain...
sourcePort: string, | ||
sourceChannel: string, | ||
timeoutHeight: Height, | ||
timeoutTimestamp: uint64, // in unix nanoseconds | ||
): uint64 { | ||
// memo and forwardingPath cannot both be non-empty | ||
abortTransactionUnless(memo != "" && forwardingPath != nil) |
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.
Yes, I think you're right.
sourcePort: string, | ||
sourceChannel: string, | ||
timeoutHeight: Height, | ||
timeoutTimestamp: uint64, // in unix nanoseconds | ||
): uint64 { | ||
// memo and forwardingPath cannot both be non-empty | ||
abortTransactionUnless(memo != "" && forwardingPath != nil) |
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.
Actually, couldn't we get rid of one of of the two memos? If the memo supposed to trigger an action on the final destination of the tokens, why cannot we have something like this?
interface FungibleTokenPacketDataV2 {
tokens: []Token
sender: string
receiver: string
memo: string
forwardingPath: ForwardingInfo
}
interface ForwardingInfo {
hops: []Hop
}
But then it would not be possible to trigger any action on intermediary hops. Is that desired behaviour?
// before propogating the error acknowledgement back to original sender chain | ||
revertInFlightChanges(packet, prevPacket) | ||
// write error acknowledgement | ||
FungibleTokenPacketAcknowledgement ack = FungibleTokenPacketAcknowledgement{false, "forwarded packet failed"} |
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.
Probably this is fine, but just wanted to comment that in case of time out, the middle chains will propagate an error ack back to the sending chain. Would there be any implications for applications or middleware on the sending chain if the packet is error acknowledged instead of timed out?
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.
Maybe change the message to make it more specific?
FungibleTokenPacketAcknowledgement ack = FungibleTokenPacketAcknowledgement{false, "forwarded packet failed"} | |
FungibleTokenPacketAcknowledgement ack = FungibleTokenPacketAcknowledgement{false, "forwarded packet timed out"} |
if len(forwardingPath.hops) > 0 { | ||
//check that next channel supports token forwarding | ||
channel = provableStore.get(channelPath(forwardingPath.hops[0].portID, forwardingPath.hops[0].channelID)) | ||
if channel.version != "ics20-2" && len(forwardingPath.hops) > 1 { |
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.
Does all the previous minting / transfer need to be reverted here, or is it implicit?
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.
Yes, it's kinda implicit: if an error ack is returned then the datagram handler should not commit any state changes. But I agree, that maybe it would be good to mention this explicitly in the specs (maybe in ICS26? I cannot remember from the top of my head if we already do that...
// the packet timed-out, so refund the tokens | ||
refundTokens(packet) | ||
// check if the packet sent is from a previously forwarded packet | ||
prevPacket = privateStore.get(packetForwardPath(packet.sourcePort, packet.sourceChannel, packet.sequence)) |
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 have an observation on the naming here from reading this quite fresh:
It took me quite a bit of time and mental gymnastic to get the packets straight in my head here (to be fair, this could just be because I am not so used to this).
In particular, I found it tricky to map all these names to the right context:
- Forwarded packet
- Previous packet
- Sent packet
- Received packet
- Packet (in the context of onTimeout, onAck, OnRecv)
At first I thought it was an error that the timeout packet was sent in to revertInFlightChanges
as sentPacket, because "surely the forwarded packet, aka prevPacket was the one we sent". But after rereading, I realized that forwardedPacket was not actually received packet. So it is correct, of course, but with all these names it was not so easy to make a mental model of the different packets.
Not sure what the best way to deal with this is, but perhaps there is a way we could tweak the names to make it a bit easier to grasp on first read.
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.
This is good feedback; thanks, @gjermundgaraba. Maybe a diagram would also help; I can try to draw something in the next couple of days.
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.
Thank you for the feedback @gjermundgaraba . Will mull over better naming, but if you have some suggestions feel free to share!
@@ -35,7 +35,7 @@ The IBC handler interface & IBC routing module interface are as defined in [ICS | |||
|
|||
### Data Structures | |||
|
|||
Only one packet data type is required: `FungibleTokenPacketData`, which specifies the denomination, amount, sending account, and receiving account or `FungibleTokenPacketDataV2` which specifies multiple tokens being sent between sender and receiver. A v2 supporting chain can optionally convert a v1 packet for channels that are still on version 1. | |||
Only one packet data type is required: `FungibleTokenPacketData`, which specifies the denomination, amount, sending account, and receiving account or `FungibleTokenPacketDataV2` which specifies multiple tokens being sent between sender and receiver along with a forwarding path that can forward tokens further beyond the initial receiving chain. A v2 supporting chain can optionally convert a v1 packet for channels that are still on version 1. |
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 we should state that this is optional explicitly.
Only one packet data type is required: `FungibleTokenPacketData`, which specifies the denomination, amount, sending account, and receiving account or `FungibleTokenPacketDataV2` which specifies multiple tokens being sent between sender and receiver along with a forwarding path that can forward tokens further beyond the initial receiving chain. A v2 supporting chain can optionally convert a v1 packet for channels that are still on version 1. | |
Only one packet data type is required: `FungibleTokenPacketData`, which specifies the denomination, amount, sending account, and receiving account or `FungibleTokenPacketDataV2` which specifies multiple tokens being sent between sender and receiver along with an optional forwarding path that can forward tokens further beyond the initial receiving chain. A v2 supporting chain can optionally convert a v1 packet for channels that are still on version 1. |
interface ForwardingInfo { | ||
hops: []Hop | ||
memo: string, | ||
} |
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.
Personally, I feel like the memo in FungibleTokenPacketData
should cover this, and that we should not include a memo field in this data structure.
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.
There would also be overlap between ForwardingInfo
/ Hop
and the Trace
type which includes the port id and channel id tuple.
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.
Personally, I feel like the memo in FungibleTokenPacketData should cover this, and that we should not include a memo field in this data structure.
I understand the concern here is preventing unintentional middleware invocation from source chain or intermediary recving chain, but also thought about what if you can rather have a memo attached to each individual hop.
Spoke about this with @colin-axner and fine to keep this structure as introducing this on a per-hop basis is opening up a big rabbit hole of potential issues.
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.
This is looking great! Had just a few small questions/comments
Token{ | ||
denom: uatom, | ||
amount: 500, | ||
trace: ["transfer/channel-1", "transfer/channel-4"], |
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 believe the trace needs to be updated to the new type trace
[
{
"portID": "transfer",
"channelID": "channel-1"
},
{
"portID": "transfer",
"channelID": "channel-4"
},
]
The Hop
type I guess could also be replaced with this type as it is functionally the same.
|
||
#### Packet forward path | ||
|
||
Those `v2` packets that have non-empty forwarding information and should thus be forwarded, must be stored in the private store, so that an acknowledgement can be written for them when receiving an acknowledgement or timeout for the forwarded packet. |
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.
Those `v2` packets that have non-empty forwarding information and should thus be forwarded, must be stored in the private store, so that an acknowledgement can be written for them when receiving an acknowledgement or timeout for the forwarded packet. | |
The `v2` packets that have non-empty forwarding information and should thus be forwarded, must be stored in the private store, so that an acknowledgement can be written for them when receiving an acknowledgement or timeout for the forwarded packet. |
hops: forwardingPath.hops[1:] | ||
memo: forwardingPath.memo | ||
} | ||
if forwardingPath.hops == 1 { |
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.
if forwardingPath.hops == 1 { | |
if len(forwardingPath.hops) == 1 { |
// we're on the last hop, we can set memo and clear | ||
// the next forwardingPath | ||
memo = forwardingPath.memo | ||
nextForwardingPath = nil |
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.
stylistic nit here, rather than re-assigning to nil and using that as a special value, I think it would be a bit nicer to always have a non nil type here, and have something like a forwardingPath.hasNext()
or forwardingPath.lastHop()
or some sort of other mechanism which examines the state of the hops and can be used.
@@ -92,6 +93,7 @@ The fungible token transfer bridge module tracks escrow addresses associated wit | |||
```typescript | |||
interface ModuleState { | |||
channelEscrowAddresses: Map<Identifier, string> | |||
channelForwardingAddresses: Map<Identifier, string> |
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.
If the only reason for using this is a logical separation, I think I would be in favour of simply using the escrow address. Having this other set of addresses adds to the overall complexity without adding a huge amount of value IMO.
And like @damiannolan is linking, the existing escrow address has already been reasoned about in terms of collisions.
Allows users to atomically route their tokens through a series of paths with a single packet.
Desired properties:
Special thanks to the @strangelove-ventures team for working on the PFM middleware which serves as a precursor to this feature