Improved ETH connector logic

[mrLSD]

Changes related to new eth-connector design.
DIagram here

Formal specification

[joshuajbouw]

Cool! Nice to see that this is pretty much good to go! I'll be reviewing this ASAP!!

[sept-en]

I can't reference this directly as the code is a part of the eth-connector branch already and not the part of the current PR, but we have ft_balance_of_eth method which duplicates the already existing get_Balance method. Probably we need to get rid of one of those.

[joshuajbouw]

This is a bit awkward to review since it is going to a branch that I can not review which needs a review. Is it at all possible if we just review the "gist" of this and then merge into that branch and then re-open eth-connector branch to master PR?

[joshuajbouw]

I would love to have made these all in line with the code but it just isn't possible with how the PR is laid out right now so I'll just list out blanket issues as I see them for now.

The biggest issue is the excessive use of String and Vec<u8> when parsing data, and the lack of size checks in a number of areas. Also, u128 is being used for balances but I believe these should be U256.

DO NOTE: Some of the read_input()[..] listed below are valid (though in hindsight you shouldn't need to add [..] as just read_input() is fine enough).

High priority

• Issue with EthConnectorContract::withdraw_near. It reads the input as read_input()[..] which alone isn't saying how many bytes the input should be, but also the field WithrdawlCallArgs.recepient_id is a String, which can be of any size. The problem is when we run validate_eth_address with that recepient_id is that there is an assert_eq! which checks that the data.len() is 20. This is problematic as anyone with input can trip that assertion. There has to be a guarantee of size.

• Issue with EthConnectorContract::deposit. It takes the Vec<u8> raw_proof and deserialises the data into a Proof but, there are now set data sizes, all fields are Vec<u8> meaning that anyone can make any of those fields any length. I would, if possible, set those sizes. Also, the deserialisation should be done in one step, not all Vec<u8> -> Vec<u8> snippets, then to other Structs. Should be done once to ensure the data parses correctly as soon as possible.

• Issue with EthConnectorContract::finish_deposit_near. Similar to above, when parsing the data, in FinishDepositCallArgs, new_owner_id, relayer_id and proof_key are all strings of no strict size. Further, there is no check to ensure that they are of the correct size. So, when you record the proof or mint near, these IDs look like they actually can end up being invalid.

• Issue with EthConnectorContract::storage_deposit, much like the others it reads an input of no discernable length. Uses AccountId which is a String with no set length. There also is no check to ensure that the length is correct in the inner methods.

• Issue with FungibleToken::storage_withdraw why is if there is a balance anything greater than 0 returns an error of ERR_WRONG_AMOUNT?

• Issue with EthConnectorContract::storage_balance_of, again the issue of reading input of no discernable length being parsed into a String with no length check. At second glance the Balance type, the u128 seems very wrong, shouldn't balance be U256?

• Issue with EthConnectContract::ft_on_transfer, same issues as above, String, no set size or checks.

• I do not agree that in FungibleToken the accounts should be stored as Strings. They should have to have a known size and be stored as bytes.

Medium priority

• Lots of functional inconsistency in the library where most of the logic is handled not in the contract module. i.e, right now we have sdk::read_input() being called in the contract module, but not in the methods themselves. Ideally, anything to do with sdk should be somewhere else. See note below, as I'll probably tackle this. Also, like EthConnectorContract::save_contract should be set_contract. There are a number of these.

• The errors that are returned via expect or unwrap should be made into errors, then sdk_unwraped or sdk_expected.

• sdk::read_input()[..] <-- generally you should explicitly state the start and end bytes. Though this isn't always the case as you can't possibly always know the length.

• The methods in lib.rs need to be sorted under the correct categories so that it's a bit more explicit which methods are mutative, and which are not.

• There are assert_eq everywhere, I think the sdk figure out where to spit them out... But not 100% sure. Reserve assertions for actual logic that is Fatal.

• A few errors don't follow the rest of the errors, like ERR_TOPIC, these need to be double-checked.

Low priority

• A lot of the naming and style is quite inconsistent with the rest of the library but, I wouldn't mind tackling this in the future. e.g EthConnectorContract::new is a getter, not a new. Also stuff like internal_unwrap_balance_of should be something like get_near_balance / get_eth_balance. If it's an actual internal method that is called by another layer, then it should be as inner_get_near_balance.

• The design is quite fine, but there are a few things to note such as some methods that are associated with a Struct but don't use it. Those could exist outside of it.

• Pointless changes from u128 to Sting then to &[u8] in ft_total_supply ft_total_supply_near ft_total_supply_eth ft_balance_of ft_balance_of_eth. These can be directly changed into bytes.

• in FungibleToken there is an arg registration_only that isn't used, this might be a TODO? It should be marked with _registration_only and noted TODO explaining its purpose. The #[allow(unused_variables)] should be dropped.

Notes

• The use of log feature is great and should be replicated throughout the library. Obviously as another PR.

• The more that I look at everything, there should be a large refactor that would look like Engine wrapped by Sdk stuff, then it'll be easier to test the actual functional logic without actually needing the Near SDK. This desperately needs to be done and I'll tackle this as I got some notes on the matter already.

• I don't think that I could've possibly gone through everything with this pass, this PR is just so large with a lot of critical methods. If the above were fixed, I would do another pass.

• I would love for a call to discuss the deposit methods as there is a bit of NEAR "magic" that I need to research on before I feel confident enough in how it works. In general it looks fine though.

[joshuajbouw]

These need to be all in the prelude.rs for both std and no_std, and then import from there. There are a few other files like this too.

[sept-en]

@joshuajbouw Great and very detailed review!

I'll address some things that I'm aware of and that we already discussed with @mrLSD to speed up the process.

Also, u128 is being used for balances but I believe these should be U256.

We should correspond the reference implementation of fungible token which uses Balance type which is indeed u128 as specified there. AFAIK, the u256 is not supported in Near runtime and u128 is used.

The biggest issue is the excessive use of String and Vec when parsing data, and the lack of size checks in a number of areas.

When we use Vec<u8> for the proofs data (logs, etc.) we can't really make its size constant or predictable, so this will be variable length anyway. Also, the same Proof structure is used in rainbow-token-connector.
What's about String usage - this is the approach widely used in Near. Though I would suggest adding the similar to is_valid_account_id method anyway.

I do not agree that in FungibleToken the accounts should be stored as Strings. They should have to have a known size and be stored as bytes.

As already mentioned above, String is used to correspond to the reference implementation.

Issue with FungibleToken::storage_withdraw why is if there is a balance anything greater than 0 returns an error of ERR_WRONG_AMOUNT?

This also matches the reference implementation. Though I'd say the returned error is not very clear and I suggest changing the description.

There are assert_eq everywhere, I think the sdk figure out where to spit them out... But not 100% sure. Reserve assertions for actual logic that is Fatal.

We don't have the near-sdk-rs available for this contract, you can see the available reimplemented sdk methods in src/sdk.rs file. AFAIK using assert is the established practice for Near smart contracts. The idea is to panic early so the promise will fail and revert. @artob correct me if I'm wrong, please.

A lot of the naming and style is quite inconsistent with the rest of the library but, I wouldn't mind tackling this in the future. e.g EthConnectorContract::new is a getter, not a new.

Already discussed it with @mrLSD internally but didn't post it there, sorry for that. We agreed that it's better to rename this to EthConnectorContract::get_instance() as this is indeed singleton. Sounds good for you?

Also stuff like internal_unwrap_balance_of should be something like get_near_balance / get_eth_balance.

For that purposes we have ft_balance_of() and ft_balance_of_eth() methods to match the NEP-141 specs. Though the ft_balance_of_eth() duplicates get_balance() as I mentioned earlier, so it'd be great to decide which one should be removed.

in FungibleToken there is an arg registration_only that isn't used, this might be a TODO? It should be marked with _registration_only and noted TODO explaining its purpose. The #[allow(unused_variables)] should be dropped.

I totally agree with you, but I think this was migrated (for consistency) from the reference implementation as well.

The more that I look at everything, there should be a large refactor that would look like Engine wrapped by Sdk stuff, then it'll be easier to test the actual functional logic without actually needing the Near SDK. This desperately needs to be done and I'll tackle this as I got some notes on the matter already.

I didn't fully understand your mentioned idea here, so please explain in detail what do you meant. But as mentioned earlier, the regular Near SDK is not used here, rather just a minimal low-level implementation.

I would love for a call to discuss the deposit methods as there is a bit of NEAR "magic" that I need to research on before I feel confident enough in how it works. In general it looks fine though.

Sure, then let's schedule a call with @mrLSD.

[sept-en]

@joshuajbouw so for your comments that intersect the NEP-141 reference implementation, I suggest for us to keep the appropriate code unchanged and rather open some issues in the near-sdk-rs repository.

[joshuajbouw]

@sept-en As I have noticed, right. When I have time, I'll give it a review and make notes and hopefully, some better practices can be put in place there. In the end, a String is basically a Vec<u8>, just with different displays.

[birchmd]

The more that I look at everything, there should be a large refactor that would look like Engine wrapped by Sdk stuff, then it'll be easier to test the actual functional logic without actually needing the Near SDK

@joshuajbouw I brought this up in a previous review as well. At that point it seemed like time pressures were such that it would not be addressed right away so I filed an issue: #28

If you work on factoring out sdk from core logic and adding unit tests then feel free to mention/close the issue as well.

[mrLSD]

@joshuajbouw thanks for yout deep diving response!

• U256 - is not supporting natively. With Alex Shevchenko we decided to use u128 for Eth too because NEAR doesn't support u128, but Eth balances are part of tottal_supply. And also we believe that SmartContract will never use so big numbers in practical cases.
• About read_input() got it - will change
• About String type - it's part of NEP-141 interface
• Proof - can't be fixed size. So it's `Vec
• Withdraw - I'll fix it`
• Please notice that fungible_token.rs is a representation of NEP-141 and "officially" implemented in another repo. So registration_only is same part of NEP-141. Firt of all it should be fixed in NEP-141

There are assert_eq everywhere
it's common practice for NEAR contracts
sdk::read_input()[..] <-- generally you should explicitly state the start and end bytes. Though this isn't always the case as you can't possibly always know the length.

It's just Vec<u8> to slice

The errors that are returned via expect or unwrap should be made into errors, then sdk_unwraped or sdk_expected.

It's just panicked without any returns

I do not agree that in FungibleToken the accounts should be stored as Strings. They should have to have a known size and be stored as bytes.

What you mean known size? NEAR account variable size.

[joshuajbouw]

The more that I look at everything, there should be a large refactor that would look like Engine wrapped by Sdk stuff, then it'll be easier to test the actual functional logic without actually needing the Near SDK

@joshuajbouw I brought this up in a previous review as well. At that point it seemed like time pressures were such that it would not be addressed right away so I filed an issue: #28

If you work on factoring out sdk from core logic and adding unit tests then feel free to mention/close the issue as well.

Thanks for filing a new issue on that. Yeah, not required right now at all, just only concerned about the function of the PRs.

[joshuajbouw]

@mrLSD thanks for the response. Yeah, @sept-en and I had a chat about it. I made the assumption that NEAR too the approach of others in that accounts were all bytes, and the TLD was a sort of layer on top of it, not actually the accounts themselves but it makes sense given you can associate private keys with a single TLD which is a good approach. Overall, given my history of working on other projects, that was a delight to find out. You can disregard my points on that.

[mrLSD]

@artob @joshuajbouw notice it was added Diagram and Formal Spec in PR description.

[artob]

U256 - is not supporting natively. With Alex Shevchenko we decided to use u128 for Eth too because NEAR doesn't support u128, but Eth balances are part of tottal_supply. And also we believe that SmartContract will never use so big numbers in practical cases.

This aspect of things will need special attention from reviewers. Even if the EVM interface is specified as uint128 in Solidity contracts, on the ABI level it will be possible for a malicious party to pass in larger values (everything is a uint256, under the hood) and to potentially cause exploitable overflows somewhere.

[artob]

@mrLSD As discussed on our call just now, let's go ahead and merge this into eth-connector and open a new PR from eth-connector to master, where we will finish the full review.

[joshuajbouw]

THANKS!