From be9413517d36cc13d4640624aef2a84bb3ba553d Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Tue, 15 May 2018 09:06:56 -0400 Subject: [PATCH] fix UpdateSharesLocation and removeDelShares --- x/stake/handler.go | 14 ++++----- x/stake/keeper_test.go | 2 +- x/stake/pool.go | 2 +- x/stake/tick_test.go | 2 +- x/stake/validator.go | 60 +++++++++++++++++++++++++++------------ x/stake/validator_test.go | 23 ++++++++------- 6 files changed, 64 insertions(+), 39 deletions(-) diff --git a/x/stake/handler.go b/x/stake/handler.go index aac654f2bc..9e5774768f 100644 --- a/x/stake/handler.go +++ b/x/stake/handler.go @@ -162,7 +162,7 @@ func delegate(ctx sdk.Context, k Keeper, delegatorAddr sdk.Address, if err != nil { return nil, err } - validator, pool, newShares := validator.addTokens(pool, bondAmt.Amount) + validator, pool, newShares := validator.addTokensFromDel(pool, bondAmt.Amount) bond.Shares = bond.Shares.Add(newShares) // Update bond height @@ -186,7 +186,7 @@ func handleMsgUnbond(ctx sdk.Context, msg MsgUnbond, k Keeper) sdk.Result { return ErrInsufficientFunds(k.codespace).Result() } - var shares sdk.Rat + var delShares sdk.Rat // test that there are enough shares to unbond if msg.Shares == "MAX" { @@ -195,11 +195,11 @@ func handleMsgUnbond(ctx sdk.Context, msg MsgUnbond, k Keeper) sdk.Result { } } else { var err sdk.Error - shares, err = sdk.NewRatFromDecimal(msg.Shares) + delShares, err = sdk.NewRatFromDecimal(msg.Shares) if err != nil { return err.Result() } - if bond.Shares.LT(shares) { + if bond.Shares.LT(delShares) { return ErrNotEnoughBondShares(k.codespace, msg.Shares).Result() } } @@ -218,11 +218,11 @@ func handleMsgUnbond(ctx sdk.Context, msg MsgUnbond, k Keeper) sdk.Result { // retrieve the amount of bonds to remove (TODO remove redundancy already serialized) if msg.Shares == "MAX" { - shares = bond.Shares + delShares = bond.Shares } // subtract bond tokens from delegator bond - bond.Shares = bond.Shares.Sub(shares) + bond.Shares = bond.Shares.Sub(delShares) // remove the bond revokeCandidacy := false @@ -244,7 +244,7 @@ func handleMsgUnbond(ctx sdk.Context, msg MsgUnbond, k Keeper) sdk.Result { // Add the coins p := k.GetPool(ctx) - validator, p, returnAmount := validator.removeShares(p, shares) + validator, p, returnAmount := validator.removeDelShares(p, delShares) returnCoins := sdk.Coins{{k.GetParams(ctx).BondDenom, returnAmount}} k.coinKeeper.AddCoins(ctx, bond.DelegatorAddr, returnCoins) diff --git a/x/stake/keeper_test.go b/x/stake/keeper_test.go index 60ebec1da7..ff6432448f 100644 --- a/x/stake/keeper_test.go +++ b/x/stake/keeper_test.go @@ -34,7 +34,7 @@ func TestValidatorBasics(t *testing.T) { for i, amt := range amts { validators[i] = NewValidator(addrVals[i], pks[i], Description{}) validators[i].Status = sdk.Bonded - validators[i].addTokens(pool, amt) + validators[i].addTokensFromDel(pool, amt) } // check the empty keeper first diff --git a/x/stake/pool.go b/x/stake/pool.go index 8cb62ad6dd..4d4aca3964 100644 --- a/x/stake/pool.go +++ b/x/stake/pool.go @@ -97,7 +97,7 @@ func (p Pool) addTokensBonded(amount int64) (p2 Pool, issuedShares sdk.Rat) { func (p Pool) removeSharesBonded(shares sdk.Rat) (p2 Pool, removedTokens int64) { removedTokens = p.bondedShareExRate().Mul(shares).Evaluate() // (tokens/shares) * shares p.BondedShares = p.BondedShares.Sub(shares) - p.BondedTokens = p.BondedTokens - removedTokens + p.BondedTokens -= removedTokens return p, removedTokens } diff --git a/x/stake/tick_test.go b/x/stake/tick_test.go index 201325e757..896ca1208d 100644 --- a/x/stake/tick_test.go +++ b/x/stake/tick_test.go @@ -80,7 +80,7 @@ func TestProcessProvisions(t *testing.T) { } mintedTokens := int64((i + 1) * 10000000) pool.TotalSupply += mintedTokens - v, pool, _ = v.addTokens(pool, mintedTokens) + v, pool, _ = v.addTokensFromDel(pool, mintedTokens) keeper.setValidator(ctx, v) validators[i] = v diff --git a/x/stake/validator.go b/x/stake/validator.go index a2000a11a1..93b0234e9c 100644 --- a/x/stake/validator.go +++ b/x/stake/validator.go @@ -149,16 +149,21 @@ func (v Validator) UpdateSharesLocation(p Pool) (Validator, Pool) { return v, p } p, tokens = p.removeSharesBonded(v.BondedShares) + v.BondedShares = sdk.ZeroRat() + case !v.UnbondingShares.IsZero(): if v.Status == sdk.Unbonding { return v, p } - p, tokens = p.removeSharesUnbonding(v.BondedShares) + p, tokens = p.removeSharesUnbonding(v.UnbondingShares) + v.UnbondingShares = sdk.ZeroRat() + case !v.UnbondedShares.IsZero(): - if v.Status == sdk.Unbonding { + if v.Status == sdk.Unbonded { return v, p } - p, tokens = p.removeSharesUnbonded(v.BondedShares) + p, tokens = p.removeSharesUnbonded(v.UnbondedShares) + v.UnbondedShares = sdk.ZeroRat() } switch v.Status { @@ -169,7 +174,6 @@ func (v Validator) UpdateSharesLocation(p Pool) (Validator, Pool) { case sdk.Unbonded, sdk.Revoked: p, v.UnbondedShares = p.addTokensUnbonded(tokens) } - return v, p } @@ -178,22 +182,34 @@ func (v Validator) UpdateSharesLocation(p Pool) (Validator, Pool) { // if bonded, the power is the BondedShares // if not bonded, the power is the amount of bonded shares which the // the validator would have it was bonded -func (v Validator) EquivalentBondedShares(p Pool) (power sdk.Rat) { +func (v Validator) EquivalentBondedShares(p Pool) (eqBondedShares sdk.Rat) { switch v.Status { case sdk.Bonded: - power = v.BondedShares + eqBondedShares = v.BondedShares case sdk.Unbonding: shares := v.UnbondingShares // ubShr exRate := p.unbondingShareExRate().Quo(p.bondedShareExRate()) // (tok/ubshr)/(tok/bshr) = bshr/ubshr - power = shares.Mul(exRate) // ubshr*bshr/ubshr = bshr + eqBondedShares = shares.Mul(exRate) // ubshr*bshr/ubshr = bshr case sdk.Unbonded, sdk.Revoked: shares := v.UnbondedShares // ubShr exRate := p.unbondedShareExRate().Quo(p.bondedShareExRate()) // (tok/ubshr)/(tok/bshr) = bshr/ubshr - power = shares.Mul(exRate) // ubshr*bshr/ubshr = bshr + eqBondedShares = shares.Mul(exRate) // ubshr*bshr/ubshr = bshr } return } +// convert the equivalent bonded shares to a worth in unbonding shares +func EquivalentBondedSharesToUnbonding(p Pool, eqBondedShares sdk.Rat) (unbondingShares sdk.Rat) { + exRate := p.bondedShareExRate().Quo(p.unbondingShareExRate()) // (tok/bshr)/(tok/ubshr) = ubshr/bshr + return eqBondedShares.Mul(exRate) // bshr*ubshr/bshr = ubshr +} + +// convert the equivalent bonded shares to a worth in unbonded shares +func EquivalentBondedSharesToUnbonded(p Pool, eqBondedShares sdk.Rat) (unbondedShares sdk.Rat) { + exRate := p.bondedShareExRate().Quo(p.unbondedShareExRate()) // (tok/bshr)/(tok/ubshr) = ubshr/bshr + return eqBondedShares.Mul(exRate) // bshr*ubshr/bshr = ubshr +} + // TODO Implement Use in query functionality // get the equivalent amount of tokens contained by a validator func (v Validator) Tokens(p Pool) sdk.Rat { @@ -210,7 +226,7 @@ func (v Validator) Tokens(p Pool) sdk.Rat { // XXX Audit this function further to make sure it's correct // add tokens to a validator -func (v Validator) addTokens(p Pool, +func (v Validator) addTokensFromDel(p Pool, amount int64) (validator2 Validator, p2 Pool, issuedDelegatorShares sdk.Rat) { var poolShares sdk.Rat @@ -234,18 +250,26 @@ func (v Validator) addTokens(p Pool, return v, p, issuedDelegatorShares } -// remove shares from a validator -func (v Validator) removeShares(p Pool, +// remove delegator shares from a validator +func (v Validator) removeDelShares(p Pool, delShares sdk.Rat) (validator2 Validator, p2 Pool, createdCoins int64) { - globalPoolSharesToRemove := v.DelegatorShareExRate(p).Mul(delShares) - if v.Status == sdk.Bonded { - p, createdCoins = p.removeSharesBonded(globalPoolSharesToRemove) - } else { - p, createdCoins = p.removeSharesUnbonded(globalPoolSharesToRemove) - } - v.BondedShares = v.BondedShares.Sub(globalPoolSharesToRemove) + eqBondedSharesToRemove := v.DelegatorShareExRate(p).Mul(delShares) v.DelegatorShares = v.DelegatorShares.Sub(delShares) + + switch v.Status { + case sdk.Bonded: + p, createdCoins = p.removeSharesBonded(eqBondedSharesToRemove) + v.BondedShares = v.BondedShares.Sub(eqBondedSharesToRemove) + case sdk.Unbonding: + unbondingShares := EquivalentBondedSharesToUnbonding(p, eqBondedSharesToRemove) + p, createdCoins = p.removeSharesUnbonding(unbondingShares) + v.UnbondingShares = v.UnbondingShares.Sub(unbondingShares) + case sdk.Unbonded, sdk.Revoked: + unbondedShares := EquivalentBondedSharesToUnbonded(p, eqBondedSharesToRemove) + p, createdCoins = p.removeSharesUnbonded(unbondedShares) + v.UnbondedShares = v.UnbondedShares.Sub(unbondedShares) + } return v, p, createdCoins } diff --git a/x/stake/validator_test.go b/x/stake/validator_test.go index 5eaf6c0360..4343ad0947 100644 --- a/x/stake/validator_test.go +++ b/x/stake/validator_test.go @@ -16,7 +16,7 @@ func TestAddTokensValidatorBonded(t *testing.T) { pool := keeper.GetPool(ctx) val := NewValidator(addrs[0], pks[0], Description{}) val.Status = sdk.Bonded - val, pool, delShares := val.addTokens(pool, 10) + val, pool, delShares := val.addTokensFromDel(pool, 10) assert.Equal(t, sdk.OneRat(), val.DelegatorShareExRate(pool)) assert.Equal(t, sdk.OneRat(), pool.bondedShareExRate()) @@ -33,7 +33,7 @@ func TestAddTokensValidatorUnbonding(t *testing.T) { pool := keeper.GetPool(ctx) val := NewValidator(addrs[0], pks[0], Description{}) val.Status = sdk.Unbonding - val, pool, delShares := val.addTokens(pool, 10) + val, pool, delShares := val.addTokensFromDel(pool, 10) assert.Equal(t, sdk.OneRat(), val.DelegatorShareExRate(pool)) assert.Equal(t, sdk.OneRat(), pool.bondedShareExRate()) @@ -50,7 +50,7 @@ func TestAddTokensValidatorUnbonded(t *testing.T) { pool := keeper.GetPool(ctx) val := NewValidator(addrs[0], pks[0], Description{}) val.Status = sdk.Unbonded - val, pool, delShares := val.addTokens(pool, 10) + val, pool, delShares := val.addTokensFromDel(pool, 10) assert.Equal(t, sdk.OneRat(), val.DelegatorShareExRate(pool)) assert.Equal(t, sdk.OneRat(), pool.bondedShareExRate()) @@ -78,7 +78,7 @@ func TestRemoveShares(t *testing.T) { assert.Equal(t, valA.DelegatorShareExRate(poolA), sdk.OneRat()) assert.Equal(t, poolA.bondedShareExRate(), sdk.OneRat()) assert.Equal(t, poolA.unbondedShareExRate(), sdk.OneRat()) - valB, poolB, coinsB := valA.removeShares(poolA, sdk.NewRat(10)) + valB, poolB, coinsB := valA.removeDelShares(poolA, sdk.NewRat(10)) // coins were created assert.Equal(t, coinsB, int64(10)) @@ -110,7 +110,7 @@ func TestRemoveShares(t *testing.T) { msg := fmt.Sprintf("validator %s (status: %d, assets: %v, liabilities: %v, DelegatorShareExRate: %v)", val.Address, val.Status, val.BondedShares, val.DelegatorShares, val.DelegatorShareExRate(pool)) msg = fmt.Sprintf("Removed %v shares from %s", shares, msg) - _, newPool, tokens := val.removeShares(pool, shares) + _, newPool, tokens := val.removeDelShares(pool, shares) require.Equal(t, tokens+newPool.UnbondedTokens+newPool.BondedTokens, pool.BondedTokens+pool.UnbondedTokens, @@ -123,7 +123,7 @@ func TestUpdateSharesLocation(t *testing.T) { val := NewValidator(addrs[0], pks[0], Description{}) val.Status = sdk.Unbonded - val, pool, _ = val.addTokens(pool, 100) + val, pool, _ = val.addTokensFromDel(pool, 100) assert.Equal(t, int64(0), val.BondedShares.Evaluate()) assert.Equal(t, int64(0), val.UnbondingShares.Evaluate()) assert.Equal(t, int64(100), val.UnbondedShares.Evaluate()) @@ -133,6 +133,7 @@ func TestUpdateSharesLocation(t *testing.T) { val.Status = sdk.Unbonding val, pool = val.UpdateSharesLocation(pool) + //require.Fail(t, "", "%v", val.BondedShares.IsZero()) assert.Equal(t, int64(0), val.BondedShares.Evaluate()) assert.Equal(t, int64(100), val.UnbondingShares.Evaluate()) assert.Equal(t, int64(0), val.UnbondedShares.Evaluate()) @@ -145,8 +146,8 @@ func TestUpdateSharesLocation(t *testing.T) { assert.Equal(t, int64(100), val.BondedShares.Evaluate()) assert.Equal(t, int64(0), val.UnbondingShares.Evaluate()) assert.Equal(t, int64(0), val.UnbondedShares.Evaluate()) - assert.Equal(t, int64(0), pool.BondedTokens) - assert.Equal(t, int64(100), pool.UnbondingTokens) + assert.Equal(t, int64(100), pool.BondedTokens) + assert.Equal(t, int64(0), pool.UnbondingTokens) assert.Equal(t, int64(0), pool.UnbondedTokens) } @@ -225,7 +226,7 @@ func OpAddTokens(r *rand.Rand, p Pool, val Validator) (Pool, Validator, int64, s tokens := int64(r.Int31n(1000)) msg := fmt.Sprintf("validator %s (status: %d, assets: %v, liabilities: %v, DelegatorShareExRate: %v)", val.Address, val.Status, val.BondedShares, val.DelegatorShares, val.DelegatorShareExRate(p)) - val, p, _ = val.addTokens(p, tokens) + val, p, _ = val.addTokensFromDel(p, tokens) msg = fmt.Sprintf("Added %d tokens to %s", tokens, msg) return p, val, -1 * tokens, msg // tokens are removed so for accounting must be negative } @@ -243,7 +244,7 @@ func OpRemoveShares(r *rand.Rand, p Pool, val Validator) (Pool, Validator, int64 msg := fmt.Sprintf("Removed %v shares from validator %s (status: %d, assets: %v, liabilities: %v, DelegatorShareExRate: %v)", shares, val.Address, val.Status, val.BondedShares, val.DelegatorShares, val.DelegatorShareExRate(p)) - val, p, tokens := val.removeShares(p, shares) + val, p, tokens := val.removeDelShares(p, shares) return p, val, tokens, msg } @@ -351,7 +352,7 @@ func TestPossibleOverflow(t *testing.T) { tokens := int64(71) msg := fmt.Sprintf("validator %s (status: %d, assets: %v, liabilities: %v, DelegatorShareExRate: %v)", val.Address, val.Status, val.BondedShares, val.DelegatorShares, val.DelegatorShareExRate(pool)) - newValidator, _, _ := val.addTokens(pool, tokens) + newValidator, _, _ := val.addTokensFromDel(pool, tokens) msg = fmt.Sprintf("Added %d tokens to %s", tokens, msg) require.False(t, newValidator.DelegatorShareExRate(pool).LT(sdk.ZeroRat()),