feat(forge): add `forge selectors collision` cmd #5116

lmanini · 2023-06-07T21:00:31Z

Motivation

Follow up PR for #5012.

Solution

Adds a subcommand to forge selectors to check for methods with colliding selectors between two contracts.
The output format is similar to that of forge inspect <contract> mi but it could still be improved if you guys dislike the current format.

To test this I've created a forge repo in the project, added Proxy.sol and Token.sol : gist.
Running forge selectors collision Token Proxy prints:

"42966c68": ("burn(uint256)", "collate_propagate_storage(bytes16)")
"9ce110d7": ("initializer()", "initializer()")
"06fdde03": ("name()", "name()")
"95d89b41": ("symbol()", "symbol()")
Error: 
At least one collision was found

and running forge selectors collision Proxy Counter prints:

No colliding method selectors between the two contracts.

Evalir

This overall looks good! Just a few comments, mainly bikeshedding on the way we handle the output:

Evalir · 2023-06-08T14:41:21Z

cli/src/cmd/forge/selectors.rs

+                let second_method_map = second_artifact.method_identifiers.as_ref().unwrap();
+
+                let mut colliding_methods = Vec::new();
+                for (k1, v1) in first_method_map {


this is ok, but maybe we could do this in a more rusty/functional way? something like filtering whichever method map for items contained in the second and collecting this into another vector.

sounds good! will rewrite this with a functional construct 👍

Evalir · 2023-06-08T14:52:39Z

cli/src/cmd/forge/selectors.rs

+                    colliding_methods.iter().for_each(|t| {
+                        println!("\"{}\": {:?}", first_method_map.get(t.0).unwrap(), t)
+                    });
+                    return Err(eyre::eyre!("At least one collision was found"))
+                }


Hmm, I was thinking, maybe this command doesn't need to error on here? We can color the output with Paint::red for saying we found collisions, and then maybe pretty print on a table instead. wdyt @mds1 / @mattsse ?

I think a common use of this command is to run it in CI to make sure you don't merge anything with selector collisions, so my suggestion would be to either:

Have this command always exit with an error (i.e. std::process::exit(1)) if a collision is found, OR

Only exit with an error if a --check flag is added

Currently we do both depending on the command,e.g. forge build --sizes exits with an error if anything is over the size limit, but forge fmt needs a --check flag. I can see an argument that the former should also have a --check flag, so I'm indifferent here and maybe we start requiring the --check flag as a standard for commands like this where it makes sense?

Also, I'm now realizing this may all be tangential to returning an Err from this function 😅

I'm more lenient towards adding the --check flag so that we can support both of your guys' visions.
Regarding the pretty print on a table, yesterday I was looking at prettytable-rs but idk what you guys think about adding it as a dep.

Other parts of the code use comfy-table to pretty-print stuff, that's what forge build --sizes uses, I think forge inspect too

Evalir · 2023-06-08T14:53:55Z

cli/src/cmd/forge/selectors.rs

+
+        /// Support build args
+        #[clap(flatten)]
+        build: Box<CoreBuildArgs>,


is this box necessary?

Rust won't complain without it, but clippy does as without the two elements of the SelectorsSubcommands enum differ quite a bit in size

lmanini · 2023-06-09T16:45:16Z

@Evalir I refactored the colliding_methods vector population and improved the output formatting - standing by for comments and tips on how to move regarding throwing an error or not on finding a collision :)

This is what the table looks like when a collision is found:

+----------+---------------+------------------------------------+
| Selector | Token         | Proxy                              |
+===============================================================+
| 42966c68 | burn(uint256) | collate_propagate_storage(bytes16) |
|----------+---------------+------------------------------------|
| 9ce110d7 | initializer() | initializer()                      |
|----------+---------------+------------------------------------|
| 06fdde03 | name()        | name()                             |
|----------+---------------+------------------------------------|
| 95d89b41 | symbol()      | symbol()                           |
+----------+---------------+------------------------------------+
Error: 
At least one collision was found

Evalir

this lgtm pending the comment! honestly if we find collisions i would just not return the Err, just print the table along with a "Collisions found" on top

Evalir · 2023-06-17T00:10:22Z

cli/src/cmd/forge/selectors.rs

+                    });
+                    println!("{table}");
+
+                    return Err(eyre::eyre!("At least one collision was found"))


Ok so what I'd do for this command is just not return an error at all—printing the table if we find collisions preceded by x collisions found is good enough for me. I'd only return an error if the command failed internally, as in building the project or trying to extract the selectors

…printing table

Evalir

aight! just pushed the last change—looks good! cc @mattsse for final approval/merge

mattsse

nice job

lgtm

lmanini added 6 commits May 31, 2023 17:13

added forge selectors collision cmd

c6b5af6

Merge branch 'master' into lmanini/selectors-collision

b0f6dcf

raises error

f04812a

Merge branch 'master' into lmanini/selectors-collision

0b95107

ran fmt + clippy

0af8ec4

improved output format

9498638

Evalir requested changes Jun 8, 2023

View reviewed changes

lmanini added 2 commits June 9, 2023 18:31

refactored vec population

fabad89

improved output formatting

c8ea93b

better method sig handling

bc2f886

Evalir requested changes Jun 17, 2023

View reviewed changes

chore: print collisions found before table instead of erroring after …

d63c36c

…printing table

Evalir approved these changes Jun 17, 2023

View reviewed changes

Evalir requested a review from mattsse June 17, 2023 02:10

mattsse approved these changes Jun 17, 2023

View reviewed changes

mattsse merged commit cf104a5 into foundry-rs:master Jun 17, 2023

lmanini deleted the lmanini/selectors-collision branch June 29, 2023 17:03

zerosnacks mentioned this pull request Jul 2, 2024

Forge: forge selectors feature request meta issue #5012

Closed

feat(forge): add forge selectors collision cmd #5116

feat(forge): add forge selectors collision cmd #5116

Uh oh!

Conversation

lmanini commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Solution

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mds1 Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmanini commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat(forge): add `forge selectors collision` cmd #5116

feat(forge): add `forge selectors collision` cmd #5116

lmanini commented Jun 7, 2023 •

edited

Loading

mds1 Jun 8, 2023 •

edited

Loading

lmanini commented Jun 9, 2023 •

edited

Loading