Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cover transaction manager with additional test cases #561 #601

Merged
merged 9 commits into from
Feb 5, 2018
Merged

Cover transaction manager with additional test cases #561 #601

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d81554b
more configuration for setupTransactionPoolAPI
canercidam Feb 1, 2018
4c3d4be
change timeout
canercidam Feb 1, 2018
155a424
fix bad expected values
canercidam Feb 1, 2018
35a2a31
revert timeout
canercidam Feb 1, 2018
e3d708a
merge fixes from #537
canercidam Feb 2, 2018
16a05eb
fix hanging test
canercidam Feb 2, 2018
1b3dbde
table-driven gas and gas price tests
canercidam Feb 2, 2018
70e00f7
rename ctxTimeout to rpcCallTimeout
canercidam Feb 5, 2018
347f726
reduce args
canercidam Feb 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions geth/transactions/txqueue_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type Manager struct {
addrLock *AddrLocker
notify bool
completionTimeout time.Duration
rpcCallTimeout time.Duration
}

// NewManager returns a new Manager.
Expand All @@ -43,6 +44,7 @@ func NewManager(nodeManager common.NodeManager, accountManager common.AccountMan
addrLock: &AddrLocker{},
notify: true,
completionTimeout: DefaultTxSendCompletionTimeout,
rpcCallTimeout: defaultTimeout,
}
}

Expand Down Expand Up @@ -166,7 +168,7 @@ func (m *Manager) completeTransaction(queuedTx *common.QueuedTx, selectedAccount
}
m.addrLock.LockAddr(queuedTx.Args.From)
defer m.addrLock.UnlockAddr(queuedTx.Args.From)
ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
ctx, cancel := context.WithTimeout(context.Background(), m.rpcCallTimeout)
defer cancel()
nonce, err := m.ethTxClient.PendingNonceAt(ctx, queuedTx.Args.From)
if err != nil {
Expand All @@ -175,7 +177,7 @@ func (m *Manager) completeTransaction(queuedTx *common.QueuedTx, selectedAccount
args := queuedTx.Args
gasPrice := (*big.Int)(args.GasPrice)
if args.GasPrice == nil {
ctx, cancel = context.WithTimeout(context.Background(), defaultTimeout)
ctx, cancel = context.WithTimeout(context.Background(), m.rpcCallTimeout)
defer cancel()
gasPrice, err = m.ethTxClient.SuggestGasPrice(ctx)
if err != nil {
Expand All @@ -193,7 +195,7 @@ func (m *Manager) completeTransaction(queuedTx *common.QueuedTx, selectedAccount

gas := (*big.Int)(args.Gas)
if args.Gas == nil {
ctx, cancel = context.WithTimeout(context.Background(), defaultTimeout)
ctx, cancel = context.WithTimeout(context.Background(), m.rpcCallTimeout)
defer cancel()
gas, err = m.ethTxClient.EstimateGas(ctx, ethereum.CallMsg{
From: args.From,
Expand Down Expand Up @@ -224,7 +226,7 @@ func (m *Manager) completeTransaction(queuedTx *common.QueuedTx, selectedAccount
if err != nil {
return hash, err
}
ctx, cancel = context.WithTimeout(context.Background(), defaultTimeout)
ctx, cancel = context.WithTimeout(context.Background(), m.rpcCallTimeout)
defer cancel()
if err := m.ethTxClient.SendTransaction(ctx, signedTx); err != nil {
return hash, err
Expand Down
160 changes: 115 additions & 45 deletions geth/transactions/txqueue_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import (
"github.com/ethereum/go-ethereum/accounts/keystore"
gethcommon "github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/rlp"
gethrpc "github.com/ethereum/go-ethereum/rpc"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/suite"
Expand Down Expand Up @@ -61,19 +63,53 @@ func (s *TxQueueTestSuite) TearDownTest() {
s.client.Close()
}

func (s *TxQueueTestSuite) setupTransactionPoolAPI(account *common.SelectedExtKey, nonce hexutil.Uint64, gas hexutil.Big, txErr error) {
s.txServiceMock.EXPECT().GetTransactionCount(gomock.Any(), account.Address, gethrpc.PendingBlockNumber).Return(&nonce, nil)
s.txServiceMock.EXPECT().GasPrice(gomock.Any()).Return(big.NewInt(10), nil)
s.txServiceMock.EXPECT().EstimateGas(gomock.Any(), gomock.Any()).Return(&gas, nil)
s.txServiceMock.EXPECT().SendRawTransaction(gomock.Any(), gomock.Any()).Return(gethcommon.Hash{}, txErr)
func (s *TxQueueTestSuite) setupTransactionPoolAPI(tx *common.QueuedTx, config *params.NodeConfig, account *common.SelectedExtKey, nonce *hexutil.Uint64, gas *hexutil.Big, txErr error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about preparing tx earlier and passing it to this method? it looks like it will cleanup an interface

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, what do you mean by "preparing tx earlier and passing it to this method"? Isn't that already how it works?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought that it is possible to pass types.Transaction instead of tx *common.QueuedTx, config *params.NodeConfig, account *common.SelectedExtKey, nonce *hexutil.Uint64, gas *hexutil.Big

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, I may be able to further reduce the code using types.Transaction. I'll look into it.

Copy link
Contributor Author

@canercidam canercidam Feb 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second glance, it will just result in moving lines elsewhere because one has to form both types.Transaction and common.QueuedTx objects in the same case. In other words, if I define a types.Transaction object in the test case and pass it to setupTransactionPoolAPI instead, that means I can remove forming a new object from s.rlpEncode and it doesn't make a difference about how I handle rest of the args such as gas, gas price and nonce.

But it's definitely possible to reduce the amount of args there by having default test values so I'm going to do that instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for checking

// Expect calls to gas functions only if there are no user defined values.
// And also set the expected gas and gas price for RLP encoding the expected tx.
var usedGas, usedGasPrice *big.Int
s.txServiceMock.EXPECT().GetTransactionCount(gomock.Any(), account.Address, gethrpc.PendingBlockNumber).Return(nonce, nil)
if tx.Args.GasPrice == nil {
usedGasPrice = big.NewInt(10)
s.txServiceMock.EXPECT().GasPrice(gomock.Any()).Return(usedGasPrice, nil)
} else {
usedGasPrice = (*big.Int)(tx.Args.GasPrice)
}
if tx.Args.Gas == nil {
s.txServiceMock.EXPECT().EstimateGas(gomock.Any(), gomock.Any()).Return(gas, nil)
usedGas = (*big.Int)(gas)
} else {
usedGas = (*big.Int)(tx.Args.Gas)
}
// Prepare the transaction anD RLP encode it.
data := s.rlpEncodeTx(tx, config, account, nonce, usedGas, usedGasPrice)
// Expect the RLP encoded transaction.
s.txServiceMock.EXPECT().SendRawTransaction(gomock.Any(), data).Return(gethcommon.Hash{}, txErr)
}

func (s *TxQueueTestSuite) rlpEncodeTx(tx *common.QueuedTx, config *params.NodeConfig, account *common.SelectedExtKey, nonce *hexutil.Uint64, gas, gasPrice *big.Int) hexutil.Bytes {
newTx := types.NewTransaction(
uint64(*nonce),
gethcommon.Address(*tx.Args.To),
tx.Args.Value.ToInt(),
gas,
gasPrice,
tx.Args.Data,
)
chainID := big.NewInt(int64(config.NetworkID))
signedTx, err := types.SignTx(newTx, types.NewEIP155Signer(chainID), account.AccountKey.PrivateKey)
s.NoError(err)
data, err := rlp.EncodeToBytes(signedTx)
s.NoError(err)
return hexutil.Bytes(data)
}

func (s *TxQueueTestSuite) setupStatusBackend(account *common.SelectedExtKey, password string, passwordErr error) {
func (s *TxQueueTestSuite) setupStatusBackend(account *common.SelectedExtKey, password string, passwordErr error) *params.NodeConfig {
nodeConfig, nodeErr := params.NewNodeConfig("/tmp", params.RopstenNetworkID, true)
s.nodeManagerMock.EXPECT().NodeConfig().Return(nodeConfig, nodeErr)
s.accountManagerMock.EXPECT().SelectedAccount().Return(account, nil)
s.accountManagerMock.EXPECT().VerifyAccountPassword(nodeConfig.KeyStoreDir, account.Address.String(), password).Return(
nil, passwordErr)
return nodeConfig
}

func (s *TxQueueTestSuite) TestCompleteTransaction() {
Expand All @@ -83,41 +119,72 @@ func (s *TxQueueTestSuite) TestCompleteTransaction() {
Address: common.FromAddress(TestConfig.Account1.Address),
AccountKey: &keystore.Key{PrivateKey: key},
}
s.setupStatusBackend(account, password, nil)

nonce := hexutil.Uint64(10)
gas := hexutil.Big(*big.NewInt(defaultGas + 1))
s.setupTransactionPoolAPI(account, nonce, gas, nil)

txQueueManager := NewManager(s.nodeManagerMock, s.accountManagerMock)

txQueueManager.Start()
defer txQueueManager.Stop()

tx := common.CreateTransaction(context.Background(), common.SendTxArgs{
From: common.FromAddress(TestConfig.Account1.Address),
To: common.ToAddress(TestConfig.Account2.Address),
})

s.NoError(txQueueManager.QueueTransaction(tx))
w := make(chan struct{})
var (
hash gethcommon.Hash
err error
)
go func() {
hash, err = txQueueManager.CompleteTransaction(tx.ID, password)
s.NoError(err)
close(w)
}()
testCases := []struct {
name string
gas *hexutil.Big
gasPrice *hexutil.Big
}{
{
"noGasDef",
nil,
nil,
},
{
"gasDefined",
(*hexutil.Big)(big.NewInt(100)),
nil,
},
{
"gasPriceDefined",
nil,
(*hexutil.Big)(big.NewInt(100)),
},
}

rst := txQueueManager.WaitForTransaction(tx)
// Check that error is assigned to the transaction.
s.NoError(rst.Error)
// Transaction should be already removed from the queue.
s.False(txQueueManager.TransactionQueue().Has(tx.ID))
s.NoError(WaitClosed(w, time.Second))
s.Equal(hash, rst.Hash)
for _, testCase := range testCases {
s.T().Run(testCase.name, func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never used it with testify, will it allow to use testify.m the same way as before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it doesn't detect that. But IMHO, it's certainly more readable & DRY and all tests run in few seconds anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good i think

go test ./geth/transactions/ -v -testify.m=TestCompleteTransaction$
=== RUN TestTxQueueTestSuite
=== RUN TestTxQueueTestSuite/TestCompleteTransaction
=== RUN TestTxQueueTestSuite/TestCompleteTransaction/noGasDef
=== RUN TestTxQueueTestSuite/TestCompleteTransaction/gasDefined
=== RUN TestTxQueueTestSuite/TestCompleteTransaction/gasPriceDefined
--- PASS: TestTxQueueTestSuite (0.01s)
--- PASS: TestTxQueueTestSuite/TestCompleteTransaction (0.01s)
--- PASS: TestTxQueueTestSuite/TestCompleteTransaction/noGasDef (0.00s)
--- PASS: TestTxQueueTestSuite/TestCompleteTransaction/gasDefined (0.00s)
--- PASS: TestTxQueueTestSuite/TestCompleteTransaction/gasPriceDefined (0.00s)
PASS
ok github.com/status-im/status-go/geth/transactions 0.055s

s.SetupTest()
config := s.setupStatusBackend(account, password, nil)
tx := common.CreateTransaction(context.Background(), common.SendTxArgs{
From: common.FromAddress(TestConfig.Account1.Address),
To: common.ToAddress(TestConfig.Account2.Address),
Gas: testCase.gas,
GasPrice: testCase.gasPrice,
})
s.setupTransactionPoolAPI(tx, config, account, &nonce, &gas, nil)

txQueueManager := NewManager(s.nodeManagerMock, s.accountManagerMock)
txQueueManager.completionTimeout = time.Second
txQueueManager.rpcCallTimeout = time.Second

txQueueManager.Start()
defer txQueueManager.Stop()

s.NoError(txQueueManager.QueueTransaction(tx))
w := make(chan struct{})
var (
hash gethcommon.Hash
err error
)
go func() {
hash, err = txQueueManager.CompleteTransaction(tx.ID, password)
s.NoError(err)
close(w)
}()

rst := txQueueManager.WaitForTransaction(tx)
// Check that error is assigned to the transaction.
s.NoError(rst.Error)
// Transaction should be already removed from the queue.
s.False(txQueueManager.TransactionQueue().Has(tx.ID))
s.NoError(WaitClosed(w, time.Second))
s.Equal(hash, rst.Hash)
})
}
}

func (s *TxQueueTestSuite) TestCompleteTransactionMultipleTimes() {
Expand All @@ -127,23 +194,26 @@ func (s *TxQueueTestSuite) TestCompleteTransactionMultipleTimes() {
Address: common.FromAddress(TestConfig.Account1.Address),
AccountKey: &keystore.Key{PrivateKey: key},
}
s.setupStatusBackend(account, password, nil)
config := s.setupStatusBackend(account, password, nil)

tx := common.CreateTransaction(context.Background(), common.SendTxArgs{
From: common.FromAddress(TestConfig.Account1.Address),
To: common.ToAddress(TestConfig.Account2.Address),
})

nonce := hexutil.Uint64(10)
gas := hexutil.Big(*big.NewInt(defaultGas + 1))
s.setupTransactionPoolAPI(account, nonce, gas, nil)
s.setupTransactionPoolAPI(tx, config, account, &nonce, &gas, nil)

txQueueManager := NewManager(s.nodeManagerMock, s.accountManagerMock)
txQueueManager.completionTimeout = time.Second
txQueueManager.rpcCallTimeout = time.Second
txQueueManager.DisableNotificactions()
txQueueManager.Start()
defer txQueueManager.Stop()

tx := common.CreateTransaction(context.Background(), common.SendTxArgs{
From: common.FromAddress(TestConfig.Account1.Address),
To: common.ToAddress(TestConfig.Account2.Address),
})

s.NoError(txQueueManager.QueueTransaction(tx))
err := txQueueManager.QueueTransaction(tx)
s.NoError(err)

var (
wg sync.WaitGroup
Expand All @@ -158,14 +228,14 @@ func (s *TxQueueTestSuite) TestCompleteTransactionMultipleTimes() {
defer wg.Done()
_, err := txQueueManager.CompleteTransaction(tx.ID, password)
mu.Lock()
defer mu.Unlock()
if err == nil {
completedTx++
} else if err == queue.ErrQueuedTxInProgress {
inprogressTx++
} else {
s.Fail("tx failed with unexpected error: ", err.Error())
}
mu.Unlock()
}()
}

Expand Down