From eb2380383306fa281926fbd2fea94f84ea62a7a9 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Tue, 13 Mar 2018 13:46:08 +0100 Subject: [PATCH 01/10] remove tx.GetFeePayer --- baseapp/baseapp_test.go | 1 - docs/guide.md | 13 +++---------- docs/sdk/overview.rst | 11 ----------- examples/kvstore/tx.go | 4 ---- mock/tx.go | 4 ---- types/tx_msg.go | 12 +++++++----- 6 files changed, 10 insertions(+), 35 deletions(-) diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index 5cc20185d4..de9a0253c5 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -328,7 +328,6 @@ func (tx testUpdatePowerTx) GetMsg() sdk.Msg { return tx func (tx testUpdatePowerTx) GetSignBytes() []byte { return nil } func (tx testUpdatePowerTx) ValidateBasic() sdk.Error { return nil } func (tx testUpdatePowerTx) GetSigners() []sdk.Address { return nil } -func (tx testUpdatePowerTx) GetFeePayer() sdk.Address { return nil } func (tx testUpdatePowerTx) GetSignatures() []sdk.StdSignature { return nil } func TestValidatorChange(t *testing.T) { diff --git a/docs/guide.md b/docs/guide.md index 1a6d21c82a..5c31d2e271 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -105,14 +105,6 @@ type Tx interface { GetMsg() Msg - // The address that pays the base fee for this message. The fee is - // deducted before the Msg is processed. - GetFeePayer() Address - - // Get the canonical byte representation of the Tx. - // Includes any signatures (or empty slots). - GetTxBytes() []byte - // Signatures returns the signature of signers who signed the Msg. // CONTRACT: Length returned is same as length of // pubkeys returned from MsgKeySigners, and the order @@ -148,8 +140,9 @@ case of Basecoin, the public key only needs to be included in the first transaction send by a given account - after that, the public key is forever stored by the application and can be left out of transactions. -Transactions can also specify the address responsible for paying the -transaction's fees using the `tx.GetFeePayer()` method. +The address responsible for paying the transactions fee is the first address +returned by msg.GetSigners(). The convenience function `FeePayer(tx Tx)` is provided +to return this. The standard way to create a transaction from a message is to use the `StdTx`: diff --git a/docs/sdk/overview.rst b/docs/sdk/overview.rst index bf7b23a606..9e79dd04ff 100644 --- a/docs/sdk/overview.rst +++ b/docs/sdk/overview.rst @@ -219,14 +219,6 @@ A transaction is a message with additional information for authentication: GetMsg() Msg - // The address that pays the base fee for this message. The fee is - // deducted before the Msg is processed. - GetFeePayer() Address - - // Get the canonical byte representation of the Tx. - // Includes any signatures (or empty slots). - GetTxBytes() []byte - // Signatures returns the signature of signers who signed the Msg. // CONTRACT: Length returned is same as length of // pubkeys returned from MsgKeySigners, and the order @@ -261,9 +253,6 @@ case of Basecoin, the public key only needs to be included in the first transaction send by a given account - after that, the public key is forever stored by the application and can be left out of transactions. -Transactions can also specify the address responsible for paying the -transaction's fees using the ``tx.GetFeePayer()`` method. - The standard way to create a transaction from a message is to use the ``StdTx``: :: diff --git a/examples/kvstore/tx.go b/examples/kvstore/tx.go index fdecf63807..c9c30c885d 100644 --- a/examples/kvstore/tx.go +++ b/examples/kvstore/tx.go @@ -51,10 +51,6 @@ func (tx kvstoreTx) GetSignatures() []sdk.StdSignature { return nil } -func (tx kvstoreTx) GetFeePayer() sdk.Address { - return nil -} - // takes raw transaction bytes and decodes them into an sdk.Tx. An sdk.Tx has // all the signatures and can be used to authenticate. func decodeTx(txBytes []byte) (sdk.Tx, sdk.Error) { diff --git a/mock/tx.go b/mock/tx.go index efe60feb91..326946eaa5 100644 --- a/mock/tx.go +++ b/mock/tx.go @@ -64,10 +64,6 @@ func (tx kvstoreTx) GetSignatures() []sdk.StdSignature { return nil } -func (tx kvstoreTx) GetFeePayer() sdk.Address { - return nil -} - // takes raw transaction bytes and decodes them into an sdk.Tx. An sdk.Tx has // all the signatures and can be used to authenticate. func decodeTx(txBytes []byte) (sdk.Tx, sdk.Error) { diff --git a/types/tx_msg.go b/types/tx_msg.go index 141f9d050d..8510237487 100644 --- a/types/tx_msg.go +++ b/types/tx_msg.go @@ -33,10 +33,6 @@ type Tx interface { // Gets the Msg. GetMsg() Msg - // The address that pays the base fee for this message. The fee is - // deducted before the Msg is processed. - GetFeePayer() Address - // Signatures returns the signature of signers who signed the Msg. // CONTRACT: Length returned is same as length of // pubkeys returned from MsgKeySigners, and the order @@ -63,9 +59,15 @@ func NewStdTx(msg Msg, sigs []StdSignature) StdTx { } } +// FeePayer returns the address responsible for paying the fees +// for the transactions. It's the first address returned by msg.GetSigners(). +// If GetSigners() is empty, this panics. +func FeePayer(tx Tx) Address { + return tx.GetMsg().GetSigners()[0] +} + //nolint func (tx StdTx) GetMsg() Msg { return tx.Msg } -func (tx StdTx) GetFeePayer() Address { return tx.Signatures[0].PubKey.Address() } // XXX but PubKey is optional! func (tx StdTx) GetSignatures() []StdSignature { return tx.Signatures } // StdSignDoc is replay-prevention structure. From be7cb6c96c7a923be818894fef6eb94b9c8c2106 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Tue, 13 Mar 2018 13:50:21 +0100 Subject: [PATCH 02/10] types: introduce StdFee --- types/tx_msg.go | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/types/tx_msg.go b/types/tx_msg.go index 8510237487..b829bef961 100644 --- a/types/tx_msg.go +++ b/types/tx_msg.go @@ -49,6 +49,7 @@ var _ Tx = (*StdTx)(nil) // NOTE: the first signature is the FeePayer (Signatures must not be nil). type StdTx struct { Msg `json:"msg"` + Fee StdFee `json:"fee"` Signatures []StdSignature `json:"signatures"` } @@ -59,6 +60,16 @@ func NewStdTx(msg Msg, sigs []StdSignature) StdTx { } } +// SetFee sets the StdFee on the transaction. +func (tx StdTx) SetFee(fee StdFee) StdTx { + tx.Fee = fee + return tx +} + +//nolint +func (tx StdTx) GetMsg() Msg { return tx.Msg } +func (tx StdTx) GetSignatures() []StdSignature { return tx.Signatures } + // FeePayer returns the address responsible for paying the fees // for the transactions. It's the first address returned by msg.GetSigners(). // If GetSigners() is empty, this panics. @@ -66,9 +77,20 @@ func FeePayer(tx Tx) Address { return tx.GetMsg().GetSigners()[0] } -//nolint -func (tx StdTx) GetMsg() Msg { return tx.Msg } -func (tx StdTx) GetSignatures() []StdSignature { return tx.Signatures } +// StdFee includes the amount of coins paid in fees and the maximum +// gas to be used by the transaction. The ratio yields an effectie "gasprice", +// which must be above some miminum to be accepted into the mempool. +type StdFee struct { + Amount Coins `json"amount"` + Gas int64 `json"gas"` +} + +func NewStdFee(gas int64, amount ...Coin) StdFee { + return StdFee{ + Amount: amount, + Gas: gas, + } +} // StdSignDoc is replay-prevention structure. // It includes the result of msg.GetSignBytes(), From 45f8ccbe90bed049ae6e0544e9b2320ce554199d Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Wed, 14 Mar 2018 18:16:52 +0100 Subject: [PATCH 03/10] x/auth: cleanup ante handler --- types/errors.go | 6 ++++++ types/tx_msg.go | 2 +- x/auth/ante.go | 45 ++++++++++++++++++++++++--------------------- 3 files changed, 31 insertions(+), 22 deletions(-) diff --git a/types/errors.go b/types/errors.go index 9d8175e309..008bd6f086 100644 --- a/types/errors.go +++ b/types/errors.go @@ -27,6 +27,7 @@ const ( CodeInsufficientFunds CodeType = 5 CodeUnknownRequest CodeType = 6 CodeUnrecognizedAddress CodeType = 7 + CodeMissingPubKey CodeType = 8 CodeGenesisParse CodeType = 0xdead // TODO: remove ? ) @@ -50,6 +51,8 @@ func CodeToDefaultMsg(code CodeType) string { return "Unknown request" case CodeUnrecognizedAddress: return "Unrecognized address" + case CodeMissingPubKey: + return "Missing pubkey" default: return fmt.Sprintf("Unknown code %d", code) } @@ -84,6 +87,9 @@ func ErrUnknownRequest(msg string) Error { func ErrUnrecognizedAddress(addr Address) Error { return newError(CodeUnrecognizedAddress, addr.String()) } +func ErrMissingPubKey(addr Address) Error { + return newError(CodeMissingPubKey, addr.String()) +} //---------------------------------------- // Error & sdkError diff --git a/types/tx_msg.go b/types/tx_msg.go index b829bef961..81719f18a7 100644 --- a/types/tx_msg.go +++ b/types/tx_msg.go @@ -78,7 +78,7 @@ func FeePayer(tx Tx) Address { } // StdFee includes the amount of coins paid in fees and the maximum -// gas to be used by the transaction. The ratio yields an effectie "gasprice", +// gas to be used by the transaction. The ratio yields an effective "gasprice", // which must be above some miminum to be accepted into the mempool. type StdFee struct { Amount Coins `json"amount"` diff --git a/x/auth/ante.go b/x/auth/ante.go index f2495af78d..f602f8911b 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -7,6 +7,9 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" ) +// NewAnteHandler returns an AnteHandler that checks +// and increments sequence numbers, checks signatures, +// and deducts fees from the first signer. func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { return func( ctx sdk.Context, tx sdk.Tx, @@ -31,32 +34,20 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { true } - // Collect accounts to set in the context - var signerAccs = make([]sdk.Account, len(signerAddrs)) - - // Get the sign bytes by collecting all sequence numbers + // Get the sign bytes (requires all sequence numbers) sequences := make([]int64, len(signerAddrs)) for i := 0; i < len(signerAddrs); i++ { sequences[i] = sigs[i].Sequence } signBytes := sdk.StdSignBytes(ctx.ChainID(), sequences, msg) - // Check fee payer sig and nonce, and deduct fee. - // This is done first because it only - // requires fetching 1 account. - payerAddr, payerSig := signerAddrs[0], sigs[0] - payerAcc, res := processSig(ctx, accountMapper, payerAddr, payerSig, signBytes) - if !res.IsOK() { - return ctx, res, true - } - signerAccs[0] = payerAcc - // TODO: Charge fee from payerAcc. - // TODO: accountMapper.SetAccount(ctx, payerAddr) + // Check sig and nonce and collect signer accounts. + var signerAccs = make([]sdk.Account, len(signerAddrs)) + for i := 0; i < len(sigs); i++ { + isFeePayer := i == 0 // first sig pays the fees - // Check sig and nonce for the rest. - for i := 1; i < len(sigs); i++ { signerAddr, sig := signerAddrs[i], sigs[i] - signerAcc, res := processSig(ctx, accountMapper, signerAddr, sig, signBytes) + signerAcc, res := processSig(ctx, accountMapper, signerAddr, sig, signBytes, isFeePayer) if !res.IsOK() { return ctx, res, true } @@ -64,13 +55,17 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { } ctx = WithSigners(ctx, signerAccs) + // TODO: tx tags (?) return ctx, sdk.Result{}, false // continue... } } // verify the signature and increment the sequence. -// if the account doesn't have a pubkey, set it as well. -func processSig(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address, sig sdk.StdSignature, signBytes []byte) (acc sdk.Account, res sdk.Result) { +// if the account doesn't have a pubkey, set it. +// deduct fee from fee payer. +func processSig(ctx sdk.Context, am sdk.AccountMapper, + addr sdk.Address, sig sdk.StdSignature, signBytes []byte, + isFeePayer bool) (acc sdk.Account, res sdk.Result) { // Get the account acc = am.GetAccount(ctx, addr) @@ -86,7 +81,8 @@ func processSig(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address, sig sdk } acc.SetSequence(seq + 1) - // Check and possibly set pubkey. + // If pubkey is not known for account, + // set it from the StdSignature pubKey := acc.GetPubKey() if pubKey.Empty() { if sig.PubKey.Empty() { @@ -97,6 +93,9 @@ func processSig(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address, sig sdk fmt.Sprintf("invalid PubKey for address %v", addr)).Result() } pubKey = sig.PubKey + if pubKey.Empty() { + return nil, sdk.ErrMissingPubKey(addr).Result() + } err := acc.SetPubKey(pubKey) if err != nil { return nil, sdk.ErrInternal("setting PubKey on signer").Result() @@ -107,6 +106,10 @@ func processSig(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address, sig sdk return nil, sdk.ErrUnauthorized("signature verification failed").Result() } + if isFeePayer { + // TODO: pay fees + } + // Save the account. am.SetAccount(ctx, acc) return From 1b16f0c68429518b07ab9f28edad3caf727b7197 Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Thu, 15 Mar 2018 17:37:16 +0100 Subject: [PATCH 04/10] Deduct fee from fee payer's account balance --- x/auth/ante.go | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/x/auth/ante.go b/x/auth/ante.go index f602f8911b..724b7c5ef6 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -26,6 +26,12 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { // TODO: can tx just implement message? msg := tx.GetMsg() + // TODO: will this always be a stdtx? should that be used in the function signature? + stdTx, ok := tx.(sdk.StdTx) + if !ok { + return ctx, sdk.ErrInternal("tx must be sdk.StdTx").Result(), true + } + // Assert that number of signatures is correct. var signerAddrs = msg.GetSigners() if len(sigs) != len(signerAddrs) { @@ -47,7 +53,8 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { isFeePayer := i == 0 // first sig pays the fees signerAddr, sig := signerAddrs[i], sigs[i] - signerAcc, res := processSig(ctx, accountMapper, signerAddr, sig, signBytes, isFeePayer) + signerAcc, res := processSig(ctx, accountMapper, signerAddr, sig, + signBytes, isFeePayer, stdTx.Fee.Amount) if !res.IsOK() { return ctx, res, true } @@ -65,7 +72,7 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { // deduct fee from fee payer. func processSig(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address, sig sdk.StdSignature, signBytes []byte, - isFeePayer bool) (acc sdk.Account, res sdk.Result) { + isFeePayer bool, feeAmount sdk.Coins) (acc sdk.Account, res sdk.Result) { // Get the account acc = am.GetAccount(ctx, addr) @@ -106,8 +113,16 @@ func processSig(ctx sdk.Context, am sdk.AccountMapper, return nil, sdk.ErrUnauthorized("signature verification failed").Result() } + // If this is the fee payer, deduct the fee. if isFeePayer { - // TODO: pay fees + coins := acc.GetCoins() + newCoins := coins.Minus(feeAmount) + if !newCoins.IsNotNegative() { + errMsg := fmt.Sprintf("%s < %s", coins, feeAmount) + return nil, sdk.ErrInsufficientFunds(errMsg).Result() + } + + acc.SetCoins(newCoins) } // Save the account. From 6dc46064cb7e0ac678769e8ffb38aec6a1bd75a6 Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Thu, 15 Mar 2018 17:37:39 +0100 Subject: [PATCH 05/10] Started on fee tests --- x/auth/ante_test.go | 50 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go index c3df2199d6..9523cdb39e 100644 --- a/x/auth/ante_test.go +++ b/x/auth/ante_test.go @@ -55,7 +55,7 @@ func checkInvalidTx(t *testing.T, anteHandler sdk.AnteHandler, ctx sdk.Context, assert.Equal(t, code, result.Code) } -func newTestTx(ctx sdk.Context, msg sdk.Msg, privs []crypto.PrivKey, seqs []int64) sdk.Tx { +func newTestTx(ctx sdk.Context, msg sdk.Msg, privs []crypto.PrivKey, seqs []int64, feeAmount int64) sdk.Tx { signBytes := sdk.StdSignBytes(ctx.ChainID(), seqs, msg) return newTestTxWithSignBytes(msg, privs, seqs, signBytes) } @@ -65,7 +65,9 @@ func newTestTxWithSignBytes(msg sdk.Msg, privs []crypto.PrivKey, seqs []int64, s for i, priv := range privs { sigs[i] = sdk.StdSignature{PubKey: priv.PubKey(), Signature: priv.Sign(signBytes), Sequence: seqs[i]} } - return sdk.NewStdTx(msg, sigs) + tx := sdk.NewStdTx(msg, sigs) + tx.SetFee(sdk.StdFee{Gas: 0, Amount: sdk.Coins{sdk.Coin{Amount: feeAmount, Denom: "atom"}}}) + return tx } // Test various error cases in the AnteHandler control flow. @@ -85,15 +87,15 @@ func TestAnteHandlerSigErrors(t *testing.T) { msg := newTestMsg(addr1, addr2) // test no signatures - tx = newTestTx(ctx, msg, []crypto.PrivKey{}, []int64{}) + tx = newTestTx(ctx, msg, []crypto.PrivKey{}, []int64{}, int64(0)) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnauthorized) // test num sigs dont match GetSigners - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}, int64(0)) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnauthorized) // test an unrecognized account - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{0, 0}) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{0, 0}, int64(0)) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnrecognizedAddress) // save the first account, but second is still unrecognized @@ -123,7 +125,7 @@ func TestAnteHandlerSequences(t *testing.T) { // msg and signatures var tx sdk.Tx msg := newTestMsg(addr1) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}, int64(0)) // test good tx from one signer checkValidTx(t, anteHandler, ctx, tx) @@ -132,12 +134,12 @@ func TestAnteHandlerSequences(t *testing.T) { checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInvalidSequence) // fix sequence, should pass - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{1}) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{1}, int64(0)) checkValidTx(t, anteHandler, ctx, tx) // new tx with another signer and correct sequences msg = newTestMsg(addr1, addr2) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{2, 0}) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{2, 0}, int64(0)) checkValidTx(t, anteHandler, ctx, tx) // replay fails @@ -145,19 +147,45 @@ func TestAnteHandlerSequences(t *testing.T) { // tx from just second signer with incorrect sequence fails msg = newTestMsg(addr2) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv2}, []int64{0}) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv2}, []int64{0}, int64(0)) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInvalidSequence) // fix the sequence and it passes - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv2}, []int64{1}) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv2}, []int64{1}, int64(0)) checkValidTx(t, anteHandler, ctx, tx) // another tx from both of them that passes msg = newTestMsg(addr1, addr2) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{3, 2}) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{3, 2}, int64(0)) checkValidTx(t, anteHandler, ctx, tx) } +// Test logic around fee deduction. +func TestAnteHandlerFees(t *testing.T) { + // // setup + // ms, capKey := setupMultiStore() + // mapper := NewAccountMapper(capKey, &BaseAccount{}) + // anteHandler := NewAnteHandler(mapper) + // ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil) + // + // // keys and addresses + // priv1, addr1 := privAndAddr() + // priv2, addr2 := privAndAddr() + // + // // set the accounts + // acc1 := mapper.NewAccountWithAddress(ctx, addr1) + // mapper.SetAccount(ctx, acc1) + // acc2 := mapper.NewAccountWithAddress(ctx, addr2) + // mapper.SetAccount(ctx, acc2) + // + // // msg and signatures + // var tx sdk.Tx + // msg := newTestMsg(addr1) + // tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}, int64(1)) + + // TODO +} + func TestAnteHandlerBadSignBytes(t *testing.T) { // setup ms, capKey := setupMultiStore() From 3babf8c2d96f5a49d9864f2c6e3d0436f10368c1 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sat, 17 Mar 2018 19:54:18 +0100 Subject: [PATCH 06/10] fix and clean fees and x/auth --- client/builder/builder.go | 2 +- examples/basecoin/app/app_test.go | 21 +++-- examples/basecoin/x/cool/types.go | 4 +- types/errors.go | 12 +-- types/tx_msg.go | 61 ++++++++------ x/auth/ante.go | 87 ++++++++++--------- x/auth/ante_test.go | 134 +++++++++++++++++++++--------- x/bank/mapper.go | 2 +- 8 files changed, 204 insertions(+), 119 deletions(-) diff --git a/client/builder/builder.go b/client/builder/builder.go index e252f40af8..a64dfeda74 100644 --- a/client/builder/builder.go +++ b/client/builder/builder.go @@ -124,7 +124,7 @@ func SignAndBuild(msg sdk.Msg, cdc *wire.Codec) ([]byte, error) { }} // marshal bytes - tx := sdk.NewStdTx(signMsg.Msg, sigs) + tx := sdk.NewStdTx(signMsg.Msg, signMsg.Fee, sigs) return cdc.MarshalBinary(tx) } diff --git a/examples/basecoin/app/app_test.go b/examples/basecoin/app/app_test.go index c2ef3e4547..4c578579ba 100644 --- a/examples/basecoin/app/app_test.go +++ b/examples/basecoin/app/app_test.go @@ -29,6 +29,10 @@ var ( addr1 = priv1.PubKey().Address() addr2 = crypto.GenPrivKeyEd25519().PubKey().Address() coins = sdk.Coins{{"foocoin", 10}} + fee = sdk.StdFee{ + sdk.Coins{{"foocoin", 0}}, + 0, + } sendMsg = bank.SendMsg{ Inputs: []bank.Input{bank.NewInput(addr1, coins)}, @@ -82,8 +86,8 @@ func TestMsgs(t *testing.T) { sequences := []int64{0} for i, m := range msgs { - sig := priv1.Sign(sdk.StdSignBytes(chainID, sequences, m.msg)) - tx := sdk.NewStdTx(m.msg, []sdk.StdSignature{{ + sig := priv1.Sign(sdk.StdSignBytes(chainID, sequences, fee, m.msg)) + tx := sdk.NewStdTx(m.msg, fee, []sdk.StdSignature{{ PubKey: priv1.PubKey(), Signature: sig, }}) @@ -180,8 +184,8 @@ func TestSendMsgWithAccounts(t *testing.T) { // Sign the tx sequences := []int64{0} - sig := priv1.Sign(sdk.StdSignBytes(chainID, sequences, sendMsg)) - tx := sdk.NewStdTx(sendMsg, []sdk.StdSignature{{ + sig := priv1.Sign(sdk.StdSignBytes(chainID, sequences, fee, sendMsg)) + tx := sdk.NewStdTx(sendMsg, fee, []sdk.StdSignature{{ PubKey: priv1.PubKey(), Signature: sig, }}) @@ -213,7 +217,7 @@ func TestSendMsgWithAccounts(t *testing.T) { // resigning the tx with the bumped sequence should work sequences = []int64{1} - sig = priv1.Sign(sdk.StdSignBytes(chainID, sequences, tx.Msg)) + sig = priv1.Sign(sdk.StdSignBytes(chainID, sequences, fee, tx.Msg)) tx.Signatures[0].Signature = sig res = bapp.Deliver(tx) assert.Equal(t, sdk.CodeOK, res.Code, res.Log) @@ -269,10 +273,13 @@ func TestQuizMsg(t *testing.T) { func SignCheckDeliver(t *testing.T, bapp *BasecoinApp, msg sdk.Msg, seq int64, expPass bool) { + // TODO: + var fee sdk.StdFee + // Sign the tx - tx := sdk.NewStdTx(msg, []sdk.StdSignature{{ + tx := sdk.NewStdTx(msg, fee, []sdk.StdSignature{{ PubKey: priv1.PubKey(), - Signature: priv1.Sign(sdk.StdSignBytes(chainID, []int64{seq}, msg)), + Signature: priv1.Sign(sdk.StdSignBytes(chainID, []int64{seq}, fee, msg)), Sequence: seq, }}) diff --git a/examples/basecoin/x/cool/types.go b/examples/basecoin/x/cool/types.go index f721bfa19b..10515c8abe 100644 --- a/examples/basecoin/x/cool/types.go +++ b/examples/basecoin/x/cool/types.go @@ -37,7 +37,7 @@ func (msg SetTrendMsg) String() string { // Validate Basic is used to quickly disqualify obviously invalid messages quickly func (msg SetTrendMsg) ValidateBasic() sdk.Error { if len(msg.Sender) == 0 { - return sdk.ErrUnrecognizedAddress(msg.Sender).Trace("") + return sdk.ErrUnrecognizedAddress(msg.Sender.String()).Trace("") } if strings.Contains(msg.Cool, "hot") { return sdk.ErrUnauthorized("").Trace("hot is not cool") @@ -88,7 +88,7 @@ func (msg QuizMsg) String() string { // Validate Basic is used to quickly disqualify obviously invalid messages quickly func (msg QuizMsg) ValidateBasic() sdk.Error { if len(msg.Sender) == 0 { - return sdk.ErrUnrecognizedAddress(msg.Sender).Trace("") + return sdk.ErrUnrecognizedAddress(msg.Sender.String()).Trace("") } return nil } diff --git a/types/errors.go b/types/errors.go index 008bd6f086..5c96d8c241 100644 --- a/types/errors.go +++ b/types/errors.go @@ -27,7 +27,7 @@ const ( CodeInsufficientFunds CodeType = 5 CodeUnknownRequest CodeType = 6 CodeUnrecognizedAddress CodeType = 7 - CodeMissingPubKey CodeType = 8 + CodeInvalidPubKey CodeType = 8 CodeGenesisParse CodeType = 0xdead // TODO: remove ? ) @@ -51,7 +51,7 @@ func CodeToDefaultMsg(code CodeType) string { return "Unknown request" case CodeUnrecognizedAddress: return "Unrecognized address" - case CodeMissingPubKey: + case CodeInvalidPubKey: return "Missing pubkey" default: return fmt.Sprintf("Unknown code %d", code) @@ -84,11 +84,11 @@ func ErrInsufficientFunds(msg string) Error { func ErrUnknownRequest(msg string) Error { return newError(CodeUnknownRequest, msg) } -func ErrUnrecognizedAddress(addr Address) Error { - return newError(CodeUnrecognizedAddress, addr.String()) +func ErrUnrecognizedAddress(msg string) Error { + return newError(CodeUnrecognizedAddress, msg) } -func ErrMissingPubKey(addr Address) Error { - return newError(CodeMissingPubKey, addr.String()) +func ErrInvalidPubKey(msg string) Error { + return newError(CodeInvalidPubKey, msg) } //---------------------------------------- diff --git a/types/tx_msg.go b/types/tx_msg.go index 81719f18a7..b41d9879a9 100644 --- a/types/tx_msg.go +++ b/types/tx_msg.go @@ -45,7 +45,7 @@ type Tx interface { var _ Tx = (*StdTx)(nil) -// StdTx is a standard way to wrap a Msg with Signatures. +// StdTx is a standard way to wrap a Msg with Fee and Signatures. // NOTE: the first signature is the FeePayer (Signatures must not be nil). type StdTx struct { Msg `json:"msg"` @@ -53,19 +53,14 @@ type StdTx struct { Signatures []StdSignature `json:"signatures"` } -func NewStdTx(msg Msg, sigs []StdSignature) StdTx { +func NewStdTx(msg Msg, fee StdFee, sigs []StdSignature) StdTx { return StdTx{ Msg: msg, + Fee: fee, Signatures: sigs, } } -// SetFee sets the StdFee on the transaction. -func (tx StdTx) SetFee(fee StdFee) StdTx { - tx.Fee = fee - return tx -} - //nolint func (tx StdTx) GetMsg() Msg { return tx.Msg } func (tx StdTx) GetSignatures() []StdSignature { return tx.Signatures } @@ -77,6 +72,8 @@ func FeePayer(tx Tx) Address { return tx.GetMsg().GetSigners()[0] } +//__________________________________________________________ + // StdFee includes the amount of coins paid in fees and the maximum // gas to be used by the transaction. The ratio yields an effective "gasprice", // which must be above some miminum to be accepted into the mempool. @@ -92,6 +89,16 @@ func NewStdFee(gas int64, amount ...Coin) StdFee { } } +func (fee StdFee) Bytes() []byte { + bz, err := json.Marshal(fee) // TODO + if err != nil { + panic(err) + } + return bz +} + +//__________________________________________________________ + // StdSignDoc is replay-prevention structure. // It includes the result of msg.GetSignBytes(), // as well as the ChainID (prevent cross chain replay) @@ -100,27 +107,18 @@ func NewStdFee(gas int64, amount ...Coin) StdFee { type StdSignDoc struct { ChainID string `json:"chain_id"` Sequences []int64 `json:"sequences"` + FeeBytes []byte `json:"fee_bytes"` MsgBytes []byte `json:"msg_bytes"` - AltBytes []byte `json:"alt_bytes"` // TODO: do we really want this ? + AltBytes []byte `json:"alt_bytes"` } -// StdSignMsg is a convenience structure for passing along -// a Msg with the other requirements for a StdSignDoc before -// it is signed. For use in the CLI -type StdSignMsg struct { - ChainID string - Sequences []int64 - Msg Msg -} - -func (msg StdSignMsg) Bytes() []byte { - return StdSignBytes(msg.ChainID, msg.Sequences, msg.Msg) -} - -func StdSignBytes(chainID string, sequences []int64, msg Msg) []byte { +// StdSignBytes returns the bytes to sign for a transaction. +// TODO: change the API to just take a chainID and StdTx ? +func StdSignBytes(chainID string, sequences []int64, fee StdFee, msg Msg) []byte { bz, err := json.Marshal(StdSignDoc{ ChainID: chainID, Sequences: sequences, + FeeBytes: fee.Bytes(), MsgBytes: msg.GetSignBytes(), }) if err != nil { @@ -129,7 +127,22 @@ func StdSignBytes(chainID string, sequences []int64, msg Msg) []byte { return bz } -//------------------------------------- +// StdSignMsg is a convenience structure for passing along +// a Msg with the other requirements for a StdSignDoc before +// it is signed. For use in the CLI. +type StdSignMsg struct { + ChainID string + Sequences []int64 + Fee StdFee + Msg Msg + // XXX: Alt +} + +func (msg StdSignMsg) Bytes() []byte { + return StdSignBytes(msg.ChainID, msg.Sequences, msg.Fee, msg.Msg) +} + +//__________________________________________________________ // Application function variable used to unmarshal transaction bytes type TxDecoder func(txBytes []byte) (Tx, Error) diff --git a/x/auth/ante.go b/x/auth/ante.go index 724b7c5ef6..4305929b97 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -1,8 +1,8 @@ package auth import ( + "bytes" "fmt" - "reflect" sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -40,44 +40,61 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { true } - // Get the sign bytes (requires all sequence numbers) + // Get the sign bytes (requires all sequence numbers and the fee) sequences := make([]int64, len(signerAddrs)) for i := 0; i < len(signerAddrs); i++ { sequences[i] = sigs[i].Sequence } - signBytes := sdk.StdSignBytes(ctx.ChainID(), sequences, msg) + fee := stdTx.Fee + signBytes := sdk.StdSignBytes(ctx.ChainID(), sequences, fee, msg) // Check sig and nonce and collect signer accounts. var signerAccs = make([]sdk.Account, len(signerAddrs)) for i := 0; i < len(sigs); i++ { - isFeePayer := i == 0 // first sig pays the fees - signerAddr, sig := signerAddrs[i], sigs[i] - signerAcc, res := processSig(ctx, accountMapper, signerAddr, sig, - signBytes, isFeePayer, stdTx.Fee.Amount) + + // check signature, return account with incremented nonce + signerAcc, res := processSig( + ctx, accountMapper, + signerAddr, sig, signBytes, + ) if !res.IsOK() { return ctx, res, true } + + // first sig pays the fees + if i == 0 { + signerAcc, res = deductFees(signerAcc, fee) + if !res.IsOK() { + return ctx, res, true + } + } + + // Save the account. + accountMapper.SetAccount(ctx, signerAcc) signerAccs[i] = signerAcc } + // cache the signer accounts in the context ctx = WithSigners(ctx, signerAccs) + // TODO: tx tags (?) + return ctx, sdk.Result{}, false // continue... } } // verify the signature and increment the sequence. // if the account doesn't have a pubkey, set it. -// deduct fee from fee payer. -func processSig(ctx sdk.Context, am sdk.AccountMapper, - addr sdk.Address, sig sdk.StdSignature, signBytes []byte, - isFeePayer bool, feeAmount sdk.Coins) (acc sdk.Account, res sdk.Result) { +func processSig( + ctx sdk.Context, am sdk.AccountMapper, + addr sdk.Address, sig sdk.StdSignature, signBytes []byte) ( + acc sdk.Account, res sdk.Result) { - // Get the account + // Get the account. acc = am.GetAccount(ctx, addr) if acc == nil { - return nil, sdk.ErrUnrecognizedAddress(addr).Result() + return nil, sdk.ErrUnrecognizedAddress(addr.String()).Result() } // Check and increment sequence number. @@ -89,23 +106,20 @@ func processSig(ctx sdk.Context, am sdk.AccountMapper, acc.SetSequence(seq + 1) // If pubkey is not known for account, - // set it from the StdSignature + // set it from the StdSignature. pubKey := acc.GetPubKey() if pubKey.Empty() { - if sig.PubKey.Empty() { - return nil, sdk.ErrInternal("public Key not found").Result() - } - if !reflect.DeepEqual(sig.PubKey.Address(), addr) { - return nil, sdk.ErrInternal( - fmt.Sprintf("invalid PubKey for address %v", addr)).Result() - } pubKey = sig.PubKey if pubKey.Empty() { - return nil, sdk.ErrMissingPubKey(addr).Result() + return nil, sdk.ErrInvalidPubKey("PubKey not found").Result() + } + if !bytes.Equal(pubKey.Address(), addr) { + return nil, sdk.ErrInvalidPubKey( + fmt.Sprintf("PubKey does not match Signer address %v", addr)).Result() } err := acc.SetPubKey(pubKey) if err != nil { - return nil, sdk.ErrInternal("setting PubKey on signer").Result() + return nil, sdk.ErrInternal("setting PubKey on signer's account").Result() } } // Check sig. @@ -113,19 +127,18 @@ func processSig(ctx sdk.Context, am sdk.AccountMapper, return nil, sdk.ErrUnauthorized("signature verification failed").Result() } - // If this is the fee payer, deduct the fee. - if isFeePayer { - coins := acc.GetCoins() - newCoins := coins.Minus(feeAmount) - if !newCoins.IsNotNegative() { - errMsg := fmt.Sprintf("%s < %s", coins, feeAmount) - return nil, sdk.ErrInsufficientFunds(errMsg).Result() - } - - acc.SetCoins(newCoins) - } - - // Save the account. - am.SetAccount(ctx, acc) return } + +// deduct the fee from the account +func deductFees(acc sdk.Account, fee sdk.StdFee) (sdk.Account, sdk.Result) { + coins := acc.GetCoins() + feeAmount := fee.Amount + newCoins := coins.Minus(feeAmount) + if !newCoins.IsNotNegative() { + errMsg := fmt.Sprintf("%s < %s", coins, feeAmount) + return nil, sdk.ErrInsufficientFunds(errMsg).Result() + } + acc.SetCoins(newCoins) + return acc, sdk.Result{} +} diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go index 9523cdb39e..cecc4a1418 100644 --- a/x/auth/ante_test.go +++ b/x/auth/ante_test.go @@ -1,38 +1,54 @@ package auth import ( - "reflect" + "encoding/json" "testing" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" abci "github.com/tendermint/abci/types" crypto "github.com/tendermint/go-crypto" ) // msg type for testing type testMsg struct { - signBytes []byte - signers []sdk.Address + signers []sdk.Address } func newTestMsg(addrs ...sdk.Address) *testMsg { return &testMsg{ - signBytes: []byte(addrs[0]), - signers: addrs, + signers: addrs, } } func (msg *testMsg) Type() string { return "testMsg" } func (msg *testMsg) Get(key interface{}) (value interface{}) { return nil } func (msg *testMsg) GetSignBytes() []byte { - return msg.signBytes + bz, err := json.Marshal(msg.signers) + if err != nil { + panic(err) + } + return bz } func (msg *testMsg) ValidateBasic() sdk.Error { return nil } func (msg *testMsg) GetSigners() []sdk.Address { return msg.signers } +func newStdFee() sdk.StdFee { + return sdk.NewStdFee(100, + sdk.Coin{"atom", 150}, + ) +} + +// coins to more than cover the fee +func newCoins() sdk.Coins { + return sdk.Coins{ + {"atom", 10000000}, + } +} + // generate a priv key and return it with its address func privAndAddr() (crypto.PrivKey, sdk.Address) { priv := crypto.GenPrivKeyEd25519() @@ -55,18 +71,17 @@ func checkInvalidTx(t *testing.T, anteHandler sdk.AnteHandler, ctx sdk.Context, assert.Equal(t, code, result.Code) } -func newTestTx(ctx sdk.Context, msg sdk.Msg, privs []crypto.PrivKey, seqs []int64, feeAmount int64) sdk.Tx { - signBytes := sdk.StdSignBytes(ctx.ChainID(), seqs, msg) - return newTestTxWithSignBytes(msg, privs, seqs, signBytes) +func newTestTx(ctx sdk.Context, msg sdk.Msg, privs []crypto.PrivKey, seqs []int64, fee sdk.StdFee) sdk.Tx { + signBytes := sdk.StdSignBytes(ctx.ChainID(), seqs, fee, msg) + return newTestTxWithSignBytes(msg, privs, seqs, fee, signBytes) } -func newTestTxWithSignBytes(msg sdk.Msg, privs []crypto.PrivKey, seqs []int64, signBytes []byte) sdk.Tx { +func newTestTxWithSignBytes(msg sdk.Msg, privs []crypto.PrivKey, seqs []int64, fee sdk.StdFee, signBytes []byte) sdk.Tx { sigs := make([]sdk.StdSignature, len(privs)) for i, priv := range privs { sigs[i] = sdk.StdSignature{PubKey: priv.PubKey(), Signature: priv.Sign(signBytes), Sequence: seqs[i]} } - tx := sdk.NewStdTx(msg, sigs) - tx.SetFee(sdk.StdFee{Gas: 0, Amount: sdk.Coins{sdk.Coin{Amount: feeAmount, Denom: "atom"}}}) + tx := sdk.NewStdTx(msg, fee, sigs) return tx } @@ -85,21 +100,26 @@ func TestAnteHandlerSigErrors(t *testing.T) { // msg and signatures var tx sdk.Tx msg := newTestMsg(addr1, addr2) + fee := newStdFee() // test no signatures - tx = newTestTx(ctx, msg, []crypto.PrivKey{}, []int64{}, int64(0)) + privs, seqs := []crypto.PrivKey{}, []int64{} + tx = newTestTx(ctx, msg, privs, seqs, fee) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnauthorized) // test num sigs dont match GetSigners - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}, int64(0)) + privs, seqs = []crypto.PrivKey{priv1}, []int64{0} + tx = newTestTx(ctx, msg, privs, seqs, fee) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnauthorized) // test an unrecognized account - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{0, 0}, int64(0)) + privs, seqs = []crypto.PrivKey{priv1, priv2}, []int64{0, 0} + tx = newTestTx(ctx, msg, privs, seqs, fee) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnrecognizedAddress) // save the first account, but second is still unrecognized acc1 := mapper.NewAccountWithAddress(ctx, addr1) + acc1.SetCoins(fee.Amount) mapper.SetAccount(ctx, acc1) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnrecognizedAddress) } @@ -118,28 +138,34 @@ func TestAnteHandlerSequences(t *testing.T) { // set the accounts acc1 := mapper.NewAccountWithAddress(ctx, addr1) + acc1.SetCoins(newCoins()) mapper.SetAccount(ctx, acc1) acc2 := mapper.NewAccountWithAddress(ctx, addr2) + acc2.SetCoins(newCoins()) mapper.SetAccount(ctx, acc2) // msg and signatures var tx sdk.Tx msg := newTestMsg(addr1) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}, int64(0)) + fee := newStdFee() // test good tx from one signer + privs, seqs := []crypto.PrivKey{priv1}, []int64{0} + tx = newTestTx(ctx, msg, privs, seqs, fee) checkValidTx(t, anteHandler, ctx, tx) // test sending it again fails (replay protection) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInvalidSequence) // fix sequence, should pass - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{1}, int64(0)) + seqs = []int64{1} + tx = newTestTx(ctx, msg, privs, seqs, fee) checkValidTx(t, anteHandler, ctx, tx) // new tx with another signer and correct sequences msg = newTestMsg(addr1, addr2) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{2, 0}, int64(0)) + privs, seqs = []crypto.PrivKey{priv1, priv2}, []int64{2, 0} + tx = newTestTx(ctx, msg, privs, seqs, fee) checkValidTx(t, anteHandler, ctx, tx) // replay fails @@ -147,16 +173,18 @@ func TestAnteHandlerSequences(t *testing.T) { // tx from just second signer with incorrect sequence fails msg = newTestMsg(addr2) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv2}, []int64{0}, int64(0)) + privs, seqs = []crypto.PrivKey{priv2}, []int64{0} + tx = newTestTx(ctx, msg, privs, seqs, fee) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInvalidSequence) // fix the sequence and it passes - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv2}, []int64{1}, int64(0)) + tx = newTestTx(ctx, msg, []crypto.PrivKey{priv2}, []int64{1}, fee) checkValidTx(t, anteHandler, ctx, tx) // another tx from both of them that passes msg = newTestMsg(addr1, addr2) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1, priv2}, []int64{3, 2}, int64(0)) + privs, seqs = []crypto.PrivKey{priv1, priv2}, []int64{3, 2} + tx = newTestTx(ctx, msg, privs, seqs, fee) checkValidTx(t, anteHandler, ctx, tx) } @@ -199,35 +227,55 @@ func TestAnteHandlerBadSignBytes(t *testing.T) { // set the accounts acc1 := mapper.NewAccountWithAddress(ctx, addr1) + acc1.SetCoins(newCoins()) mapper.SetAccount(ctx, acc1) acc2 := mapper.NewAccountWithAddress(ctx, addr2) + acc2.SetCoins(newCoins()) mapper.SetAccount(ctx, acc2) var tx sdk.Tx + msg := newTestMsg(addr1) + fee := newStdFee() // test good tx and signBytes - msg := newTestMsg(addr1) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}) + privs, seqs := []crypto.PrivKey{priv1}, []int64{0} + tx = newTestTx(ctx, msg, privs, seqs, fee) checkValidTx(t, anteHandler, ctx, tx) - // test invalid chain_id - tx = newTestTxWithSignBytes(msg, []crypto.PrivKey{priv1}, []int64{1}, sdk.StdSignBytes("", []int64{1}, msg)) - checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnauthorized) - // test wrong seqs - tx = newTestTxWithSignBytes(msg, []crypto.PrivKey{priv1}, []int64{1}, sdk.StdSignBytes(ctx.ChainID(), []int64{2}, msg)) - checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnauthorized) - // test wrong msg - tx = newTestTxWithSignBytes(msg, []crypto.PrivKey{priv1}, []int64{1}, sdk.StdSignBytes(ctx.ChainID(), []int64{1}, newTestMsg(addr2))) - checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnauthorized) + chainID := ctx.ChainID() + codeUnauth := sdk.CodeUnauthorized + + cases := []struct { + chainID string + seqs []int64 + fee sdk.StdFee + msg sdk.Msg + code sdk.CodeType + }{ + {"", []int64{1}, fee, msg, codeUnauth}, // test invalid chain_id + {chainID, []int64{2}, fee, msg, codeUnauth}, // test wrong seqs + {chainID, []int64{1}, fee, newTestMsg(addr2), codeUnauth}, // test wrong msg + } + + privs, seqs = []crypto.PrivKey{priv1}, []int64{1} + for _, cs := range cases { + tx := newTestTxWithSignBytes( + msg, privs, seqs, fee, + sdk.StdSignBytes(cs.chainID, cs.seqs, cs.fee, cs.msg), + ) + checkInvalidTx(t, anteHandler, ctx, tx, cs.code) + } // test wrong signer if public key exist - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv2}, []int64{1}) + privs, seqs = []crypto.PrivKey{priv2}, []int64{1} + tx = newTestTx(ctx, msg, privs, seqs, fee) checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeUnauthorized) // test wrong signer if public doesn't exist msg = newTestMsg(addr2) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}) - checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInternal) + privs, seqs = []crypto.PrivKey{priv1}, []int64{0} + tx = newTestTx(ctx, msg, privs, seqs, fee) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInvalidPubKey) } @@ -244,33 +292,37 @@ func TestAnteHandlerSetPubKey(t *testing.T) { // set the accounts acc1 := mapper.NewAccountWithAddress(ctx, addr1) + acc1.SetCoins(newCoins()) mapper.SetAccount(ctx, acc1) acc2 := mapper.NewAccountWithAddress(ctx, addr2) + acc2.SetCoins(newCoins()) mapper.SetAccount(ctx, acc2) var tx sdk.Tx // test good tx and set public key msg := newTestMsg(addr1) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}) + privs, seqs := []crypto.PrivKey{priv1}, []int64{0} + fee := newStdFee() + tx = newTestTx(ctx, msg, privs, seqs, fee) checkValidTx(t, anteHandler, ctx, tx) acc1 = mapper.GetAccount(ctx, addr1) - reflect.DeepEqual(acc1.GetPubKey(), priv1.PubKey()) + require.Equal(t, acc1.GetPubKey(), priv1.PubKey()) // test public key not found msg = newTestMsg(addr2) - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}) + tx = newTestTx(ctx, msg, privs, seqs, fee) sigs := tx.GetSignatures() sigs[0].PubKey = crypto.PubKey{} - checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInternal) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInvalidPubKey) acc2 = mapper.GetAccount(ctx, addr2) assert.True(t, acc2.GetPubKey().Empty()) // test invalid signature and public key - tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}) - checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInternal) + tx = newTestTx(ctx, msg, privs, seqs, fee) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInvalidPubKey) acc2 = mapper.GetAccount(ctx, addr2) assert.True(t, acc2.GetPubKey().Empty()) diff --git a/x/bank/mapper.go b/x/bank/mapper.go index 76e7f4e2f5..ab2e854202 100644 --- a/x/bank/mapper.go +++ b/x/bank/mapper.go @@ -20,7 +20,7 @@ func NewCoinKeeper(am sdk.AccountMapper) CoinKeeper { func (ck CoinKeeper) SubtractCoins(ctx sdk.Context, addr sdk.Address, amt sdk.Coins) (sdk.Coins, sdk.Error) { acc := ck.am.GetAccount(ctx, addr) if acc == nil { - return amt, sdk.ErrUnrecognizedAddress(addr) + return amt, sdk.ErrUnrecognizedAddress(addr.String()) } coins := acc.GetCoins() From dd4a86b856b30c1477398983f767e765fbed2a72 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sat, 17 Mar 2018 21:20:24 +0100 Subject: [PATCH 07/10] x/auth: crank the test coverage --- examples/basecoin/app/app_test.go | 3 - types/coin.go | 8 ++- x/auth/ante.go | 10 +++- x/auth/ante_test.go | 52 +++++++++------- x/auth/baseaccount_test.go | 98 +++++++++++++++++++++++++------ x/auth/context.go | 6 +- 6 files changed, 131 insertions(+), 46 deletions(-) diff --git a/examples/basecoin/app/app_test.go b/examples/basecoin/app/app_test.go index 4c578579ba..66de2b68b4 100644 --- a/examples/basecoin/app/app_test.go +++ b/examples/basecoin/app/app_test.go @@ -273,9 +273,6 @@ func TestQuizMsg(t *testing.T) { func SignCheckDeliver(t *testing.T, bapp *BasecoinApp, msg sdk.Msg, seq int64, expPass bool) { - // TODO: - var fee sdk.StdFee - // Sign the tx tx := sdk.NewStdTx(msg, fee, []sdk.StdSignature{{ PubKey: priv1.PubKey(), diff --git a/types/coin.go b/types/coin.go index ad541b99bd..92871cd179 100644 --- a/types/coin.go +++ b/types/coin.go @@ -139,8 +139,14 @@ func (coins Coins) IsGTE(coinsB Coins) bool { } // IsZero returns true if there are no coins +// or all coins are zero. func (coins Coins) IsZero() bool { - return len(coins) == 0 + for _, coin := range coins { + if !coin.IsZero() { + return false + } + } + return true } // IsEqual returns true if the two sets of Coins have the same value diff --git a/x/auth/ante.go b/x/auth/ante.go index 4305929b97..2da0d92868 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -64,9 +64,12 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler { // first sig pays the fees if i == 0 { - signerAcc, res = deductFees(signerAcc, fee) - if !res.IsOK() { - return ctx, res, true + // TODO: min fee + if !fee.Amount.IsZero() { + signerAcc, res = deductFees(signerAcc, fee) + if !res.IsOK() { + return ctx, res, true + } } } @@ -134,6 +137,7 @@ func processSig( func deductFees(acc sdk.Account, fee sdk.StdFee) (sdk.Account, sdk.Result) { coins := acc.GetCoins() feeAmount := fee.Amount + newCoins := coins.Minus(feeAmount) if !newCoins.IsNotNegative() { errMsg := fmt.Sprintf("%s < %s", coins, feeAmount) diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go index cecc4a1418..ecd2226230 100644 --- a/x/auth/ante_test.go +++ b/x/auth/ante_test.go @@ -190,28 +190,38 @@ func TestAnteHandlerSequences(t *testing.T) { // Test logic around fee deduction. func TestAnteHandlerFees(t *testing.T) { - // // setup - // ms, capKey := setupMultiStore() - // mapper := NewAccountMapper(capKey, &BaseAccount{}) - // anteHandler := NewAnteHandler(mapper) - // ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil) - // - // // keys and addresses - // priv1, addr1 := privAndAddr() - // priv2, addr2 := privAndAddr() - // - // // set the accounts - // acc1 := mapper.NewAccountWithAddress(ctx, addr1) - // mapper.SetAccount(ctx, acc1) - // acc2 := mapper.NewAccountWithAddress(ctx, addr2) - // mapper.SetAccount(ctx, acc2) - // - // // msg and signatures - // var tx sdk.Tx - // msg := newTestMsg(addr1) - // tx = newTestTx(ctx, msg, []crypto.PrivKey{priv1}, []int64{0}, int64(1)) + // setup + ms, capKey := setupMultiStore() + mapper := NewAccountMapper(capKey, &BaseAccount{}) + anteHandler := NewAnteHandler(mapper) + ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil) - // TODO + // keys and addresses + priv1, addr1 := privAndAddr() + + // set the accounts + acc1 := mapper.NewAccountWithAddress(ctx, addr1) + mapper.SetAccount(ctx, acc1) + + // msg and signatures + var tx sdk.Tx + msg := newTestMsg(addr1) + privs, seqs := []crypto.PrivKey{priv1}, []int64{0} + fee := sdk.NewStdFee(100, + sdk.Coin{"atom", 150}, + ) + + // signer does not have enough funds to pay the fee + tx = newTestTx(ctx, msg, privs, seqs, fee) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInsufficientFunds) + + acc1.SetCoins(sdk.Coins{{"atom", 149}}) + mapper.SetAccount(ctx, acc1) + checkInvalidTx(t, anteHandler, ctx, tx, sdk.CodeInsufficientFunds) + + acc1.SetCoins(sdk.Coins{{"atom", 150}}) + mapper.SetAccount(ctx, acc1) + checkValidTx(t, anteHandler, ctx, tx) } func TestAnteHandlerBadSignBytes(t *testing.T) { diff --git a/x/auth/baseaccount_test.go b/x/auth/baseaccount_test.go index 85674af18b..85adc8cdca 100644 --- a/x/auth/baseaccount_test.go +++ b/x/auth/baseaccount_test.go @@ -11,42 +11,106 @@ import ( wire "github.com/cosmos/cosmos-sdk/wire" ) -func TestBaseAccount(t *testing.T) { +func keyPubAddr() (crypto.PrivKey, crypto.PubKey, sdk.Address) { key := crypto.GenPrivKeyEd25519() pub := key.PubKey() addr := pub.Address() + return key.Wrap(), pub, addr +} + +func TestBaseAccountAddressPubKey(t *testing.T) { + _, pub1, addr1 := keyPubAddr() + _, pub2, addr2 := keyPubAddr() + acc := NewBaseAccountWithAddress(addr1) + + // check the address (set) and pubkey (not set) + assert.EqualValues(t, addr1, acc.GetAddress()) + assert.EqualValues(t, crypto.PubKey{}, acc.GetPubKey()) + + // cant override address + err := acc.SetAddress(addr2) + assert.NotNil(t, err) + assert.EqualValues(t, addr1, acc.GetAddress()) + + // set the pubkey + err = acc.SetPubKey(pub1) + assert.Nil(t, err) + assert.Equal(t, pub1, acc.GetPubKey()) + + // cant override pubkey + err = acc.SetPubKey(pub2) + assert.NotNil(t, err) + assert.Equal(t, pub1, acc.GetPubKey()) + + //------------------------------------ + + // can set address on empty account + acc2 := BaseAccount{} + err = acc2.SetAddress(addr2) + assert.Nil(t, err) + assert.EqualValues(t, addr2, acc2.GetAddress()) +} + +func TestBaseAccountCoins(t *testing.T) { + _, _, addr := keyPubAddr() + acc := NewBaseAccountWithAddress(addr) + + someCoins := sdk.Coins{{"atom", 123}, {"eth", 246}} + + err := acc.SetCoins(someCoins) + assert.Nil(t, err) + assert.Equal(t, someCoins, acc.GetCoins()) +} + +func TestBaseAccountSequence(t *testing.T) { + _, _, addr := keyPubAddr() + acc := NewBaseAccountWithAddress(addr) + + seq := int64(7) + + err := acc.SetSequence(seq) + assert.Nil(t, err) + assert.Equal(t, seq, acc.GetSequence()) +} + +func TestBaseAccountMarshal(t *testing.T) { + _, pub, addr := keyPubAddr() + acc := NewBaseAccountWithAddress(addr) + someCoins := sdk.Coins{{"atom", 123}, {"eth", 246}} seq := int64(7) - acc := NewBaseAccountWithAddress(addr) + // set everything on the account + err := acc.SetPubKey(pub) + assert.Nil(t, err) + err = acc.SetSequence(seq) + assert.Nil(t, err) + err = acc.SetCoins(someCoins) + assert.Nil(t, err) // need a codec for marshaling codec := wire.NewCodec() wire.RegisterCrypto(codec) - err := acc.SetPubKey(pub) - assert.Nil(t, err) - assert.Equal(t, pub, acc.GetPubKey()) - - assert.EqualValues(t, addr, acc.GetAddress()) - - err = acc.SetCoins(someCoins) - assert.Nil(t, err) - assert.Equal(t, someCoins, acc.GetCoins()) - - err = acc.SetSequence(seq) - assert.Nil(t, err) - assert.Equal(t, seq, acc.GetSequence()) - b, err := codec.MarshalBinary(acc) assert.Nil(t, err) - var acc2 BaseAccount + acc2 := BaseAccount{} err = codec.UnmarshalBinary(b, &acc2) assert.Nil(t, err) assert.Equal(t, acc, acc2) + // error on bad bytes acc2 = BaseAccount{} err = codec.UnmarshalBinary(b[:len(b)/2], &acc2) assert.NotNil(t, err) + +} + +func TestBaseAccountGetSet(t *testing.T) { + _, _, addr := keyPubAddr() + acc := NewBaseAccountWithAddress(addr) + + assert.Panics(t, func() { acc.Get("key") }) + assert.Panics(t, func() { acc.Set("key", "value") }) } diff --git a/x/auth/context.go b/x/auth/context.go index 90de99f783..91259d9e6d 100644 --- a/x/auth/context.go +++ b/x/auth/context.go @@ -38,5 +38,9 @@ func WithSigners(ctx types.Context, accounts []types.Account) types.Context { } func GetSigners(ctx types.Context) []types.Account { - return ctx.Value(contextKeySigners).([]types.Account) + v := ctx.Value(contextKeySigners) + if v == nil { + return []types.Account{} + } + return v.([]types.Account) } From 2ed4de5e8dc458b7caecc6840520d56eac807e25 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sat, 17 Mar 2018 21:40:44 +0100 Subject: [PATCH 08/10] shame: forgot x/auth/context_test.go --- x/auth/context_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 x/auth/context_test.go diff --git a/x/auth/context_test.go b/x/auth/context_test.go new file mode 100644 index 0000000000..0e4db8b080 --- /dev/null +++ b/x/auth/context_test.go @@ -0,0 +1,39 @@ +package auth + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + abci "github.com/tendermint/abci/types" + + sdk "github.com/cosmos/cosmos-sdk/types" +) + +func TestContextWithSigners(t *testing.T) { + ms, _ := setupMultiStore() + ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, nil) + + _, _, addr1 := keyPubAddr() + _, _, addr2 := keyPubAddr() + acc1 := NewBaseAccountWithAddress(addr1) + acc1.SetSequence(7132) + acc2 := NewBaseAccountWithAddress(addr2) + acc2.SetSequence(8821) + + // new ctx has no signers + signers := GetSigners(ctx) + assert.Equal(t, 0, len(signers)) + + ctx2 := WithSigners(ctx, []sdk.Account{&acc1, &acc2}) + + // original context is unchanged + signers = GetSigners(ctx) + assert.Equal(t, 0, len(signers)) + + // new context has signers + signers = GetSigners(ctx2) + assert.Equal(t, 2, len(signers)) + assert.Equal(t, acc1, *(signers[0].(*BaseAccount))) + assert.Equal(t, acc2, *(signers[1].(*BaseAccount))) +} From a908a01fe2fe8300a43a831787fc9fe9c6fbc768 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sat, 17 Mar 2018 21:54:21 +0100 Subject: [PATCH 09/10] types/tx_msg_test.go --- types/tx_msg_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 types/tx_msg_test.go diff --git a/types/tx_msg_test.go b/types/tx_msg_test.go new file mode 100644 index 0000000000..f72cdea26e --- /dev/null +++ b/types/tx_msg_test.go @@ -0,0 +1,30 @@ +package types + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + crypto "github.com/tendermint/go-crypto" +) + +func newStdFee() StdFee { + return NewStdFee(100, + Coin{"atom", 150}, + ) +} + +func TestStdTx(t *testing.T) { + priv := crypto.GenPrivKeyEd25519() + addr := priv.PubKey().Address() + msg := NewTestMsg(addr) + fee := newStdFee() + sigs := []StdSignature{} + + tx := NewStdTx(msg, fee, sigs) + assert.Equal(t, msg, tx.GetMsg()) + assert.Equal(t, sigs, tx.GetSignatures()) + + feePayer := FeePayer(tx) + assert.Equal(t, addr, feePayer) +} From 7c3213fa00198e75fa732920807b86eee86a591c Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sat, 17 Mar 2018 21:53:27 +0100 Subject: [PATCH 10/10] fixes from review --- types/errors.go | 2 +- types/tx_msg.go | 29 +++++++++++++++++++++++++++ x/auth/ante.go | 4 +++- x/auth/ante_test.go | 40 +++++++++++++------------------------- x/auth/baseaccount_test.go | 5 +++-- 5 files changed, 49 insertions(+), 31 deletions(-) diff --git a/types/errors.go b/types/errors.go index 5c96d8c241..48e08b1164 100644 --- a/types/errors.go +++ b/types/errors.go @@ -52,7 +52,7 @@ func CodeToDefaultMsg(code CodeType) string { case CodeUnrecognizedAddress: return "Unrecognized address" case CodeInvalidPubKey: - return "Missing pubkey" + return "Invalid pubkey" default: return fmt.Sprintf("Unknown code %d", code) } diff --git a/types/tx_msg.go b/types/tx_msg.go index b41d9879a9..21bc330540 100644 --- a/types/tx_msg.go +++ b/types/tx_msg.go @@ -146,3 +146,32 @@ func (msg StdSignMsg) Bytes() []byte { // Application function variable used to unmarshal transaction bytes type TxDecoder func(txBytes []byte) (Tx, Error) + +//__________________________________________________________ + +var _ Msg = (*TestMsg)(nil) + +// msg type for testing +type TestMsg struct { + signers []Address +} + +func NewTestMsg(addrs ...Address) *TestMsg { + return &TestMsg{ + signers: addrs, + } +} + +func (msg *TestMsg) Type() string { return "TestMsg" } +func (msg *TestMsg) Get(key interface{}) (value interface{}) { return nil } +func (msg *TestMsg) GetSignBytes() []byte { + bz, err := json.Marshal(msg.signers) + if err != nil { + panic(err) + } + return bz +} +func (msg *TestMsg) ValidateBasic() Error { return nil } +func (msg *TestMsg) GetSigners() []Address { + return msg.signers +} diff --git a/x/auth/ante.go b/x/auth/ante.go index 2da0d92868..43b9bb8341 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -133,7 +133,9 @@ func processSig( return } -// deduct the fee from the account +// Deduct the fee from the account. +// We could use the CoinKeeper (in addition to the AccountMapper, +// because the CoinKeeper doesn't give us accounts), but it seems easier to do this. func deductFees(acc sdk.Account, fee sdk.StdFee) (sdk.Account, sdk.Result) { coins := acc.GetCoins() feeAmount := fee.Amount diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go index ecd2226230..5bbb3d0b69 100644 --- a/x/auth/ante_test.go +++ b/x/auth/ante_test.go @@ -1,7 +1,6 @@ package auth import ( - "encoding/json" "testing" sdk "github.com/cosmos/cosmos-sdk/types" @@ -11,29 +10,8 @@ import ( crypto "github.com/tendermint/go-crypto" ) -// msg type for testing -type testMsg struct { - signers []sdk.Address -} - -func newTestMsg(addrs ...sdk.Address) *testMsg { - return &testMsg{ - signers: addrs, - } -} - -func (msg *testMsg) Type() string { return "testMsg" } -func (msg *testMsg) Get(key interface{}) (value interface{}) { return nil } -func (msg *testMsg) GetSignBytes() []byte { - bz, err := json.Marshal(msg.signers) - if err != nil { - panic(err) - } - return bz -} -func (msg *testMsg) ValidateBasic() sdk.Error { return nil } -func (msg *testMsg) GetSigners() []sdk.Address { - return msg.signers +func newTestMsg(addrs ...sdk.Address) *sdk.TestMsg { + return sdk.NewTestMsg(addrs...) } func newStdFee() sdk.StdFee { @@ -246,6 +224,10 @@ func TestAnteHandlerBadSignBytes(t *testing.T) { var tx sdk.Tx msg := newTestMsg(addr1) fee := newStdFee() + fee2 := newStdFee() + fee2.Gas += 100 + fee3 := newStdFee() + fee3.Amount[0].Amount += 100 // test good tx and signBytes privs, seqs := []crypto.PrivKey{priv1}, []int64{0} @@ -253,6 +235,7 @@ func TestAnteHandlerBadSignBytes(t *testing.T) { checkValidTx(t, anteHandler, ctx, tx) chainID := ctx.ChainID() + chainID2 := chainID + "somemorestuff" codeUnauth := sdk.CodeUnauthorized cases := []struct { @@ -262,9 +245,12 @@ func TestAnteHandlerBadSignBytes(t *testing.T) { msg sdk.Msg code sdk.CodeType }{ - {"", []int64{1}, fee, msg, codeUnauth}, // test invalid chain_id - {chainID, []int64{2}, fee, msg, codeUnauth}, // test wrong seqs - {chainID, []int64{1}, fee, newTestMsg(addr2), codeUnauth}, // test wrong msg + {chainID2, []int64{1}, fee, msg, codeUnauth}, // test wrong chain_id + {chainID, []int64{2}, fee, msg, codeUnauth}, // test wrong seqs + {chainID, []int64{1, 2}, fee, msg, codeUnauth}, // test wrong seqs + {chainID, []int64{1}, fee, newTestMsg(addr2), codeUnauth}, // test wrong msg + {chainID, []int64{1}, fee2, newTestMsg(addr2), codeUnauth}, // test wrong fee + {chainID, []int64{1}, fee3, newTestMsg(addr2), codeUnauth}, // test wrong fee } privs, seqs = []crypto.PrivKey{priv1}, []int64{1} diff --git a/x/auth/baseaccount_test.go b/x/auth/baseaccount_test.go index 85adc8cdca..b2f5b54ae2 100644 --- a/x/auth/baseaccount_test.go +++ b/x/auth/baseaccount_test.go @@ -27,7 +27,7 @@ func TestBaseAccountAddressPubKey(t *testing.T) { assert.EqualValues(t, addr1, acc.GetAddress()) assert.EqualValues(t, crypto.PubKey{}, acc.GetPubKey()) - // cant override address + // can't override address err := acc.SetAddress(addr2) assert.NotNil(t, err) assert.EqualValues(t, addr1, acc.GetAddress()) @@ -37,7 +37,7 @@ func TestBaseAccountAddressPubKey(t *testing.T) { assert.Nil(t, err) assert.Equal(t, pub1, acc.GetPubKey()) - // cant override pubkey + // can't override pubkey err = acc.SetPubKey(pub2) assert.NotNil(t, err) assert.Equal(t, pub1, acc.GetPubKey()) @@ -111,6 +111,7 @@ func TestBaseAccountGetSet(t *testing.T) { _, _, addr := keyPubAddr() acc := NewBaseAccountWithAddress(addr) + // Get/Set are not yet defined - all values cause a panic. assert.Panics(t, func() { acc.Get("key") }) assert.Panics(t, func() { acc.Set("key", "value") }) }