Skip to content
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

Refund user when ExitToNear promise fails #311

Merged
merged 1 commit into from
Nov 9, 2021
Merged

Conversation

birchmd
Copy link
Member

@birchmd birchmd commented Oct 18, 2021

Closes #269

The solution we agreed on was the one suggested by @mfornet where we change the ExitToNear precompile API to accept an address to refund the assets to in case the exit promise fails. This is a breaking change, however, to allow this PR to be merged without breaking anything the API change is hidden behind a compile-time feature flag. @mfornet @sept-en and I will need to coordinate on migrating the existing bridged ERC-20 assets and enabling this feature. After this feature is enabled by default then we can clean up the code to remove the old logic.

This PR is relatively large, so guide the reviewer I draw your attention to the following pieces:

  • The main logic change is of course in engine-precompiles/src/native.rs; the refund itself happens in the new function in engine/src/lib.rs
  • I've added a few new tests to engine-tests/src/tests/erc20_connector.rs, however because these tests rely on resolving promises they could not use the AuroraRunner testing utility which existing tests used. Therefore, there is a great deal of boilerplate code added as well to wrap up near-sdk-sim calls. I recommend simply focusing on the tests themselves (in particular the assert statements show how the balances behave with and without this refund mechanism).
  • The whole test suite passes with and without the new feature enabled. You can check this locally but running make check and make check error-refund=yes. As a sanity test you could also check locally that various tests fail if you do not consistently enable the feature (i.e. if you compile the contract with the new feature, but then run the tests without it or vice versa).
  • There are two "new" solidity contracts, but really they are the old contracts with small modifications for the API change. Unfortunately I do not know if conditional compilation is a thing in solidity, so I had to duplicate the whole contract. Below is the diff
$ diff etc/eth-contracts/contracts/test/Tester.sol etc/eth-contracts/contracts/test/TesterV2.sol
6c6
< contract Tester {
---
> contract TesterV2 {
44,45c44,46
<         bytes memory input = abi.encodePacked("\x00", recipient);
<         uint input_size = 1 + recipient.length;
---
>         address sender = msg.sender;
>         bytes memory input = abi.encodePacked("\x00", sender, recipient);
>         uint input_size = 1 + 20 + recipient.length;


$ diff etc/eth-contracts/contracts/EvmErc20.sol etc/eth-contracts/contracts/EvmErc20V2.sol
15c15
< contract EvmErc20 is ERC20, AdminControlled, IExit {
---
> contract EvmErc20V2 is ERC20, AdminControlled, IExit {
52c52,53
<         _burn(_msgSender(), amount);
---
>         address sender = _msgSender();
>         _burn(sender, amount);
55,56c56,57
<         bytes memory input = abi.encodePacked("\x01", amount_b, recipient);
<         uint input_size = 1 + 32 + recipient.length;
---
>         bytes memory input = abi.encodePacked("\x01", sender, amount_b, recipient);
>         uint input_size = 1 + 20 + 32 + recipient.length;

@birchmd birchmd added C-enhancement Category: New feature or request P-high Pririoty: high A-connector Area: Issues that relate to the connector. A-testing Area: If something has added tests, or changed them. A-fungible-token Area: Fungible token related. labels Oct 18, 2021
@birchmd birchmd linked an issue Oct 18, 2021 that may be closed by this pull request
Copy link
Contributor

@joshuajbouw joshuajbouw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding comments for now, I'm going to need a fresh start in the morning to go through this.

engine-precompiles/src/native.rs Show resolved Hide resolved
engine-precompiles/src/native.rs Show resolved Hide resolved
@joshuajbouw joshuajbouw added C-BREAKING Category: Breaking public API or RPC changes and removed C-BREAKING Category: Breaking public API or RPC changes labels Nov 4, 2021
engine-precompiles/src/native.rs Show resolved Hide resolved
engine-precompiles/src/native.rs Show resolved Hide resolved
engine-precompiles/src/native.rs Outdated Show resolved Hide resolved
engine-precompiles/src/native.rs Show resolved Hide resolved
engine/src/engine.rs Outdated Show resolved Hide resolved
engine/src/lib.rs Show resolved Hide resolved
engine/src/lib.rs Show resolved Hide resolved
engine/src/lib.rs Outdated Show resolved Hide resolved
engine/src/lib.rs Outdated Show resolved Hide resolved
);
fn schedule_promise(promise: PromiseCreateArgs) -> u64 {
sdk::log!(&crate::prelude::format!(
"Call contract: {}.{}",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfornet there are tests that depend on the specific format of this log. Do we use it in production as well? If not I will change to harmonize with other logs (ie underscores in the label and only space (no colon) as the separator with the value).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change them to fit the guideline

Copy link
Contributor

@mfornet mfornet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for PR, and sorry for late review. Everything is good here.

What is the plan ahead regarding upgrades to the ERC20 contracts 🤔 . The two paths ahead I'm seeing are:

  • Deploy the new contract on the old address.
  • Deploy the new binaries on different address; move all balances from one to the other; update the erc20-nep141 mapping.

The first one is easier, but I'm concerned about this breaking some assumptions about how this particular tokens were deployed. What do you think?

In any case we should plan ahead, and have this contracts to be upgradable. So in case we need to do an upgrade in the future it is easier. We can set the owner to be aurora account. This is not blocking this PR.

@birchmd birchmd merged commit 0f87233 into develop Nov 9, 2021
@birchmd birchmd deleted the 269-handle-exit-errors branch November 9, 2021 18:16
@birchmd
Copy link
Member Author

birchmd commented Nov 9, 2021

Thanks for PR, and sorry for late review. Everything is good here.

What is the plan ahead regarding upgrades to the ERC20 contracts 🤔 . The two paths ahead I'm seeing are:

  • Deploy the new contract on the old address.
  • Deploy the new binaries on different address; move all balances from one to the other; update the erc20-nep141 mapping.

The first one is easier, but I'm concerned about this breaking some assumptions about how this particular tokens were deployed. What do you think?

In any case we should plan ahead, and have this contracts to be upgradable. So in case we need to do an upgrade in the future it is easier. We can set the owner to be aurora account. This is not blocking this PR.

I'm not sure which is the better option. Perhaps @sept-en would like to weigh in.

@joshuajbouw joshuajbouw mentioned this pull request Dec 10, 2021
artob pushed a commit that referenced this pull request Dec 10, 2021
* Feat(engine): London hard fork support (#244)
* Fix(exit precompile): Address to refund in case of error is an argument (#311)
* Feat(engine): Make engine parametric in storage access (#314)
* Test verifying the EVM log returns the correct address (#341)
* Remove sdk::current_account_id usage from engine-precompiles (#346)
* Remove Default trait bound from engine IO (#342)
* Remove some sdk usage from core logic (#347)
* Factor out blockchain environment variable access as a trait (#349)
* Factor out NEAR promise host functions into a trait (#353)
* Borsh deserialized value field for call args (#351)
* Refactor(eth-connector): Use Result return values instead of panicking (#355)
* Gate all NEAR host functions behind the contract feature (#356)
* Bump @openzeppelin/contracts from 4.3.2 to 4.3.3 in /etc/eth-contracts
* Chore: Newtypes for gas (#344)
* Feat(standalone): Standalone (#345)
* Minor fixes to sdk refactor (#359)
* Refactor(engine): Move submit logic into engine module (#366)
* Feat(standalone): Storage backend (#375)
* NEAR random numbers from solidity contract (#368)
* Feat(standalone): EVM tracing via SputnikVM (#383)
* Feat(standalone): Bootstrap storage from relayer and state snapshots (#379)
* Feat(standalone): Structures and logic for keeping storage in sync with the blockchain (#382)
* Feat(standalone): Capture geth-like tracing from SputnikVM events (#384)
* Connector cleanup (#374)
* Remove betanet
* Fix(engine): original_storage bug fix; more tracing tests (#390)
* Increase NEAR Gas for ft_on_transfer (#389)

Co-authored-by: Andrew Bednoff <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Evgeny Ukhanov <[email protected]>
Co-authored-by: Marcelo Fornet <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-connector Area: Issues that relate to the connector. A-fungible-token Area: Fungible token related. A-testing Area: If something has added tests, or changed them. C-enhancement Category: New feature or request P-high Pririoty: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle errors when sending tokens within Aurora to NEAR
3 participants