EVM tokens deposit & withdraw #24

mrLSD · 2021-04-07T05:34:23Z

ETH accounts deposit
ETH accounts withdraw
Changed transfers logic
Modified FT logic for EVM and NEAR account balance storage
Added ETH -> NEAR. NEAR -> ETH transfer

… prover

birchmd

This is an initial review. The main suggestion is about code organization to make it easier to read and test. I recognize this is potentially a large refactor and maybe not a high enough priority given the timelines. Otherwise, there are some questions for my own understanding.

I haven't looked very closely at the correctness of the logic yet. I will do that in another pass.

birchmd · 2021-04-12T20:05:02Z

src/connector.rs

+        let total_supply = self.ft.ft_total_supply_near();
+        sdk::return_output(&total_supply.to_string().as_bytes());
+        #[cfg(feature = "log")]
+        sdk::log(format!("Total supply NEAR: {}", total_supply));


Seems a little counter-intuitive to put the logging after the return. I would expect a return to be the last statement and all following code unreachable. But maybe that is not the case with this return_output API? I guess it just sets some state internal to the Near contract runtime which is used as the return value after the wasm execution finishes?

In any case, I think the code would map closer to normal rust patterns if return_output acted similar to the return keyword.

src/connector.rs

birchmd · 2021-04-12T20:40:05Z

src/fungible_token.rs

+            self.total_supply_eth = self
+                .total_supply_eth
+                .checked_add(amount)
+                .expect("ERR_TOTAL_SUPPLY_OVERFLOW");


Question: is there a difference between expect and sdk::panic_utf8?

If not, then it seems like it would be nice to be consistent in which we use. E.g.

let new_balance = balance.checked_add(amount).expect("ERR_BALANCE_OVERFLOW"); // other check_add calls ...

If there is a difference then I am interested in knowing what the difference is and when it is appropriate to use either.

src/lib.rs

birchmd · 2021-04-12T20:49:21Z

src/sdk.rs

@@ -156,7 +155,6 @@ mod exports {
    }
 }

-#[allow(dead_code)]


Hooray!

It would be great if we could eventually remove all of these that are peppered throughout the codebase.

mrLSD · 2021-04-13T07:09:17Z

@birchmd FYI the code was developed not only by me for that repo.
Some questions related to other contributors.

mrLSD · 2021-04-13T07:42:01Z

@birchmd
I suppose the twitter-style function is evil. Extreme minimalism is beautiful, but the logic of perception is lost. Functions are not reused. Jumps are needed to understand the flow. And most importantly, the functions you are talking about are really small. It is not obvious to me why they should be split.

For example, if you try to understand the low-level logic of how contracts work through macros in current core contracts, I'm sure you will spend weeks doing this.

This is beautiful, but for a low-level implementation it can make the logic very difficult to understand.

birchmd · 2021-04-13T14:27:21Z

@mrLSD

the code was developed not only by me

Yes, please don't take this as a personal attack. I am only starting with this PR because it is high priority. If my questions about general patterns are inappropriate here then maybe I should start a discussion thread about them?

It is not obvious to me why they should be split.

I think making the inputs and outputs of a function clear in its type signature makes it easier to see what a function is trying to do at a glance, without necessarily diving into the specifics in the body. But I think the biggest benefit of isolating core logic from sdk calls is that we can then write unit tests for the logic without mocking up the sdk externals. I know the logic here is simple, but it still important to have test coverage because then making any future changes to the code will be safer.

And to be clear, I understand if we cannot do that sort of change now because of the time constraints. But if that is the case then I think it would be good to file an issue so that we return to it later.

for a low-level implementation it can make the logic very difficult to understand

I agree the macros hide lots of details. I was only asking why those details are important. If they are not important then I think there is value is hiding those details because it means there is less cognitive overhead for future developers looking at this code who may not be familiar with the idiosyncrasy of how rust compiles to wasm. I know all the details myself because I have worked with wasm-based smart contracts in rust before, but that may not be true of all developers who work on this project in the future.

* EIP712-Withdraw: fixed encoding rules and order. * EIP712-Withdraw: `verify_withdraw_eip712` returns `true` only if the sender address equals to the address of message signer. * EIP712-Withdraw: update tests. * EIP712-Withdraw: refactoring. * ethabit::encode_token_packed: use right-padded encoding for `Address`. * WithdrawFromEthCallArgs: fixed `amount` type conversion.

