R4R: Allow the undelegation of more coins than were delegated; More validity checks.

docs/spec/auth/05_vesting.md

@@ -251,10 +251,12 @@ func DelegateCoins(t Time, from Account, amount Coins) {
 ### Undelegating
 
 For a vesting account attempting to undelegate `D` coins, the following is performed:
+NOTE: `DV < D` and `(DV + DF) < D` may be possible due to quirks in the rounding of
+delegation/undelegation logic.
 
-1. Verify `(DV + DF) >= D > 0` (this is simply a sanity check)
+1. Verify `D > 0`
 2. Compute `X := min(DF, D)` (portion of `D` that should become free, prioritizing free coins)
-3. Compute `Y := D - X` (portion of `D` that should remain vesting)
+3. Compute `Y := min(DV, D - X)` (portion of `D` that should remain vesting)
 4. Set `DF -= X`
 5. Set `DV -= Y`
 6. Set `BC += D`

types/coin.go

@@ -389,8 +389,6 @@ func (coins Coins) AmountOf(denom string) Int {
 
 // IsAllPositive returns true if there is at least one coin and all currencies
 // have a positive value.
-//
-// TODO: Remove once unsigned integers are used.
 func (coins Coins) IsAllPositive() bool {
 	if len(coins) == 0 {
 		return false

x/auth/account.go

@@ -295,12 +295,13 @@ func (bva *BaseVestingAccount) TrackUndelegation(amount sdk.Coins) {
 			panic("undelegation attempt with zero coins")
 		}
 		delegatedFree := bva.DelegatedFree.AmountOf(coin.Denom)
+		delegatedVesting := bva.DelegatedVesting.AmountOf(coin.Denom)
 
 		// compute x and y per the specification, where:
 		// X := min(DF, D)
-		// Y := D - X
+		// Y := min(D - X, DV)
 		x := sdk.MinInt(delegatedFree, coin.Amount)
-		y := coin.Amount.Sub(x)
+		y := sdk.MinInt(delegatedVesting, coin.Amount.Sub(x))
 
 		if !x.IsZero() {
 			xCoin := sdk.NewCoin(coin.Denom, x)

x/bank/keeper.go

@@ -47,7 +47,13 @@ func NewBaseKeeper(ak auth.AccountKeeper,
 }
 
 // SetCoins sets the coins at the addr.
-func (keeper BaseKeeper) SetCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) sdk.Error {
+func (keeper BaseKeeper) SetCoins(
+	ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins,
+) sdk.Error {
+
+	if !amt.IsValid() {
+		return sdk.ErrInvalidCoins(amt.String())
+	}
 	return setCoins(ctx, keeper.ak, addr, amt)
 }
 
@@ -56,6 +62,9 @@ func (keeper BaseKeeper) SubtractCoins(
 	ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins,
 ) (sdk.Coins, sdk.Tags, sdk.Error) {
 
+	if !amt.IsValid() {
+		return nil, nil, sdk.ErrInvalidCoins(amt.String())
+	}
 	return subtractCoins(ctx, keeper.ak, addr, amt)
 }
 
@@ -64,6 +73,9 @@ func (keeper BaseKeeper) AddCoins(
 	ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins,
 ) (sdk.Coins, sdk.Tags, sdk.Error) {
 
+	if !amt.IsValid() {
+		return nil, nil, sdk.ErrInvalidCoins(amt.String())
+	}
 	return addCoins(ctx, keeper.ak, addr, amt)
 }
 
@@ -78,14 +90,27 @@ func (keeper BaseKeeper) InputOutputCoins(
 // DelegateCoins performs delegation by deducting amt coins from an account with
 // address addr. For vesting accounts, delegations amounts are tracked for both
 // vesting and vested coins.
-func (keeper BaseKeeper) DelegateCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error) {
+func (keeper BaseKeeper) DelegateCoins(
+	ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins,
+) (sdk.Tags, sdk.Error) {
+
+	if !amt.IsValid() {
+		return nil, sdk.ErrInvalidCoins(amt.String())
+	}
 	return delegateCoins(ctx, keeper.ak, addr, amt)
 }
 
 // UndelegateCoins performs undelegation by crediting amt coins to an account with
 // address addr. For vesting accounts, undelegation amounts are tracked for both
 // vesting and vested coins.
-func (keeper BaseKeeper) UndelegateCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error) {
+// If any of the undelegation amounts are negative, an error is returned.
+func (keeper BaseKeeper) UndelegateCoins(
+	ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins,
+) (sdk.Tags, sdk.Error) {
+
+	if !amt.IsValid() {
+		return nil, sdk.ErrInvalidCoins(amt.String())
+	}
 	return undelegateCoins(ctx, keeper.ak, addr, amt)
 }
 
@@ -126,6 +151,10 @@ func NewBaseSendKeeper(ak auth.AccountKeeper,
 func (keeper BaseSendKeeper) SendCoins(
 	ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins,
 ) (sdk.Tags, sdk.Error) {
+
+	if !amt.IsValid() {
+		return nil, sdk.ErrInvalidCoins(amt.String())
+	}
 	return sendCoins(ctx, keeper.ak, fromAddr, toAddr, amt)
 }
 
@@ -188,6 +217,9 @@ func getCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress) sdk.C
 }
 
 func setCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) sdk.Error {
+	if !amt.IsValid() {
+		return sdk.ErrInvalidCoins(amt.String())
+	}
 	acc := am.GetAccount(ctx, addr)
 	if acc == nil {
 		acc = am.NewAccountWithAddress(ctx, addr)
@@ -218,6 +250,11 @@ func setAccount(ctx sdk.Context, ak auth.AccountKeeper, acc auth.Account) {
 //
 // CONTRACT: If the account is a vesting account, the amount has to be spendable.
 func subtractCoins(ctx sdk.Context, ak auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Tags, sdk.Error) {
+
+	if !amt.IsValid() {
+		return nil, nil, sdk.ErrInvalidCoins(amt.String())
+	}
+
 	oldCoins, spendableCoins := sdk.Coins{}, sdk.Coins{}
 
 	acc := getAccount(ctx, ak, addr)
@@ -244,6 +281,11 @@ func subtractCoins(ctx sdk.Context, ak auth.AccountKeeper, addr sdk.AccAddress,
 
 // AddCoins adds amt to the coins at the addr.
 func addCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Tags, sdk.Error) {
+
+	if !amt.IsValid() {
+		return nil, nil, sdk.ErrInvalidCoins(amt.String())
+	}
+
 	oldCoins := getCoins(ctx, am, addr)
 	newCoins := oldCoins.Add(amt)
 
@@ -260,9 +302,9 @@ func addCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt s
 }
 
 // SendCoins moves coins from one account to another
+// Returns ErrInvalidCoins if amt is invalid.
 func sendCoins(ctx sdk.Context, am auth.AccountKeeper, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error) {
 	// Safety check ensuring that when sending coins the keeper must maintain the
-	// supply invariant.
 	if !amt.IsValid() {
 		return nil, sdk.ErrInvalidCoins(amt.String())
 	}
@@ -284,7 +326,7 @@ func sendCoins(ctx sdk.Context, am auth.AccountKeeper, fromAddr sdk.AccAddress,
 // NOTE: Make sure to revert state changes from tx on error
 func inputOutputCoins(ctx sdk.Context, am auth.AccountKeeper, inputs []Input, outputs []Output) (sdk.Tags, sdk.Error) {
 	// Safety check ensuring that when sending coins the keeper must maintain the
-	// supply invariant.
+	// Check supply invariant and validity of Coins.
 	if err := ValidateInputsOutputs(inputs, outputs); err != nil {
 		return nil, err
 	}
@@ -314,6 +356,10 @@ func delegateCoins(
 	ctx sdk.Context, ak auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins,
 ) (sdk.Tags, sdk.Error) {
 
+	if !amt.IsValid() {
+		return nil, sdk.ErrInvalidCoins(amt.String())
+	}
+
 	acc := getAccount(ctx, ak, addr)
 	if acc == nil {
 		return nil, sdk.ErrUnknownAddress(fmt.Sprintf("account %s does not exist", addr))
@@ -344,6 +390,10 @@ func undelegateCoins(
 	ctx sdk.Context, ak auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins,
 ) (sdk.Tags, sdk.Error) {
 
+	if !amt.IsValid() {
+		return nil, sdk.ErrInvalidCoins(amt.String())
+	}
+
 	acc := getAccount(ctx, ak, addr)
 	if acc == nil {
 		return nil, sdk.ErrUnknownAddress(fmt.Sprintf("account %s does not exist", addr))
@@ -361,22 +411,24 @@ func undelegateCoins(
 	), nil
 }
 
-func trackDelegation(acc auth.Account, blockTime time.Time, amount sdk.Coins) error {
+// CONTRACT: assumes that amt is valid.
+func trackDelegation(acc auth.Account, blockTime time.Time, amt sdk.Coins) error {
 	vacc, ok := acc.(auth.VestingAccount)
 	if ok {
-		vacc.TrackDelegation(blockTime, amount)
+		vacc.TrackDelegation(blockTime, amt)
 		return nil
 	}
 
-	return acc.SetCoins(acc.GetCoins().Sub(amount))
+	return acc.SetCoins(acc.GetCoins().Sub(amt))
 }
 
-func trackUndelegation(acc auth.Account, amount sdk.Coins) error {
+// CONTRACT: assumes that amt is valid.
+func trackUndelegation(acc auth.Account, amt sdk.Coins) error {
 	vacc, ok := acc.(auth.VestingAccount)
 	if ok {
-		vacc.TrackUndelegation(amount)
+		vacc.TrackUndelegation(amt)
 		return nil
 	}
 
-	return acc.SetCoins(acc.GetCoins().Add(amount))
+	return acc.SetCoins(acc.GetCoins().Add(amt))
 }

x/bank/msgs.go

@@ -35,6 +35,9 @@ func (msg MsgSend) ValidateBasic() sdk.Error {
 	if msg.ToAddress.Empty() {
 		return sdk.ErrInvalidAddress("missing recipient address")
 	}
+	if !msg.Amount.IsValid() {
+		return sdk.ErrInvalidCoins("send amount is invalid: " + msg.Amount.String())
+	}
 	if !msg.Amount.IsAllPositive() {
 		return sdk.ErrInsufficientCoins("send amount must be positive")
 	}

x/distribution/keeper/delegation.go

@@ -107,9 +107,11 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de
 	k.SetFeePool(ctx, feePool)
 
 	// add coins to user account
-	withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, del.GetDelegatorAddr())
-	if _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil {
-		return err
+	if !coins.IsZero() {
+		withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, del.GetDelegatorAddr())
+		if _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil {
+			return err
+		}
 	}
 
 	// remove delegator starting info

x/distribution/keeper/hooks.go

@@ -37,11 +37,13 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr
 		h.k.SetOutstandingRewards(ctx, outstanding.Sub(commission))
 
 		// add to validator account
-		accAddr := sdk.AccAddress(valAddr)
-		withdrawAddr := h.k.GetDelegatorWithdrawAddr(ctx, accAddr)
+		if !coins.IsZero() {
+			accAddr := sdk.AccAddress(valAddr)
+			withdrawAddr := h.k.GetDelegatorWithdrawAddr(ctx, accAddr)
 
-		if _, _, err := h.k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil {
-			panic(err)
+			if _, _, err := h.k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil {
+				panic(err)
+			}
 		}
 	}
 	// remove commission record

x/distribution/keeper/keeper.go

@@ -87,11 +87,13 @@ func (k Keeper) WithdrawValidatorCommission(ctx sdk.Context, valAddr sdk.ValAddr
 	outstanding := k.GetOutstandingRewards(ctx)
 	k.SetOutstandingRewards(ctx, outstanding.Sub(sdk.NewDecCoins(coins)))
 
-	accAddr := sdk.AccAddress(valAddr)
-	withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, accAddr)
+	if !coins.IsZero() {
+		accAddr := sdk.AccAddress(valAddr)
+		withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, accAddr)
 
-	if _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil {
-		return err
+		if _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil {
+			return err
+		}
 	}
 
 	return nil

x/ibc/handler.go

@@ -44,6 +44,7 @@ func handleIBCReceiveMsg(ctx sdk.Context, ibcm Mapper, ck BankKeeper, msg IBCRec
 		return ErrInvalidSequence(ibcm.codespace).Result()
 	}
 
+	// XXX Check that packet.Coins is valid and positive (nonzero)
 	_, _, err := ck.AddCoins(ctx, packet.DestAddr, packet.Coins)
 	if err != nil {
 		return err.Result()

x/mock/simulation/rand_util.go

@@ -1,6 +1,7 @@
 package simulation
 
 import (
+	"fmt"
 	"math/big"
 	"math/rand"
 	"time"
@@ -37,14 +38,37 @@ func RandStringOfLength(r *rand.Rand, n int) string {
 }
 
 // Generate a random amount
+// Note: The range of RandomAmount includes max, and is, in fact, biased to return max.
 func RandomAmount(r *rand.Rand, max sdk.Int) sdk.Int {
-	return sdk.NewInt(int64(r.Intn(int(max.Int64()))))
+	// return sdk.NewInt(int64(r.Intn(int(max.Int64()))))
+	max2 := big.NewInt(0).Mul(max.BigInt(), big.NewInt(2))
+	randInt := big.NewInt(0).Rand(r, max2)
+	if randInt.Cmp(max.BigInt()) > 0 {
+		randInt = max.BigInt()
+	}
+	result := sdk.NewIntFromBigInt(randInt)
+	// Sanity
+	if result.GT(max) {
+		panic(fmt.Sprintf("%v > %v", result, max))
+	}
+	return result
 }
 
 // RandomDecAmount generates a random decimal amount
+// Note: The range of RandomDecAmount includes max, and is, in fact, biased to return max.
 func RandomDecAmount(r *rand.Rand, max sdk.Dec) sdk.Dec {
-	randInt := big.NewInt(0).Rand(r, max.Int)
-	return sdk.NewDecFromBigIntWithPrec(randInt, sdk.Precision)
+	// randInt := big.NewInt(0).Rand(r, max.Int)
+	max2 := big.NewInt(0).Mul(max.Int, big.NewInt(2))
+	randInt := big.NewInt(0).Rand(r, max2)
+	if randInt.Cmp(max.Int) > 0 {
+		randInt = max.Int
+	}
+	result := sdk.NewDecFromBigIntWithPrec(randInt, sdk.Precision)
+	// Sanity
+	if result.GT(max) {
+		panic(fmt.Sprintf("%v > %v", result, max))
+	}
+	return result
 }
 
 // RandomSetGenesis wraps mock.RandomSetGenesis, but using simulation accounts

x/slashing/keeper.go

@@ -47,7 +47,16 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio
 	consAddr := sdk.ConsAddress(addr)
 	pubkey, err := k.getPubkey(ctx, addr)
 	if err != nil {
-		panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr))
+		// ignore evidence that cannot be handled.
+		return
+		// NOTE:
+		// We used to panic with:
+		// `panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr))`,
+		// but this couples the expectations of the app to both Tendermint and
+		// the simulator.  Both are expected to provide the full range of
+		// allowable but none of the disallowed evidence types.  Instead of
+		// getting this coordination right, it is easier to relax the
+		// constraints and ignore evidence that cannot be handled.
 	}
 
 	// Reject evidence if the double-sign is too old

x/staking/keeper/delegation_test.go

@@ -207,7 +207,7 @@ func TestUnbondDelegation(t *testing.T) {
 }
 
 func TestUnbondingDelegationsMaxEntries(t *testing.T) {
-	ctx, _, keeper := CreateTestInput(t, false, 0)
+	ctx, _, keeper := CreateTestInput(t, false, 1)
 	pool := keeper.GetPool(ctx)
 	startTokens := sdk.TokensFromTendermintPower(10)
 	pool.NotBondedTokens = startTokens
@@ -372,7 +372,7 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) {
 }
 
 func TestUndelegateFromUnbondedValidator(t *testing.T) {
-	ctx, _, keeper := CreateTestInput(t, false, 0)
+	ctx, _, keeper := CreateTestInput(t, false, 1)
 	pool := keeper.GetPool(ctx)
 	startTokens := sdk.TokensFromTendermintPower(20)
 	pool.NotBondedTokens = startTokens

x/staking/keeper/keeper_test.go

@@ -30,11 +30,11 @@ func TestPool(t *testing.T) {
 
 	//check that the empty keeper loads the default
 	resPool := keeper.GetPool(ctx)
-	require.True(t, expPool.Equal(resPool))
+	require.Equal(t, expPool, resPool)
 
 	//modify a params, save, and retrieve
 	expPool.BondedTokens = sdk.NewInt(777)
 	keeper.SetPool(ctx, expPool)
 	resPool = keeper.GetPool(ctx)
-	require.True(t, expPool.Equal(resPool))
+	require.Equal(t, expPool, resPool)
 }