-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Code Jar Factory #138
Add Code Jar Factory #138
Conversation
This patch improves Code Jar to use CodeJarStub, which is a stub that's semi-verifiable on Etherscan for deployments, and significantly reduces the complexity/noise of how CodeJar works. This is also the basis for CodeJar deployments, which will be included in a different branch.
This patch changes CodeJar to be deployed from a factory, which just... deploys it. But it uses `CREATE2` such that CodeJar is _guaranteed_ to be the same bytecode if it's the same deployment address. This removes the vast majority of "attack vectors" from depending on CodeJar to be at the same address on all chains.
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.
I increasingly feel as though we actually shouldn't be deploying anything from the QuarkWalletFactory
... well, except for wallet / proxy instances.
I was tempted in the QuarkWalletProxyFactory
to rewrite it with a constructor that takes as an argument walletImplementation
, and does not depend on CodeJar
or QuarkStateManager
at all... 🤔
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.
might be a fluke, but gas snapshot run is failing and not producing any diffs.
If I had may way, there'd be no constructors. Only immutables. FYI: with this, you can still do initiation. You just need to check for initialization first, so it's not even that bad for cases that realllllly need constructors. The only other thing is |
constructor() { | ||
codeJar = new CodeJar(); | ||
constructor(CodeJar codeJar_) { | ||
codeJar = codeJar_; | ||
stateManager = new QuarkStateManager(); |
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.
We might as well pass in the stateManager
too. Feels weird to pass in one in the constructor and not the other.
console.log("============================================================="); | ||
console.log("Deploying QuarkWalletFactory"); | ||
|
||
quarkWalletFactory = new QuarkWalletFactory(); | ||
quarkWalletFactory = new QuarkWalletFactory(codeJar); |
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.
The downside of this approach is we now require the first two txns to be submitted perfectly on each chain instead of just one txn.
Although it's neat that we can prove CodeJar
was deployed via a factory using CREATE2, this decision does slightly complicate our deployment process and increase the operational burden.
It almost feels like CodeJar
is just its own separate project at this point and should be deployed independently using a separate EOA.
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.
I think 🧑🍳 would prefer for codejar to be a (sub)project that is deployed independently.
I believe we can accomplish that without separating it out into its own repo, but we can also take the step of doing the necessary surgery to separate it entirely if we want.
I agree it increasingly feels most "correct" to not consider codejar itself a part of quark-core
, but a separate project upon which quark-core
depends.
0499de8
to
ca843bb
Compare
This patch changes CodeJar to be deployed from a factory, which just... deploys it. But it uses
CREATE2
such that CodeJar is guaranteed to be the same bytecode if it's the same deployment address. This removes the vast majority of "attack vectors" from depending on CodeJar to be at the same address on all chains.