WIP Allow undelegation of more coins than delegated; Add more validity checks

Author: jaekwon

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,8 +35,8 @@ func (msg MsgSend) ValidateBasic() sdk.Error {
 	if msg.ToAddress.Empty() {
 		return sdk.ErrInvalidAddress("missing recipient address")
 	}
-	if !msg.Amount.IsAllPositive() {
-		return sdk.ErrInsufficientCoins("send amount must be positive")
+	if !msg.Amount.IsValid() {
+		return sdk.ErrInvalidCoins(msg.Amount.String())
 	}
 	return nil
 }
@@ -112,7 +112,7 @@ func (in Input) ValidateBasic() sdk.Error {
 	if !in.Coins.IsValid() {
 		return sdk.ErrInvalidCoins(in.Coins.String())
 	}
-	if !in.Coins.IsAllPositive() {
+	if !in.Coins.IsValid() {
 		return sdk.ErrInvalidCoins(in.Coins.String())
 	}
 	return nil
@@ -140,7 +140,7 @@ func (out Output) ValidateBasic() sdk.Error {
 	if !out.Coins.IsValid() {
 		return sdk.ErrInvalidCoins(out.Coins.String())
 	}
-	if !out.Coins.IsAllPositive() {
+	if !out.Coins.IsValid() {
 		return sdk.ErrInvalidCoins(out.Coins.String())
 	}
 	return nil