…ra-engine into evm-deposit-withdraw

mrLSD · 2021-04-17T09:11:40Z

@artob

* Use references in fungible_token to avoid cloning * cargo fmt

birchmd

Some questions about the new tests:

birchmd · 2021-04-19T14:48:19Z

src/engine.rs

+                    // TODO: should be aligned to eth-connector balance management logic
+                    //Engine::set_balance(&address, &basic.balance);


Looks like outdated comment+code that can be deleted.

birchmd · 2021-04-19T15:06:26Z

tests/test_connector.rs

+}
+
+#[test]
+fn test_deposit_eth_and_near() {


I am uncertain about this test. It calls init twice, so I am unsure what happens to the state in between. I'm also not clear on why we need two different custodian addresses between these calls.

I would expect this test to be similar to the ones above and below; it initializes the contract, deposits some near and eth, then checks the all the total supplies are the right numbers. Is there a complication here I am missing?

We have different Proof and EIP712 messages that related to different custodian_address. Kirill can't generate data for the same custodian address. That's why we call init again.

birchmd · 2021-04-19T15:09:01Z

tests/test_connector.rs

+}
+/*
+#[test]
+fn test_withdraw_eth() {


Should this test still be commented out?

@birchmd that method will be removed, as Alex Shevchenko announced. So we don't need that test.

birchmd · 2021-04-19T15:15:16Z

tests/test_connector.rs

+}
+
+#[test]
+fn test_withdraw_near() {


As part of this test I think we should check the balance of RECIPIENT_ETH_ADDRESS too.

@birchmd as Alex Shevchenko announced that logic will be changed.

Clippy reports many errors like the following: ``` error: use of `expect` followed by a function call --> src/prover.rs:143:14 | 143 | .expect(str_from_slice(FAILED_PARSE)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| { panic!("{}", str_from_slice(FAILED_PARSE).to_string()) })` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#expect_fun_call ``` This commit corrects all those errors by using a new trait on Option and Result that can take an expect message in bytes.

mrLSD · 2021-04-19T19:23:12Z

@birchmd FYI: as we discussed last Friday, we should implement new logic for EVM and nETH tokens.
It will be a new PR.

birchmd · 2021-04-19T19:44:48Z

we discussed last Friday, we should implement new logic for EVM and nETH tokens.

@mrLSD I remember Alex Shevchenko presented a design for bridging ERC-20 tokens into the EVM and he said this could work for native Eth as well. It wasn't clear to me what the conclusion was since we already have work on the eth-connector. You're saying we are definitely changing over to the new design? And this means some sections of the logic will undergo significant change in a future iteration?

birchmd

Another round of comments:

birchmd · 2021-04-19T18:38:52Z

src/connector.rs

+    }
+
+    pub fn init_contract() {
+        //assert_eq!(sdk::current_account_id(), sdk::predecessor_account_id());


Outdated code

birchmd · 2021-04-19T18:45:30Z

Makefile

@@ -1,6 +1,6 @@
 CARGO = cargo
 NEAR  = near
-FEATURES = contract
+FEATURES = contract,integration-test


I'm not sure what our release pipeline looks like, but if it uses make release then I think we should not be including the integration-test feature by default. That feature should only be enabled when compiling the contract for testing.

As I said @artob we should remove it from Make file

birchmd · 2021-04-19T18:51:09Z

src/lib.rs

+        EthConnectorContract::new().storage_balance_of()
+    }
+
+    #[cfg(feature = "integration-test")]


I see, so the reason we need this feature is to mock up the prover contract functions the eth-connector uses. I think it would be nicer if we didn't have to define these functions here and instead did something within the tests themselves. I understand this probably adds more complexity to the tests, so we might not want to do it now, but it might be something to think about in a code cleanup in the future.

The main question is - how?

Ideally the prover contract stub would be a separate resource only used in the tests and it would be deployed to the simulated environment as part of the init setup in the tests.

The part I'm not sure on is if we would need to commit the compiled wasm for that stub to the repo, or if it could be compiled as part of the test. I know nearcore has some contracts that are only used for testing, so we can look at what was done there.

Thanks for the idea! I think we will implement it as the next step of testing.

birchmd · 2021-04-19T19:07:56Z

src/deposit_event.rs

+
+/// Data that was emitted by the Ethereum Deposited NEAR-token event.
+#[derive(Debug, PartialEq)]
+pub struct EthDepositedNearEvent {


It seems like there is a lot of code duplication between EthDepositedNearEvent and EthDepositedEthEvent. Is this "accidental duplication" (i.e. two logically separate implementations which happen to look the same now, but may evolve separately, so having the duplication is a feature for future changes)? Or is it true duplication in the sense that EthDepositedNearEvent and EthDepositedEthEvent will always need to be changed together?

If it is the latter then I think we could reduce the duplication by either using a type parameter

pub struct EthDepositedEvent<T> { pub eth_custodian_address: EthAddress, pub sender: T, pub recipient: T, pub amount: U128, pub fee: U128, }

or an enum

pub struct EthDepositedEvent { pub eth_custodian_address: EthAddress, pub amount: U128, pub fee: U128, pub parties: Parties // TODO: better naming } pub enum Parties { Near { sender: AccountId, recipient: AccountId }, Eth { sender: EthAddress, recipient: EthAddress }, }

Personally I think the type parameter is a cleaner approach.

tests/test_connector.rs

src/prover.rs

birchmd · 2021-04-19T19:25:02Z

src/deposit_event.rs

+}
+
+impl EthDepositedNearEvent {
+    #[allow(dead_code)]


Could we remove all the #[allow(dead_code)] annotations from things that are now being used?

birchmd · 2021-04-19T19:29:56Z

src/prover.rs

+}
+
+/// Parameters of Etherium event
+pub type EthEventParams = Vec<(String, ParamType, bool)>;


Seems like this should be Vec<EventParam>. It would avoid the map call in EthEvent::fetch_log_entry_data.

birchmd · 2021-04-19T20:00:01Z

src/connector.rs

+        .save_contract();
+    }
+
+    pub fn deposit_near(&self) {


This is maybe a silly question, but how does a user inside the EVM use the Near they have deposited?

Hmm, I guess this is a silly question. I think I completely misunderstood what is supposed to be going on here. The eth and near parts of the FungibleToken are actually different buckets containing the same type of token ($ETH). I was confused because Near has its own token ($NEAR) and so that's what I assumed was being referenced with comments like /// Total supply of the all NEAR token.

If I understand correctly now, the total_supply_eth represents the amount of $ETH which is owned by addresses within our EVM, and total_supply_near represents the amount of $ETH owned by accounts on the Near platform. There are functions which allow moving $ETH into/out of EVM addresses from Near accounts. There must also be some way for these Near accounts to send the $ETH back over to Ethereum via the bridge? And I guess the relayer_eth_account is some way of moving $ETH directly into the Near EVM from Ethereum?

Is there a higher level description of what is supposed to be going on here? I think I've slowly pieced together what the code is doing, but having some documentation would be really helpful in making sure I understand it correctly.

artob · 2021-04-21T16:56:26Z

Thanks @mrLSD for the hard work, and @birchmd for the multiple rounds of reviews!

I've now merged this to the standalone eth-connector branch, on par with master. I'm keeping master reserved for code about to get imminently deployed on chain, so let's let this mature (and get tested) on its standalone branch for now until it's time to deploy it on chain in the next weeks.

@mrLSD Please target eth-connector as the base branch for follow-up pull requests such as #36.

joshuajbouw · 2021-04-21T17:52:03Z

Oh shoot, I was about 10% into my review. No worries :D

mrLSD added 10 commits April 1, 2021 19:48

Added prover & extended types

98b302a

Mode Borsh args from types to parameters

97a3759

Merge branch 'master' of github.com:aurora-is-near/aurora-engine into…

a889328

… prover

Added fungible tokens

25154f0

Added eth-connector

a0942a2

Modify assert for fee

5876814

Fix formatting

facc786

Extend eth-conenctor with EVM token logic

b055a54

Changed eth-connector deposit logic

eb9e361

Added changes for ETH deposit/withdraw and Engine changes

fcc1332

mrLSD self-assigned this Apr 7, 2021

mrLSD added 10 commits April 7, 2021 16:59

Mint ETH-tokens

978474b

Added: transfer_eth

041879e

ETH withdraw basic method

b5ffb23

eth-withdraw validation structure & modified Deposit-eth fields

a7e73f4

eth transfer and withdraw logic

45a5ca9

eip712 message verifier - started encoding

676a86c

added encode-packed

e345d71

virefy EIP712 message for withdraw

de7f36d

Changed EIP712 message fields

f1f6479

Modify logs for EIP712 messages

944b6e2

artob requested a review from birchmd April 12, 2021 16:22

artob added this to In progress in EVM on TestNet via automation Apr 12, 2021

artob added the C-enhancement Category: New feature or request label Apr 12, 2021

artob requested review from artob and joshuajbouw April 12, 2021 16:25

birchmd reviewed Apr 12, 2021

View reviewed changes

sept-en and others added 11 commits April 14, 2021 20:32

Extend tests for eth-connector

96b3c0e

Merge branch 'evm-deposit-withdraw' of github.com:aurora-is-near/auro…

348e5cc

…ra-engine into evm-deposit-withdraw

eth-connector test deposit & balance & total_supply

80f033b

Imporved tests

07251fd

FT tests

2de5af8

Fixed verify_transfer_eip712

e71d52a

Change test_withdraw_near

09b79c1

Tests for: ft_transfer, ft_transfer_call

f2e1f11

test_eth_deposit_balance_total_supply

356ad42

test and ifx: deposit_eth, withdraw_near

6cae502

mrLSD marked this pull request as ready for review April 17, 2021 09:10

artob requested a review from birchmd April 19, 2021 14:20

mrLSD changed the base branch from eth-connector to master April 19, 2021 14:23

References in fungible token (#29)

d920662

* Use references in fungible_token to avoid cloning * cargo fmt

birchmd reviewed Apr 19, 2021

View reviewed changes

birchmd added 2 commits April 19, 2021 15:37

Fix: hide logging behind feature flag

27f90ef

birchmd reviewed Apr 19, 2021

View reviewed changes

artob changed the base branch from master to eth-connector April 21, 2021 16:46

artob merged commit e2f3746 into eth-connector Apr 21, 2021

artob deleted the evm-deposit-withdraw branch April 21, 2021 16:49

EVM on TestNet automation moved this from In progress to Done Apr 21, 2021

artob mentioned this pull request Apr 21, 2021

Improved ETH connector logic #36

Merged

birchmd mentioned this pull request May 18, 2021

Eugene's Eth connector review #97

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVM tokens deposit & withdraw #24

EVM tokens deposit & withdraw #24

mrLSD commented Apr 7, 2021

birchmd left a comment

birchmd Apr 12, 2021

birchmd Apr 12, 2021

birchmd Apr 12, 2021

mrLSD commented Apr 13, 2021

mrLSD commented Apr 13, 2021

birchmd commented Apr 13, 2021

mrLSD commented Apr 17, 2021

birchmd left a comment

birchmd Apr 19, 2021

birchmd Apr 19, 2021

mrLSD Apr 19, 2021

birchmd Apr 19, 2021

mrLSD Apr 19, 2021

birchmd Apr 19, 2021

mrLSD Apr 19, 2021

mrLSD commented Apr 19, 2021

birchmd commented Apr 19, 2021

birchmd left a comment

birchmd Apr 19, 2021

birchmd Apr 19, 2021

mrLSD Apr 20, 2021

birchmd Apr 19, 2021

mrLSD Apr 20, 2021

birchmd Apr 20, 2021

mrLSD Apr 20, 2021

birchmd Apr 19, 2021

birchmd Apr 19, 2021

birchmd Apr 19, 2021

birchmd Apr 19, 2021

birchmd Apr 19, 2021

mrLSD Apr 20, 2021

artob commented Apr 21, 2021

joshuajbouw commented Apr 21, 2021

		// TODO: should be aligned to eth-connector balance management logic
		//Engine::set_balance(&address, &basic.balance);

EVM tokens deposit & withdraw #24

EVM tokens deposit & withdraw #24

Conversation

mrLSD commented Apr 7, 2021

birchmd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrLSD commented Apr 13, 2021

mrLSD commented Apr 13, 2021

birchmd commented Apr 13, 2021

mrLSD commented Apr 17, 2021

birchmd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrLSD commented Apr 19, 2021

birchmd commented Apr 19, 2021

birchmd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artob commented Apr 21, 2021

joshuajbouw commented Apr 21, 2021