Validate max gas is greater than zero for strk fee settings #2796
Conversation
franciszekjob
requested review from
cptartur,
ddoktorski and
kkawula
as code owners
… of https://github.com/foundry-rs/starknet-foundry into franciszekjob/2706-validate-fee-args-greater-than-zero
crates/sncast/tests/e2e/invoke.rs
Outdated
There was a problem hiding this comment.
Do these tests refer to the reported issue?
#2697
I suppose that even if max_fee is greater than 0 an error may occur
There was a problem hiding this comment.
These ones refer to situation when user explicitly passes 0 to fee args.
The case when max gas or max gas unit price is calculated based on other values, is handled e.g. here.
… of https://github.com/foundry-rs/starknet-foundry into franciszekjob/2706-validate-fee-args-greater-than-zero
crates/sncast/tests/e2e/invoke.rs
Outdated
There was a problem hiding this comment.
The most important cases to test here are the ones where values calculated from --max-fee are 0. We should add these.
There was a problem hiding this comment.
Fine, so let's break it down:
- case when max fee and max gas unit price are passed - calculated
max_gaswill always be >= 1 because of previous check formax_fee < max_gas_unit_price, so it's unreachable for it to be equal to 0 - case when max fee and max gas are passed - same as above
- case when only max fee is passed - it's tested by test_calculated_max_gas_equal_to_zero_when_max_fee_passed.
| 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()), |
There was a problem hiding this comment.
Why are you using NonZeroU64::new in some test cases and Felt::from... in other cases for the same field? Let's pick a consistent behavior if it's possible.
There was a problem hiding this comment.
It's because fee-related fields in FeeArgs use NonZeroFelt while in FeeSettings::Strk/Eth they use either NonZeroFelt, NonZeroU64 or NonZeroU128 (ofc wrapped Option).
There was a problem hiding this comment.
cc @Draggu please take a look at these
crates/conversions/src/felt.rs
Outdated
| 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"); |
There was a problem hiding this comment.
As @ksew1 said ^ don't panic in fallible conversion. It is highly unexpected.
… into franciszekjob/2706-validate-fee-args-greater-than-zero
franciszekjob
changed the title
| output, | ||
| indoc! {r" | ||
| 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 |
There was a problem hiding this comment.
Does it need to include : Tried to create NonZeroFelt from 0?
| } | ||
|
|
||
| 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")); |
There was a problem hiding this comment.
Couldn't some rounding errors still make this <1 in special cases?
There was a problem hiding this comment.
No, because we explicitly ensure that max_fee >= max_gas_unit_price before this calculation. Since floor_div performs exact integer division (rounding down), and both max_fee and max_gas_unit_price are non-zero, the result of floor_div will always be >=1.
Closes #2706
Introduced changes
Validate if fee args are greater than 0
max_fee,max_gasandmax_gas_unit_pricefromOption<Felt>toOption<NonZeroFelt>Checklist
CHANGELOG.md