Skip to content

ETH connector#59

Merged
artob merged 119 commits into
developfrom
eth-connector
May 28, 2021
Merged

ETH connector#59
artob merged 119 commits into
developfrom
eth-connector

Conversation

@mrLSD
Copy link
Copy Markdown
Member

@mrLSD mrLSD commented May 5, 2021

ETH connector contains fugible token logic for specific flow.
To verify logic:

  • DIagram - represent Diagram of algorithm and process flow
  • Formal specification - represent algorithm in a formal way as functional style code with full type inference.

mrLSD and others added 30 commits April 1, 2021 19:48
* 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.
Comment thread src/connector.rs Outdated
Comment on lines +540 to +546
if fee > 0 {
let evm_relayer_address: EthAddress = engine
.get_relayer(&message_data.relayer.as_bytes())
.expect("ERR_WRONG_RELAYER_ID")
.0;
self.mint_eth(evm_relayer_address, fee);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This part works incorrectly and actually is critical. Currently, this works like that:

  1. If the fee is zero, the user receives the full amount, and the transaction is "Succeeded".
  2. If the fee is greater than thero:
    a) Query for the registered relayer, the relayer is registered. Then mint appropriate amount of fee to the relayer. The transaction is "Succeeded".
    b) Query for the registered relayer, the relayer is NOT registered. So we panic with "ERR_WRONG_RELAYER_ID", the transaction is "Failed". The user is not receiving his money, the proof was recorded as used on the previous steps so it's not possible to finish the deposit for this transaction any more. So basically, it's easy to burn any produced proof by not registering the relayer.

The suggested solution for that is the following:

  • Make sure we never ever panic in the ft_on_transfer execution (at least for the ETH branch), as the proof was recorded on the previous steps so we won't be able to restore it.
  • If the fee > 0, but there's no registered relayer for the predecessor account, mint the fee to the recipient as well. If we don't mint the fee for the recipient, this amount won't be minted and there's no sense in it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note also the todo

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hugely important on what you said. If there is a proof, we need to be able to use it on the previous steps. They must ALWAYS be restorable, only once, of course.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's no way to straight-forwardly revert the chain of promises at the moment. So this is correct that the proof is recorded in the previous step and only after that the tokens are minted. This protects from proof reusage and possible reentrancy attempts. We just need to try making all possible checks before storing the proof and don't let it panic at a later stage.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point!
@joshuajbouw FYI it's not related to Proof. We can reproduce it without Deposit call because ft_transfer_call is a public method.

Copy link
Copy Markdown
Member Author

@mrLSD mrLSD May 13, 2021

Choose a reason for hiding this comment

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

@sept-en @joshuajbouw I fixed that issue. Also, I placed an early checker for all possible assertions in ft_on_transfer.

@mrLSD
Copy link
Copy Markdown
Member Author

mrLSD commented May 20, 2021

Improvements & proposals:
#98 #104

