LIP-25: Governor contract

[kyriediculous]

This PR implements the Governor contract according to the spec defined in LIP25 https://github.com/livepeer/LIPs/pull/33

[coveralls]

Pull Request Test Coverage Report for Build 0584af19-66f6-4ea8-b536-951fa4c036b8

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

---

Totals
Change from base Build a623d597-a8ef-4d73-97a8-c86567553e85:	0.0%
Covered Lines:	807
Relevant Lines:	807

---

💛 - Coveralls

[kyriediculous]

moved solium package to ethlint in 93301cc

https://github.com/duaraghav8/Ethlint

If you're currently using the solium package for npm install, it is highly recommended that you move to ethlint. The solium package will not receive updates after December, 2019. There are no differences between the updates pushed to ethlint and solium packages.

[kyriediculous]

I will add the necessary tests to bring coverage up.

[kyriediculous]

Should there also be a function to cancel staged updates in the scenario that an update is staged, but before it is executed there is a desire to not move forward with it?

From a feature perspective it's a similar discussion as the nonce in my opinion:

Is social convention enough for updates cancellation (e.g. staged but never executed upon).

Or do we want guarantees that a cancelled update can never be executed (explicit cancellation function)

From a technical perspective having explicit cleanups would be nice for reducing unnecessary state growth.

But again it comes at a cost.

Also we'd have to figure out ( and probably put in scope for this LIP) what the cancellation process would look like and how that would work with delays.

[yondonfu]

Or do we want guarantees that a cancelled update can never be executed (explicit cancellation function)

I think this is preferable since after the delay for a staged update any account can call the execute() function to execute the update which means never executing a staged update would be insufficient.

Also we'd have to figure out ( and probably put in scope for this LIP) what the cancellation process would look like and how that would work with delays.

We could allow a cancel() function to be called for a staged update as long as it hasn't been executed yet (in this case the staged update would have been removed from the Governor anyway). One reason why a staged update would be canceled is if during the delay a bug in the implementation was discovered. The social convention would be that the cancel() function should only be used for this reason.

[kyriediculous]

482bdbb - adds a cancel(Update memory _update) function that works similar to the other Governor functions

56c3078 - forwards revert reasons from external calls

e7c13ef - adds test for our modified Ownable.sol implementation (mainly the onlyThis modifier)

5ef3569 - misc fixes

[kyriediculous]

This should be ready for final review now.

once this PR is approved we can move it to last call. I'd like to clean up the commit history a little bit after final review though.

[kyriediculous]

I believe 21161ac addresses all change requests

[kyriediculous]

e4af94a fixes a typo and some descriptions to match revert reasons

[kyriediculous]

522ffdb fixes the changes discussed after the security review

• make execute payable instead of stage
• introduce a nonce to the update struct so that we can differentiate between updates , i.e. unique identifier. Not stored on-chain.

[kyriediculous]

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

Should we check the codesize of the target address ?

[yondonfu]

Should we check the codesize of the target address ?

I think its reasonable to just have the update submitter be responsible for ensuring that the contract exists via a check off-chain instead of adding that check in the contract since the update submitter is already responsible for ensuring that the rest of the update (i.e. tx data) is correct.

[yondonfu]

Updates look good to me - let's rebase on top of streamflow since a few changes have been made there recently?

[kyriediculous]

Done !