Implement LIP-40

[yondonfu]

This PR implements LIP-40.

TODO:

• [x] Add an integration test for the Minter upgrade as described in the deployment procedure for LIP-40

[coveralls]

Pull Request Test Coverage Report for Build 49fc7f9a-712b-4501-af41-891e4649911c

• 18 of 18 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build e4adf4e1-3d95-4969-8f2a-f80ae94a960a:	0.0%
Covered Lines:	715
Relevant Lines:	715

---

💛 - Coveralls

[kyriediculous]

Deleted my previous comment about creating a getter for the constant PERC_DIVISOR and then using that getter in the solidity unit tests instead of hardcoded values because I thought it wouldn't work but it actually does. But leaving it up to you since it's quite trivial.

[kyriediculous]

This looks good to me so far but I'll be awaiting the integration test to give my final approval.

[yondonfu]

Deleted my previous comment about creating a getter for the constant PERC_DIVISOR and then using that getter in the solidity unit tests instead of hardcoded values because I thought it wouldn't work but it actually does. But leaving it up to you since it's quite trivial.

I have a slight preference to continue defining the constant in the tests because:

• It avoids adding an additional getter that would only be used for a Solidity test
• The validPerc() tests check that the library exhibits the right behavior given an expected constant value
• Even if the constant is defined in the library I think we'd still need to define the constant in the JS tests for contracts that use the library internally (i.e. BondingManager) since the getter wouldn't be exposed on the contract artifact used in the tests

[yondonfu]

This looks good to me so far but I'll be awaiting the integration test to give my final approval.

Integration test added in 78d22e2

9208057 contains a required update that was surfaced by the integration test. See this comment for more details.

I'll update the LIP-40 spec after this change is reviewed.

[kyriediculous]

Let's rebase !

[yondonfu]

Rebased

[yondonfu]

Force pushed to update a Minter unit test to use a realistic mock value for BondingManager.getTotalBonded() (the previous value exceeded the total supply). See this line. No other changes were made.

[kyriediculous]

While the added integration test checks for the correctness of the upgrade path it provides no guarantees about the actual execution of that upgrade.

What did you have in mind for actually deploying those changes to production? I Don't see a migration that would currently handle this upgrade. Were you planning on adding that? The current 3_deploy_contracts.js migration file would not work for this because we're actually changing params and not just adding contracts to the protocol.

Do you think it would be valuable to write a separate upgrade script for this using common web3 libraries so that we can actually integration test for that script instead of the separate steps we would execute manually ? We can re-use that script in a migration.

On a sidenote I'm actually wondering again how we executed the streamflow update because the Migrations contract stores the last number of the executed migration, so AFAIK we would have explicitly told truffle to re-execute 3_deploy_contracts for the streamflow update? I don't think this is what would be convention though.

Edit: The upgrade path checks out code-wise to me and the integration test surely helps in confirming that but what I'm a little bit more worried about is the actual upgrade execution itself.

[yondonfu]

What did you have in mind for actually deploying those changes to production? I Don't see a migration that would currently handle this upgrade.

Upgrades on mainnet do not use migration scripts because all the upgrade operations (minus the deployment of a new contract) are currently multisig transactions that need to be manually submitted. The steps that will be manually executed are described in the LIP-40 spec and outlined in the MinterUpgrade integration test which might be the best that we can do for now given that we need to manually execute the steps of an upgrade. In the future, if LIP-25 is deployed, we could construct the update (with all the relevant transaction data), test it and then use that same update data when executing the upgrade (a manual action would still be required, but we would always only need one instead of potentially many).

On a sidenote I'm actually wondering again how we executed the streamflow update because the Migrations contract stores the last number of the executed migration, so AFAIK we would have explicitly told truffle to re-execute 3_deploy_contracts for the streamflow update?

The Streamflow upgrade was executed manually due to the reasons mentioned above. We currently do not rely on migration scripts outside of development and deployment to new networks.

[kyriediculous]

are currently multisig transactions that need to be manually submitted

I see and the implementation doesn't allow for automation. Unfortunate but okay, let's be very thorough then with the actual upgrade execution itself.

[kyriediculous]

I'm adding integration tests for #383 and you mentioned wanting to test for revert reasons of calls. However many of our current contracts do not have revert reasons.

I added them to the Minter.migrateToNewMinter function in #383 for testability but since we are deploying a new minter anyway what do you think of adding them for the new deployment of the Minter as well ?

    function migrateToNewMinter(IMinter _newMinter) external onlyControllerOwner whenSystemPaused {
        // New Minter cannot be the current Minter
        require(_newMinter != this, "New minter cannot be the current minter");
        // Check for null address
        require(address(_newMinter) != address(0), "New minter is null address");

        IController newMinterController = _newMinter.getController();
        // New Minter must have same Controller as current Minter
        require(newMinterController == controller, "New minter has a different Controller");
        // New Minter's Controller must have the current Minter registered
        require(newMinterController.getContract(keccak256("Minter")) == address(this), "Current Minter not registered with Controller");

        // Transfer ownership of token to new Minter
        livepeerToken().transferOwnership(address(_newMinter));
        // Transfer current Minter's token balance to new Minter
        livepeerToken().transfer(address(_newMinter), livepeerToken().balanceOf(address(this)));
        // Transfer current Minter's ETH balance to new Minter
        _newMinter.depositETH.value(address(this).balance)();
    }

[yondonfu]

since we are deploying a new minter anyway what do you think of adding them for the new deployment of the Minter as well

I think this is reasonable as revert reasons add information to error paths that already exist in the contract and do not impact contract behavior in any other way.

Added revert reasons in d9272c8

[kyriediculous]

In relation to LIP-36 (#387 ):

Currently our contract explicitly implement their interfaces (i.e contract Minter is IMinter) but this seems rather unnecessary and even weird (and it increases the bytecode size slightly).

Even for ERC20 it is weird because the contract doesn't need to implement the interface for itself but for external callers that require it to implement the interface. The only thing this explicitness really gives us is compile-time errors when a contract doesn't implement its interface correctly.

Furthermore it restricts us from adding public variable getters to the interface as it results in multiple declarations with the same name.

e.g. uint public a would implicitly result in a getter function a() public view returns(uint)

The workaround is very obvious , create an additional getter function getA() public view returns(uint) , but that again seems rather unnecessary to have duplicate getters when it's avoidable.

Therefore I would suggest for the Minter to no longer explicitly inherit its interface. This would allow us to add the related getter to the interface for external contracts to use without having to add additional getters to the implementation (and interface) itself.

---

There are no behavioural changes that result from this. The only real drawback is that explicit type conversions from Minter to IMinter are not allowed as it would no longer inherit its interface. That only results in us having to remove the following check in migrateToNewMinter(): require(_newMinter != this);. Seeing we already check for the addresses to not be equal I think we can safely remove this as well as explicit interface inheritance.

[yondonfu]

@kyriediculous and I discussed the above comment in Discord. The conclusion was that if we define the following in the IMinter abstract contract:

function currentMintableTokens() external view returns (uint256);

then the Minter could override the external currentMintableTokens() function with the public getter for the currentMintableTokens state variable. Given that this workaround is available, the decision was made to leave the code in this PR as-is and to freeze it for deployment.