feat(platform-wallet): expose sync_watermark() on PlatformAddressWallet#3723
feat(platform-wallet): expose sync_watermark() on PlatformAddressWallet#3723lklimek wants to merge 1 commit into
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>
📝 WalkthroughWalkthroughTwo accessor methods expose the sync watermark state: a crate-visible getter in 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: "3f6a1fe1eecc567356b61bdf002b34fffc460850e9266b13235263fc2ab96ef8"
)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)
| pub async fn sync_watermark(&self) -> Option<u64> { | ||
| let guard = self.provider.read().await; | ||
| let raw = guard.as_ref().map(|p| p.last_known_recent_block())?; | ||
| (raw > 0).then_some(raw) |
There was a problem hiding this comment.
🟡 Suggestion: sync_watermark()'s public None contract is not covered by a focused test
sync_watermark() intentionally collapses two distinct internal states into the same public result: it returns None both when the provider is uninitialized and when the stored watermark is 0. That behavior is documented here and matches the existing apply_sync_state convention, but this PR adds no test that locks those semantics in. A later refactor of provider initialization or persisted-state replay could change one branch without any test failing, because nothing currently asserts the three externally observable cases: uninitialized provider -> None, initialized provider with watermark 0 -> None, and non-zero watermark -> Some(value).
source: ['codex']
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