-
Notifications
You must be signed in to change notification settings - Fork 156
Deterministic deployment of contracts using Create2 #114
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
base: develop
Are you sure you want to change the base?
Conversation
// Upgrade Executor | ||
// (Deployed using ABI and bytecode from @offchainlabs/upgrade-executor) | ||
// | ||
const upgradeExecutorTemplate = await deployContract( |
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.
Note that in the original script we used the UpgradeExecutor abi and bytecode from the UpgradeExecutor repo instead of the factory from the sdk (source)
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 we should keep that behavior, i left a comment about it
Signer, | ||
Wallet, | ||
} from 'ethers' | ||
import { Interface, hexZeroPad, defaultAbiCoder } from 'ethers/lib/utils' |
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.
import { Interface, hexZeroPad, defaultAbiCoder } from 'ethers/lib/utils' | |
import { Interface, hexZeroPad } from 'ethers/lib/utils' |
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.
There are some unused imports here, as well as some commented bits, that are used in #115 .
And I just realized that while separating the PRs, this one was left a bit messy, apologies for that 🙏
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.
Happy to clean this up, and move whatever is needed to the other PR if needed.
// Parent chain helper contracts | ||
// | ||
// Multicall2 | ||
const parentMulticall2 = await deployContract( |
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.
probably a nit but i think it'd be easier to review the diff if we left the variable / function names the same and left the deployments in the same order
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.
Yeah, I see what you mean. We decided to start changing the terminology used from L1/L2 to Parent/Child, and I tried to keep the changes only to the function that I was working on, but seeing now the diff, I think this PR could have been split into 2: add create2, and changes in terminology.
Co-authored-by: Henry <[email protected]>
Co-authored-by: Henry <[email protected]>
This PR adds the possibility of deploying the TokenBridgeCreator contract along with its templates to deterministic addresses using Create2.
Verification of contracts is done in #115 , so that PR should be merged before this one.