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 job deploying contracts from dapp-development branch #119

Merged
merged 8 commits into from
Aug 17, 2022
Merged

Conversation

michalinacienciala
Copy link
Contributor

@michalinacienciala michalinacienciala commented Jul 28, 2022

There are situations when team developing T Token Dashboard needs to
locally test some functionalities using modified contracts, for example
ones with shorter authorization decrease delay. We decided to create a
dapp-development branch in each of the upstream modules of
threshold-network/token-dashboard CI module, which would store the
code of these modified contracts. In this PR we create a
contracts-dapp-development-deployment-testnet job which deploys the
contracts, creates an NPM package (with dappdev<environment> suffix
an dapp-development-<environment> tag) and publishes it to the NPM
registry. At the end, the job also starts similar deployment for a
downstream module. The job gets triggered only as a result of
workflow_dispath event from a dapp-development branch. Currently
only goerli environment is supported. We don't run system and unit
tests for dapp-development branch, as the tests are not configured to
work with the modified contracts.
Generally, the goal of the changes is to have the full set of
dapp-development-friendly contracts deployed to the NPM registry, so
that the dApp developers could quickly use them by upgrading the
token-dashboard dependencies using yarn upgrade <package-name>@dapp-development-goerli.
If the workflow gets dispatched from a different branch than
dapp-development, the deploy will behave as it used to, publishing
package with deployed unmodified contracts to the NPM registry under
<environment> tag.

TODO:

Refs:
threshold-network/token-dashboard#136
keep-network/keep-core#3121
keep-network/tbtc-v2#392

There are situations when team developing T Token Dashboard needs to
locally test some functionalities using modified contracts, for example
ones with shorter authorization decrease delay. We decided to create a
`dapp-development` branch in each of the upstream modules of
`threshold-network/token-dashboard` CI module, which would store the
code of these modified contracts. In this PR we create a
`contracts-dapp-development-deployment-testnet` job which deploys the
contracts, creates an NPM package (with `dappdev<environment>` suffix
an `dapp-development-<environment>` tag) and publishes it to the NPM
registry. At the end, the job also starts similar deployment for a
downstream module. The job gets triggered only as a result of
`workflow_dispath` event from a `dapp-development` branch. Currently
only `goerli` environment is supported. We don't run system and unit
tests for `dapp-development` branch, as the tests are not configured to
work with the modified contracts.
Generally, the goal of the changes is to have the full set of
dapp-development-friendly contracts deployed to the NPM registry, so
that the dApp developers could quickly use them by upgrading the
`token-dashboard` dependencies using `yarn upgrade
<package-name>@dapp-development-goerli`.
If the workflow gets dispatched from a different branch than
`dapp-development`, the deploy will behave as it used to, publishing
package with deployed unmodified contracts to the NPM registry under
`<environment>` tag.
url: https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}
environment: ${{ github.event.inputs.environment }}
upstream_builds: ${{ github.event.inputs.upstream_builds }}
upstream_ref: dapp-development
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered whether to use explicit value (dapp-development) here, or ${{ github.event.inputs.upstream_ref }}. Decided to use the former - it gives us less flexibility, but the general intention is that we always want to run the downstream modules on the same dapp-development branch. And by hardcoding it, we avoid the nasty consequences of the situation when somebody triggers the workflow from dapp-development branch, but with upstream_ref set to a different branch (which, if not for the harcoded value, would result in triggering of a downstream workflow from that different branch, resulting in publishing of goerli-tagged package that uses modified contracts in it's dependencies) .

@michalinacienciala
Copy link
Contributor Author

michalinacienciala commented Jul 28, 2022

I dry-runned the workflow here: https://github.com/threshold-network/solidity-contracts/runs/7561186309?check_suite_focus=true. The workflow passed, but I noticed two things that seem strange to me:

  1. Although in contracts-dapp-development-deployment-testnet job I don't have the step that prepares the Tenderly config, somehow the tenderly verification worked. I don't understand that. According to hardhat-tenderly documentation

For this plugin to function you need to create a config.yaml file at $HOME/.tenderly/config.yaml or %HOMEPATH%\.tenderly\config.yaml and add an access_key field to it

Without the step that runs the script preparing that config, the file should not exist and Tenderly should not be able to authorize. But from the logs we see that it does. Either the plugin documentation is incomplete or I'm missing something...

  1. In one of the runs I made when testing the new workflow structure, the contract deployment script did not mint the expected amount of T (minted 0.0 T). Here is the run: https://github.com/threshold-network/solidity-contracts/runs/7560768157?check_suite_focus=true.
    I also had similar situation with NU (minted 0.0 NU) in another run:https://github.com/threshold-network/solidity-contracts/runs/7556698468?check_suite_focus=true.
    Most of the runs minted correct amounts of tokens.

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.
Base automatically changed from ci-goerli to main August 3, 2022 14:47
The contracts deployed in the
`contracts-dapp-development-deployment-testnet` job will be used by dApp
developers to build their local environments and deploy dApp previews.
We want more flexibility there than on the public-facing testnet dApp and
we want to use different deployer accounts for those two different types
of testnet dApps.
@michalinacienciala
Copy link
Contributor Author

I tested the deployment again, this time on a branch being combination of changes from #119 and #118 (+ some small changes in workflows to allow for testing). Deployment has failed due to an issue in one of the modified deployment scripts, which I described in https://github.com/threshold-network/solidity-contracts/pull/118/files#r943208193.
The #119 can reviewed, but we will not be able to publish the package until the problem with failing deployment script gets fixed.

@michalinacienciala
Copy link
Contributor Author

The #119 can reviewed, but we will not be able to publish the package until the problem with failing deployment script gets fixed.

The problem with the deployment script got fixed. I verified the #118 + #119 again, works as expected. Workflow run: https://github.com/threshold-network/solidity-contracts/runs/7801852357?check_suite_focus=true.

.github/workflows/contracts.yaml Outdated Show resolved Hide resolved
.github/workflows/contracts.yaml Outdated Show resolved Hide resolved
# branch, which are slightly modified to help with the process of testing some
# features on the Threshold Token dApp. The job starts only if workflow gets
# triggered by the `workflow_dispatch` event on the branch `dapp-development`.
contracts-dapp-development-deployment-testnet:
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering whether common parts of contracts-deployment-testnet and contracts-dapp-development-deployment-testnet could be extracted to a reusable workflow or action. But this is something that could be handled in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely something we could do! There's more places in our workflows where using custom actions or reusable workflows could be useful. Let's not wait for these improvements to not postpone the merge. I will refactor the workflow in the future.

We don't want to fill the default value with value that is supported, to
prevent from accidental dispatches of the workflow by people who don't
fully understand how the inputs should be configured. Previously we used
explicitely incorrect default value, but that may be too much and may be
a bit confusing. Let's leave the input without default.
Previously `-` was not supported as a value of `environment` property in
the `npm-version-bump` action. Now action supports hyphens and we can
change the suffix to more readible format.
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