From 896005e1bb0e3e8cedb6a29e8988d1e556119d99 Mon Sep 17 00:00:00 2001 From: Michael Birch Date: Mon, 20 Nov 2023 08:11:43 -0500 Subject: [PATCH] Feat: Exit and XCC promises are sequential (#868) ## Description This PR is addressing a [usability issue with XCC](https://github.com/aurora-is-near/aurora-contracts-sdk/pull/13#discussion_r1329912329) brought up by @karim-en . It is somewhat common that XCC requires tokens to be bridged from the user's Aurora address to their XCC account on Near. Naturally, this bridging needs to happen before the XCC promise resolves so that the tokens are available for it to use. However, due to the async nature of Near we could not guarantee that the bridging would happen before the XCC call. In this PR we change how the Engine produces promises so that they are sequential instead of concurrent (i.e. all the promises produced by the EVM are connected by the `then` combinator, in the order they were produced during the EVM execution). This means a contract which calls the exit precompile and then later calls XCC will have the promises happen in that same order, as the developer intended. ## Performance / NEAR gas cost considerations N/A ## Testing Existing XCC tests --- engine-sdk/src/near_runtime.rs | 244 +++++++++++++---------- engine-sdk/src/promise.rs | 18 ++ engine-standalone-storage/src/promise.rs | 8 + engine-test-doubles/src/promise.rs | 33 +++ engine/src/engine.rs | 31 ++- engine/src/xcc.rs | 27 ++- 6 files changed, 241 insertions(+), 120 deletions(-) diff --git a/engine-sdk/src/near_runtime.rs b/engine-sdk/src/near_runtime.rs index b9a9f3f38..be75fc24d 100644 --- a/engine-sdk/src/near_runtime.rs +++ b/engine-sdk/src/near_runtime.rs @@ -100,6 +100,118 @@ impl Runtime { ); } } + + #[allow(clippy::too_many_lines)] + unsafe fn append_batch_actions(id: u64, args: &PromiseBatchAction) { + for action in &args.actions { + match action { + PromiseAction::CreateAccount => { + exports::promise_batch_action_create_account(id); + } + PromiseAction::Transfer { amount } => { + let amount = amount.as_u128(); + let amount_addr = core::ptr::addr_of!(amount); + exports::promise_batch_action_transfer(id, amount_addr as _); + } + PromiseAction::DeployContract { code } => { + let code = code.as_slice(); + exports::promise_batch_action_deploy_contract( + id, + code.len() as _, + code.as_ptr() as _, + ); + } + PromiseAction::FunctionCall { + name, + gas, + attached_yocto, + args, + } => { + let method_name = name.as_bytes(); + let arguments = args.as_slice(); + let amount = attached_yocto.as_u128(); + let amount_addr = core::ptr::addr_of!(amount); + exports::promise_batch_action_function_call( + id, + method_name.len() as _, + method_name.as_ptr() as _, + arguments.len() as _, + arguments.as_ptr() as _, + amount_addr as _, + gas.as_u64(), + ); + } + PromiseAction::Stake { amount, public_key } => { + feature_gated!("all-promise-actions", { + let amount = amount.as_u128(); + let amount_addr = core::ptr::addr_of!(amount); + let pk: RawPublicKey = public_key.into(); + let pk_bytes = pk.as_bytes(); + exports::promise_batch_action_stake( + id, + amount_addr as _, + pk_bytes.len() as _, + pk_bytes.as_ptr() as _, + ); + }); + } + PromiseAction::AddFullAccessKey { public_key, nonce } => { + let pk: RawPublicKey = public_key.into(); + let pk_bytes = pk.as_bytes(); + exports::promise_batch_action_add_key_with_full_access( + id, + pk_bytes.len() as _, + pk_bytes.as_ptr() as _, + *nonce, + ); + } + PromiseAction::AddFunctionCallKey { + public_key, + nonce, + allowance, + receiver_id, + function_names, + } => { + let pk: RawPublicKey = public_key.into(); + let pk_bytes = pk.as_bytes(); + let allowance = allowance.as_u128(); + let allowance_addr = core::ptr::addr_of!(allowance); + let receiver_id = receiver_id.as_bytes(); + let function_names = function_names.as_bytes(); + exports::promise_batch_action_add_key_with_function_call( + id, + pk_bytes.len() as _, + pk_bytes.as_ptr() as _, + *nonce, + allowance_addr as _, + receiver_id.len() as _, + receiver_id.as_ptr() as _, + function_names.len() as _, + function_names.as_ptr() as _, + ); + } + PromiseAction::DeleteKey { public_key } => { + let pk: RawPublicKey = public_key.into(); + let pk_bytes = pk.as_bytes(); + exports::promise_batch_action_delete_key( + id, + pk_bytes.len() as _, + pk_bytes.as_ptr() as _, + ); + } + PromiseAction::DeleteAccount { beneficiary_id } => { + feature_gated!("all-promise-actions", { + let beneficiary_id = beneficiary_id.as_bytes(); + exports::promise_batch_action_delete_key( + id, + beneficiary_id.len() as _, + beneficiary_id.as_ptr() as _, + ); + }); + } + } + } + } } impl StorageIntermediate for RegisterIndex { @@ -350,120 +462,28 @@ impl crate::promise::PromiseHandler for Runtime { PromiseId::new(id) } - #[allow(clippy::too_many_lines)] unsafe fn promise_create_batch(&mut self, args: &PromiseBatchAction) -> PromiseId { let account_id = args.target_account_id.as_bytes(); let id = { exports::promise_batch_create(account_id.len() as _, account_id.as_ptr() as _) }; - for action in &args.actions { - match action { - PromiseAction::CreateAccount => { - exports::promise_batch_action_create_account(id); - } - PromiseAction::Transfer { amount } => { - let amount = amount.as_u128(); - let amount_addr = core::ptr::addr_of!(amount); - exports::promise_batch_action_transfer(id, amount_addr as _); - } - PromiseAction::DeployContract { code } => { - let code = code.as_slice(); - exports::promise_batch_action_deploy_contract( - id, - code.len() as _, - code.as_ptr() as _, - ); - } - PromiseAction::FunctionCall { - name, - gas, - attached_yocto, - args, - } => { - let method_name = name.as_bytes(); - let arguments = args.as_slice(); - let amount = attached_yocto.as_u128(); - let amount_addr = core::ptr::addr_of!(amount); - exports::promise_batch_action_function_call( - id, - method_name.len() as _, - method_name.as_ptr() as _, - arguments.len() as _, - arguments.as_ptr() as _, - amount_addr as _, - gas.as_u64(), - ); - } - PromiseAction::Stake { amount, public_key } => { - feature_gated!("all-promise-actions", { - let amount = amount.as_u128(); - let amount_addr = core::ptr::addr_of!(amount); - let pk: RawPublicKey = public_key.into(); - let pk_bytes = pk.as_bytes(); - exports::promise_batch_action_stake( - id, - amount_addr as _, - pk_bytes.len() as _, - pk_bytes.as_ptr() as _, - ); - }); - } - PromiseAction::AddFullAccessKey { public_key, nonce } => { - let pk: RawPublicKey = public_key.into(); - let pk_bytes = pk.as_bytes(); - exports::promise_batch_action_add_key_with_full_access( - id, - pk_bytes.len() as _, - pk_bytes.as_ptr() as _, - *nonce, - ); - } - PromiseAction::AddFunctionCallKey { - public_key, - nonce, - allowance, - receiver_id, - function_names, - } => { - let pk: RawPublicKey = public_key.into(); - let pk_bytes = pk.as_bytes(); - let allowance = allowance.as_u128(); - let allowance_addr = core::ptr::addr_of!(allowance); - let receiver_id = receiver_id.as_bytes(); - let function_names = function_names.as_bytes(); - exports::promise_batch_action_add_key_with_function_call( - id, - pk_bytes.len() as _, - pk_bytes.as_ptr() as _, - *nonce, - allowance_addr as _, - receiver_id.len() as _, - receiver_id.as_ptr() as _, - function_names.len() as _, - function_names.as_ptr() as _, - ); - } - PromiseAction::DeleteKey { public_key } => { - let pk: RawPublicKey = public_key.into(); - let pk_bytes = pk.as_bytes(); - exports::promise_batch_action_delete_key( - id, - pk_bytes.len() as _, - pk_bytes.as_ptr() as _, - ); - } - PromiseAction::DeleteAccount { beneficiary_id } => { - feature_gated!("all-promise-actions", { - let beneficiary_id = beneficiary_id.as_bytes(); - exports::promise_batch_action_delete_key( - id, - beneficiary_id.len() as _, - beneficiary_id.as_ptr() as _, - ); - }); - } - } - } + Self::append_batch_actions(id, args); + + PromiseId::new(id) + } + + unsafe fn promise_attach_batch_callback( + &mut self, + base: PromiseId, + args: &PromiseBatchAction, + ) -> PromiseId { + let account_id = args.target_account_id.as_bytes(); + + let id = { + exports::promise_batch_then(base.raw(), account_id.len() as _, account_id.as_ptr() as _) + }; + + Self::append_batch_actions(id, args); PromiseId::new(id) } @@ -658,7 +678,11 @@ pub(crate) mod exports { ) -> u64; pub(crate) fn promise_and(promise_idx_ptr: u64, promise_idx_count: u64) -> u64; pub(crate) fn promise_batch_create(account_id_len: u64, account_id_ptr: u64) -> u64; - fn promise_batch_then(promise_index: u64, account_id_len: u64, account_id_ptr: u64) -> u64; + pub(crate) fn promise_batch_then( + promise_index: u64, + account_id_len: u64, + account_id_ptr: u64, + ) -> u64; // ####################### // # Promise API actions # // ####################### diff --git a/engine-sdk/src/promise.rs b/engine-sdk/src/promise.rs index 67a516348..8658d2605 100644 --- a/engine-sdk/src/promise.rs +++ b/engine-sdk/src/promise.rs @@ -51,6 +51,16 @@ pub trait PromiseHandler { /// code or adding/removing access keys. unsafe fn promise_create_batch(&mut self, args: &PromiseBatchAction) -> PromiseId; + /// # Safety + /// See note on `promise_create_call`. Promise batches in particular must be used very + /// carefully because they can take destructive actions such as deploying new contract + /// code or adding/removing access keys. + unsafe fn promise_attach_batch_callback( + &mut self, + base: PromiseId, + args: &PromiseBatchAction, + ) -> PromiseId; + fn promise_return(&mut self, promise: PromiseId); /// # Safety @@ -132,6 +142,14 @@ impl PromiseHandler for Noop { PromiseId::new(0) } + unsafe fn promise_attach_batch_callback( + &mut self, + _base: PromiseId, + _args: &PromiseBatchAction, + ) -> PromiseId { + PromiseId::new(0) + } + fn promise_return(&mut self, _promise: PromiseId) {} fn read_only(&self) -> Self::ReadOnly { diff --git a/engine-standalone-storage/src/promise.rs b/engine-standalone-storage/src/promise.rs index 35478668e..c76c512d6 100644 --- a/engine-standalone-storage/src/promise.rs +++ b/engine-standalone-storage/src/promise.rs @@ -53,6 +53,14 @@ impl<'a> PromiseHandler for NoScheduler<'a> { PromiseId::new(0) } + unsafe fn promise_attach_batch_callback( + &mut self, + _base: PromiseId, + _args: &PromiseBatchAction, + ) -> PromiseId { + PromiseId::new(0) + } + fn promise_return(&mut self, _promise: PromiseId) {} fn read_only(&self) -> Self::ReadOnly { diff --git a/engine-test-doubles/src/promise.rs b/engine-test-doubles/src/promise.rs index 7da0e1a26..cb4f47103 100644 --- a/engine-test-doubles/src/promise.rs +++ b/engine-test-doubles/src/promise.rs @@ -33,6 +33,22 @@ impl PromiseTracker { self.internal_index += 1; id } + + fn remove_as_near_promise(&mut self, id: u64) -> Option { + let result = match self.scheduled_promises.remove(&id)? { + PromiseArgs::Batch(x) => NearPromise::Simple(SimpleNearPromise::Batch(x)), + PromiseArgs::Create(x) => NearPromise::Simple(SimpleNearPromise::Create(x)), + PromiseArgs::Recursive(x) => x, + PromiseArgs::Callback { base, callback } => { + let base_promise = self.remove_as_near_promise(base.raw())?; + NearPromise::Then { + base: Box::new(base_promise), + callback: SimpleNearPromise::Create(callback), + } + } + }; + Some(result) + } } impl PromiseHandler for PromiseTracker { @@ -91,6 +107,23 @@ impl PromiseHandler for PromiseTracker { PromiseId::new(id) } + unsafe fn promise_attach_batch_callback( + &mut self, + base: PromiseId, + args: &PromiseBatchAction, + ) -> PromiseId { + let id = self.take_id(); + let base_promise = self + .remove_as_near_promise(base.raw()) + .expect("Base promise id must be known"); + let new_promise = PromiseArgs::Recursive(NearPromise::Then { + base: Box::new(base_promise), + callback: SimpleNearPromise::Batch(args.clone()), + }); + self.scheduled_promises.insert(id, new_promise); + PromiseId::new(id) + } + fn promise_return(&mut self, promise: PromiseId) { self.returned_promise = Some(promise); } diff --git a/engine/src/engine.rs b/engine/src/engine.rs index cf1e7bcae..9d0823e72 100644 --- a/engine/src/engine.rs +++ b/engine/src/engine.rs @@ -1621,6 +1621,7 @@ where P: PromiseHandler, I: IO + Copy, { + let mut previous_promise: Option = None; logs.into_iter() .filter_map(|log| { if log.address == exit_to_near::ADDRESS.raw() @@ -1633,15 +1634,33 @@ where // Safety: this promise creation is safe because it does not come from // users directly. The exit precompiles only create promises which we // are able to execute without violating any security invariants. - unsafe { schedule_promise(handler, &promise) } + let id = unsafe { + match previous_promise { + Some(base_id) => { + schedule_promise_callback(handler, base_id, &promise) + } + None => schedule_promise(handler, &promise), + } + }; + previous_promise = Some(id); } PromiseArgs::Callback(promise) => { // Safety: This is safe because the promise data comes from our own // exit precompiles. See note above. - unsafe { - let base_id = schedule_promise(handler, &promise.base); + let base_id = unsafe { + match previous_promise { + Some(base_id) => schedule_promise_callback( + handler, + base_id, + &promise.base, + ), + None => schedule_promise(handler, &promise.base), + } + }; + let id = unsafe { schedule_promise_callback(handler, base_id, &promise.callback) - } + }; + previous_promise = Some(id); } PromiseArgs::Recursive(_) => { unreachable!("Exit precompiles do not produce recursive promises") @@ -1664,13 +1683,15 @@ where let required_near = Yocto::new(U256::from_big_endian(log.topics[1].as_bytes()).low_u128()); if let Ok(promise) = PromiseCreateArgs::try_from_slice(&log.data) { - crate::xcc::handle_precompile_promise( + let id = crate::xcc::handle_precompile_promise( io, handler, + previous_promise, &promise, required_near, current_account_id, ); + previous_promise = Some(id); } } // do not pass on these "internal logs" to caller diff --git a/engine/src/xcc.rs b/engine/src/xcc.rs index 63e1a62fc..fa11798a0 100644 --- a/engine/src/xcc.rs +++ b/engine/src/xcc.rs @@ -179,10 +179,12 @@ where pub fn handle_precompile_promise( io: &I, handler: &mut P, + base_id: Option, promise: &PromiseCreateArgs, required_near: Yocto, current_account_id: &AccountId, -) where +) -> PromiseId +where P: PromiseHandler, I: IO + Copy, { @@ -256,7 +258,12 @@ pub fn handle_precompile_promise( // (not the main engine account), and the actions performed are only (1) create it // for the first time and/or (2) deploy the code from our storage (i.e. the deployed // code is controlled by us, not the user). - let promise_id = unsafe { handler.promise_create_batch(&batch) }; + let promise_id = unsafe { + match base_id { + Some(id) => handler.promise_attach_batch_callback(id, &batch), + None => handler.promise_create_batch(&batch), + } + }; // Add a callback here to update the version of the account let args = AddressVersionUpdateArgs { address: sender, @@ -275,7 +282,7 @@ pub fn handle_precompile_promise( // metadata that has just been deployed above. unsafe { Some(handler.promise_attach_callback(promise_id, &callback)) } } - AddressVersionStatus::UpToDate => None, + AddressVersionStatus::UpToDate => base_id, }; // 2. If some NEAR is required for this call (from storage staking for a new account // and/or attached NEAR to the call the user wants to make), then we need to have the @@ -333,12 +340,12 @@ pub fn handle_precompile_promise( // user directly. The XCC precompile will only construct promises that target the `execute` // and `schedule` methods of the user's router contract. Therefore, the user cannot have // the engine make arbitrary calls. - let _promise_id = unsafe { + unsafe { match withdraw_id { None => handler.promise_create_call(promise), Some(withdraw_id) => handler.promise_attach_callback(withdraw_id, promise), } - }; + } } /// Read the current wasm bytecode for the router contracts @@ -520,6 +527,16 @@ impl<'a, H: PromiseHandler> PromiseHandler for PromiseInterceptor<'a, H> { id } + unsafe fn promise_attach_batch_callback( + &mut self, + base: PromiseId, + args: &PromiseBatchAction, + ) -> PromiseId { + let id = self.inner.promise_attach_batch_callback(base, args); + self.promises.push(id); + id + } + fn promise_return(&mut self, promise: PromiseId) { self.inner.promise_return(promise); }