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 support for deployment on Goerli testnet #114

Merged
merged 9 commits into from
Aug 3, 2022
Merged

Add support for deployment on Goerli testnet #114

merged 9 commits into from
Aug 3, 2022

Conversation

michalinacienciala
Copy link
Contributor

@michalinacienciala michalinacienciala commented Jul 6, 2022

Görli became a recommended test network after Ropsten's deprecation
notice (https://blog.ethereum.org/2022/06/21/testnet-deprecation/).
We're modifying GitHub Actions workflow for deploying contracts to
support the deployment on Görli. We're also leaving the possibility of
deployment on Ropsten (this will be removed once we have the Görli
deployment battle-tested and Ropsten gets shut down).
We also change the way we handle keep-core dependency - in V2 of our
system we'll no longer planning to publish new keep-core packages -
hence we hardcode used version of the package for given testnet.

TODO:

  • Configure GOERLI_ETH_HOSTNAME_HTTP , GOERLI_ETH_CONTRACT_OWNER_PRIVATE_KEY, GOERLI_KEEP_ETH_CONTRACT_OWNER_PRIVATE_KEY, TENDERLY_TOKEN GitHub secrets (@nkuba - could you do that? It would be also good to make similar ROPSTEN/GOERLI distinction in the secrets in the keep-network org)
  • Configure CI_GITHUB_TOKEN GitHub secret (@nkuba)
  • Verify the workflow once new V2-compatible config in keep-core/ci is ready
  • Remove testing configuration from the workflow

Refs:
keep-network/keep-core#3012
keep-network/keep-core#3081
keep-network/tbtc-v2#382

Base automatically changed from goerli-deployment to main July 7, 2022 13:14
Görli became a recommended test network after Ropsten's deprecation
notice (https://blog.ethereum.org/2022/06/21/testnet-deprecation/).
We're modifying GitHub Actions workflow for deploying contracts to
support the deployment on Görli. We're also leaving the possibility of
deployment on Ropsten (this will be removed once we have the Görli
deployment battle-tested and Ropsten gets shut down).
We also change the way we handle `keep-core` dependency - in V2 of our
system we'll no longer planning to publish ne `keep-core` packages -
hence we hardcode used version of the package for given testnet.

NOTE: We're temporarily using some testing configuration in the
workflow, which needs to be removed before merge to `main`.
As the Ropsten testnet becomes deprecated in the near future, we are
switching to deployment on Goerli. If deployment on Ropsten will be
needed in the interm period, we will do it manually, not via GH Actions.
@michalinacienciala
Copy link
Contributor Author

Workflow verified: https://github.com/threshold-network/solidity-contracts/runs/7322557630?check_suite_focus=true. Everything except triggering next CI build works as expected. There's a problem with CI_GITHUB_TOKEN value - looks that the secret contains wrong credentials. Will need @pdyraga / @nkuba help with setting that correctly.

Change is only temporary, it should be reverted before merge to `main`.
We are removing the temporary configuration used for workflow testing.
We are also bumping the version of used custom GH actions from `v1` to
`v2`, as `v2` uses the new order of modules executions.
@michalinacienciala
Copy link
Contributor Author

Everything except triggering next CI build works as expected. There's a problem with CI_GITHUB_TOKEN value - looks that the secret contains wrong credentials. Will need @pdyraga / @nkuba help with setting that correctly.

New value of CI_GITHUB_TOKEN has been set. Workflow has been verified again (in slightly shortened version, focusing on execution of Notify CI... step: https://github.com/threshold-network/solidity-contracts/runs/7341634837?check_suite_focus=true. Works as expected. PR ready for review.

run: |
yarn upgrade \
@keep-network/keep-core@${{ steps.upstream-builds-query.outputs.keep-core-contracts-version }}
run: yarn upgrade @keep-network/[email protected]
Copy link
Member

Choose a reason for hiding this comment

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

Could we get a package that is tagged with the environment name?

Suggested change
run: yarn upgrade @keep-network/keep-core@1.8.1-goerli.0
run: yarn upgrade @keep-network/keep-core@${{ github.event.inputs.environment }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can. Resolving keep-core dependency using goerli tag did not work for me in another project, but here apparently it works fine. Changed in c9f33ca.

We tag the latest `@keep-network/keep-core` package containing contracts
migrated on some environment with tag that is a name of that
environment. We can use that tag instead of the explicit version when
resolving contracts of dependant projects.
@@ -99,7 +99,7 @@ jobs:

contracts-deployment-testnet:
needs: [contracts-build-and-test]
if: github.event_name == 'workflow_dispatch'
if: github.event_name == 'workflow_dispatch' && github.event.inputs.environment == 'goerli'
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add && github.event.inputs.environment == 'goerli'?
It's a manual workflow dispatch step, so maybe we could skip it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could. Generally, right now we only support goerli and we expect that the workflow is dispatched with this environment. I introduced github.event.inputs.environment == 'goerli' condition to not run the deploy job if workflow gets accidentally run on different environment. But even without this condition we don't risk publishing of a package with some invalid contracts - deploy will fail either due to unsupported environment or due to incorrect account being used.
Actually, returning error instead of cleanly exiting the workflow may be a better idea in case wrong environment is provided - I will remove this environment check...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now we only support `goerli` network and we expect that the
workflow (when triggered manually) is always dispatched with this
`environment`. Previously we introduced
`github.event.inputs.environment == 'goerli'` condition to not run the
deploy job if workflow gets accidentally run on a different environment.
But even without this condition we don't risk publishing of a package
with some invalid contracts - deploy will fail either due to
unsupported `environment` or due to incorrect account being used.
Actually, returning error instead of cleanly exiting the workflow may be
a better idea in case wrong `environment` is provided - this will alarm
the scheduler that something went wrong with the deployment.
@nkuba nkuba enabled auto-merge August 3, 2022 14:37
@nkuba nkuba merged commit 73d02b1 into main Aug 3, 2022
@nkuba nkuba deleted the ci-goerli branch August 3, 2022 14:47
@pdyraga pdyraga added this to the v1.2.0 milestone Sep 29, 2022
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.

3 participants