Skip to content

EthConnector: don't burn NEP-141 on deposit.#133

Merged
artob merged 2 commits intodevelopfrom
eth-connector-remove-burn
Jun 11, 2021
Merged

EthConnector: don't burn NEP-141 on deposit.#133
artob merged 2 commits intodevelopfrom
eth-connector-remove-burn

Conversation

@sept-en
Copy link
Copy Markdown
Contributor

@sept-en sept-en commented Jun 9, 2021

We used to have the following workflow for depositing ETH to Aurora:

  • Deposit nETH (NEP-141) tokens to Aurora account
  • Burn Aurora's nETH tokens
  • Mint ETH to Ethereum address on Aurora.

When we call exitToNear precompile for ETH, we expect to burn ETH on Aurora and make ft_transfer of nETH tokens from Aurora to the recipient on NEAR. The problem using the previously mentioned deposit workflow is that we don't have nETH tokens available on Aurora account when we try to perform this exit.

Possible solutions:

1) Don't burn nETH tokens on Aurora account during the deposit

This solution is currently implemented in the PR.

Pros:

  • We have needed liquidity and keep calling just a regular ft_transfer when calling exitToNear precompile for ETH.
  • We have consistency with the approach that we use for ERC20 connector for Aurora (User transfers some NEP-141 tokens to Aurora account and we mint appropriate ERC20 tokens without burning NEP-141)
  • We don't need to add any additional functionality to the contract interface and this is quite straightforward.

Cons:

  • The balance of total_balance_near (which should be renamed to total_balance_nep141 or something similar) will grow and will represent both nETH and ETH tokens, which might be not the thing we want.
  • The balance of total_balance will be the sum of total_balance_near and total_balance_eth. And in this way is not really something meaningful.

2) Add needed private functions to Aurora contract

Add the private function self_mint_and_ft_transfer (and self_mint_and_withdraw for exitToEthereum) that will be called only from the appropriate precompile for ETH. The function will first mint the needed amount of nETH to Aurora account and then perform a regular ft_transfer to the recipient.

Pros:

  • total_supply_near, total_supply_eth, total_supply values remain meaningful and we don't have any redundant balance for nETH that we keep in the contract.
  • nETH tokens are minted only when the appropriate precompile is executed and immediately transferred to the recipient.

Cons:

  • We have a different approach comparing to that we use when bridging NEP141->ERC20 on Aurora (this was described in the 1st approach).
  • We introduce another one point where we mint some tokens -> This is another point for failure and needs a thorough review.

I expect that the 1st approach more likely will be accepted thus I implemented this here.
However, I'm more into 2nd approach so I think it worth discussing.

NB: this needs to be solved for both ETH exits: exitToNear, exitToEthereum.

@birchmd
Copy link
Copy Markdown
Member

birchmd commented Jun 9, 2021

The cons of the first approach seem to primarily be bookkeeping related. If we correct for the double-counting in total_supply in any public getters for example then this effectively mitigates the drawback as far as I can tell. On the other hand, cons of the second approach seem to be more complexity related and I think it's better to reduce complexity where possible.

@mfornet
Copy link
Copy Markdown
Contributor

mfornet commented Jun 9, 2021

Strongly prefer first approach. Being consistent and having a "simple" solution seems a bigger win. The cons are related to the way balances are being displayed, here we have the freedom to update them to show expected values, or educated developers consuming this interface.

@sept-en sept-en requested a review from mrLSD June 10, 2021 00:08
Copy link
Copy Markdown
Member

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

One question I have about changing the behaviour here: what happens to any transfers than have already happened? Can they never exit?

Comment thread src/connector.rs Outdated
Copy link
Copy Markdown
Member

@mrLSD mrLSD left a comment

Choose a reason for hiding this comment

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

Implemented solution looks clear & simple.

Copy link
Copy Markdown
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.

Good here. Straight forward.

@artob artob self-assigned this Jun 11, 2021
@artob artob added C-enhancement Category: New feature or request P-critical Priority: critical labels Jun 11, 2021
@artob artob merged commit 94cb338 into develop Jun 11, 2021
@artob artob deleted the eth-connector-remove-burn branch June 11, 2021 10:16
@artob
Copy link
Copy Markdown
Contributor

artob commented Jun 11, 2021

One question I have about changing the behaviour here: what happens to any transfers than have already happened? Can they never exit?

@sept-en Please do remember to answer @birchmd's question here.

@sept-en
Copy link
Copy Markdown
Contributor Author

sept-en commented Jun 11, 2021

@artob Sorry, forgot to answer it here.

One question I have about changing the behaviour here: what happens to any transfers than have already happened? Can they never exit?

@birchmd That's correct. This is not an issue for us because ETH connector hasn't been deployed to Mainnet yet, so there we will start from scratch. On Testnet I solved it just by bridging some Ropsten directly to develop.aurora account (that I use for testing). For sure on Mainnet this won't be an option, but it's good that we don't have ETH connector here and don't need to implement any specific balance migration there.

artob added a commit that referenced this pull request Jun 17, 2021
* Introduce precompiles for the ETH & ERC-20 connectors. (#51)
* Implement generational storage with `SELFDESTRUCT` tests. (#84)
* Remove the dependency on Lunarity. (#115)
* Fix Clippy complaint with `+nightly`. (#117)
* Simplify the `sdk::read_u64` return type. (#118)
* Add an `is_used_proof` interface. (#120)
* Add an `evm-bully=yes` build to CI. (#121)
* Handle transaction gas limit properly. (#123)
* Fix u128 JSON parsing & tests in the ETH connector. (#125)
* Fix evm-bully builds. (#130)
* Add JSON custom error types. (#131)
* Don't burn NEP-141 on deposit. (#133)
* Fix needless borrows. (#135)
* Improve and refactor the ETH connector. (#136)
* Add a macro for logging. (#142)

Co-authored-by: Aleksey Kladov <aleksey@near.org>
Co-authored-by: Arto Bendiken <arto@aurora.dev>
Co-authored-by: Evgeny Ukhanov <evgeny@aurora.dev>
Co-authored-by: Frank Braun <frank@aurora.dev>
Co-authored-by: Joshua J. Bouw <joshua@aurora.dev>
Co-authored-by: Kirill <kirill@aurora.dev>
Co-authored-by: Marcelo Fornet <marcelo@aurora.dev>
Co-authored-by: Michael Birch <michael@aurora.dev>
artob added a commit that referenced this pull request Jun 17, 2021
* Introduce precompiles for the ETH & ERC-20 connectors. (#51)
* Implement generational storage with `SELFDESTRUCT` tests. (#84)
* Fix u128 JSON parsing & tests in the ETH connector. (#125)
* Add JSON custom error types. (#131)
* Don't burn NEP-141 on deposit. (#133)
* Fix needless borrows. (#135)
* Improve and refactor the ETH connector. (#136)
* Add a macro for logging. (#142)

Co-authored-by: Aleksey Kladov <aleksey@near.org>
Co-authored-by: Arto Bendiken <arto@aurora.dev>
Co-authored-by: Evgeny Ukhanov <evgeny@aurora.dev>
Co-authored-by: Frank Braun <frank@aurora.dev>
Co-authored-by: Joshua J. Bouw <joshua@aurora.dev>
Co-authored-by: Kirill <kirill@aurora.dev>
Co-authored-by: Marcelo Fornet <marcelo@aurora.dev>
Co-authored-by: Michael Birch <michael@aurora.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-enhancement Category: New feature or request P-critical Priority: critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants