Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support gas calculation #1037

Merged
merged 34 commits into from
Nov 28, 2023
Merged

Support gas calculation #1037

merged 34 commits into from
Nov 28, 2023

Conversation

war-in
Copy link
Member

@war-in war-in commented Nov 3, 2023

Closes #834

Introduced changes

  • SierraCasmRunner was copied (and slightly modified) to enable gas calculation

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@war-in war-in self-assigned this Nov 3, 2023
@war-in war-in requested review from a team, MaksymilianDemitraszek, drknzz and tkumor3 and removed request for a team November 3, 2023 11:51
@war-in war-in marked this pull request as draft November 3, 2023 11:51
@war-in war-in requested review from Arcticae and removed request for drknzz and tkumor3 November 3, 2023 11:51
# Conflicts:
#	crates/forge/src/pretty_printing.rs
#	crates/forge/src/running.rs
#	crates/forge/src/test_case_summary.rs
@MaksymilianDemitraszek
Copy link
Member

I would like to review this one before merging

@war-in
Copy link
Member Author

war-in commented Nov 20, 2023

Please do not review 🔒 Changes are coming

# Conflicts:
#	crates/forge-runner/src/running.rs
#	crates/forge/src/lib.rs
#	crates/forge/src/pretty_printing.rs
@war-in
Copy link
Member Author

war-in commented Nov 21, 2023

Changelog still needs updating, and docs if applicable

Not sure tbh, because it does not affect users (I've removed printing)

Copy link
Contributor

@drknzz drknzz left a comment

Choose a reason for hiding this comment

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

:shipit:

crates/forge-runner/Cargo.toml Outdated Show resolved Hide resolved
crates/forge-runner/src/running.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/running.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/sierra_casm_runner.rs Show resolved Hide resolved
crates/forge/tests/integration/gas.rs Show resolved Hide resolved
crates/forge-runner/src/running.rs Show resolved Hide resolved
crates/forge-runner/src/running.rs Show resolved Hide resolved
crates/forge-runner/src/running.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/running.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/sierra_casm_runner.rs Show resolved Hide resolved
crates/forge-runner/src/test_execution_syscall_handler.rs Outdated Show resolved Hide resolved
@war-in war-in requested a review from Arcticae November 24, 2023 09:30
@war-in
Copy link
Member Author

war-in commented Nov 24, 2023

As @piotmag769 and @Arcticae mentioned, we should add support for the onchain data.
It is described here and we should implement this as soon as we close this PR.

Created issue
#1186

Copy link
Member

@MaksymilianDemitraszek MaksymilianDemitraszek left a comment

Choose a reason for hiding this comment

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

Ok with merging 👍 I don't expect we will have it correct in the first shot.
Testing is imo a bit thin.

Please address comments before merging

crates/forge-runner/src/sierra_casm_runner.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/sierra_casm_runner.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/running.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/running.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/sierra_casm_runner.rs Outdated Show resolved Hide resolved
crates/forge-runner/src/test_execution_syscall_handler.rs Outdated Show resolved Hide resolved
@MaksymilianDemitraszek
Copy link
Member

Are there any test checking if gas from cheatnet is added to the final sum?

@war-in
Copy link
Member Author

war-in commented Nov 27, 2023

Are there any test checking if gas from cheatnet is added to the final sum?

Just added them 10974de

# Conflicts:
#	crates/cheatnet/tests/starknet/resources.rs
@Arcticae Arcticae added this pull request to the merge queue Nov 28, 2023
Merged via the queue into master with commit a679213 Nov 28, 2023
8 checks passed
@Arcticae Arcticae deleted the war-in/834-support-gas branch November 28, 2023 16:40
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.

Support calculating gas cost of whole test execution
6 participants