-
Notifications
You must be signed in to change notification settings - Fork 82
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
fix: fix in the exit_to_eth precompile #834
Conversation
engine-precompiles/src/native.rs
Outdated
@@ -537,7 +533,7 @@ impl<I: IO> Precompile for ExitToEthereum<I> { | |||
.try_into() | |||
.map_err(|_| ExitError::Other(Cow::from("ERR_INVALID_RECIPIENT_ADDRESS")))?; | |||
( | |||
self.current_account_id.clone(), | |||
get_eth_connector_contract_account(&self.io)?, | |||
// There is no way to inject json, given the encoding of both arguments | |||
// as decimal and hexadecimal respectively. | |||
WithdrawCallArgs { |
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.
One more change is needed here. As @karim-en pointed out in our call, the Engine Withdraw used Borsh serialization for the arguments and so does the new eth-connector, however regular NEP-141 tokens use JSON (see ERC-20 implementation below).
I think there are two options for what to do:
- Change the eth-connector to use JSON args for withdraw, thus making it consistent with the other bridged NEP-141 tokens, and then make the corresponding change here.
- Add another config to the Engine to say what format the withdraw arguments are supposed to be in.
I personally think 1 is the better solution.
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 also like the first option, but this change should be backward compatible, because this change can break any services or contracts that interact with the engine contract.
To make it backward compatible I think we can try to deserialise the input twice, as an borsh, and then try to deserialise it as a json if it was failed here
PS: I've considered just the scenario where the eth-base silo points to the current build-in eth connector, not the separate/new one. For the new connector we can support just the json input.
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.
@birchmd what existing problem or business goal is it to solve by introducing changes that break the existing API? I do not have enough context to understand the essence of such a controversial proposal.
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.
@karim-en, what is the JSON format for this message should be?
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.
@karim-en Backwards compatibility is maintained by the main Aurora Engine withdraw
(which delegates to the new connector's engine_withdraw
) remaining borsh-encoded. I don't think we need fallback serialization on the new withdraw
method of the new connector.
@aleksuss the format will be identical to the ERC-20 case.
@mrLSD This is not a breaking change to the existing API because the Aurora Engine maintains backwards compatibility. The reason to API of the new contract to be different from Aurora Engine is because then it is consistent with all other bridged NEP-141 tokens (i.e. all ERC-20 tokens bridged from Ethereum). All NEP-141 tokens having a consistent interface allows us to use any bridged token as the base token for a silo, not just ETH. Having alternate base tokens is a business requirement.
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.
@birchmd don't we need to support both versions of deserialization in the main aurora contract in the withdraw
function? In case we will use the main contract as an eth-connector for using ETH as the base token in silo?
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.
Good point! Yes, we may need to do that in the short term. Maybe then option 2 from my initial comment is the better solution for now. In the future I still think it would be good for there to be one consistent withdraw API.
engine-precompiles/src/native.rs
Outdated
@@ -537,7 +533,7 @@ impl<I: IO> Precompile for ExitToEthereum<I> { | |||
.try_into() | |||
.map_err(|_| ExitError::Other(Cow::from("ERR_INVALID_RECIPIENT_ADDRESS")))?; | |||
( | |||
self.current_account_id.clone(), | |||
get_eth_connector_contract_account(&self.io)?, | |||
// There is no way to inject json, given the encoding of both arguments | |||
// as decimal and hexadecimal respectively. | |||
WithdrawCallArgs { |
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.
@birchmd what existing problem or business goal is it to solve by introducing changes that break the existing API? I do not have enough context to understand the essence of such a controversial proposal.
d222146
to
d3baec9
Compare
d3baec9
to
3b0b713
Compare
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.
LGTM
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.
LGTM
## Release 3.2.0 ### Changes - Changed structure `SetEthConnectorContractAccountArgs` for setting eth connector account. It was extended with additional field: `withdraw_serialize_type` for defining serialization type for withdraw arguments by [@aleksuss]. ([#834]) - Updated rocksdb up to 0.21.0 by [@aleksuss]. ([#840]) ### Additions - Added a possibility of mirroring deployed ERC-20 contracts in the main Aurora contract in Silo mode by [@aleksuss]. ([#844]) - Allow to initialize hashchain directly with the `new` method by [@birchmd]. ([#846]) - Added a silo logic which allows to set fixed gas costs per transaction by [@aleksuss]. ([#746]) - Added a new type of transaction which allows to add full access key into account of the smart contract by [@aleksuss]. ([#847]) [#746]: #746 [#834]: #834 [#840]: #840 [#844]: #844 [#846]: #846 [#847]: #847 --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: i-fix-typos <[email protected]> Co-authored-by: Evgeny Ukhanov <[email protected]> Co-authored-by: Michael Birch <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Karim <[email protected]> Co-authored-by: Joshua J. Bouw <[email protected]> Co-authored-by: ForwardSlashBack <[email protected]>
## Release 3.2.0 ### Changes - Changed structure `SetEthConnectorContractAccountArgs` for setting eth connector account. It was extended with additional field: `withdraw_serialize_type` for defining serialization type for withdraw arguments by [@aleksuss]. ([#834]) - Updated rocksdb up to 0.21.0 by [@aleksuss]. ([#840]) ### Additions - Added a possibility of mirroring deployed ERC-20 contracts in the main Aurora contract in Silo mode by [@aleksuss]. ([#844]) - Allow to initialize hashchain directly with the `new` method by [@birchmd]. ([#846]) - Added a silo logic which allows to set fixed gas costs per transaction by [@aleksuss]. ([#746]) - Added a new type of transaction which allows to add full access key into account of the smart contract by [@aleksuss]. ([#847]) [#746]: #746 [#834]: #834 [#840]: #840 [#844]: #844 [#846]: #846 [#847]: #847 --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: i-fix-typos <[email protected]> Co-authored-by: Evgeny Ukhanov <[email protected]> Co-authored-by: Michael Birch <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Karim <[email protected]> Co-authored-by: Joshua J. Bouw <[email protected]> Co-authored-by: ForwardSlashBack <[email protected]>
Description
The PR fixes the exit to ethereum precompile.
Performance / NEAR gas cost considerations
There are no changes in performance.
Testing
There are no tests for testing this part of the code, but it would be nice to have.