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): label addresses in traces #553

Closed
fubhy opened this issue Jan 23, 2022 · 8 comments · Fixed by #629
Closed

feat(forge): label addresses in traces #553

fubhy opened this issue Jan 23, 2022 · 8 comments · Fixed by #629
Labels
A-cheatcodes Area: cheatcodes A-tracing Area: tracing C-forge Command: forge Cmd-forge-test Command: forge test T-feature Type: feature

Comments

@fubhy
Copy link
Contributor

fubhy commented Jan 23, 2022

I believe it would further improve readability of the traces output (forge test -vvvv) if addresses were labeled with the name of the contract at the address or a custom label for EOAs. For EOAs (and also assigning custom labels for contracts e.g. to differentiate between different instances of the same contract), it might be cool to add a cheatcode to manually assign custom labels.

[PASS] testHappyPath() (gas: 12520)
Traces:
  [25936] ClaimOwnershipTest::testHappyPath()
    ├─ [24082] Dispatcher::setNominatedOwner(Alice [0x185a4dc360ce69bdccee33b3784b0282f7961aea])
    │   ├─ emit NominatedOwnerSet(nominatedOwner: Alice [0x185a4dc360ce69bdccee33b3784b0282f7961aea])
    │   └─ ← ()
    ├─ [0] VM::expectEmit(true, true, true, true)
    │   └─ ← ()
    ├─ emit OwnershipTransferred(prevOwner: Bob [0xb4c79dab8f259c7aee6e5b2aa729821864227e84], nextOwner: Alice [0x185a4dc360ce69bdccee33b3784b0282f7961aea])
    ├─ [0] VM::prank(Alice [0x185a4dc360ce69bdccee33b3784b0282f7961aea])
    │   └─ ← ()
    ├─ [1890] Dispatcher::claimOwnership()
    │   ├─ emit OwnershipTransferred(prevOwner: Bob [0xb4c79dab8f259c7aee6e5b2aa729821864227e84], nextOwner: Alice [ 0x185a4dc360ce69bdccee33b3784b0282f7961aea])
    │   └─ ← ()
    ├─ [448] Dispatcher::getOwner()
    │   └─ ← Alice [0x185a4dc360ce69bdccee33b3784b0282f7961aea]
    ├─ [405] Dispatcher::getNominatedOwner()
    │   └─ ← 0x0000000000000000000000000000000000000000
    └─ ← ()

E.g.

vm.setLabel(address(alice), "Alice")
vm.setLabel(address(bob), "Bob")

For fork tests, it might also be nice to try and do a reverse ENS lookup?

@onbjerg onbjerg added T-feature Type: feature C-forge Command: forge Cmd-forge-test Command: forge test A-tracing Area: tracing labels Jan 23, 2022
@brockelmore
Copy link
Member

I am cautious to throw "unneeded" http calls to maintain speed. That being said, I'm all for labels, that's a great idea. Should be easy to implement - adding a tag field to a CallTraceNode, then in the pretty_print or update_identified function, build a mapping and populate all nodes tag field that have the tagged address. Should the tag take precedence over contract name, I assume?

@brockelmore brockelmore changed the title Label addresses in traces feat(forge): label addresses in traces Jan 23, 2022
@mds1
Copy link
Collaborator

mds1 commented Jan 23, 2022

Should the tag take precedence over contract name, I assume?

Love the idea, and agree tag should take precedence over contract name

Also relevant is that TrueBlocks has a large address book of mainnet addresses we could pull from, more info here: gakonst/ethers-rs#769 (comment). Could either include a copy with the binary, or it's just one extra HTTP call for that full list

@brockelmore
Copy link
Member

Oh interesting - should tagging be cached to disk or only be dynamic? I can see cases for both

@mds1
Copy link
Collaborator

mds1 commented Jan 24, 2022

Hmm, my current thinking is that vm.label(address,string) should be dynamic and only persist during that test run, but something like the TrueBlocks database could be cached and re-fetched for updates every few weeks (or allow updates on demand)

@fubhy
Copy link
Contributor Author

fubhy commented Jan 24, 2022

Good point about the test performance. What about an optional test flag that pulls in these addresses when set and otherwise uses them from a cached database, possibly a plain file similar to .gas-snapshot? It could then even use TrueBlock as a fallback for ENS (or vica versa). Agreed about the persistence though:

Ordered with increasing priority (higher overrides lower):

  1. Local contract names: Scoped to the test run [not cached]
  2. Address label registry: Global scope, cached (.address-labels?). Can be populated from ENS / TrueBlocks [optionally]
  3. Manually set labels: Scoped to test run [not cached]

It should be possible to manually add custom records to .address-labels too imho for things that are recorded on neither ENS / TrueBlocks but are repeated throughout your [fork] tests.

Might be a good idea to start with just a manually maintained registry and consider TrueBlocks, etc. separately. We could provide an option to extract all unlabeled addresses at the end of a test run into the .address-labels file to allow the user to then manually refine that list with labels?

@gakonst
Copy link
Member

gakonst commented Jan 24, 2022

Supportive. I'd recommend we go the MVP path here and add the cheatcode for labelling, and if manual labelling becomes too much of an issue we start adding more configs on forge. We could also have a bunch of pre-configured labels in forge-std that one could import in their tests instead.

@fubhy
Copy link
Contributor Author

fubhy commented Jan 24, 2022

Supportive. I'd recommend we go the MVP path here and add the cheatcode for labelling ...

Agreed. I assume by that you also mean automatically derived labels for instances of local contracts in addition to the cheatcode, right?

@gakonst
Copy link
Member

gakonst commented Jan 24, 2022

If automatic derivation is easy, then yeah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes A-tracing Area: tracing C-forge Command: forge Cmd-forge-test Command: forge test T-feature Type: feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants