Skip to content

Comments

Speed up queries#399

Merged
n13 merged 15 commits intomainfrom
speed-up-queries
Feb 21, 2026
Merged

Speed up queries#399
n13 merged 15 commits intomainfrom
speed-up-queries

Conversation

@n13
Copy link
Collaborator

@n13 n13 commented Feb 20, 2026

Optimize subsquid transaction queries

Split the single complex EventsByAccounts GraphQL query (which used a 3-branch OR across transfers, reversible transfers, and miner rewards) into 3 simple parallel queries. The combined OR was causing the subsquid server to take 6-11s; individual queries each return in ~1s and run concurrently via Future.wait. Scheduled transfers also now fetch in parallel with the other queries — so the total load is 4 concurrent requests instead of 2 sequential ones.

Also: Removed unused method and gql query

Results

1 Account

Metric Before After
Total time 6,000 ms 1,200 ms
Speedup 500%

8 Accounts

Metric Before After
Run 3 16,038 ms 1,786 ms
Speedup 900%

@n13
Copy link
Collaborator Author

n13 commented Feb 21, 2026

The PR implements the parallel query fetching as described, which should indeed significantly improve performance. However, there is a critical logic flaw in the pagination implementation for the merged streams that will cause data to be missed as the user scrolls.

Critical Issue: Pagination Logic

In _fetchOtherTransfers, you are passing the global offset to each of the three parallel queries:

// quantus_sdk/lib/src/services/chain_history_service.dart

final results = await Future.wait([
  _fetchSingleType(..., offset: offset, ...), // Query A
  _fetchSingleType(..., offset: offset, ...), // Query B
  _fetchSingleType(..., offset: offset, ...), // Query C
]);

Why this is broken:
When you merge and sort the results from the previous page (offset 0), you might take, for example, 8 items from "Transfers" and 2 from "Rewards".

  • For the next page (offset 10), you pass offset: 10 to all queries.
  • The "Transfers" query will skip its top 10 items (correct-ish, but we only showed 8).
  • The "Rewards" query will skip its top 10 items (incorrect, we only showed 2).
  • Result: You will permanently skip 8 items from the "Rewards" list that should have been shown on the second page.

Recommendation:
Since the underlying GraphQL API seems to support only simple offsets and not a complex cursor state for multiple streams, you cannot easily fix this without changing the API or state management.

  • Ideal Fix: The backend should handle the merging of these streams so pagination is consistent.
  • Client-side Mitigation: You would need to track the offset for each stream independently (e.g., transfersOffset, rewardsOffset, reversibleOffset) based on how many items from each stream were actually consumed/displayed in the previous page. However, fetchAllTransactionTypes is stateless, making this difficult.

Other Observations

  1. Biometrics Refactor: The simplification of LocalAuthService (removing configurable timeouts and PIN management) and the deletion of AutoLockScreen / ChangePinScreen looks consistent and clean.
  2. Code Cleanup: The removal of getTransactionsInBlock is good if it's indeed unused. Ensure no other parts of the app (outside the diff scope) rely on it.
  3. Timing Logs: The added Stopwatch and print statements are helpful for debugging this performance change.

Summary:
The speedup is great, but the pagination logic needs to be addressed to prevent data loss in the transaction history feed.

@n13
Copy link
Collaborator Author

n13 commented Feb 21, 2026

The PR looks solid and the critical pagination issue identified in the comments has been addressed.

Here is my review of the current state of the code:

1. Pagination Logic Fix (Verified)

The critical flaw where a single global offset was being passed to all parallel queries has been fixed.

  • chain_history_service.dart: The fetchAllTransactionTypes and _fetchOtherTransfers methods now correctly accept and return separate offsets for each stream (transfersOffset, reversibleOffset, rewardsOffset).
  • unified_pagination_controller.dart: The controller now tracks these three separate offsets in the PaginationState and passes them correctly to the service for subsequent pages.
  • Merging Strategy: The logic of fetching limit items from each stream, merging, sorting by timestamp, taking the top limit, and then calculating the nextOffset based on how many items of each specific type were actually used is correct. This ensures that unused items from a previous fetch (which were discarded because they weren't in the top limit) will be correctly re-fetched in the next page.

2. Scheduled Transfers Limitation

One minor observation: fetchScheduledTransfers is called without an offset or limit (using default limit=10, offset=0).

  • This means you will only ever see the top 10 scheduled transfers.
  • If a user has more than 10 scheduled transfers, they will not be able to see the rest, and pagination does not apply to this stream.
  • Given that scheduled transfers are usually few, this is likely an acceptable trade-off for simplicity, but it's worth noting.

3. Code Cleanup & Performance

  • Unused Code: getTransactionsInBlock has been correctly removed and is no longer used in the codebase.
  • Performance: The use of Future.wait to run the 3 (or 4) queries in parallel is a major improvement and the logic handles the concurrency correctly.
  • Debug: The Stopwatch and printTiming additions are helpful for verifying the speedup and are correctly gated behind a debug flag (or at least easily removable).

Summary

The PR is in good shape. The logic is sound, the critical bug is fixed, and the performance improvement is significant.

Verdict: Approve (with a minor note about the 10-item limit on scheduled transfers).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have unrelated changes here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have unrelated changes here?

final accountAsync = ref.watch(activeAccountProvider);
final balanceAsync = ref.watch(balanceProvider);
final txAsync = ref.watch(activeAccountTransactionsProvider);
if (txAsync.isLoading && _txLoadingStopwatch == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe remove it or flag it if we don't need it anymore

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have unrelated changes here?

Copy link
Collaborator

@dewabisma dewabisma left a comment

Choose a reason for hiding this comment

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

The changes for the graphql query seems good. Other thing about this PR is that it has unrelated changes which give noise to the process of review. Let's just create new PR next time so we don't mix the PR intention.

Also, for the stopwatch code, do we still need it? if so maybe just flag profiling code or remove it if not needed

@n13
Copy link
Collaborator Author

n13 commented Feb 21, 2026

The PR looks solid. The query optimization strategy (splitting the complex OR query into parallel simple queries and merging results) is a standard and effective pattern for GraphQL performance, especially with Subsquid/Subquery. The pagination logic using individual offsets for each stream is correctly implemented to ensure no data is skipped or duplicated across pages.

I also noticed the removal of the granular authentication settings (PIN, Auto-lock timer, etc.), which simplifies the app significantly.

Here is the review:

## Review: Speed up queries

Great work on this optimization! The strategy of splitting the complex `OR` query into parallel requests is the right approach here and should yield significant performance gains as shown in your benchmarks. The pagination logic for merging these streams is also implemented correctly.

### Key Highlights
- **Query Splitting**: Breaking down `EventsByAccounts` into `Transfers`, `Reversible`, and `Rewards` queries avoids the expensive `OR` operations in the DB layer.
- **Parallel Execution**: Using `Future.wait` to fetch all data streams concurrently is excellent.
- **Pagination Logic**: The "merge and trim" strategy with individual offsets (`transfersOffset`, `reversibleOffset`, etc.) is the correct way to handle pagination across multiple sorted streams. By advancing the offset only by the number of *used* items from each stream, you ensure that unused items from the current fetch are correctly candidates for the next page.

### Observations
- **Auth Settings Removal**: I see a significant cleanup of the Authentication settings (removal of `AuthenticationSettingsScreen`, `ChangePinScreen`, `AutoLockScreen`). This simplifies the `LocalAuthService` and `SettingsScreen` considerably. Just noting this to ensure it aligns with the product requirements to remove these user-configurable options (like custom auto-lock timers).
- **Debug Script**: The addition of `scripts/debug_subsquid.sh` is very helpful for verifying the performance improvements.

### Code Quality
- The refactoring in `chain_history_service.dart` to use `_fetchSingleType` keeps the code DRY and readable despite the added complexity of managing multiple queries.
- The `PaginationState` updates clearly track the state needed for this new pagination model.

Everything looks good to merge!

@n13 n13 merged commit 9dba9a2 into main Feb 21, 2026
1 check passed
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.

2 participants