livepeer / protocol

Livepeer protocol
MIT License
152 stars 45 forks source link

Funds migration to Refunder contract from JobsManager #369

Closed yondonfu closed 4 years ago

yondonfu commented 4 years ago

This PR adds support for funds migration from the JobsManager to a refunder address specified by the owner of the Controller. In practice, the address specified will be the Refunder contract which will allow users to withdraw a refund amount that matches the amount they currently have deposited with the JobsManager.

449bd36 is mainly test refactoring.

6b3e3c9 contains the actual updates to the JobsManager.

The updated JobsManager code in this PR will be deployed as the target implementation for the current JobsManager proxy. After this target implementation upgrade, funds will be migrated to the Refunder contract and all state mutating functions on the JobsManager proxy will be disabled [1].

See the upgrade plan for more details on how funds migration and the Refunder contract will work.

[1] Besides the functions that are only callable by the Controller owner - these functions only update global contract params and do not affect user state.

yondonfu commented 4 years ago

do you think it's worth adding a revert reason to prevent the state updates along the lines of "contract deprecated since streamflow" ?

Unfortunately, revert reason strings were not supported by solc until v0.4.22, but the master branch is using Truffle v4.0.4 which bundles solc v0.4.18 (older versions of Truffle did not allow you to choose a solc version). If we want to use a revert reason string here we would need to upgrade to a newer version of Truffle to compile the contracts with solc >= v0.4.22 which introduces the potential headache of upgrading Truffle (there may be changes in other areas of the codebase required?). Additionally, using a different compiler version would be one more change (albeit small) to the JobsManager. In the interest of keeping the changes required to upgrade the JobsManager as small as possible, I'm ok with not using a revert reason string here.

For code readability I would have perhaps opted for a modifier to be ran before whenSystemNotPaused

Agreed that the modifier improves code readability (as well as the readability of the diff). 73a6190 uses the noStateUpdates() modifier.

kyriediculous commented 4 years ago

Unfortunate RE revert reasons.

Changes look good, let's rebase !

yondonfu commented 4 years ago

Rebased