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
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 320 at r1 (raw file):

    /// Similar to `add_invoke_tx_from_args`, but with the sender address set to the funded account,
    /// nonce set (and incremented) and resource bounds set to the default (non-trivial).
    /// Assumes the tx should not be reverted.

Why do we assume that?

Code quote:

/// Assumes the tx should not be reverted.

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 320 at r1 (raw file):

Previously, nimrod-starkware wrote…

Why do we assume that?

to reduce boilerplate. in future PRs I add an option in add_invoke_tx_from_args to check for reverts - if you want a reverting tx, use add_invoke_tx_from_args

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @meship-starkware)

@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
@dorimedini-starkware dorimedini-starkware force-pushed the 10-14-starknet_os_flow_tests_reduce_boilerplate_with_add_funded_account_invoke branch from 1f680d7 to 1d91673 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.

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

@graphite-app graphite-app bot changed the base branch from 10-14-starknet_os_flow_tests_move_nonce_manager_into_test_manager to graphite-base/9563 October 15, 2025 10:41
@dorimedini-starkware dorimedini-starkware force-pushed the 10-14-starknet_os_flow_tests_reduce_boilerplate_with_add_funded_account_invoke branch from 1d91673 to aea0fb6 Compare October 15, 2025 10:54
@graphite-app graphite-app bot changed the base branch from graphite-base/9563 to main October 15, 2025 10:54
Copy link

graphite-app bot commented Oct 15, 2025

Merge activity

  • Oct 15, 10:54 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 34abd60 Oct 15, 2025
18 of 32 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.

3 participants