Conversation
waiting for final design
|
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 LogicIn // 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:
Recommendation:
Other Observations
Summary: |
|
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
2. Scheduled Transfers LimitationOne minor observation:
3. Code Cleanup & Performance
SummaryThe 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). |
There was a problem hiding this comment.
Why do we have unrelated changes here?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Maybe remove it or flag it if we don't need it anymore
There was a problem hiding this comment.
Why do we have unrelated changes here?
dewabisma
left a comment
There was a problem hiding this comment.
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
|
The PR looks solid. The query optimization strategy (splitting the complex 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! |
Optimize subsquid transaction queries
Split the single complex
EventsByAccountsGraphQL 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 viaFuture.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
8 Accounts