test(blockifier): inner revert in cairo 1 contract

[AvivYossef-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[AvivYossef-starkware]

• refactor(blockifier): revert contract test #2941
• test(blockifier): revert storgae changes #2913
• test(blockifier): inner revert in cairo 1 contract #2889 👈 (View in Graphite)
• test(blockifier): storage changes after inner revert #2918
• main-v0.13.4

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

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-23T07:55:09Z
• Started at 2024-12-23T10:09:18Z
• Started at 2024-12-23T11:10:58Z
• Started at 2024-12-23T11:12:55Z
• Started at 2024-12-23T11:14:40Z
• Started at 2024-12-24T09:04:10Z
• Started at 2024-12-24T09:06:59Z
• Started at 2024-12-24T09:25:08Z
• Started at 2024-12-24T10:23:58Z
• Started at 2024-12-24T10:47:21Z
• Started at 2024-12-24T10:49:45Z
• Started at 2024-12-24T11:44:40Z
• Started at 2024-12-24T12:21:17Z
• Started at 2024-12-24T12:21:53Z

[AvivYossef-starkware]

It won't pass the CI until #2895 will merge

Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

[dorimedini-starkware]

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

---

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 669 at r2 (raw file):

                else {
                    assert(inner_error == 'execute_and_revert', 'Wrong_error');
                }

consider adding this for clarity. non-blocking

Suggestion:

                else {
                    assert(entry_point_selector == selector!("middle_revert_contract"));
                    assert(inner_error == 'execute_and_revert', 'Wrong_error');
                }

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 87 at r2 (raw file):

/// 2. Contract B error as expected.
/// 3. Tracked resources are correctly identified as SierraGas in all calls.
#[rstest]

if you use rstest you should probably use the case attributes

Suggestion:

#[rstest]
#[cfg_attr(feature = "cairo_native", case::native(RunnableCairo1::Native))]
#[case::vm(RunnableCairo1::Casm)]
/// This test verifies the behavior of a contract call sequence with nested calls and state
/// assertions.
///
/// - Contract A calls Contract B and asserts that the state remains unchanged.
/// - Contract B calls Contract C and panics.
/// - Contract C modifies the state but does not panic.
///
/// The test ensures that:
/// 1. Contract A's state remains unaffected despite the modifications in Contract C.
/// 2. Contract B error as expected.
/// 3. Tracked resources are correctly identified as SierraGas in all calls.

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 88 at r2 (raw file):

/// 3. Tracked resources are correctly identified as SierraGas in all calls.
#[rstest]
fn test_call_contract_and_than_revert(runnable_version: RunnableCairo1) {

if you use rstest you should probably use the case attributes

Suggestion:

#[rstest]
#[cfg_attr(feature = "cairo_native", case::native(RunnableCairo1::Native))]
#[case::vm(RunnableCairo1::Casm)]
/// This test verifies the behavior of a contract call sequence with nested calls and state
/// assertions.
///
/// - Contract A calls Contract B and asserts that the state remains unchanged.
/// - Contract B calls Contract C and panics.
/// - Contract C modifies the state but does not panic.
///
/// The test ensures that:
/// 1. Contract A's state remains unaffected despite the modifications in Contract C.
/// 2. Contract B error as expected.
/// 3. Tracked resources are correctly identified as SierraGas in all calls.
fn test_call_contract_and_than_revert(#[case] runnable_version: RunnableCairo1) {

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 144 at r2 (raw file):

        assert_eq!(call.tracked_resource, TrackedResource::SierraGas);
    }
}

how about testing also that state changes in A are not reverted?
maybe add some dummy storage-write in A's context, and assert it's still included in the diff?

[Yoni-Starkware]

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

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 134 at r2 (raw file):

    assert!(inner_call.execution.failed);
    assert!(inner_call.execution.events.is_empty());
    assert!(inner_call.execution.l2_to_l1_messages.is_empty());

Check C as well - it should not have events or messages on it, even though it didn't fail

Code quote:

    assert!(inner_call.execution.events.is_empty());
    assert!(inner_call.execution.l2_to_l1_messages.is_empty());

[AvivYossef-starkware]

Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @ilyalesokhin-starkware, and @Yoni-Starkware)

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 87 at r2 (raw file):

Previously, dorimedini-starkware wrote…

if you use rstest you should probably use the case attributes

Done.

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 88 at r2 (raw file):

Previously, dorimedini-starkware wrote…

if you use rstest you should probably use the case attributes

Done.

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 134 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Check C as well - it should not have events or messages on it, even though it didn't fail

Done.

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 144 at r2 (raw file):

Previously, dorimedini-starkware wrote…

how about testing also that state changes in A are not reverted?
maybe add some dummy storage-write in A's context, and assert it's still included in the diff?

Sounds good, I opened separate PR for it

[dorimedini-starkware]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware, @ilyalesokhin-starkware, and @Yoni-Starkware)

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 145 at r3 (raw file):

    };
    assert!(!inner_inner_call_c.execution.failed);
    

non-blocking but you seem to have extra whitespace here - please check your local formatter (if you merge this, this will appear as a diff in someone else's PR later)

[Yoni-Starkware]

Reviewed 1 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @ilyalesokhin-starkware)

[AvivYossef-starkware]

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 145 at r3 (raw file):

Previously, dorimedini-starkware wrote…

non-blocking but you seem to have extra whitespace here - please check your local formatter (if you merge this, this will appear as a diff in someone else's PR later)

Is it ok now?
The CI failed on the previous

[dorimedini-starkware]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @ilyalesokhin-starkware)

---

a discussion (no related file):
blocking until bottom PR is merged, so I'm sure it's over main-v0.13.4

[AvivYossef-starkware]

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

a discussion (no related file):

Previously, dorimedini-starkware wrote…

blocking until bottom PR is merged, so I'm sure it's over main-v0.13.4

Done.

[dorimedini-starkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[AvivYossef-starkware]

Merge activity

• Dec 24, 7:56 AM EST: A user merged this pull request with Graphite.