Feat(contracts)/upgradability

[zmalatrax]

This PR follows the #1136 PR which introduced a UUPS Proxy.
A few changes to the upgradability mechanism were made. The SortitionModule and the KlerosCore have been made upgradeable. This is the first step before having the whole Kleros-v2 stack upgradeable. It can be considered WIP as the KlerosCore might need some design changes.

Upgrade Mechanism

Instead of relying on a boolean initialized which only allows initializing the proxy storage once, the upgradeable contracts inherit from Initializable.sol, copied from OpenZeppelin.
It introduces two modifiers: initializer and reinitializer(uint8 version), and the function _disableInitializers(). initializer acts the same as reinitializer(1).

This contract allows re-initializing the proxy storage, if needed, by using the reinitializer(uint8 version) with a higher version than the previous one. It is quite useful when adding new modules requiring initialization.

The version increment can be superior to 1, which allows initializing parent contracts in between if needed. OpenZeppelin has some docs about it here. The version cap is 255, a contract whose version is 255 cannot be re-initialized.

This is what _disableInitializers() do, bumping the initialize version to 255. Calling it in the constructor of the implementation contract will disable the initialization, preventing potential attackers from taking over the implementation contract (see self-destruct attacks..)

KlerosCore Upgradability

The KlerosCore contract is heavy: the deployed bytecode weighs 22.220 kiB in the dev branch (sizes are taken from hardhat-contract-sizer, running yarn hardhat size-contracts). Making it upgradeable (transferring the constructor to an initialize() function and inheriting from two abstract contracts UUPSProxiable and Initializable adds 2.183 kiB to the deployed bytecode. Without reducing the contract size, the upgradeable version of KlerosCore is above 24 kiB, making it impossible to deploy on mainnet.

This is why an optimization of the KlerosCore size is needed.

Optimization

With different optimizations (errors instead of require statements, private constants over unused public constants, caching calls to storage in a variable if there are more than 3 calls to it (empirical value based on tests, I haven't compared the ops but it should add up), removing cache variables if called less than 3 times (low impact, can even be considered negative as it reduces the readability) the size of KlerosCore have been reduced by almost 1 kiB.

Here is a recap table of the measured sizes of KlerosCore:

KlerosCore version	Size of KlerosCore (kiB) - hardhat-contract-sizer	Changes (+/- kiB)
Original (dev branch)	22.220	0
Upgradeable KlerosCore	24.403	+ 2.183
Optimized Upgradeable KlerosCore	23.422	- 0.980

Fig.1.1: KlerosCore size comparison

Here is a table giving measured changes of specific changes made to KlerosCore when optimizing it:

Change to KlerosCore	Measured impact of change - hardhat-contract-sizer (kiB)
Replace modifier onlyByGovernor by a private view function EDIT: reverted for code readability, as long as we can afford it	- 0.366
Cache courts[_courtID] in storage variable court in changeCourtParameters()	- 0.326
Make public constants private	- 0.257
Remove cached variable disputeKit in DisputeKitJump case from appeal()	- 0.015
Remove cached variable court in getTimesPerPeriod()	-0.008
Remove cached variable rate in changeCurrencyRates()	- 0.005

Fig.1.2: Details of KlerosCore optimization

As the details show, removing cached variables when they are called less than 3 times has a minimal impact. In the case of appeal() the variable caches an extremely long path which is of great help for readability purposes. However, for the other two cases, caching a variable has small changes to readability. Even if pretty small, I'd take those changes into account.

Replacing a modifier by a private function is kind of desperate but, as the modifier is called many times, it has the biggest impact. It allows having 0.518 kiB free rather than 0.212 kiB. But even with 0.5 kiB, adding new functionality is rather limited.

This is why splitting the contracts in (at least) two should be the most considered option.

Splitting KlerosCore

Due to its size and the difficulty of reducing it through optimization, it is better to split up KlerosCore.
The split design choices must be well thought out and semantically correct. I haven't had time to think much about it. The functions specified in IArbitratorV2 could be split into one contract, as well as the governance-related functions.

Upgrade Scripts

Proxy Options

To deploy the upgradeable contracts through a custom UUPS proxy, changes to the deployment scripts had to be made, defining the proxy options and how hardhat-deploy should behave when upgrading the proxy. The previous PR #1136's part on the deployment script is outdated as the upgrading mechanism doesn't work due to missing flags.

Here is an updated version:

const sortitionModule = await deploy("SortitionModule", {
    from: deployer,
    proxy: {
      proxyContract: "UUPSProxy",
      proxyArgs: ["{implementation}", "{data}"],
      checkProxyAdmin: false,
      checkABIConflict: false,
      execute: {
        init: {
          methodName: "initialize",
          args: [deployer, KlerosCoreAddress, 1800, 1800, rng.address, RNG_LOOKAHEAD], // minStakingTime, maxFreezingTime
        },
        onUpgrade: {
          methodName: "governor",
          args: [],
        },
      },
    },
    log: true,
  });

Setting the checkProxyAdmin and checkABIConflict flags to false allows not checking if there is a deployed Admin contract or a selector conflict between the proxy functions and the implementation functions, which is only necessary for Transparent proxies.

The onUpgrade function is defined. This specifies the post-upgrade function, the function to be called once the upgrade is done. There usually is no need to call a post-upgrade function unless the proxy storage needs to be initialized again. However, if no updateMethod is defined (i.e. onUpgrade field is not defined), hardhat-deploy will try calling upgradeTo() which is not defined in UUPSProxiable.sol (as in the new versions of OpenZeppelin's proxy).

Calling a public variable getter is a workaround to bypass this limitation.

I have tweaked the hardhat-deploy package to allow specifying a custom upgrade function (which function with which arguments to be called when an upgrade needs to be done). For more details, you can check this issue and PR that I've proposed to hardhat-deploy, but no feedback has been given yet... and therefore not merged.

For testing purposes, a package has been published. It allows specifying the upgrade function to be used when upgrading through the new ProxyOptions field upgradeFunction:

upgradeFunction: {
    methodName: "upgradeToAndCall",
    upgradeArgs: ['{implementation}', '{data}']
},

Upgrading Multiple contracts

Selective Upgrades

The current deployment scripts deploy multiple contracts at once. When we want to upgrade a specific contract, using these scripts would re-deploy non-upgradeable contracts, which are not to be used by the upgraded contracts (unless the proxy storage is also updated). Therefore a script for upgrades only must be written.

However, we should be able to select which contract to upgrade rather than having hardhat-deploy trying to upgrade every upgradeable contract in one script.

To do so, every upgradeable contract should have its XX-upgrade-contract-name.ts script with at least two tags: ["Upgrade", "ContractName"]

Prior to v0.11.37, hardhat-deploy doesn't allow executing deployment scripts that intersect the given tags:
yarn hardhat deploy --network localhost --tags Upgrade,KlerosCore will try running all scripts with the tag "Upgrade" or "KlerosCore".

The version v0.11.37 introduces the flag --tags-require-all which will look for the intersection of the given tags:
yarn hardhat deploy --network localhost --tags Upgrade,KlerosCore --tags-require-all will try running all scripts with the tags "Upgrade" and "KlerosCore".

It allows a more selective choice of deployments with proper tags (not having a unique tag for the sole purpose of running it alone). However, it doesn't support combining AND & OR operations with tags.

But what if we want to upgrade multiple contracts in a specific order?

Upgrading contracts in chains

To upgrade multiple contracts, the scripts should have a common tag/group of tags (e.g. "Upgrade" for every upgrade script, ["Upgrade", "Arbitration"] to upgrade all arbitration contracts...).

Then, the scripts should be ordered with two digits at the beginning of the filename. For example, upgrading the KlerosCore before the SortitionModule would be: 00-upgrade-kleros-core.ts and 01-upgrade-sortition-module.ts.

NOTE on upgradeable contracts importing another upgradeable contract

The SortitionModule imports KlerosCore to have its interface and interact with it. As per the Solidity docs, the deployed bytecode includes an extra 33 bytes which are metadata. These metadata are the ipfs hash of the compiler version, the sources used, the ABI and NatSpec documentation.

More details can be found in this short article.

This metadata includes the sources used, so it is based on the whole source code of the contract, which includes the imported contract source code.
When modifying an imported contract (e.g. KlerosCore), it will modify the deployed bytecode of the contract importing the modified contract (e.g. SortitionModule). As a result, if we perform an upgrade of the KlerosCore, running all upgrade scripts thinking that only the KlerosCore will be upgraded (only changes made), hardhat-deploy will also upgrade the SortitionModule due to this metadata changes. If overlooked, it results in a loss of funds (and block size).

The metadata can be excluded from the bytecode with the flag --no-cbor-metadata when compiling. Metadata are used when verifying the source code, they are crucial for transparency towards users.

Importing an interface instead of the whole contract to make external calls to it should be considered.

Tests

Tests covering the UUPS proxy have been written, but they don't include the Initializable contract in it. It should be done though.
The arbitration/index.ts#DisputeKitClassic has been updated, replacing the SortitionModule & KlerosCore factories with a new deployment, specific to the test.

No tests including the upgrade scripts were written, it has been tested empirically.

• Deploying a first version with yarn start-local
• Redeploy the whole thing with yarn deploy-local, performing an upgrade while doing so (but ineffective as it re-deploy many contracts which aren't used)
  OR
• Upgrade by running chosen upgrade scripts, such as:
  • yarn hardhat deploy --network localhost --tags Upgrade
  • yarn hardhat deploy --network localhost --tags Upgrade,SortitionModule --tags-require-all
  • yarn hardhat deploy --network localhost --tags Upgrade,KlerosCore --tags-require-all

Some yarn scripts should be added to the package.json to easily launch upgrades.

Hardhat tests for the upgrading through hardhat-deploy should also be written. This can be done by making use of deployments.fixture. Broadly, more tests should be written, to have a better coverage of v2 contracts.

[netlify]

✅ Deploy Preview for kleros-v2 ready!

Name	Link
🔨 Latest commit	acb94e9
🔍 Latest deploy log	https://app.netlify.com/sites/kleros-v2/deploys/651315621b6b71000822d33e
😎 Deploy Preview	https://deploy-preview-1214--kleros-v2.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codeclimate]

Code Climate has analyzed commit acb94e9 and detected 19 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Duplication	18

View more on Code Climate.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

No Coverage information
0.2% Duplication

[jaybuidl]

Iterations on @zmalatrax's work

• DRY refactor of the deployment scripts to reuse the deployment logic more easily.
• Made other contracts upgradable: the dispute kits, the gateways, the randomiser RNG, the policy and dispute template registries, in addition to KlerosCore and SortitionModule already migrated by @zmalatrax.
• Some state variables were initialized during definition, this is now done in the initializer function (example 1, example 2).
• Some state variables were immutable, the keyword was removed as these variables cannot be set by the initializer function.
• Fixed some nonce arithmetics (since it takes 2 txs to deploy) and we usually want to compute the proxy address which is deployed after the implementation.
• Inlined and removed the AbstractedDisputeKit, which made initialization harder and was going against our coding principles anyway.
• Cherry-picked some changes from Reduction of KlerosCore bytecode size #1225 to reduce the KlerosCore bytecode size.
• Added a script calling the Etherscan API to mark the proxy as such (so the buttons Read/Write as Proxy do show up).
• The markdown generation script now renders the proxy and implementation as one line