From f75abb7e6eb91e606fa597521f9059255b54b245 Mon Sep 17 00:00:00 2001 From: Jon C Date: Fri, 5 Apr 2024 00:11:38 +0200 Subject: [PATCH 1/8] Improve offline test to actually send the transaction --- token/cli/tests/command.rs | 63 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/token/cli/tests/command.rs b/token/cli/tests/command.rs index 1e96223545b..309f05458c0 100644 --- a/token/cli/tests/command.rs +++ b/token/cli/tests/command.rs @@ -2978,6 +2978,9 @@ async fn offline_multisig_transfer_with_nonce(test_validator: &TestValidator, pa 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| { @@ -3059,28 +3062,82 @@ async fn offline_multisig_transfer_with_nonce(test_validator: &TestValidator, pa "--owner", &multisig_pubkey.to_string(), "--fee-payer", - &multisig_members[0].to_string(), + &payer.pubkey().to_string(), + "--program-id", + &program_id.to_string(), ], ) .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> = Arc::new( + ProgramRpcClient::new(config.rpc_client.clone(), ProgramRpcClientSendTransaction), + ); + config.program_client = program_client; + 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", + &fee_payer_keypair_file.path().to_str().unwrap(), + "--mint-decimals", + &format!("{}", TEST_DECIMALS), + "--multisig-signer", + &multisig_members[1].to_string(), + "--multisig-signer", + multisig_paths[2].path().to_str().unwrap(), + "--from", + &source.to_string(), + "--owner", + &multisig_pubkey.to_string(), + "--fee-payer", + &fee_payer_keypair_file.path().to_str().unwrap(), + "--program-id", + &program_id.to_string(), + "--signer", + signer, + ], + ) + .await + .unwrap(); + + let account = config.rpc_client.get_account(&source).await.unwrap(); + let token_account = StateWithExtensionsOwned::::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::::unpack(account.data).unwrap(); + let amount = spl_token::ui_amount_to_amount(10.0, TEST_DECIMALS); + assert_eq!(token_account.base.amount, amount); } } From 27fe1e992fda33f95cd35b7f8d3a5b98ee0924f8 Mon Sep 17 00:00:00 2001 From: Jon C Date: Thu, 4 Apr 2024 14:23:53 +0200 Subject: [PATCH 2/8] token-client: Specify compute unit limit more clearly --- token/cli/src/clap_app.rs | 34 ++++++++++- token/cli/src/command.rs | 11 ++-- token/cli/src/config.rs | 19 +++++-- token/cli/tests/command.rs | 9 +-- token/client/src/token.rs | 56 +++++++++++-------- .../tests/confidential_transfer.rs | 6 +- token/program-2022-test/tests/program_test.rs | 11 ++-- 7 files changed, 97 insertions(+), 49 deletions(-) diff --git a/token/cli/src/clap_app.rs b/token/cli/src/clap_app.rs index f82af2eb79b..a3d3eaab821 100644 --- a/token/cli/src/clap_app.rs +++ b/token/cli/src/clap_app.rs @@ -15,6 +15,7 @@ use { }, solana_sdk::{instruction::AccountMeta, pubkey::Pubkey}, spl_token_2022::instruction::{AuthorityType, MAX_SIGNERS, MIN_SIGNERS}, + spl_token_client::token::ComputeUnitLimit, std::{fmt, str::FromStr}, strum::IntoEnumIterator, strum_macros::{EnumIter, EnumString, IntoStaticStr}, @@ -73,7 +74,10 @@ pub const COMPUTE_UNIT_PRICE_ARG: ArgConstant<'static> = ArgConstant { pub const COMPUTE_UNIT_LIMIT_ARG: ArgConstant<'static> = ArgConstant { name: "compute_unit_limit", long: "--with-compute-unit-limit", - help: "Set compute unit limit for transaction, in compute units.", + help: "Set compute unit limit for transaction, in compute units; also accepts \ + keyword SIMULATED to use compute units from transaction simulation prior \ + to sending. Note that SIMULATED may fail if accounts are modified by another \ + transaction between simulation and execution.", }; pub static VALID_TOKEN_PROGRAM_IDS: [Pubkey; 2] = [spl_token_2022::ID, spl_token::ID]; @@ -350,6 +354,32 @@ where Err(e) => Err(e), } } + +fn is_compute_unit_limit_or_simulated(string: T) -> Result<(), String> +where + T: AsRef + fmt::Display, +{ + if string.as_ref().parse::().is_ok() || string.as_ref() == "SIMULATED" { + Ok(()) + } else { + Err(format!( + "Unable to parse input compute unit limit as integer or SIMULATED, provided: {string}" + )) + } +} +pub(crate) fn parse_compute_unit_limit(string: T) -> Result +where + T: AsRef + fmt::Display, +{ + match string.as_ref().parse::() { + Ok(compute_unit_limit) => Ok(ComputeUnitLimit::Static(compute_unit_limit)), + Err(_) if string.as_ref() == "SIMULATED" => Ok(ComputeUnitLimit::Simulated), + _ => Err(format!( + "Unable to parse compute unit limit, provided: {string}" + )), + } +} + struct SignOnlyNeedsFullMintSpec {} impl offline::ArgsConfig for SignOnlyNeedsFullMintSpec { fn sign_only_arg<'a, 'b>(&self, arg: Arg<'a, 'b>) -> Arg<'a, 'b> { @@ -629,7 +659,7 @@ pub fn app<'a, 'b>( .takes_value(true) .global(true) .value_name("COMPUTE-UNIT-LIMIT") - .validator(is_parsable::) + .validator(is_compute_unit_limit_or_simulated) .help(COMPUTE_UNIT_LIMIT_ARG.help) ) .arg( diff --git a/token/cli/src/command.rs b/token/cli/src/command.rs index 80615c01ee8..e902432061d 100644 --- a/token/cli/src/command.rs +++ 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, config: &Config<'_>, ) -> Result, 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) @@ -434,7 +430,8 @@ async fn command_set_interest_rate( ) -> CommandResult { // 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 = base_token_client(config, &token_pubkey, None)? + .with_compute_unit_limit(ComputeUnitLimit::Static(2_500)); let token = config_token_client(token, config)?; if !config.sign_only { diff --git a/token/cli/src/config.rs b/token/cli/src/config.rs index 96072fea064..24b43d9eccd 100644 --- a/token/cli/src/config.rs +++ b/token/cli/src/config.rs @@ -1,5 +1,8 @@ use { - crate::clap_app::{Error, COMPUTE_UNIT_LIMIT_ARG, COMPUTE_UNIT_PRICE_ARG, MULTISIG_SIGNER_ARG}, + crate::clap_app::{ + parse_compute_unit_limit, Error, COMPUTE_UNIT_LIMIT_ARG, COMPUTE_UNIT_PRICE_ARG, + MULTISIG_SIGNER_ARG, + }, clap::ArgMatches, solana_clap_utils::{ input_parsers::{pubkey_of_signer, value_of}, @@ -20,8 +23,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 +74,7 @@ pub struct Config<'a> { pub program_id: Pubkey, pub restrict_to_program_id: bool, pub compute_unit_price: Option, - pub compute_unit_limit: Option, + pub compute_unit_limit: ComputeUnitLimit, } impl<'a> Config<'a> { @@ -282,7 +288,10 @@ impl<'a> Config<'a> { 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 = matches + .value_of(COMPUTE_UNIT_LIMIT_ARG.name) + .map(|x| parse_compute_unit_limit(x).unwrap()) + .unwrap_or(ComputeUnitLimit::Default); Self { default_signer, rpc_client, diff --git a/token/cli/tests/command.rs b/token/cli/tests/command.rs index 309f05458c0..077467cc43e 100644 --- a/token/cli/tests/command.rs +++ b/token/cli/tests/command.rs @@ -44,7 +44,7 @@ use { client::{ ProgramClient, ProgramOfflineClient, ProgramRpcClient, ProgramRpcClientSendTransaction, }, - token::Token, + token::{ComputeUnitLimit, Token}, }, spl_token_group_interface::state::{TokenGroup, TokenGroupMember}, spl_token_metadata_interface::state::TokenMetadata, @@ -201,7 +201,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 +230,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, } } @@ -2991,6 +2991,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; @@ -4081,7 +4082,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; } } diff --git a/token/client/src/token.rs b/token/client/src/token.rs index 59c90be5daf..a9fefc21f70 100644 --- a/token/client/src/token.rs +++ b/token/client/src/token.rs @@ -332,6 +332,13 @@ impl TokenMemo { } } +#[derive(Debug, Clone)] +pub enum ComputeUnitLimit { + Default, + Simulated, + Static(u32), +} + pub struct Token { client: Arc>, pubkey: Pubkey, /* token mint */ @@ -344,7 +351,7 @@ pub struct Token { memo: Arc>>, transfer_hook_accounts: Option>, compute_unit_price: Option, - compute_unit_limit: Option, + compute_unit_limit: ComputeUnitLimit, } impl fmt::Debug for Token { @@ -411,7 +418,7 @@ where memo: Arc::new(RwLock::new(None)), transfer_hook_accounts: None, compute_unit_price: None, - compute_unit_limit: None, + compute_unit_limit: ComputeUnitLimit::Default, } } @@ -466,8 +473,8 @@ where self } - pub fn with_compute_unit_limit(mut self, compute_unit_limit: u32) -> Self { - self.compute_unit_limit = Some(compute_unit_limit); + pub fn with_compute_unit_limit(mut self, compute_unit_limit: ComputeUnitLimit) -> Self { + self.compute_unit_limit = compute_unit_limit; self } @@ -548,19 +555,16 @@ where .simulate_transaction(&transaction) .await .map_err(TokenError::Client)?; - if let Ok(units_consumed) = simulation_result.get_compute_units_consumed() { - // Overwrite the compute unit limit instruction with the actual units consumed - let compute_unit_limit = - u32::try_from(units_consumed).map_err(|x| TokenError::Client(x.into()))?; - instructions - .last_mut() - .expect("Compute budget instruction was added earlier") - .data = ComputeBudgetInstruction::set_compute_unit_limit(compute_unit_limit).data; - } else { - // `get_compute_units_consumed()` fails for offline signing, so we - // catch that error and remove the instruction that was added - instructions.pop(); - } + let units_consumed = simulation_result + .get_compute_units_consumed() + .map_err(TokenError::Client)?; + // Overwrite the compute unit limit instruction with the actual units consumed + let compute_unit_limit = + u32::try_from(units_consumed).map_err(|x| TokenError::Client(x.into()))?; + instructions + .last_mut() + .expect("Compute budget instruction was added earlier") + .data = ComputeBudgetInstruction::set_compute_unit_limit(compute_unit_limit).data; Ok(()) } @@ -619,13 +623,17 @@ where // all instructions have been added to the transaction, so be sure to // keep this instruction as the last one before creating and sending the // transaction. - if let Some(compute_unit_limit) = self.compute_unit_limit { - instructions.push(ComputeBudgetInstruction::set_compute_unit_limit( - compute_unit_limit, - )); - } else { - self.add_compute_unit_limit_from_simulation(&mut instructions, &blockhash) - .await?; + match self.compute_unit_limit { + ComputeUnitLimit::Default => {} + ComputeUnitLimit::Simulated => { + self.add_compute_unit_limit_from_simulation(&mut instructions, &blockhash) + .await?; + } + ComputeUnitLimit::Static(compute_unit_limit) => { + instructions.push(ComputeBudgetInstruction::set_compute_unit_limit( + compute_unit_limit, + )); + } } let message = Message::new_with_blockhash(&instructions, fee_payer, &blockhash); diff --git a/token/program-2022-test/tests/confidential_transfer.rs b/token/program-2022-test/tests/confidential_transfer.rs index de152010318..73a1d77f07b 100644 --- a/token/program-2022-test/tests/confidential_transfer.rs +++ b/token/program-2022-test/tests/confidential_transfer.rs @@ -39,7 +39,7 @@ use { }, spl_token_client::{ proof_generation::transfer_with_fee_split_proof_data, - token::{ExtensionInitializationParams, TokenError as TokenClientError}, + token::{ComputeUnitLimit, ExtensionInitializationParams, TokenError as TokenClientError}, }, std::{convert::TryInto, mem::size_of}, }; @@ -2526,7 +2526,7 @@ async fn confidential_transfer_transfer_with_split_proof_contexts_in_parallel() // With split proofs in parallel, one of the transactions does more work // than the other, which isn't caught during the simulation to discover the // compute unit limit. - let token = token.with_compute_unit_limit(500_000); + let token = token.with_compute_unit_limit(ComputeUnitLimit::Static(500_000)); token .confidential_transfer_transfer_with_split_proofs_in_parallel( &alice_meta.token_account, @@ -2948,7 +2948,7 @@ async fn confidential_transfer_transfer_with_fee_and_split_proof_context_in_para // With split proofs in parallel, one of the transactions does more work // than the other, which isn't caught during the simulation to discover the // compute unit limit. - let token = token.with_compute_unit_limit(500_000); + let token = token.with_compute_unit_limit(ComputeUnitLimit::Static(500_000)); token .confidential_transfer_transfer_with_fee_and_split_proofs_in_parallel( &alice_meta.token_account, diff --git a/token/program-2022-test/tests/program_test.rs b/token/program-2022-test/tests/program_test.rs index ef106aedfb1..0d4979383b4 100644 --- a/token/program-2022-test/tests/program_test.rs +++ b/token/program-2022-test/tests/program_test.rs @@ -20,7 +20,7 @@ use { ProgramBanksClient, ProgramBanksClientProcessTransaction, ProgramClient, SendTransaction, SimulateTransaction, }, - token::{ExtensionInitializationParams, Token, TokenResult}, + token::{ComputeUnitLimit, ExtensionInitializationParams, Token, TokenResult}, }, std::sync::Arc, }; @@ -113,7 +113,8 @@ impl TestContext { &mint_account.pubkey(), Some(decimals), Arc::new(keypair_clone(&payer)), - ); + ) + .with_compute_unit_limit(ComputeUnitLimit::Simulated); let token_unchecked = Token::new( Arc::clone(&client), @@ -121,7 +122,8 @@ impl TestContext { &mint_account.pubkey(), None, Arc::new(payer), - ); + ) + .with_compute_unit_limit(ComputeUnitLimit::Simulated); token .create_mint( @@ -157,7 +159,8 @@ impl TestContext { Token::create_native_mint(Arc::clone(&client), &id(), Arc::new(keypair_clone(&payer))) .await?; // unchecked native is never needed because decimals is known statically - let token_unchecked = Token::new_native(Arc::clone(&client), &id(), Arc::new(payer)); + let token_unchecked = Token::new_native(Arc::clone(&client), &id(), Arc::new(payer)) + .with_compute_unit_limit(ComputeUnitLimit::Simulated); self.token_context = Some(TokenContext { decimals: native_mint::DECIMALS, mint_authority: Keypair::new(), /* bogus */ From 08b78bede685b64d2cc69a02cedd05303f2f3d4f Mon Sep 17 00:00:00 2001 From: Jon C Date: Fri, 5 Apr 2024 00:16:56 +0200 Subject: [PATCH 3/8] Require compute unit limit if price is set --- token/cli/src/clap_app.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/token/cli/src/clap_app.rs b/token/cli/src/clap_app.rs index a3d3eaab821..f45bc4e7fee 100644 --- a/token/cli/src/clap_app.rs +++ b/token/cli/src/clap_app.rs @@ -670,6 +670,7 @@ pub fn app<'a, 'b>( .value_name("COMPUTE-UNIT-PRICE") .validator(is_parsable::) .help(COMPUTE_UNIT_PRICE_ARG.help) + .requires(COMPUTE_UNIT_LIMIT_ARG.name) ) .bench_subcommand() .subcommand(SubCommand::with_name(CommandName::CreateToken.into()).about("Create a new token") From 861f91851cac446758ed3771750a73ffe86b7e7f Mon Sep 17 00:00:00 2001 From: Jon C Date: Thu, 25 Apr 2024 17:33:06 +0200 Subject: [PATCH 4/8] Parse args as suggested --- token/cli/src/clap_app.rs | 34 ++-------------------------------- token/cli/src/config.rs | 30 ++++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 40 deletions(-) diff --git a/token/cli/src/clap_app.rs b/token/cli/src/clap_app.rs index f45bc4e7fee..39f01a1d0b8 100644 --- a/token/cli/src/clap_app.rs +++ b/token/cli/src/clap_app.rs @@ -15,7 +15,6 @@ use { }, solana_sdk::{instruction::AccountMeta, pubkey::Pubkey}, spl_token_2022::instruction::{AuthorityType, MAX_SIGNERS, MIN_SIGNERS}, - spl_token_client::token::ComputeUnitLimit, std::{fmt, str::FromStr}, strum::IntoEnumIterator, strum_macros::{EnumIter, EnumString, IntoStaticStr}, @@ -74,10 +73,7 @@ pub const COMPUTE_UNIT_PRICE_ARG: ArgConstant<'static> = ArgConstant { pub const COMPUTE_UNIT_LIMIT_ARG: ArgConstant<'static> = ArgConstant { name: "compute_unit_limit", long: "--with-compute-unit-limit", - help: "Set compute unit limit for transaction, in compute units; also accepts \ - keyword SIMULATED to use compute units from transaction simulation prior \ - to sending. Note that SIMULATED may fail if accounts are modified by another \ - transaction between simulation and execution.", + help: "Set compute unit limit for transaction, in compute units.", }; pub static VALID_TOKEN_PROGRAM_IDS: [Pubkey; 2] = [spl_token_2022::ID, spl_token::ID]; @@ -355,31 +351,6 @@ where } } -fn is_compute_unit_limit_or_simulated(string: T) -> Result<(), String> -where - T: AsRef + fmt::Display, -{ - if string.as_ref().parse::().is_ok() || string.as_ref() == "SIMULATED" { - Ok(()) - } else { - Err(format!( - "Unable to parse input compute unit limit as integer or SIMULATED, provided: {string}" - )) - } -} -pub(crate) fn parse_compute_unit_limit(string: T) -> Result -where - T: AsRef + fmt::Display, -{ - match string.as_ref().parse::() { - Ok(compute_unit_limit) => Ok(ComputeUnitLimit::Static(compute_unit_limit)), - Err(_) if string.as_ref() == "SIMULATED" => Ok(ComputeUnitLimit::Simulated), - _ => Err(format!( - "Unable to parse compute unit limit, provided: {string}" - )), - } -} - struct SignOnlyNeedsFullMintSpec {} impl offline::ArgsConfig for SignOnlyNeedsFullMintSpec { fn sign_only_arg<'a, 'b>(&self, arg: Arg<'a, 'b>) -> Arg<'a, 'b> { @@ -659,7 +630,7 @@ pub fn app<'a, 'b>( .takes_value(true) .global(true) .value_name("COMPUTE-UNIT-LIMIT") - .validator(is_compute_unit_limit_or_simulated) + .validator(is_parsable::) .help(COMPUTE_UNIT_LIMIT_ARG.help) ) .arg( @@ -670,7 +641,6 @@ pub fn app<'a, 'b>( .value_name("COMPUTE-UNIT-PRICE") .validator(is_parsable::) .help(COMPUTE_UNIT_PRICE_ARG.help) - .requires(COMPUTE_UNIT_LIMIT_ARG.name) ) .bench_subcommand() .subcommand(SubCommand::with_name(CommandName::CreateToken.into()).about("Create a new token") diff --git a/token/cli/src/config.rs b/token/cli/src/config.rs index 24b43d9eccd..26c90358fd2 100644 --- a/token/cli/src/config.rs +++ b/token/cli/src/config.rs @@ -1,8 +1,5 @@ use { - crate::clap_app::{ - parse_compute_unit_limit, Error, COMPUTE_UNIT_LIMIT_ARG, COMPUTE_UNIT_PRICE_ARG, - MULTISIG_SIGNER_ARG, - }, + crate::clap_app::{Error, COMPUTE_UNIT_LIMIT_ARG, COMPUTE_UNIT_PRICE_ARG, MULTISIG_SIGNER_ARG}, clap::ArgMatches, solana_clap_utils::{ input_parsers::{pubkey_of_signer, value_of}, @@ -286,12 +283,29 @@ 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) + { + eprintln!( + "error: need to set {} if {} and {} are set", + COMPUTE_UNIT_LIMIT_ARG.name, COMPUTE_UNIT_PRICE_ARG.name, BLOCKHASH_ARG.name, + ); + exit(1); + } + 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 = matches - .value_of(COMPUTE_UNIT_LIMIT_ARG.name) - .map(|x| parse_compute_unit_limit(x).unwrap()) - .unwrap_or(ComputeUnitLimit::Default); + 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, From 2d2496742db740c713392d7672a45d9da9b0bcac Mon Sep 17 00:00:00 2001 From: Jon C Date: Thu, 25 Apr 2024 17:35:08 +0200 Subject: [PATCH 5/8] Test using compute unit price for offline signing --- token/cli/src/clap_app.rs | 4 +- token/cli/tests/command.rs | 158 +++++++++++++++++++++---------------- 2 files changed, 90 insertions(+), 72 deletions(-) diff --git a/token/cli/src/clap_app.rs b/token/cli/src/clap_app.rs index 39f01a1d0b8..b9817bff002 100644 --- a/token/cli/src/clap_app.rs +++ b/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; @@ -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, diff --git a/token/cli/tests/command.rs b/token/cli/tests/command.rs index 077467cc43e..f89d5b3d668 100644 --- a/token/cli/tests/command.rs +++ b/token/cli/tests/command.rs @@ -48,7 +48,12 @@ use { }, 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, }; @@ -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>(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,7 +2979,11 @@ 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, +) { let m = 2; let n = 3u8; @@ -3036,40 +3045,42 @@ async fn offline_multisig_transfer_with_nonce(test_validator: &TestValidator, pa let program_client: Arc> = 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", - &payer.pubkey().to_string(), - "--program-id", - &program_id.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 let member_prefix = format!("{}=", multisig_members[1]); @@ -3096,40 +3107,42 @@ async fn offline_multisig_transfer_with_nonce(test_validator: &TestValidator, pa ProgramRpcClient::new(config.rpc_client.clone(), ProgramRpcClientSendTransaction), ); config.program_client = program_client; - 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", - &fee_payer_keypair_file.path().to_str().unwrap(), - "--mint-decimals", - &format!("{}", TEST_DECIMALS), - "--multisig-signer", - &multisig_members[1].to_string(), - "--multisig-signer", - multisig_paths[2].path().to_str().unwrap(), - "--from", - &source.to_string(), - "--owner", - &multisig_pubkey.to_string(), - "--fee-payer", - &fee_payer_keypair_file.path().to_str().unwrap(), - "--program-id", - &program_id.to_string(), - "--signer", - signer, - ], - ) - .await - .unwrap(); + 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::::unpack(account.data).unwrap(); @@ -3142,6 +3155,11 @@ async fn offline_multisig_transfer_with_nonce(test_validator: &TestValidator, pa } } +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; From 52ed7785077bd41e9090550366ee19669ce57a07 Mon Sep 17 00:00:00 2001 From: Jon C Date: Thu, 25 Apr 2024 17:35:23 +0200 Subject: [PATCH 6/8] Fixup test --- token/cli/src/command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/token/cli/src/command.rs b/token/cli/src/command.rs index e902432061d..cfd06f03697 100644 --- a/token/cli/src/command.rs +++ b/token/cli/src/command.rs @@ -431,7 +431,7 @@ async fn command_set_interest_rate( // 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(ComputeUnitLimit::Static(2_500)); + .with_compute_unit_limit(ComputeUnitLimit::Static(5_000)); let token = config_token_client(token, config)?; if !config.sign_only { From 68ea4d96a0fadc9f1e18e0abb311b09a762fc3b6 Mon Sep 17 00:00:00 2001 From: Jon C Date: Thu, 25 Apr 2024 23:59:52 +0200 Subject: [PATCH 7/8] Set static limit properly for set_interest_rate --- token/cli/src/command.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/token/cli/src/command.rs b/token/cli/src/command.rs index cfd06f03697..ee675b4e853 100644 --- a/token/cli/src/command.rs +++ b/token/cli/src/command.rs @@ -428,11 +428,13 @@ async fn command_set_interest_rate( rate_bps: i16, bulk_signers: Vec>, ) -> 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(ComputeUnitLimit::Static(5_000)); - 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?; From 4072a9685dbe0ffb74a1d5a41afdb1084881f9ef Mon Sep 17 00:00:00 2001 From: Jon C Date: Fri, 26 Apr 2024 00:07:09 +0200 Subject: [PATCH 8/8] Use clap error directly --- token/cli/src/config.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/token/cli/src/config.rs b/token/cli/src/config.rs index 26c90358fd2..89137a51b31 100644 --- a/token/cli/src/config.rs +++ b/token/cli/src/config.rs @@ -288,11 +288,14 @@ impl<'a> Config<'a> { && matches.is_present(COMPUTE_UNIT_PRICE_ARG.name) && !matches.is_present(COMPUTE_UNIT_LIMIT_ARG.name) { - eprintln!( - "error: need to set {} if {} and {} are set", - COMPUTE_UNIT_LIMIT_ARG.name, COMPUTE_UNIT_PRICE_ARG.name, BLOCKHASH_ARG.name, - ); - exit(1); + 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);