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