test(starknet_consensus_orchestrator): central transaction execution info object test

[Yael-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[Yael-Starkware]

• test(starknet_batcher): verify execution info order matches the order of transactions in a block #3616
• feat(starknet_consensus_orchestrator): add compressed_state_diff to cende objects #3557
• feat(starknet_consensus_orchestrator): pass central objects from batcher to cende #3384
• test(starknet_consensus_orchestrator): central transaction execution info object test #3337 👈 (View in Graphite)
• feat(sequencing): add execution info central object #3238
• chore(sequencing): reorder central transactions fields #3230
• test(sequencing): add non-default values to central objects test #3198
• test(sequencing): add non-default values to central objects test #3197
• main

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

[Yael-Starkware]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 261 at r1 (raw file):

/// Recursively traverse a transaction execution info json and sort the HashSet fields.
fn sort_execution_info_hashset_fields(json: &Value) -> Value {

This is a pretty ugly solution but I couldn't find another way to compare the jsons. please let me know if you have a better idea.

[DvirYo-starkware]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/Cargo.toml line 12 at r1 (raw file):

assert_matches.workspace = true
async-trait.workspace = true
blockifier= { workspace = true, features = ["transaction_serde"] }

What does this feature do?
Do you need to change something here?

sequencer/.github/workflows/blockifier_ci.yml

Line 56 in 15285e6

# transaction_serde is not activated by any workspace crate; test the build.

Code quote:

blockifier= { workspace = true, features = ["transaction_serde"] }

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 268 at r1 (raw file):

                if key == "accessed_storage_keys" || key == "accessed_contract_addresses" {
                    if let Value::Array(array) = value {
                        let mut sorted_array = array.clone();

What happens if the inner values in the array also have HashSet? I think this function will not work. The items will be serialized using serde_json::to_string, and the order of this serialization is not deterministic.
It may work fine for this case, but comment and explain this explicitly.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 301 at r1 (raw file):

    let python_json = read_json_file(python_json_path);

    assert_json_eq(&rust_json, &python_json, "serialize_central_objects test".to_string());

Can you add the test name to the error message?

Code quote:

 "serialize_central_objects test".to_string()

---

crates/sequencing/papyrus_consensus_orchestrator/resources/central_transaction_execution_info.json line 1 at r1 (raw file):

{

Don't use default values here, like in the previous PR.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects.rs line 364 at r1 (raw file):

/// A mapping from a transaction execution resource to its actual usage.
#[derive(Debug, Eq, PartialEq, Serialize, Deserialize)]

This is needed only for tests? I think it is fine to not compile only for tests, but just to make sure.

Code quote:

Deserialize

[github-actions]

Artifacts upload workflows:

• Started at 2025-01-16T10:42:02Z
• Started at 2025-01-21T13:44:59Z
• Started at 2025-01-21T14:02:36Z
• Started at 2025-01-22T06:02:38Z
• Started at 2025-01-22T06:30:07Z

[Yael-Starkware]

Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/Cargo.toml line 12 at r1 (raw file):

Previously, DvirYo-starkware wrote…

What does this feature do?
Do you need to change something here?

sequencer/.github/workflows/blockifier_ci.yml

Line 56 in 15285e6

# transaction_serde is not activated by any workspace crate; test the build.

will check with Dori

---

crates/sequencing/papyrus_consensus_orchestrator/resources/central_transaction_execution_info.json line 1 at r1 (raw file):

Previously, DvirYo-starkware wrote…

Don't use default values here, like in the previous PR.

Done.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 268 at r1 (raw file):

Previously, DvirYo-starkware wrote…

What happens if the inner values in the array also have HashSet? I think this function will not work. The items will be serialized using serde_json::to_string, and the order of this serialization is not deterministic.
It may work fine for this case, but comment and explain this explicitly.

code was deleted.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 301 at r1 (raw file):

Previously, DvirYo-starkware wrote…

Can you add the test name to the error message?

the test name is the only thing in the error message .
or maybe I misunderstood you.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects.rs line 364 at r1 (raw file):

Previously, DvirYo-starkware wrote…

This is needed only for tests? I think it is fine to not compile only for tests, but just to make sure.

yes

[dafnamatsry]

Reviewed all commit messages.
Reviewable status: 0 of 5 files reviewed, 7 unresolved discussions (waiting on @DvirYo-starkware and @Yael-Starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 282 at r2 (raw file):

// the entire object, we fill only one CallInfo with non-default values and the other CallInfos are
// None.
fn central_transaction_execution_info() -> Value {

Split to several functions:

• test_call_info()with empty inner_calls - which can be used for all the CallInfo mebers. And maybe this would work:

CallInfo {
    inner_calls: vec![test_call_info()]
    ...test_call_info()
}

• test_transaction_receipt()

You can also add test_execution_resources() which is repeated here.

[Yael-Starkware]

Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 282 at r2 (raw file):

Previously, dafnamatsry wrote…

Split to several functions:

• test_call_info()with empty inner_calls - which can be used for all the CallInfo mebers. And maybe this would work:

CallInfo {
    inner_calls: vec![test_call_info()]
    ...test_call_info()
}

• test_transaction_receipt()

You can also add test_execution_resources() which is repeated here.

done.

[Yael-Starkware]

Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/Cargo.toml line 12 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

will check with Dori

this is not relevant anymore.

[DvirYo-starkware]

I think I see changes also from different PRs, for example, the function fn central_sierra_contract_class_json() -> Value. please remove them

Reviewed 4 of 5 files at r3, all commit messages.
Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 301 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

the test name is the only thing in the error message .
or maybe I misunderstood you.

I mean, for example, for execution info, print the case name transaction_execution_info. Not very important

[Yael-Starkware]

Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 301 at r1 (raw file):

Previously, DvirYo-starkware wrote…

I mean, for example, for execution info, print the case name transaction_execution_info. Not very important

the test name always appears in the title anyway, so I change the string to describe the failure.

[dafnamatsry]

Reviewed 1 of 5 files at r3, 3 of 3 files at r5, all commit messages.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @DvirYo-starkware and @Yael-Starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 377 at r5 (raw file):

            counter.insert(BuiltinName::pedersen, 4);
            counter
        },

Suggestion:

        builtin_instance_counter: HashMap::from([
            (BuiltinName::range_check, 31),
            (BuiltinName::pedersen, 4),
        ]),

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 437 at r5 (raw file):

        validate_call_info: Some(CallInfo { inner_calls: vec![call_info()], ..call_info() }),
        execute_call_info: None,
        fee_transfer_call_info: None,

and update the comment...

Suggestion:

        execute_call_info: Some(CallInfo { inner_calls: vec![call_info()], ..call_info() }),,
        fee_transfer_call_info: Some(CallInfo { inner_calls: vec![call_info()], ..call_info() }),,

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 460 at r5 (raw file):

                starknet_resources: StarknetResources {
                    // The archival_data has private fields so it cannot be assigned, however, it is
                    // not being used in the central object anyway so it can be default.

This is true for the whole resources filed. Consider using TransactionResources::default().

Code quote:

                    // The archival_data has private fields so it cannot be assigned, however, it is
                    // not being used in the central object anyway so it can be default.

[Yael-Starkware]

Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 437 at r5 (raw file):

Previously, dafnamatsry wrote…

and update the comment...

no need to test the same object again and again.
it also makes the json file more complex to create and maintain.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 460 at r5 (raw file):

Previously, dafnamatsry wrote…

This is true for the whole resources filed. Consider using TransactionResources::default().

I think this suggestion makes the test cover less edge cases.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 377 at r5 (raw file):

            counter.insert(BuiltinName::pedersen, 4);
            counter
        },

Done.

[dafnamatsry]

Reviewable status: 3 of 5 files reviewed, 5 unresolved discussions (waiting on @DvirYo-starkware and @Yael-Starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 437 at r5 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

no need to test the same object again and again.
it also makes the json file more complex to create and maintain.

Putting None here doesn't test the serialisation of the field at all.
It would be enough to change the type of the field to Optional<SomeOtherType> and the test would not catch that.

[Yael-Starkware]

Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 437 at r5 (raw file):

Previously, dafnamatsry wrote…

Putting None here doesn't test the serialisation of the field at all.
It would be enough to change the type of the field to Optional<SomeOtherType> and the test would not catch that.

Done.

[dafnamatsry]

Reviewed 3 of 8 files at r7, 5 of 5 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware)

[DvirYo-starkware]

Reviewed 1 of 2 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)