Skip to content

Conversation

@dorimedini-starkware
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

This was referenced Oct 14, 2025
Copy link
Collaborator Author

dorimedini-starkware commented Oct 14, 2025

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: 0 of 2 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware, @meship-starkware, and @nimrod-starkware)


a discussion (no related file):
reasons:

  1. one less object to carry around
  2. allows the next PR - reduce boilerplate with a utility method that invokes from the funded account with default resource bounds, while advancing the relevant nonce

Copy link
Contributor

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

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


crates/starknet_os_flow_tests/src/test_manager.rs line 247 at r1 (raw file):

            per_block_transactions: vec![vec![]],
        }
    }

Suggestion:

    /// Creates a new `TestManager` with the provided initial state data and nonce manager.
    pub(crate) fn new_with_initial_state_data_and_nonce_manager(
        initial_state_data: InitialStateData<S>,
        nonce_manager: NonceManager,
    ) -> Self {
        Self {
            initial_state: initial_state_data.initial_state,
            execution_contracts: initial_state_data.execution_contracts,
            nonce_manager,
            per_block_transactions: vec![vec![]],
        }
    }

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 @nimrod-starkware)


crates/starknet_os_flow_tests/src/test_manager.rs line 247 at r1 (raw file):

            per_block_transactions: vec![vec![]],
        }
    }

why? it's clear from the signature, and the current state of nonces is part of the initial state of the system

@dorimedini-starkware dorimedini-starkware force-pushed the 10-14-starknet_os_flow_tests_move_nonce_manager_into_test_manager branch from 3f7b611 to 13bb534 Compare October 15, 2025 10:25
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: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @meship-starkware, and @nimrod-starkware)


crates/starknet_os_flow_tests/src/test_manager.rs line 247 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why? it's clear from the signature, and the current state of nonces is part of the initial state of the system

done

Copy link
Contributor

@nimrod-starkware nimrod-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:

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

@dorimedini-starkware dorimedini-starkware changed the base branch from 09-30-starknet_os_flow_tests_migrate_test_reverted_invoke_tx to main October 15, 2025 10:41
@dorimedini-starkware dorimedini-starkware force-pushed the 10-14-starknet_os_flow_tests_move_nonce_manager_into_test_manager branch from 13bb534 to eeb8412 Compare October 15, 2025 10:41
@graphite-app
Copy link

graphite-app bot commented Oct 15, 2025

Merge activity

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

@dorimedini-starkware dorimedini-starkware added this pull request to the merge queue Oct 15, 2025
Merged via the queue into main with commit c1cdf8a Oct 15, 2025
14 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants