From e843e475fd212f10ef4aef821a70f84c11814783 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Wed, 18 Dec 2024 10:20:11 +0100 Subject: [PATCH 01/36] Validate max gas is greater than zero for in for strk fee settings --- crates/sncast/src/helpers/fee.rs | 34 +++++++++------ crates/sncast/tests/e2e/invoke.rs | 72 +++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 14 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 7492961687..a06099b6fe 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -91,13 +91,20 @@ impl FeeArgs { { bail!("--max-fee should be greater than or equal to --max-gas-unit-price") } - (None, _, _) => FeeSettings::Strk { - max_gas: self.max_gas.map(TryIntoConv::try_into_).transpose()?, - max_gas_unit_price: self - .max_gas_unit_price - .map(TryIntoConv::try_into_) - .transpose()?, - }, + (None, _, _) => { + if let Some(max_gas) = self.max_gas { + if max_gas == Felt::ZERO { + bail!("--max-gas should be greater than 0") + } + } + FeeSettings::Strk { + max_gas: self.max_gas.map(TryIntoConv::try_into_).transpose()?, + max_gas_unit_price: self + .max_gas_unit_price + .map(TryIntoConv::try_into_) + .transpose()?, + } + } (Some(max_fee), None, Some(max_gas_unit_price)) => FeeSettings::Strk { max_gas: Some( max_fee @@ -120,15 +127,14 @@ impl FeeArgs { .await? .l1_gas_price() .price_in_fri; + let max_gas = max_fee + .floor_div(&NonZeroFelt::from_felt_unchecked(max_gas_unit_price)); + if max_gas == Felt::ZERO { + bail!("--max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee") + } FeeSettings::Strk { - max_gas: Some( - max_fee - .floor_div(&NonZeroFelt::from_felt_unchecked( - max_gas_unit_price, - )) - .try_into_()?, - ), + max_gas: Some(max_gas.try_into_()?), max_gas_unit_price: Some(max_gas_unit_price.try_into_()?), } } diff --git a/crates/sncast/tests/e2e/invoke.rs b/crates/sncast/tests/e2e/invoke.rs index 4d7fa721f2..ee543010be 100644 --- a/crates/sncast/tests/e2e/invoke.rs +++ b/crates/sncast/tests/e2e/invoke.rs @@ -388,6 +388,78 @@ fn test_too_low_max_fee() { ); } +#[test] +fn test_max_gas_equal_to_zero() { + let args = vec![ + "--accounts-file", + ACCOUNT_FILE_PATH, + "--account", + "user11", + "--wait", + "invoke", + "--url", + URL, + "--contract-address", + MAP_CONTRACT_ADDRESS_SEPOLIA, + "--function", + "put", + "--calldata", + "0x1", + "0x2", + "--max-gas", + "0", + "--fee-token", + "strk", + ]; + + let snapbox = runner(&args); + let output = snapbox.assert().success(); + + assert_stderr_contains( + output, + indoc! {r" + command: invoke + error: --max-gas should be greater than 0 + "}, + ); +} + +#[test] +fn test_max_gas_calculated_from_max_fee_equal_to_zero() { + let args = vec![ + "--accounts-file", + ACCOUNT_FILE_PATH, + "--account", + "user11", + "--wait", + "invoke", + "--url", + URL, + "--contract-address", + MAP_CONTRACT_ADDRESS_SEPOLIA, + "--function", + "put", + "--calldata", + "0x1", + "0x2", + "--max-fee", + "0", + "--fee-token", + "strk", + ]; + + let snapbox = runner(&args); + let output = snapbox.assert().success(); + + assert_stderr_contains( + output, + indoc! {r" + command: invoke + error: --max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee + "}, + ); +} + #[tokio::test] async fn test_happy_case_cairo_expression_calldata() { let tempdir = create_and_deploy_oz_account().await; From 196ac53c56ce27d10e2f9d316eb4c5b04ff00f7c Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Wed, 18 Dec 2024 10:37:14 +0100 Subject: [PATCH 02/36] Print max fee conversion info when using v3 txs --- crates/sncast/src/helpers/fee.rs | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index a06099b6fe..78b04a8359 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -79,6 +79,9 @@ impl FeeArgs { }) } FeeToken::Strk => { + if self.max_fee.is_some() { + print_max_fee_conversion_info(); + } let settings = match (self.max_fee, self.max_gas, self.max_gas_unit_price) { (Some(_), Some(_), Some(_)) => { bail!("Passing all --max-fee, --max-gas and --max-gas-unit-price is conflicting. Please pass only two of them or less") @@ -113,14 +116,23 @@ impl FeeArgs { ), max_gas_unit_price: Some(max_gas_unit_price.try_into_()?), }, - (Some(max_fee), Some(max_gas), None) => FeeSettings::Strk { - max_gas: Some(max_gas.try_into_()?), - max_gas_unit_price: Some( - max_fee - .floor_div(&NonZeroFelt::from_felt_unchecked(max_gas)) - .try_into_()?, - ), - }, + + (Some(max_fee), Some(max_gas), None) => { + let max_gas_unit_price = + max_fee.floor_div(&NonZeroFelt::from_felt_unchecked(max_gas)); + if max_gas_unit_price == Felt::ZERO { + bail!("--max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee") + } + + FeeSettings::Strk { + max_gas: Some(max_gas.try_into_()?), + max_gas_unit_price: Some( + max_fee + .floor_div(&NonZeroFelt::from_felt_unchecked(max_gas)) + .try_into_()?, + ), + } + } (Some(max_fee), None, None) => { let max_gas_unit_price = provider .get_block_with_tx_hashes(block_id) @@ -267,3 +279,7 @@ fn parse_fee_token(s: &str) -> Result { Ok(parsed_token) } + +fn print_max_fee_conversion_info() { + println!("Specyfing '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags"); +} From beb7e671e9d4555aaa072c51cdb1673e06ba92a8 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Wed, 18 Dec 2024 10:41:53 +0100 Subject: [PATCH 03/36] Fix typo --- crates/sncast/src/helpers/fee.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 78b04a8359..2c51dc3a4e 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -281,5 +281,5 @@ fn parse_fee_token(s: &str) -> Result { } fn print_max_fee_conversion_info() { - println!("Specyfing '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags"); + println!("Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags"); } From 52fa843c3981ee9307ad814fac702ed1f08bd6b7 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Wed, 18 Dec 2024 10:52:01 +0100 Subject: [PATCH 04/36] Adjust test --- crates/sncast/src/helpers/fee.rs | 2 +- crates/sncast/tests/e2e/invoke.rs | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 2c51dc3a4e..de7d2190b2 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -281,5 +281,5 @@ fn parse_fee_token(s: &str) -> Result { } fn print_max_fee_conversion_info() { - println!("Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags"); + println!("Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags\n"); } diff --git a/crates/sncast/tests/e2e/invoke.rs b/crates/sncast/tests/e2e/invoke.rs index ee543010be..ea72768a58 100644 --- a/crates/sncast/tests/e2e/invoke.rs +++ b/crates/sncast/tests/e2e/invoke.rs @@ -454,6 +454,8 @@ fn test_max_gas_calculated_from_max_fee_equal_to_zero() { assert_stderr_contains( output, indoc! {r" + Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags + command: invoke error: --max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee "}, From eacff90921d27eb8d61afceb49af027a03d8ce2e Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Wed, 18 Dec 2024 11:13:25 +0100 Subject: [PATCH 05/36] Update `test_happy_case_strk` --- crates/sncast/tests/e2e/invoke.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/sncast/tests/e2e/invoke.rs b/crates/sncast/tests/e2e/invoke.rs index ea72768a58..1ad3906d6f 100644 --- a/crates/sncast/tests/e2e/invoke.rs +++ b/crates/sncast/tests/e2e/invoke.rs @@ -128,11 +128,13 @@ async fn test_happy_case_strk(class_hash: Felt, account_type: AccountType) { ]; let snapbox = runner(&args).current_dir(tempdir.path()); - let output = snapbox.assert().success().get_output().stdout.clone(); + let output = snapbox.assert().success(); + let stdout = output.get_output().stdout.clone(); - let hash = get_transaction_hash(&output); + let hash = get_transaction_hash(&stdout); let receipt = get_transaction_receipt(hash).await; + assert_stdout_contains(output, "Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags"); assert!(matches!(receipt, Invoke(_))); } From ef43fae4541ccb9d99b1d78a6fc0eecb42651422 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Wed, 18 Dec 2024 11:19:29 +0100 Subject: [PATCH 06/36] Fix tests --- crates/sncast/tests/e2e/account/deploy.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/sncast/tests/e2e/account/deploy.rs b/crates/sncast/tests/e2e/account/deploy.rs index 4ea89bb962..5a638d1dba 100644 --- a/crates/sncast/tests/e2e/account/deploy.rs +++ b/crates/sncast/tests/e2e/account/deploy.rs @@ -857,6 +857,8 @@ pub async fn test_deploy_keystore_other_args() { let snapbox = runner(&args).current_dir(tempdir.path()); snapbox.assert().stdout_matches(indoc! {r" + Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags + command: account deploy transaction_hash: 0x0[..] From 0a2f5bb6f0571e6e5c44f1746c33e9ed0a6096a2 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Wed, 18 Dec 2024 12:49:07 +0100 Subject: [PATCH 07/36] Fix tests --- crates/sncast/tests/e2e/account/deploy.rs | 4 ++++ crates/sncast/tests/e2e/invoke.rs | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/sncast/tests/e2e/account/deploy.rs b/crates/sncast/tests/e2e/account/deploy.rs index 5a638d1dba..f22fa44533 100644 --- a/crates/sncast/tests/e2e/account/deploy.rs +++ b/crates/sncast/tests/e2e/account/deploy.rs @@ -463,6 +463,8 @@ pub async fn test_valid_class_hash() { let snapbox = runner(&args).current_dir(tempdir.path()); snapbox.assert().success().stdout_matches(indoc! {r" + Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags + command: account deploy transaction_hash: [..] @@ -584,6 +586,8 @@ pub async fn test_happy_case_keystore(account_type: &str) { let snapbox = runner(&args).current_dir(tempdir.path()); snapbox.assert().stdout_matches(indoc! {r" + Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags + command: account deploy transaction_hash: 0x0[..] diff --git a/crates/sncast/tests/e2e/invoke.rs b/crates/sncast/tests/e2e/invoke.rs index 1ad3906d6f..c2c5175020 100644 --- a/crates/sncast/tests/e2e/invoke.rs +++ b/crates/sncast/tests/e2e/invoke.rs @@ -456,8 +456,6 @@ fn test_max_gas_calculated_from_max_fee_equal_to_zero() { assert_stderr_contains( output, indoc! {r" - Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags - command: invoke error: --max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee "}, From 8dce237722f6ed425a26d424997819bfeb376673 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Wed, 18 Dec 2024 16:53:49 +0100 Subject: [PATCH 08/36] Check if max gas unit price > 0 when it's passed with max fee --- crates/sncast/src/helpers/fee.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index a06099b6fe..9bbab485d0 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -113,14 +113,22 @@ impl FeeArgs { ), max_gas_unit_price: Some(max_gas_unit_price.try_into_()?), }, - (Some(max_fee), Some(max_gas), None) => FeeSettings::Strk { - max_gas: Some(max_gas.try_into_()?), - max_gas_unit_price: Some( - max_fee - .floor_div(&NonZeroFelt::from_felt_unchecked(max_gas)) - .try_into_()?, - ), - }, + (Some(max_fee), Some(max_gas), None) => { + let max_gas_unit_price = + max_fee.floor_div(&NonZeroFelt::from_felt_unchecked(max_gas)); + if max_gas_unit_price == Felt::ZERO { + bail!("--max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee") + } + + FeeSettings::Strk { + max_gas: Some(max_gas.try_into_()?), + max_gas_unit_price: Some( + max_fee + .floor_div(&NonZeroFelt::from_felt_unchecked(max_gas)) + .try_into_()?, + ), + } + } (Some(max_fee), None, None) => { let max_gas_unit_price = provider .get_block_with_tx_hashes(block_id) From cb3d529eae98e70ab484cc567093d5361cd77c60 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Thu, 19 Dec 2024 00:00:28 +0100 Subject: [PATCH 09/36] Refactor checking if feee args are greater than zero --- crates/sncast/src/helpers/fee.rs | 79 +++++++++++++------------- crates/sncast/tests/integration/fee.rs | 40 ++++++------- 2 files changed, 61 insertions(+), 58 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 9bbab485d0..4ce5a8b429 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -15,16 +15,16 @@ pub struct FeeArgs { pub fee_token: Option, /// Max fee for the transaction. If not provided, will be automatically estimated. - #[clap(short, long)] - pub max_fee: Option, + #[clap(value_parser = parse_non_zero_felt, short, long)] + pub max_fee: Option, /// Max gas amount. If not provided, will be automatically estimated. (Only for STRK fee payment) - #[clap(long)] - pub max_gas: Option, + #[clap(value_parser = parse_non_zero_felt, long)] + pub max_gas: Option, /// Max gas price in Fri. If not provided, will be automatically estimated. (Only for STRK fee payment) - #[clap(long)] - pub max_gas_unit_price: Option, + #[clap(value_parser = parse_non_zero_felt, long)] + pub max_gas_unit_price: Option, } impl From for FeeArgs { @@ -32,7 +32,7 @@ impl From for FeeArgs { match script_fee_settings { ScriptFeeSettings::Eth { max_fee } => Self { fee_token: Some(FeeToken::Eth), - max_fee, + max_fee: max_fee.and_then(|value| NonZeroFelt::try_from(value).ok()), max_gas: None, max_gas_unit_price: None, }, @@ -42,9 +42,13 @@ impl From for FeeArgs { max_gas_unit_price, } => Self { fee_token: Some(FeeToken::Strk), - max_fee, - max_gas: max_gas.map(Into::into), - max_gas_unit_price: max_gas_unit_price.map(Into::into), + max_fee: max_fee.and_then(|value| NonZeroFelt::try_from(value).ok()), + max_gas: max_gas + .map(Into::into) + .and_then(|val: Felt| NonZeroFelt::try_from(val).ok()), + max_gas_unit_price: max_gas_unit_price + .map(Into::into) + .and_then(|val: Felt| NonZeroFelt::try_from(val).ok()), }, } } @@ -75,7 +79,7 @@ impl FeeArgs { "--max-gas-unit-price is not supported for ETH fee payment" ); Ok(FeeSettings::Eth { - max_fee: self.max_fee, + max_fee: self.max_fee.map(Felt::from), }) } FeeToken::Strk => { @@ -91,42 +95,35 @@ impl FeeArgs { { bail!("--max-fee should be greater than or equal to --max-gas-unit-price") } - (None, _, _) => { - if let Some(max_gas) = self.max_gas { - if max_gas == Felt::ZERO { - bail!("--max-gas should be greater than 0") - } - } - FeeSettings::Strk { - max_gas: self.max_gas.map(TryIntoConv::try_into_).transpose()?, - max_gas_unit_price: self - .max_gas_unit_price - .map(TryIntoConv::try_into_) - .transpose()?, - } - } + (None, _, _) => FeeSettings::Strk { + max_gas: self + .max_gas + .map(Felt::from) + .map(TryIntoConv::try_into_) + .transpose()?, + max_gas_unit_price: self + .max_gas_unit_price + .map(Felt::from) + .map(TryIntoConv::try_into_) + .transpose()?, + }, (Some(max_fee), None, Some(max_gas_unit_price)) => FeeSettings::Strk { max_gas: Some( - max_fee - .floor_div(&NonZeroFelt::from_felt_unchecked(max_gas_unit_price)) + Felt::from(max_fee) + .floor_div(&max_gas_unit_price) .try_into_()?, ), - max_gas_unit_price: Some(max_gas_unit_price.try_into_()?), + max_gas_unit_price: Some(Felt::from(max_gas_unit_price).try_into()?), }, (Some(max_fee), Some(max_gas), None) => { - let max_gas_unit_price = - max_fee.floor_div(&NonZeroFelt::from_felt_unchecked(max_gas)); + let max_gas_unit_price = Felt::from(max_fee).floor_div(&max_gas); if max_gas_unit_price == Felt::ZERO { bail!("--max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee") } FeeSettings::Strk { - max_gas: Some(max_gas.try_into_()?), - max_gas_unit_price: Some( - max_fee - .floor_div(&NonZeroFelt::from_felt_unchecked(max_gas)) - .try_into_()?, - ), + max_gas: Some(Felt::from(max_gas).try_into_()?), + max_gas_unit_price: Some(max_gas_unit_price.try_into_()?), } } (Some(max_fee), None, None) => { @@ -135,8 +132,8 @@ impl FeeArgs { .await? .l1_gas_price() .price_in_fri; - let max_gas = max_fee - .floor_div(&NonZeroFelt::from_felt_unchecked(max_gas_unit_price)); + let max_gas = Felt::from(max_fee) + .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?); if max_gas == Felt::ZERO { bail!("--max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee") } @@ -275,3 +272,9 @@ fn parse_fee_token(s: &str) -> Result { Ok(parsed_token) } + +fn parse_non_zero_felt(s: &str) -> Result { + let felt: Felt = s.parse().map_err(|_| "Failed to parse value")?; + felt.try_into() + .map_err(|_| "Value should be greater than 0".to_string()) +} diff --git a/crates/sncast/tests/integration/fee.rs b/crates/sncast/tests/integration/fee.rs index 8e94db6825..f84ebecb2c 100644 --- a/crates/sncast/tests/integration/fee.rs +++ b/crates/sncast/tests/integration/fee.rs @@ -5,8 +5,8 @@ use starknet::accounts::{AccountFactory, OpenZeppelinAccountFactory}; use starknet::providers::jsonrpc::HttpTransport; use starknet::providers::{JsonRpcClient, Provider}; use starknet::signers::{LocalWallet, SigningKey}; +use starknet_types_core::felt::Felt; use url::Url; - const MAX_FEE: u64 = 1_000_000_000_000; async fn get_factory() -> OpenZeppelinAccountFactory> { @@ -26,7 +26,7 @@ async fn test_happy_case_eth() { let args = FeeArgs { fee_token: Some(FeeToken::Eth), - max_fee: Some(100_u32.into()), + max_fee: Some(Felt::from(100_u32).try_into().unwrap()), max_gas: None, max_gas_unit_price: None, }; @@ -50,8 +50,8 @@ async fn test_max_gas_eth() { let args = FeeArgs { fee_token: Some(FeeToken::Eth), - max_fee: Some(100_u32.into()), - max_gas: Some(100_u32.into()), + max_fee: Some(Felt::from(100_u32).try_into().unwrap()), + max_gas: Some(Felt::from(100_u32).try_into().unwrap()), max_gas_unit_price: None, }; @@ -71,9 +71,9 @@ async fn test_max_gas_unit_price_eth() { let args = FeeArgs { fee_token: Some(FeeToken::Eth), - max_fee: Some(100_u32.into()), + max_fee: Some(Felt::from(100).try_into().unwrap()), max_gas: None, - max_gas_unit_price: Some(100_u32.into()), + max_gas_unit_price: Some(Felt::from(100_u32).try_into().unwrap()), }; let error = args @@ -92,9 +92,9 @@ async fn test_all_args() { let args = FeeArgs { fee_token: Some(FeeToken::Strk), - max_fee: Some(100_u32.into()), - max_gas: Some(100_u32.into()), - max_gas_unit_price: Some(100_u32.into()), + max_fee: Some(Felt::from(100_u32).try_into().unwrap()), + max_gas: Some(Felt::from(100_u32).try_into().unwrap()), + max_gas_unit_price: Some(Felt::from(100_u32).try_into().unwrap()), }; let error = args @@ -113,8 +113,8 @@ async fn test_max_fee_less_than_max_gas() { let args = FeeArgs { fee_token: Some(FeeToken::Strk), - max_fee: Some(50_u32.into()), - max_gas: Some(100_u32.into()), + max_fee: Some(Felt::from(50_u32).try_into().unwrap()), + max_gas: Some(Felt::from(100_u32).try_into().unwrap()), max_gas_unit_price: None, }; @@ -134,9 +134,9 @@ async fn test_max_fee_less_than_max_gas_unit_price() { let args = FeeArgs { fee_token: Some(FeeToken::Strk), - max_fee: Some(50_u32.into()), + max_fee: Some(Felt::from(50_u32).try_into().unwrap()), max_gas: None, - max_gas_unit_price: Some(100_u32.into()), + max_gas_unit_price: Some(Felt::from(100).try_into().unwrap()), }; let error = args @@ -154,7 +154,7 @@ async fn test_strk_fee_get_max_fee() { let args = FeeArgs { fee_token: Some(FeeToken::Strk), - max_fee: Some(MAX_FEE.into()), + max_fee: Some(Felt::from(MAX_FEE).try_into().unwrap()), max_gas: None, max_gas_unit_price: None, }; @@ -184,8 +184,8 @@ async fn test_strk_fee_get_max_fee_with_max_gas() { let args = FeeArgs { fee_token: Some(FeeToken::Strk), - max_fee: Some(MAX_FEE.into()), - max_gas: Some(1_000_000_u32.into()), + max_fee: Some(Felt::from(MAX_FEE).try_into().unwrap()), + max_gas: Some(Felt::from(1_000_000_u32).try_into().unwrap()), max_gas_unit_price: None, }; @@ -223,8 +223,8 @@ async fn test_strk_fee_get_max_gas_and_max_gas_unit_price() { let args = FeeArgs { fee_token: Some(FeeToken::Strk), max_fee: None, - max_gas: Some(1_000_000_u32.into()), - max_gas_unit_price: Some(1_000_u32.into()), + max_gas: Some(Felt::from(1_000_000_u32).try_into().unwrap()), + max_gas_unit_price: Some(Felt::from(1_000_u32).try_into().unwrap()), }; let settings = args @@ -247,9 +247,9 @@ async fn test_strk_fee_get_max_fee_with_max_gas_unit_price() { let args = FeeArgs { fee_token: Some(FeeToken::Strk), - max_fee: Some(MAX_FEE.into()), + max_fee: Some(Felt::from(MAX_FEE).try_into().unwrap()), max_gas: None, - max_gas_unit_price: Some(1_000_u32.into()), + max_gas_unit_price: Some(Felt::from(1_000_u32).try_into().unwrap()), }; let settings = args From a411a4edb9960b6f7128f13a6f40d9479921cc6d Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Thu, 19 Dec 2024 00:19:28 +0100 Subject: [PATCH 10/36] Update error messages --- crates/sncast/src/helpers/fee.rs | 4 ++-- crates/sncast/tests/e2e/invoke.rs | 10 ++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 4ce5a8b429..a570749459 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -118,7 +118,7 @@ impl FeeArgs { (Some(max_fee), Some(max_gas), None) => { let max_gas_unit_price = Felt::from(max_fee).floor_div(&max_gas); if max_gas_unit_price == Felt::ZERO { - bail!("--max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee") + bail!("Calculated max gas unit price from provided --max-fee and --max-gas is zero. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price") } FeeSettings::Strk { @@ -135,7 +135,7 @@ impl FeeArgs { let max_gas = Felt::from(max_fee) .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?); if max_gas == Felt::ZERO { - bail!("--max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee") + bail!("Calculated max-gas from provided --max-fee and the current network gas price is zero. Please increase --max-fee to obtain a positive gas amount") } FeeSettings::Strk { diff --git a/crates/sncast/tests/e2e/invoke.rs b/crates/sncast/tests/e2e/invoke.rs index ee543010be..1a96bff6bd 100644 --- a/crates/sncast/tests/e2e/invoke.rs +++ b/crates/sncast/tests/e2e/invoke.rs @@ -413,13 +413,12 @@ fn test_max_gas_equal_to_zero() { ]; let snapbox = runner(&args); - let output = snapbox.assert().success(); + let output = snapbox.assert().code(2); assert_stderr_contains( output, indoc! {r" - command: invoke - error: --max-gas should be greater than 0 + error: invalid value '0' for '--max-gas ': Value should be greater than 0 "}, ); } @@ -449,13 +448,12 @@ fn test_max_gas_calculated_from_max_fee_equal_to_zero() { ]; let snapbox = runner(&args); - let output = snapbox.assert().success(); + let output = snapbox.assert().code(2); assert_stderr_contains( output, indoc! {r" - command: invoke - error: --max-gas calculated from --max-fee should be greater than 0. Please increase --max-fee + error: invalid value '0' for '--max-fee ': Value should be greater than 0 "}, ); } From 397a8ba0596a0b0b8f378b2594e12eeb30f9c741 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Thu, 19 Dec 2024 01:34:31 +0100 Subject: [PATCH 11/36] Update docs and changelog --- CHANGELOG.md | 6 ++++++ docs/src/appendix/sncast/account/deploy.md | 6 +++--- docs/src/appendix/sncast/declare.md | 6 +++--- docs/src/appendix/sncast/deploy.md | 6 +++--- docs/src/appendix/sncast/invoke.md | 6 +++--- docs/src/appendix/sncast/multicall/run.md | 6 +++--- 6 files changed, 21 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dcddb0ec7..0044d94246 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Cast + +### Changed + +- Values passed with `--max-fee`, `--max-gas` and `--max-gas-unit-price` flags are required to be > 0 + ## [0.35.1] - 2024-12-16 ### Forge diff --git a/docs/src/appendix/sncast/account/deploy.md b/docs/src/appendix/sncast/account/deploy.md index 0357908ee1..9bea12f7b4 100644 --- a/docs/src/appendix/sncast/account/deploy.md +++ b/docs/src/appendix/sncast/account/deploy.md @@ -16,7 +16,7 @@ Overrides url from `snfoundry.toml`. ## `--max-fee, -m ` Optional. -Maximum fee for the `deploy_account` transaction in Fri or Wei depending on fee token or transaction version. When not used, defaults to auto-estimation. +Maximum fee for the `deploy_account` transaction in Fri or Wei depending on fee token or transaction version. When not used, defaults to auto-estimation. Must be greater than zero. ## `--fee-token ` Optional. When not used, defaults to STRK. @@ -26,12 +26,12 @@ Token used for fee payment. Possible values: ETH, STRK. ## `--max-gas ` Optional. -Maximum gas for the `deploy_account` transaction. When not used, defaults to auto-estimation. (Only for STRK fee payment) +Maximum gas for the `deploy_account` transaction. When not used, defaults to auto-estimation. Must be greater than zero. (Only for STRK fee payment) ## ` --max-gas-unit-price ` Optional. -Maximum gas unit price for the `deploy_account` transaction paid in Fri. When not used, defaults to auto-estimation. (Only for STRK fee payment) +Maximum gas unit price for the `deploy_account` transaction paid in Fri. When not used, defaults to auto-estimation. Must be greater than zero. (Only for STRK fee payment) ## `--version, -v ` Optional. When not used, defaults to v3. diff --git a/docs/src/appendix/sncast/declare.md b/docs/src/appendix/sncast/declare.md index 28247ee49b..7ee362515c 100644 --- a/docs/src/appendix/sncast/declare.md +++ b/docs/src/appendix/sncast/declare.md @@ -20,7 +20,7 @@ Overrides url from `snfoundry.toml`. ## `--max-fee, -m ` Optional. -Maximum fee for the `declare` transaction in Fri or Wei depending on fee token or transaction version. When not used, defaults to auto-estimation. +Maximum fee for the `declare` transaction in Fri or Wei depending on fee token or transaction version. When not used, defaults to auto-estimation. Must be greater than zero. ## `--fee-token ` Optional. When not used, defaults to STRK. @@ -30,12 +30,12 @@ Token used for fee payment. Possible values: ETH, STRK. ## `--max-gas ` Optional. -Maximum gas for the `declare` transaction. When not used, defaults to auto-estimation. (Only for STRK fee payment) +Maximum gas for the `declare` transaction. When not used, defaults to auto-estimation. Must be greater than zero. (Only for STRK fee payment) ## ` --max-gas-unit-price ` Optional. -Maximum gas unit price for the `declare` transaction paid in Fri. When not used, defaults to auto-estimation. (Only for STRK fee payment) +Maximum gas unit price for the `declare` transaction paid in Fri. When not used, defaults to auto-estimation. Must be greater than zero. (Only for STRK fee payment) ## `--version, -v ` Optional. When not used, defaults to v3. diff --git a/docs/src/appendix/sncast/deploy.md b/docs/src/appendix/sncast/deploy.md index f5a611cdb7..f10c16185c 100644 --- a/docs/src/appendix/sncast/deploy.md +++ b/docs/src/appendix/sncast/deploy.md @@ -35,7 +35,7 @@ If passed, the salt will be additionally modified with an account address. ## `--max-fee, -m ` Optional. -Maximum fee for the `deploy` transaction in Fri or Wei depending on fee token or transaction version. When not used, defaults to auto-estimation. +Maximum fee for the `deploy` transaction in Fri or Wei depending on fee token or transaction version. When not used, defaults to auto-estimation. Must be greater than zero. ## `--fee-token ` Optional. When not used, defaults to STRK. @@ -45,12 +45,12 @@ Token used for fee payment. Possible values: ETH, STRK. ## `--max-gas ` Optional. -Maximum gas for the `deploy` transaction. When not used, defaults to auto-estimation. (Only for STRK fee payment) +Maximum gas for the `deploy` transaction. When not used, defaults to auto-estimation. Must be greater than zero. (Only for STRK fee payment) ## ` --max-gas-unit-price ` Optional. -Maximum gas unit price for the `deploy` transaction paid in Fri. When not used, defaults to auto-estimation. (Only for STRK fee payment) +Maximum gas unit price for the `deploy` transaction paid in Fri. When not used, defaults to auto-estimation. Must be greater than zero. (Only for STRK fee payment) ## `--version, -v ` Optional. When not used, defaults to v3. diff --git a/docs/src/appendix/sncast/invoke.md b/docs/src/appendix/sncast/invoke.md index d5922648a3..313dee972d 100644 --- a/docs/src/appendix/sncast/invoke.md +++ b/docs/src/appendix/sncast/invoke.md @@ -31,7 +31,7 @@ Overrides url from `snfoundry.toml`. ## `--max-fee, -m ` Optional. -Maximum fee for the `invoke` transaction in Fri or Wei depending on fee token or transaction version. When not used, defaults to auto-estimation. +Maximum fee for the `invoke` transaction in Fri or Wei depending on fee token or transaction version. When not used, defaults to auto-estimation. Must be greater than zero. ## `--fee-token ` Optional. When not used, defaults to STRK. @@ -41,12 +41,12 @@ Token used for fee payment. Possible values: ETH, STRK. ## `--max-gas ` Optional. -Maximum gas for the `invoke` transaction. When not used, defaults to auto-estimation. (Only for STRK fee payment) +Maximum gas for the `invoke` transaction. When not used, defaults to auto-estimation. Must be greater than zero. (Only for STRK fee payment) ## ` --max-gas-unit-price ` Optional. -Maximum gas unit price for the `invoke` transaction paid in Fri. When not used, defaults to auto-estimation. (Only for STRK fee payment) +Maximum gas unit price for the `invoke` transaction paid in Fri. When not used, defaults to auto-estimation. Must be greater than zero. (Only for STRK fee payment) ## `--version, -v ` Optional. When not used, defaults to v3. diff --git a/docs/src/appendix/sncast/multicall/run.md b/docs/src/appendix/sncast/multicall/run.md index ad481c2833..b2a5ae4809 100644 --- a/docs/src/appendix/sncast/multicall/run.md +++ b/docs/src/appendix/sncast/multicall/run.md @@ -21,7 +21,7 @@ Overrides url from `snfoundry.toml`. ## `--max-fee, -m ` Optional. -Maximum fee for the `invoke` transaction in Fri or Wei depending on fee token or transaction version. When not used, defaults to auto-estimation. +Maximum fee for the `invoke` transaction in Fri or Wei depending on fee token or transaction version. When not used, defaults to auto-estimation. Must be greater than zero. ## `--fee-token ` Optional. When not used, defaults to STRK. @@ -31,12 +31,12 @@ Token used for fee payment. Possible values: ETH, STRK. ## `--max-gas ` Optional. -Maximum gas for the `invoke` transaction. When not used, defaults to auto-estimation. (Only for STRK fee payment) +Maximum gas for the `invoke` transaction. When not used, defaults to auto-estimation. Must be greater than zero. (Only for STRK fee payment) ## ` --max-gas-unit-price ` Optional. -Maximum gas unit price for the `invoke` transaction paid in Fri. When not used, defaults to auto-estimation. (Only for STRK fee payment) +Maximum gas unit price for the `invoke` transaction paid in Fri. When not used, defaults to auto-estimation. Must be greater than zero. (Only for STRK fee payment) ## `--version, -v ` Optional. When not used, defaults to v3. From 77f4d4d97fa01971fe255a89f4173cb64987f2f9 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Thu, 19 Dec 2024 01:39:12 +0100 Subject: [PATCH 12/36] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0044d94246..c446758d19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Values passed with `--max-fee`, `--max-gas` and `--max-gas-unit-price` flags are required to be > 0 +- Values passed to the `--max-fee`, `--max-gas`, and `--max-gas-unit-price` flags must be greater than 0 ## [0.35.1] - 2024-12-16 From e5c22d260258b5a7eb6945cbe994aea8c71f1026 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Thu, 19 Dec 2024 11:52:41 +0100 Subject: [PATCH 13/36] Refactor checking if calculated value is zero --- crates/sncast/src/helpers/fee.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index a570749459..e46566a39b 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -116,14 +116,11 @@ impl FeeArgs { max_gas_unit_price: Some(Felt::from(max_gas_unit_price).try_into()?), }, (Some(max_fee), Some(max_gas), None) => { - let max_gas_unit_price = Felt::from(max_fee).floor_div(&max_gas); - if max_gas_unit_price == Felt::ZERO { - bail!("Calculated max gas unit price from provided --max-fee and --max-gas is zero. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price") - } + let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)).expect("Calculated max gas unit price from provided --max-fee and --max-gas is zero. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price"); FeeSettings::Strk { max_gas: Some(Felt::from(max_gas).try_into_()?), - max_gas_unit_price: Some(max_gas_unit_price.try_into_()?), + max_gas_unit_price: Some(Felt::from(max_gas_unit_price).try_into_()?), } } (Some(max_fee), None, None) => { @@ -132,14 +129,11 @@ impl FeeArgs { .await? .l1_gas_price() .price_in_fri; - let max_gas = Felt::from(max_fee) - .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?); - if max_gas == Felt::ZERO { - bail!("Calculated max-gas from provided --max-fee and the current network gas price is zero. Please increase --max-fee to obtain a positive gas amount") - } + let max_gas = NonZeroFelt::try_from(Felt::from(max_fee) + .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?)).expect("Calculated max-gas from provided --max-fee and the current network gas price is zero. Please increase --max-fee to obtain a positive gas amount"); FeeSettings::Strk { - max_gas: Some(max_gas.try_into_()?), + max_gas: Some(Felt::from(max_gas).try_into_()?), max_gas_unit_price: Some(max_gas_unit_price.try_into_()?), } } From 969e37b0551bc6ee68880d17216c5b0c03711af5 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Thu, 19 Dec 2024 11:58:20 +0100 Subject: [PATCH 14/36] Cover case when max fee and max gas unit price are passed --- crates/sncast/src/helpers/fee.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index e46566a39b..cb7a9bfa62 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -107,17 +107,15 @@ impl FeeArgs { .map(TryIntoConv::try_into_) .transpose()?, }, - (Some(max_fee), None, Some(max_gas_unit_price)) => FeeSettings::Strk { - max_gas: Some( - Felt::from(max_fee) - .floor_div(&max_gas_unit_price) - .try_into_()?, - ), - max_gas_unit_price: Some(Felt::from(max_gas_unit_price).try_into()?), - }, + (Some(max_fee), None, Some(max_gas_unit_price)) => { + let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)).expect("Calculated max gas from provided --max-fee and --max-gas-unit-price is zero. Please increase --max-fee to obtain a positive gas amount"); + FeeSettings::Strk { + max_gas: Some(Felt::from(max_gas).try_into_()?), + max_gas_unit_price: Some(Felt::from(max_gas_unit_price).try_into()?), + } + } (Some(max_fee), Some(max_gas), None) => { let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)).expect("Calculated max gas unit price from provided --max-fee and --max-gas is zero. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price"); - FeeSettings::Strk { max_gas: Some(Felt::from(max_gas).try_into_()?), max_gas_unit_price: Some(Felt::from(max_gas_unit_price).try_into_()?), @@ -131,7 +129,6 @@ impl FeeArgs { .price_in_fri; let max_gas = NonZeroFelt::try_from(Felt::from(max_fee) .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?)).expect("Calculated max-gas from provided --max-fee and the current network gas price is zero. Please increase --max-fee to obtain a positive gas amount"); - FeeSettings::Strk { max_gas: Some(Felt::from(max_gas).try_into_()?), max_gas_unit_price: Some(max_gas_unit_price.try_into_()?), From 436e51ff0e913c97fba9046fec601dfe8479b49c Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Thu, 19 Dec 2024 13:04:56 +0100 Subject: [PATCH 15/36] Display converted values in `print_max_fee_conversion_info` --- crates/sncast/src/helpers/fee.rs | 24 ++++++++++++++++++----- crates/sncast/tests/e2e/account/deploy.rs | 3 +++ crates/sncast/tests/e2e/invoke.rs | 8 +++++++- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 90dd3036e1..3984c7746d 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -83,9 +83,6 @@ impl FeeArgs { }) } FeeToken::Strk => { - if self.max_fee.is_some() { - print_max_fee_conversion_info(); - } let settings = match (self.max_fee, self.max_gas, self.max_gas_unit_price) { (Some(_), Some(_), Some(_)) => { bail!("Passing all --max-fee, --max-gas and --max-gas-unit-price is conflicting. Please pass only two of them or less") @@ -112,6 +109,11 @@ impl FeeArgs { }, (Some(max_fee), None, Some(max_gas_unit_price)) => { let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)).expect("Calculated max gas from provided --max-fee and --max-gas-unit-price is zero. Please increase --max-fee to obtain a positive gas amount"); + print_max_fee_conversion_info( + max_fee.into(), + max_gas.into(), + max_gas_unit_price.into(), + ); FeeSettings::Strk { max_gas: Some(Felt::from(max_gas).try_into_()?), max_gas_unit_price: Some(Felt::from(max_gas_unit_price).try_into()?), @@ -119,6 +121,11 @@ impl FeeArgs { } (Some(max_fee), Some(max_gas), None) => { let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)).expect("Calculated max gas unit price from provided --max-fee and --max-gas is zero. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price"); + print_max_fee_conversion_info( + max_fee.into(), + max_gas.into(), + max_gas_unit_price.into(), + ); FeeSettings::Strk { max_gas: Some(Felt::from(max_gas).try_into_()?), max_gas_unit_price: Some(Felt::from(max_gas_unit_price).try_into_()?), @@ -132,6 +139,11 @@ impl FeeArgs { .price_in_fri; let max_gas = NonZeroFelt::try_from(Felt::from(max_fee) .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?)).expect("Calculated max-gas from provided --max-fee and the current network gas price is zero. Please increase --max-fee to obtain a positive gas amount"); + print_max_fee_conversion_info( + max_fee.into(), + max_gas.into(), + max_gas_unit_price, + ); FeeSettings::Strk { max_gas: Some(Felt::from(max_gas).try_into_()?), max_gas_unit_price: Some(max_gas_unit_price.try_into_()?), @@ -267,8 +279,10 @@ fn parse_fee_token(s: &str) -> Result { Ok(parsed_token) } -fn print_max_fee_conversion_info() { - println!("Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags\n"); +fn print_max_fee_conversion_info(max_fee: Felt, max_gas: Felt, max_gas_unit_price: Felt) { + println!( + "Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags\nConverted {max_fee} max fee to {max_gas} max gas and {max_gas_unit_price} max gas unit price", + ); } fn parse_non_zero_felt(s: &str) -> Result { diff --git a/crates/sncast/tests/e2e/account/deploy.rs b/crates/sncast/tests/e2e/account/deploy.rs index f22fa44533..e09157b110 100644 --- a/crates/sncast/tests/e2e/account/deploy.rs +++ b/crates/sncast/tests/e2e/account/deploy.rs @@ -464,6 +464,7 @@ pub async fn test_valid_class_hash() { snapbox.assert().success().stdout_matches(indoc! {r" Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags + Converted [..] max fee to [..] max gas and [..] max gas unit price command: account deploy transaction_hash: [..] @@ -587,6 +588,7 @@ pub async fn test_happy_case_keystore(account_type: &str) { snapbox.assert().stdout_matches(indoc! {r" Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags + Converted [..] max fee to [..] max gas and [..] max gas unit price command: account deploy transaction_hash: 0x0[..] @@ -862,6 +864,7 @@ pub async fn test_deploy_keystore_other_args() { let snapbox = runner(&args).current_dir(tempdir.path()); snapbox.assert().stdout_matches(indoc! {r" Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags + Converted [..] max fee to [..] max gas and [..] max gas unit price command: account deploy transaction_hash: 0x0[..] diff --git a/crates/sncast/tests/e2e/invoke.rs b/crates/sncast/tests/e2e/invoke.rs index 7801bc50ae..c2cb09cf46 100644 --- a/crates/sncast/tests/e2e/invoke.rs +++ b/crates/sncast/tests/e2e/invoke.rs @@ -134,7 +134,13 @@ async fn test_happy_case_strk(class_hash: Felt, account_type: AccountType) { let hash = get_transaction_hash(&stdout); let receipt = get_transaction_receipt(hash).await; - assert_stdout_contains(output, "Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags"); + assert_stdout_contains( + output, + indoc! { + "Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags + Converted [..] max fee to [..] max gas and [..] max gas unit price" + }, + ); assert!(matches!(receipt, Invoke(_))); } From ee9b354df3270f82a54ed9df5b224dff3a28ebcd Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Thu, 19 Dec 2024 17:44:38 +0100 Subject: [PATCH 16/36] Add newline ad the end of fee conversion message --- crates/sncast/src/helpers/fee.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 3984c7746d..a93112bace 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -281,7 +281,7 @@ fn parse_fee_token(s: &str) -> Result { fn print_max_fee_conversion_info(max_fee: Felt, max_gas: Felt, max_gas_unit_price: Felt) { println!( - "Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags\nConverted {max_fee} max fee to {max_gas} max gas and {max_gas_unit_price} max gas unit price", + "Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags\nConverted {max_fee} max fee to {max_gas} max gas and {max_gas_unit_price} max gas unit price\n", ); } From 4788a9b4856faedd0992dfdd57087f16986665ed Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Thu, 19 Dec 2024 20:08:02 +0100 Subject: [PATCH 17/36] Refactor `ScriptFeeSettings` and `FeeSettings` --- crates/conversions/src/felt.rs | 43 +++++++++++- .../src/serde/deserialize/deserialize_impl.rs | 25 ++++++- crates/sncast/src/helpers/fee.rs | 69 ++++++++++--------- .../src/starknet_commands/account/deploy.rs | 14 +++- .../sncast/src/starknet_commands/declare.rs | 16 +++-- crates/sncast/src/starknet_commands/deploy.rs | 6 +- crates/sncast/src/starknet_commands/invoke.rs | 18 ++++- crates/sncast/tests/integration/fee.rs | 37 +++++----- 8 files changed, 158 insertions(+), 70 deletions(-) diff --git a/crates/conversions/src/felt.rs b/crates/conversions/src/felt.rs index 12aebdf173..f3122aa456 100644 --- a/crates/conversions/src/felt.rs +++ b/crates/conversions/src/felt.rs @@ -2,12 +2,15 @@ use crate::{ byte_array::ByteArray, serde::serialize::SerializeToFeltVec, string::{TryFromDecStr, TryFromHexStr}, - FromConv, IntoConv, + FromConv, IntoConv, TryFromConv, }; use conversions::padded_felt::PaddedFelt; use starknet_api::core::{ClassHash, ContractAddress, EntryPointSelector, Nonce}; use starknet_types_core::felt::{Felt, FromStrError}; -use std::vec; +use std::{ + num::{NonZeroU128, NonZeroU64}, + vec, +}; impl FromConv for Felt { fn from_(value: ClassHash) -> Felt { @@ -39,6 +42,42 @@ impl FromConv for Felt { } } +impl TryFromConv for NonZeroU64 { + type Error = String; + fn try_from_(value: Felt) -> Result { + if value == Felt::ZERO { + Err("value should be greater than 0".to_string()) + } else { + let value: u64 = value.try_into().expect("failed to convert Felt to u64"); + Ok(NonZeroU64::new(value).unwrap()) + } + } +} + +impl TryFromConv for NonZeroU128 { + type Error = String; + fn try_from_(value: Felt) -> Result { + if value == Felt::ZERO { + Err("value should be greater than 0".to_string()) + } else { + let value: u128 = value.try_into().expect("failed to convert Felt to u128"); + Ok(NonZeroU128::new(value).unwrap()) + } + } +} + +impl FromConv for Felt { + fn from_(value: NonZeroU64) -> Felt { + Felt::from(value.get()) + } +} + +impl FromConv for Felt { + fn from_(value: NonZeroU128) -> Felt { + Felt::from(value.get()) + } +} + impl TryFromDecStr for T where T: FromConv, diff --git a/crates/conversions/src/serde/deserialize/deserialize_impl.rs b/crates/conversions/src/serde/deserialize/deserialize_impl.rs index fc31ddb067..2f39246b1b 100644 --- a/crates/conversions/src/serde/deserialize/deserialize_impl.rs +++ b/crates/conversions/src/serde/deserialize/deserialize_impl.rs @@ -3,8 +3,8 @@ use crate::{byte_array::ByteArray, IntoConv}; use num_traits::cast::ToPrimitive; use starknet::providers::Url; use starknet_api::core::{ClassHash, ContractAddress, EntryPointSelector, Nonce}; -use starknet_types_core::felt::Felt; -use std::num::NonZeroU32; +use starknet_types_core::felt::{Felt, NonZeroFelt}; +use std::num::{NonZeroU128, NonZeroU32, NonZeroU64}; impl CairoDeserialize for Url { fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { @@ -71,6 +71,27 @@ impl CairoDeserialize for bool { } } +impl CairoDeserialize for NonZeroFelt { + fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { + let felt = reader.read::()?; + NonZeroFelt::try_from(felt).map_err(|_| BufferReadError::ParseFailed) + } +} + +impl CairoDeserialize for NonZeroU64 { + fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { + let val: u64 = reader.read()?; + NonZeroU64::try_from(val).map_err(|_| BufferReadError::ParseFailed) + } +} + +impl CairoDeserialize for NonZeroU128 { + fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { + let val: u128 = reader.read()?; + NonZeroU128::try_from(val).map_err(|_| BufferReadError::ParseFailed) + } +} + macro_rules! impl_deserialize_for_felt_type { ($type:ty) => { impl CairoDeserialize for $type { diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index cb7a9bfa62..328ad4550e 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -1,12 +1,14 @@ -use anyhow::{bail, ensure, Error, Result}; +use anyhow::{bail, ensure, Context, Error, Result}; use clap::{Args, ValueEnum}; -use conversions::serde::deserialize::CairoDeserialize; -use conversions::TryIntoConv; +use conversions::{serde::deserialize::CairoDeserialize, FromConv, TryFromConv}; use shared::print::print_as_warning; use starknet::core::types::BlockId; use starknet::providers::Provider; use starknet_types_core::felt::{Felt, NonZeroFelt}; -use std::str::FromStr; +use std::{ + num::{NonZeroU128, NonZeroU64}, + str::FromStr, +}; #[derive(Args, Debug, Clone)] pub struct FeeArgs { @@ -32,7 +34,7 @@ impl From for FeeArgs { match script_fee_settings { ScriptFeeSettings::Eth { max_fee } => Self { fee_token: Some(FeeToken::Eth), - max_fee: max_fee.and_then(|value| NonZeroFelt::try_from(value).ok()), + max_fee, max_gas: None, max_gas_unit_price: None, }, @@ -42,13 +44,10 @@ impl From for FeeArgs { max_gas_unit_price, } => Self { fee_token: Some(FeeToken::Strk), - max_fee: max_fee.and_then(|value| NonZeroFelt::try_from(value).ok()), - max_gas: max_gas - .map(Into::into) - .and_then(|val: Felt| NonZeroFelt::try_from(val).ok()), + max_fee, + max_gas: max_gas.and_then(|val| NonZeroFelt::try_from(Felt::from_(val)).ok()), max_gas_unit_price: max_gas_unit_price - .map(Into::into) - .and_then(|val: Felt| NonZeroFelt::try_from(val).ok()), + .and_then(|val| NonZeroFelt::try_from(Felt::from_(val)).ok()), }, } } @@ -79,7 +78,7 @@ impl FeeArgs { "--max-gas-unit-price is not supported for ETH fee payment" ); Ok(FeeSettings::Eth { - max_fee: self.max_fee.map(Felt::from), + max_fee: self.max_fee, }) } FeeToken::Strk => { @@ -98,27 +97,29 @@ impl FeeArgs { (None, _, _) => FeeSettings::Strk { max_gas: self .max_gas - .map(Felt::from) - .map(TryIntoConv::try_into_) - .transpose()?, + .and_then(|val| NonZeroU64::try_from_(Felt::from(val)).ok()), max_gas_unit_price: self .max_gas_unit_price - .map(Felt::from) - .map(TryIntoConv::try_into_) - .transpose()?, + .and_then(|val| NonZeroU128::try_from_(Felt::from(val)).ok()), }, (Some(max_fee), None, Some(max_gas_unit_price)) => { - let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)).expect("Calculated max gas from provided --max-fee and --max-gas-unit-price is zero. Please increase --max-fee to obtain a positive gas amount"); + let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)).context("Calculated max gas from provided --max-fee and --max-gas-unit-price is zero. Please increase --max-fee to obtain a positive gas amount")?; FeeSettings::Strk { - max_gas: Some(Felt::from(max_gas).try_into_()?), - max_gas_unit_price: Some(Felt::from(max_gas_unit_price).try_into()?), + max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), + max_gas_unit_price: NonZeroU128::try_from_(Felt::from( + max_gas_unit_price, + )) + .ok(), } } (Some(max_fee), Some(max_gas), None) => { - let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)).expect("Calculated max gas unit price from provided --max-fee and --max-gas is zero. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price"); + let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)).context("Calculated max gas unit price from provided --max-fee and --max-gas is zero. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price")?; FeeSettings::Strk { - max_gas: Some(Felt::from(max_gas).try_into_()?), - max_gas_unit_price: Some(Felt::from(max_gas_unit_price).try_into_()?), + max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), + max_gas_unit_price: NonZeroU128::try_from_(Felt::from( + max_gas_unit_price, + )) + .ok(), } } (Some(max_fee), None, None) => { @@ -128,10 +129,10 @@ impl FeeArgs { .l1_gas_price() .price_in_fri; let max_gas = NonZeroFelt::try_from(Felt::from(max_fee) - .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?)).expect("Calculated max-gas from provided --max-fee and the current network gas price is zero. Please increase --max-fee to obtain a positive gas amount"); + .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?)).context("Calculated max-gas from provided --max-fee and the current network gas price is zero. Please increase --max-fee to obtain a positive gas amount")?; FeeSettings::Strk { - max_gas: Some(Felt::from(max_gas).try_into_()?), - max_gas_unit_price: Some(max_gas_unit_price.try_into_()?), + max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), + max_gas_unit_price: NonZeroU128::try_from_(max_gas_unit_price).ok(), } } }; @@ -154,23 +155,23 @@ pub enum FeeToken { #[derive(Debug, PartialEq, CairoDeserialize)] pub enum ScriptFeeSettings { Eth { - max_fee: Option, + max_fee: Option, }, Strk { - max_fee: Option, - max_gas: Option, - max_gas_unit_price: Option, + max_fee: Option, + max_gas: Option, + max_gas_unit_price: Option, }, } #[derive(Debug, PartialEq)] pub enum FeeSettings { Eth { - max_fee: Option, + max_fee: Option, }, Strk { - max_gas: Option, - max_gas_unit_price: Option, + max_gas: Option, + max_gas_unit_price: Option, }, } diff --git a/crates/sncast/src/starknet_commands/account/deploy.rs b/crates/sncast/src/starknet_commands/account/deploy.rs index 335c19194e..e8c8bfed96 100644 --- a/crates/sncast/src/starknet_commands/account/deploy.rs +++ b/crates/sncast/src/starknet_commands/account/deploy.rs @@ -263,7 +263,11 @@ where let result = match fee_settings { FeeSettings::Eth { max_fee } => { let deployment = account_factory.deploy_v1(salt); - let deployment = apply_optional(deployment, max_fee, AccountDeploymentV1::max_fee); + let deployment = apply_optional( + deployment, + max_fee.map(Felt::from), + AccountDeploymentV1::max_fee, + ); deployment.send().await } FeeSettings::Strk { @@ -271,10 +275,14 @@ where max_gas_unit_price, } => { let deployment = account_factory.deploy_v3(salt); - let deployment = apply_optional(deployment, max_gas, AccountDeploymentV3::gas); let deployment = apply_optional( deployment, - max_gas_unit_price, + max_gas.map(std::num::NonZero::get), + AccountDeploymentV3::gas, + ); + let deployment = apply_optional( + deployment, + max_gas_unit_price.map(std::num::NonZero::get), AccountDeploymentV3::gas_price, ); deployment.send().await diff --git a/crates/sncast/src/starknet_commands/declare.rs b/crates/sncast/src/starknet_commands/declare.rs index d25a779d37..1fe283adfb 100644 --- a/crates/sncast/src/starknet_commands/declare.rs +++ b/crates/sncast/src/starknet_commands/declare.rs @@ -105,7 +105,8 @@ pub async fn declare( casm_class_hash, ); - let declaration = apply_optional(declaration, max_fee, DeclarationV2::max_fee); + let declaration = + apply_optional(declaration, max_fee.map(Felt::from), DeclarationV2::max_fee); let declaration = apply_optional(declaration, declare.nonce, DeclarationV2::nonce); declaration.send().await @@ -119,9 +120,16 @@ pub async fn declare( casm_class_hash, ); - let declaration = apply_optional(declaration, max_gas, DeclarationV3::gas); - let declaration = - apply_optional(declaration, max_gas_unit_price, DeclarationV3::gas_price); + let declaration = apply_optional( + declaration, + max_gas.map(std::num::NonZero::get), + DeclarationV3::gas, + ); + let declaration = apply_optional( + declaration, + max_gas_unit_price.map(std::num::NonZero::get), + DeclarationV3::gas_price, + ); let declaration = apply_optional(declaration, declare.nonce, DeclarationV3::nonce); declaration.send().await diff --git a/crates/sncast/src/starknet_commands/deploy.rs b/crates/sncast/src/starknet_commands/deploy.rs index 8b3bcb4e29..fb713c6259 100644 --- a/crates/sncast/src/starknet_commands/deploy.rs +++ b/crates/sncast/src/starknet_commands/deploy.rs @@ -91,7 +91,7 @@ pub async fn deploy( let execution = factory.deploy_v1(calldata.clone(), salt, unique); let execution = match max_fee { None => execution, - Some(max_fee) => execution.max_fee(max_fee), + Some(max_fee) => execution.max_fee(max_fee.into()), }; let execution = match nonce { None => execution, @@ -107,11 +107,11 @@ pub async fn deploy( let execution = match max_gas { None => execution, - Some(max_gas) => execution.gas(max_gas), + Some(max_gas) => execution.gas(max_gas.into()), }; let execution = match max_gas_unit_price { None => execution, - Some(max_gas_unit_price) => execution.gas_price(max_gas_unit_price), + Some(max_gas_unit_price) => execution.gas_price(max_gas_unit_price.into()), }; let execution = match nonce { None => execution, diff --git a/crates/sncast/src/starknet_commands/invoke.rs b/crates/sncast/src/starknet_commands/invoke.rs index 8d481d6cf8..d469cfd2d6 100644 --- a/crates/sncast/src/starknet_commands/invoke.rs +++ b/crates/sncast/src/starknet_commands/invoke.rs @@ -89,7 +89,11 @@ pub async fn execute_calls( FeeSettings::Eth { max_fee } => { let execution_calls = account.execute_v1(calls); - let execution = apply_optional(execution_calls, max_fee, ExecutionV1::max_fee); + let execution = apply_optional( + execution_calls, + max_fee.map(Felt::from), + ExecutionV1::max_fee, + ); let execution = apply_optional(execution, nonce, ExecutionV1::nonce); execution.send().await } @@ -99,8 +103,16 @@ pub async fn execute_calls( } => { let execution_calls = account.execute_v3(calls); - let execution = apply_optional(execution_calls, max_gas, ExecutionV3::gas); - let execution = apply_optional(execution, max_gas_unit_price, ExecutionV3::gas_price); + let execution = apply_optional( + execution_calls, + max_gas.map(std::num::NonZero::get), + ExecutionV3::gas, + ); + let execution = apply_optional( + execution, + max_gas_unit_price.map(std::num::NonZero::get), + ExecutionV3::gas_price, + ); let execution = apply_optional(execution, nonce, ExecutionV3::nonce); execution.send().await } diff --git a/crates/sncast/tests/integration/fee.rs b/crates/sncast/tests/integration/fee.rs index f84ebecb2c..362b2867df 100644 --- a/crates/sncast/tests/integration/fee.rs +++ b/crates/sncast/tests/integration/fee.rs @@ -1,3 +1,5 @@ +use std::num::{NonZeroU128, NonZeroU64}; + use crate::helpers::constants::URL; use sncast::helpers::constants::OZ_CLASS_HASH; use sncast::helpers::fee::{FeeArgs, FeeSettings, FeeToken}; @@ -39,7 +41,7 @@ async fn test_happy_case_eth() { assert_eq!( settings, FeeSettings::Eth { - max_fee: Some(100_u32.into()) + max_fee: Some(Felt::from(100_u32).try_into().unwrap()) } ); } @@ -169,10 +171,9 @@ async fn test_strk_fee_get_max_fee() { max_gas, max_gas_unit_price, } => { - assert_eq!( - u128::from(max_gas.unwrap()) * max_gas_unit_price.unwrap(), - MAX_FEE.into() - ); + let max_gas: u64 = max_gas.unwrap().into(); + let max_gas_unit_price: u128 = max_gas_unit_price.unwrap().into(); + assert_eq!(u128::from(max_gas) * max_gas_unit_price, MAX_FEE.into()); } FeeSettings::Eth { .. } => unreachable!(), } @@ -197,8 +198,8 @@ async fn test_strk_fee_get_max_fee_with_max_gas() { assert_eq!( settings, FeeSettings::Strk { - max_gas: Some(1_000_000), - max_gas_unit_price: Some(u128::from(MAX_FEE / 1_000_000)), + max_gas: Some(NonZeroU64::new(1_000_000).unwrap()), + max_gas_unit_price: Some(NonZeroU128::new((MAX_FEE / 1_000_000).into()).unwrap()), } ); @@ -207,10 +208,9 @@ async fn test_strk_fee_get_max_fee_with_max_gas() { max_gas, max_gas_unit_price, } => { - assert_eq!( - u128::from(max_gas.unwrap()) * max_gas_unit_price.unwrap(), - MAX_FEE.into() - ); + let max_gas: u64 = max_gas.unwrap().into(); + let max_gas_unit_price: u128 = max_gas_unit_price.unwrap().into(); + assert_eq!(u128::from(max_gas) * max_gas_unit_price, MAX_FEE.into()); } FeeSettings::Eth { .. } => unreachable!(), } @@ -235,8 +235,8 @@ async fn test_strk_fee_get_max_gas_and_max_gas_unit_price() { assert_eq!( settings, FeeSettings::Strk { - max_gas: Some(1_000_000), - max_gas_unit_price: Some(1_000), + max_gas: Some(NonZeroU64::new(1_000_000).unwrap()), + max_gas_unit_price: Some(NonZeroU128::new(1_000).unwrap()), } ); } @@ -260,8 +260,8 @@ async fn test_strk_fee_get_max_fee_with_max_gas_unit_price() { assert_eq!( settings, FeeSettings::Strk { - max_gas: Some(MAX_FEE / 1_000), - max_gas_unit_price: Some(1_000), + max_gas: Some(NonZeroU64::new(MAX_FEE / 1_000).unwrap()), + max_gas_unit_price: Some(NonZeroU128::new(1_000).unwrap()), } ); @@ -270,10 +270,9 @@ async fn test_strk_fee_get_max_fee_with_max_gas_unit_price() { max_gas, max_gas_unit_price, } => { - assert_eq!( - u128::from(max_gas.unwrap()) * max_gas_unit_price.unwrap(), - MAX_FEE.into() - ); + let max_gas: u64 = max_gas.unwrap().into(); + let max_gas_unit_price: u128 = max_gas_unit_price.unwrap().into(); + assert_eq!(u128::from(max_gas) * max_gas_unit_price, MAX_FEE.into()); } FeeSettings::Eth { .. } => unreachable!(), } From 80916bf90bda9bdb5f1fd051c231f792c6e82510 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Fri, 20 Dec 2024 01:13:42 +0100 Subject: [PATCH 18/36] Fix tests --- crates/sncast/src/helpers/fee.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 208d05aaff..6865358e40 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -104,6 +104,11 @@ impl FeeArgs { }, (Some(max_fee), None, Some(max_gas_unit_price)) => { let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)).context("Calculated max gas from provided --max-fee and --max-gas-unit-price is zero. Please increase --max-fee to obtain a positive gas amount")?; + print_max_fee_conversion_info( + max_fee.into(), + max_gas.into(), + max_gas_unit_price.into(), + ); FeeSettings::Strk { max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), max_gas_unit_price: NonZeroU128::try_from_(Felt::from( @@ -114,6 +119,11 @@ impl FeeArgs { } (Some(max_fee), Some(max_gas), None) => { let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)).context("Calculated max gas unit price from provided --max-fee and --max-gas is zero. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price")?; + print_max_fee_conversion_info( + max_fee.into(), + max_gas.into(), + max_gas_unit_price.into(), + ); FeeSettings::Strk { max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), max_gas_unit_price: NonZeroU128::try_from_(Felt::from( @@ -130,6 +140,11 @@ impl FeeArgs { .price_in_fri; let max_gas = NonZeroFelt::try_from(Felt::from(max_fee) .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?)).context("Calculated max-gas from provided --max-fee and the current network gas price is zero. Please increase --max-fee to obtain a positive gas amount")?; + print_max_fee_conversion_info( + max_fee.into(), + max_gas.into(), + max_gas_unit_price, + ); FeeSettings::Strk { max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), max_gas_unit_price: NonZeroU128::try_from_(max_gas_unit_price).ok(), From 31168e4ab6ce417f2f992fc4d92bb62864401e38 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Fri, 20 Dec 2024 10:58:08 +0100 Subject: [PATCH 19/36] Format --- crates/sncast/tests/integration/fee.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/sncast/tests/integration/fee.rs b/crates/sncast/tests/integration/fee.rs index 362b2867df..c6c30d2ed9 100644 --- a/crates/sncast/tests/integration/fee.rs +++ b/crates/sncast/tests/integration/fee.rs @@ -9,6 +9,7 @@ use starknet::providers::{JsonRpcClient, Provider}; use starknet::signers::{LocalWallet, SigningKey}; use starknet_types_core::felt::Felt; use url::Url; + const MAX_FEE: u64 = 1_000_000_000_000; async fn get_factory() -> OpenZeppelinAccountFactory> { From 7b7337e9dffa9677ad1468e2fb5e0689e02c81de Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Fri, 20 Dec 2024 12:52:51 +0100 Subject: [PATCH 20/36] Rename `zero` to `0` --- crates/sncast/src/helpers/fee.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 328ad4550e..f6e4689806 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -103,7 +103,7 @@ impl FeeArgs { .and_then(|val| NonZeroU128::try_from_(Felt::from(val)).ok()), }, (Some(max_fee), None, Some(max_gas_unit_price)) => { - let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)).context("Calculated max gas from provided --max-fee and --max-gas-unit-price is zero. Please increase --max-fee to obtain a positive gas amount")?; + let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)).context("Calculated max gas from provided --max-fee and --max-gas-unit-price is 0. Please increase --max-fee to obtain a positive gas amount")?; FeeSettings::Strk { max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), max_gas_unit_price: NonZeroU128::try_from_(Felt::from( @@ -113,7 +113,7 @@ impl FeeArgs { } } (Some(max_fee), Some(max_gas), None) => { - let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)).context("Calculated max gas unit price from provided --max-fee and --max-gas is zero. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price")?; + let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)).context("Calculated max gas unit price from provided --max-fee and --max-gas is 0. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price")?; FeeSettings::Strk { max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), max_gas_unit_price: NonZeroU128::try_from_(Felt::from( @@ -129,7 +129,7 @@ impl FeeArgs { .l1_gas_price() .price_in_fri; let max_gas = NonZeroFelt::try_from(Felt::from(max_fee) - .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?)).context("Calculated max-gas from provided --max-fee and the current network gas price is zero. Please increase --max-fee to obtain a positive gas amount")?; + .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?)).context("Calculated max-gas from provided --max-fee and the current network gas price is 0. Please increase --max-fee to obtain a positive gas amount")?; FeeSettings::Strk { max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), max_gas_unit_price: NonZeroU128::try_from_(max_gas_unit_price).ok(), From 2db18e62da2b004f05c7e983afd3459aeb22a839 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Fri, 20 Dec 2024 13:17:40 +0100 Subject: [PATCH 21/36] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c446758d19..715bb661ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Cast +### Added + +- When using `--max-fee` with transactions v3, calculated max gas and max gas unit price are automatically validated to ensure they are greater than 0 after conversion + ### Changed - Values passed to the `--max-fee`, `--max-gas`, and `--max-gas-unit-price` flags must be greater than 0 From 41e4cd5414839b019ab430158c04ef7612b178e6 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Fri, 20 Dec 2024 13:18:33 +0100 Subject: [PATCH 22/36] Add `impl_deserialize_for_nonzero_num_type` macro --- .../src/serde/deserialize/deserialize_impl.rs | 33 ++++++++----------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/crates/conversions/src/serde/deserialize/deserialize_impl.rs b/crates/conversions/src/serde/deserialize/deserialize_impl.rs index 2f39246b1b..081a4220af 100644 --- a/crates/conversions/src/serde/deserialize/deserialize_impl.rs +++ b/crates/conversions/src/serde/deserialize/deserialize_impl.rs @@ -4,7 +4,7 @@ use num_traits::cast::ToPrimitive; use starknet::providers::Url; use starknet_api::core::{ClassHash, ContractAddress, EntryPointSelector, Nonce}; use starknet_types_core::felt::{Felt, NonZeroFelt}; -use std::num::{NonZeroU128, NonZeroU32, NonZeroU64}; +use std::num::NonZero; impl CairoDeserialize for Url { fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { @@ -13,12 +13,6 @@ impl CairoDeserialize for Url { } } -impl CairoDeserialize for NonZeroU32 { - fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { - NonZeroU32::new(reader.read()?).ok_or(BufferReadError::ParseFailed) - } -} - impl CairoDeserialize for Felt { fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { reader.read_felt() @@ -78,18 +72,15 @@ impl CairoDeserialize for NonZeroFelt { } } -impl CairoDeserialize for NonZeroU64 { - fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { - let val: u64 = reader.read()?; - NonZeroU64::try_from(val).map_err(|_| BufferReadError::ParseFailed) - } -} - -impl CairoDeserialize for NonZeroU128 { - fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { - let val: u128 = reader.read()?; - NonZeroU128::try_from(val).map_err(|_| BufferReadError::ParseFailed) - } +macro_rules! impl_deserialize_for_nonzero_num_type { + ($type:ty) => { + impl CairoDeserialize for NonZero<$type> { + fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { + let val = <$type>::deserialize(reader)?; + NonZero::new(val).ok_or(BufferReadError::ParseFailed) + } + } + }; } macro_rules! impl_deserialize_for_felt_type { @@ -120,6 +111,10 @@ impl_deserialize_for_felt_type!(ContractAddress); impl_deserialize_for_felt_type!(Nonce); impl_deserialize_for_felt_type!(EntryPointSelector); +impl_deserialize_for_nonzero_num_type!(u32); +impl_deserialize_for_nonzero_num_type!(u64); +impl_deserialize_for_nonzero_num_type!(u128); + impl_deserialize_for_num_type!(u8); impl_deserialize_for_num_type!(u16); impl_deserialize_for_num_type!(u32); From 73e085f47ad53064cc780c6d28d88c22f648bb42 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Fri, 20 Dec 2024 13:45:09 +0100 Subject: [PATCH 23/36] Apply other code review suggestions --- crates/conversions/src/felt.rs | 54 +++++++++++++++------------- crates/sncast/src/helpers/fee.rs | 60 ++++++++++++++++++++------------ 2 files changed, 67 insertions(+), 47 deletions(-) diff --git a/crates/conversions/src/felt.rs b/crates/conversions/src/felt.rs index f3122aa456..43cb8126b9 100644 --- a/crates/conversions/src/felt.rs +++ b/crates/conversions/src/felt.rs @@ -6,9 +6,9 @@ use crate::{ }; use conversions::padded_felt::PaddedFelt; use starknet_api::core::{ClassHash, ContractAddress, EntryPointSelector, Nonce}; -use starknet_types_core::felt::{Felt, FromStrError}; +use starknet_types_core::felt::{Felt, FromStrError, NonZeroFelt}; use std::{ - num::{NonZeroU128, NonZeroU64}, + num::{NonZero, NonZeroU128, NonZeroU64}, vec, }; @@ -42,39 +42,45 @@ impl FromConv for Felt { } } -impl TryFromConv for NonZeroU64 { +impl TryFromConv for NonZeroU64 { type Error = String; - fn try_from_(value: Felt) -> Result { - if value == Felt::ZERO { - Err("value should be greater than 0".to_string()) - } else { - let value: u64 = value.try_into().expect("failed to convert Felt to u64"); - Ok(NonZeroU64::new(value).unwrap()) - } + fn try_from_(value: NonZeroFelt) -> Result { + let value: u64 = Felt::from(value) + .try_into() + .map_err(|_| "felt was too large to fit in u64")?; + Ok(NonZero::new(value) + .unwrap_or_else(|| unreachable!("non zero felt is always greater than 0"))) } } -impl TryFromConv for NonZeroU128 { +impl TryFromConv for NonZeroU128 { type Error = String; - fn try_from_(value: Felt) -> Result { - if value == Felt::ZERO { - Err("value should be greater than 0".to_string()) - } else { - let value: u128 = value.try_into().expect("failed to convert Felt to u128"); - Ok(NonZeroU128::new(value).unwrap()) - } + fn try_from_(value: NonZeroFelt) -> Result { + let value: u128 = Felt::from(value) + .try_into() + .map_err(|_| "felt was too large to fit in u128")?; + Ok(NonZero::new(value) + .unwrap_or_else(|| unreachable!("non zero felt is always greater than 0"))) } } -impl FromConv for Felt { - fn from_(value: NonZeroU64) -> Felt { - Felt::from(value.get()) +impl FromConv for NonZeroFelt { + fn from_(value: NonZeroU64) -> Self { + NonZeroFelt::try_from(Felt::from(value.get())).unwrap_or_else(|_| { + unreachable!( + "NonZeroU64 is always greater than 0, so it should be convertible to NonZeroFelt" + ) + }) } } -impl FromConv for Felt { - fn from_(value: NonZeroU128) -> Felt { - Felt::from(value.get()) +impl FromConv for NonZeroFelt { + fn from_(value: NonZeroU128) -> Self { + NonZeroFelt::try_from(Felt::from(value.get())).unwrap_or_else(|_| { + unreachable!( + "NonZeroU128 is always greater than 0, so it should be convertible to NonZeroFelt" + ) + }) } } diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index f6e4689806..c6b1f02e8b 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -45,9 +45,8 @@ impl From for FeeArgs { } => Self { fee_token: Some(FeeToken::Strk), max_fee, - max_gas: max_gas.and_then(|val| NonZeroFelt::try_from(Felt::from_(val)).ok()), - max_gas_unit_price: max_gas_unit_price - .and_then(|val| NonZeroFelt::try_from(Felt::from_(val)).ok()), + max_gas: max_gas.map(NonZeroFelt::from_), + max_gas_unit_price: max_gas_unit_price.map(NonZeroFelt::from_), }, } } @@ -97,42 +96,57 @@ impl FeeArgs { (None, _, _) => FeeSettings::Strk { max_gas: self .max_gas - .and_then(|val| NonZeroU64::try_from_(Felt::from(val)).ok()), + .map(NonZeroU64::try_from_) + .transpose() + .map_err(anyhow::Error::msg)?, max_gas_unit_price: self .max_gas_unit_price - .and_then(|val| NonZeroU128::try_from_(Felt::from(val)).ok()), + .map(NonZeroU128::try_from_) + .transpose() + .map_err(anyhow::Error::msg)?, }, (Some(max_fee), None, Some(max_gas_unit_price)) => { let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)).context("Calculated max gas from provided --max-fee and --max-gas-unit-price is 0. Please increase --max-fee to obtain a positive gas amount")?; FeeSettings::Strk { - max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), - max_gas_unit_price: NonZeroU128::try_from_(Felt::from( - max_gas_unit_price, - )) - .ok(), + max_gas: Some( + NonZeroU64::try_from_(max_gas).map_err(anyhow::Error::msg)?, + ), + max_gas_unit_price: Some( + NonZeroU128::try_from_(max_gas_unit_price) + .map_err(anyhow::Error::msg)?, + ), } } (Some(max_fee), Some(max_gas), None) => { let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)).context("Calculated max gas unit price from provided --max-fee and --max-gas is 0. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price")?; FeeSettings::Strk { - max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), - max_gas_unit_price: NonZeroU128::try_from_(Felt::from( - max_gas_unit_price, - )) - .ok(), + max_gas: Some( + NonZeroU64::try_from_(max_gas).map_err(anyhow::Error::msg)?, + ), + max_gas_unit_price: Some( + NonZeroU128::try_from_(max_gas_unit_price) + .map_err(anyhow::Error::msg)?, + ), } } (Some(max_fee), None, None) => { - let max_gas_unit_price = provider - .get_block_with_tx_hashes(block_id) - .await? - .l1_gas_price() - .price_in_fri; + let max_gas_unit_price = NonZeroFelt::try_from( + provider + .get_block_with_tx_hashes(block_id) + .await? + .l1_gas_price() + .price_in_fri, + )?; let max_gas = NonZeroFelt::try_from(Felt::from(max_fee) - .floor_div(&NonZeroFelt::try_from(max_gas_unit_price)?)).context("Calculated max-gas from provided --max-fee and the current network gas price is 0. Please increase --max-fee to obtain a positive gas amount")?; + .floor_div(&max_gas_unit_price)).context("Calculated max-gas from provided --max-fee and the current network gas price is 0. Please increase --max-fee to obtain a positive gas amount")?; FeeSettings::Strk { - max_gas: NonZeroU64::try_from_(Felt::from(max_gas)).ok(), - max_gas_unit_price: NonZeroU128::try_from_(max_gas_unit_price).ok(), + max_gas: Some( + NonZeroU64::try_from_(max_gas).map_err(anyhow::Error::msg)?, + ), + max_gas_unit_price: Some( + NonZeroU128::try_from_(max_gas_unit_price) + .map_err(anyhow::Error::msg)?, + ), } } }; From 69ed7c8f1a28bf80ff2e958bf04a41f9c17b8c80 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Mon, 6 Jan 2025 15:37:09 +0100 Subject: [PATCH 24/36] Fix linting --- crates/sncast/src/helpers/fee.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 6e93737093..adb9e916c2 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -61,6 +61,7 @@ impl FeeArgs { } } + #[allow(clippy::too_many_lines)] pub async fn try_into_fee_settings( &self, provider: P, From 7d4bbedaf0b75818e758b1843ef5ba43896f8b00 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Tue, 7 Jan 2025 09:47:43 +0100 Subject: [PATCH 25/36] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 715bb661ab..8e17b7cc95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Values passed to the `--max-fee`, `--max-gas`, and `--max-gas-unit-price` flags must be greater than 0 +- In case of implicit conversion of fee settings, values are verified to be greater than 0 ## [0.35.1] - 2024-12-16 From 0779442b3c49f6750ea638003d4311810d8bdeec Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Tue, 7 Jan 2025 10:55:29 +0100 Subject: [PATCH 26/36] Add conversions tests --- crates/conversions/src/felt.rs | 51 ++----------------- crates/conversions/src/lib.rs | 3 ++ crates/conversions/src/non_zero_felt.rs | 23 +++++++++ crates/conversions/src/non_zero_u128.rs | 14 +++++ crates/conversions/src/non_zero_u64.rs | 14 +++++ crates/conversions/tests/e2e/mod.rs | 3 ++ crates/conversions/tests/e2e/non_zero_felt.rs | 21 ++++++++ crates/conversions/tests/e2e/non_zero_u128.rs | 36 +++++++++++++ crates/conversions/tests/e2e/non_zero_u64.rs | 36 +++++++++++++ 9 files changed, 153 insertions(+), 48 deletions(-) create mode 100644 crates/conversions/src/non_zero_felt.rs create mode 100644 crates/conversions/src/non_zero_u128.rs create mode 100644 crates/conversions/src/non_zero_u64.rs create mode 100644 crates/conversions/tests/e2e/non_zero_felt.rs create mode 100644 crates/conversions/tests/e2e/non_zero_u128.rs create mode 100644 crates/conversions/tests/e2e/non_zero_u64.rs diff --git a/crates/conversions/src/felt.rs b/crates/conversions/src/felt.rs index 43cb8126b9..12aebdf173 100644 --- a/crates/conversions/src/felt.rs +++ b/crates/conversions/src/felt.rs @@ -2,15 +2,12 @@ use crate::{ byte_array::ByteArray, serde::serialize::SerializeToFeltVec, string::{TryFromDecStr, TryFromHexStr}, - FromConv, IntoConv, TryFromConv, + FromConv, IntoConv, }; use conversions::padded_felt::PaddedFelt; use starknet_api::core::{ClassHash, ContractAddress, EntryPointSelector, Nonce}; -use starknet_types_core::felt::{Felt, FromStrError, NonZeroFelt}; -use std::{ - num::{NonZero, NonZeroU128, NonZeroU64}, - vec, -}; +use starknet_types_core::felt::{Felt, FromStrError}; +use std::vec; impl FromConv for Felt { fn from_(value: ClassHash) -> Felt { @@ -42,48 +39,6 @@ impl FromConv for Felt { } } -impl TryFromConv for NonZeroU64 { - type Error = String; - fn try_from_(value: NonZeroFelt) -> Result { - let value: u64 = Felt::from(value) - .try_into() - .map_err(|_| "felt was too large to fit in u64")?; - Ok(NonZero::new(value) - .unwrap_or_else(|| unreachable!("non zero felt is always greater than 0"))) - } -} - -impl TryFromConv for NonZeroU128 { - type Error = String; - fn try_from_(value: NonZeroFelt) -> Result { - let value: u128 = Felt::from(value) - .try_into() - .map_err(|_| "felt was too large to fit in u128")?; - Ok(NonZero::new(value) - .unwrap_or_else(|| unreachable!("non zero felt is always greater than 0"))) - } -} - -impl FromConv for NonZeroFelt { - fn from_(value: NonZeroU64) -> Self { - NonZeroFelt::try_from(Felt::from(value.get())).unwrap_or_else(|_| { - unreachable!( - "NonZeroU64 is always greater than 0, so it should be convertible to NonZeroFelt" - ) - }) - } -} - -impl FromConv for NonZeroFelt { - fn from_(value: NonZeroU128) -> Self { - NonZeroFelt::try_from(Felt::from(value.get())).unwrap_or_else(|_| { - unreachable!( - "NonZeroU128 is always greater than 0, so it should be convertible to NonZeroFelt" - ) - }) - } -} - impl TryFromDecStr for T where T: FromConv, diff --git a/crates/conversions/src/lib.rs b/crates/conversions/src/lib.rs index 71af471e1d..2c625bcf3a 100644 --- a/crates/conversions/src/lib.rs +++ b/crates/conversions/src/lib.rs @@ -6,6 +6,9 @@ pub mod contract_address; pub mod entrypoint_selector; pub mod eth_address; pub mod felt; +pub mod non_zero_felt; +pub mod non_zero_u128; +pub mod non_zero_u64; pub mod nonce; pub mod padded_felt; pub mod primitive; diff --git a/crates/conversions/src/non_zero_felt.rs b/crates/conversions/src/non_zero_felt.rs new file mode 100644 index 0000000000..268c48b209 --- /dev/null +++ b/crates/conversions/src/non_zero_felt.rs @@ -0,0 +1,23 @@ +use crate::FromConv; +use starknet_types_core::felt::{Felt, NonZeroFelt}; +use std::num::{NonZeroU128, NonZeroU64}; + +impl FromConv for NonZeroFelt { + fn from_(value: NonZeroU64) -> Self { + NonZeroFelt::try_from(Felt::from(value.get())).unwrap_or_else(|_| { + unreachable!( + "NonZeroU64 is always greater than 0, so it should be convertible to NonZeroFelt" + ) + }) + } +} + +impl FromConv for NonZeroFelt { + fn from_(value: NonZeroU128) -> Self { + NonZeroFelt::try_from(Felt::from(value.get())).unwrap_or_else(|_| { + unreachable!( + "NonZeroU128 is always greater than 0, so it should be convertible to NonZeroFelt" + ) + }) + } +} diff --git a/crates/conversions/src/non_zero_u128.rs b/crates/conversions/src/non_zero_u128.rs new file mode 100644 index 0000000000..2e65c1d7ab --- /dev/null +++ b/crates/conversions/src/non_zero_u128.rs @@ -0,0 +1,14 @@ +use crate::TryFromConv; +use starknet_types_core::felt::{Felt, NonZeroFelt}; +use std::num::{NonZero, NonZeroU128}; + +impl TryFromConv for NonZeroU128 { + type Error = String; + fn try_from_(value: NonZeroFelt) -> Result { + let value: u128 = Felt::from(value) + .try_into() + .map_err(|_| "felt was too large to fit in u128")?; + Ok(NonZero::new(value) + .unwrap_or_else(|| unreachable!("non zero felt is always greater than 0"))) + } +} diff --git a/crates/conversions/src/non_zero_u64.rs b/crates/conversions/src/non_zero_u64.rs new file mode 100644 index 0000000000..6459cb089d --- /dev/null +++ b/crates/conversions/src/non_zero_u64.rs @@ -0,0 +1,14 @@ +use crate::TryFromConv; +use starknet_types_core::felt::{Felt, NonZeroFelt}; +use std::num::{NonZero, NonZeroU64}; + +impl TryFromConv for NonZeroU64 { + type Error = String; + fn try_from_(value: NonZeroFelt) -> Result { + let value: u64 = Felt::from(value) + .try_into() + .map_err(|_| "felt was too large to fit in u64")?; + Ok(NonZero::new(value) + .unwrap_or_else(|| unreachable!("non zero felt is always greater than 0"))) + } +} diff --git a/crates/conversions/tests/e2e/mod.rs b/crates/conversions/tests/e2e/mod.rs index a030ba2019..98a2224299 100644 --- a/crates/conversions/tests/e2e/mod.rs +++ b/crates/conversions/tests/e2e/mod.rs @@ -3,6 +3,9 @@ mod contract_address; mod entrypoint_selector; mod felt; mod field_elements; +mod non_zero_felt; +mod non_zero_u128; +mod non_zero_u64; mod nonce; mod padded_felt; mod string; diff --git a/crates/conversions/tests/e2e/non_zero_felt.rs b/crates/conversions/tests/e2e/non_zero_felt.rs new file mode 100644 index 0000000000..758bc86fb7 --- /dev/null +++ b/crates/conversions/tests/e2e/non_zero_felt.rs @@ -0,0 +1,21 @@ +#[cfg(test)] +mod tests_non_zero_felt { + use std::num::{NonZeroU128, NonZeroU64}; + + use conversions::FromConv; + use starknet_types_core::felt::{Felt, NonZeroFelt}; + + #[test] + fn test_happy_case() { + let non_zero_felt = NonZeroFelt::try_from(Felt::from(1_u8)).unwrap(); + + assert_eq!( + non_zero_felt, + NonZeroFelt::from_(NonZeroU64::new(1).unwrap()) + ); + assert_eq!( + non_zero_felt, + NonZeroFelt::from_(NonZeroU128::new(1).unwrap()) + ); + } +} diff --git a/crates/conversions/tests/e2e/non_zero_u128.rs b/crates/conversions/tests/e2e/non_zero_u128.rs new file mode 100644 index 0000000000..af1a336c34 --- /dev/null +++ b/crates/conversions/tests/e2e/non_zero_u128.rs @@ -0,0 +1,36 @@ +#[cfg(test)] +mod tests_non_zero_u128 { + use conversions::TryFromConv; + use starknet_types_core::felt::{Felt, NonZeroFelt}; + use std::num::NonZeroU128; + + #[test] + fn test_happy_case() { + let non_zero_u128 = NonZeroU128::new(1).unwrap(); + + assert_eq!( + non_zero_u128, + NonZeroU128::try_from_(NonZeroFelt::try_from(Felt::from(1_u8)).unwrap()).unwrap() + ); + } + + #[test] + fn test_limit() { + let felt = Felt::from_dec_str(&u128::MAX.to_string()).unwrap(); + let non_zero_felt = NonZeroFelt::try_from(felt).unwrap(); + + let result = NonZeroU128::try_from_(non_zero_felt); + assert!(result.is_ok()); + assert_eq!(result.unwrap().get(), u128::MAX); + } + + #[test] + fn test_felt_too_large() { + let large_felt = Felt::from_dec_str("340282366920938463463374607431768211456").unwrap(); // 2^128 + let non_zero_felt = NonZeroFelt::try_from(large_felt).unwrap(); + + let result = NonZeroU128::try_from_(non_zero_felt); + assert!(result.is_err()); + assert_eq!(result.unwrap_err(), "felt was too large to fit in u128"); + } +} diff --git a/crates/conversions/tests/e2e/non_zero_u64.rs b/crates/conversions/tests/e2e/non_zero_u64.rs new file mode 100644 index 0000000000..77177e5ffa --- /dev/null +++ b/crates/conversions/tests/e2e/non_zero_u64.rs @@ -0,0 +1,36 @@ +#[cfg(test)] +mod tests_non_zero_u64 { + use conversions::TryFromConv; + use starknet_types_core::felt::{Felt, NonZeroFelt}; + use std::num::NonZeroU64; + + #[test] + fn test_happy_case() { + let non_zero_u64 = NonZeroU64::new(1).unwrap(); + + assert_eq!( + non_zero_u64, + NonZeroU64::try_from_(NonZeroFelt::try_from(Felt::from(1_u8)).unwrap()).unwrap() + ); + } + + #[test] + fn test_limit() { + let felt = Felt::from_dec_str(&u64::MAX.to_string()).unwrap(); + let non_zero_felt = NonZeroFelt::try_from(felt).unwrap(); + + let result = NonZeroU64::try_from_(non_zero_felt); + assert!(result.is_ok()); + assert_eq!(result.unwrap().get(), u64::MAX); + } + + #[test] + fn test_felt_too_large() { + let large_felt = Felt::from_dec_str("18446744073709551616").unwrap(); // 2^64 + let non_zero_felt = NonZeroFelt::try_from(large_felt).unwrap(); + + let result = NonZeroU64::try_from_(non_zero_felt); + assert!(result.is_err()); + assert_eq!(result.unwrap_err(), "felt was too large to fit in u64"); + } +} From 0c80350123c32c0bd18656d0bb6aceabb243641e Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Tue, 7 Jan 2025 11:02:23 +0100 Subject: [PATCH 27/36] Update changelog - remove duplicated entry --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7905cdd94..eb7ff73c8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Values passed to the `--max-fee`, `--max-gas`, and `--max-gas-unit-price` flags must be greater than 0 -- In case of implicit conversion of fee settings, values are verified to be greater than 0 ## [0.35.1] - 2024-12-16 From 0e185b6897c7ba52a2a63254bf72276b53ce436f Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Tue, 7 Jan 2025 11:04:02 +0100 Subject: [PATCH 28/36] Fix headings in changelog --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb7ff73c8c..0a0d235cb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,12 +9,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Cast -### Added +#### Added - When using `--max-fee` with transactions v3, calculated max gas and max gas unit price are automatically validated to ensure they are greater than 0 after conversion - interactive interface that allows setting created or imported account as the default -### Changed +#### Changed - Values passed to the `--max-fee`, `--max-gas`, and `--max-gas-unit-price` flags must be greater than 0 From a784e6987ff3449cc6bff8a10c1dacdf398a20ad Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Tue, 7 Jan 2025 11:52:54 +0100 Subject: [PATCH 29/36] Refactor `FeeArgs.try_into_fee_settings()` --- crates/sncast/src/helpers/fee.rs | 24 ++++++++++++++---------- crates/sncast/tests/e2e/invoke.rs | 9 +++++---- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index c6b1f02e8b..aadc3a088f 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -85,14 +85,6 @@ impl FeeArgs { (Some(_), Some(_), Some(_)) => { bail!("Passing all --max-fee, --max-gas and --max-gas-unit-price is conflicting. Please pass only two of them or less") } - (Some(max_fee), Some(max_gas), None) if max_fee < max_gas => { - bail!("--max-fee should be greater than or equal to --max-gas amount") - } - (Some(max_fee), None, Some(max_gas_unit_price)) - if max_fee < max_gas_unit_price => - { - bail!("--max-fee should be greater than or equal to --max-gas-unit-price") - } (None, _, _) => FeeSettings::Strk { max_gas: self .max_gas @@ -106,7 +98,14 @@ impl FeeArgs { .map_err(anyhow::Error::msg)?, }, (Some(max_fee), None, Some(max_gas_unit_price)) => { - let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)).context("Calculated max gas from provided --max-fee and --max-gas-unit-price is 0. Please increase --max-fee to obtain a positive gas amount")?; + if max_fee < max_gas_unit_price { + bail!( + "--max-fee should be greater than or equal to --max-gas-unit-price" + ); + } + + let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)) + .unwrap_or_else(|_| unreachable!("Calculated max gas cannot be 0 because max_fee >= max_gas_unit_price ensures a positive result")); FeeSettings::Strk { max_gas: Some( NonZeroU64::try_from_(max_gas).map_err(anyhow::Error::msg)?, @@ -118,7 +117,12 @@ impl FeeArgs { } } (Some(max_fee), Some(max_gas), None) => { - let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)).context("Calculated max gas unit price from provided --max-fee and --max-gas is 0. Please increase --max-fee or decrease --max-gas to ensure a positive gas unit price")?; + if max_fee < max_gas { + bail!("--max-fee should be greater than or equal to --max-gas amount"); + } + + let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)) + .unwrap_or_else(|_| unreachable!("Calculated max gas unit price cannot be 0 because max_fee >= max_gas ensures a positive result")); FeeSettings::Strk { max_gas: Some( NonZeroU64::try_from_(max_gas).map_err(anyhow::Error::msg)?, diff --git a/crates/sncast/tests/e2e/invoke.rs b/crates/sncast/tests/e2e/invoke.rs index 1a96bff6bd..a06232c2b2 100644 --- a/crates/sncast/tests/e2e/invoke.rs +++ b/crates/sncast/tests/e2e/invoke.rs @@ -424,7 +424,7 @@ fn test_max_gas_equal_to_zero() { } #[test] -fn test_max_gas_calculated_from_max_fee_equal_to_zero() { +fn test_calculated_max_gas_equal_to_zero_when_max_fee_passed() { let args = vec![ "--accounts-file", ACCOUNT_FILE_PATH, @@ -442,18 +442,19 @@ fn test_max_gas_calculated_from_max_fee_equal_to_zero() { "0x1", "0x2", "--max-fee", - "0", + "999999", "--fee-token", "strk", ]; let snapbox = runner(&args); - let output = snapbox.assert().code(2); + let output = snapbox.assert().success(); assert_stderr_contains( output, indoc! {r" - error: invalid value '0' for '--max-fee ': Value should be greater than 0 + command: invoke + error: Calculated max-gas from provided --max-fee and the current network gas price is 0. Please increase --max-fee to obtain a positive gas amount: Tried to create NonZeroFelt from 0 "}, ); } From 091e4847834ed93b1239ce604cf8739fddee00d9 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Tue, 7 Jan 2025 12:42:08 +0100 Subject: [PATCH 30/36] Update `unreachable!` messages in `FeeArgs.try_into_fee_settings()` --- crates/sncast/src/helpers/fee.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index aadc3a088f..d0eab8e002 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -105,7 +105,7 @@ impl FeeArgs { } let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)) - .unwrap_or_else(|_| unreachable!("Calculated max gas cannot be 0 because max_fee >= max_gas_unit_price ensures a positive result")); + .unwrap_or_else(|_| unreachable!("Calculated max gas must be >= 1 because max_fee >= max_gas_unit_price ensures a positive result")); FeeSettings::Strk { max_gas: Some( NonZeroU64::try_from_(max_gas).map_err(anyhow::Error::msg)?, @@ -122,7 +122,7 @@ impl FeeArgs { } let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)) - .unwrap_or_else(|_| unreachable!("Calculated max gas unit price cannot be 0 because max_fee >= max_gas ensures a positive result")); + .unwrap_or_else(|_| unreachable!("Calculated max gas unit price must be >= 1 because max_fee >= max_gas ensures a positive result")); FeeSettings::Strk { max_gas: Some( NonZeroU64::try_from_(max_gas).map_err(anyhow::Error::msg)?, From b32e7a460ee1e7ff93060de4c46b41f3c224b7ed Mon Sep 17 00:00:00 2001 From: Franciszek Job <54181625+franciszekjob@users.noreply.github.com> Date: Tue, 7 Jan 2025 13:59:15 +0100 Subject: [PATCH 31/36] Update crates/sncast/src/helpers/fee.rs Co-authored-by: Piotr Figiela <77412592+Draggu@users.noreply.github.com> --- crates/sncast/src/helpers/fee.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index adb9e916c2..30b2dba828 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -295,7 +295,7 @@ fn parse_fee_token(s: &str) -> Result { Ok(parsed_token) } -fn print_max_fee_conversion_info(max_fee: Felt, max_gas: Felt, max_gas_unit_price: Felt) { +fn print_max_fee_conversion_info(max_fee: impl Into, max_gas: impl Into, max_gas_unit_price: impl Into) { println!( "Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags\nConverted {max_fee} max fee to {max_gas} max gas and {max_gas_unit_price} max gas unit price\n", ); From f161effeb9b88f839697a6a99565ce826578df86 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Fri, 10 Jan 2025 02:08:49 +0100 Subject: [PATCH 32/36] Fix linting --- crates/sncast/src/helpers/fee.rs | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index 49099d2201..b13c6f0915 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -107,11 +107,7 @@ impl FeeArgs { let max_gas = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas_unit_price)) .unwrap_or_else(|_| unreachable!("Calculated max gas must be >= 1 because max_fee >= max_gas_unit_price ensures a positive result")); - print_max_fee_conversion_info( - max_fee.into(), - max_gas.into(), - max_gas_unit_price.into(), - ); + print_max_fee_conversion_info(max_fee, max_gas, max_gas_unit_price); FeeSettings::Strk { max_gas: Some( NonZeroU64::try_from_(max_gas).map_err(anyhow::Error::msg)?, @@ -129,11 +125,7 @@ impl FeeArgs { let max_gas_unit_price = NonZeroFelt::try_from(Felt::from(max_fee).floor_div(&max_gas)) .unwrap_or_else(|_| unreachable!("Calculated max gas unit price must be >= 1 because max_fee >= max_gas ensures a positive result")); - print_max_fee_conversion_info( - max_fee.into(), - max_gas.into(), - max_gas_unit_price.into(), - ); + print_max_fee_conversion_info(max_fee, max_gas, max_gas_unit_price); FeeSettings::Strk { max_gas: Some( NonZeroU64::try_from_(max_gas).map_err(anyhow::Error::msg)?, @@ -154,11 +146,7 @@ impl FeeArgs { )?; let max_gas = NonZeroFelt::try_from(Felt::from(max_fee) .floor_div(&max_gas_unit_price)).context("Calculated max-gas from provided --max-fee and the current network gas price is 0. Please increase --max-fee to obtain a positive gas amount")?; - print_max_fee_conversion_info( - max_fee.into(), - max_gas.into(), - max_gas_unit_price.into(), - ); + print_max_fee_conversion_info(max_fee, max_gas, max_gas_unit_price); FeeSettings::Strk { max_gas: Some( NonZeroU64::try_from_(max_gas).map_err(anyhow::Error::msg)?, @@ -299,7 +287,14 @@ fn parse_fee_token(s: &str) -> Result { Ok(parsed_token) } -fn print_max_fee_conversion_info(max_fee: impl Into, max_gas: impl Into, max_gas_unit_price: impl Into) { +fn print_max_fee_conversion_info( + max_fee: impl Into, + max_gas: impl Into, + max_gas_unit_price: impl Into, +) { + let max_fee: Felt = max_fee.into(); + let max_gas: Felt = max_gas.into(); + let max_gas_unit_price: Felt = max_gas_unit_price.into(); println!( "Specifying '--max-fee' flag while using v3 transactions results in conversion to '--max-gas' and '--max-gas-unit-price' flags\nConverted {max_fee} max fee to {max_gas} max gas and {max_gas_unit_price} max gas unit price\n", ); From 9682eab8d3744c626ef463c81c07243a76430ee6 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Fri, 10 Jan 2025 15:33:48 +0100 Subject: [PATCH 33/36] Use `Felt::TWO.pow` --- crates/conversions/tests/e2e/non_zero_u128.rs | 2 +- crates/conversions/tests/e2e/non_zero_u64.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/conversions/tests/e2e/non_zero_u128.rs b/crates/conversions/tests/e2e/non_zero_u128.rs index af1a336c34..b79b3f6b67 100644 --- a/crates/conversions/tests/e2e/non_zero_u128.rs +++ b/crates/conversions/tests/e2e/non_zero_u128.rs @@ -26,7 +26,7 @@ mod tests_non_zero_u128 { #[test] fn test_felt_too_large() { - let large_felt = Felt::from_dec_str("340282366920938463463374607431768211456").unwrap(); // 2^128 + let large_felt = Felt::TWO.pow(128_u8); let non_zero_felt = NonZeroFelt::try_from(large_felt).unwrap(); let result = NonZeroU128::try_from_(non_zero_felt); diff --git a/crates/conversions/tests/e2e/non_zero_u64.rs b/crates/conversions/tests/e2e/non_zero_u64.rs index 77177e5ffa..5a9a141dbd 100644 --- a/crates/conversions/tests/e2e/non_zero_u64.rs +++ b/crates/conversions/tests/e2e/non_zero_u64.rs @@ -26,7 +26,7 @@ mod tests_non_zero_u64 { #[test] fn test_felt_too_large() { - let large_felt = Felt::from_dec_str("18446744073709551616").unwrap(); // 2^64 + let large_felt = Felt::TWO.pow(64_u8); let non_zero_felt = NonZeroFelt::try_from(large_felt).unwrap(); let result = NonZeroU64::try_from_(non_zero_felt); From a7c0acf0349997e3e467f20fafe468cdafb3a74e Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Fri, 10 Jan 2025 15:43:52 +0100 Subject: [PATCH 34/36] Remove use of `to_bigint()` in `impl_deserialize_for_num_type` --- .../conversions/src/serde/deserialize/deserialize_impl.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/conversions/src/serde/deserialize/deserialize_impl.rs b/crates/conversions/src/serde/deserialize/deserialize_impl.rs index 081a4220af..668521a305 100644 --- a/crates/conversions/src/serde/deserialize/deserialize_impl.rs +++ b/crates/conversions/src/serde/deserialize/deserialize_impl.rs @@ -92,15 +92,13 @@ macro_rules! impl_deserialize_for_felt_type { } }; } + macro_rules! impl_deserialize_for_num_type { ($type:ty) => { impl CairoDeserialize for $type { fn deserialize(reader: &mut BufferReader<'_>) -> BufferReadResult { let felt = Felt::deserialize(reader)?; - - felt.to_bigint() - .try_into() - .map_err(|_| BufferReadError::ParseFailed) + felt.try_into().map_err(|_| BufferReadError::ParseFailed) } } }; From e20072304f578a007f17cbdd30a99c4f7ad0e3ac Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Tue, 14 Jan 2025 13:30:16 +0100 Subject: [PATCH 35/36] Add todo --- crates/sncast/src/helpers/fee.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/sncast/src/helpers/fee.rs b/crates/sncast/src/helpers/fee.rs index d0eab8e002..55160ffc06 100644 --- a/crates/sncast/src/helpers/fee.rs +++ b/crates/sncast/src/helpers/fee.rs @@ -141,6 +141,7 @@ impl FeeArgs { .l1_gas_price() .price_in_fri, )?; + // TODO(#2852) let max_gas = NonZeroFelt::try_from(Felt::from(max_fee) .floor_div(&max_gas_unit_price)).context("Calculated max-gas from provided --max-fee and the current network gas price is 0. Please increase --max-fee to obtain a positive gas amount")?; FeeSettings::Strk { From 4d7229ffdb30a18593f7f07844ecb4da2c44f630 Mon Sep 17 00:00:00 2001 From: Fiiranek Date: Tue, 14 Jan 2025 14:04:10 +0100 Subject: [PATCH 36/36] Add todo --- crates/sncast/tests/e2e/invoke.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/sncast/tests/e2e/invoke.rs b/crates/sncast/tests/e2e/invoke.rs index 7eb81cf771..17cb37357b 100644 --- a/crates/sncast/tests/e2e/invoke.rs +++ b/crates/sncast/tests/e2e/invoke.rs @@ -450,6 +450,7 @@ fn test_calculated_max_gas_equal_to_zero_when_max_fee_passed() { let snapbox = runner(&args); let output = snapbox.assert().success(); + // TODO(#2852) assert_stderr_contains( output, indoc! {r"