Skip to content
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

Improved role staking #42

Merged
merged 7 commits into from Apr 17, 2019

Conversation

mnaamani
Copy link
Contributor

Here is my proposal for improving the staking mechanism for the Actors/Roles.

Locked amount. We were locking the entire balance in the actor account.

The amount locked is now only the minimum stake required to enter role. Unlike the validators set, currently there is no elaborate selection algorithm for who makes it into the set of actors for a specific role, only criteria is staking minimum amount and there being an empty slot. (In future a dedicated working group will have responsibility of managing this set) So there is no need to lock the entire account balance, or even an advantage for a member to add more than minimum stake to their actor account.

Actor account can still withdraw for following reasons: TransactionPayment, and Fee. This allows for covering transaction fees for any extrinsic sent by actor account. The account cannot transfer balance or reserve while it is active in its role or unbonding.

Rewards are paid out to actor account if its balance falls below the minimum stake requirement. (This will happen for two reasons, spending on tx fees and if the minimum stake is increased). Otherwise it goes to the member account.

Copy link
Contributor

@jfinkhaeuser jfinkhaeuser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right direction to go, but I think I also identified a bunch of edge cases that need covering. Maybe not all need a solution right now, but some should IMHO be addressed.

src/roles/actors.rs Show resolved Hide resolved
src/roles/actors.rs Outdated Show resolved Hide resolved
// send reward to member account - not the actor account
if let Ok(member_account) = T::Members::lookup_account_by_member_id(actor.member_id) {
let _ = T::Currency::deposit_into_existing(&member_account, params.reward);
// reward can top up balance if it is below minimum stake requirement
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the right change, but for two comments:

  1. The reward must still exceed the balance used up during the lifetime of a block for anything to ever be paid out to the member and to guarantee that the actor can continue acting. I'm not sure this is enough, therefore.
  2. The effect of this is that busy actors actually produce less reward for their member than idle actors. I think that's the opposite of what's intended.

Maybe it's enough for getting Athens up and running, because the storage provider incentives are really only properly to be added in future. But it really doesn't seem entirely complete at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed IRL, these edge cases are correct, but would have to be solved by the working group in selecting the best set of role parameters to ensure storage providers are earning more than than their tx fees, as well as introducing a reward model that takes into account how busy the actors are.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern for Athens is really that storage providers don't run out of balance for transaction fees - I guess it's okay for now to require people to pay attention.

src/roles/actors.rs Show resolved Hide resolved
@jfinkhaeuser
Copy link
Contributor

Ok, after the discussion, I think it's ok to move forward. I'd change the flags setting code just to be a bit safer, but otherwise let's merge this PR :)

@mnaamani
Copy link
Contributor Author

Updated flag setting code and tested locally works as expected. This is good to merge

@mnaamani mnaamani added this to In progress in Athens Release Apr 17, 2019
@mnaamani mnaamani merged commit 5d9448f into Joystream:development Apr 17, 2019
@mnaamani mnaamani moved this from In progress to Done in Athens Release Apr 18, 2019
mnaamani added a commit to mnaamani/joystream that referenced this pull request Mar 20, 2020
Lezek123 pushed a commit to Lezek123/substrate-runtime-joystream that referenced this pull request May 21, 2020
* Be explicit in supported input types

* label as React$Node
@mnaamani mnaamani deleted the improved-role-staking branch May 22, 2020 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants