Skip to content

Conversation

dorimedini-starkware
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator Author

dorimedini-starkware commented Sep 30, 2025

Copy link
Contributor

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have access to the reason for reverting?
If so,, can you verify that the revert reason is as expected?

@AvivYossef-starkware reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @Yoni-Starkware)


crates/starknet_os_flow_tests/src/tests.rs line 263 at r1 (raw file):

        sender_address: *FUNDED_ACCOUNT_ADDRESS,
        nonce: nonce_manager.next(*FUNDED_ACCOUNT_ADDRESS),
        calldata: create_calldata(test_contract_address, "write_and_revert", &[]),

The entry point needs to receive a value (storage write).

Code quote:

calldata: create_calldata(test_contract_address, "write_and_revert", &[])

crates/starknet_os_flow_tests/src/tests.rs line 284 at r1 (raw file):

    assert!(
        test_output.decompressed_state_diff.storage_updates.contains_key(&STRK_FEE_TOKEN_ADDRESS)
    );

Can you check that srorage key[FUNDED_ACCOUNT_ADDRESS] has changed? ( for the STRK_FEE_TOKEN_ADDRESS )

Code quote:

    assert!(
        test_output.decompressed_state_diff.storage_updates.contains_key(&STRK_FEE_TOKEN_ADDRESS)
    );

crates/blockifier_test_utils/resources/feature_contracts/cairo1/test_contract.cairo line 63 at r1 (raw file):

    }

    #[external(v0)]

Can you use test_revert_helper ?
( or do you want to be more specific )

Code quote:

    fn write_and_revert(self: @ContractState, address: StorageAddress, value: felt252) {
        let address_domain = 0;
        syscalls::storage_write_syscall(address_domain, address, value).unwrap_syscall();
        assert(1 == 0, 'Panic for revert');
    }

    #[external(v0)]

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already done :) (2 PRs ahead of this one)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @meship-starkware, and @Yoni-Starkware)


crates/starknet_os_flow_tests/src/tests.rs line 263 at r1 (raw file):

Previously, AvivYossef-starkware wrote…

The entry point needs to receive a value (storage write).

you are correct, this is caught and fixed here, where I add control over revert reasons

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @meship-starkware, and @Yoni-Starkware)


crates/blockifier_test_utils/resources/feature_contracts/cairo1/test_contract.cairo line 63 at r1 (raw file):

Previously, AvivYossef-starkware wrote…

Can you use test_revert_helper ?
( or do you want to be more specific )

  1. I want to be more specific
  2. I want to use the same call and calldata for both cairo0 and cairo1 contracts - test_revert_helper is not on the cairo0 contract

Copy link
Contributor

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware, @meship-starkware, and @Yoni-Starkware)


crates/starknet_os_flow_tests/src/tests.rs line 263 at r1 (raw file):

Previously, dorimedini-starkware wrote…

you are correct, this is caught and fixed here, where I add control over revert reasons

sababa


crates/starknet_os_flow_tests/src/tests.rs line 243 at r1 (raw file):

#[rstest]
#[tokio::test]
async fn test_reverted_invoke_tx(

Suggestion:

/// This test verifies that when an entry point modifies storage and then reverts (panics):
/// 1. All storage changes made before the revert are properly rolled back.
/// 2. The transaction fee is still deducted from the caller's account.
async fn test_reverted_invoke_tx(

crates/starknet_os_flow_tests/src/tests.rs line 284 at r1 (raw file):

    assert!(
        test_output.decompressed_state_diff.storage_updates.contains_key(&STRK_FEE_TOKEN_ADDRESS)
    );

Can we verify specifically that the sender's balance changes?

Code quote:

    assert!(
        test_output.decompressed_state_diff.storage_updates.contains_key(&STRK_FEE_TOKEN_ADDRESS)
    );

@dorimedini-starkware dorimedini-starkware force-pushed the 09-30-starknet_os_flow_tests_migrate_test_reverted_invoke_tx branch from 618347c to 45ca679 Compare October 5, 2025 07:44
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @meship-starkware, and @Yoni-Starkware)


crates/starknet_os_flow_tests/src/tests.rs line 263 at r1 (raw file):

Previously, AvivYossef-starkware wrote…

sababa

Done.

Copy link
Contributor

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@AvivYossef-starkware reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware and @Yoni-Starkware)

@dorimedini-starkware dorimedini-starkware changed the base branch from 09-30-starknet_os_flow_tests_reduce_boilerplate_with_perform_default_validations to main October 5, 2025 10:25
@dorimedini-starkware dorimedini-starkware force-pushed the 09-30-starknet_os_flow_tests_migrate_test_reverted_invoke_tx branch from 45ca679 to 8b36fcf Compare October 5, 2025 10:26
Copy link

graphite-app bot commented Oct 5, 2025

Merge activity

  • Oct 5, 10:26 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants