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

feat(forge): add labels for precompile addresses #7544

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Apr 2, 2024

Motivation

Closes #7530

Not all precompiles are labeled, this PR add missing labels

Solution

  • added precompile addresses in constants
  • in precompile decode match on address
  • add trace labels to all precompile addresses

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Precompile calls should already be labeled with PRECOMPILES::name(...) already. I don't really know about this.

pub const BLAKE_2F_ADDRESS: Address = address!("0000000000000000000000000000000000000009");

/// The PointEvaluation precompile address.
pub const POINT_EVALUATION_ADDRESS: Address = address!("000000000000000000000000000000000000000a");
Copy link
Member

Choose a reason for hiding this comment

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

Please move these into a nested module named precompiles and drop the ADDRESS suffix.

let data = &trace.data;

let (signature, args) = match x {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the tuple and remove the decoded_call macro, it's worse than before

@grandizzy
Copy link
Collaborator Author

Precompile calls should already be labeled with PRECOMPILES::name(...) already. I don't really know about this.

That's right, thing is that labels for precompiles are not displayed along all traces but only when precompile is decoded, so for example in #7530 for following test setup

        address target = address(1);
        batchInbox.deposit{value: 1 ether}(target);
        assertEq(batchInbox.balances(target), 1 ether);

traces are shown as

    ├─ [22647] BatchInbox::deposit{value: 1000000000000000000}(0x0000000000000000000000000000000000000001)
    │   └─ ← [Stop] 
    ├─ [519] BatchInbox::balances(0x0000000000000000000000000000000000000001) [staticcall]
    │   └─ ← [Return] 1000000000000000000 [1e18]

as with current PR the ECRecover label is attached making clear that's a precompile:

    ├─ [22647] BatchInbox::deposit{value: 1000000000000000000}(ECRecover: [0x0000000000000000000000000000000000000001])
    │   └─ ← [Stop] 
    ├─ [519] BatchInbox::balances(ECRecover: [0x0000000000000000000000000000000000000001]) [staticcall]
    │   └─ ← [Return] 1000000000000000000 [1e18]

IMO would be a nice addition but nothing critical so we could drop this one if you think it doesn't worth @DaniPopes

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.

pending @DaniPopes nits then this is g2m

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.

Automatically label known contract addresses in traces
3 participants