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

Improve cleanup of pending transactions state #4025

Closed
usame-algan opened this issue Aug 1, 2024 · 2 comments · Fixed by #4057
Closed

Improve cleanup of pending transactions state #4025

usame-algan opened this issue Aug 1, 2024 · 2 comments · Fixed by #4057
Assignees
Labels
bug Something isn't working major Major bug (to be solved in the next release)

Comments

@usame-algan
Copy link
Member

Bug description

A pending transaction state is cleaned up whenever one of the following transaction events are emitted:

TxEvent.SIGNATURE_INDEXED | TxEvent.SUCCESS | TxEvent.REVERTED | TxEvent.FAILED

As long as there is still a pending transaction we don't allow users to execute another transaction. There are reports that users end up in this state even though there is seemingly no pending transaction.

We should implement a more robust way of cleaning up this state instead of only relying on events that are emitted if the app is opened and the tx watcher is running.

Steps to reproduce

Need to figure out steps to reproduce this. Simply submitting a transaction and closing the app doesn't work because as soon as the app is reopened, we start the watcher based on the pending tx state and the watcher emits those events.

Expected result

If there is no pending transaction in the queue there should also be no pending transaction in the local storage

Obtained result

Transaction is executed but the pending transaction state is still there

@usame-algan usame-algan added the bug Something isn't working label Aug 1, 2024
@katspaugh katspaugh added the major Major bug (to be solved in the next release) label Aug 1, 2024
@usame-algan
Copy link
Member Author

usame-algan commented Aug 1, 2024

Points to discuss:

  • Adding a TTL for pending transactions so they are cleaned up one way or another
  • Allowing users to execute transactions even if there is still a pending transaction but in case it has already been indexed
  • Check if the contract nonce has increased and clean up pending txs accordingly
  • Check the queued txs against pending txs and clean up those that don't have a match

@usame-algan
Copy link
Member Author

usame-algan commented Aug 9, 2024

The issue is a bit more nuanced. If a pending transaction is found in the history we emit a SUCCESS event which then cleans up the pending state. However if in the meantime the history grew too much (more than 25 transactions) the transaction that was pending will not appear on the first page and will not be cleaned up.

One way to solve this is to add another listener to the safeInfo and compare nonces of the safe and the pending tx. If the safe nonce is larger than the pending tx nonce we can safely clean it up. One potential issue here is that a SUCCESS event won't be emitted anymore in case the safeInfo listener runs before the txHistory listener.

Another way which is implemented in #4057 is to get the tx watcher working for nested safe transactions as we don't need to clean up old pending txs if they are not there in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working major Major bug (to be solved in the next release)
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants