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.

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

feat(forge): add vm.lastCallGas cheatcode #7573

Merged
merged 35 commits into from
Apr 8, 2024
Merged

feat(forge): add vm.lastCallGas cheatcode #7573

merged 35 commits into from
Apr 8, 2024

Conversation

zerosnacks
Copy link
Member

@zerosnacks zerosnacks commented Apr 5, 2024 •
edited
Loading

Motivation

Closes #7272

Solution

  • Add the lastCallGas cheatcode, primarily to be used in isolation mode for accurate measurements.
  • Adds exclude_contracts to test filter
  • Returns a Gas struct instead of proposed uint256, with details like gas spend on memory and gas refunds

@zerosnacks
add `gasUsed and lastGasUsed methods to Vm
a737fb9
@zerosnacks
reorder
92d0f82
@zerosnacks
basic sketching of idea, planning to use Gas struct to deliver as muc…
b9a0ed7 
…h gas related information rather than a uint256
@zerosnacks
add transformation
2f72c83
@zerosnacks
to prevent recording gas by default, only enable after recordGas is e…
3cd8f13 
…nabled
@zerosnacks
update struct layout, implementation builds, now connecting to cheatc…
cc485cf 
…odes
@zerosnacks
fix cheatcodes
06f38f5
@zerosnacks
refactor to use simple u64 as Gas struct doesnt have a default
208c1e7
@zerosnacks
change from Gas struct to simple u64 as I ran into issues with cache …
cb9d668 
…being reset to 0
@zerosnacks
it appears cheatcodes are resolved before the actual function calls a…
98b9975 
…re therefore it doesnt actually remember the correct value from the previous execution but only of the previous executed cheatcode
@zerosnacks
still not working
4cc57d2
@zerosnacks
finally works, stupid me didnt realize i had to cross call frames
d68a0de
@zerosnacks
emit gas record
c63b29e
@zerosnacks
test convenient single field access
7ebbcd0
@zerosnacks
add gas record struct back
b846537
@zerosnacks
pass down isolate bool, only enable gas tracing if enabled
b2b1c4d
@zerosnacks
raise error if cheatcode is used outside of isolation mode
1736f2b
@zerosnacks
mark as view
61644f9
@zerosnacks
show gas refund and memory expansion
eea3102
@zerosnacks
improve example
179e450
@zerosnacks
add isolation test, currently does not run as expected
49d33c8
@zerosnacks
fix fmt
05a7a63
mattsse
mattsse reviewed Apr 5, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this looks clean, only have one comment re name,

wdyt @klkvr ?

crates/cheatcodes/spec/src/vm.rs Outdated Show resolved Hide resolved
@mds1
Copy link
Collaborator

mds1 commented Apr 5, 2024

Before merge let's make sure this feature is actually the best solution to the problem, ref #7272 (comment)

zerosnacks
zerosnacks commented Apr 5, 2024
View reviewed changes
crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved
crates/forge/tests/it/isolate.rs Outdated Show resolved Hide resolved
testdata/default/isolation/RecordGas.t.sol Outdated Show resolved Hide resolved
klkvr
klkvr reviewed Apr 5, 2024
View reviewed changes
crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
@klkvr
Copy link
Member

klkvr commented Apr 5, 2024 •
edited
Loading

Can't we make this work for non-isolated tests as well?

@zerosnacks zerosnacks changed the title feat(forge): add vm.lastGasUsed cheatcode feat(forge): add vm.lastCallGas cheatcode Apr 8, 2024
Evalir
Evalir reviewed Apr 8, 2024
View reviewed changes
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

excited to see this, i think it's going to be pretty useful. nits:

crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
crates/forge/tests/it/test_helpers.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
@zerosnacks zerosnacks marked this pull request as ready for review April 8, 2024 11:27
klkvr
klkvr requested changes Apr 8, 2024
View reviewed changes
Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

nice work! lgtm overall, would like to improve tests for this a bit

testdata/default/cheats/LastCallGas.t.sol Outdated Show resolved Hide resolved
@zerosnacks
remove isolate standalone, create additional entry in cheats and docu…
86ca22d 
…ment subset of cheats that require to be tested in isolation mode as well
@zerosnacks
improve tests, use asserts and add option to exclude contracts from t…
999faec 
…est filter, not just individual tests or paths
klkvr
klkvr approved these changes Apr 8, 2024
View reviewed changes
Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

lgtm

Evalir
Evalir approved these changes Apr 8, 2024
View reviewed changes
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

tests look great, lgtm

@zerosnacks zerosnacks merged commit b88d167 into master Apr 8, 2024
19 checks passed
@zerosnacks zerosnacks deleted the zerosnacks/add-vm-lastGasUsed branch April 8, 2024 15:51
This was referenced Apr 26, 2024
Merged
Merged
@Karrq Karrq mentioned this pull request Jul 11, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klkvr klkvr klkvr approved these changes

@Evalir Evalir Evalir approved these changes

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a lastGasUsed() function in Vm.sol
5 participants
@zerosnacks @mds1 @klkvr @mattsse @Evalir