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

chore: reduce default wait interval #550

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

joepegler
Copy link
Collaborator

@joepegler joepegler commented Aug 7, 2024

PR-Codex overview

This PR focuses on updating the version of the package, adjusting the default polling interval, and refining error messages in the Bundler class for better clarity.

Detailed summary

  • Added a new version 4.6.4 to CHANGELOG.md and noted a patch change to shorten the default pollInterval.
  • Updated the version in package.json from 4.6.3 to 4.5.6.
  • Introduced a constant POLL_INTERVAL set to 2000 in src/bundler/Bundler.ts.
  • Replaced the hardcoded default polling interval with POLL_INTERVAL.
  • Improved error messages in the Bundler class for clarity during receipt retrieval failures.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

github-actions bot commented Aug 7, 2024

size-limit report 📦

Path Size
core (esm) 55.9 KB (0%)
core (cjs) 61.08 KB (0%)
account (tree-shaking) 53.38 KB (0%)
bundler (tree-shaking) 2.57 KB (0%)
paymaster (tree-shaking) 2.21 KB (0%)
modules (tree-shaking) 40.98 KB (0%)

Copy link
Contributor

@AmanRaj1608 AmanRaj1608 left a comment

Choose a reason for hiding this comment

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

If this is for some specific chain request their used to be a file where we can adjust this interval for specific chains.

@livingrockrises
Copy link
Contributor

Whats the benefit of reducing default polling to 2 ms from 5?

we removed the hard coded value but the constant variable can be changed by env var setting?

@joepegler
Copy link
Collaborator Author

Hey,

So this number we're picking is just a default. Devs can go and set this themselves but we don't really document it widely so they'd have to search it out so in all likelihood they probably don't customise it all that often.

This value we're discussing is, in effect, the 'max amount of unnecessary milliseconds after a tx has been mined that we make a user wait for the receipt'. We should set it to as small a number as possible (without causing 502 timeouts on the backend). It has a big impact on the effective latency that end users notice imo.

I think getting this number right is pretty important, and it might be different from chain to chain based on backend load, block-mining-times per chain etc etc.

@joepegler
Copy link
Collaborator Author

The problem is: 'clients don't know when the backend has mined a tx'. The current solution is for the client to send requests every 'n' milliseconds to check if the backend is done (polling). This PR reduces 'n'. A better (definitely longer term) solution might be to switch the backend to a websocket (which has pub/sub bi-directional flow of information), so the backend can notify the client when txs have been mined, instead of the client having to ask on an interval (which unnecssarily adds to the wait time)

@joepegler
Copy link
Collaborator Author

Also Chirag in this PR I'm not added an env var here. I've just added the constant directly in the file - there's no change in that regard.

@joepegler joepegler closed this Sep 3, 2024
@joepegler joepegler reopened this Sep 12, 2024
@joepegler joepegler force-pushed the feat/reduce_default_wait_interval branch from 421523a to cb217a8 Compare September 26, 2024 15:24
@joepegler joepegler merged commit 71d3925 into develop Oct 11, 2024
1 check passed
@joepegler joepegler deleted the feat/reduce_default_wait_interval branch October 11, 2024 14:08
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