Skip to content

Display gas report table #3842

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: share-contracts-data-store
Choose a base branch
from
Open

Display gas report table #3842

wants to merge 3 commits into from
+239 −8

Conversation

@ddoktorski
Copy link
Contributor

Towards #3660

@ddoktorski
Copy link
Contributor Author

ddoktorski commented Oct 21, 2025 •
edited
Loading

This change is part of the following stack:

Change managed by git-spice.

@ddoktorski ddoktorski requested a review from a team as a code owner October 21, 2025 11:52
@ddoktorski ddoktorski requested review from MKowalski8 and franciszekjob and removed request for a team October 21, 2025 11:52
This was referenced Oct 21, 2025
Merged
Open
Merged
@ddoktorski ddoktorski marked this pull request as draft October 21, 2025 11:52
@ddoktorski ddoktorski force-pushed the add-gas-report-logic branch 2 times, most recently from 8a9f925 to 8f3b1f6 Compare October 22, 2025 18:37
@ddoktorski ddoktorski mentioned this pull request Oct 22, 2025
Open
@ddoktorski ddoktorski changed the base branch from add-gas-report-logic to share-contracts-data-store October 22, 2025 18:58
@ddoktorski ddoktorski force-pushed the 3660-gas-report branch 2 times, most recently from 8517806 to 73d507a Compare October 23, 2025 12:32
cptartur
cptartur reviewed Oct 24, 2025
View reviewed changes
crates/forge-runner/src/gas/report.rs Outdated
if !contract_info.functions.is_empty() {
let table = format_table_output(contract_info, name);
writeln!(f, "\n{table}")?;
Copy link
Member

Choose a reason for hiding this comment

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

In what case can there be contract without functions? In case it wasn't actually called in tests or some other ones as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this shouldn't really happen. It's just an extra cautious step to make sure we don't display an empty table

crates/forge-runner/src/gas/report.rs Outdated
Comment on lines 3 to 4
use comfy_table::modifiers::UTF8_ROUND_CORNERS;
use comfy_table::{Cell, Color, Table};
Copy link
Member

Choose a reason for hiding this comment

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

Glad someone already made a lib for that and we don't have to deal with printing tables

crates/forge-runner/src/gas/report.rs Outdated
Cell::new(report_data.gas_stats.min.to_string()).fg(Color::Green),
Cell::new(report_data.gas_stats.max.to_string()).fg(Color::Red),
Cell::new(report_data.gas_stats.mean.round().to_string()).fg(Color::Yellow),
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this coloring. I'd rather eventually have some colors based on values or something. Maybe let's just have them all single color or even just white?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

franciszekjob
franciszekjob reviewed Oct 24, 2025
View reviewed changes
Copy link
Contributor

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

In case when no functions are invoked, we don't display the table. Maybe we should inform a user by displaying info/warning message?

@ddoktorski ddoktorski force-pushed the share-contracts-data-store branch from 1c1e984 to b5ef498 Compare October 24, 2025 12:01
@ddoktorski ddoktorski force-pushed the share-contracts-data-store branch from b5ef498 to 6bd6031 Compare October 28, 2025 09:58
@ddoktorski ddoktorski marked this pull request as ready for review October 28, 2025 10:30
@ddoktorski ddoktorski mentioned this pull request Oct 28, 2025
Open
@ddoktorski
Copy link
Contributor Author

In case when no functions are invoked, we don't display the table. Maybe we should inform a user by displaying info/warning message?

We could add a note stating that a gas report hasn't been produced because there were no contract calls, but I'm not sure if that's necessary. Any thoughts @cptartur?

@ddoktorski ddoktorski mentioned this pull request Oct 28, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MKowalski8 MKowalski8 Awaiting requested review from MKowalski8 MKowalski8 is a code owner automatically assigned from foundry-rs/starknet-foundry

@cptartur cptartur Awaiting requested review from cptartur

@franciszekjob franciszekjob Awaiting requested review from franciszekjob

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ddoktorski @cptartur @franciszekjob