Eth-connector: change `total_supply` behaviour. Refactoring. by sept-en · Pull Request #136 · aurora-is-near/aurora-engine

sept-en · 2021-06-10T17:46:12Z

This is a follow-up to #133 PR. This is required as that PR contains needed changes so we won't fix tests twice just to make CI pass. Once that PR is merged, the current one needs to be rebased on top of develop branch.

Following our discussion with @mfornet, we need to change the ft_total_supply meaning to correspond to the NEP-141 standard. Also, some other refactoring was required as many members of the team frequently get confusing with ft_total_supply_near thinking that this represents bridged Near to Aurora (which we don't have) and other similar things.

Please note, that this PR most likely will require state migration as inner fields of the FungibleToken structure were renamed. However, this should be ok on Mainnet as we don't have an eth-connector available there.

Changes:

ft_total_supply() now represents the amount of nETH (ETH NEP-141 tokens on NEAR) to correspond to NEP-141 standard.
FungibleToken: rename total_supply field -> total_eth_supply_on_near.
FungibleToken: rename total_supply_eth field -> total_eth_supply_on_aurora.
Rename ft_total_supply_near() -> ft_total_eth_supply_on_near().
Rename ft_total_supply_eth() -> ft_total_supply_eth_on_near().
Rename finish_deposit_near() -> finish_deposit().
deposit() method refactoring.
Minor refactoring.
Fix tests according to changes and refactoring.

mfornet

I appreciate this level of verbosity for function names. Using functions like ft_total_supply_near was not useful to understand what was happening there.

artob · 2021-06-11T10:21:16Z

This is a follow-up to #133 PR. This is required as that PR contains needed changes so we won't fix tests twice just to make CI pass. Once that PR is merged, the current one needs to be rebased on top of develop branch.

@sept-en I've merged #133, so please proceed with the rebase here.

* `ft_total_supply()` now represents the amount of nETH (ETH NEP-141 tokens on NEAR) to correspond to NEP-141 standard. * Rename `ft_total_supply_near()` -> `ft_total_eth_supply_on_near()`. * Rename `ft_total_supply_eth()` -> `ft_total_supply_eth_on_near()`. * Rename `finish_deposit_near()` -> `finish_deposit()`. * `deposit()` method refactoring. * Minor refactoring. * Fix tests according to changes and refactoring.

Co-authored-by: Marcelo Fornet <mfornet94@gmail.com>

sept-en · 2021-06-11T11:32:04Z

@artob rebase is finished.

sept-en · 2021-06-11T12:58:13Z

Update:
@mfornet pointed out that as FungibleToken is being borsh-serialized when we save it to storage, it seems that we should fine and don't need any storage migration (as we neither added any new fields nor changed the order of variables in the current PR).

birchmd

LGTM! Thanks for this!

mrLSD · 2021-06-12T13:25:41Z

@sept-en @mfornet
I think this name is unfortunate:

*_on_near
*_on_aurora

But in reality it means:

*_for_near_account
*_for_eth_account

For me, this naming is misleading.

However, if it blocks somehow release, I will do approve immediately. Just let me know.

sept-en · 2021-06-12T13:52:39Z

@mrLSD for_eth_account is not really concise suffix because it's not really clear whether it's about Aurora or Ethereum eth account.

If we look at the Near and Aurora as separate entities (not taking into account that Aurora EVM is inside the Near account), we can say that there are no Aurora accounts on Near and vice versa.

So I think the naming is quite clear if we get used to it.
Moreover, we already use similar naming in EthCustodian for several months already.

mrLSD · 2021-06-12T14:17:04Z

If we look at the Near and Aurora as separate entities (not taking into account that Aurora EVM is inside the Near account), we can say that there are no Aurora accounts on Near and vice versa.

For me, it's not clear how it affects current accounts balance logic.

We deposit via Proof to Aurora Near accounts
We deposit via Proof to Aurora Eth accounts
Aurora doesn't have its own accounts and account management itself. Taking this into account, the proposed naming for me does not reflect this logic.

So I think the naming is quite clear if we get used to it.
Moreover, we already use similar naming in EthCustodian for several months already.

It doesn't mean that if we already use that naming anywhere that naming is clear.

joshuajbouw

Straight forward. Thanks for this.

…gine into eth-connector-total-supply-changes-and-refactoring

mfornet

Great, much easier to use this names.

artob · 2021-06-17T08:51:15Z

@sept-en Please fix the merge conflict (due to #51) and then I will merge.

…gine into eth-connector-total-supply-changes-and-refactoring

sept-en · 2021-06-17T10:56:36Z

Aurora doesn't have its own accounts and account management itself. Taking this into account, the proposed naming for me does not reflect this logic.

The idea for these names is to represent these balances from a top-level conceptual point of view. We don't want to have very detailed implementation specifics represented in function names. I think having top-level abstractions is a good approach.

sept-en · 2021-06-17T10:56:50Z

@artob merged. Please check!

* 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>

* 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>

sept-en added C-enhancement Category: New feature or request P-critical Priority: critical labels Jun 10, 2021

sept-en requested review from artob, birchmd, joshuajbouw, mfornet and mrLSD June 10, 2021 17:46

mfornet reviewed Jun 11, 2021

View reviewed changes

Comment thread doc/eth-connector.md Outdated

Comment thread src/fungible_token.rs Outdated

Comment thread src/fungible_token.rs Outdated

Comment thread src/fungible_token.rs Outdated

Comment thread src/fungible_token.rs Outdated

sept-en and others added 5 commits June 11, 2021 14:26

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

0f92d78

Remove method

73f3fcf

Update src/fungible_token.rs.

bc747fd

Co-authored-by: Marcelo Fornet <mfornet94@gmail.com>

Review fixes.

87829de

sept-en force-pushed the eth-connector-total-supply-changes-and-refactoring branch from 2702729 to 87829de Compare June 11, 2021 11:30

sept-en requested a review from mfornet June 11, 2021 11:31

birchmd approved these changes Jun 11, 2021

View reviewed changes

joshuajbouw approved these changes Jun 14, 2021

View reviewed changes

Merge branch 'develop' of https://github.com/aurora-is-near/aurora-en…

cad50d6

…gine into eth-connector-total-supply-changes-and-refactoring

artob assigned mfornet Jun 17, 2021

mfornet approved these changes Jun 17, 2021

View reviewed changes

artob assigned artob and unassigned mfornet Jun 17, 2021

Minor fixes

c91a87c

artob approved these changes Jun 17, 2021

View reviewed changes

Merge branch 'develop' of https://github.com/aurora-is-near/aurora-en…

37ed2a1

…gine into eth-connector-total-supply-changes-and-refactoring

artob merged commit 1007ce2 into develop Jun 17, 2021

artob deleted the eth-connector-total-supply-changes-and-refactoring branch June 17, 2021 11:15

Conversation

sept-en commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfornet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artob commented Jun 11, 2021

Uh oh!

sept-en commented Jun 11, 2021

Uh oh!

sept-en commented Jun 11, 2021

Uh oh!

birchmd left a comment

Choose a reason for hiding this comment

Uh oh!

mrLSD commented Jun 12, 2021

Uh oh!

sept-en commented Jun 12, 2021

Uh oh!

mrLSD commented Jun 12, 2021

Uh oh!

joshuajbouw left a comment

Choose a reason for hiding this comment

Uh oh!

mfornet left a comment

Choose a reason for hiding this comment

Uh oh!

artob commented Jun 17, 2021

Uh oh!

sept-en commented Jun 17, 2021

Uh oh!

sept-en commented Jun 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sept-en commented Jun 10, 2021 •

edited

Loading