Script to deploy smart contract changes to a network via governance #4706

asaj · 2020-08-14T14:55:42Z

Description

This PR introduces the make-release script, which reads a backwards compatibility report against the latest release generated by check-version, deploys any changed contracts to the specified network, and outputs a JSON of the proposal that should be submitted to governance in order to promote the release.

This script is part of a series of tools that are intended to make smart contract deployments as easy and safe as possible. These include:

check-deployed: Takes smart-contract source code and confirms that the specified network is running that code. Used to verify that the latest tagged release is in fact what has been released to the network.
check-versions: Takes two branches and checks that, given the smart contract versions and source code of branch A, branch B has correctly set the smart contract versions given the source code changes. Outputs a summary of all smart contract changes between branch A and B. Used to sanity check that the developers' understanding of all smart contract changes from A to B are fully understood, and to specify what contracts need to be upgraded in order to release the contracts in branch B onto a network that is currently running the contracts in branch A.
make-release: Takes the compatibility report output by check-versions, deploys the specified contracts, and outputs a governance proposal. Used to propose a release of the contracts in branch B onto a network that is currently running the contracts in branch A.
verify-release: Takes two branches and a proposal ID, creates a compatibility report with check-versions, and confirms that the on-chain governance proposal is the result of make-release on branches A and B. Used by CELO holders to verify contract upgrade proposals before voting.

Other changes

Adds proxies for all linked libraries so that they can be upgraded in the future without having to upgrade contracts that link them. There is some special casing to redeploy all linked libraries with proxies in release 1, which should be removed after the release is made.
Adds versions to linked libraries
Flag rename in check-versions
Use 'Test' suffix to denote test contracts everywhere
Don't ignore linked list libraries in contract version check

Tested

First, spin up a ganache network with the contracts/0 release:

git checkout release/contracts/0
yarn devchain generate-tar .tmp/devchain.tar.gz && yarn devchain run-tar .tmp/devchain.tar.gz

Then, populate the initialization data for DowntimeSlasher:

echo '{"DowntimeSlasher": ["0x000000000000000000000000000000000000ce10", 10, 5, 20]}' > initialize_data.json

Finally, run the script:

yarn make-release -a release/contracts/0 -b asaj/make-release -n development -i initialize_data.json -p proposal.json -r report.json

Related issues

-Part of broader work towards #4426

Backwards compatibility

That's the goal!

nambrot

Catching up right now, so please forgive any inaccurate statements.

If I understand correctly, the dilemma we are facing is that in master, changes to linked libraries imply that all core contracts that rely on the changes would need to be explicitly upgraded as well?

The solution that you have outlined here is to make all linked libraries proxied, so that changes to libraries can be done via repointing the library proxy instead of having to upgrade the core contracts itself.

I couldn't tell from the PR how exactly you changed the core contracts to access the linked library via proxies vs. directly?

Additionally, would you mind elaborating again why you felt that upgrading all linked libraries was favorable in your opinion? Vs. Having core contracts being able to explicitly rely on possibly older versions of libraries. I assume it is because the invariant of having the source code at a commit represents the contract bytecode in totality on a network?

packages/protocol/scripts/truffle/make-release.ts

asaj · 2020-08-17T14:27:29Z

If I understand correctly, the dilemma we are facing is that in master, changes to linked libraries imply that all core contracts that rely on the changes would need to be explicitly upgraded as well?

Correct, that is the implication. However, because changes to a library do not cause bytecode changes to any contracts that link that library, our current compatibility tool will not recognize this implication.

The solution that you have outlined here is to make all linked libraries proxied, so that changes to libraries can be done via repointing the library proxy instead of having to upgrade the core contracts itself.

Exactly

I couldn't tell from the PR how exactly you changed the core contracts to access the linked library via proxies vs. directly?

There are two pieces to this.

The first is to ensure changes to all contracts that link libraries, so that the compatibility tool flags them as needing to be upgraded. There is a sanity check in make-release.ts that confirms that all contracts that link libraries are included in the compatibility report.

The second is to deploy the proxied libraries before any of the contracts that link them, and to explicitly link the contracts to those proxied libraries before deploying the contracts.

Additionally, would you mind elaborating again why you felt that upgrading all linked libraries was favorable in your opinion? Vs. Having core contracts being able to explicitly rely on possibly older versions of libraries. I assume it is because the invariant of having the source code at a commit represents the contract bytecode in totality on a network?

Correct, having core contracts rely on older versions of libraries breaks the invariant that the latest release branch represents exactly the contract code that is running on public networks.

yorhodes

forgot to click submit review!

packages/contractkit/src/base.ts

yorhodes · 2020-08-14T18:23:09Z

packages/protocol/contracts/common/proxies/AddressLinkedListProxy.sol

+import "../Proxy.sol";
+
+/* solhint-disable no-empty-blocks */
+contract AddressLinkedListProxy is Proxy {}


any reason why these need to be explicitly separate instead of just reusing Proxy?

I think we wanted separate build artifacts for each. Happy to revisit this question outside the scope of this PR

Yeah, this definitely made things easier when relying on Truffle for contract deployments. I guess going forward we'll be doing upgrade deployments somewhat differently (using this PR's script), but keeping with this convention might still be good to keep things organized, keep track of which contracts are proxied, + we still use Truffle for any dev deployments/new testnets.

yorhodes · 2020-08-21T19:19:31Z

packages/protocol/scripts/truffle/make-release.ts

+ *
+ */
+
+class ContractDependencies {


I think there are some libraries for this like https://github.com/ConsenSys/surya
not suggesting we use it at this stage but good to be aware of in the future
or similarly publishing these scripts as our own npm package for truffle projects

Fortunately our migrations will fail if our hardcoded list of dependencies don't have complete coverage, so it's not as fragile as these sorts of things normally are.

yorhodes · 2020-08-21T19:22:35Z

packages/protocol/scripts/truffle/make-release.ts

+  // This makes essentially every contract dependent on Governance.
+  console.log(`Transferring ownership of ${contractName}Proxy to Governance`)
+  if (!dryRun) {
+    await proxy._transferOwnership(addresses.get('Governance'))


nit but we could pass in only the governance address to this function and fetch it once outside

Did it this way to cover the future case where the governance address might change due to a new governance proxy being deployed

packages/protocol/scripts/truffle/make-release.ts

yorhodes · 2020-08-21T19:26:47Z

packages/protocol/scripts/truffle/make-release.ts

+        }
+        // 2. Link dependencies.
+        const Contract = await artifacts.require(contractName)
+        await Promise.all(contractDependencies.map((d) => Contract.link(d, addresses.get(d))))


does this mutate the artifacts?
do we know if we store the artifacts linked or unlinked?

Yes I believe it does. They start off unlinked, and then they get linked here

On a related note, though not related to this PR directly, have we decided how we're going to be managing artifacts within the release process?

m-chrzan

Looks awesome! Left some comments, mostly nits.

packages/protocol/contracts/common/Signatures.sol

packages/protocol/contracts/common/linkedlists/AddressLinkedList.sol

m-chrzan · 2020-08-22T16:20:42Z

packages/protocol/contracts/common/proxies/AddressLinkedListProxy.sol

+import "../Proxy.sol";
+
+/* solhint-disable no-empty-blocks */
+contract AddressLinkedListProxy is Proxy {}


Yeah, this definitely made things easier when relying on Truffle for contract deployments. I guess going forward we'll be doing upgrade deployments somewhat differently (using this PR's script), but keeping with this convention might still be good to keep things organized, keep track of which contracts are proxied, + we still use Truffle for any dev deployments/new testnets.

m-chrzan · 2020-08-22T16:24:11Z

packages/protocol/contracts/identity/test/AttestationsTest.sol

@@ -7,7 +7,7 @@ import "../Attestations.sol";
 * but mocks the implementations of the validator set getters. Otherwise we
 * couldn't test `request` with the current ganache local testnet.
 */