Comment thread src/fungible_token.rs
Comment on lines +364 to +366
} else {
None
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is wrong as according to the NEP-145 specs: If the account is unknown to the contract then the total account storage balance returned will be zero.
So we need to always return StorageBalance, and zero for unknown accounts.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed at #98

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please note the appropriate issue in near-sdk-rs.

Comment thread src/fungible_token.rs Outdated
Evgeny Kuzyakov and others added 2 commits May 26, 2021 10:16
* Eugene's review 2021-05-08

* Eth connector improvements after review 2021-05-18 (#98)

* Adde improvements afterreview - account checking, msg len check, json lib

* Added json parser. Changed balance_of

* Fixed balance_of test

* json for ft_transfer & fix test

* Json args for ft_transfer_call & fixed tests

* Added json for FT storage methods

* Fix ft_resolve transfer promise result check. Removed total_supply_near

* Fix to NEP-145: storage_balance_of

* Fix storage_balance_of to NEP-145

* Eth connector statistic (#104)

* Added get_accounts_counter & tests

* Clippy fix

* Fixed: check accounts_contains_key & added comments

* Lint: cargo fmt

* Chanched key for Statistics

* Update src/connector.rs

Imptove comments

Co-authored-by: Michael Birch <michael@near.org>

* Changed:
- get_accounts_counter - return le_bytes
Added:
- test_get_accounts_counter_and_transfer - check recalculation accounts

Co-authored-by: Michael Birch <michael@near.org>

Co-authored-by: Michael Birch <michael@near.org>

Co-authored-by: Evgeny Ukhanov <mrlsd@ya.ru>
Co-authored-by: Michael Birch <michael@near.org>
Comment thread src/storage.rs Outdated
Contract = 0x0,
FungibleToken = 0x1,
UsedEvent = 0x2,
StatisticsAuroraAccountsCounter = 0x3,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It makes sense to start statistics Id from something like 0xF or 0x10. So if other fields related to the contract itself are added in the future, they will come in order and not going to be mixed with Ids for Statistics.

I'll create a PR for that.

mrLSD and others added 7 commits May 27, 2021 23:26
* Added tests:
* test_deposit_near_with_zero_fee
* test_deposit_evm_with_zero_fee

* Added:
* test_deposit_evm_amount_equal_fee_non_zero
* check_execution_status_failure

* Improved:
* test_deposit_with_same_proof
Added:
* test_deposit_wrong_custodian_address

* Added tests:
* test_deposit_near_amount_less_fee
* test_deposit_evm_amount_less_fee
* test_deposit_near_amount_zero_fee_non_zero
* test_deposit_evm_amount_zero_fee_non_zero
* test_deposit_near_amount_equal_fee_non_zero

* Changed: test_deposit_wrong_custodian_address
* EthConnector: make AdminControlled and pausable.

* EthConnector: add AdminControlled&pausability tests.

* Stylystic fixes.

* AdminControlled: add panic message when paused.

* AdminControlled: minor refactoring.

* AdminControlled: add errors. Naming improvements.

* AdminControlled: doc improvements.

* Test fixes.

* Fix fmt

* AdminControlled: make IO in main contract module.

* AdminControlled:explicitly check panic msg in tests.
* Rename `check_execution_status_failure()` method to
  `assert_execution_status_failure()`;
* `assert_execution_status_failure()`: accept panic message as an arg.
* Explicitly specify the panic message for each
  `assert_execution_status_failure()` if the promise didn't fail with
  the provided error message.

* Eth-connector tests: improve checks for failure msg.

* Usually the converted to string has either of the following two messages formats:
  "Action #0: Smart contract panicked: ERR_MSG [src/some_file.rs:LINE_NUMBER:COLUMN_NUMBER]"
  "right: 'MISMATCHED_DATA': ERR_MSG [src/some_file.rs:LINE_NUMBER:COLUMN_NUMBER]"
  So the ": ERR_MSG [" pattern should catch all invariants of error,
  even if one of the errors message is a subset of another one
  (e.g. `ERR_MSG_FAILED` is a subset of `ERR_MSG_FAILED_FOO`).
* Document `aurora encode-address` usage.

* Cache cargo artifacts between CI runs. (#92)

* Address comments from audit. (#86)

* Validate register length in `read_input_arr20()`
* Only read register length in `Engine::get_code_size`
* Add `read_input_borsh()`
* Ensure `method.args.len() == args_decoded.len()`
* Ensure register size is 8 in `read_u64`
* Use constant to specify the register ID used in `read_input()`

* Reduce size of `cargo cache` in CI. (#95)

* Define a `Wei` newtype for balances. (#96)

* Fix evm-bully builds after recent refactoring. (#100)

* Refactor `Engine::get_state` to return a `Result`. (#99)

* Ensure that `Cargo.lock` in the repo is valid. (#101)

* Remove unneeded nightly feature. (#102)

* Dispatch precompiles on the full address. (#107)

* Support state migration on upgrade. (#103)

* Remove resolved TODOs

* Fix state migration test

* Conditional compilation minor improvements.

Co-authored-by: Arto Bendiken <arto@aurora.dev>
Co-authored-by: Michael Birch <michael@near.org>
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
* Add `set_eth_connector_contract_data()` method.
* Minor refactoring: `init_contract()` method now accepts `InitCallArgs`
  instead of making IO operations in it.
@artob artob changed the base branch from master to develop May 28, 2021 19:25
@artob artob assigned sept-en and unassigned mrLSD May 28, 2021
@artob artob assigned artob and unassigned sept-en May 28, 2021
@artob artob merged commit 5144281 into develop May 28, 2021
@artob artob deleted the eth-connector branch May 28, 2021 20:16
mfornet added a commit that referenced this pull request Jun 4, 2021
* Base precompile code between connectors (#73)

* Base precompile code between connectors

* Handle errors and validate input

* Use proper result

* Document `aurora encode-address` usage.

* Cache cargo artifacts between CI runs. (#92)

* Address comments from audit. (#86)

* Validate register length in `read_input_arr20()`
* Only read register length in `Engine::get_code_size`
* Add `read_input_borsh()`
* Ensure `method.args.len() == args_decoded.len()`
* Ensure register size is 8 in `read_u64`
* Use constant to specify the register ID used in `read_input()`

* Reduce size of `cargo cache` in CI. (#95)

* Define a `Wei` newtype for balances. (#96)

* Fix evm-bully builds after recent refactoring. (#100)

* Refactor `Engine::get_state` to return a `Result`. (#99)

* Ensure that `Cargo.lock` in the repo is valid. (#101)

* Remove unneeded nightly feature. (#102)

* Implement BC generational storage.

* fix address input

* remove note

* put key on the end of the storage key

* remove pub from methods

* Dispatch precompiles on the full address. (#107)

* Support state migration on upgrade. (#103)

* Implement the ETH connector. (#59)

* Move when to call `set_generation`

* Fix arg

Co-authored-by: Marcelo Fornet <mfornet94@gmail.com>
Co-authored-by: Arto Bendiken <arto@aurora.dev>
Co-authored-by: Michael Birch <michael@near.org>
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
Co-authored-by: Evgeny Ukhanov <mrlsd@ya.ru>
mfornet added a commit that referenced this pull request Jun 10, 2021
* Add tests for state check after selfdestruct

* Aurora runner tracks storage usage to avoid underflow when storage is released in future transactions (#85)

* Implement generational storage (#87)

* Base precompile code between connectors (#73)

* Base precompile code between connectors

* Handle errors and validate input

* Use proper result

* Document `aurora encode-address` usage.

* Cache cargo artifacts between CI runs. (#92)

* Address comments from audit. (#86)

* Validate register length in `read_input_arr20()`
* Only read register length in `Engine::get_code_size`
* Add `read_input_borsh()`
* Ensure `method.args.len() == args_decoded.len()`
* Ensure register size is 8 in `read_u64`
* Use constant to specify the register ID used in `read_input()`

* Reduce size of `cargo cache` in CI. (#95)

* Define a `Wei` newtype for balances. (#96)

* Fix evm-bully builds after recent refactoring. (#100)

* Refactor `Engine::get_state` to return a `Result`. (#99)

* Ensure that `Cargo.lock` in the repo is valid. (#101)

* Remove unneeded nightly feature. (#102)

* Implement BC generational storage.

* fix address input

* remove note

* put key on the end of the storage key

* remove pub from methods

* Dispatch precompiles on the full address. (#107)

* Support state migration on upgrade. (#103)

* Implement the ETH connector. (#59)

* Move when to call `set_generation`

* Fix arg

Co-authored-by: Marcelo Fornet <mfornet94@gmail.com>
Co-authored-by: Arto Bendiken <arto@aurora.dev>
Co-authored-by: Michael Birch <michael@near.org>
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
Co-authored-by: Evgeny Ukhanov <mrlsd@ya.ru>

* Fix layout of the key

* Fix all tests (don't wipe the storage all the time)

* Use correct generation in writing storage

* Remove unnecessary references

Co-authored-by: Michael Birch <michael@near.org>
Co-authored-by: Joshua J. Bouw <dev@joshuajbouw.com>
Co-authored-by: Arto Bendiken <arto@aurora.dev>
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
Co-authored-by: Evgeny Ukhanov <mrlsd@ya.ru>
Co-authored-by: Michael Birch <michael.birch@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants