Deduct min fee on error

cmd/bcpd/app/app.go

@@ -48,12 +48,9 @@ func Chain(authFn x.Authenticator) app.Decorators {
 		utils.NewSavepoint().OnCheck(),
 		sigs.NewDecorator(),
 		multisig.NewDecorator(authFn),
-		cash.NewFeeDecorator(authFn, ctrl),
+		cash.NewDynamicFeeDecorator(authFn, ctrl),
 		// cannot pay for fee with hashlock...
 		hashlock.NewDecorator(),
-		// on DeliverTx, bad tx will increment nonce and take fee
-		// even if the message fails
-		utils.NewSavepoint().OnDeliver(),
 		// make sure we execute all the transactions in batch after savepoint
 		batch.NewDecorator(),
 	)

cmd/bcpd/app/app_test.go

@@ -1,7 +1,6 @@
 package app
 
 import (
-	"bytes"
 	"encoding/binary"
 	"encoding/hex"
 	"encoding/json"
@@ -228,7 +227,7 @@ func withWalletAppState(t require.TestingT, accounts []*account) string {
 		Gconf map[string]interface{} `json:"gconf"`
 	}{
 		Gconf: map[string]interface{}{
-			cash.GconfCollectorAddress: "fake-collector-address",
+			cash.GconfCollectorAddress: "66616b652d636f6c6c6563746f722d61646472657373",
 			cash.GconfMinimalFee:       coin.Coin{}, // no fee
 		},
 	}
@@ -272,38 +271,37 @@ func (c *contract) signers() []*account {
 }
 
 func appStateGenesis(t require.TestingT, contracts []*contract) string {
-	var buff bytes.Buffer
-	for i, acc := range contracts {
-		_, err := buff.WriteString(fmt.Sprintf(`{
-            "name": "wallet%d",
-			"address": "%X",
-			"coins": [{
-                "whole": 50000,
-                "ticker": "ETH"
-            },{
-                "whole": 1234,
-				"ticker": "FRNK"
-			}]
-		},`, i, acc.address()))
-		require.NoError(t, err)
+	type dt map[string]interface{}
+	type arr []interface{}
+
+	var wallets arr
+	for i, c := range contracts {
+		wallets = append(wallets, dt{
+			"name":    fmt.Sprintf("wallet%d", i),
+			"address": c.address(),
+			"coins": arr{
+				dt{"whole": 50000, "ticker": "ETH"},
+				dt{"whole": 1234, "ticker": "FRNK"},
+			},
+		})
 	}
 
-	walletStr := buff.String()
-	walletStr = walletStr[:len(walletStr)-1]
-	appState := fmt.Sprintf(`{
-		"wallets": [%s],
-        "currencies": [{
-            "ticker": "ETH",
-            "name": "Smells like ethereum",
-            "sig_figs": 9
-        },{
-            "ticker": "FRNK",
-            "name": "Frankie",
-            "sig_figs": 3
-		}]
-	}`, walletStr)
-
-	return appState
+	state := dt{
+		"wallets": wallets,
+		"currencies": arr{
+			dt{"ticker": "ETH", "name": "Smells like ethereum", "sig_figs": 9},
+			dt{"ticker": "FRNK", "name": "Frankie", "sig_figs": 3},
+		},
+		"gconf": dt{
+			"cash:collector_address": "66616b652d636f6c6c6563746f722d61646472657373",
+		},
+	}
+
+	b, err := json.MarshalIndent(state, "", "\t")
+	if err != nil {
+		panic(err)
+	}
+	return string(b)
 }
 
 // sendToken creates the transaction, signs it and sends it
@@ -392,8 +390,7 @@ func sendBatch(t require.TestingT, fail bool, baseApp app.BaseApp, chainID strin
 
 // createContract creates an immutable contract, signs the transaction and sends it
 // checks contract has been created correctly
-func createContract(t require.TestingT, baseApp app.BaseApp, chainID string, height int64, signers []*account,
-	activationThreshold int64, contractSigs ...[]byte) []byte {
+func createContract(t require.TestingT, baseApp app.BaseApp, chainID string, height int64, signers []*account, activationThreshold int64, contractSigs ...[]byte) []byte {
 	msg := &multisig.CreateContractMsg{
 		Sigs:                contractSigs,
 		ActivationThreshold: activationThreshold,
@@ -420,8 +417,7 @@ func createContract(t require.TestingT, baseApp app.BaseApp, chainID string, hei
 
 // signAndCommit signs tx with signatures from signers and submits to the chain
 // asserts and fails the test in case of errors during the process
-func signAndCommit(t require.TestingT, fail bool, app app.BaseApp, tx *Tx, signers []*account, chainID string,
-	height int64) abci.ResponseDeliverTx {
+func signAndCommit(t require.TestingT, fail bool, app app.BaseApp, tx *Tx, signers []*account, chainID string, height int64) abci.ResponseDeliverTx {
 	for _, signer := range signers {
 		sig, err := sigs.SignTx(signer.pk, tx, chainID, signer.nonce())
 		require.NoError(t, err)

cmd/bnsd/app/app.go

@@ -50,15 +50,12 @@ func Chain(authFn x.Authenticator) app.Decorators {
 		utils.NewSavepoint().OnCheck(),
 		sigs.NewDecorator(),
 		multisig.NewDecorator(authFn),
-		cash.NewFeeDecorator(authFn, ctrl),
+		cash.NewDynamicFeeDecorator(authFn, ctrl),
 		// cannot pay for fee with hashlock...
 		hashlock.NewDecorator(),
 		// batch commented out temporarily to minimize release features
 		// make sure we execute all the transactions in batch before the save point
 		//batch.NewDecorator(),
-		// on DeliverTx, bad tx will increment nonce and take fee
-		// even if the message fails
-		utils.NewSavepoint().OnDeliver(),
 	)
 }
 

cmd/bnsd/app/testdata/fixtures/app.go

@@ -91,7 +91,7 @@ func appStateGenesis(keyAddress weave.Address) []byte {
 			},
 		},
 		Gconf: map[string]interface{}{
-			cash.GconfCollectorAddress: "fake-collector-address",
+			cash.GconfCollectorAddress: "66616b652d636f6c6c6563746f722d61646472657373",
 			cash.GconfMinimalFee:       coin.Coin{Whole: 0}, // no fee
 		},
 	}

examples/mycoind/app/app_test.go

@@ -38,7 +38,7 @@ func testInitChain(t *testing.T, myApp app.BaseApp, addr string) {
 			},
 		},
 		"gconf": dict{
-			cash.GconfCollectorAddress: "fake-collector-address",
+			cash.GconfCollectorAddress: "66616b652d636f6c6c6563746f722d61646472657373",
 			cash.GconfMinimalFee:       coin.Coin{Whole: 0}, // no fee
 		},
 	})

examples/mycoind/app/init.go

@@ -56,7 +56,7 @@ func GenInitOptions(args []string) (json.RawMessage, error) {
 			},
 		},
 		"gconf": dict{
-			cash.GconfCollectorAddress: "fake-collector-address",
+			cash.GconfCollectorAddress: "66616b652d636f6c6c6563746f722d61646472657373",
 			cash.GconfMinimalFee:       coin.Coin{Whole: 0}, // no fee
 		},
 	})

x/cash/controller.go

@@ -6,13 +6,18 @@ import (
 	"github.com/iov-one/weave/errors"
 )
 
+// CoinsMover is an interface for moving coins between accounts.
+type CoinMover interface {
+	// Moving coins must happen from the source to the destination address.
+	// Zero or negative values must result in an error.
+	MoveCoins(store weave.KVStore, src weave.Address, dest weave.Address, amount coin.Coin) error
+}
+
 // Controller is the functionality needed by cash.Handler and cash.Decorator.
 // BaseController should work plenty fine, but you can add other logic if so
 // desired
 type Controller interface {
-	// MoveCoins removes funds from the source account and adds them to the
-	// destination account. This operation is atomic.
-	MoveCoins(store weave.KVStore, src weave.Address, dest weave.Address, amount coin.Coin) error
+	CoinMover
 
 	// IssueCoins increase the number of funds on given accouunt by a
 	// specified amount.
@@ -54,6 +59,9 @@ func (c BaseController) Balance(store weave.KVStore, src weave.Address) (coin.Co
 func (c BaseController) MoveCoins(store weave.KVStore,
 	src weave.Address, dest weave.Address, amount coin.Coin) error {
 
+	if amount.IsZero() {
+		return errors.ErrInvalidAmount.New("zero value")
+	}
 	if !amount.IsPositive() {
 		return errors.ErrInvalidAmount.Newf("non-positive SendMsg: %#v", &amount)
 	}

x/cash/dynamicfee.go

@@ -0,0 +1,192 @@
+/*
+
+DynamicFeeDecorator is an enhanced version the basic FeeDecorator with better
+handling of transaction errors and ability to deduct/enforce app-specific fees.
+
+The business logic is:
+1. If a transaction fee < min fee, or a transaction fee cannot be paid, reject
+   it with an error.
+2. Run the transaction.
+3. If a transaction processing results in an error, revert all transaction
+   changes and charge only the min fee.
+
+TODO: If a transaction succeeded, but requested a RequiredFee higher than paid
+fee, revert all transaction changes and refund all but the min fee, returning
+an error.
+
+If a transaction succeeded, and at least RequiredFee was paid, everything is
+committed and we return success
+
+It also embeds a checkpoint inside, so in the typical application stack:
+
+	cash.NewFeeDecorator(authFn, ctrl),
+	utils.NewSavepoint().OnDeliver(),
+
+can be replaced by
+
+	cash.NewDynamicFeeDecorator(authFn, ctrl),
+
+As with FeeDecorator, all deducted fees are send to the collector, whose
+address is configured via gconf package.
+
+*/
+
+package cash
+
+import (
+	"github.com/iov-one/weave"
+	coin "github.com/iov-one/weave/coin"
+	"github.com/iov-one/weave/errors"
+	"github.com/iov-one/weave/gconf"
+	"github.com/iov-one/weave/x"
+)
+
+type DynamicFeeDecorator struct {
+	auth x.Authenticator
+	ctrl CoinMover
+}
+
+var _ weave.Decorator = DynamicFeeDecorator{}
+
+// NewDynamicFeeDecorator returns a DynamicFeeDecorator with the given
+// minimum fee, and all collected fees going to a default address.
+func NewDynamicFeeDecorator(auth x.Authenticator, ctrl Controller) DynamicFeeDecorator {
+	return DynamicFeeDecorator{
+		auth: auth,
+		ctrl: ctrl,
+	}
+}
+
+// Check verifies and deducts fees before calling down the stack
+func (d DynamicFeeDecorator) Check(ctx weave.Context, store weave.KVStore, tx weave.Tx, next weave.Checker) (res weave.CheckResult, ferr error) {
+	fee, payer, cache, err := d.prepare(ctx, store, tx)
+	if err != nil {
+		return res, errors.Wrap(err, "cannot prepare")
+	}
+
+	defer func() {
+		if ferr == nil {
+			cache.Write()
+			res.GasPayment += toPayment(fee)
+		} else {
+			cache.Discard()
+			_ = d.chargeMinimalFee(store, payer)
+		}
+	}()
+
+	if err := d.chargeFee(cache, payer, fee); err != nil {
+		return res, errors.Wrap(err, "cannot charge fee")
+	}
+	return next.Check(ctx, cache, tx)
+}
+
+// Deliver verifies and deducts fees before calling down the stack
+func (d DynamicFeeDecorator) Deliver(ctx weave.Context, store weave.KVStore, tx weave.Tx, next weave.Deliverer) (res weave.DeliverResult, ferr error) {
+	fee, payer, cache, err := d.prepare(ctx, store, tx)
+	if err != nil {
+		return res, errors.Wrap(err, "cannot prepare")
+	}
+
+	defer func() {
+		if ferr == nil {
+			cache.Write()
+		} else {
+			cache.Discard()
+			_ = d.chargeMinimalFee(store, payer)
+		}
+	}()
+
+	if err := d.chargeFee(cache, payer, fee); err != nil {
+		return weave.DeliverResult{}, errors.Wrap(err, "cannot charge fee")
+	}
+	return next.Deliver(ctx, cache, tx)
+}
+
+func (d DynamicFeeDecorator) chargeFee(store weave.KVStore, src weave.Address, amount coin.Coin) error {
+	if amount.IsZero() {
+		return nil
+	}
+	dest := gconf.Address(store, GconfCollectorAddress)
+	return d.ctrl.MoveCoins(store, src, dest, amount)
+}
+
+// chargeMinimalFee deduct an anty span fee from a given account.
+func (d DynamicFeeDecorator) chargeMinimalFee(store weave.KVStore, src weave.Address) error {
+	fee := gconf.Coin(store, GconfMinimalFee)
+	if fee.IsZero() {
+		return nil
+	}
+	if fee.Ticker == "" {
+		return errors.ErrHuman.New("minimal fee without a ticker")
+	}
+	return d.chargeFee(store, src, fee)
+}
+
+// prepare is all shared setup between Check and Deliver. It computes the fee
+// for the transaction, ensures that the payer is authenticated and prepares
+// the database transaction.
+func (d DynamicFeeDecorator) prepare(ctx weave.Context, store weave.KVStore, tx weave.Tx) (fee coin.Coin, payer weave.Address, cache weave.KVCacheWrap, err error) {
+	finfo, err := d.extractFee(ctx, tx, store)
+	if err != nil {
+		return fee, payer, cache, errors.Wrap(err, "cannot extract fee")
+	}
+	// Dererefence the fees (handling nil).
+	if pfee := finfo.GetFees(); pfee != nil {
+		fee = *pfee
+	}
+	payer = finfo.GetPayer()
+
+	// Verify we have access to the money.
+	if !d.auth.HasAddress(ctx, payer) {
+		err := errors.ErrUnauthorized.New("fee payer signature missing")
+		return fee, payer, cache, err
+	}
+
+	// Ensure we can execute subtransactions (see check on utils.Savepoint).
+	cstore, ok := store.(weave.CacheableKVStore)
+	if !ok {
+		err = errors.ErrInternal.New("need cachable kvstore")
+		return fee, payer, cache, err
+	}
+	cache = cstore.CacheWrap()
+	return fee, payer, cache, nil
+}
+
+// this returns the fee info to deduct and the error if incorrectly set
+func (d DynamicFeeDecorator) extractFee(ctx weave.Context, tx weave.Tx, store weave.KVStore) (*FeeInfo, error) {
+	var finfo *FeeInfo
+	ftx, ok := tx.(FeeTx)
+	if ok {
+		payer := x.MainSigner(ctx, d.auth).Address()
+		finfo = ftx.GetFees().DefaultPayer(payer)
+	}
+
+	txFee := finfo.GetFees()
+	if coin.IsEmpty(txFee) {
+		minFee := gconf.Coin(store, GconfMinimalFee)
+		if minFee.IsZero() {
+			return finfo, nil
+		}
+		return nil, errors.ErrInsufficientAmount.New("zero transaction fee is not allowed")
+	}
+
+	if err := finfo.Validate(); err != nil {
+		return nil, errors.Wrap(err, "invalid fee")
+	}
+
+	minFee := gconf.Coin(store, GconfMinimalFee)
+	if minFee.IsZero() {
+		return finfo, nil
+	}
+	if minFee.Ticker == "" {
+		return nil, errors.ErrHuman.New("minumal fee curency not set")
+	}
+	if !txFee.SameType(minFee) {
+		return nil, coin.ErrInvalidCurrency.Newf("min fee is %s and tx fee is %s", minFee.Ticker, txFee.Ticker)
+
+	}
+	if !txFee.IsGTE(minFee) {
+		return nil, errors.ErrInsufficientAmount.Newf("transaction fee less than minimum: %v", txFee)
+	}
+	return finfo, nil
+}