Token-locker: comments and review #174
Conversation
token-locker/src/lib.rs
Outdated
| #[callback] | ||
| #[serializer(borsh)] | ||
| verification_success: bool, | ||
| /// Account Id of the token on NEAR | ||
| #[serializer(borsh)] token: String, |
There was a problem hiding this comment.
Why token is String not an AccountId?
token-locker/src/lib.rs
Outdated
| #[serializer(borsh)] token: String, | ||
| /// Account Id of the token recipient on NEAR | ||
| #[serializer(borsh)] new_owner_id: String, |
There was a problem hiding this comment.
Why new_owner_id is a String, not an AccountId? Also, recipient maybe a better name to be consistent with other functions.
token-locker/src/lib.rs
Outdated
| #[serializer(borsh)] proof: Proof, | ||
| /// The NEAR account id of the token | ||
| #[serializer(borsh)] token: String, |
There was a problem hiding this comment.
Why String not AccountId? It is not clear it is NEAR account ID, or token name, or symbols or maybe Eth address...
| #[ext_contract(ext_token)] | ||
| pub trait ExtToken { | ||
| /// Transfer tokens to receiver NEAR account | ||
| fn ft_transfer( | ||
| &mut self, | ||
| receiver_id: AccountId, | ||
| amount: U128, | ||
| memo: Option<String>, |
There was a problem hiding this comment.
I don't get what memo mean in this context. But I think it is something standard, and we always use None for that field. So I think it doesn't matter.
| #[ext_contract(ext_token)] | ||
| pub trait ExtToken { | ||
| /// Transfer tokens to receiver NEAR account | ||
| fn ft_transfer( | ||
| &mut self, | ||
| receiver_id: AccountId, | ||
| amount: U128, |
There was a problem hiding this comment.
Balance I think is more logical here as a type, but maybe it is standard and can not be changed. Not sure about that.
| @@ -326,6 +363,7 @@ impl Contract { | |||
| required_deposit | |||
| } | |||
|
|
|||
| /// Update Ethereum Address of the Bridge Token Factory | |||
| #[private] | |||
| pub fn update_factory_address(&mut self, factory_address: String) { | |||
| self.eth_factory_address = validate_eth_address(factory_address); | |||
There was a problem hiding this comment.
I think validate_eth_address is not a clear name, it is more like a constructor for EthAddress from String. I think the name eth_address_from_string is more clear. I think it is Ok to do some checks in the constructor. I expect that the function validate_eth_address return is this address is valid or not.
| @@ -68,54 +73,80 @@ pub struct Contract { | |||
| pub is_whitelist_mode_enabled: bool, | |||
There was a problem hiding this comment.
The Ethereum Factory also have a whitelist. Are these whitelists consistent with each other? If yes, how is this achieved?
| @@ -300,7 +350,7 @@ impl Contract { | |||
| } | |||
| } | |||
|
|
|||
| /// Checks whether the provided proof is already used. | |||
| /// Checks whether the provided proof of Ethereum `burn` event is already used for unlocking tokens | |||
| pub fn is_used_proof(&self, #[serializer(borsh)] proof: Proof) -> bool { | |||
There was a problem hiding this comment.
Where this function is used? Is it just a part of public API?
|
|
||
| // The deposit required to cover the cost | ||
| // of additional storage of used proof | ||
| let required_deposit = self.record_proof(&proof); | ||
| assert!(env::attached_deposit() >= required_deposit); |
There was a problem hiding this comment.
It also should be an extra 2 YOCTO which we used for ft_transfer_call. And maybe we want to return extra money back if the deposit is bigger than required.
|
|
||
| // The deposit required to cover the cost | ||
| // of additional storage of used proof | ||
| let required_deposit = self.record_proof(&proof); |
There was a problem hiding this comment.
What if we get an error during ft_transfer_call? In that case, we will record that proof is already used, but we don't unlock tokens.
| @@ -81,17 +81,17 @@ contract ProofConsumer is Ownable, IProofConsumer { | |||
| .outcome | |||
| .executor_id | |||
| ) == keccak256(nearTokenLocker), | |||
There was a problem hiding this comment.
Maybe it is better to do all these checks at the begging of the function. I think we shouldn't store it in the usedProofs incorrect proofs.
No description provided.