MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
12.06k stars 4.93k forks source link

[Bug]: 429 errors from JSON-RPC endpoint shouldn't be treated as tx failures #15361

Open wbt opened 2 years ago

wbt commented 2 years ago

Describe the bug

This is a spin-off issue from https://github.com/tablelandnetwork/js-tableland/issues/83, part 1. More detail can be found there.

In short, when MetaMask gets a 429 response from the JSON-RPC endpoint when it checks on transaction status, it should catch that error and retry after some delay, instead of bubbling up the error to application code for handling as if the transaction failed (e.g. due to user denying transaction signature, EVN revert, etc.). Bubbling this up as an error to code that can't do anything to control the rate of requests, and/or in many cases requiring a user who has even less ability to understand the error or affect the request rate to repeat a transaction (while the original will generally still go through!) leads to pretty bad web3 user experience.

In addition, MetaMask should be throttling its requests appropriately to not hit those 429 errors, but handling that issue should probably be deferred until after verification that it's able to handle the limit error appropriately, because no matter how reasonable a rate MetaMask uses, it might still hit limits (e.g. on an endpoint that aggregates all public requests under one rate limit).

See also #15000.

Steps to reproduce

  1. Add the polygon-mumbai testnet (network name Mumbai, RPC URL https://rpc-mumbai.maticvigil.com, chain id 80001, currency symbol MATIC, block explorer https://mumbai.polygonscan.com)
  2. Trigger a transaction on a contract on this network in code that waits for a response before moving on to another step.
  3. When the 429 error is hit, the code acts as if the transaction fails, e.g. prompting the user to resubmit before being able to proceed to whatever is supposed to follow a successful transaction.
  4. The original transaction still succeeds, in addition to any duplicates that may follow.

Error messages or log output

inpage.js:1 MetaMask - RPC Error: Non-200 status code: '429' details: 'Rate limit exceeded: 40 per 1 second. Check respon…ticvigil.com/ to avoid hitting public ratelimits.', code: -32005,

Version

10.17.0

Build type

No response

Browser

Chrome

Operating system

Windows

Hardware wallet

No response

Additional context

Sometimes, this error is bubbled up as a failure, and sometimes it's just logged but the code can continue on - it's not clear what distinguishes the two cases but it's below application-level code.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

github-actions[bot] commented 1 year ago

This issue was closed because there has been no follow up activity in the last 45 days. If you feel this was closed in error, please reopen and provide evidence on the latest release of the extension. Thank you for your contributions.

wbt commented 1 year ago

I still don't think this should be closed, and don't see any suggestion that the issue was reviewed by the team after the first stale label.