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

fix(trace): properly report fallback() fn #9287

Merged
merged 5 commits into from
Nov 10, 2024
Merged

fix(trace): properly report fallback() fn #9287

merged 5 commits into from
Nov 10, 2024

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Nov 8, 2024 •
edited
Loading

Motivation

closes #9115

Solution

  • when decoding functions for contracts with fallback make sure the decoded function is in contract sigs, otherwise use fallback for trace

@grandizzy grandizzy marked this pull request as ready for review November 8, 2024 12:53
zerosnacks
zerosnacks reviewed Nov 8, 2024
View reviewed changes
Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

Nice! Small note in regards to test case completeness w/ JSON

@sakulstra would you mind double checking the proposed output format?

crates/forge/tests/cli/cmd.rs Outdated Show resolved Hide resolved
zerosnacks
zerosnacks approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

Lgtm, pending final check from @sakulstra

@sakulstra
Copy link
Contributor

looks fine to me, thanks a lot for looking into this!

@grandizzy grandizzy enabled auto-merge (squash) November 10, 2024 10:35
@grandizzy grandizzy disabled auto-merge November 10, 2024 10:44
@grandizzy grandizzy enabled auto-merge (squash) November 10, 2024 11:07
@grandizzy grandizzy changed the title fix(trace): check fn sigs for contract with fallbacks fix(trace): properly report fallback() fn Nov 10, 2024
@grandizzy grandizzy merged commit e028b92 into foundry-rs:master Nov 10, 2024
21 checks passed
@grandizzy grandizzy deleted the issue-9115 branch November 10, 2024 11:19
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
@grandizzy @rplusq
fix(trace): check fn sigs for contract with fallbacks (foundry-rs#9287)
f03746d 
* fix(trace): check fn sigs for contract with fallbacks

* Add Json test

* Execute test with traces

* Simplify, check only for decoded function
@grandizzy grandizzy added T-bug Type: bug C-cast Command: cast labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zerosnacks zerosnacks zerosnacks approved these changes

@yash-atreya yash-atreya yash-atreya approved these changes

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

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

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

Assignees
No one assigned
Labels
C-cast Command: cast T-bug Type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: --gas-report groups all methods of all contracts that use the same type of proxy contract
4 participants
@grandizzy @sakulstra @yash-atreya @zerosnacks