livepeer / LIPs

Livepeer Improvement Proposals
9 stars 13 forks source link

Transcoder withdrawal address #4

Open yondonfu opened 6 years ago

yondonfu commented 6 years ago

At the moment, transcoders use a single address for the following purposes:

Transcoders can instead use one address as their identifier address (so that contract functions can validate that the transcoder is the sender) and use one address as their withdrawal address.

There are a number of benefits that come with allowing a transcoder to specify a withdrawal address that is distinct from its identifier address

From an implementation point of view this would require an additional withdrawalAddress in the transcoder struct in the BondingManager contract. This value of withdrawalAddress could be specified when invoking the BondingManager.transcoder() function. (One note here is that the BondingManager.transcoder() would become even more overloaded in this case because it is used not only to register as a transcoder, but also for changing rates and now setting a withdrawal address. It might be worth splitting up the function into two separate functions, one for registering with a withdrawal address and one for setting rates).

BondingManager.withdrawStake() would check if the sender has a withdrawalAddress as a transcoder - if so, the function will ask the Minter to transfer LPT rewards to withdrawalAddress, if not the function will ask the Minter to transfer LPT rewards to the sender address. BondingManager.withdrawFees() would work similarly.

BondingManager is delegate proxied meaning any changes during an upgrade will have to be storage layout compatible. At the moment, the addition of withdrawalAddress to the transcoder struct should be acceptable because the transcoder struct is only used in a mapping and not in any arrays and we would be adding an additional storage variable and not deleting or replacing any existing ones (we would need to make sure it comes after all existing variables in the transcoder struct!).

dob commented 6 years ago

Related concept: is there any merit in the "cold" address (withdrawl address), being able to submit a tx which replaces the "hot" address? The reason is that the hot address could be easily compromised...but if so then maybe only short term damage could be done if replaced quickly.

Don't want to derail the discussion around the above, but I guess this issue is a place for discussion before actually submitting the PR proposal.

j0sh commented 6 years ago

I like it.

Another note about overloading BondingManager.transcoder() . We might end up putting additional fields in there as well, eg a URI . Does this mean we should have a separate setter function for each field? Or is a more generalized approach possible without increasing gas too much?

yondonfu commented 6 years ago

is there any merit in the "cold" address (withdrawl address), being able to submit a tx which replaces the "hot" address? The reason is that the hot address could be easily compromised...but if so then maybe only short term damage could be done if replaced quickly

Another potentially simpler alternative to minimize damage would be to allow the withdrawal address to invoke the BondingManager.unbond() and BondingManager.withdraw() functions to get funds out of the contract. The complication here is that the protocol currently allows bonding from the unbonding state, so an attacker with access to the hot key could still force the transcoder to remain bonded by invoking BondingManager.bond(). A possible solution here is disallowing bonding from the unbonding state if you were a registered transcoder.

We might end up putting additional fields in there as well, eg a URI . Does this mean we should have a separate setter function for each field?

Not necessarily - there could be a single separate setter function for updating rates while BondingManager.transcoder() allows for registration as a transcoder in addition to setting the initial rates and withdrawal address. As for the URI, it would be nice if we can use a separate contract that supports lookups for additional metadata (e.g. custom ENS registry + resolver that would allow transcoders to register not only human readable names, but also metadata such as URIs) associated with an address (and the transcoder's identifying address would be the only address permitted to make updates) rather than including additional read only information in this existing contract