From a7468066310778feeee12e0d61e553ac4e95c776 Mon Sep 17 00:00:00 2001 From: Michael Birch Date: Fri, 17 Nov 2023 10:03:41 -0500 Subject: [PATCH] Feat(xcc): Use native bridge logic to unwrap wNEAR (#867) ## Description Since #750 has been merged, the ERC-20 connector is able to unwrap wrapped Near tokens into Near native tokens. This logic was duplicated by the XCC router, but in this PR we remove the logic from XCC and use the bridge's unwrapping instead. This has a nice side-effect of saving us 20 Tgas from the XCC overhead. ## Performance / NEAR gas cost considerations No impact on regular transactions, 20 Tgas improvement to XCC transactions. ## Testing Existing tests. Specifically this relies heavily on the upgrade test introduced in #866 --- engine/src/xcc.rs | 38 +++++------ etc/xcc-router/src/lib.rs | 130 ++++++++++++++++++++------------------ 2 files changed, 86 insertions(+), 82 deletions(-) diff --git a/engine/src/xcc.rs b/engine/src/xcc.rs index 6a39e69a1..63e1a62fc 100644 --- a/engine/src/xcc.rs +++ b/engine/src/xcc.rs @@ -25,7 +25,7 @@ pub const CODE_KEY: &[u8] = b"router_code"; pub const VERSION_UPDATE_GAS: NearGas = NearGas::new(5_000_000_000_000); pub const INITIALIZE_GAS: NearGas = NearGas::new(15_000_000_000_000); pub const UPGRADE_GAS: NearGas = NearGas::new(20_000_000_000_000); -pub const UNWRAP_AND_REFUND_GAS: NearGas = NearGas::new(25_000_000_000_000); +pub const REFUND_GAS: NearGas = NearGas::new(5_000_000_000_000); pub const WITHDRAW_GAS: NearGas = NearGas::new(40_000_000_000_000); /// Solidity selector for the `withdrawToNear` function /// `https://www.4byte.directory/signatures/?bytes4_signature=0x6b351848` @@ -312,22 +312,20 @@ pub fn handle_precompile_promise( AddressVersionStatus::DeployNeeded { create_needed } => create_needed, AddressVersionStatus::UpToDate => false, }; - let args = format!( - r#"{{"amount": "{}", "refund_needed": {}}}"#, - required_near.as_u128(), - refund_needed, - ); - let unwrap_call = PromiseCreateArgs { - target_account_id: promise.target_account_id.clone(), - method: "unwrap_and_refund_storage".into(), - args: args.into_bytes(), - attached_balance: ZERO_YOCTO, - attached_gas: UNWRAP_AND_REFUND_GAS, - }; - // Safety: This call is safe because the router's `unwrap_and_refund_storage` method - // does not violate any security invariants. It only interacts with the wrap.near contract - // to obtain NEAR from WNEAR. - unsafe { Some(handler.promise_attach_callback(id, &unwrap_call)) } + if refund_needed { + let refund_call = PromiseCreateArgs { + target_account_id: promise.target_account_id.clone(), + method: "send_refund".into(), + args: Vec::new(), + attached_balance: ZERO_YOCTO, + attached_gas: REFUND_GAS, + }; + // Safety: This call is safe because the router's `send_refund` method + // does not violate any security invariants. It only sends NEAR back to this contract. + unsafe { Some(handler.promise_attach_callback(id, &refund_call)) } + } else { + Some(id) + } }; // 3. Finally we can do the call the user wanted to do. @@ -457,8 +455,9 @@ impl AddressVersionStatus { } fn withdraw_to_near_args(recipient: &AccountId, amount: Yocto) -> Vec { + let recipient_with_msg = format!("{recipient}:unwrap"); let args = ethabi::encode(&[ - ethabi::Token::Bytes(recipient.as_bytes().to_vec()), + ethabi::Token::Bytes(recipient_with_msg.into_bytes()), ethabi::Token::Uint(U256::from(amount.as_u128())), ]); [&WITHDRAW_TO_NEAR_SELECTOR, args.as_slice()].concat() @@ -537,6 +536,7 @@ mod tests { #[test] fn test_withdraw_to_near_encoding() { let recipient: AccountId = "some_account.near".parse().unwrap(); + let recipient_with_msg = format!("{recipient}:unwrap"); let amount = Yocto::new(1332654); #[allow(deprecated)] let withdraw_function = ethabi::Function { @@ -559,7 +559,7 @@ mod tests { }; let expected_tx_data = withdraw_function .encode_input(&[ - ethabi::Token::Bytes(recipient.as_bytes().to_vec()), + ethabi::Token::Bytes(recipient_with_msg.into_bytes()), ethabi::Token::Uint(U256::from(amount.as_u128())), ]) .unwrap(); diff --git a/etc/xcc-router/src/lib.rs b/etc/xcc-router/src/lib.rs index 096b38e7d..d55a3ced4 100644 --- a/etc/xcc-router/src/lib.rs +++ b/etc/xcc-router/src/lib.rs @@ -4,7 +4,7 @@ use aurora_engine_types::parameters::{ }; use near_sdk::borsh::{self, BorshDeserialize, BorshSerialize}; use near_sdk::collections::{LazyOption, LookupMap}; -use near_sdk::json_types::{U128, U64}; +use near_sdk::json_types::U64; use near_sdk::BorshStorageKey; use near_sdk::{ env, near_bindgen, AccountId, Gas, PanicOnDefault, Promise, PromiseIndex, PromiseResult, @@ -27,15 +27,9 @@ const CURRENT_VERSION: u32 = std::include!("VERSION"); const ERR_ILLEGAL_CALLER: &str = "ERR_ILLEGAL_CALLER"; const INITIALIZE_GAS: Gas = Gas(15_000_000_000_000); -/// Gas cost estimated from mainnet data. Cost seems to consistently be 3 Tgas, but we add a -/// little more to be safe. Example: -/// https://explorer.mainnet.near.org/transactions/3U9SKbGKM3MchLa2hLTNuYLdErcEDneJGbGv1cHZXuvE#HsHabUdJ7DRJcseNa4GQTYwm8KtbB4mqsq2AUssJWWv6 -const WNEAR_WITHDRAW_GAS: Gas = Gas(5_000_000_000_000); /// Gas cost estimated from mainnet data. Example: /// https://explorer.mainnet.near.org/transactions/5NbZ7SfrodNxeLcSkCmLAEdbZfbkk9cjqz3zSDwktKrk#D7un3c3Nxv7Ee3JpQSKiM97LbwCDFPbMo5iLoijGPXPM const WNEAR_REGISTER_GAS: Gas = Gas(5_000_000_000_000); -/// Gas cost estimated from simulation tests. -const REFUND_GAS: Gas = Gas(5_000_000_000_000); /// Registration amount computed from FT token source code, see /// https://github.com/near/near-sdk-rs/blob/master/near-contract-standards/src/fungible_token/core_impl.rs#L50 /// https://github.com/near/near-sdk-rs/blob/master/near-contract-standards/src/fungible_token/storage_impl.rs#L101 @@ -129,7 +123,7 @@ impl Router { /// contract calls is used by the address associated with the sub-account this router contract /// is deployed at. pub fn execute(&self, #[serializer(borsh)] promise: PromiseArgs) { - self.require_parent_caller(); + self.assert_preconditions(); let promise_id = Router::promise_create(promise); env::promise_return(promise_id) @@ -137,7 +131,7 @@ impl Router { /// Similar security considerations here as for `execute`. pub fn schedule(&mut self, #[serializer(borsh)] promise: PromiseArgs) { - self.require_parent_caller(); + self.assert_preconditions(); let nonce = self.nonce.get().unwrap_or_default(); self.scheduled_promises.insert(&nonce, &promise); @@ -160,42 +154,11 @@ impl Router { env::promise_return(promise_id) } - /// The router will receive wNEAR deposits from its user. This function is to - /// unwrap that wNEAR into NEAR. Additionally, this function will transfer some - /// NEAR back to its parent, if needed. This transfer is done because the parent - /// must cover the storage staking cost with the router account is first created, - /// but the user ultimately is responsible to pay for it. - pub fn unwrap_and_refund_storage(&self, amount: U128, refund_needed: bool) { - self.require_parent_caller(); - - let args = format!(r#"{{"amount": "{}"}}"#, amount.0); - let id = env::promise_create( - self.wnear_account.clone(), - "near_withdraw", - args.as_bytes(), - 1, - WNEAR_WITHDRAW_GAS, - ); - let final_id = if refund_needed { - env::promise_then( - id, - env::current_account_id(), - "send_refund", - &[], - 0, - REFUND_GAS, - ) - } else { - id - }; - env::promise_return(final_id); - } - /// Allows the parent contract to trigger an update to the logic of this contract /// (by deploying a new contract to this account); #[payable] pub fn deploy_upgrade(&mut self, #[serializer(borsh)] args: DeployUpgradeParams) { - self.require_parent_caller(); + self.assert_preconditions(); let promise_id = env::promise_batch_create(&env::current_account_id()); env::promise_batch_action_deploy_contract(promise_id, &args.code); @@ -209,35 +172,38 @@ impl Router { env::promise_return(promise_id); } - #[private] pub fn send_refund(&self) -> Promise { - let parent = self - .parent - .get() - .unwrap_or_else(|| env::panic_str("ERR_CONTRACT_NOT_INITIALIZED")); + let parent = self.get_parent().unwrap_or_else(env_panic); + + require_caller(&parent) + .and_then(|_| require_no_failed_promises()) + .unwrap_or_else(env_panic); Promise::new(parent).transfer(REFUND_AMOUNT) } } impl Router { - fn require_parent_caller(&self) { - let caller = env::predecessor_account_id(); - let parent = self - .parent - .get() - .unwrap_or_else(|| env::panic_str("ERR_CONTRACT_NOT_INITIALIZED")); - if caller != parent { - env::panic_str(ERR_ILLEGAL_CALLER); - } - // Any method that can only be called by the parent should also only be executed if - // the parent's execution was successful. - let num_promises = env::promise_results_count(); - for index in 0..num_promises { - if let PromiseResult::Failed | PromiseResult::NotReady = env::promise_result(index) { - env::panic_str("ERR_CALLBACK_OF_FAILED_PROMISE"); - } - } + fn get_parent(&self) -> Result { + self.parent.get().ok_or(Error::ContractNotInitialized) + } + + /// Checks the following preconditions: + /// 1. Contract is initialized + /// 2. predecessor_account_id == self.parent + /// 3. There are no failed promise results + /// These preconditions must be checked on methods where are important for + /// the security of the contract (e.g. `execute`). + fn require_preconditions(&self) -> Result<(), Error> { + let parent = self.get_parent()?; + require_caller(&parent)?; + require_no_failed_promises()?; + Ok(()) + } + + /// Panics if any of the preconditions checked in `require_preconditions` are not met. + fn assert_preconditions(&self) { + self.require_preconditions().unwrap_or_else(env_panic); } fn promise_create(promise: PromiseArgs) -> PromiseIndex { @@ -399,3 +365,41 @@ fn to_sdk_pk(key: &aurora_engine_types::parameters::NearPublicKey) -> near_sdk:: // Unwrap should be safe because we only encode valid public keys data.try_into().unwrap() } + +fn require_caller(caller: &AccountId) -> Result<(), Error> { + if caller != &env::predecessor_account_id() { + return Err(Error::IllegalCaller); + } + Ok(()) +} + +fn require_no_failed_promises() -> Result<(), Error> { + let num_promises = env::promise_results_count(); + for index in 0..num_promises { + if let PromiseResult::Failed | PromiseResult::NotReady = env::promise_result(index) { + return Err(Error::CallbackOfFailedPromise); + } + } + Ok(()) +} + +fn env_panic(e: Error) -> T { + env::panic_str(e.as_ref()) +} + +#[derive(Debug)] +enum Error { + ContractNotInitialized, + IllegalCaller, + CallbackOfFailedPromise, +} + +impl AsRef for Error { + fn as_ref(&self) -> &str { + match self { + Self::ContractNotInitialized => "ERR_CONTRACT_NOT_INITIALIZED", + Self::IllegalCaller => ERR_ILLEGAL_CALLER, + Self::CallbackOfFailedPromise => "ERR_CALLBACK_OF_FAILED_PROMISE", + } + } +}