build: add more utils to regression test

[AvivYossef-starkware]

This change is 

[AvivYossef-starkware]

• test: blockifier regression test test utils #956 : 2 dependent PRs (#943 , #980 )
• build: add more utils to regression test #952 👈
• build: blockifier regression test add get fee token addresses #948
• build: blockifier regression test add block info #946
• build: regression test crate #945
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @AvivYossef-starkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

Attention: Patch coverage is 23.07692% with 20 lines in your changes missing coverage. Please review.

Project coverage is 68.67%. Comparing base (b0cfe82) to head (c64c596).
Report is 151 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier_regression_test/src/utils.rs	0.00%	15 Missing ⚠️
crates/blockifier/src/versioned_constants.rs	54.54%	5 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (b0cfe82) and HEAD (c64c596). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (b0cfe82)	HEAD (c64c596)
	3	1

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #952      +/-   ##
==========================================
- Coverage   74.18%   68.67%   -5.52%     
==========================================
  Files         359      132     -227     
  Lines       36240    13270   -22970     
  Branches    36240    13270   -22970     
==========================================
- Hits        26886     9113   -17773     
+ Misses       7220     3701    -3519     
+ Partials     2134      456    -1678     

Flag	Coverage Δ
	68.67% <23.07%> (-5.52%)	⬇️

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.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [67.001 ms 67.126 ms 67.287 ms]
change: [-7.8518% -4.3583% -1.4110%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
1 (1.00%) high mild
5 (5.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [67.146 ms 67.226 ms 67.315 ms]
change: [-7.3741% -4.0815% -1.1478%] (p = 0.01 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

[aner-starkware]

Reviewed 3 of 5 files at r1, all commit messages.
Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware)

---

crates/blockifier_regression_test/src/test_state_reader.rs line 77 at r2 (raw file):

            [0, 13, 2, 1] => Ok(StarknetVersion::V0_13_2_1),
            _ => Err(StateError::StateReadError("Failed to match starknet version".to_string())),
        }

Isn't it possible to do this using some sort of macro?

Code quote:

        match raw_version {
            [0, 13, 0] => Ok(StarknetVersion::V0_13_0),
            [0, 13, 1] => Ok(StarknetVersion::V0_13_1),
            [0, 13, 1, 1] => Ok(StarknetVersion::V0_13_1_1),
            [0, 13, 2] => Ok(StarknetVersion::V0_13_2),
            [0, 13, 2, 1] => Ok(StarknetVersion::V0_13_2_1),
            _ => Err(StateError::StateReadError("Failed to match starknet version".to_string())),
        }

---

crates/blockifier_regression_test/src/test_state_reader.rs line 77 at r2 (raw file):

            [0, 13, 2, 1] => Ok(StarknetVersion::V0_13_2_1),
            _ => Err(StateError::StateReadError("Failed to match starknet version".to_string())),
        }

You're changing this code in the following PR; why do you want me to review it here?

Code quote:

        let block_header: BlockHeader = serde_json::from_value(
            self.0.send_rpc_request("starknet_getBlockWithTxHashes", get_block_params)?,
        )
        .map_err(serde_err_to_state_err)?;
        let raw_version = block_header.starknet_version.0.as_slice();
        match raw_version {
            [0, 13, 0] => Ok(StarknetVersion::V0_13_0),
            [0, 13, 1] => Ok(StarknetVersion::V0_13_1),
            [0, 13, 1, 1] => Ok(StarknetVersion::V0_13_1_1),
            [0, 13, 2] => Ok(StarknetVersion::V0_13_2),
            [0, 13, 2, 1] => Ok(StarknetVersion::V0_13_2_1),
            _ => Err(StateError::StateReadError("Failed to match starknet version".to_string())),
        }

---

crates/blockifier_regression_test/src/test_utils.rs line 26 at r2 (raw file):

pub fn get_chain_info() -> ChainInfo {
    ChainInfo { chain_id: ChainId::Mainnet, fee_token_addresses: get_fee_token_addresses() }
}

Could you add some docstring here?

Code quote:

pub fn get_rpc_state_reader_config() -> RpcStateReaderConfig {
    RpcStateReaderConfig {
        url: RPC_NODE_URL.to_string(),
        json_rpc_version: JSON_RPC_VERSION.to_string(),
    }
}

pub fn get_chain_info() -> ChainInfo {
    ChainInfo { chain_id: ChainId::Mainnet, fee_token_addresses: get_fee_token_addresses() }
}

---

