fix(sdk): call on_address_found for seeded balances in incremental-only sync#3468
fix(sdk): call on_address_found for seeded balances in incremental-only sync#3468
Conversation
…ly sync In incremental-only mode, sync_address_balances seeds result.found from the provider's current_balances() but never calls on_address_found for those entries. This means the provider's internal state (e.g., found_balances in WalletAddressProvider) is never populated, making previously discovered addresses invisible to apply_results_to_wallet(). The fix adds on_address_found calls for each seeded entry, matching the behavior of the full tree scan path which already calls this callback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit b56bbc3) |
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "87380f0e4c84914164a2f4f962e5c93ddcc31492b049bc1d681106ae4b86467d"
)Xcode manual integration:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, minimal fix that correctly adds the missing on_address_found callback in the incremental-only seed loop, mirroring the full-scan pattern at lines 154-155 and 196-197. The change is correct and well-commented. The only gap is the lack of a regression test for the exact code path being fixed.
Reviewed commit: b56bbc3
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/platform/address_sync/mod.rs`:
- [SUGGESTION] lines 343-351: No test covers the incremental-only sync path where this bug lived
The existing `test_sync_address_balances` test passes `None` for `last_sync_timestamp`, which always triggers a full tree scan. The bug being fixed only manifests in incremental-only mode, yet no test exercises that path.
To regress-protect this fix, a test would need to:
1. Implement `current_balances()` and `last_sync_height()` on the test provider (the current `TestAddressProvider` returns empty defaults for both, making incremental-only mode unreachable)
2. Pass `Some(recent_timestamp)` to `sync_address_balances` to trigger `needs_full_scan == false`
3. Assert that `provider.found` is populated after sync — i.e., `on_address_found` was called for each seeded balance
Without this, the fix could silently regress in a future refactor since the existing suite only validates the full-scan code path.
| for (index, key, funds) in provider.current_balances() { | ||
| result.found.insert((index, key), funds); | ||
| result.found.insert((index, key.clone()), funds); | ||
| // Notify the provider so it can update its internal state | ||
| // (e.g., populate found_balances, extend gap limit). Without this, | ||
| // addresses discovered during a previous full scan are invisible to | ||
| // the consumer in incremental-only mode because on_address_found | ||
| // is the only path that populates the provider's found set. | ||
| provider.on_address_found(index, &key, funds); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No test covers the incremental-only sync path where this bug lived
The existing test_sync_address_balances test passes None for last_sync_timestamp, which always triggers a full tree scan. The bug being fixed only manifests in incremental-only mode, yet no test exercises that path.
To regress-protect this fix, a test would need to:
- Implement
current_balances()andlast_sync_height()on the test provider (the currentTestAddressProviderreturns empty defaults for both, making incremental-only mode unreachable) - Pass
Some(recent_timestamp)tosync_address_balancesto triggerneeds_full_scan == false - Assert that
provider.foundis populated after sync — i.e.,on_address_foundwas called for each seeded balance
Without this, the fix could silently regress in a future refactor since the existing suite only validates the full-scan code path.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/platform/address_sync/mod.rs`:
- [SUGGESTION] lines 343-351: No test covers the incremental-only sync path where this bug lived
The existing `test_sync_address_balances` test passes `None` for `last_sync_timestamp`, which always triggers a full tree scan. The bug being fixed only manifests in incremental-only mode, yet no test exercises that path.
To regress-protect this fix, a test would need to:
1. Implement `current_balances()` and `last_sync_height()` on the test provider (the current `TestAddressProvider` returns empty defaults for both, making incremental-only mode unreachable)
2. Pass `Some(recent_timestamp)` to `sync_address_balances` to trigger `needs_full_scan == false`
3. Assert that `provider.found` is populated after sync — i.e., `on_address_found` was called for each seeded balance
Without this, the fix could silently regress in a future refactor since the existing suite only validates the full-scan code path.
Summary
Fixes a bug where newly funded platform addresses are never discovered during incremental-only address sync, making them permanently invisible to the wallet.
Root Cause
In
sync_address_balances()(incremental-only mode),result.foundis seeded fromprovider.current_balances()buton_address_foundis not called for those entries. This callback is the only mechanism that populates the provider's internal found set (e.g.,found_balancesinWalletAddressProvider).Failing Scenario
top_up()— state transition confirmed on Platformsync_address_balances()call runs in incremental-only mode (last_sync_timestamp > 0)result.foundis seeded fromcurrent_balances()(the wallet's stored balances from previous sync)on_address_foundis NOT called for seeded entriesfound_balancesstays empty in the providerapply_results_to_wallet()iterates overfound_balances— finds nothing — wallet never updatesWhy Full Scan Works
The full tree scan path (via
process_trunk_resultandprocess_branch_result) callson_address_foundfor every discovered address (lines 155, 197). Incremental-only mode skips the tree scan entirely and only processes deltas, but the delta callbacks (on_address_foundat lines 622, 683) only fire whennew_balance != current_balance— if no delta exists in the incremental window, the address is never reported.Fix
Add
provider.on_address_found(index, &key, funds)calls in the incremental-only seed loop, matching the full scan behavior. This ensures the provider's internal state is populated regardless of sync mode.Related Issue (not fixed here)
In
dash-evo-tool,src/backend_task/wallet/fetch_platform_address_balances.rs:116-120: when incremental sync returns no data,new_sync_timestamp = 0is saved to DB, causing a destructive oscillation between full scan and incremental modes. This is a separate app-layer bug.Test Plan
cargo build -p dash-sdk --all-features🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit