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

Fee integration #62

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

Fee integration #62

wants to merge 53 commits into from

Conversation

karim-en
Copy link
Collaborator

@karim-en karim-en commented Jul 20, 2023

Description

This PR contains Fee Implementation for Deposit and Withdrawal flows for Ether transfer using Rainbow Bridge (Business requirement).

Specification:

  • Deposit and withdrawal can have separate fee percentages.
  • By default, the fee collected is locked in the same contract that the owner could claim using the function claim_fee.
  • The fee is transferred to the fee owner account instead of the current contract account if the set_fee_owner was called.
  • All the fee setters are only callable by the owner.
  • The fee_amount has lower and upper bounds.
  • If the the calculated fee_amount is higher than the withdraw/deposit amount then charge the full amount as a fee and don't transfer anything to the recipient.
  • The fee is set in 6 decimal precision 10% = 0.1 * 10e6 = 100_000
  • It is possible to set a particular fee for silos on deposit and withdrawal. The silo is just a NEAR account, so as a side effect, it is possible to set a specific fee to any NEAR account.

Added public function:

fn calculate_fee_amount(&self, amount: U128, fee_type: FeeType, silo: Option<AccountId>) -> U128;
fn set_deposit_fee(&mut self, fee: Option<Fee>);
fn set_withdraw_fee(&mut self, fee: Option<Fee>);
fn set_deposit_fee_per_silo(&mut self, silo: AccountId, fee: Option<Fee>);
fn set_withdraw_fee_per_silo(&mut self, silo: AccountId, fee: Option<Fee>);
fn set_fee_owner(&mut self, owner: Option<AccountId>);
fn claim_fee(&mut self, amount: U128, receiver_id: Option<AccountId>);
fn migrate_fee_storage() -> Self;
fn get_deposit_fee(&self) -> Option<Fee>;
fn get_withdraw_fee(&self) -> Option<Fee>;
fn get_deposit_fee_per_silo(&self, silo: AccountId) -> Option<Fee>;
fn get_withdraw_fee_per_silo(&self, silo: AccountId) -> Option<Fee>;
fn get_fee_owner(&self) -> AccountId;

Migration:
The migration is needed due to the added field fee to the contract structure. To migrate the storage we need to call:
pub fn migrate_fee_storage() -> Self

Performance / NEAR gas cost considerations

The gas for withdrawal and deposit transactions will be increased a little bit due to additional fee calculations and reads from the storage.

Testing

Unit tests have been added to cover multiple fee cases.

How should this be reviewed

The review should verify the functions withdraw, engine_withdraw, and finish_deposit and make sure that the burn/mint/transfer logic is correct.

Additional information

@karim-en karim-en requested review from aleksuss and mrLSD July 28, 2023 00:37
eth-connector/src/lib.rs Outdated Show resolved Hide resolved
eth-connector/src/fee.rs Outdated Show resolved Hide resolved
Comment on lines 23 to 27
pub struct FeeStorage {
pub deposit_fee: Option<Fee>,
pub withdraw_fee: Option<Fee>,
pub withdraw_fee_per_silo: UnorderedMap<AccountId, Fee>,
pub deposit_fee_per_silo: UnorderedMap<AccountId, Fee>,
Copy link

Choose a reason for hiding this comment

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

Please describe all the fields and the logic of how it's expected to work here, e.g. if deposit and withdrawals fee settings override per silo configurations (some may think it is, but it's actually the opposite), what happens if either of these settings are set, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eth-connector/src/lib.rs Outdated Show resolved Hide resolved
eth-connector/src/lib.rs Outdated Show resolved Hide resolved
eth-connector/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 16 to 20
pub struct Fee {
pub fee_percentage: U128,
pub lower_bound: Option<U128>,
pub upper_bound: Option<U128>,
}
Copy link

Choose a reason for hiding this comment

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

Please describe all the fields.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eth-connector/src/fee.rs Outdated Show resolved Hide resolved
@@ -556,31 +677,52 @@ impl FundsFinish for EthConnectorContract {

log!("Finish deposit with the amount: {}", deposit_call.amount);

// Mint - calculate new balances
self.mint_eth_on_near(&deposit_call.new_owner_id, deposit_call.amount);
// Store proof only after `mint` calculations
Copy link

Choose a reason for hiding this comment

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

This comment needs to be updated.
Also, why don't we mint the whole amount to the current_account_id in the first place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We mint to the current_account_id just in case there is an ft-transfer-call when depositing to eth account, but on deposit to near account, we can mint directly to the new_owner_id.
If are asking how this worked with one call, then it worked because the new_owner_id == current_account_id on this ft-transfer-call.
additional to that it is not clear why actually the env::predecessor_account_id(), was used instead of env::current_account_id()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eth-connector/src/lib.rs Outdated Show resolved Hide resolved
eth-connector/src/lib.rs Outdated Show resolved Hide resolved
eth-connector/src/fee.rs Outdated Show resolved Hide resolved
eth-connector/src/fee.rs Outdated Show resolved Hide resolved
eth-connector/src/fee.rs Outdated Show resolved Hide resolved
karim-en and others added 3 commits August 11, 2023 14:50
Co-authored-by: Oleksandr Anyshchenko <oleksandr.anyshchenko@aurora.dev>
eth-connector/src/lib.rs Outdated Show resolved Hide resolved
eth-connector/src/lib.rs Outdated Show resolved Hide resolved
eth-connector/src/fee.rs Outdated Show resolved Hide resolved
eth-connector/src/lib.rs Outdated Show resolved Hide resolved
eth-connector/src/lib.rs Outdated Show resolved Hide resolved
@mrLSD
Copy link
Collaborator

mrLSD commented Aug 22, 2023

@karim-en please cover also with integration tests: eth-connector-tests crate.

@mrLSD
Copy link
Collaborator

mrLSD commented Aug 22, 2023

@karim-en and I believe AIP also should be extended.

@karim-en karim-en marked this pull request as draft March 12, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants