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

WIP: Staking Specification #465

Merged
merged 1 commit into from
Mar 20, 2018
Merged

WIP: Staking Specification #465

merged 1 commit into from
Mar 20, 2018

Conversation

milosevic
Copy link

This part is ready for review @rigelrozanski @ebuchman.

It does not contains yet fee-related logic, full unbonding of a candidate, liveliness transactions and stashing logic. We also haven't defined yet withdrawal logic. These should be addressed in the following PRs.

Please review the whole technical-spec and examples document, not just the content of this PR, as the previous version was merged to develop without review process.

@codecov
Copy link

codecov bot commented Feb 15, 2018

Codecov Report

Merging #465 into develop will increase coverage by 0.13%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #465      +/-   ##
===========================================
+ Coverage    62.32%   62.45%   +0.13%     
===========================================
  Files           33       33              
  Lines         1818     1814       -4     
===========================================
  Hits          1133     1133              
+ Misses         596      593       -3     
+ Partials        89       88       -1


Validator provisions are minted on an hourly basis (the first block of a new
hour). The annual target of between 7% and 20%. The long-term target ratio of
bonded tokens to unbonded tokens is 67%.
Copy link
Contributor

Choose a reason for hiding this comment

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

Confusing, there are two concepts here.

  1. Inflation, which has No target, it is a moving value between 7% and 20% depending on how close the global staked value of atoms is 67%
  2. Target % Bonded Atoms - This is the 67% number

It's confusing to have these sentences one after the other, should distinguish these two percentages a bit more

hour). The annual target of between 7% and 20%. The long-term target ratio of
bonded tokens to unbonded tokens is 67%.

The target annual inflation rate is recalculated for each previsions cycle. The
Copy link
Contributor

Choose a reason for hiding this comment

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

again the inflation rate is not a target, just a moving amount, change language throughout

inflation is also subject to a rate change (positive of negative) depending or
the distance from the desired ratio (67%). The maximum rate change possible is
defined to be 13% per year, however the annual inflation is capped as between
7% and 20%.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also note that because its 13% per year, it would take an entire year for the inflation to move from its minimum threshold to it's maximum (that is how we came up with the 13% rate)

- PubKey: separated key from the owner of the candidate as is used strictly
for participating in consensus.
- Status: it can be Bonded (active validator), Unbonded (validator candidate) or Revoked
- PubKey: candidate public key that is used strictly for participating in consensus
Copy link
Contributor

Choose a reason for hiding this comment

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

The PubKey is also used for identification when delegating atoms (you delegate to a PubKey)

either unbonding or unbonded.
- PubKey: separated key from the owner of the candidate as is used strictly
for participating in consensus.
- Status: it can be Bonded (active validator), Unbonded (validator candidate) or Revoked
Copy link
Contributor

Choose a reason for hiding this comment

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

Status can also be unbonding to either revoked or unbonded

@@ -113,7 +129,7 @@ Candidate parameters are described:
- RedelegatingShares: The portion of `IssuedDelegatorShares` which are
currently re-delegating to a new validator
- VotingPower: Proportional to the amount of bonded tokens which the validator
has if the candidate is a validator.
has if `Candidate.Status` is `Bonded`; otherwise it is equal to `0`
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe confused you with of my own errors here.... Anyways the status should never be equal to 0, should always be set to one of the status options (defined in the code, check it out)

difference (amount that should have been slashed from the first validator) is
assigned to the amount being paid out.

TODO: Explain what is StartSlashRation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please explain it here ... also it's not a Ration :P

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a lot of the text you removed right above here explains this too... not sure why you removed this text!... maybe come up with something better based off of old text?

Shares rational.Rat // amount of shares which are unbonding
NewCandidate crypto.PubKey // validator to bond to after unbond
}
```
Q: Why we need Payout address in the re-delegation element?
Copy link
Contributor

@rigelrozanski rigelrozanski Feb 19, 2018

Choose a reason for hiding this comment

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

I think the payout address is the address to re-delegate to... Maybe we should rename to something like ReceivingDelegation

structure from the store, and `saveCandidate(store, candidate)` to save it. Similarly, we use
`loadDelegatorBond(store, sender, PubKey)` to load delegator bond with the key (sender and PubKey) from the store, and
`saveDelegatorBond(store, sender, bond)` to save it. `removeDelegatorBond(store, sender, bond)` is used to remove the
bond from the store.
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is good, but maybe we should organize into a list?

A validator candidacy can be declared using the `TxDeclareCandidacy` transaction.
During this transaction a self-delegation transaction is executed to bond
tokens which are sent in with the transaction (TODO: What does this mean?).
A validator candidacy is declared using the `TxDeclareCandidacy` transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't want to mention that a candidate effectively also has a self delegation? I think this concept is important - it informs others how a candidate can send more tokens to themselves/withdraw

// TODO: Add logic regarding startSlashRatio
unbondDelegationQueue.add(unbondDelegationElem)

TODO: Figure out where we should move coins after they are unbonded as they should stay somewhere during UnbondingPeriod
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose that we have a new global store named unbonding which should make everything much more clear

7% and 20%.
if bond.Shares < tx.Shares return
//TODO: How we can enforce that the same bond can't be redelegated? Should we split the current bond into two so
// we have a special bond for redelegation?
Copy link
Contributor

Choose a reason for hiding this comment

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

When you are redegating, the shares stay with the original validator until the queue has completed... this way it is impossible to have multiple re-delegations... resolves your question?

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Did you add any new pseudo code that doesn't reflect old pseudo code? if so let me know, I didn't review all the pseudo code in detail

@adrianbrink adrianbrink changed the title Add reDelegation and tick logic and other improvements of the spec WIP: Staking Specification Feb 26, 2018
Copy link
Contributor

@adrianbrink adrianbrink left a comment

Choose a reason for hiding this comment

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

Overall I really like all the details and the amount of pseudocode.

I think we should try to slim it down with regards to text. Also, let's try and find a sensible structure in which to break this up, so that it doesn't all live in the same file.

@@ -38,7 +38,7 @@

At the core of the Staking module is the concept of a pool which denotes
collection of Atoms contributed by different Atom holders. There are two global
pools in the Staking module: the bonded pool and unbonded pool. Bonded Atoms
pools in the Staking module: the bonded pool and unbonding pool. Bonded Atoms
Copy link
Author

Choose a reason for hiding this comment

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

I believe it is really unbonded pool. Unbonding pool is something different that we will also probably introduce.

Copy link
Member

Choose a reason for hiding this comment

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

Yeh this is definitely wrong.

We should probably stop pushing to each others branches to avoid these kinds of things @adrianbrink @ebuchman

But calling it "unbonded" is misleading too since it really is bonded it's just not with an active validator.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm okay - let's come up with some better language - because in this case it is technically "unbonded" because it's no longer slashable - however it can be rebonded by that candidate if it becomes a validator - It's in Limbo Land

@milosevic
Copy link
Author

This part of the spec is ready for review and it could be merged into develop. Missing features should come with new (smaller) PRs so it's easier to review.
@ebuchman @rigelrozanski

@ebuchman ebuchman dismissed rigelrozanski’s stale review March 20, 2018 00:14

we'll keep working on it

@ebuchman ebuchman merged commit 4bfa40a into develop Mar 20, 2018
@ebuchman ebuchman deleted the improve_staking_spec branch March 20, 2018 00:15
kocubinski pushed a commit that referenced this pull request Jul 4, 2023
Co-authored-by: Dev Ojha <[email protected]>
(cherry picked from commit 4c20864)

Co-authored-by: Supanat <[email protected]>
Neoplayer pushed a commit to Neoplayer/cosmos-sdk that referenced this pull request Jul 18, 2023
…mos#465) (cosmos#476)

* Reemitting events from cache context in gov endblockers (cosmos#465)

Co-authored-by: Dev Ojha <[email protected]>
(cherry picked from commit 4c20864)

* updates

---------

Co-authored-by: Supanat <[email protected]>
Co-authored-by: Roman <[email protected]>
kocubinski pushed a commit that referenced this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants