Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Token-locker: comments and review #174

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
91 changes: 72 additions & 19 deletions token-locker/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,19 @@ enum StorageKey {
WhitelistAccounts,
}

/// Contract for Bridge Token-Locker which responsible for
/// (1) locking tokens on NEAR side
/// (2) unlocking tokens in case corresponding tokens were burned on Ethereum side
/// Only way to unlock locked tokens is to mint corresponded tokens on Ethereum side and after to burn them.
#[near_bindgen]
#[derive(BorshDeserialize, BorshSerialize, PanicOnDefault)]
pub struct Contract {
/// The account of the prover that we can use to prove.
/// The account of the prover that we can use
/// to prove the authenticity of events generated on Ethereum side.
pub prover_account: AccountId,
/// Ethereum address of the token factory contract, in hex.
pub eth_factory_address: EthAddress,
/// Hashes of the events that were already used.
/// Hashes of the Ethereum `Withdraw` events that were already used for unlock tokens.
pub used_events: UnorderedSet<Vec<u8>>,
/// Mask determining all paused functions
paused: Mask,
Expand All @@ -68,54 +73,93 @@ pub struct Contract {
pub is_whitelist_mode_enabled: bool,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Ethereum Factory also have a whitelist. Are these whitelists consistent with each other? If yes, how is this achieved?

}

/// Interface for Bridge Token-Locker Contract
#[ext_contract(ext_self)]
pub trait ExtContract {
/// The method which called on the last step of the deposit procedure (locking tokens)
#[result_serializer(borsh)]
fn finish_deposit(
&self,
#[serializer(borsh)] token: AccountId,
#[serializer(borsh)] amount: Balance,
#[serializer(borsh)] recipient: EthAddress,
/// Token NEAR address
#[serializer(borsh)]
token: AccountId,
/// Amount of tokens to transfer
#[serializer(borsh)]
amount: Balance,
/// The Ethereum address of the tokens recipient
#[serializer(borsh)]
recipient: EthAddress,
) -> result_types::Lock;

/// The method which called on the last step of the `withdraw` procedure (unlocking tokens)
#[result_serializer(borsh)]
fn finish_withdraw(
&self,
/// This method is called as a callback after `burn` event verification.
/// `verification_success` is a result of verification the proof of the Ethereum `burn` event
#[callback]
#[serializer(borsh)]
verification_success: bool,
#[serializer(borsh)] token: String,
#[serializer(borsh)] new_owner_id: String,
#[serializer(borsh)] amount: Balance,
#[serializer(borsh)] proof: Proof,
/// Account Id of the token on NEAR
#[serializer(borsh)]
token: String,
/// Account Id of the token recipient on NEAR
#[serializer(borsh)]
new_owner_id: String,
/// The amount of the burned tokens
#[serializer(borsh)]
amount: Balance,
/// The proof of the Ethereum `burn` event
#[serializer(borsh)]
proof: Proof,
) -> Promise;

/// The last step of logging token metadata (the token name, symbols etc).
/// This logging is necessary for transferring information about token to Ethereum
#[result_serializer(borsh)]
fn finish_log_metadata(
&self,
#[callback] metadata: FungibleTokenMetadata,
/// The information about token (name, symbols etc)
#[callback]
metadata: FungibleTokenMetadata,
/// NEAR account id of the token
token_id: AccountId,
) -> result_types::Metadata;

/// The method which called after checking the token Storage Balance of recipient
/// during the `withdraw` procedure
fn storage_balance_callback(
&self,
#[callback] storage_balance: Option<StorageBalance>,
#[serializer(borsh)] proof: Proof,
#[serializer(borsh)] token: String,
#[serializer(borsh)] recipient: AccountId,
#[serializer(borsh)] amount: Balance,
/// The result of checking token Storage Balance ot the recipient
#[callback]
storage_balance: Option<StorageBalance>,
/// The proof of the Ethereum `burn` event.
#[serializer(borsh)]
proof: Proof,
/// The NEAR account id of the token
#[serializer(borsh)]
token: String,
/// The NEAR account id of the token recipient
#[serializer(borsh)]
recipient: AccountId,
/// The amount of burned tokens
#[serializer(borsh)]
amount: Balance,
);
}

/// Interface for the ERC20 Bridge Tokens
#[ext_contract(ext_token)]
pub trait ExtToken {
/// Transfer tokens to receiver NEAR account
fn ft_transfer(
&mut self,
receiver_id: AccountId,
amount: U128,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Balance I think is more logical here as a type, but maybe it is standard and can not be changed. Not sure about that.

memo: Option<String>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

) -> PromiseOrValue<U128>;

/// Transfer tokens to receiver NEAR account with specific message
fn ft_transfer_call(
&mut self,
receiver_id: AccountId,
Expand All @@ -124,16 +168,22 @@ pub trait ExtToken {
msg: String,
) -> PromiseOrValue<U128>;

/// Get the information about token (name, symbols etc)
fn ft_metadata(&self) -> FungibleTokenMetadata;

/// Get the storage balance for the specific account
fn storage_balance_of(&mut self, account_id: Option<AccountId>) -> Option<StorageBalance>;
}

#[near_bindgen]
impl Contract {
/// The Bridge Token-Locker initialization.
///
/// # Arguments
///
/// * `prover_account`: NEAR account of the Near Prover contract which check the correctness of the Ethereum Events;
/// * `factory_address`: Ethereum address of the token factory contract, in hex.
#[init]
/// `prover_account`: NEAR account of the Near Prover contract;
/// `factory_address`: Ethereum address of the token factory contract, in hex.
pub fn new(prover_account: AccountId, factory_address: String) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why factory_address is String not EthAddress?

Self {
prover_account,
Expand Down Expand Up @@ -274,8 +324,10 @@ impl Contract {
#[serializer(borsh)] proof: Proof,
) -> Promise {
assert!(verification_success, "Failed to verify the proof");
let required_deposit = self.record_proof(&proof);

// The deposit required to cover the cost
// of additional storage of used proof
let required_deposit = self.record_proof(&proof);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

assert!(env::attached_deposit() >= required_deposit);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


let Recipient { target, message } = parse_recipient(new_owner_id);
Expand All @@ -300,7 +352,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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where this function is used? Is it just a part of public API?

self.used_events.contains(&proof.get_key())
}
Expand All @@ -326,6 +378,7 @@ impl Contract {
required_deposit
}

/// Update Ethereum Address of the Bridge Token Factory
#[private]
pub fn update_factory_address(&mut self, factory_address: String) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why String not EthAddress?

self.eth_factory_address = validate_eth_address(factory_address);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Expand Down