docs(Upgradable): code removal after deplyoment

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::<u8>::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?;

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<FunctionCallArgs>) -> Promise;
 
     /// Initializes the duration of the delay for deploying the staged code. It defaults to zero if