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

fix: handle etherscan rate limit errors #7209

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

matthewwalsh0
Copy link
Member

@matthewwalsh0 matthewwalsh0 commented Sep 13, 2023

Description

The mobile client uses the core TransactionController which supports the use of an API key when querying Etherscan for incoming or updated transactions.

The extension, using its TransactionController, only queries Etherscan for normal transactions using a single callout.

The mobile client, using the core TransactionController, queries Etherscan for both normal transactions and token transfer transactions related to an account. This requires 2 callouts and therefore consistently results in a rate limit error on one of the callouts if no API key is provided.

Previously, if either request failed due to a rate limit error, the update would be skipped, and no transactions would be added. This PR instead processes whatever transactions are found, regardless of which specific callout returned a rate limit error.

Since both callouts are performed simultaneously, it is currently random chance which callout results in the rate limit error, meaning both normal and token transfer transactions are still supported, but either could experience sporadic delays.

Additional changes include:

  • Add back the background timer to ensure incoming transactions continue to be polled, even when the app is in background mode.
  • Disable the startBlock argument in Etherscan requests to match the previous behaviour where a bug meant this was never being utilised.

Recordings

Transaction History After Import

import_history.mov

New Incoming Transaction & Notification

new_incoming.mov

Issue

Fixes #1249, #1250, #1251

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Add background timer to update incoming transactions.
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@matthewwalsh0 matthewwalsh0 added the team-confirmations-system DEPRECATED: please use "team-confirmations" label instead label Sep 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01% ⚠️

Comparison is base (57dbc0e) 33.15% compared to head (100f485) 33.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7209      +/-   ##
==========================================
- Coverage   33.15%   33.15%   -0.01%     
==========================================
  Files        1005     1005              
  Lines       32656    32659       +3     
  Branches     8398     8398              
==========================================
  Hits        10827    10827              
- Misses      21829    21832       +3     
Files Changed Coverage Δ
app/components/Nav/Main/index.js 6.20% <0.00%> (-0.14%) ⬇️
app/core/AppConstants.ts 85.71% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matthewwalsh0 matthewwalsh0 marked this pull request as ready for review September 13, 2023 10:33
@matthewwalsh0 matthewwalsh0 requested a review from a team as a code owner September 13, 2023 10:33
vinistevam
vinistevam previously approved these changes Sep 13, 2023
OGPoyraz
OGPoyraz previously approved these changes Sep 13, 2023
Ignore duplicate incoming transactions.
@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@sleepytanya
Copy link
Contributor

sleepytanya commented Sep 13, 2023

All incoming transaction notifications work as expected, push notification are also seen when the app is on the background.
iOS incoming transactions notifications (1- normal send / 2 - token transfer):

eth.mov
token.mov

iOS incoming transactions notifications in the background mode (1 - normal send / 2 - token transfer):

bckgr_send.mov

https://github.com/MetaMask/metamask-mobile/assets/104780023/240f14e9-b9ab-4bdb-96ae-aa3d04734283
Transaction history after import (iOS):

history_ios.mov

Android incoming ETH notification:

eth.mov

Android incoming token notification:

tst.mov

Android incoming ETH notification in background mode:

send_eth_bkgr.mov

Android incoming TST notification in background mode:

tst_bkgr.mov
Screen.Recording.2023-09-14.at.11.10.40.AM.mov

Transaction history after import on Android:

Copy link
Contributor

@sleepytanya sleepytanya left a comment

Choose a reason for hiding this comment

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

LGTM

@matthewwalsh0 matthewwalsh0 added the QA Passed A successful QA run through has been done label Sep 13, 2023
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@matthewwalsh0 matthewwalsh0 merged commit 4bfb9bd into main Sep 13, 2023
25 checks passed
@matthewwalsh0 matthewwalsh0 deleted the fix/handle-etherscan-rate-limit-errors branch September 13, 2023 20:37
@github-actions github-actions bot locked and limited conversation to collaborators Sep 13, 2023
@metamaskbot metamaskbot added the release-7.8.0 Issue or pull request that will be included in release 7.8.0 label Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.8.0 Issue or pull request that will be included in release 7.8.0 team-confirmations-system DEPRECATED: please use "team-confirmations" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants