-
Notifications
You must be signed in to change notification settings - Fork 107
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
Improve Gateway API for sending tokens to Polkadot #1009
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1009 +/- ##
==========================================
- Coverage 80.98% 80.84% -0.14%
==========================================
Files 52 52
Lines 2161 2156 -5
Branches 72 74 +2
==========================================
- Hits 1750 1743 -7
- Misses 396 397 +1
- Partials 15 16 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Nice consolidation!
} | ||
|
||
enum Kind { | ||
Index, |
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.
What is this Index
kind? A parachain ID?
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 you install pallet_indices on a substrate chain, then its possible for the chain to map a u32
to a AccountId
under the hood.
So its basically a "short" id for accountid.
In fact Substrate allows all sorts of mappings:
https://paritytech.github.io/polkadot-sdk/master/sp_runtime/enum.MultiAddress.html
if (destinationAddress.isAddress32()) { | ||
payload = SubstrateTypes.SendTokenToAssetHubAddress32(token, destinationAddress.asAddress32(), amount); | ||
} else { | ||
revert Unsupported(); | ||
} | ||
} else { | ||
if (destinationAddress.isAddress32()) { | ||
payload = SubstrateTypes.SendTokenToAddress32( | ||
token, destinationChain, destinationAddress.asAddress32(), amount | ||
); | ||
} else if (destinationAddress.isAddress20()) { | ||
payload = SubstrateTypes.SendTokenToAddress20( | ||
token, destinationChain, destinationAddress.asAddress20(), amount | ||
); | ||
} else { | ||
revert Unsupported(); | ||
} |
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.
What if the destinationAddress
is Index
?
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.
That's not supported right now, requires more plumbing on the BridgeHub side, to convert the message into the right XCM.
Not many parachains use pallet_indices
anyway.
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.
Cool!
token, destinationChain, destinationAddress.asAddress32(), amount | ||
); | ||
} else if (destinationAddress.isAddress20()) { | ||
payload = SubstrateTypes.SendTokenToAddress20( |
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.
ooh will this make it easier for native Moonbeam support?
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.
ah i also see we had it already, so just a refactor
The previous API was messy, and had duplicate
sentToken
functions in order handle different recipient address types.I've changed my mind on this design, and have introduced
MultiAddress
type, which encapsulates different substrate address types.