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.

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

Expose CheckAttempt to TxManager, saves some unnecessary calls #1327

Conversation

j16r
Copy link
Contributor

@j16r j16r commented Jun 6, 2019

The initial phase of the EthTx adapter creates a transaction, then attempts to do gas bumping on it.

Gas Bumping is never needed on the initial transaction so half of this code is redundant, and because the existing records/information is not pushed into the BumpGasUntilSafe method, which is meant to be used with a freshly retrieved Tx, it creates a lot of redundant work. BumpGasUntilSafe actually needing to do any work in real life is highly unlikely, so I have optimised this path: instead just check for a receipt on the odd chance that the chain was quick to accept.

Full list of changes:

  • CheckAttempt has no side effects, and clearly indicates the states of a TxAttempt, this can now more easily be unit tested.
  • processAttempt (previously checkAttempt) now does all the work of gas bumping retrying etc. The logic of this code is far easier to understand now because the logic for determining state is no peppered in together.
  • BumpGasUntilSafe/CheckAttempt always return a receipt if one was retrieved, this makes them safer to use - the logic for using them can be simplified
  • BumpGasUntilSafe also now returns a state, and always returns a receipt (because it uses CheckAttempt which does the same) which means we can distinguish between fatal errors and just receipt retrieval errors, which we choose to ignore so that attempts can be checked again for the next head

@j16r j16r force-pushed the chore/eth_tx_adapter_initial_tx_creation_attempt_and_attempt_checking branch 2 times, most recently from dedd94a to fb62842 Compare June 6, 2019 21:05
@j16r j16r marked this pull request as ready for review June 6, 2019 21:10
@j16r j16r force-pushed the chore/eth_tx_adapter_initial_tx_creation_attempt_and_attempt_checking branch from fb62842 to b5669ea Compare June 6, 2019 21:18
@j16r j16r force-pushed the chore/eth_tx_adapter_initial_tx_creation_attempt_and_attempt_checking branch from b5669ea to ba12f49 Compare June 6, 2019 21:54
@j16r j16r requested review from dimroc and se3000 June 6, 2019 21:55
j16r added 4 commits June 6, 2019 16:27
Make sure receipt confirmed at is far enough ago that it has met minimum
number of confirmations because this test does not simulate new heads
@j16r j16r force-pushed the chore/eth_tx_adapter_initial_tx_creation_attempt_and_attempt_checking branch 2 times, most recently from a7a3add to 03391f6 Compare June 7, 2019 00:30
By returning the state from BumpGasUntilSafe the EthTx adapter can
distinguish between fatal errors that should kill the job, and those
that we can retry on (failing to get the tx receipt). It is now
orthogonal to CheckAttempt and has simpler "success" logic.
@j16r j16r force-pushed the chore/eth_tx_adapter_initial_tx_creation_attempt_and_attempt_checking branch from 03391f6 to 61cae6c Compare June 7, 2019 14:51
@j16r j16r force-pushed the chore/eth_tx_adapter_initial_tx_creation_attempt_and_attempt_checking branch 2 times, most recently from 842e630 to 57ccbd1 Compare June 7, 2019 17:36
@j16r j16r force-pushed the chore/eth_tx_adapter_initial_tx_creation_attempt_and_attempt_checking branch 3 times, most recently from b70dc5d to c83e3ed Compare June 7, 2019 18:59
@j16r j16r force-pushed the chore/eth_tx_adapter_initial_tx_creation_attempt_and_attempt_checking branch from c83e3ed to 32e57ce Compare June 7, 2019 19:39
dimroc
dimroc previously approved these changes Jun 8, 2019
@@ -176,9 +168,9 @@ func TestEthTxAdapter_Perform_ConfirmedWithBytesAndNoDataPrefix(t *testing.T) {
return nil
})
ethMock.Register("eth_blockNumber", utils.Uint64ToHex(sentAt))
receipt := models.TxReceipt{Hash: hash, BlockNumber: cltest.Int(confirmed)}
safe := sentAt - store.Config.MinOutgoingConfirmations()
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional

This line originally threw me off, because conceptually it's impossible for something to be safe before something that's sent. I later discovered you're using the receipt's BlockNumber to gauge whether something is safe so this works out.

If possible, I would recommend renaming sentAt to something else, perhaps currentHeight?

I would also rename the test TestEthTxAdapter_Perform_ConfirmedWithBytesAndNoDataPrefix from using the language Confirmed to using the more appropriate language Safe: TestEthTxAdapter_Perform_SafeWithBytesAndNoDataPrefix

@j16r j16r merged commit ff3a5f8 into master Jun 8, 2019
@j16r j16r deleted the chore/eth_tx_adapter_initial_tx_creation_attempt_and_attempt_checking branch June 8, 2019 18:08
asoliman92 pushed a commit that referenced this pull request Aug 27, 2024
asoliman92 pushed a commit that referenced this pull request Aug 28, 2024
asoliman92 pushed a commit that referenced this pull request Aug 28, 2024
asoliman92 pushed a commit that referenced this pull request Aug 28, 2024
asoliman92 pushed a commit that referenced this pull request Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants