feat(platform-wallet): expose sync_watermark() on PlatformAddressWallet#3723
Conversation
Adds an async accessor for the unified provider's last_known_recent_block watermark, so callers driving sync_balances() directly (e.g. PA-007/PA-007b e2e tests) can observe progress without waiting for the periodic PlatformAddressSyncManager event. Returns Option<u64> with None for both "provider not initialised" and "watermark == 0" — the existing apply_sync_state convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTwo accessor methods expose the sync watermark: a crate-visible getter in PlatformPaymentAddressProvider returns last_known_recent_block, and PlatformAddressWallet::sync_watermark() reads that value async and returns None for missing or zero, otherwise Some(non-zero). ChangesSync Watermark Accessor Chain
Suggested Labels
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs (1)
428-430:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake watermark updates monotonic in
update_sync_state.
last_known_recent_blockis overwritten directly, so out-of-order sync results can move the watermark backward. That breaks the no-regression objective and can report stale progress viasync_watermark().Suggested fix
pub(crate) fn update_sync_state( &mut self, result: &AddressSyncResult<PlatformAddressTag, PlatformP2PKHAddress>, ) { - self.sync_height = result.new_sync_height; - self.sync_timestamp = result.new_sync_timestamp; - self.last_known_recent_block = result.last_known_recent_block; + self.sync_height = self.sync_height.max(result.new_sync_height); + self.sync_timestamp = self.sync_timestamp.max(result.new_sync_timestamp); + self.last_known_recent_block = self + .last_known_recent_block + .max(result.last_known_recent_block); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs` around lines 428 - 430, update_sync_state currently assigns self.last_known_recent_block (and potentially sync_height/sync_timestamp) directly from result, allowing out-of-order results to move the watermark backward; change the updates to be monotonic: set self.last_known_recent_block = max(self.last_known_recent_block, result.last_known_recent_block) and similarly only advance self.sync_height and self.sync_timestamp if result values are greater than the current ones (use std::cmp::max or conditional checks). Update the logic inside update_sync_state so no field is ever decreased by a newer result and keep existing field names (self.sync_height, self.sync_timestamp, self.last_known_recent_block) to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs`:
- Around line 428-430: update_sync_state currently assigns
self.last_known_recent_block (and potentially sync_height/sync_timestamp)
directly from result, allowing out-of-order results to move the watermark
backward; change the updates to be monotonic: set self.last_known_recent_block =
max(self.last_known_recent_block, result.last_known_recent_block) and similarly
only advance self.sync_height and self.sync_timestamp if result values are
greater than the current ones (use std::cmp::max or conditional checks). Update
the logic inside update_sync_state so no field is ever decreased by a newer
result and keep existing field names (self.sync_height, self.sync_timestamp,
self.last_known_recent_block) to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44e6647e-e6df-4ce4-96f2-a50c7c1242ce
📒 Files selected for processing (2)
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs
|
✅ 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: "5d3c59eda54be2be8210a268abb2c3f5c76c7c03725c5fcb56240623baeea68b"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR is a small, read-only API addition that exposes existing sync state without changing wallet mutation or persistence behavior. I did not find a correctness, security, or consensus issue in the implementation. The only review-worthy gap is that the new public Option contract is not pinned down by a focused test.
🟡 1 suggestion(s)
|
Checked CodeRabbit's monotonic-update suggestion. I don't think we should take it in this PR: that behavior was the scope of the superseded #3647 and was intentionally dropped when #3723 narrowed to only expose existing watermark state. In the active |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3723 +/- ##
==========================================
Coverage 87.17% 87.17%
==========================================
Files 2600 2601 +1
Lines 318021 318220 +199
==========================================
+ Hits 277224 277420 +196
- Misses 40797 40800 +3
🚀 New features to boost your workflow:
|
3 participants
Issue being fixed or feature implemented
The PA-007 / PA-007b e2e tests need to observe incremental-sync progress without depending on the periodic
PlatformAddressSyncManagerevent — they callsync_balances()directly and need to verify the watermark advanced. No public accessor existed.This PR is the minimal extracted slice from the closed #3647 (which also bundled a controversial
update_sync_statemonotonic-merge change — that is being abandoned, see #3647 discussion with @QuantumExplorer and @thepastaclaw).What was done?
pub(crate) fn last_known_recent_block(&self) -> u64onPlatformPaymentAddressProvider— crate-visible mirror of the field so wallet-level helpers can read it without going through theAddressProvidertrait.pub async fn sync_watermark(&self) -> Option<u64>onPlatformAddressWallet.Nonefor "provider not initialised" or "watermark == 0", matching the existingapply_sync_stateconvention.No behavioural change to existing call sites — pure additive surface.
How Has This Been Tested?
cargo check -p platform-walletcleancargo clippy -p platform-wallet --all-targets -- -D warningscleancargo test -p platform-wallet --lib→ 124 passed, 0 failedBreaking Changes
None.
Checklist:
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit