R4R: Improve SDK Error Messages & Allow Unicode

PENDING.md

@@ -34,6 +34,8 @@ IMPROVEMENTS
 * Gaia
 
 * SDK
+  * [\#3604] Improve SDK funds related error messages and allow for unicode in
+  JSON ABCI log.
 
 * Tendermint
 
@@ -50,4 +52,4 @@ BUG FIXES
 * SDK
   * [\#3582](https://github.com/cosmos/cosmos-sdk/pull/3582) Running `make test_unit was failing due to a missing tag
 
-* Tendermint
+* Tendermint

types/errors.go

@@ -1,13 +1,14 @@
 package types
 
 import (
+	"bytes"
+	"encoding/json"
 	"fmt"
 	"strings"
 
+	"github.com/pkg/errors"
 	cmn "github.com/tendermint/tendermint/libs/common"
 
-	"github.com/cosmos/cosmos-sdk/codec"
-
 	abci "github.com/tendermint/tendermint/abci/types"
 )
 
@@ -246,19 +247,22 @@ func (err *sdkError) Code() CodeType {
 
 // Implements ABCIError.
 func (err *sdkError) ABCILog() string {
-	cdc := codec.New()
 	errMsg := err.cmnError.Error()
 	jsonErr := humanReadableError{
 		Codespace: err.codespace,
 		Code:      err.code,
 		Message:   errMsg,
 	}
-	bz, er := cdc.MarshalJSON(jsonErr)
-	if er != nil {
-		panic(er)
+
+	var buff bytes.Buffer
+	enc := json.NewEncoder(&buff)
+	enc.SetEscapeHTML(false)
+
+	if err := enc.Encode(jsonErr); err != nil {
+		panic(errors.Wrap(err, "failed to encode ABCI error log"))
 	}
-	stringifiedJSON := string(bz)
-	return stringifiedJSON
+
+	return strings.TrimSpace(buff.String())
 }
 
 func (err *sdkError) Result() Result {

types/errors_test.go

@@ -72,18 +72,23 @@ func TestAppendMsgToErr(t *testing.T) {
 
 		// plain msg error
 		msg := AppendMsgToErr("something unexpected happened", errMsg)
-		require.Equal(t, fmt.Sprintf("something unexpected happened; %s",
-			errMsg),
+		require.Equal(
+			t,
+			fmt.Sprintf("something unexpected happened; %s", errMsg),
 			msg,
-			fmt.Sprintf("Should have formatted the error message of ABCI Log. tc #%d", i))
+			fmt.Sprintf("Should have formatted the error message of ABCI Log. tc #%d", i),
+		)
 
 		// ABCI Log msg error
 		msg = AppendMsgToErr("something unexpected happened", abciLog)
 		msgIdx := mustGetMsgIndex(abciLog)
-		require.Equal(t, fmt.Sprintf("%s%s; %s}",
-			abciLog[:msgIdx],
-			"something unexpected happened",
-			abciLog[msgIdx:len(abciLog)-1]),
+		require.Equal(
+			t,
+			fmt.Sprintf("%s%s; %s}",
+				abciLog[:msgIdx],
+				"something unexpected happened",
+				abciLog[msgIdx:len(abciLog)-1],
+			),
 			msg,
 			fmt.Sprintf("Should have formatted the error message of ABCI Log. tc #%d", i))
 	}

x/auth/ante.go

@@ -306,20 +306,22 @@ func DeductFees(blockTime time.Time, acc Account, fee StdFee) (Account, sdk.Resu
 	// get the resulting coins deducting the fees
 	newCoins, ok := coins.SafeMinus(feeAmount)
 	if ok {
-		errMsg := fmt.Sprintf("%s < %s", coins, feeAmount)
-		return nil, sdk.ErrInsufficientFunds(errMsg).Result()
+		return nil, sdk.ErrInsufficientFunds(
+			fmt.Sprintf("insufficient funds to pay for fees; %s < %s", coins, feeAmount),
+		).Result()
 	}
 
 	// Validate the account has enough "spendable" coins as this will cover cases
 	// such as vesting accounts.
 	spendableCoins := acc.SpendableCoins(blockTime)
 	if _, hasNeg := spendableCoins.SafeMinus(feeAmount); hasNeg {
-		return nil, sdk.ErrInsufficientFunds(fmt.Sprintf("%s < %s", spendableCoins, feeAmount)).Result()
+		return nil, sdk.ErrInsufficientFunds(
+			fmt.Sprintf("insufficient funds to pay for fees; %s < %s", spendableCoins, feeAmount),
+		).Result()
 	}
 
 	if err := acc.SetCoins(newCoins); err != nil {
-		// Handle w/ #870
-		panic(err)
+		return nil, sdk.ErrInternal(err.Error()).Result()
 	}
 
 	return acc, sdk.Result{}

x/bank/keeper.go

@@ -230,7 +230,9 @@ func subtractCoins(ctx sdk.Context, ak auth.AccountKeeper, addr sdk.AccAddress,
 	// So the check here is sufficient instead of subtracting from oldCoins.
 	_, hasNeg := spendableCoins.SafeMinus(amt)
 	if hasNeg {
-		return amt, nil, sdk.ErrInsufficientCoins(fmt.Sprintf("%s < %s", spendableCoins, amt))
+		return amt, nil, sdk.ErrInsufficientCoins(
+			fmt.Sprintf("insufficient account funds; %s < %s", spendableCoins, amt),
+		)
 	}
 
 	newCoins := oldCoins.Minus(amt) // should not panic as spendable coins was already checked
@@ -246,7 +248,9 @@ func addCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt s
 	newCoins := oldCoins.Plus(amt)
 
 	if newCoins.IsAnyNegative() {
-		return amt, nil, sdk.ErrInsufficientCoins(fmt.Sprintf("%s < %s", oldCoins, amt))
+		return amt, nil, sdk.ErrInsufficientCoins(
+			fmt.Sprintf("insufficient account funds; %s < %s", oldCoins, amt),
+		)
 	}
 
 	err := setCoins(ctx, am, addr, newCoins)
@@ -319,7 +323,9 @@ func delegateCoins(
 
 	_, hasNeg := oldCoins.SafeMinus(amt)
 	if hasNeg {
-		return nil, sdk.ErrInsufficientCoins(fmt.Sprintf("%s < %s", oldCoins, amt))
+		return nil, sdk.ErrInsufficientCoins(
+			fmt.Sprintf("insufficient account funds; %s < %s", oldCoins, amt),
+		)
 	}
 
 	if err := trackDelegation(acc, ctx.BlockHeader().Time, amt); err != nil {