Skip to content
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 deploycontracts flag #1235

Merged
merged 5 commits into from
Apr 25, 2023
Merged

Add deploycontracts flag #1235

merged 5 commits into from
Apr 25, 2023

Conversation

lalexgap
Copy link
Contributor

Adds a deploycontracts flag to the RPC server that determines whether the RPC deploys the Create2Deployer and Nitro Adjudicator. This allows the RPC server to work against any chain JSON-RPC node instead of relying on hardhat docker.

@netlify
Copy link

netlify bot commented Apr 25, 2023

Deploy Preview for nitrodocs canceled.

Name Link
🔨 Latest commit fe77830
🔍 Latest deploy log https://app.netlify.com/sites/nitrodocs/deploys/64484828c22cf60008764d0d

@lalexgap lalexgap marked this pull request as ready for review April 25, 2023 04:10
Copy link
Contributor

@geoknee geoknee left a comment

Choose a reason for hiding this comment

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

LGTM.

I'm wondering if we can push some of this code down into the chain service, to keep main.go nice and lean?

@@ -60,7 +60,7 @@ const config: HardhatUserConfig & {watcher: any} = {
},
networks: {
hardhat: {
chainId: 31337,
chainId: 1337,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we require / assume this chainId in go-nitro somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, most of our tests and the RPC server use a chainid of 1337. The RPC server should probably accept a chain id instead of using a hardcoded one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to accept a chain argument in fe77830. If none is provided we use 1337

@geoknee
Copy link
Contributor

geoknee commented Apr 25, 2023

What happens if multiple nodes try to all deploy the adjudicator contract? The check for nonzero bytecode will fail if another node has submitted a tx to deploy the adjudicator, but that tx has not yet been mined.

I haven't yet found a definitive source, but it appears that a second deploy tx will not revert ethereum/go-ethereum#17881 . I'm not sure if we want to change any of our code to exploit that...probably not.

@lalexgap
Copy link
Contributor Author

LGTM.

I'm wondering if we can push some of this code down into the chain service, to keep main.go nice and lean?

Good idea, done in bb3962e

@lalexgap lalexgap merged commit f528776 into main Apr 25, 2023
@geoknee geoknee deleted the auto-deploy-create2 branch April 26, 2023 08:47
NiloCK pushed a commit that referenced this pull request Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants