search

livepeer / protocol

Livepeer protocol
MIT License
154 stars 45 forks source link

confluence: Add L2 LPT and fix LPT calls from the Minter #531

Closed yondonfu closed 2 years ago

yondonfu commented 2 years ago

What does this pull request do? Explain your changes. (required)

Note: All descriptions refer to contracts on L2

This PR fixes two bugs in the Minter:

  1. Previously, the Minter called the LPT contract using the function interface mint(address,uint256) public returns (bool). The new LPT implementation interface for minting is slightly different: mint(address,uint256) external. The function interface used in the Minter expected a return value from the LPT contract the call to the LPT contract would revert because the compiled bytecode for invoking a function with a return value involves a check to ensure that the returned data is the correct size [1]. Since the LPT contract does not return any data for the mint() function, the call from the Minter reverted in createReward(). In practice, this caused reward calls on testnet to always revert. The fix for this issue is to change the function interface for mint() used by the Minter to not expect a return value.

  2. Previously, the Minter was calling transferOwnership() on the LPT contract in migrateToNewMinter(). The new LPT implementation does not have a transferOwnership() function. As a result, a call to migrateToNewMinter() would revert because the Minter would call a non-existent function on the LPT contract. The fix for this issue is to remove the transferOwnership() call in migrateToNewMinter(). Instead, the admin of the LPT contract (in practice, the Governor) is responsible for revoking the MINTER_ROLE for the old Minter and granting the MINTER_ROLE to the new Minter.

In order to update the tests to fix the above issues and to ensure that we don't run into other incompatibility issues between protocol contracts and the LPT contract, I copied the LPT contract from https://github.com/livepeer/arbitrum-lpt-bridge/blob/main/contracts/L2/token/LivepeerToken.sol into this repo.

[1] For a more in-depth analysis of this problem as it pertained to ERC20 token implementations refer to https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca

Specific updates (required)

See commit history.

How did you test each of these updates (required)

Updated unit tests.

Does this pull request close any open issues?

Fixes #509

Checklist:

yondonfu commented 2 years ago

Pinned the LPT contract to solc 0.8.8

I'll squash the fixup commit before merging.

since the entire LPT contract is now moved into this repo, the last commit in this open PR will no longer be valid.

Note that I have a separate draft PR that is based off the changes in your PR that adds the option of specifying an already deployed LPT address during deployment of protocol contracts. The PR itself still needs some tweaking, but the general idea is that you can specify an already deployed LPT address or deploy the LPT implementation from this repo. Now that we have copied the L2 LPT contract impl from arbitrum-lpt-bridge the latter is a viable option. But, you could still just specify an already deployed LPT address so the changes that enable that are still useful.