R4R: Results from x/staking & x/slashing peer review

cmd/gaia/app/app.go

@@ -355,10 +355,6 @@ func (h StakingHooks) AfterValidatorBonded(ctx sdk.Context, consAddr sdk.ConsAdd
 	h.dh.AfterValidatorBonded(ctx, consAddr, valAddr)
 	h.sh.AfterValidatorBonded(ctx, consAddr, valAddr)
 }
-func (h StakingHooks) AfterValidatorPowerDidChange(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
-	h.dh.AfterValidatorPowerDidChange(ctx, consAddr, valAddr)
-	h.sh.AfterValidatorPowerDidChange(ctx, consAddr, valAddr)
-}
 func (h StakingHooks) AfterValidatorBeginUnbonding(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
 	h.dh.AfterValidatorBeginUnbonding(ctx, consAddr, valAddr)
 	h.sh.AfterValidatorBeginUnbonding(ctx, consAddr, valAddr)

cmd/gaia/app/export.go

@@ -150,7 +150,6 @@ func (app *GaiaApp) prepForZeroHeightGenesis(ctx sdk.Context, jailWhiteList []st
 			panic("expected validator, not found")
 		}
 
-		validator.BondHeight = 0
 		validator.UnbondingHeight = 0
 		valConsAddrs = append(valConsAddrs, validator.ConsAddress())
 		if applyWhiteList && !whiteListMap[addr.String()] {

docs/spec/staking/hooks.md

@@ -15,8 +15,6 @@ following hooks can registered with staking:
    - called when a validator is bonded
  - `AfterValidatorBeginUnbonding(Context, ConsAddress, ValAddress)`
    - called when a validator begins unbonding
- - `AfterValidatorPowerDidChange(Context, ConsAddress, ValAddress)`
-   - called at EndBlock when a validator's power is changed
  - `BeforeDelegationCreated(Context, AccAddress, ValAddress)`
    - called when a delegation is created
  - `BeforeDelegationSharesModified(Context, AccAddress, ValAddress)`

docs/spec/staking/state.md

@@ -187,7 +187,6 @@ type RedelegationEntry struct {
     CompletionTime time.Time // unix time for redelegation completion
     InitialBalance sdk.Coin  // initial balance when redelegation started
     Balance        sdk.Coin  // current balance (current value held in destination validator)
-    SharesSrc      sdk.Dec   // amount of source-validator shares removed by redelegation
     SharesDst      sdk.Dec   // amount of destination-validator shares created by redelegation
 }
 ```

types/int.go

@@ -18,6 +18,8 @@ func gt(i *big.Int, i2 *big.Int) bool { return i.Cmp(i2) == 1 }
 
 func lt(i *big.Int, i2 *big.Int) bool { return i.Cmp(i2) == -1 }
 
+func lte(i *big.Int, i2 *big.Int) bool { return i.Cmp(i2) <= 0 }
+
 func add(i *big.Int, i2 *big.Int) *big.Int { return new(big.Int).Add(i, i2) }
 
 func sub(i *big.Int, i2 *big.Int) *big.Int { return new(big.Int).Sub(i, i2) }
@@ -191,6 +193,10 @@ func (i Int) LT(i2 Int) bool {
 	return lt(i.i, i2.i)
 }
 
+func (i Int) LTE(i2 Int) bool {
+	return lte(i.i, i2.i)
+}
+
 // Add adds Int from another
 func (i Int) Add(i2 Int) (res Int) {
 	res = Int{add(i.i, i2.i)}

types/stake.go

@@ -46,7 +46,6 @@ type Validator interface {
 	GetTokens() Int               // validation tokens
 	GetCommission() Dec           // validator commission rate
 	GetDelegatorShares() Dec      // total outstanding delegator shares
-	GetBondHeight() int64         // height in which the validator became active
 	GetDelegatorShareExRate() Dec // tokens per delegator share exchange rate
 }
 
@@ -122,7 +121,6 @@ type StakingHooks interface {
 
 	AfterValidatorBonded(ctx Context, consAddr ConsAddress, valAddr ValAddress)         // Must be called when a validator is bonded
 	AfterValidatorBeginUnbonding(ctx Context, consAddr ConsAddress, valAddr ValAddress) // Must be called when a validator begins unbonding
-	AfterValidatorPowerDidChange(ctx Context, consAddr ConsAddress, valAddr ValAddress) // Called at EndBlock when a validator's power did change
 
 	BeforeDelegationCreated(ctx Context, delAddr AccAddress, valAddr ValAddress)        // Must be called when a delegation is created
 	BeforeDelegationSharesModified(ctx Context, delAddr AccAddress, valAddr ValAddress) // Must be called when a delegation's shares are modified

x/distribution/keeper/hooks.go

@@ -82,8 +82,6 @@ func (h Hooks) AfterValidatorBeginUnbonding(ctx sdk.Context, _ sdk.ConsAddress,
 }
 func (h Hooks) AfterValidatorBonded(ctx sdk.Context, _ sdk.ConsAddress, valAddr sdk.ValAddress) {
 }
-func (h Hooks) AfterValidatorPowerDidChange(ctx sdk.Context, _ sdk.ConsAddress, valAddr sdk.ValAddress) {
-}
 func (h Hooks) BeforeValidatorSlashed(ctx sdk.Context, valAddr sdk.ValAddress, fraction sdk.Dec) {
 	// record the slash event
 	h.k.updateValidatorSlashFraction(ctx, valAddr, fraction)

x/slashing/hooks.go

@@ -66,7 +66,6 @@ func (h Hooks) AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) {
 
 // nolint - unused hooks
 func (h Hooks) AfterValidatorBeginUnbonding(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress)  {}
-func (h Hooks) AfterValidatorPowerDidChange(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress)  {}
 func (h Hooks) BeforeValidatorModified(_ sdk.Context, _ sdk.ValAddress)                          {}
 func (h Hooks) BeforeDelegationCreated(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress)        {}
 func (h Hooks) BeforeDelegationSharesModified(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) {}

x/staking/client/module_client.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/x/staking/client/cli"
+	"github.com/cosmos/cosmos-sdk/x/staking/types"
 )
 
 // ModuleClient exports all client functionality from this module
@@ -21,7 +22,7 @@ func NewModuleClient(storeKey string, cdc *amino.Codec) ModuleClient {
 // GetQueryCmd returns the cli query commands for this module
 func (mc ModuleClient) GetQueryCmd() *cobra.Command {
 	stakingQueryCmd := &cobra.Command{
-		Use:   "staking",
+		Use:   types.ModuleName,
 		Short: "Querying commands for the staking module",
 	}
 	stakingQueryCmd.AddCommand(client.GetCommands(
@@ -46,7 +47,7 @@ func (mc ModuleClient) GetQueryCmd() *cobra.Command {
 // GetTxCmd returns the transaction commands for this module
 func (mc ModuleClient) GetTxCmd() *cobra.Command {
 	stakingTxCmd := &cobra.Command{
-		Use:   "staking",
+		Use:   types.ModuleName,
 		Short: "Staking transaction subcommands",
 	}
 

x/staking/genesis.go

@@ -147,21 +147,14 @@ func ValidateGenesis(data types.GenesisState) error {
 	if err != nil {
 		return err
 	}
-	err = validateParams(data.Params)
+	err = data.Params.Validate()
 	if err != nil {
 		return err
 	}
 
 	return nil
 }
 
-func validateParams(params types.Params) error {
-	if params.BondDenom == "" {
-		return fmt.Errorf("staking parameter BondDenom can't be an empty string")
-	}
-	return nil
-}
-
 func validateGenesisStateValidators(validators []types.Validator) (err error) {
 	addrMap := make(map[string]bool, len(validators))
 	for i := 0; i < len(validators); i++ {

x/staking/keeper/delegation.go

@@ -72,22 +72,21 @@ func (k Keeper) GetDelegatorDelegations(ctx sdk.Context, delegator sdk.AccAddres
 	return delegations[:i] // trim if the array length < maxRetrieve
 }
 
-// set the delegation
+// set a delegation
 func (k Keeper) SetDelegation(ctx sdk.Context, delegation types.Delegation) {
 	store := ctx.KVStore(k.storeKey)
 	b := types.MustMarshalDelegation(k.cdc, delegation)
 	store.Set(GetDelegationKey(delegation.DelegatorAddr, delegation.ValidatorAddr), b)
 }
 
-// remove a delegation from store
+// remove a delegation
 func (k Keeper) RemoveDelegation(ctx sdk.Context, delegation types.Delegation) {
+	// TODO: Consider calling hooks outside of the store wrapper functions, it's unobvious.
 	k.BeforeDelegationRemoved(ctx, delegation.DelegatorAddr, delegation.ValidatorAddr)
 	store := ctx.KVStore(k.storeKey)
 	store.Delete(GetDelegationKey(delegation.DelegatorAddr, delegation.ValidatorAddr))
 }
 
-//_____________________________________________________________________________________
-
 // return a given amount of all the delegator unbonding-delegations
 func (k Keeper) GetUnbondingDelegations(ctx sdk.Context, delegator sdk.AccAddress,
 	maxRetrieve uint16) (unbondingDelegations []types.UnbondingDelegation) {
@@ -153,7 +152,7 @@ func (k Keeper) IterateUnbondingDelegations(ctx sdk.Context, fn func(index int64
 	}
 }
 
-// HasMaxUnbondingDelegationEntries - unbonding delegation has maximum number of entries
+// HasMaxUnbondingDelegationEntries - check if unbonding delegation has maximum number of entries
 func (k Keeper) HasMaxUnbondingDelegationEntries(ctx sdk.Context,
 	delegatorAddr sdk.AccAddress, validatorAddr sdk.ValAddress) bool {
 
@@ -197,7 +196,6 @@ func (k Keeper) SetUnbondingDelegationEntry(ctx sdk.Context,
 	return ubd
 }
 
-//________________________________________________
 // unbonding delegation queue timeslice operations
 
 // gets a specific unbonding queue timeslice. A timeslice is a slice of DVPairs
@@ -241,7 +239,7 @@ func (k Keeper) UBDQueueIterator(ctx sdk.Context, endTime time.Time) sdk.Iterato
 }
 
 // Returns a concatenated list of all the timeslices inclusively previous to
-// currTime, and deletes the timeslices from the queue
+// curTime, and deletes the timeslices from the queue
 func (k Keeper) DequeueAllMatureUBDQueue(ctx sdk.Context,
 	currTime time.Time) (matureUnbonds []types.DVPair) {
 
@@ -258,8 +256,6 @@ func (k Keeper) DequeueAllMatureUBDQueue(ctx sdk.Context,
 	return matureUnbonds
 }
 
-//_____________________________________________________________________________________
-
 // return a given amount of all the delegator redelegations
 func (k Keeper) GetRedelegations(ctx sdk.Context, delegator sdk.AccAddress,
 	maxRetrieve uint16) (redelegations []types.Redelegation) {
@@ -356,11 +352,10 @@ func (k Keeper) SetRedelegationEntry(ctx sdk.Context,
 
 	red, found := k.GetRedelegation(ctx, delegatorAddr, validatorSrcAddr, validatorDstAddr)
 	if found {
-		red.AddEntry(creationHeight, minTime, balance, sharesSrc, sharesDst)
+		red.AddEntry(creationHeight, minTime, balance, sharesDst)
 	} else {
 		red = types.NewRedelegation(delegatorAddr, validatorSrcAddr,
-			validatorDstAddr, creationHeight, minTime, balance, sharesSrc,
-			sharesDst)
+			validatorDstAddr, creationHeight, minTime, balance, sharesDst)
 	}
 	k.SetRedelegation(ctx, red)
 	return red
@@ -390,7 +385,6 @@ func (k Keeper) RemoveRedelegation(ctx sdk.Context, red types.Redelegation) {
 	store.Delete(GetREDByValDstIndexKey(red.DelegatorAddr, red.ValidatorSrcAddr, red.ValidatorDstAddr))
 }
 
-//________________________________________________
 // redelegation queue timeslice operations
 
 // Gets a specific redelegation queue timeslice. A timeslice is a slice of DVVTriplets corresponding to redelegations
@@ -448,20 +442,18 @@ func (k Keeper) DequeueAllMatureRedelegationQueue(ctx sdk.Context, currTime time
 	return matureRedelegations
 }
 
-//_____________________________________________________________________________________
-
 // Perform a delegation, set/update everything necessary within the store.
 func (k Keeper) Delegate(ctx sdk.Context, delAddr sdk.AccAddress, bondAmt sdk.Coin,
 	validator types.Validator, subtractAccount bool) (newShares sdk.Dec, err sdk.Error) {
 
 	// In some situations, the exchange rate becomes invalid, e.g. if
-	// validator loses all tokens due to slashing.  In this case,
+	// validator loses all tokens due to slashing. In this case,
 	// make all future delegations invalid.
 	if validator.DelegatorShareExRate().IsZero() {
 		return sdk.ZeroDec(), types.ErrDelegatorShareExRateInvalid(k.Codespace())
 	}
 
-	// Get or create the delegator delegation
+	// Get or create the delegation object
 	delegation, found := k.GetDelegation(ctx, delAddr, validator.OperatorAddr)
 	if !found {
 		delegation = types.Delegation{
@@ -490,24 +482,27 @@ func (k Keeper) Delegate(ctx sdk.Context, delAddr sdk.AccAddress, bondAmt sdk.Co
 	// Update delegation
 	delegation.Shares = delegation.Shares.Add(newShares)
 	k.SetDelegation(ctx, delegation)
+
+	// Call the after-modification hook
 	k.AfterDelegationModified(ctx, delegation.DelegatorAddr, delegation.ValidatorAddr)
 
 	return newShares, nil
 }
 
-// unbond the the delegation return
+// unbond a particular delegation and perform associated store operations
 func (k Keeper) unbond(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress,
 	shares sdk.Dec) (amount sdk.Int, err sdk.Error) {
 
-	// check if delegation has any shares in it unbond
+	// check if a delegation object exists in the store
 	delegation, found := k.GetDelegation(ctx, delAddr, valAddr)
 	if !found {
 		return amount, types.ErrNoDelegatorForAddress(k.Codespace())
 	}
 
+	// call the before-delegation-modified hook
 	k.BeforeDelegationSharesModified(ctx, delAddr, valAddr)
 
-	// retrieve the amount to remove
+	// ensure that we have enough shares to remove
 	if delegation.Shares.LT(shares) {
 		return amount, types.ErrNotEnoughDelegationShares(k.Codespace(), delegation.Shares.String())
 	}
@@ -518,23 +513,20 @@ func (k Keeper) unbond(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValA
 		return amount, types.ErrNoValidatorFound(k.Codespace())
 	}
 
-	// subtract shares from delegator
+	// subtract shares from delegation
 	delegation.Shares = delegation.Shares.Sub(shares)
 
-	// remove the delegation
+	// update or remove the delegation
 	if delegation.Shares.IsZero() {
-
-		// if the delegation is the operator of the validator then
-		// trigger a jail validator
+		// if the delegator is the operator of the validator then jail the validator
 		if bytes.Equal(delegation.DelegatorAddr, validator.OperatorAddr) && !validator.Jailed {
 			k.jailValidator(ctx, validator)
 			validator = k.mustGetValidator(ctx, validator.OperatorAddr)
 		}
-
 		k.RemoveDelegation(ctx, delegation)
 	} else {
-		// update the delegation
 		k.SetDelegation(ctx, delegation)
+		// call the after delegation modification hook
 		k.AfterDelegationModified(ctx, delegation.DelegatorAddr, delegation.ValidatorAddr)
 	}
 
@@ -549,15 +541,14 @@ func (k Keeper) unbond(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValA
 	return amount, nil
 }
 
-//______________________________________________________________________________________________________
-
 // get info for begin functions: completionTime and CreationHeight
 func (k Keeper) getBeginInfo(ctx sdk.Context, valSrcAddr sdk.ValAddress) (
 	completionTime time.Time, height int64, completeNow bool) {
 
 	validator, found := k.GetValidator(ctx, valSrcAddr)
 
 	switch {
+	// TODO: when would the validator not be found?
 	case !found || validator.Status == sdk.Bonded:
 
 		// the longest wait - just unbonding period from now
@@ -578,7 +569,7 @@ func (k Keeper) getBeginInfo(ctx sdk.Context, valSrcAddr sdk.ValAddress) (
 	}
 }
 
-// begin unbonding an unbonding record
+// begin unbonding part or all of a delegation
 func (k Keeper) Undelegate(ctx sdk.Context, delAddr sdk.AccAddress,
 	valAddr sdk.ValAddress, sharesAmount sdk.Dec) (completionTime time.Time, sdkErr sdk.Error) {
 
@@ -633,7 +624,7 @@ func (k Keeper) CompleteUnbonding(ctx sdk.Context, delAddr sdk.AccAddress,
 			ubd.RemoveEntry(int64(i))
 			i--
 
-			_, _, err := k.bankKeeper.AddCoins(ctx, ubd.DelegatorAddr, sdk.Coins{entry.Balance})
+			_, err := k.bankKeeper.UndelegateCoins(ctx, ubd.DelegatorAddr, sdk.Coins{entry.Balance})
 			if err != nil {
 				return err
 			}

x/staking/keeper/delegation_test.go

@@ -587,7 +587,6 @@ func TestRedelegation(t *testing.T) {
 	require.True(t, has)
 
 	// modify a records, save, and retrieve
-	rd.Entries[0].SharesSrc = sdk.NewDec(21)
 	rd.Entries[0].SharesDst = sdk.NewDec(21)
 	keeper.SetRedelegation(ctx, rd)