crates/gateway/src/lib.rs line 7 at r2 (raw file):

pub mod errors;
pub mod gateway;
pub mod rpc_objects;

Blocked until approved by @dorimedini-starkware .

Code quote:

pub 

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.298 ms 66.376 ms 66.471 ms]
change: [-8.8265% -5.2817% -2.1649%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe

[AvivYossef-starkware]

Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @aner-starkware)

---

crates/blockifier_regression_test/src/test_state_reader.rs line 77 at r2 (raw file):

Previously, aner-starkware wrote…

You're changing this code in the following PR; why do you want me to review it here?

I added a test in the following PR, and this function doesn't work as expected, I am moving the fix to this PR

---

crates/blockifier_regression_test/src/test_utils.rs line 26 at r2 (raw file):

Previously, aner-starkware wrote…

Could you add some docstring here?

Done.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [67.568 ms 67.664 ms 67.776 ms]
change: [-7.6334% -4.3078% -1.4736%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe

[aner-starkware]

crates/gateway/src/lib.rs line 7 at r2 (raw file):

Previously, aner-starkware wrote…

Blocked until approved by @dorimedini-starkware .

Please verify with GW team that this change is OK.

[dorimedini-starkware]

Reviewed 2 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

---

crates/blockifier_regression_test/src/test_state_reader.rs line 77 at r2 (raw file):

Previously, aner-starkware wrote…

Isn't it possible to do this using some sort of macro?

better yet: add an impl TryFrom<Vec<u8>> for StarknetVersion block in the define_vesioned_constants macro

---

crates/blockifier_regression_test/src/test_state_reader.rs line 57 at r4 (raw file):

            None => Self(RpcStateReader::from_number(&get_rpc_state_reader_config(), block_number)),
        }
    }

IMO it's less intuitive to change behavior like this based on None value.
consider reverting this change, and implementing a for_testing associated method on RpcStateReaderConfig that uses the constants you define in test_utils.
callers of TestStateReader::new should always pass explicit config...
or, you can also add a new_for_testing associated method on TestStateReader which calls new with this default config.

Code quote:

    pub fn new(config: Option<&RpcStateReaderConfig>, block_number: BlockNumber) -> Self {
        match config {
            Some(config) => Self(RpcStateReader::from_number(config, block_number)),
            None => Self(RpcStateReader::from_number(&get_rpc_state_reader_config(), block_number)),
        }
    }

---

crates/blockifier_regression_test/src/test_state_reader.rs line 84 at r4 (raw file):

        let starknet_version = self.get_starknet_version()?;
        let versioned_constants: &'static VersionedConstants = starknet_version.into();
        Ok(versioned_constants)

does this work? more concise

Suggestion:

        Ok(self.get_starknet_version()?.into())

---

crates/blockifier_regression_test/src/test_state_reader.rs line 90 at r4 (raw file):

        Ok(BlockContext::new(
            self.get_block_info()?,
            get_chain_info(),

this should be configurable; we may want to reexecute testnet blocks, no?

Code quote:

get_chain_info(),

---

crates/blockifier_regression_test/src/test_state_reader.rs line 91 at r4 (raw file):

            self.get_block_info()?,
            get_chain_info(),
            self.get_versioned_constants()?.clone(),

why clone? it's static; do we always clone VC when creating block context?

Code quote:

self.get_versioned_constants()?.clone()

---

crates/blockifier_regression_test/src/test_state_reader.rs line 103 at r4 (raw file):

            CachedState::new(test_state_reader),
            block_context,
            TransactionExecutorConfig::default(),

don't we want to control this? is it in a later PR?

Code quote:

TransactionExecutorConfig::default()

---

crates/blockifier_regression_test/src/test_utils.rs line 0 at r4 (raw file):
consider renaming this module to utils - it's only "test utils" because of the crate it's in

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.662 ms 34.717 ms 34.777 ms]
change: [-3.9844% -2.6832% -1.5708%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe

[dorimedini-starkware]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

[aner-starkware]

Reviewed 1 of 2 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @AvivYossef-starkware)

[aner-starkware]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @AvivYossef-starkware)

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.352 ms 34.385 ms 34.424 ms]
change: [-4.0116% -3.2451% -2.5073%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
1 (1.00%) high mild
5 (5.00%) high severe

[AvivYossef-starkware]

Reviewable status: 3 of 7 files reviewed, 6 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)

---

crates/blockifier_regression_test/src/test_state_reader.rs line 77 at r2 (raw file):

Previously, dorimedini-starkware wrote…

better yet: add an impl TryFrom<Vec<u8>> for StarknetVersion block in the define_vesioned_constants macro

Done

---

crates/blockifier_regression_test/src/test_state_reader.rs line 57 at r4 (raw file):

Previously, dorimedini-starkware wrote…

IMO it's less intuitive to change behavior like this based on None value.
consider reverting this change, and implementing a for_testing associated method on RpcStateReaderConfig that uses the constants you define in test_utils.
callers of TestStateReader::new should always pass explicit config...
or, you can also add a new_for_testing associated method on TestStateReader which calls new with this default config.

Done.

---

crates/blockifier_regression_test/src/test_state_reader.rs line 84 at r4 (raw file):

Previously, dorimedini-starkware wrote…

does this work? more concise

Yes, thanks

---

crates/blockifier_regression_test/src/test_state_reader.rs line 90 at r4 (raw file):

Previously, dorimedini-starkware wrote…

this should be configurable; we may want to reexecute testnet blocks, no?

Yes, but I thought I would start with support for Mainnet only. I might need to make more changes to support Testnet.

---

crates/blockifier_regression_test/src/test_state_reader.rs line 91 at r4 (raw file):

Previously, dorimedini-starkware wrote…

why clone? it's static; do we always clone VC when creating block context?

The block context needs to own the versioned constants, so it does not matter that it is static. We also clone it here. and here

---

crates/blockifier_regression_test/src/test_state_reader.rs line 103 at r4 (raw file):

Previously, dorimedini-starkware wrote…

don't we want to control this? is it in a later PR?

Yes, I just try to start with the minimum that works

---

crates/gateway/src/lib.rs line 7 at r2 (raw file):

Previously, aner-starkware wrote…

Please verify with GW team that this change is OK.

@dafnamatsry can you plz approve?

[dorimedini-starkware]

Reviewed 4 of 4 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware)

---

crates/blockifier/src/versioned_constants.rs line 69 at r6 (raw file):

        impl TryFrom<&str> for StarknetVersion {

fix whitespace

Code quote:

        }



        impl TryFrom<&str> for StarknetVersion {

---

crates/blockifier/src/versioned_constants.rs line 79 at r6 (raw file):

                }
            }
        }

consider adding this to prevent human errors in macro construction

Suggestion:

}

#[cfg(test)]
#[test]
fn test_variant_name_string_consistency() {
    $(
        assert_eq!(
            "v".to_owned() + $version_str,
            String::from(stringify!($variant)).to_lowercase().replace("_",".")
        );
    )*
}

[dorimedini-starkware]

Reviewed 1 of 2 files at r8, all commit messages.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware)

---

crates/blockifier/src/versioned_constants.rs line 79 at r6 (raw file):

Previously, dorimedini-starkware wrote…

consider adding this to prevent human errors in macro construction

ok now that you are rebased, you can compare the version string to the variant using String::from(StarknetVersion::$variant)

[dorimedini-starkware]

Reviewed 1 of 2 files at r8.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware)

---

crates/blockifier/src/versioned_constants.rs line 117 at r8 (raw file):

        #[cfg(test)]
        mod tests {

the mod is required? tests need a separate scope?

Code quote:

mod tests {

---

crates/blockifier/src/versioned_constants.rs line 124 at r8 (raw file):

                    assert_eq!(
                        "v".to_owned() + $version_str,
                        String::from(stringify!($variant)).to_lowercase().replace("_", ".")

now that you are rebased, you can do this instead

Suggestion:

String::from(StarknetVersion::$variant)

[AvivYossef-starkware]

Reviewable status: 5 of 7 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/src/versioned_constants.rs line 69 at r6 (raw file):

Previously, dorimedini-starkware wrote…

fix whitespace

Done.

---

crates/blockifier/src/versioned_constants.rs line 79 at r6 (raw file):

Previously, dorimedini-starkware wrote…

consider adding this to prevent human errors in macro construction

Done.

---

crates/blockifier/src/versioned_constants.rs line 117 at r8 (raw file):

Previously, dorimedini-starkware wrote…

the mod is required? tests need a separate scope?

Yes, "Rust requires tests to be inside a module that is annotated with #[cfg(test)] to ensure they are compiled and run only during testing."

---

crates/blockifier/src/versioned_constants.rs line 124 at r8 (raw file):

Previously, dorimedini-starkware wrote…

now that you are rebased, you can do this instead

Done.

[dorimedini-starkware]

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