R4R remove global shares #1644

rigelrozanski · 2018-07-11T18:50:30Z

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md
Updated Gaia/Examples
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
Added appropriate labels to PR (ex. wip, ready-for-review, docs)

…hares

cwgoes

Reviewed quickly, looks vastly simplified, a few tiny suggestions.

cwgoes · 2018-07-12T21:25:15Z

types/rational.go

@@ -252,3 +252,11 @@ func RatsEqual(r1s, r2s []Rat) bool {
 func RatEq(t *testing.T, exp, got Rat) (*testing.T, bool, string, Rat, Rat) {
 	return t, exp.Equal(got), "expected:\t%v\ngot:\t\t%v", exp, got
 }
+
+// test if two rat arrays are the equal


Return the lesser of two rats?

updated to minimum rational between two

cwgoes · 2018-07-12T22:22:04Z

x/stake/types/pool.go

-	p.BondedTokens += amount
-	p.LooseTokens -= amount
-	if p.LooseTokens < 0 {
+func (p Pool) addBondedTokens(bondedTokens sdk.Rat) Pool {


Rename looseTokensToBonded perhaps?

yeah that's more concise - good idea - changed

cwgoes · 2018-07-12T22:22:19Z

x/stake/types/pool.go

-	p.BondedTokens -= removedTokens
-	p.LooseTokens += removedTokens
-	if p.UnbondedTokens < 0 {
+func (p Pool) removeBondedTokens(bondedTokens sdk.Rat) Pool {


Renamed bondedTokensToLoose perhaps?

rigelrozanski · 2018-07-12T22:46:37Z

The "don't get your spot back for equal heights and tokens" test is failing under x/stake/keeper/validator_test.go

cwgoes · 2018-07-13T00:02:03Z

I also get:

--- FAIL: TestSlashWithRedelegation (0.01s)
	Error Trace:	slash_test.go:361
	Error:      	func (assert.PanicTestFunc)(0x9bae00) should not panic
	Panic value:	sanity check: bonded tokens negative, pool: {23/1 -1/1 0 7/100 0 0/1}
	Test:       	TestSlashWithRedelegation

cwgoes · 2018-07-13T00:45:46Z

x/stake/types/validator.go

-	issuedDelegatorShares := equivalentBondedShares.Quo(exRate)
-	v.DelegatorShares = v.DelegatorShares.Add(issuedDelegatorShares)
+	v.Tokens = v.Tokens.Add(sdk.NewRat(amount))
+	issuedShares := v.Tokens.Quo(exRate)


@rigelrozanski I think this line is the problem - computing v.Tokens.Quo(exRate) (all shares), but I think we want sdk.NewRat(amount).Quo(exRate) (newly added shares)

Great, thanks for resolving

…nge rate calculation

codecov · 2018-07-13T07:05:55Z

Codecov Report

Merging #1644 into master will decrease coverage by 0.3%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #1644      +/-   ##
==========================================
- Coverage   62.83%   62.53%   -0.31%     
==========================================
  Files         124      123       -1     
  Lines        6998     6995       -3     
==========================================
- Hits         4397     4374      -23     
- Misses       2349     2370      +21     
+ Partials      252      251       -1

jbibla

approving changes to state.md. let's revise the approval process sooner rather than later 😄

cwgoes

A few minor comments

cwgoes · 2018-07-13T17:41:21Z

docs/spec/staking/state.md

-validator shares for stake in the global pools, moving Atom inflation
-information, etc.
+information about the total amounts of Atoms in all states, moving Atom
+inflation information, etc.

 ```golang
 type Pool struct {
    LooseTokens int64   // tokens not associated with any validator


LooseTokens is now tokens not associated with any bonded validator, right? (it includes tokens associated with *unbonded validators)

Correct - just updated the comment to reflect this

cwgoes · 2018-07-13T17:42:09Z

docs/spec/staking/state.md

@@ -85,7 +74,9 @@ type Validator struct {
    ConsensusPubKey crypto.PubKey  // Tendermint consensus pubkey of validator
    Revoked         bool           // has the validator been revoked?

-    PoolShares      PoolShares     // total shares for tokens held in the pool
+    PoolShares      sdk.BondStatus // total shares for tokens held in the pool


Should this line be removed?

Yes, removed

cwgoes · 2018-07-13T17:48:52Z

x/stake/types/validator.go

-		pool, poolShares = pool.addTokensUnbonding(amount)
-	case sdk.Bonded:
-		pool, poolShares = pool.addTokensBonded(amount)
+	if v.Status == sdk.Bonded {


amountRat can be declared a line earlier to avoid the extra sdk.Rat allocation

cwgoes · 2018-07-13T17:49:13Z

x/stake/types/validator.go


-	return v, pool, issuedDelegatorShares
+	return v, pool, issuedShares
 }

 // RemoveDelShares removes delegator shares from a validator.
 //
 // NOTE: This function assumes the shares have already been updated for the


Comment no longer applicable I think?

correct - removed

cwgoes

Second pass

cwgoes · 2018-07-13T18:05:18Z

x/stake/handler.go


 	// Process types.Validator Provisions
 	blockTime := ctx.BlockHeader().Time
 	if pool.InflationLastTime+blockTime >= 3600 {
 		pool.InflationLastTime = blockTime
-		pool = k.ProcessProvisions(ctx)
+		pool.ProcessProvisions(params)


Do we need to capture the returned Pool here?

yes, THANK YOU - added

cwgoes · 2018-07-13T18:07:18Z

x/stake/keeper/slash.go

 	}

 	// Cannot decrease balance below zero
-	sharesToRemove := sdk.MinInt(remainingSlashAmount, validator.PoolShares.Amount.RoundInt())
+	burned := sdk.MinRat(remainingSlashAmount, validator.Tokens)


A bit confusing; maybe tokensToBurn? They aren't burned until a few lines later

okay - added

cwgoes · 2018-07-13T18:08:45Z

x/stake/types/pool.go

 }

 //____________________________________________________________________

 // get the bond ratio of the global state
 func (p Pool) BondedRatio() sdk.Rat {
-	if p.TokenSupply() > 0 {
-		return sdk.NewRat(p.BondedTokens, p.TokenSupply())
+	if p.TokenSupply().GT(sdk.ZeroRat()) {


supply = p.TokenSupply(), avoid calculating twice

cwgoes · 2018-07-13T18:09:30Z

x/stake/types/pool.go

+	p.BondedTokens = p.BondedTokens.Sub(bondedTokens)
+	p.LooseTokens = p.LooseTokens.Add(bondedTokens)
+	if p.BondedTokens.LT(sdk.ZeroRat()) {
+		panic(fmt.Sprintf("sanity check: bonded tokens negative, pool: %v", p))


Maybe we should have a sdk.URat type (not for this PR)

yeah could maybe be good

cwgoes · 2018-07-13T18:15:47Z

x/stake/types/validator.go

 		}
-		pool, tokens = pool.removeSharesUnbonded(v.PoolShares.Amount)
-
 	case sdk.Unbonding:


Do we ever use this status (sdk.Unbonding)? I only see it in validator_test.go and I'm not clear why we need it in the model; unbonding delegations / redelegations seem to provide the requisite tracking

Also see #1673

Incorrect - we need this status for when an entire validator begins unbonding (with it's delegators still attached) because it has been revoked - opening an issue to verify that this is happening

rigelrozanski · 2018-07-13T19:23:48Z

Addressed comments - if there is anything else small - feel free to push to this branch

cwgoes

Tested ACK

wip removing pool shares

2bf78d9

rigelrozanski added C:x/staking wip labels Jul 11, 2018

rigelrozanski requested review from cwgoes and ebuchman as code owners July 11, 2018 18:50

rigelrozanski added 11 commits July 11, 2018 17:48

Merge branch 'rigel/clearer-staking-logic' into rigel/remove-global-s…

b24b537

…hares

remove PoolShares/Tokens entirely

53afa92

worked through stake/type compile error

d1270ce

work through a bunch of keeper errors

8252e47

worked through compile errors

4563fb7

debugging tests

1daa854

resolve compilation error

cc754a8

resolved types errors

68946e6

...

f2eb305

move inflation to pool type

a0ab1d6

...

b5ba334

cwgoes reviewed Jul 12, 2018

View reviewed changes

stumped problem

885615f

Merge branch 'master' into rigel/remove-global-shares

d487b56

cwgoes reviewed Jul 13, 2018

View reviewed changes

cwgoes and others added 6 commits July 13, 2018 02:47

Calculate newly issued shares, remove unnecessary pool arg from excha…

244d1ba

…nge rate calculation

Rounding changed

3a5a396

Update x/slashing tests for sdk.Rat BondedTokens

40c8bcb

testing fixes

f448183

resolved test fixes

ef11205

cwgoes comments, changelog, lint

00bbef0

cli bugfixes

754baa7

rigelrozanski added 3 commits July 13, 2018 03:40

..

a97101d

cli fixed

b3111bc

spec update

1565645

rigelrozanski requested review from jbibla and zramsay as code owners July 13, 2018 07:54

rigelrozanski added the ready-for-review label Jul 13, 2018

rigelrozanski changed the title ~~WIP remove global shares~~ R4R remove global shares Jul 13, 2018

rigelrozanski removed the wip label Jul 13, 2018

jbibla approved these changes Jul 13, 2018

View reviewed changes

Merge branch 'master' into rigel/remove-global-shares

257ec17

cwgoes reviewed Jul 13, 2018

View reviewed changes

'make format'

54c0bdb

cwgoes suggested changes Jul 13, 2018

View reviewed changes

cwgoes comments

813087a

cwgoes approved these changes Jul 13, 2018

View reviewed changes

Merge 'master' into rigel/remove-global-shares

597dbfc

cwgoes force-pushed the rigel/remove-global-shares branch from 9ea3b6a to 597dbfc Compare July 13, 2018 19:40

Increase test_cover parallelism

f8d72be

ebuchman merged commit 3231daa into master Jul 13, 2018

ebuchman deleted the rigel/remove-global-shares branch July 13, 2018 20:46

R4R remove global shares #1644

R4R remove global shares #1644

Conversation

rigelrozanski commented Jul 11, 2018 • edited Loading

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski commented Jul 12, 2018

cwgoes commented Jul 13, 2018

Choose a reason for hiding this comment

rigelrozanski Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 13, 2018 • edited Loading

Codecov Report

jbibla left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski commented Jul 13, 2018

cwgoes left a comment

Choose a reason for hiding this comment

rigelrozanski commented Jul 11, 2018 •

edited

Loading

rigelrozanski Jul 13, 2018 •

edited

Loading

codecov bot commented Jul 13, 2018 •

edited

Loading