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

feat: query transactions on networks independently #5432

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

Conversation

MuckT
Copy link
Collaborator

@MuckT MuckT commented May 17, 2024

Description

PR for independent network queries; useFetchTransactions in src/transactions/feed/queryHelper.ts has a lot going on to manage the transaction state. In the future, something like @tanstack/react-query might serve us better, but is outside of the scope of this PR.

Video

iOS Before iOS After
Screen.Recording.2024-05-17.at.3.19.30.PM.mov
Screen.Recording.2024-05-17.at.3.14.28.PM.mov

Test plan

prerequisite: add your wallet address to the overrides for base_and_polygon_targeting in Statsig.

  • Tested locally on iOS
  • Tested locally on Android
  • Unit tests updated

Related issues

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

  • Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

Copy link

codecov bot commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.44%. Comparing base (a012a8d) to head (5be15de).
Report is 14 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5432      +/-   ##
==========================================
+ Coverage   86.40%   86.44%   +0.03%     
==========================================
  Files         761      761              
  Lines       31359    31430      +71     
  Branches     5389     5409      +20     
==========================================
+ Hits        27097    27169      +72     
+ Misses       4031     4030       -1     
  Partials      231      231              
Files Coverage Δ
src/transactions/feed/queryHelper.ts 95.78% <100.00%> (+0.67%) ⬆️

... and 13 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a012a8d...5be15de. Read the comment docs.

@MuckT MuckT marked this pull request as ready for review May 30, 2024 20:32
}, {})
for (const promise of promises) {
const { networkId, result } = await promise
yield { [networkId]: result }
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, this would start all the requests in parallel and then serially wait for requests to complete one after the other right? If so, if we have a slow running request at the first one, it would block all subsequent requests right? And if one of them fails, wouldn't all subsequent requests be ignored even if they succeed? Wonder if we can do a non generator based solution where we just invoke some action that can update tx feed for a single n/w

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All the requests are started in parallel within the map and you are correct that a slow request would block the subsequent requests from being processed; however, they will not be removed from the activeRequests state. During the next polling cycle, the slower request will be skipped, if it's listed as having an active request, and the other networks will be processed accordingly.

handleResult currently updates the transaction feed for a single network; however, it's called from two hooks and has some heavy lifting interacting fetchedResult state. Dispatching it from queryTransactionsFeed might be a bit complicated when we have to deal with empty pages and possible refetches.

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.

None yet

2 participants