-
Notifications
You must be signed in to change notification settings - Fork 19
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
P 1275 omni bridge precompile #3254
Conversation
only ts-test require small adjust. All other code should be ready fro review. |
fn pay_in( | ||
handle: &mut impl PrecompileHandle, | ||
amount: U256, | ||
dest_id: u8, |
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.
We can live with it for now - is there a more general way of mapping the ChainType
in pallet code?
Imagine we want to bridge to some non-evm chain later
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.
Then we should define the type id for all different types of chain category, and hard code mapping it in precompile.
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.
for evm interface, adding one extra parameter by that time.
amount: U256, | ||
dest_id: u8, | ||
native: bool, | ||
asset_id: U256, |
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 the same problem:
we either have native == true
, or non-native with asset_id
, it's actually an enum
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.
in evm contract, there is no ethereum struct can 100% consistently support rust enum.
we have { native, id(u32) }, this enum in rust carry extra parameter u32.
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.
dest_id share this problem too.
{ heima, Ethereum(chain_id) } carry extra chain_id info.
@0xverin ideally you could take a look at the test too |
…tentry/litentry-parachain into p-1275-omni-bridge-precompile
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.
Ts tests part looks good!
Still confirming ts-test, let me merge manually later. |
Yes, it is caused by failed to decode event data. ready to merge. TODO fix in future. |
omni bridge precompile