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.

Sign up for GitHub

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

Jump to bottom

Token-locker: comments and review #174

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Token-locker: comments and review #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1a667db
comments for token-locker contract
olga24912 Oct 5, 2022
64cfe51
comments for ExtContract
olga24912 Oct 5, 2022
5614283
comments for ExtToken
olga24912 Oct 5, 2022
ded9028
comments for new
olga24912 Oct 5, 2022
9d3f1bb
add comment for update_factory_address
olga24912 Oct 5, 2022
ede7cd9
fmt
olga24912 Oct 5, 2022
32fa114
comment for is_used_proof
olga24912 Oct 5, 2022
747650c
comment for required deposit
olga24912 Oct 5, 2022
45fe0d8
fix comments
olga24912 Oct 5, 2022
1b97f1c
fix error msg in parse and consume proof
olga24912 Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions erc20-bridge-token/contracts/ProofConsumer.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ contract ProofConsumer is Ownable, IProofConsumer {
.receipt_ids[0];
require(
!usedProofs[receiptId],
"The burn event proof cannot be reused"
"The lock/burn event proof cannot be reused"
);
usedProofs[receiptId] = true;

Expand All @@ -81,17 +81,17 @@ contract ProofConsumer is Ownable, IProofConsumer {
.outcome
.executor_id
) == keccak256(nearTokenLocker),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"Can only unlock tokens/set metadata from the linked proof produced on Near blockchain"
"Can only mint/unlock tokens/set metadata from the linked proof produced on Near blockchain"
);

result = fullOutcomeProof.outcome_proof.outcome_with_id.outcome.status;
require(
!result.failed,
"Can't use failed execution outcome for unlocking the tokens or set metadata"
"Can't use failed execution outcome for minting/unlocking the tokens or set metadata"
);
require(
!result.unknown,
"Can't use unknown execution outcome for unlocking the tokens or set metadata"
"Can't use unknown execution outcome for minting/unlocking the tokens or set metadata"
);
}
}
64 changes: 58 additions & 6 deletions token-locker/src/lib.rs
View file
Open in desktop
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,8 +73,16 @@ 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)
///
/// # Arguments
///
/// * `token` - Token NEAR address
/// * `amount` - Amount of tokens to transfer
/// * `recipient` - The Ethereum address of the tokens recipient
#[result_serializer(borsh)]
fn finish_deposit(
&self,
Expand All @@ -78,6 +91,16 @@ pub trait ExtContract {
#[serializer(borsh)] recipient: EthAddress,
) -> result_types::Lock;

/// The method which called on the last step of the `withdraw` procedure (unlocking tokens)
///
/// # Arguments
///
/// * `verification_success` - 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
/// * `token` - Account Id of the token on NEAR
/// * `new_owner_id` - Account Id of the token recipient on NEAR
/// * `amount` - The amount of the burned tokens
/// * `proof` - The proof of the Ethereum `burn` event
#[result_serializer(borsh)]
fn finish_withdraw(
&self,
Expand All @@ -90,13 +113,30 @@ pub trait ExtContract {
#[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
///
/// # Arguments
///
/// * `metadata` - The information about token (name, symbols etc)
/// * `token_id` - NEAR account id of the token
#[result_serializer(borsh)]
fn finish_log_metadata(
&self,
#[callback] metadata: FungibleTokenMetadata,
token_id: AccountId,
) -> result_types::Metadata;

/// The method which called after checking the token Storage Balance of recipient
/// during the `withdraw` procedure
///
/// # Arguments
///
/// * `storage_balance` - The result of checking token Storage Balance ot the recipient
/// * `proof` - The proof of the Ethereum `burn` event.
/// * `token` - The NEAR account id of the token
/// * `recipient` - The NEAR account id of the token recipient
/// * `amount` - The amount of burned tokens
fn storage_balance_callback(
&self,
#[callback] storage_balance: Option<StorageBalance>,
Expand All @@ -107,15 +147,18 @@ pub trait ExtContract {
);
}

/// 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 +167,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 +323,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 +351,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 +377,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