-contract TestAttestations is Attestations {
+contract AttestationsTest is Attestations {


Hmm, definitely a major nit, but to me XTest sounds like "file containing tests for X", TestX like "a version of X used for testing". Most of these files I think are the latter, so just wondering why the rename?

Just to be consistent, happy to rename to TestX rather than XTest but would prefer not to block this PR on that

packages/protocol/scripts/bash/check-versions.sh

packages/protocol/scripts/truffle/make-release.ts

m-chrzan · 2020-08-22T17:34:04Z

packages/protocol/scripts/truffle/make-release.ts

+        }
+        // 2. Link dependencies.
+        const Contract = await artifacts.require(contractName)
+        await Promise.all(contractDependencies.map((d) => Contract.link(d, addresses.get(d))))


On a related note, though not related to this PR directly, have we decided how we're going to be managing artifacts within the release process?

m-chrzan · 2020-08-22T17:39:28Z

packages/protocol/scripts/truffle/make-release.ts

+              contract: `${contractName}Proxy`,
+              function: '_setImplementation',
+              args: [contract.address],
+              value: '0',


Do we also want a human readable description like above?

I think it's sufficiently readable, the problem with the registry repointing is that the registry ID is not human readable

…monorepo into asaj/make-release

… governance" (#4706) into "oz-prerelease" (#5194) * Cherry pick 8a8b822 * Resolve merge conflicts from cherry-pick * Remove double-specification of ICeloVersionedContract Co-authored-by: Asa Oines <[email protected]>

…elo-org#4706) ### Description This PR introduces the `make-release` script, which reads a backwards compatibility report against the latest release generated by `check-version`, deploys any changed contracts to the specified network, and outputs a JSON of the proposal that should be submitted to governance in order to promote the release. This script is part of a series of tools that are intended to make smart contract deployments as easy and safe as possible. These include: 1. *check-deployed*: Takes smart-contract source code and confirms that the specified network is running that code. Used to verify that the latest tagged release is in fact what has been released to the network. 2. *check-versions*: Takes two branches and checks that, given the smart contract versions and source code of branch A, branch B has correctly set the smart contract versions given the source code changes. Outputs a summary of all smart contract changes between branch A and B. Used to sanity check that the developers' understanding of all smart contract changes from A to B are fully understood, and to specify what contracts need to be upgraded in order to release the contracts in branch B onto a network that is currently running the contracts in branch A. 3. *make-release*: Takes the compatibility report output by *check-versions*, deploys the specified contracts, and outputs a governance proposal. Used to propose a release of the contracts in branch B onto a network that is currently running the contracts in branch A. 4. *verify-release*: Takes two branches and a proposal ID, creates a compatibility report with *check-versions*, and confirms that the on-chain governance proposal is the result of *make-release* on branches A and B. Used by CELO holders to verify contract upgrade proposals before voting. ## Other changes - Adds proxies for all linked libraries so that they can be upgraded in the future without having to upgrade contracts that link them. There is some special casing to redeploy all linked libraries with proxies in release 1, which should be removed after the release is made. - Adds versions to linked libraries - Flag rename in `check-versions` - Use 'Test' suffix to denote test contracts everywhere - Don't ignore linked list libraries in contract version check ### Tested First, spin up a ganache network with the contracts/0 release: ``` git checkout release/contracts/0 yarn devchain generate-tar .tmp/devchain.tar.gz && yarn devchain run-tar .tmp/devchain.tar.gz ``` Then, populate the initialization data for DowntimeSlasher: ``` echo '{"DowntimeSlasher": ["0x000000000000000000000000000000000000ce10", 10, 5, 20]}' > initialize_data.json ``` Finally, run the script: ``` yarn make-release -a release/contracts/0 -b asaj/make-release -n development -i initialize_data.json -p proposal.json -r report.json ``` ### Related issues -Part of broader work towards celo-org#4426 ### Backwards compatibility That's the goal!

Asa Oines added 24 commits August 4, 2020 12:28

WIP make-release

3588479

Merge branch 'master' into asaj/make-release

9bff9b9

Somewhat functional

6c4ba95

Smarter releases WIP

5875573

Add library proxies

8b6ea77

Missing proxy

29a2d56

Make it work with upgradable libraries

220c76e

Read initialize data json

3326ccf

Sanity checks for release 1

18170e5

Add version number to contracts that link libraries

3d0f783

update version numbers

4ea8b4e

Add version to linkedlist

3b123df

Ignore proxies when generating compatibility report

67ca837

Version fixidity lib

86d1abd

cleanup

de9e1fd

Merge branch 'master' into asaj/make-release

57a1529

Add script for making releases

6f308cc

oops

295e5e0

oops

995dfc8

cleanup

150aaf7

typo

1832bf9

Fix

1ca0365

Merge branch 'master' into asaj/make-release

2817f8a

Cleanup

88e8fc0

asaj assigned nambrot, m-chrzan and yorhodes Aug 14, 2020

nambrot reviewed Aug 17, 2020

View reviewed changes

asaj marked this pull request as ready for review August 17, 2020 17:38

Asa Oines added 6 commits August 20, 2020 09:10

Address comments

a3b58e5

Merge branch 'master' into asaj/make-release

783c4a6

Update docs

10a3278

Make work

4429df1

Merge branch 'master' into asaj/make-release

b7dbc16

Merge branch 'master' into asaj/make-release

101c5af

yorhodes approved these changes Aug 21, 2020

View reviewed changes

m-chrzan approved these changes Aug 22, 2020

View reviewed changes

Asa Oines added 4 commits August 24, 2020 18:13

Inline libraries where possible

14bd5b3

Address feedback

783e5b7

Merge branch 'asaj/make-release' of https://github.com/celo-org/celo-…

4d3a08d

…monorepo into asaj/make-release

Merge branch 'master' into asaj/make-release

dae202d

asaj assigned asaj and unassigned nambrot, yorhodes and m-chrzan Aug 24, 2020

asaj added the automerge Have PR merge automatically when checks pass label Aug 24, 2020

mergify bot and others added 6 commits August 25, 2020 00:10

Merge branch 'master' into asaj/make-release

c0687ba

Merge branch 'master' into asaj/make-release

48079ef

Merge branch 'master' into asaj/make-release

e02ffbb

Merge branch 'master' into asaj/make-release

3beb283

Inlined library should be inlined

36e977b

Ignore inlined libraries

2c22704

mergify bot merged commit 8a8b822 into master Aug 25, 2020

mergify bot deleted the asaj/make-release branch August 25, 2020 14:16

m-chrzan mentioned this pull request Sep 10, 2020

Finalize the automation of the smart contract upgrade process #4426

Closed

6 tasks

nambrot mentioned this pull request Sep 24, 2020

Cherry-pick "Script to deploy smart contract changes to a network via governance" into "oz-prerelease" #5194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Script to deploy smart contract changes to a network via governance #4706

Script to deploy smart contract changes to a network via governance #4706

asaj commented Aug 14, 2020 •

edited

Loading

nambrot left a comment

asaj commented Aug 17, 2020

yorhodes left a comment

yorhodes Aug 14, 2020

asaj Aug 21, 2020

m-chrzan Aug 22, 2020

yorhodes Aug 21, 2020

asaj Aug 21, 2020

yorhodes Aug 21, 2020

asaj Aug 24, 2020

yorhodes Aug 21, 2020

asaj Aug 21, 2020

m-chrzan Aug 22, 2020

m-chrzan left a comment

m-chrzan Aug 22, 2020

m-chrzan Aug 22, 2020

asaj Aug 24, 2020 •

edited

Loading

m-chrzan Aug 22, 2020

m-chrzan Aug 22, 2020

asaj Aug 24, 2020

Script to deploy smart contract changes to a network via governance #4706

Script to deploy smart contract changes to a network via governance #4706

Conversation

asaj commented Aug 14, 2020 • edited Loading

Description

Other changes

Tested

Related issues

Backwards compatibility

nambrot left a comment

Choose a reason for hiding this comment

asaj commented Aug 17, 2020

yorhodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-chrzan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asaj Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asaj commented Aug 14, 2020 •

edited

Loading

asaj Aug 24, 2020 •

edited

Loading