New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A pass at upgradeability #75

Merged
merged 10 commits into from Sep 28, 2017

Conversation

Projects
None yet
3 participants
@yondonfu
Member

yondonfu commented Sep 21, 2017

  • ManagerProxy.sol contains the logic for forwarding method calls to target contracts such as BondingManager, JobsManager, etc. Target contract methods are invoked using delegatecall thereby allowing the target contract to execute methods and modify state in the context of the proxy contract
  • The file tests/ManagerProxy.js runs through a simple scenario of setting/getting storage types that are 32 bytes or less and swaps out an old version of a target contract with a new version
  • Added Minter.sol which owns the LivepeerToken contracts. When bonding or making deposits for jobs, tokens are transferred to this Minter contract. The Minter can mint new tokens if it is called by the BondingManager. The Minter can also transfer tokens in its possession to a recipient if called by the BondingManager or JobsManager
  • Register all contracts with the controller so that a user can always get the most up to date version of a contract
  • Minter.sol and LivepeerToken.sol are not proxy target contracts and are not meant to be upgradeable
  • Cleaned up the test suite a little bit so we actually have unit tests that rely on mocked contracts. I commented out the integration test for now because I'm still trying to figure out the best strategy for that - ideally we can run our integration tests on any chain/client and not rely solely on TestRPC features (i.e. mine a block per tx, ability to fast forward time/blocks)

I'm going to leave some clarifying comments, but there is a lot in this one so it might be easier to review by reading over it once and then we can sit down and run through it

@yondonfu yondonfu requested review from dob and ericxtang Sep 21, 2017

