From a508990e0882ada1867640cd4361319295bc01b9 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 13 Jul 2017 16:20:21 +0200 Subject: [PATCH] Cleaned up fees and errors a bit from feedback --- errors/common.go | 10 ++++------ errors/main.go | 7 +++++++ modules/fee/error_test.go | 19 +++++++++++++++++++ modules/fee/errors.go | 9 +++++++++ modules/fee/handler.go | 11 +++++++++-- modules/fee/handler_test.go | 3 ++- 6 files changed, 50 insertions(+), 9 deletions(-) create mode 100644 modules/fee/error_test.go diff --git a/errors/common.go b/errors/common.go index 5eb4b1ee3..81d786bee 100644 --- a/errors/common.go +++ b/errors/common.go @@ -6,6 +6,7 @@ import ( "reflect" "github.com/pkg/errors" + abci "github.com/tendermint/abci/types" ) @@ -40,8 +41,7 @@ func unwrap(i interface{}) interface{} { func ErrUnknownTxType(tx interface{}) TMError { msg := fmt.Sprintf("%T", unwrap(tx)) - w := errors.Wrap(errUnknownTxType, msg) - return WithCode(w, abci.CodeType_UnknownRequest) + return WithMessage(msg, errUnknownTxType, abci.CodeType_UnknownRequest) } func IsUnknownTxTypeErr(err error) bool { return IsSameError(errUnknownTxType, err) @@ -49,16 +49,14 @@ func IsUnknownTxTypeErr(err error) bool { func ErrInvalidFormat(expected string, tx interface{}) TMError { msg := fmt.Sprintf("%T not %s", unwrap(tx), expected) - w := errors.Wrap(errInvalidFormat, msg) - return WithCode(w, abci.CodeType_UnknownRequest) + return WithMessage(msg, errInvalidFormat, abci.CodeType_UnknownRequest) } func IsInvalidFormatErr(err error) bool { return IsSameError(errInvalidFormat, err) } func ErrUnknownModule(mod string) TMError { - w := errors.Wrap(errUnknownModule, mod) - return WithCode(w, abci.CodeType_UnknownRequest) + return WithMessage(mod, errUnknownModule, abci.CodeType_UnknownRequest) } func IsUnknownModuleErr(err error) bool { return IsSameError(errUnknownModule, err) diff --git a/errors/main.go b/errors/main.go index 21f261148..a75a523c0 100644 --- a/errors/main.go +++ b/errors/main.go @@ -104,6 +104,13 @@ func WithCode(err error, code abci.CodeType) TMError { } } +// WithMessage prepends some text to the error, then calls WithCode +// It wraps the original error, so IsSameError will still match on err +func WithMessage(prefix string, err error, code abci.CodeType) TMError { + e2 := errors.WithMessage(err, prefix) + return WithCode(e2, code) +} + // New adds a stacktrace if necessary and sets the code and msg, // overriding the state if err was already TMError func New(msg string, code abci.CodeType) TMError { diff --git a/modules/fee/error_test.go b/modules/fee/error_test.go new file mode 100644 index 000000000..4d255b880 --- /dev/null +++ b/modules/fee/error_test.go @@ -0,0 +1,19 @@ +package fee + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestErrors(t *testing.T) { + assert := assert.New(t) + + e := ErrInsufficientFees() + assert.True(IsInsufficientFeesErr(e)) + assert.False(IsWrongFeeDenomErr(e)) + + e2 := ErrWrongFeeDenom("atom") + assert.False(IsInsufficientFeesErr(e2)) + assert.True(IsWrongFeeDenomErr(e2)) +} diff --git a/modules/fee/errors.go b/modules/fee/errors.go index cf2633039..06cdd10c5 100644 --- a/modules/fee/errors.go +++ b/modules/fee/errors.go @@ -5,11 +5,13 @@ import ( "fmt" abci "github.com/tendermint/abci/types" + "github.com/tendermint/basecoin/errors" ) var ( errInsufficientFees = fmt.Errorf("Insufficient Fees") + errWrongFeeDenom = fmt.Errorf("Required fee denomination") ) func ErrInsufficientFees() errors.TMError { @@ -18,3 +20,10 @@ func ErrInsufficientFees() errors.TMError { func IsInsufficientFeesErr(err error) bool { return errors.IsSameError(errInsufficientFees, err) } + +func ErrWrongFeeDenom(denom string) errors.TMError { + return errors.WithMessage(denom, errWrongFeeDenom, abci.CodeType_BaseInvalidInput) +} +func IsWrongFeeDenomErr(err error) bool { + return errors.IsSameError(errWrongFeeDenom, err) +} diff --git a/modules/fee/handler.go b/modules/fee/handler.go index 2ada56cb0..4e12a15a6 100644 --- a/modules/fee/handler.go +++ b/modules/fee/handler.go @@ -18,7 +18,11 @@ var Bank = basecoin.Actor{App: NameFee, Address: []byte("bank")} // SimpleFeeMiddleware - middleware for fee checking, constant amount // It used modules.coin to move the money type SimpleFeeMiddleware struct { - MinFee coin.Coin // + // the fee must be the same denomination and >= this amount + // if the amount is 0, then the fee tx wrapper is optional + MinFee coin.Coin + // all fees go here, which could be a dump (Bank) or something reachable + // by other app logic Collector basecoin.Actor stack.PassOption } @@ -60,8 +64,11 @@ func (h SimpleFeeMiddleware) doTx(ctx basecoin.Context, store state.KVStore, tx return res, errors.ErrInvalidFormat(TypeFees, tx) } - // see if it is big enough... + // see if it is the proper denom and big enough fee := feeTx.Fee + if fee.Denom != h.MinFee.Denom { + return res, ErrWrongFeeDenom(h.MinFee.Denom) + } if !fee.IsGTE(h.MinFee) { return res, ErrInsufficientFees() } diff --git a/modules/fee/handler_test.go b/modules/fee/handler_test.go index 15e28e1df..b775c2d56 100644 --- a/modules/fee/handler_test.go +++ b/modules/fee/handler_test.go @@ -6,12 +6,13 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/tendermint/tmlibs/log" + "github.com/tendermint/basecoin" "github.com/tendermint/basecoin/modules/coin" "github.com/tendermint/basecoin/modules/fee" "github.com/tendermint/basecoin/stack" "github.com/tendermint/basecoin/state" - "github.com/tendermint/tmlibs/log" ) func TestFeeChecks(t *testing.T) {