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.
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
ICS20-2: Add Token Forwarding ability to ICS20-2 #1090
Changes from 17 commits
a40890b
b10ba74
1de6ca0
675ccd8
6b565d0
78e06a4
49f4b5d
b702942
3fcb972
a278f1b
4e9547e
a42ea6e
2d98d83
eb6fbf5
ab34a49
055ae83
2e0f5da
a759ca6
2a71fbd
0b4f63f
eb6bd36
ec5fcc3
89aec20
58b3462
77414dc
712721b
c667418
e613e77
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
I agree we could use directly
channelEscrowAddresses
. ThechannelEscrowAddresses
mapping is populated duringonChanOpenInit
at line 157 oronChainOpenTry
at line 181. That means we could potentially access this mapping at line 331 using thepacket.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.
note cosmos/cosmos-sdk#7737 (comment)
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.
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.
The forwarding address is mainly just used as pass through storage based on how ics20 is constructed in the non-forwarding case. I think the spec just needs to specify that this address should be an address the transfer module controls. I don't think it should reuse the escrow addresses
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, I can't think of a reason why you need channel isolated forwarding addresses cosmos/ibc-go#6561 (comment)
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.
The reason I decided to split it off to a separate address is just an additional security blanket. The balance of the escrow addresses is security critical. If the balance ends up in an invalid state, that can be a very painful issue to unwind.
It should be completely fine if there's no bugs. But if there's a bug in forwarding, i figure having a separate account might make the impact a bit more isolated.
Of course it is receiving from and sending back to the escrow addresses, so if there's issues there it could still be a problem for escrow account balances.
I'm ok either way. I just figured to start out with a separate account for clarity and avoiding having the escrow account representing more than one thing semantically.
We can use the same account as well as @colin-axner suggests
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:
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:
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?
But then it would not be possible to trigger any action on intermediary hops. Is that desired behaviour?
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...
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()
orforwardingPath.lastHop()
or some sort of other mechanism which examines the state of the hops and can be used.