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

Revise all submit-transaction methods in the TxHelpers module #352

Open
barakman opened this issue Feb 5, 2024 · 1 comment
Open

Revise all submit-transaction methods in the TxHelpers module #352

barakman opened this issue Feb 5, 2024 · 1 comment
Assignees
Labels
bug Something isn't working

Comments

@barakman
Copy link
Collaborator

barakman commented Feb 5, 2024

The following methods are used for submitting transactions:

  1. submit_transaction
  2. submit_private_transaction
  3. cancel_private_transaction

These methods are used inconsistently across the code, sometimes receiving a signed transaction as input, sometimes receiving an unsigned transaction as input, and sometimes receiving a raw transaction as input.

In addition to that, not every attempt to submit a transaction is complemented with an attempt to cancel it upon timeout.
This requires a higher-level decision on which transactions should be cancelled when needed, and which should not be.

Technically, submitting a transaction requires only the signed transaction as input, but cancelling it requires the original (unsigned) transaction which was used in order to create it, as input.

So designing a solution for this problem should also take into account how every submitted transaction which may need to be cancelled, can be easily cancelled.

An addition bug in the cancellation method, is that it resets the transaction's data attribute but not its value attribute.
As a result, cancelling a transaction which includes ETH, might end up with that transaction being only "partially cancelled" (namely, the ETH-transfer part in that transaction will effectively take place despite the cancellation).

The aforementioned methods are used only in one place outside of this module, in method generate_shutdown_tx.
So the implemented solution should also include this specific usage.


Naming and coding conventions:

  1. A signed transaction can be referred to as signed_tx; no need to use signed_arb_tx in the method itself, as it is designated for handling any type of transaction and not just arb transactions; same goes for unsigned_tx and raw_tx; of course, in the calling method they should be named based on what they truly represent (e.g., approve_tx)
  2. Methods submit_transaction is designated for "tenderly OR non-ethereum", while method submit_private_transaction is designated for all other cases, i.e., "non-tenderly AND ethereum"; These two methods should be named accordingly

An additional issue:

  • Methods submit_transaction returns tx_receipt
  • Methods submit_private_transaction returns tx_receipt["transactionHash"]

Where tx_receipt is the dictionary returned from web3.eth.wait_for_transaction_receipt in BOTH cases.
This could potentially lead to unpredictable and undesirable behavior.

@barakman barakman added the bug Something isn't working label Feb 5, 2024
@barakman barakman self-assigned this Feb 5, 2024
@barakman
Copy link
Collaborator Author

barakman commented Feb 5, 2024

Handled in PR #355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant