test(blockifier): max gas price for new resource bounds

[aner-starkware]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.05%. Comparing base (b0cfe82) to head (887c539).
Report is 76 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #997      +/-   ##
==========================================
- Coverage   74.18%   70.05%   -4.13%     
==========================================
  Files         359       87     -272     
  Lines       36240    11219   -25021     
  Branches    36240    11219   -25021     
==========================================
- Hits        26886     7860   -19026     
+ Misses       7220     2971    -4249     
+ Partials     2134      388    -1746     

Flag	Coverage Δ
	70.05% <ø> (-4.13%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TzahiTaub]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)

---

crates/blockifier/src/transaction/transactions_test.rs line 951 at r1 (raw file):

        max_fee: Fee(MAX_FEE)
    };
    let tx = &account_invoke_tx(valid_invoke_tx_args.clone());

I think its better to start with asserting a successful execution of this tx with the default resource bounds defined below.

---

crates/blockifier/src/transaction/transactions_test.rs line 979 at r1 (raw file):

    // Max gas price too low, new resource bounds.
    for (_resource, insufficient_price_resource_bounds) in [

This is a used var

Suggestion:

insufficient_resource

---

crates/blockifier/src/transaction/transactions_test.rs line 1016 at r1 (raw file):

        });
        let execution_error = invalid_v3_tx.execute(state, block_context, true, true).unwrap_err();
        assert_matches!(

Add a TODO to add a variation of more than 1 insufficient resource, after the error message is updated to return all of them.

[aner-starkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/blockifier/src/transaction/transactions_test.rs line 951 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

I think its better to start with asserting a successful execution of this tx with the default resource bounds defined below.

Done. I added this assertion at the end to avoid complicating the test by increasing the nonce.

---

crates/blockifier/src/transaction/transactions_test.rs line 979 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

This is a used var

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 1016 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Add a TODO to add a variation of more than 1 insufficient resource, after the error message is updated to return all of them.

Done.

[TzahiTaub]

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)

---

crates/blockifier/src/test_utils.rs line 167 at r2 (raw file):

// TODO: Default testing bounds should probably be AllResourceBounds variant.
pub fn default_testing_resource_bounds() -> ValidResourceBounds {

Why are these defined? Seems they are not used anywhere

---

crates/blockifier/src/transaction/transactions_test.rs line 951 at r1 (raw file):

Previously, aner-starkware wrote…

Done. I added this assertion at the end to avoid complicating the test by increasing the nonce.

Can you just run it before with a different sender address (account_contract.get_instance_address(1))? The current is problematic, because if for some reason the "valid" tx will fail because of some bug, it will probably mean the invalid txs will earlier, and they will lead the debugger to the wrong direction of where the bug is.

[TzahiTaub]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @dorimedini-starkware)

---

crates/blockifier/src/transaction/transactions_test.rs line 951 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Can you just run it before with a different sender address (account_contract.get_instance_address(1))? The current is problematic, because if for some reason the "valid" tx will fail because of some bug, it will probably mean the invalid txs will earlier, and they will lead the debugger to the wrong direction of where the bug is.

OK, never mind, didn't think about it right.

[TzahiTaub]

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)

---

crates/blockifier/src/transaction/transactions_test.rs line 1134 at r1 (raw file):

                l1_data_gas: ResourceBounds { max_amount: 0, max_price_per_unit: actual_strk_l1_data_gas_price.into() }
            }),
        ..valid_invoke_tx_args.clone()

Isn't this included in the new test you've made?

Code quote:

    // Max L1 gas price too low, new resource bounds.
    // TODO: update l1_data max_amount and l2_data max_amount once minimal estimations are done.
    let insufficient_max_l1_gas_price = u128::from(actual_strk_l1_gas_price) - 1;
    let invalid_v3_tx = account_invoke_tx(invoke_tx_args! {
        resource_bounds: ValidResourceBounds::AllResources(
            AllResourceBounds{
                l1_gas: ResourceBounds{max_amount: minimal_l1_gas, max_price_per_unit: insufficient_max_l1_gas_price },
                l2_gas: ResourceBounds { max_amount: 0, max_price_per_unit: actual_strk_l2_gas_price.into() },
                l1_data_gas: ResourceBounds { max_amount: 0, max_price_per_unit: actual_strk_l1_data_gas_price.into() }
            }),
        ..valid_invoke_tx_args.clone()

[aner-starkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/blockifier/src/test_utils.rs line 167 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Why are these defined? Seems they are not used anywhere

Done.

[aner-starkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/blockifier/src/transaction/transactions_test.rs line 951 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

OK, never mind, didn't think about it right.

Done.

[TzahiTaub]

Reviewed 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)