-
Notifications
You must be signed in to change notification settings - Fork 37
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
Support create2 #626
Support create2 #626
Conversation
082b7c8
to
a737316
Compare
Good catch! We should update that example everywhere so that the owner is received.
Let's leave this out of scope for now. We'll have to define something eventually, but not needed yet.
I think everywhere where it says "Deploying [ module]" we should add the strategy
I think that to get this out quickly we can deploy the factory outside of ignition core for development networks and wait for that tx to confirm before calling ignition. We should have a way for the strategy to be able to drive its initialization, but I think it'd be better to have this out sooner.
All of them should be in the same address, so it's a matter of doing an
I'll ask pascal about this
Same |
This is trickier than what I had in mind actually. Because we don't have a mechanism to pass "strategy config" to core. Maybe we can make the usage of crate x transparent, and let users handle the nonce. This would mean that the strategy gets to handle some sort of config. One potential way to implement this can be by accepting a strategy object in ignition core. |
6707ff9
to
f729a3c
Compare
executionState: DeploymentExecutionState | ||
): DeploymentStrategyGenerator { | ||
const artifact = await this._loadArtifact(executionState.artifactId); | ||
const salt = ethers.id("test"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this intentionally left as test
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It hasn't been intentionally left as test
. We need to review how we approach setting a salt and make sure we are happy with it.
I do want to point out that createX is explicitly not audited. |
A POC create2 strategy based on `createX`.
This might be voodoo, but I preferred the check against a number rather than a boolean in the previous validation, so brought into the later test.
This is a refactor to avoid having to change lots of tests when you tweak a single contract. I updated the contract loader as well in passing in some of the older tests.
I initially just changed the test, but then realised that the Foo contract changes across projects. Instead we now pull in the artifact so the same bytecode is used across the create2 tests.
We are going to limit the types exposed by the two exported strategies. Our intent to expand release full Strategy support. In the meantime we are limiting the types exposed. A hack has been put in place that exports reduced versions of the BasicStrategy and the Create2Strategy, but then sneakly replaces the implementation with alternate classes. There are simpler hacks, but this one works with API Extractor.
We should assume that strategies are being newed up programmatically in scripts. Hence the constructor should be simple - only the config. We will inject other key services via init.
If the user does not provide a salt we throw. We do not want the unexpected behaviour of a fallback default value.
We want the strategy to initialize only if execution is about to commence, so after all pre-execution validations and checks.
These can likely be cleaner, but they are internal and will need to change for the Strategy API in the future anyway.
Number and string cover our current use case so lets narrow and expand later when the Strategy API lands.
This is internal and is recorded down to the journal and used in reconciliation.
We switch to a pattern of passing both config and strategy name into core. The intention is to include strategy initializers in core's `deploy` signature. This will allow us to support command line usage, while also providing programmatic support.
We reconcile all futures. But as strategy is set per run, it does not make sense to reconcile already complete futures based on strategy name or strategy config.
A journal from the previous version will have a strategy name but not a config. That means any deployment restarted with an in flight transaction will fail as the comparison would have been between undefined and the new basic strategy config `{}`. Instead, we now convert `undefined` for strategy config into `{}`. All future versions of Ignition will have that property defined. We know it is equivalent to `{}` as only the basic strategy was allowed originally.
We where using `ethers.id` to hash the salt and force it to be 32bytes long. We only need to ensure that it is 32bytes long. We now pass the salt through cleanly, but have a strategy config validation that enforces a 32 byte length.
Don't show the stacktrace, show a cleaner HH style error.
This is a proof of concept of how to support create2 in Ignition.
It adds a new execution strategy that alters the way contract deployment futures are handled. The contract is proxied through a create2 factory rather than becoming a deployment transaction.
This PR uses createX as the create2 factory. If deploying to
sepolia
a known address is used. If deploying ephemerally, the create2 factory is deployed as part of a newinit
step in the execution strategy, then leveraged when processing futures.Resolves #629
Testing Locally
To deploy a module using
create2
cd examples/ts-sample npx hardhat ignition deploy ./ignition/modules/LockModule.ts --strategy create2
A createX factory will be deployed and the Lock contract (with passed value) will be created via the factory (note this affects the set owner!).
Future Considerations
{ basic, create2 }
create2
is being used?create2
(same salt + bytecode)?init
functions on contracts forcreate2
Testing Plan
This PR and the strategies config PR include both unit and integration tests. A manual round of tests will include:
create2
--verify
enabled./example
projects with create2 enabled