fix(forge): Optionally use create2 factory in tests and non-broadcasting scripts #6656
Conversation
RPate97
changed the title
|
Thanks for opening this! I agree this needs to be introduced in a way that avoids breaking changes, since it's likely users have assertions with hardcoded expected create2 addresses. What do you think about doing this as a config option / CLI flag instead? My rationale is that users are unlikely to only need this feature in a subset of tests, making a global flag simpler than a cheatcode, since a cheat might need to be declared in multiple places. And declaring a cheat in multiple places (since not all tests share the same setup) makes it more error prone / hard to be consistent everywhere. And we should remove this in v1 and make this the default for simplicity, cc @Evalir to document this wherever v1 breaking changes are being tracked Also cc @zobront who originally pointed out this issue |
I think that's a good idea. I've updated this PR. |
RPate97
force-pushed
the
pate/issue-5529
branch
3 times, most recently
from
478e31b
to
ac45ba5
Compare
crates/cheatcodes/src/inspector.rs
Outdated
|
|
||
| fn process_create<DB: DatabaseExt>( |
There was a problem hiding this comment.
Could we add some docs on what the two functions do now? as we have apply_create2_deployer and process_create, and previously process_create was fallible, but now it will always succeed and update state in case of scripts.
There was a problem hiding this comment.
@Evalir Just bumping since this is ready for review. Thanks.
RPate97
force-pushed
the
pate/issue-5529
branch
2 times, most recently
from
1c92c86
to
7b5da01
Compare
RPate97
force-pushed
the
pate/issue-5529
branch
from
7b5da01
to
6ad0f7d
Compare
RPate97
force-pushed
the
pate/issue-5529
branch
from
6ad0f7d
to
78130a2
Compare
RPate97
force-pushed
the
pate/issue-5529
branch
from
78130a2
to
c2c2515
Compare
|
Hey @Evalir, this continues to be an issue for us. The PR has been ready for a month, so I just wanted to bump it. Totally understand you've got a lot on your plate, but I just want to make sure that this doesn't get buried into oblivion :) |
|
Understood @sam-goldman — let's fix this :) |
| @@ -1241,6 +1235,14 @@ impl<DB: DatabaseExt> Inspector<DB> for Cheatcodes { | |||
| } | |||
| } | |||
|
|
|||
| // Apply the Create2 deployer | |||
| if self.broadcast.is_some() || self.config.always_use_create_2_factory { | |||
There was a problem hiding this comment.
the title says "non broadcasting scripts", but this applies the create2 deployer on broadcasting scripts only?
There was a problem hiding this comment.
This is intended to cause the create2 deployer to be applied in a broadcasting script, as is the current behavior. If the always_use_create_2_factory is true, then the create2 deployer should also be used in tests and non-broadcasting scripts. This should be the default behavior in my opinion, but using an option ensures this change is backward compatible.
I believe this PR correctly implements that behavior, but I am new to Rust, so let me know if you think I'm misunderstanding something about the implementation.
There was a problem hiding this comment.
as @mds1 said, I think this makes sense and this looks like an appropriate solution imo
any concerns @DaniPopes
I only have nits
sorry for the delayed review
RPate97
force-pushed
the
pate/issue-5529
branch
from
03f1f8a
to
e28aa9f
Compare
Motivation
#5529
Solution
Resolves #5529 by adding logic to allow the default create2 factory to be used in tests and non-broadcasting scripts. I chose to make this change non-breaking by introducing a new config option / CLI flag
always_use_create_2_factory, which must be set totrueto enable this behavior.In the future, this option could be removed or changed relatively easily to make using the create2 factory the default behavior if a breaking change is acceptable.