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

Increased time interval of pending tx tracker #4872

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cloudonshore
Copy link

Explanation

References

Changelog

@metamask/package-a

  • : Your change here
  • : Your change here

@metamask/package-b

  • : Your change here
  • : Your change here

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@cloudonshore cloudonshore requested review from a team as code owners October 30, 2024 05:03
@mcmire
Copy link
Contributor

mcmire commented Nov 12, 2024

@cloudonshore Hi Sam, some of us on the Wallet Framework team took a look at the PR and the details you left in your strategy doc. Improving the user experience on chains that aren't Mainnet sounds like a worthwhile goal. However, we still need to mindful of Mainnet, and we're not sure the strategy you've implemented would be ideal across the board.

It seems that in your PR you are increasing the number of times a request is made to fetch the status of a transaction. We suspect that this kind of request (or any request that fetches on-chain data) is likely fairly expensive in comparison to fetching the block number. Not only this, but the transaction status request seems to be made regardless of whether a new block is available. This seems wasteful for slower chains like Mainnet.

Instead of checking transaction statuses more often, wouldn't it be more efficient to check for a new block more often, i.e. reduce the polling interval of the block tracker? It seems that you were concerned about the impact of this change for other parts of the codebase that rely on the block tracker, especially on faster chains, so here are a couple of alternative strategies for your consideration:

  • Reduce the polling interval of the block tracker globally, but throttle block-tracker-based operations other than transactions so that they continue to run every 20 seconds (or whatever speed we would like). This would be closest to what your PR is doing, but would not result in excess requests on Mainnet, and would allow us to control how many requests we are making for more expensive operations such as account balance updates if we like.
  • Reduce the polling interval just while transactions are pending. This would speed up balance updates and other block-tracker-based operations indiscriminately, but the time in which this speedup would occur would be limited. This option would also be much easier to implement than the previous one.

What are your thoughts?

@cloudonshore
Copy link
Author

92% of transactions on metamask are not eth mainnet.
The majority of these networks produce blocks in the range of 1-3 seconds.
So in order to realize the optimal transaction inclusion experience that these networks afford, we would need to decrease the block polling time to at least 1-3 seconds, and then every time there was a new block, ALSO query the transaction receipt. This seems like it would increase the total number of calls (polling both for new blocks, and then checking for transaction status every time we see a new block), as just polling the block alone wouldn't be enough to satisfy the polling requirements to resolve the pending transaction.

In my PR, accelerated transaction polling only occurs when a new pending transaction is added, and it only runs for a short burst after the transaction is added. This should help resolve transactions faster while using the absolute minimum number of calls. @mcmire

@blurpesec
Copy link
Contributor

This seems like it would increase the total number of calls (polling both for new blocks, and then checking for transaction status every time we see a new block), as just polling the block alone wouldn't be enough to satisfy the polling requirements to resolve the pending transaction.

This may also introduce issues with integrations with 3rd party RPC providers as it would vastly increase requests the user is sending to 3rd party RPC clients - leading to more occurrences of hitting rate limits for users.

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.

3 participants