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

Separating transcoder candidacy from delegation #70

Merged
merged 3 commits into from Sep 11, 2017

Conversation

Projects
None yet
3 participants
@yondonfu
Member

yondonfu commented Sep 7, 2017

A spike to see how the code looks when transcoder candidacy is separated from delegation - opening PR to get some eyes on the structure, not meant to be considered for a merge yet. There are no updated tests yet.

  • bond can be called on anyone
  • If a user bonds to himself and calls transcoder, we try to add the user to the transcoder pools with his delegated stake. If a user is not bonded to himself and calls transcoder, we try to add the user to the transcoder pools with 0 stake
  • If a user resigns as a transcoder, he enters the Resigned state and the delegatorWithdrawRound field is set. When delegatorWithdrawRound is now or in the past, the user enters the NotRegistered state. A user can only re-register as a transcoder if he is in the NotRegistered state. This is to prevent a user from resigning and immediately re-registering
  • All bonded stake is tracked in the bondedAmount field of a Delegator struct
  • If a user is not a bonded delegator and reward or updateTranscoderFeePool is called, it becomes a bonded delegator and the appropriate values are set
  • Note that the changes in this spike allow for a user to be a registered transcoder and have bonded stake delegated to someone else

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

@dob

Even though there was not that much code removed relative to what was added, the reward calculations feel a little simpler to me now. How do you feel about it?

Any got-ya's that you ran into when pushing on this that we hadn't considered before, and that would make going with this implementation more difficult or more of a tradeoff?

@@ -29,9 +29,7 @@ contract BondingManager is IBondingManager, Manager {
// Represents a transcoder's current state
struct Transcoder {
address transcoderAddress; // The address of this transcoder

This comment has been minimized.

@dob

dob Sep 7, 2017

Member

why get rid of transcoderAddress? It's just used as the key in the mapping anyway so not necessary?

@dob

dob Sep 7, 2017

Member

why get rid of transcoderAddress? It's just used as the key in the mapping anyway so not necessary?

This comment has been minimized.

@yondonfu

yondonfu Sep 7, 2017

Member

Yeah having the address in the struct in addition to using as the key in the mapping didn't seem necessary and removing it also saves some gas since there is one less store op

@yondonfu

yondonfu Sep 7, 2017

Member

Yeah having the address in the struct in addition to using as the key in the mapping didn't seem necessary and removing it also saves some gas since there is one less store op

Show outdated Hide outdated contracts/bonding/BondingManager.sol
Show outdated Hide outdated contracts/bonding/BondingManager.sol
// Sender is neither a transcoder or delegator
revert();
}
delete delegators[msg.sender];

This comment has been minimized.

@dob

dob Sep 7, 2017

Member

Will this be a problem if someone withdraws, but then receives a reward or fee payout after the fact?

@dob

dob Sep 7, 2017

Member

Will this be a problem if someone withdraws, but then receives a reward or fee payout after the fact?

This comment has been minimized.

@yondonfu

yondonfu Sep 7, 2017

Member

If you are a delegator that unbonds and then withdraws, you shouldn't be eligible for any future rewards or fee payouts since you are no longer bonded to the transcoder. If you are a transcoder and you unbond/withdraw your stake, the next time you receive rewards or fee payouts, a delegator struct corresponding to your address will be populated with the correct amounts - essentially re-instantiating a bonded delegator

@yondonfu

yondonfu Sep 7, 2017

Member

If you are a delegator that unbonds and then withdraws, you shouldn't be eligible for any future rewards or fee payouts since you are no longer bonded to the transcoder. If you are a transcoder and you unbond/withdraw your stake, the next time you receive rewards or fee payouts, a delegator struct corresponding to your address will be populated with the correct amounts - essentially re-instantiating a bonded delegator

Show outdated Hide outdated contracts/bonding/BondingManager.sol
@yondonfu

This comment has been minimized.

Show comment
Hide comment
@yondonfu

yondonfu Sep 7, 2017

Member

I don't see any immediate issues with this change. However, I think it would be worth changing the name of the Delegator struct to something like Staker or Stakeholder for clarity. I think the name Delegator can be confusing when reading the code (re: Eric's comment about a delegator having to call reward). A user that actively participates in the protocol would be an on-chain stakeholder. On-chain stakeholders can delegate their bonded stake to any other stakeholders (we can refer to these stakeholders as delegators, but don't necessary need to explicitly name them delegators in the contract code).

Member

yondonfu commented Sep 7, 2017

I don't see any immediate issues with this change. However, I think it would be worth changing the name of the Delegator struct to something like Staker or Stakeholder for clarity. I think the name Delegator can be confusing when reading the code (re: Eric's comment about a delegator having to call reward). A user that actively participates in the protocol would be an on-chain stakeholder. On-chain stakeholders can delegate their bonded stake to any other stakeholders (we can refer to these stakeholders as delegators, but don't necessary need to explicitly name them delegators in the contract code).

@dob

This comment has been minimized.

Show comment
Hide comment
@dob

dob Sep 7, 2017

Member
Member

dob commented Sep 7, 2017

@yondonfu yondonfu changed the title from [SPIKE] Separating transcoder candidacy from delegation to Separating transcoder candidacy from delegation Sep 8, 2017

@yondonfu

This comment has been minimized.

Show comment
Hide comment
@yondonfu

yondonfu Sep 8, 2017

Member

Ready for another review

  • Anyone that participates in the protocol is a delegator and can delegate to any other delegator
  • delegatedAmount tracks the amount of stake other delegator have delegated to a particular delegator
  • A result from the above is that a user can now register as a transcoder with stake already delegated to him
  • Fixed failing tests
  • Did some refactoring of internal function logic to encapsulate code that updates transcoder stake in clearly named functions
Member

yondonfu commented Sep 8, 2017

Ready for another review

  • Anyone that participates in the protocol is a delegator and can delegate to any other delegator
  • delegatedAmount tracks the amount of stake other delegator have delegated to a particular delegator
  • A result from the above is that a user can now register as a transcoder with stake already delegated to him
  • Fixed failing tests
  • Did some refactoring of internal function logic to encapsulate code that updates transcoder stake in clearly named functions
@dob

dob approved these changes Sep 11, 2017

Looks good to me. 🚢

@yondonfu yondonfu merged commit 9d0336f into develop Sep 11, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@yondonfu yondonfu deleted the yf/transcoder-as-delegator branch Sep 11, 2017

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