diff --git a/near-plugins-derive/tests/upgradable.rs b/near-plugins-derive/tests/upgradable.rs index 97ad58e..fa85fd9 100644 --- a/near-plugins-derive/tests/upgradable.rs +++ b/near-plugins-derive/tests/upgradable.rs @@ -561,6 +561,78 @@ async fn test_deploy_code_with_migration_failure_rollback() -> anyhow::Result<() Ok(()) } +/// Deploys staged code in a batch transaction with two function call actions: +/// +/// 1. `up_deploy_code` with a function call to a migration method that fails +/// 2. `up_stage_code` to remove staged code from storage +/// +/// The pitfall is that a failure in the promise returned by 1 does _not_ make the transaction fail +/// and 2 executes anyway. +#[tokio::test] +async fn test_deploy_code_in_batch_transaction_pitfall() -> anyhow::Result<()> { + let worker = workspaces::sandbox().await?; + let dao = worker.dev_create_account().await?; + let setup = Setup::new(worker.clone(), Some(dao.id().clone()), None).await?; + + // Compile the other version of the contract and stage its code. + let code = common::repo::compile_project( + Path::new(PROJECT_PATH_STATE_MIGRATION), + "upgradable_state_migration", + ) + .await?; + let res = setup + .upgradable_contract + .up_stage_code(&dao, code.clone()) + .await?; + assert_success_with_unit_return(res); + setup.assert_staged_code(Some(code)).await; + + // Construct the function call actions to be executed in a batch transaction. + // Note that we are attaching a call to `migrate_with_failure`, which will fail. + let fn_call_deploy = workspaces::operations::Function::new("up_deploy_code") + .args_json(json!({ "function_call_args": FunctionCallArgs { + function_name: "migrate_with_failure".to_string(), + arguments: Vec::new(), + amount: 0, + gas: Gas::ONE_TERA, + } })) + .gas(Gas::ONE_TERA.0 * 200); + let fn_call_remove_code = workspaces::operations::Function::new("up_stage_code") + .args_borsh(Vec::::new()) + .gas(Gas::ONE_TERA.0 * 90); + + let res = dao + .batch(setup.contract.id()) + .call(fn_call_deploy) + .call(fn_call_remove_code) + .transact() + .await?; + + // Here is the pitfall: Despite the failure of `migrate_with_failure`, the transaction succeeds. + // This is due to `fn_call_deploy` _successfully_ returning a promise `p`. The promise `p` + // fails, however that does not affect the result of the transaction. + assert_success_with_unit_return(res.clone()); + + // Verify the promise resulting from `fn_call_deploy` failed. There seems to be no public API to + // get the status of an `ExecutionOutcome`, hence `is_failure` is used in combination with debug + // formatting. Since this is test code we can use debug formatting for this purpose. + let fn_call_deploy_receipt = res + .receipt_outcomes() + .get(1) + .expect("There should be at least two receipts"); + assert!(fn_call_deploy_receipt.is_failure()); + assert!(format!("{:?}", fn_call_deploy_receipt).contains("Failing migration on purpose")); + + // Verify `code` wasn't deployed by calling a function that is defined only in the initial + // contract but not in the contract corresponding to `code`. + setup.assert_is_set_up(&setup.unauth_account).await; + + // However the staged code was removed, i.e. `fn_call_remove_code` was executed anyway. + setup.assert_staged_code(None).await; + + Ok(()) +} + #[tokio::test] async fn test_deploy_code_with_delay() -> anyhow::Result<()> { let worker = workspaces::sandbox().await?; diff --git a/near-plugins/src/upgradable.rs b/near-plugins/src/upgradable.rs index 9d8c8ff..a3829ee 100644 --- a/near-plugins/src/upgradable.rs +++ b/near-plugins/src/upgradable.rs @@ -33,8 +33,9 @@ //! //! ## Stale staged code //! -//! After the code is deployed, it should be removed from staging. This will prevent old code with a -//! security vulnerability to be deployed. +//! After the code is deployed, it should be removed from staging. This prevents deploying old code +//! that might contain a security vulnerability and avoids the issues described in +//! [`Self::up_deploy_code`]. //! //! ## Upgrading code that contains a security vulnerability //! @@ -102,6 +103,8 @@ pub trait Upgradable { /// Allows an authorized account to deploy the staged code. It panics if no code is staged. /// + /// # Attaching a function call + /// /// If `function_call_args` are provided, code is deployed in a batch promise that contains the /// `DeployContractAction` followed by `FunctionCallAction`. In case the function call fails, /// the deployment is rolled back and the initial code remains active. For this purpose, @@ -112,6 +115,26 @@ pub trait Upgradable { /// version of the contract. A failure during state migration can leave the contract in a broken /// state, which is avoided by the roleback mechanism described above. /// + /// # Removal of staged code + /// + /// After deployment, staged code remains in storage. It is not removed automatically as this + /// would cost extra gas and therefore increase the risk of the transaction hitting NEAR's gas + /// limit. Moreover, in case the deployment is roled back due to a failure in the attached + /// function call, the staged code might still be required. + /// + /// Once staged code is no longer needed, it can be removed by passing the appropriate arguments + /// to [`Self::up_stage_code`]. Removing staged code allows to [unstake tokens] that are storage + /// staked. + /// + /// It is recommended to remove staged code as soon as possible to avoid deploying code and + /// executing an attached function call multiple times. Using batch transaction for this purpose + /// can be dangerous. Since `up_deploy_code` returns a promise, there can be unexpected outcomes + /// when it is combined in a batch transaction with another function call that removes code from + /// storage. This is demonstrated in the `Upgradable` test + /// `test_deploy_code_in_batch_transaction_pitfall`. + /// + /// # Permissions + /// /// In the default implementation, this method is protected by access control provided by the /// `AccessControllable` plugin. The roles which may successfully call this method are /// specified via the `code_deployers` field of the `Upgradable` macro's `access_control_roles` @@ -120,6 +143,7 @@ pub trait Upgradable { /// /// [asynchronous design]: https://docs.near.org/concepts/basics/transactions/overview /// [state migration]: https://docs.near.org/develop/upgrade#migrating-the-state + /// [storage staked]: https://docs.near.org/concepts/storage/storage-staking#btw-you-can-remove-data-to-unstake-some-tokens fn up_deploy_code(&mut self, function_call_args: Option) -> Promise; /// Initializes the duration of the delay for deploying the staged code. It defaults to zero if