Skip to content
This repository has been archived by the owner on Jan 31, 2025. It is now read-only.

feat: add DeployProtocol script #91

Merged
merged 5 commits into from
Jun 7, 2023
Merged

Conversation

andreivladbrg
Copy link
Member

Addresses #76

@andreivladbrg andreivladbrg requested a review from PaulRBerg June 4, 2023 12:58
build: bump v2-core
@andreivladbrg andreivladbrg force-pushed the feat/deploy-protocol-script branch from b4aed49 to b117cd1 Compare June 4, 2023 12:59
Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM.

Don't we also need a DeployDeterministicProtocol script which deploys V2 Core from the precompiles?

.github/workflows/deploy-protocol.yml Outdated Show resolved Hide resolved
.github/workflows/deploy-protocol.yml Outdated Show resolved Hide resolved
script/DeployProtocol.s.sol Outdated Show resolved Hide resolved
script/DeployProtocol.s.sol Show resolved Hide resolved
script/DeployProtocol.s.sol Outdated Show resolved Hide resolved
script/DeployProtocol.s.sol Outdated Show resolved Hide resolved
script/DeployProtocol.s.sol Outdated Show resolved Hide resolved
script/DeployProtocol.s.sol Outdated Show resolved Hide resolved
script/DeployProtocol.s.sol Show resolved Hide resolved
@andreivladbrg
Copy link
Member Author

andreivladbrg commented Jun 6, 2023

Don't we also need a DeployDeterministicProtocol script which deploys V2 Core from the precompiles?

I don't think so. This script is not for testing purpose.

As I remember on call we had, you mentioned that we will not use the deterministic deployments anymore, should we remove them?

Addressed the suggestions made, is it good to be merged?

@PaulRBerg
Copy link
Member

This script is not for testing purpose.

Hmmmmmmm ... I'm thinking that we might have to move the Precompiles under script in V2 Core.

you mentioned that we will not use the deterministic deployments anymore, should we remove them?

Yes, but it doesn't follow that it wouldn't be useful to have the deterministic deployment scripts - just in case.

Also, in the meantime, I have added a create2Salt as a parameter in the run function, which guarantees that the addresses will be different. Whereas the normal deployment scripts may still lead to the same addresses when the deployer's nonces are identical across chains.

is it good to be merged?

Yes. We can consider how (and if) we implement the DeployDeterministicProtocol later.

@PaulRBerg PaulRBerg merged commit 75fc637 into main Jun 7, 2023
@PaulRBerg PaulRBerg deleted the feat/deploy-protocol-script branch June 7, 2023 10:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants