livepeer / LIPs

Livepeer Improvement Proposals
9 stars 13 forks source link

LIP-40: Minter Math Precision #40

Closed yondonfu closed 3 years ago

yondonfu commented 4 years ago

This is the discussion thread for LIP-40: Minter Math Precision which proposes an increase in the number of decimal places of precision supported in the percentage math operations of the Minter contract. The draft was recently merged. All feedback is welcome! All reviewers should refer to the LIP text for the latest version of the proposal.

yondonfu commented 4 years ago

LIP-40 has just been assigned the Last Call status which kicks of a 10 day Last Call period for any remaining feedback before it will be assigned the Proposed status.

dob commented 4 years ago

In the proposal, there is a step where the new minter is registered with the controller and then a function is invoked to migrate the funds from the old minter to the new minter. Although low probability, is there a timing issue at play here?

If the new minter is registered, and then someone calls reward before the migration txn has occurred, does anything get screwed up with the accounting in the protocol? Can this be mitigated by registering and migrating in an atomic transaction?

yondonfu commented 4 years ago

If the new minter is registered, and then someone calls reward before the migration txn has occurred, does anything get screwed up with the accounting in the protocol?

Regardless of whether the migration tx has been executed or not, the reward caller would receive 0 tokens because the currentMintableTokens state variable in the new Minter will be 0 until the next round is initialized. As a result, when reward is called before the next round is initialized, the calculated reward to mint would be 0. This is the reason why LIP-40 notes that all active orchestrators would need to call reward before the described upgrade takes place.

Can this be mitigated by registering and migrating in an atomic transaction?

Registering the new Minter and executing them migration in a single atomic tx would be ideal, but unfortunately it is not possible without LIP-25 because the current Controller owner is a Gnosis multisig that does not support batch transactions. If LIP-25 is accepted and deployed then a single atomic tx would be possible.

EDIT: I actually think a single atomic tx might be possible pre-LIP-25 with a delegatecall proxy. Looking into it and will report back later today.

yondonfu commented 4 years ago

Follow up on the above comment:

I actually think a single atomic tx might be possible pre-LIP-25 with a delegatecall proxy. Looking into it and will report back later today.

This actually is not possible. I considered 2 ways to use a delegatecall proxy:

  1. Controller owner -> proxy which delegatecalls setContractInfo() on the Controller and migrateToNewMinter() on the old Minter

  2. Controller owner -> proxy which delegatecalls a script contract which calls setContractInfo() on the Controller and migrateToNewMinter() on the old Minter

Option 1 doesn't work because while msg.sender in setContractInfo() and migrateToNewMinter() will be the Controller owner, the storage context during the execution of these functions will be that of the proxy. As a result, we would be accessing storage values on the proxy instead of storage values on the Controller and old Minter

Option 2 doesn't work because msg.sender in setContractInfo() and migrateToNewMinter() will be the proxy instead of the Controller owner.

In conclusion, registering the new Minter and executing the migration to the new Minter in a single atomic tx is not possible without LIP-25.

yondonfu commented 4 years ago

47 contains an update to the LIP noting that the depositETH() function on the new Minter contract needs to be callable when the Controller is paused in order to allow the old Minter to transfer ETH to the new Minter.

yondonfu commented 4 years ago

This LIP has been assigned the Proposed status in #48. Since it is bundled in LIP-35, it can be voted on as a part of that bundle when a poll is created for LIP-35.

yondonfu commented 3 years ago

This LIP has been assigned the Accepted status as a result of the LIP-35 poll.

yondonfu commented 3 years ago

This LIP has been assigned the Final status as the relevant changes have been deployed in an upgrade.