import "./ManagerProxyTarget.sol";
contract ManagerProxy is ManagerProxyTarget {

This comment has been minimized.

@yondonfu

yondonfu Sep 21, 2017

Member

The proxy contract inherits from the ManagerProxyTarget base contract to ensure that it starts off with the same storage layout as the target contract

@yondonfu

yondonfu Sep 21, 2017

Member

The proxy contract inherits from the ManagerProxyTarget base contract to ensure that it starts off with the same storage layout as the target contract

token = LivepeerToken(_token);
function BondingManager(address _controller) Manager(_controller) {}
function initialize(uint64 _unbondingPeriod, uint256 _numActiveTranscoders) external beforeInitialization returns (bool) {

This comment has been minimized.

@yondonfu

yondonfu Sep 21, 2017

Member

When we instantiate a target contract using its constructor we update the storage of the target contract. However, we want to update the storage of the proxy contract. So, the logic of the constructor is now moved into this initialize function that the proxy contract can call and execute it its own context to modify the storage of the proxy contract.

@yondonfu

yondonfu Sep 21, 2017

Member

When we instantiate a target contract using its constructor we update the storage of the target contract. However, we want to update the storage of the proxy contract. So, the logic of the constructor is now moved into this initialize function that the proxy contract can call and execute it its own context to modify the storage of the proxy contract.

function BondingManager(address _controller) Manager(_controller) {}
function initialize(uint64 _unbondingPeriod, uint256 _numActiveTranscoders) external beforeInitialization returns (bool) {
finishInitialization();

This comment has been minimized.

@yondonfu

yondonfu Sep 21, 2017

Member

It's important to call this to set the initialized flag (inherited from Initializable) to true so that the initialize method cannot be called more than once

@yondonfu

yondonfu Sep 21, 2017

Member

It's important to call this to set the initialized flag (inherited from Initializable) to true so that the initialize method cannot be called more than once

@@ -441,7 +430,7 @@ contract BondingManager is IBondingManager, Manager {
* Returns address of elected active transcoder and its price per segment
* @param _maxPricePerSegment Max price (in LPT base units) per segment of a stream
*/
function electActiveTranscoder(uint256 _maxPricePerSegment) external constant returns (address, uint256) {
function electActiveTranscoder(uint256 _maxPricePerSegment) external constant returns (address) {

This comment has been minimized.

@yondonfu

yondonfu Sep 21, 2017

Member

I changed this to just return the address of the elected transcoder. Originally it returned the address of the elected transcoder and the transcoder's price. It was returning the price before because the default behavior was to use the transcoder's price which might be lower than the max price per segment set by a broadcaster when calling job from the JobsManager.

There were some points made here #55 that I thought made sense. Defaulting to the max price per segment set by a broadcaster would mean we do not need to return the transcoder's price here. This also helps with using a proxy contract to return values when using the BondingManager as a target contract - the proxy contract cannot return tuples right now. Since we are now just returning a single address the proxy contract can return that value just fine

@yondonfu

yondonfu Sep 21, 2017

Member

I changed this to just return the address of the elected transcoder. Originally it returned the address of the elected transcoder and the transcoder's price. It was returning the price before because the default behavior was to use the transcoder's price which might be lower than the max price per segment set by a broadcaster when calling job from the JobsManager.

There were some points made here #55 that I thought made sense. Defaulting to the max price per segment set by a broadcaster would mean we do not need to return the transcoder's price here. This also helps with using a proxy contract to return values when using the BondingManager as a target contract - the proxy contract cannot return tuples right now. Since we are now just returning a single address the proxy contract can return that value just fine

This comment has been minimized.

@dob

dob Sep 22, 2017

Member

Agree that just using the maxPricePerSegment is better since it's essentially the broadcaster indicating their willingness to pay a certain price and have a higher probability of being matched with an available transcoder.

@dob

dob Sep 22, 2017

Member

Agree that just using the maxPricePerSegment is better since it's essentially the broadcaster indicating their willingness to pay a certain price and have a higher probability of being matched with an available transcoder.

return initialTokenSupply.mul(initialYearlyInflation).div(100).div(roundsManager().roundsPerYear()).mul(transcoderActiveStake).div(totalActiveTranscoderStake);
// Transcoder getters
function getTranscoderDelegatorWithdrawRound(address _transcoder) public constant returns (uint256) {

This comment has been minimized.

@yondonfu

yondonfu Sep 21, 2017

Member

We need these getters to individually retrieve values for a transcoder or delegator struct (the proxy contract cannot return tuples at the moment)

@yondonfu

yondonfu Sep 21, 2017

Member

We need these getters to individually retrieve values for a transcoder or delegator struct (the proxy contract cannot return tuples at the moment)

This comment has been minimized.

@dob

dob Sep 22, 2017

Member

Any indication on the performance hit of querying individually for each field vs being able to return a struct directly? To populate the transcoder table with all it's accompanying data could take 100s of calls.

@dob

dob Sep 22, 2017

Member

Any indication on the performance hit of querying individually for each field vs being able to return a struct directly? To populate the transcoder table with all it's accompanying data could take 100s of calls.

This comment has been minimized.

@yondonfu

yondonfu Sep 22, 2017

Member

Not sure, I'll make a note to test this out. Since constant getters are using the eth_call RPC request for a node maybe we can batch together the RPC requests to make it faster

@yondonfu

yondonfu Sep 22, 2017

Member

Not sure, I'll make a note to test this out. Since constant getters are using the eth_call RPC request for a node maybe we can batch together the RPC requests to make it faster

@@ -613,6 +658,7 @@ contract BondingManager is IBondingManager, Manager {
*/
function increaseTranscoderStake(address _transcoder, uint256 _totalAmount, uint256 _transcoderShare, uint256 _round) internal returns (bool) {
delegators[_transcoder].bondedAmount = delegators[_transcoder].bondedAmount.add(_transcoderShare);
delegators[_transcoder].delegatedAmount = delegators[_transcoder].delegatedAmount.add(_totalAmount);

This comment has been minimized.

@yondonfu

yondonfu Sep 21, 2017

Member

This was not updated properly before. When we increase a transcoder's stake we should not only increase the transcoder's stake in the transcoder pools, but also update the total amount delegated to it

@yondonfu

yondonfu Sep 21, 2017

Member

This was not updated properly before. When we increase a transcoder's stake we should not only increase the transcoder's stake in the transcoder pools, but also update the total amount delegated to it

return true;
}
/*
* @dev Withdraw deposited funds
* FIXME: Attacker can withdraw funds before transcoder has a chance to claim them

This comment has been minimized.

@yondonfu

yondonfu Sep 21, 2017

Member

Going to need logic to make sure this method is not used to withdraw funds before a transcoder has a chance to claim them. Will track with an issue

@yondonfu

yondonfu Sep 21, 2017

Member

Going to need logic to make sure this method is not used to withdraw funds before a transcoder has a chance to claim them. Will track with an issue

job.broadcasterAddress = msg.sender;
job.transcoderAddress = electedTranscoder;
NewJob(electedTranscoder, msg.sender, numJobs);
NewJob(electedTranscoder, msg.sender, numJobs, _streamId, _transcodingOptions);

This comment has been minimized.

@yondonfu

yondonfu Sep 21, 2017

Member

We currently cannot return streamId or transcodingOptions when using a proxy contract because the delegatecall mechanism cannot return dynamic length types (i.e. strings). Instead we can include these fields in events and a node could retrieve the values from the event

@yondonfu

yondonfu Sep 21, 2017

Member

We currently cannot return streamId or transcodingOptions when using a proxy contract because the delegatecall mechanism cannot return dynamic length types (i.e. strings). Instead we can include these fields in events and a node could retrieve the values from the event

}
// Based on https://github.com/AugurProject/augur-core/blob/develop/src/libraries/Delegator.sol
function() public payable {

This comment has been minimized.

@ericxtang

ericxtang Sep 21, 2017

Member

Dose this make every call payable?

@ericxtang

ericxtang Sep 21, 2017

Member

Dose this make every call payable?

This comment has been minimized.

@yondonfu

yondonfu Sep 22, 2017

Member

Discussed offline and the fallback function needs to be payable for the case when a user is calling the verify() function in JobsManager which is payable. If we really wanted to prevent anyone from accidentally sending ether to this contract except for when verify() is called, we could add a check for the method signature and if it is not the signature for verify() and msg.value > 0 we throw

@yondonfu

yondonfu Sep 22, 2017

Member

Discussed offline and the fallback function needs to be payable for the case when a user is calling the verify() function in JobsManager which is payable. If we really wanted to prevent anyone from accidentally sending ether to this contract except for when verify() is called, we could add a check for the method signature and if it is not the signature for verify() and msg.value > 0 we throw

@dob

Overall it seems like this approach gives us the benefit of being able to easily upgrade contract logic, at the expense of hacking around solidity and the EVM in the following ways:

  • Requiring getters and a single read for each property of a struct.
  • Needing to read certain dynamic values from LOGs instead of the chain (which may go away with Byzantium).
  • Dropping down to assembly and delegating calls from proxy to target contracts. Not that this is explicity unsafe, but we're fiddling around with memory and addresses where we place call data and read return values.
  • Needing to keep storage values in the same order within contracts as we add new ones during upgrades. This is potentially prone to bugs, programmer error, or even breakage due to underlying tools or EVM updates.

I think that, especially due to this last point, the way to look at this is: it gives us the ability to upgrade if we need to using this mechanism for now...but we may not be able to rely on it for all cases and forever into the future.

@@ -441,7 +430,7 @@ contract BondingManager is IBondingManager, Manager {
* Returns address of elected active transcoder and its price per segment
* @param _maxPricePerSegment Max price (in LPT base units) per segment of a stream
*/
function electActiveTranscoder(uint256 _maxPricePerSegment) external constant returns (address, uint256) {
function electActiveTranscoder(uint256 _maxPricePerSegment) external constant returns (address) {

This comment has been minimized.

@dob

dob Sep 22, 2017

Member

Agree that just using the maxPricePerSegment is better since it's essentially the broadcaster indicating their willingness to pay a certain price and have a higher probability of being matched with an available transcoder.

@dob

dob Sep 22, 2017

Member

Agree that just using the maxPricePerSegment is better since it's essentially the broadcaster indicating their willingness to pay a certain price and have a higher probability of being matched with an available transcoder.

return initialTokenSupply.mul(initialYearlyInflation).div(100).div(roundsManager().roundsPerYear()).mul(transcoderActiveStake).div(totalActiveTranscoderStake);
// Transcoder getters
function getTranscoderDelegatorWithdrawRound(address _transcoder) public constant returns (uint256) {

This comment has been minimized.

@dob

dob Sep 22, 2017

Member

Any indication on the performance hit of querying individually for each field vs being able to return a struct directly? To populate the transcoder table with all it's accompanying data could take 100s of calls.

@dob

dob Sep 22, 2017

Member

Any indication on the performance hit of querying individually for each field vs being able to return a struct directly? To populate the transcoder table with all it's accompanying data could take 100s of calls.

// Mint token reward and allocate to this protocol contract
token.mint(this, mintedTokens);
// Mint token reward
uint256 mintedTokens = minter().mint(activeTranscoders[activeTranscoderPositions[msg.sender]].key, totalActiveTranscoderStake);

This comment has been minimized.

@dob

dob Sep 22, 2017

Member

what's the reason for getting rid of mintedTokensPerReward()?

@dob

dob Sep 22, 2017

Member

what's the reason for getting rid of mintedTokensPerReward()?

This comment has been minimized.

@yondonfu

yondonfu Sep 22, 2017

Member

With the addition of the Minter contract I thought it might make sense to move the logic for calculating how many new tokens to mint into the Minter contract. Now, the BondingManager tells the Minter what the current transcoder's stake and the total active transcoder stake is. The Minter then uses that information to decide how many tokens to mint.

@yondonfu

yondonfu Sep 22, 2017

Member

With the addition of the Minter contract I thought it might make sense to move the logic for calculating how many new tokens to mint into the Minter contract. Now, the BondingManager tells the Minter what the current transcoder's stake and the total active transcoder stake is. The Minter then uses that information to decide how many tokens to mint.

@yondonfu

This comment has been minimized.

Show comment
Hide comment
@yondonfu

yondonfu Sep 22, 2017

Member

Agreed on how to look at the upgradeability related additions in this PR. I think it makes sense to have a working version of this approach in this codebase for now and press on with other tasks knowing that we have a basic upgradeability solution to work with. And perhaps we will get more information in time (i.e. feedback, Byzantium on the mainnet, Solidity development) that changes how we look at this solution (for better or for worse)

Member

yondonfu commented Sep 22, 2017

Agreed on how to look at the upgradeability related additions in this PR. I think it makes sense to have a working version of this approach in this codebase for now and press on with other tasks knowing that we have a basic upgradeability solution to work with. And perhaps we will get more information in time (i.e. feedback, Byzantium on the mainnet, Solidity development) that changes how we look at this solution (for better or for worse)

@yondonfu yondonfu merged commit 70e22b0 into develop Sep 28, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@yondonfu yondonfu deleted the yf/upgradeability branch Sep 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment