From 3d5cf393b938a1041e8ff8217f610d0d808e1dbf Mon Sep 17 00:00:00 2001 From: rigel rozanski Date: Fri, 14 Jul 2017 16:29:43 -0400 Subject: [PATCH] PR changes --- app/app_test.go | 20 ++++++------ cmd/basecli/commands/cmds.go | 9 +++--- docs/guide/counter/plugins/counter/counter.go | 5 ++- errors/common.go | 20 ------------ modules/nonce/errors.go | 32 +++++++++++++++++++ modules/nonce/replaycheck.go | 10 ++++-- modules/nonce/tx.go | 6 ++-- modules/nonce/tx_test.go | 32 ++++++++++++++++--- tests/cli/common.sh | 16 +++++----- tests/cli/counter.sh | 8 ++--- 10 files changed, 99 insertions(+), 59 deletions(-) create mode 100644 modules/nonce/errors.go diff --git a/app/app_test.go b/app/app_test.go index 629ec750c1..2342bf2414 100644 --- a/app/app_test.go +++ b/app/app_test.go @@ -56,17 +56,17 @@ func (at *appTest) signTx(tx basecoin.Tx) basecoin.Tx { return stx.Wrap() } -func (at *appTest) getTx(coins coin.Coins) basecoin.Tx { +func (at *appTest) getTx(coins coin.Coins, sequence uint32) basecoin.Tx { tx := at.baseTx(coins) - tx = nonce.NewTx(1, []basecoin.Actor{at.acctIn.Actor()}, tx) + tx = nonce.NewTx(sequence, []basecoin.Actor{at.acctIn.Actor()}, tx) tx = base.NewChainTx(at.chainID, 0, tx) return at.signTx(tx) } -func (at *appTest) feeTx(coins coin.Coins, toll coin.Coin) basecoin.Tx { +func (at *appTest) feeTx(coins coin.Coins, toll coin.Coin, sequence uint32) basecoin.Tx { tx := at.baseTx(coins) tx = fee.NewFee(tx, toll, at.acctIn.Actor()) - tx = nonce.NewTx(1, []basecoin.Actor{at.acctIn.Actor()}, tx) + tx = nonce.NewTx(sequence, []basecoin.Actor{at.acctIn.Actor()}, tx) tx = base.NewChainTx(at.chainID, 0, tx) return at.signTx(tx) } @@ -213,22 +213,22 @@ func TestTx(t *testing.T) { //Bad Balance at.acctIn.Coins = coin.Coins{{"mycoin", 2}} at.initAccount(at.acctIn) - res, _, _ := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}), true) + res, _, _ := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}, 1), true) assert.True(res.IsErr(), "ExecTx/Bad CheckTx: Expected error return from ExecTx, returned: %v", res) - res, diffIn, diffOut := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}), false) + res, diffIn, diffOut := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}, 1), false) assert.True(res.IsErr(), "ExecTx/Bad DeliverTx: Expected error return from ExecTx, returned: %v", res) assert.True(diffIn.IsZero()) assert.True(diffOut.IsZero()) //Regular CheckTx at.reset() - res, _, _ = at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}), true) + res, _, _ = at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}, 1), true) assert.True(res.IsOK(), "ExecTx/Good CheckTx: Expected OK return from ExecTx, Error: %v", res) //Regular DeliverTx at.reset() amt := coin.Coins{{"mycoin", 3}} - res, diffIn, diffOut = at.exec(t, at.getTx(amt), false) + res, diffIn, diffOut = at.exec(t, at.getTx(amt, 1), false) assert.True(res.IsOK(), "ExecTx/Good DeliverTx: Expected OK return from ExecTx, Error: %v", res) assert.Equal(amt.Negative(), diffIn) assert.Equal(amt, diffOut) @@ -237,7 +237,7 @@ func TestTx(t *testing.T) { at.reset() amt = coin.Coins{{"mycoin", 4}} toll := coin.Coin{"mycoin", 1} - res, diffIn, diffOut = at.exec(t, at.feeTx(amt, toll), false) + res, diffIn, diffOut = at.exec(t, at.feeTx(amt, toll, 1), false) assert.True(res.IsOK(), "ExecTx/Good DeliverTx: Expected OK return from ExecTx, Error: %v", res) payment := amt.Plus(coin.Coins{toll}).Negative() assert.Equal(payment, diffIn) @@ -249,7 +249,7 @@ func TestQuery(t *testing.T) { assert := assert.New(t) at := newAppTest(t) - res, _, _ := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}), false) + res, _, _ := at.exec(t, at.getTx(coin.Coins{{"mycoin", 5}}, 1), false) assert.True(res.IsOK(), "Commit, DeliverTx: Expected OK return from DeliverTx, Error: %v", res) resQueryPreCommit := at.app.Query(abci.RequestQuery{ diff --git a/cmd/basecli/commands/cmds.go b/cmd/basecli/commands/cmds.go index cf97f9581c..3732f38fc0 100644 --- a/cmd/basecli/commands/cmds.go +++ b/cmd/basecli/commands/cmds.go @@ -120,8 +120,8 @@ func WrapNonceTx(tx basecoin.Tx) (res basecoin.Tx, err error) { return res, fmt.Errorf("sequence must be greater than 0") } signers := []basecoin.Actor{GetSignerAct()} - tx = nonce.NewTx(uint32(seq), signers, tx) - return tx, nil + res = nonce.NewTx(uint32(seq), signers, tx) + return } // WrapFeeTx checks for FlagFee and if present wraps the tx with a @@ -136,7 +136,8 @@ func WrapFeeTx(tx basecoin.Tx) (res basecoin.Tx, err error) { if toll.IsZero() { return tx, nil } - return fee.NewFee(tx, toll, GetSignerAct()), nil + res = fee.NewFee(tx, toll, GetSignerAct()) + return } // WrapChainTx will wrap the tx with a ChainTx from the standard flags @@ -147,7 +148,7 @@ func WrapChainTx(tx basecoin.Tx) (res basecoin.Tx, err error) { return res, errors.New("No chain-id provided") } res = base.NewChainTx(chain, uint64(expires), tx) - return res, nil + return } // GetSignerAct returns the address of the signer of the tx diff --git a/docs/guide/counter/plugins/counter/counter.go b/docs/guide/counter/plugins/counter/counter.go index d99a3c41ea..df07195f4c 100644 --- a/docs/guide/counter/plugins/counter/counter.go +++ b/docs/guide/counter/plugins/counter/counter.go @@ -35,9 +35,8 @@ func init() { // Tx - struct for all counter transactions type Tx struct { - Valid bool `json:"valid"` - Fee coin.Coins `json:"fee"` - Sequence int `json:""` + Valid bool `json:"valid"` + Fee coin.Coins `json:"fee"` } // NewTx - return a new counter transaction struct wrapped as a basecoin transaction diff --git a/errors/common.go b/errors/common.go index a6f37a71ab..1ef42177f9 100644 --- a/errors/common.go +++ b/errors/common.go @@ -19,20 +19,14 @@ var ( errMissingSignature = fmt.Errorf("Signature missing") errTooManySignatures = fmt.Errorf("Too many signatures") errNoChain = fmt.Errorf("No chain id provided") - errNoNonce = fmt.Errorf("Tx doesn't contain nonce") - errNotMember = fmt.Errorf("nonce contains non-permissioned member") - errBadNonce = fmt.Errorf("Bad nonce sequence") errTxEmpty = fmt.Errorf("The provided Tx is empty") - errZeroSequence = fmt.Errorf("Sequence number cannot be zero") errWrongChain = fmt.Errorf("Wrong chain for tx") errUnknownTxType = fmt.Errorf("Tx type unknown") errInvalidFormat = fmt.Errorf("Invalid format") errUnknownModule = fmt.Errorf("Unknown module") errExpired = fmt.Errorf("Tx expired") errUnknownKey = fmt.Errorf("Unknown key") -) -var ( internalErr = abci.CodeType_InternalError encodingErr = abci.CodeType_EncodingError unauthorized = abci.CodeType_Unauthorized @@ -142,20 +136,6 @@ func IsNoChainErr(err error) bool { return IsSameError(errNoChain, err) } -func ErrNoNonce() TMError { - return WithCode(errNoNonce, unauthorized) -} -func ErrBadNonce(got, expected uint32) TMError { - return WithCode(fmt.Errorf("Bad nonce sequence, got %d, expected %d", got, expected), unauthorized) -} -func ErrNotMember() TMError { - return WithCode(errBadNonce, unauthorized) -} - -func ErrZeroSequence() TMError { - return WithCode(errZeroSequence, unauthorized) -} - func ErrTxEmpty() TMError { return WithCode(errTxEmpty, unauthorized) } diff --git a/modules/nonce/errors.go b/modules/nonce/errors.go new file mode 100644 index 0000000000..f88629b457 --- /dev/null +++ b/modules/nonce/errors.go @@ -0,0 +1,32 @@ +//nolint +package nonce + +import ( + "fmt" + + abci "github.com/tendermint/abci/types" + + "github.com/tendermint/basecoin/errors" +) + +var ( + errNoNonce = fmt.Errorf("Tx doesn't contain nonce") + errNotMember = fmt.Errorf("nonce contains non-permissioned member") + errZeroSequence = fmt.Errorf("Sequence number cannot be zero") + + unauthorized = abci.CodeType_Unauthorized +) + +func ErrBadNonce(got, expected uint32) errors.TMError { + return errors.WithCode(fmt.Errorf("Bad nonce sequence, got %d, expected %d", got, expected), unauthorized) +} + +func ErrNoNonce() errors.TMError { + return errors.WithCode(errNoNonce, unauthorized) +} +func ErrNotMember() errors.TMError { + return errors.WithCode(errNotMember, unauthorized) +} +func ErrZeroSequence() errors.TMError { + return errors.WithCode(errZeroSequence, unauthorized) +} diff --git a/modules/nonce/replaycheck.go b/modules/nonce/replaycheck.go index b9f28d253c..4494a2b109 100644 --- a/modules/nonce/replaycheck.go +++ b/modules/nonce/replaycheck.go @@ -2,7 +2,6 @@ package nonce import ( "github.com/tendermint/basecoin" - "github.com/tendermint/basecoin/errors" "github.com/tendermint/basecoin/stack" "github.com/tendermint/basecoin/state" ) @@ -57,11 +56,16 @@ func (r ReplayCheck) checkIncrementNonceTx(ctx basecoin.Context, store state.KVS // make sure it is a the nonce Tx (Tx from this package) nonceTx, ok := tx.Unwrap().(Tx) if !ok { - return tx, errors.ErrNoNonce() + return tx, ErrNoNonce() + } + + err := nonceTx.ValidateBasic() + if err != nil { + return tx, err } // check the nonce sequence number - err := nonceTx.CheckIncrementSeq(ctx, store) + err = nonceTx.CheckIncrementSeq(ctx, store) if err != nil { return tx, err } diff --git a/modules/nonce/tx.go b/modules/nonce/tx.go index bb3d197297..819ea12452 100644 --- a/modules/nonce/tx.go +++ b/modules/nonce/tx.go @@ -52,7 +52,7 @@ func (n Tx) ValidateBasic() error { case n.Tx.Empty(): return errors.ErrTxEmpty() case n.Sequence == 0: - return errors.ErrZeroSequence() + return ErrZeroSequence() case len(n.Signers) == 0: return errors.ErrNoSigners() } @@ -73,13 +73,13 @@ func (n Tx) CheckIncrementSeq(ctx basecoin.Context, store state.KVStore) error { return err } if n.Sequence != cur+1 { - return errors.ErrBadNonce(n.Sequence, cur+1) + return ErrBadNonce(n.Sequence, cur+1) } // make sure they all signed for _, s := range n.Signers { if !ctx.HasPermission(s) { - return errors.ErrNotMember() + return ErrNotMember() } } diff --git a/modules/nonce/tx_test.go b/modules/nonce/tx_test.go index 2d4406207d..0f967cd9b8 100644 --- a/modules/nonce/tx_test.go +++ b/modules/nonce/tx_test.go @@ -17,13 +17,25 @@ func TestNonce(t *testing.T) { // generic args here... chainID := "my-chain" + chain2ID := "woohoo" + height := uint64(100) ctx := stack.MockContext(chainID, height) store := state.NewMemKVStore() - act1 := basecoin.Actor{App: "fooz", Address: []byte{1, 2, 3, 4}} - act2 := basecoin.Actor{App: "fooz", Address: []byte{1, 1, 1, 1}} - act3 := basecoin.Actor{App: "fooz", Address: []byte{3, 3, 3, 3}} + appName1 := "fooz" + appName2 := "foot" + + //root actors for the tests + act1 := basecoin.Actor{ChainID: chainID, App: appName1, Address: []byte{1, 2, 3, 4}} + act2 := basecoin.Actor{ChainID: chainID, App: appName1, Address: []byte{1, 1, 1, 1}} + act3 := basecoin.Actor{ChainID: chainID, App: appName1, Address: []byte{3, 3, 3, 3}} + act1DiffChain := basecoin.Actor{ChainID: chain2ID, App: appName1, Address: []byte{1, 2, 3, 4}} + act2DiffChain := basecoin.Actor{ChainID: chain2ID, App: appName1, Address: []byte{1, 1, 1, 1}} + act3DiffChain := basecoin.Actor{ChainID: chain2ID, App: appName1, Address: []byte{3, 3, 3, 3}} + act1DiffApp := basecoin.Actor{ChainID: chainID, App: appName2, Address: []byte{1, 2, 3, 4}} + act2DiffApp := basecoin.Actor{ChainID: chainID, App: appName2, Address: []byte{1, 1, 1, 1}} + act3DiffApp := basecoin.Actor{ChainID: chainID, App: appName2, Address: []byte{3, 3, 3, 3}} // let's construct some tests to make the table a bit less verbose set0 := []basecoin.Actor{} @@ -34,6 +46,12 @@ func TestNonce(t *testing.T) { set123 := []basecoin.Actor{act1, act2, act3} set321 := []basecoin.Actor{act3, act2, act1} + //some more test cases for different chains and apps for each actor + set123Chain2 := []basecoin.Actor{act1DiffChain, act2DiffChain, act3DiffChain} + set123App2 := []basecoin.Actor{act1DiffApp, act2DiffApp, act3DiffApp} + set123MixedChains := []basecoin.Actor{act1, act2DiffChain, act3} + set123MixedApps := []basecoin.Actor{act1, act2DiffApp, act3} + testList := []struct { valid bool seq uint32 @@ -60,12 +78,18 @@ func TestNonce(t *testing.T) { {false, 2, set12, set12}, // but can't repeat sequence {true, 3, set12, set321}, // no effect from extra sigs - // tripple sigs also work + // triple sigs also work {false, 2, set123, set123}, // must start with seq=1 {false, 1, set123, set12}, // all must sign {true, 1, set123, set321}, // this works {true, 2, set321, set321}, // other order is the same {false, 2, set321, set321}, // no repetition + + // signers from different chain and apps + {false, 3, set123, set123Chain2}, // sign with different chain actors + {false, 3, set123, set123App2}, // sign with different app actors + {false, 3, set123, set123MixedChains}, // sign with mixed chain actor + {false, 3, set123, set123MixedApps}, // sign with mixed app actors } raw := stack.NewRawTx([]byte{42}) diff --git a/tests/cli/common.sh b/tests/cli/common.sh index 6242086f07..b195184b43 100644 --- a/tests/cli/common.sh +++ b/tests/cli/common.sh @@ -160,11 +160,11 @@ checkSendTx() { assertEquals "line=${LINENO}, proper height" $2 $(echo $TX | jq .height) assertEquals "line=${LINENO}, type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) - NTX=$(echo $TX | jq .data.data.tx) - assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) - CTX=$(echo $NTX | jq .data.tx) + CTX=$(echo $TX | jq .data.data.tx) assertEquals "line=${LINENO}, type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) - STX=$(echo $CTX | jq .data.tx) + NTX=$(echo $CTX | jq .data.tx) + assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) + STX=$(echo $NTX | jq .data.tx) assertEquals "line=${LINENO}, type=coin/send" '"coin/send"' $(echo $STX | jq .type) assertEquals "line=${LINENO}, proper sender" "\"$3\"" $(echo $STX | jq .data.inputs[0].address.addr) assertEquals "line=${LINENO}, proper out amount" "$4" $(echo $STX | jq .data.outputs[0].coins[0].amount) @@ -182,11 +182,11 @@ checkSendFeeTx() { assertEquals "line=${LINENO}, proper height" $2 $(echo $TX | jq .height) assertEquals "line=${LINENO}, type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) - NTX=$(echo $TX | jq .data.data.tx) - assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) - CTX=$(echo $NTX | jq .data.tx) + CTX=$(echo $TX | jq .data.data.tx) assertEquals "line=${LINENO}, type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) - FTX=$(echo $CTX | jq .data.tx) + NTX=$(echo $CTX | jq .data.tx) + assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) + FTX=$(echo $NTX | jq .data.tx) assertEquals "line=${LINENO}, type=fee/tx" '"fee/tx"' $(echo $FTX | jq .type) assertEquals "line=${LINENO}, proper fee" "$5" $(echo $FTX | jq .data.fee.amount) STX=$(echo $FTX | jq .data.tx) diff --git a/tests/cli/counter.sh b/tests/cli/counter.sh index 4c56eeaf88..f21c9066d0 100755 --- a/tests/cli/counter.sh +++ b/tests/cli/counter.sh @@ -83,11 +83,11 @@ test03AddCount() { if assertTrue "Line=${LINENO}, found tx" $?; then assertEquals "Line=${LINENO}, proper height" $TX_HEIGHT $(echo $TX | jq .height) assertEquals "Line=${LINENO}, type=sigs/one" '"sigs/one"' $(echo $TX | jq .data.type) - NTX=$(echo $TX | jq .data.data.tx) - assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) - CTX=$(echo $NTX | jq .data.tx) + CTX=$(echo $TX | jq .data.data.tx) assertEquals "Line=${LINENO}, type=chain/tx" '"chain/tx"' $(echo $CTX | jq .type) - CNTX=$(echo $CTX | jq .data.tx) + NTX=$(echo $CTX | jq .data.tx) + assertEquals "line=${LINENO}, type=nonce" '"nonce"' $(echo $NTX | jq .type) + CNTX=$(echo $NTX | jq .data.tx) assertEquals "Line=${LINENO}, type=cntr/count" '"cntr/count"' $(echo $CNTX | jq .type) assertEquals "Line=${LINENO}, proper fee" "10" $(echo $CNTX | jq .data.fee[0].amount) fi