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(cast/forge): add label addresses in foundry config #6680

Merged

Conversation

cool-mestorf
Copy link
Contributor

Motivation

Inspecting & testing transactions that require multiple address labels is quite painful at this point because it makes the cast command or test script unnecessarily verbose.

$ cast run <tx> \
  --label 0x1F98431c8aD98523631AE4a59f267346ea31F984:"Uniswap V3: Factory" \
  --label 0xC36442b4a4522E871399CD717aBDD847Ab11FE88:"Uniswap V3: Positions NFT" # ...
function setUp() public {
    vm.label(address(0x1F98431c8aD98523631AE4a59f267346ea31F984), "Uniswap V3: Factory");
    vm.label(address(0xC36442b4a4522E871399CD717aBDD847Ab11FE88), "Uniswap V3: Positions NFT");
    // ...
}

Solution

Add labels section in foundry config that allows to pre-label frequently used addresses. Most evm addresses are used as the same purpose across multiple chains, so the config is considered standalone section. The config is then merged with given cli argument in cast run / given cheatcode labeled addresses in forge test and the merged labels collection is used to label the final trace.

# ...
[labels]
0x1F98431c8aD98523631AE4a59f267346ea31F984 = "Uniswap V3: Factory"
0xC36442b4a4522E871399CD717aBDD847Ab11FE88 = "Uniswap V3: Positions NFT"

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.

supportive. nits:

Comment on lines 229 to 233
pub fn get_labels(&self) -> HashMap<Address, String> {
let mut labels = self.labels.clone();
labels.extend(self.config.labels.clone().into_iter());
labels
}
Copy link
Member

Choose a reason for hiding this comment

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

any chance we could just return a reference here so we don't have to copy on every access? this can get expensive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think much better way to solve this is passing a label getter closure or object to its usages such as Error::encode(err.to_error_msg(...)) or InspectorData::labels because most of the time we don't need the entire hashmap of labels... but I will try to fix this to return a reference for this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming config does not change over program lifecycle, just copied labels from config on cheatcodes creation and used it as single source of truth. 4621127

jail.create_file(
"foundry.toml",
r#"
[labels]
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the labels in config be chain specific though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that most address labels for evm chains can be safely reused because contract addresses created using CREATE2 and externally owned accounts created from same private key or seed phrase share the same address over all chains. Tagging Uniswap V3: Factory over all chains would be cumbersome. I think it would be much better if there is chain-specific label section and globally shared label section to keep chain-specific labels from universally used addresses, but I just kept this PR's scope into global labels only.

@cool-mestorf
Copy link
Contributor Author

clippy is failing only on CI and it is also failing in master branch. Is there anything I can do to solve this?

@DaniPopes
Copy link
Member

Nope, it's a bug in Clippy

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.

I'm personally okay with the labels being global by default for now, and we can improve on this later by having a per-chain config. Wondering what you think @mds1 @mattsse

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.

seems reasonable.

having them globally not chain specific should be good enough, we could add chain info if this turns out to be an issue

pending @mds1

@Evalir Evalir merged commit 4d033da into foundry-rs:master Jan 4, 2024
18 of 19 checks passed
@cool-mestorf cool-mestorf deleted the feat/default-labels-for-addresses branch January 5, 2024 01:28
RPate97 pushed a commit to RPate97/foundry that referenced this pull request Jan 12, 2024
)

* add label section in config

* use labeled addresses in `cast run`

* use labeled addresses in `forge test` trace

* use `labels` field as single source of state

* fix test build

---------

Co-authored-by: George Dent <georgedent118@gmail.com>
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.

6 participants