token-cli: Fix offline signing by making simulated compute unit limit explicit (#6550)

* Improve offline test to actually send the transaction

* token-client: Specify compute unit limit more clearly

* Require compute unit limit if price is set

* Parse args as suggested

* Test using compute unit price for offline signing

* Fixup test

* Set static limit properly for set_interest_rate

* Use clap error directly

Author: joncinque

token/cli/src/clap_app.rs

@@ -17,7 +17,7 @@ use {
     spl_token_2022::instruction::{AuthorityType, MAX_SIGNERS, MIN_SIGNERS},
     std::{fmt, str::FromStr},
     strum::IntoEnumIterator,
-    strum_macros::{EnumIter, EnumString, IntoStaticStr},
+    strum_macros::{AsRefStr, EnumIter, EnumString, IntoStaticStr},
 };
 
 pub type Error = Box<dyn std::error::Error + Send + Sync>;
@@ -78,7 +78,7 @@ pub const COMPUTE_UNIT_LIMIT_ARG: ArgConstant<'static> = ArgConstant {
 
 pub static VALID_TOKEN_PROGRAM_IDS: [Pubkey; 2] = [spl_token_2022::ID, spl_token::ID];
 
-#[derive(Debug, Clone, Copy, PartialEq, EnumString, IntoStaticStr)]
+#[derive(AsRefStr, Debug, Clone, Copy, PartialEq, EnumString, IntoStaticStr)]
 #[strum(serialize_all = "kebab-case")]
 pub enum CommandName {
     CreateToken,
@@ -350,6 +350,7 @@ where
         Err(e) => Err(e),
     }
 }
+
 struct SignOnlyNeedsFullMintSpec {}
 impl offline::ArgsConfig for SignOnlyNeedsFullMintSpec {
     fn sign_only_arg<'a, 'b>(&self, arg: Arg<'a, 'b>) -> Arg<'a, 'b> {

token/cli/src/command.rs

@@ -68,7 +68,7 @@ use {
     },
     spl_token_client::{
         client::{ProgramRpcClientSendTransaction, RpcClientResponse},
-        token::{ExtensionInitializationParams, Token},
+        token::{ComputeUnitLimit, ExtensionInitializationParams, Token},
     },
     spl_token_group_interface::state::TokenGroup,
     spl_token_metadata_interface::state::{Field, TokenMetadata},
@@ -152,11 +152,7 @@ fn config_token_client(
     token: Token<ProgramRpcClientSendTransaction>,
     config: &Config<'_>,
 ) -> Result<Token<ProgramRpcClientSendTransaction>, Error> {
-    let token = if let Some(compute_unit_limit) = config.compute_unit_limit {
-        token.with_compute_unit_limit(compute_unit_limit)
-    } else {
-        token
-    };
+    let token = token.with_compute_unit_limit(config.compute_unit_limit.clone());
 
     let token = if let Some(compute_unit_price) = config.compute_unit_price {
         token.with_compute_unit_price(compute_unit_price)
@@ -432,10 +428,13 @@ async fn command_set_interest_rate(
     rate_bps: i16,
     bulk_signers: Vec<Arc<dyn Signer>>,
 ) -> CommandResult {
+    let mut token = token_client_from_config(config, &token_pubkey, None)?;
     // Because set_interest_rate depends on the time, it can cost more between
     // simulation and execution. To help that, just set a static compute limit
-    let token = base_token_client(config, &token_pubkey, None)?.with_compute_unit_limit(2_500);
-    let token = config_token_client(token, config)?;
+    // if none has been set
+    if !matches!(config.compute_unit_limit, ComputeUnitLimit::Static(_)) {
+        token = token.with_compute_unit_limit(ComputeUnitLimit::Static(2_500));
+    }
 
     if !config.sign_only {
         let mint_account = config.get_account_checked(&token_pubkey).await?;

token/cli/src/config.rs

@@ -20,8 +20,11 @@ use {
         extension::StateWithExtensionsOwned,
         state::{Account, Mint},
     },
-    spl_token_client::client::{
-        ProgramClient, ProgramOfflineClient, ProgramRpcClient, ProgramRpcClientSendTransaction,
+    spl_token_client::{
+        client::{
+            ProgramClient, ProgramOfflineClient, ProgramRpcClient, ProgramRpcClientSendTransaction,
+        },
+        token::ComputeUnitLimit,
     },
     std::{process::exit, rc::Rc, sync::Arc},
 };
@@ -68,7 +71,7 @@ pub struct Config<'a> {
     pub program_id: Pubkey,
     pub restrict_to_program_id: bool,
     pub compute_unit_price: Option<u64>,
-    pub compute_unit_limit: Option<u32>,
+    pub compute_unit_limit: ComputeUnitLimit,
 }
 
 impl<'a> Config<'a> {
@@ -280,9 +283,32 @@ impl<'a> Config<'a> {
                 (default_program_id, false)
             };
 
+        // need to specify a compute limit if compute price and blockhash are specified
+        if matches.is_present(BLOCKHASH_ARG.name)
+            && matches.is_present(COMPUTE_UNIT_PRICE_ARG.name)
+            && !matches.is_present(COMPUTE_UNIT_LIMIT_ARG.name)
+        {
+            clap::Error::with_description(
+                &format!(
+                    "Need to set `{}` if `{}` and `--{}` are set",
+                    COMPUTE_UNIT_LIMIT_ARG.long, COMPUTE_UNIT_PRICE_ARG.long, BLOCKHASH_ARG.long,
+                ),
+                clap::ErrorKind::MissingRequiredArgument,
+            )
+            .exit();
+        }
+
         let nonce_blockhash = value_of(matches, BLOCKHASH_ARG.name);
         let compute_unit_price = value_of(matches, COMPUTE_UNIT_PRICE_ARG.name);
-        let compute_unit_limit = value_of(matches, COMPUTE_UNIT_LIMIT_ARG.name);
+        let compute_unit_limit = value_of(matches, COMPUTE_UNIT_LIMIT_ARG.name)
+            .map(ComputeUnitLimit::Static)
+            .unwrap_or_else(|| {
+                if nonce_blockhash.is_some() {
+                    ComputeUnitLimit::Default
+                } else {
+                    ComputeUnitLimit::Simulated
+                }
+            });
         Self {
             default_signer,
             rpc_client,

token/cli/tests/command.rs

@@ -44,11 +44,16 @@ use {
         client::{
             ProgramClient, ProgramOfflineClient, ProgramRpcClient, ProgramRpcClientSendTransaction,
         },
-        token::Token,
+        token::{ComputeUnitLimit, Token},
     },
     spl_token_group_interface::state::{TokenGroup, TokenGroupMember},
     spl_token_metadata_interface::state::TokenMetadata,
-    std::{ffi::OsString, path::PathBuf, str::FromStr, sync::Arc},
+    std::{
+        ffi::{OsStr, OsString},
+        path::PathBuf,
+        str::FromStr,
+        sync::Arc,
+    },
     tempfile::NamedTempFile,
 };
 
@@ -201,7 +206,7 @@ fn test_config_with_default_signer<'a>(
         program_id: *program_id,
         restrict_to_program_id: true,
         compute_unit_price: None,
-        compute_unit_limit: None,
+        compute_unit_limit: ComputeUnitLimit::Simulated,
     }
 }
 
@@ -230,7 +235,7 @@ fn test_config_without_default_signer<'a>(
         program_id: *program_id,
         restrict_to_program_id: true,
         compute_unit_price: None,
-        compute_unit_limit: None,
+        compute_unit_limit: ComputeUnitLimit::Simulated,
     }
 }
 
@@ -441,7 +446,7 @@ where
     process_command(&sub_command, matches, config, wallet_manager, bulk_signers).await
 }
 
-async fn exec_test_cmd(config: &Config<'_>, args: &[&str]) -> CommandResult {
+async fn exec_test_cmd<T: AsRef<OsStr>>(config: &Config<'_>, args: &[T]) -> CommandResult {
     let default_decimals = format!("{}", spl_token_2022::native_mint::DECIMALS);
     let minimum_signers_help = minimum_signers_help_string();
     let multisig_member_help = multisig_member_help_string();
@@ -2974,10 +2979,17 @@ async fn multisig_transfer(test_validator: &TestValidator, payer: &Keypair) {
     }
 }
 
-async fn offline_multisig_transfer_with_nonce(test_validator: &TestValidator, payer: &Keypair) {
+async fn do_offline_multisig_transfer(
+    test_validator: &TestValidator,
+    payer: &Keypair,
+    compute_unit_price: Option<u64>,
+) {
     let m = 2;
     let n = 3u8;
 
+    let fee_payer_keypair_file = NamedTempFile::new().unwrap();
+    write_keypair_file(payer, &fee_payer_keypair_file).unwrap();
+
     let (multisig_members, multisig_paths): (Vec<_>, Vec<_>) = std::iter::repeat_with(Keypair::new)
         .take(n as usize)
         .map(|s| {
@@ -2988,6 +3000,7 @@ async fn offline_multisig_transfer_with_nonce(test_validator: &TestValidator, pa
         .unzip();
     for program_id in VALID_TOKEN_PROGRAM_IDS.iter() {
         let mut config = test_config_with_default_signer(test_validator, payer, program_id);
+        config.compute_unit_limit = ComputeUnitLimit::Default;
         let token = create_token(&config, payer).await;
         let nonce = create_nonce(&config, payer).await;
 
@@ -3032,58 +3045,121 @@ async fn offline_multisig_transfer_with_nonce(test_validator: &TestValidator, pa
         let program_client: Arc<dyn ProgramClient<ProgramRpcClientSendTransaction>> = Arc::new(
             ProgramOfflineClient::new(blockhash, ProgramRpcClientSendTransaction),
         );
+        let mut args = vec![
+            "spl-token".to_string(),
+            CommandName::Transfer.as_ref().to_string(),
+            token.to_string(),
+            "10".to_string(),
+            destination.to_string(),
+            "--blockhash".to_string(),
+            blockhash.to_string(),
+            "--nonce".to_string(),
+            nonce.to_string(),
+            "--nonce-authority".to_string(),
+            payer.pubkey().to_string(),
+            "--sign-only".to_string(),
+            "--mint-decimals".to_string(),
+            format!("{}", TEST_DECIMALS),
+            "--multisig-signer".to_string(),
+            multisig_paths[1].path().to_str().unwrap().to_string(),
+            "--multisig-signer".to_string(),
+            multisig_members[2].to_string(),
+            "--from".to_string(),
+            source.to_string(),
+            "--owner".to_string(),
+            multisig_pubkey.to_string(),
+            "--fee-payer".to_string(),
+            payer.pubkey().to_string(),
+            "--program-id".to_string(),
+            program_id.to_string(),
+        ];
+        if let Some(compute_unit_price) = compute_unit_price {
+            args.push("--with-compute-unit-price".to_string());
+            args.push(compute_unit_price.to_string());
+            args.push("--with-compute-unit-limit".to_string());
+            args.push(10_000.to_string());
+        }
         config.program_client = program_client;
-        let result = exec_test_cmd(
-            &config,
-            &[
-                "spl-token",
-                CommandName::Transfer.into(),
-                &token.to_string(),
-                "10",
-                &destination.to_string(),
-                "--blockhash",
-                &blockhash.to_string(),
-                "--nonce",
-                &nonce.to_string(),
-                "--nonce-authority",
-                &payer.pubkey().to_string(),
-                "--sign-only",
-                "--mint-decimals",
-                &format!("{}", TEST_DECIMALS),
-                "--multisig-signer",
-                multisig_paths[1].path().to_str().unwrap(),
-                "--multisig-signer",
-                &multisig_members[2].to_string(),
-                "--from",
-                &source.to_string(),
-                "--owner",
-                &multisig_pubkey.to_string(),
-                "--fee-payer",
-                &multisig_members[0].to_string(),
-            ],
-        )
-        .await
-        .unwrap();
+        let result = exec_test_cmd(&config, &args).await.unwrap();
         // the provided signer has a signature, denoted by the pubkey followed
         // by "=" and the signature
-        assert!(result.contains(&format!("{}=", multisig_members[1])));
+        let member_prefix = format!("{}=", multisig_members[1]);
+        let signature_position = result.find(&member_prefix).unwrap();
+        let end_position = result[signature_position..].find('\n').unwrap();
+        let signer = result[signature_position..].get(..end_position).unwrap();
 
         // other three expected signers are absent
         let absent_signers_position = result.find("Absent Signers").unwrap();
         let absent_signers = result.get(absent_signers_position..).unwrap();
-        assert!(absent_signers.contains(&multisig_members[0].to_string()));
         assert!(absent_signers.contains(&multisig_members[2].to_string()));
         assert!(absent_signers.contains(&payer.pubkey().to_string()));
 
         // and nothing else is marked a signer
+        assert!(!absent_signers.contains(&multisig_members[0].to_string()));
         assert!(!absent_signers.contains(&multisig_pubkey.to_string()));
         assert!(!absent_signers.contains(&nonce.to_string()));
         assert!(!absent_signers.contains(&source.to_string()));
         assert!(!absent_signers.contains(&destination.to_string()));
         assert!(!absent_signers.contains(&token.to_string()));
+
+        // now send the transaction
+        let program_client: Arc<dyn ProgramClient<ProgramRpcClientSendTransaction>> = Arc::new(
+            ProgramRpcClient::new(config.rpc_client.clone(), ProgramRpcClientSendTransaction),
+        );
+        config.program_client = program_client;
+        let mut args = vec![
+            "spl-token".to_string(),
+            CommandName::Transfer.as_ref().to_string(),
+            token.to_string(),
+            "10".to_string(),
+            destination.to_string(),
+            "--blockhash".to_string(),
+            blockhash.to_string(),
+            "--nonce".to_string(),
+            nonce.to_string(),
+            "--nonce-authority".to_string(),
+            fee_payer_keypair_file.path().to_str().unwrap().to_string(),
+            "--mint-decimals".to_string(),
+            format!("{}", TEST_DECIMALS),
+            "--multisig-signer".to_string(),
+            multisig_members[1].to_string(),
+            "--multisig-signer".to_string(),
+            multisig_paths[2].path().to_str().unwrap().to_string(),
+            "--from".to_string(),
+            source.to_string(),
+            "--owner".to_string(),
+            multisig_pubkey.to_string(),
+            "--fee-payer".to_string(),
+            fee_payer_keypair_file.path().to_str().unwrap().to_string(),
+            "--program-id".to_string(),
+            program_id.to_string(),
+            "--signer".to_string(),
+            signer.to_string(),
+        ];
+        if let Some(compute_unit_price) = compute_unit_price {
+            args.push("--with-compute-unit-price".to_string());
+            args.push(compute_unit_price.to_string());
+            args.push("--with-compute-unit-limit".to_string());
+            args.push(10_000.to_string());
+        }
+        exec_test_cmd(&config, &args).await.unwrap();
+
+        let account = config.rpc_client.get_account(&source).await.unwrap();
+        let token_account = StateWithExtensionsOwned::<Account>::unpack(account.data).unwrap();
+        let amount = spl_token::ui_amount_to_amount(90.0, TEST_DECIMALS);
+        assert_eq!(token_account.base.amount, amount);
+        let account = config.rpc_client.get_account(&destination).await.unwrap();
+        let token_account = StateWithExtensionsOwned::<Account>::unpack(account.data).unwrap();
+        let amount = spl_token::ui_amount_to_amount(10.0, TEST_DECIMALS);
+        assert_eq!(token_account.base.amount, amount);
     }
 }
 
+async fn offline_multisig_transfer_with_nonce(test_validator: &TestValidator, payer: &Keypair) {
+    do_offline_multisig_transfer(test_validator, payer, None).await;
+    do_offline_multisig_transfer(test_validator, payer, Some(10)).await;
+}
+
 async fn withdraw_excess_lamports_from_multisig(test_validator: &TestValidator, payer: &Keypair) {
     let m = 3;
     let n = 5u8;
@@ -4024,7 +4100,7 @@ async fn compute_budget(test_validator: &TestValidator, payer: &Keypair) {
     for program_id in VALID_TOKEN_PROGRAM_IDS.iter() {
         let mut config = test_config_with_default_signer(test_validator, payer, program_id);
         config.compute_unit_price = Some(42);
-        config.compute_unit_limit = Some(30_000);
+        config.compute_unit_limit = ComputeUnitLimit::Static(30_000);
         run_transfer_test(&config, payer).await;
     }
 }