feat(rs-sdk): implement incremental address balance synchronization#3152
Merged
QuantumExplorer merged 6 commits intov3.1-devfrom Feb 24, 2026
Merged
feat(rs-sdk): implement incremental address balance synchronization#3152QuantumExplorer merged 6 commits intov3.1-devfrom
QuantumExplorer merged 6 commits intov3.1-devfrom
Conversation
✅ gRPC Query Coverage Report |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an incremental catch‑up path to address balance synchronization. A new optional last_sync_timestamp selects between full tree scan and incremental-only catch‑up. Public APIs, provider trait, FFI, result types, metrics, and internal flows/helpers were extended to support compacted/recent batches and new sync timestamps/heights. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Sdk
participant Provider
participant Backend
Caller->>Sdk: sync_address_balances(provider, config, last_sync_timestamp)
alt no timestamp OR elapsed > config.full_rescan_after_time_s
Note over Sdk: Full tree scan + incremental catch‑up
Sdk->>Backend: trunk_query (snapshot)
Backend-->>Sdk: trunk_result (checkpoint_height, block_time)
Sdk->>Provider: on_address_found / on_address_absent (apply trunk)
loop branch queries
Sdk->>Backend: branch_query(range)
Backend-->>Sdk: branch_result
Sdk->>Provider: on_address_found / on_address_absent (apply branch)
end
Sdk->>Backend: compacted_changes_query(since checkpoint)
Backend-->>Sdk: compacted_changes
Sdk->>Backend: recent_changes_query(range)
Backend-->>Sdk: recent_changes
Sdk->>Provider: apply incremental updates
else within threshold
Note over Sdk,Provider: Incremental‑only catch‑up
Provider->>Provider: current_balances() (base)
Sdk->>Backend: compacted_changes_query(since provider.last_sync_height())
Backend-->>Sdk: compacted_changes
Sdk->>Backend: recent_changes_query(range)
Backend-->>Sdk: recent_changes
Sdk->>Provider: apply incremental updates
end
Sdk-->>Caller: AddressSyncResult { checkpoint_height, new_sync_height, new_sync_timestamp, metrics }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/address_sync/mod.rs (1)
361-391: Tworesult.found.getlookups for the same key — can be collapsed into one.Both
current_balanceandnonceare extracted from the same map entry with separate.getcalls. The same pattern repeats in the Phase 2 loop (lines 448–474).♻️ Proposed refactor (same pattern for Phase 2)
- let current_balance = result - .found - .get(&(*index, key.clone())) - .map(|f| f.balance) - .unwrap_or(0); let new_balance = match credit_op { // ... }; if new_balance != current_balance { - let nonce = result - .found - .get(&(*index, key.clone())) - .map(|f| f.nonce) - .unwrap_or(0); + let (current_balance, nonce) = result + .found + .get(&(*index, key.clone())) + .map(|f| (f.balance, f.nonce)) + .unwrap_or((0, 0)); + + let new_balance = match credit_op { + // ... + }; + + if new_balance != current_balance { let funds = AddressFunds { nonce, balance: new_balance, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/address_sync/mod.rs` around lines 361 - 391, The code does two separate result.found.get lookups for the same key to obtain balance and nonce (used in current_balance and later nonce), which is redundant; change the logic in the BlockAwareCreditOperation handling (around current_balance/new_balance) to perform a single let entry = result.found.get(&(*index, key.clone())) and extract both balance and nonce from that Option (e.g., match or if let), compute new_balance, and then use the extracted nonce when constructing AddressFunds and calling provider.on_address_found; apply the identical refactor to the Phase 2 loop (the similar block at lines referenced in the review) so both balance and nonce are read from one map lookup and avoid duplicate get calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk/src/platform/address_sync/mod.rs`:
- Around line 393-402: Replace the unchecked additions on u64 block heights with
saturating_add to match existing safety patterns: where the code sets
current_height = entry.end_block_height + 1 and later uses entry.block_height +
1, change those to current_height = entry.end_block_height.saturating_add(1) and
similar for entry.block_height.saturating_add(1) so additions cannot overflow;
update the occurrences around the current_height assignment and the later phase
that references entry.block_height to use saturating_add(1).
- Around line 369-376: The intermediate sum using Iterator::sum for total_to_add
can overflow before applying saturating_add; replace the sum with a saturating
fold so intermediate accumulation uses saturating_add as well. In the
BlockAwareCreditOperation::AddToCreditsOperations arm (variables total_to_add,
current_balance, current_height), compute total_to_add by folding over
operations starting from 0u64 and using saturating_add for each credits value
(ensuring the accumulator and credits are u64), then use
current_balance.saturating_add(total_to_add) as before.
In `@packages/rs-sdk/src/platform/address_sync/provider.rs`:
- Around line 105-116: The intra-doc link [AddressSyncConfig::last_sync_height]
in the doc comment for the method current_balances is unresolved because
AddressSyncConfig is not in scope; add AddressSyncConfig to the module imports
or fully qualify the path in the doc comment to fix the warning. Locate the
current_balances method in provider.rs and either add a use for
AddressSyncConfig (the same module that defines AddressSyncConfig) at the top of
the file or change the doc link to the full path (e.g.,
crate::platform::address_sync::AddressSyncConfig::last_sync_height) so the
rustdoc link resolves.
---
Nitpick comments:
In `@packages/rs-sdk/src/platform/address_sync/mod.rs`:
- Around line 361-391: The code does two separate result.found.get lookups for
the same key to obtain balance and nonce (used in current_balance and later
nonce), which is redundant; change the logic in the BlockAwareCreditOperation
handling (around current_balance/new_balance) to perform a single let entry =
result.found.get(&(*index, key.clone())) and extract both balance and nonce from
that Option (e.g., match or if let), compute new_balance, and then use the
extracted nonce when constructing AddressFunds and calling
provider.on_address_found; apply the identical refactor to the Phase 2 loop (the
similar block at lines referenced in the review) so both balance and nonce are
read from one map lookup and avoid duplicate get calls.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-sdk/src/platform/address_sync/provider.rspackages/rs-sdk/src/platform/address_sync/types.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-sdk-ffi/src/address_sync/types.rs (1)
55-76:⚠️ Potential issue | 🟡 MinorFFI metrics struct is missing
compacted_queriesandrecent_queries; result struct is missingnew_sync_height.The platform-level
AddressSyncMetricsnow includescompacted_queriesandrecent_queries, andAddressSyncResultincludesnew_sync_height. These are absent from the FFI structs, so FFI callers (e.g., Swift/iOS) have no way to retrieve the new sync height needed for subsequent incremental syncs, nor can they inspect the new metric counters.Since the FFI always passes
Noneforlast_sync_timestamptoday, the caller can't trigger incremental mode yet—butnew_sync_heightis still set by the sync and would be needed once the FFI surface is extended. Consider adding these fields now, or at least leaving a TODO.Proposed additions
For
DashSDKAddressSyncMetrics:/// Number of iterations (0 = trunk only, 1+ = trunk plus branch rounds) pub iterations: u32, + + /// Number of compacted incremental queries + pub compacted_queries: u32, + + /// Number of recent incremental queries + pub recent_queries: u32, }For
DashSDKAddressSyncResult:/// Metrics about the sync process pub metrics: DashSDKAddressSyncMetrics, + + /// The new sync height to pass back on the next call. + /// Used for incremental sync on subsequent calls. + pub new_sync_height: u64, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/types.rs` around lines 55 - 76, The FFI structs are missing new fields: add compacted_queries and recent_queries to DashSDKAddressSyncMetrics and add new_sync_height to DashSDKAddressSyncResult (or add a TODO if you intentionally defer); ensure you update the struct definitions (DashSDKAddressSyncMetrics and AddressSyncResult), keep #[repr(C)] and the derives, choose the same numeric types as the platform-level types (e.g., u32/u64 to match AddressSyncMetrics and AddressSyncResult), and update Default/Copy/Clone implementations if needed so the FFI surface can expose the new counters and the new_sync_height for incremental syncs.
♻️ Duplicate comments (2)
packages/rs-sdk/src/platform/address_sync/mod.rs (2)
383-392:Iterator::sum()can panic on overflow in debug mode — use saturating fold.The intermediate
.sum()on line 390 is not protected by the outersaturating_add.Iterator::sum::<u64>()panics in debug builds on overflow and silently wraps in release. Replace with a saturating fold for consistency with thesaturating_addon line 391.Proposed fix
let total_to_add: u64 = operations .iter() .filter(|(height, _)| **height >= current_height) .map(|(_, credits)| *credits) - .sum(); + .fold(0u64, |acc, c| acc.saturating_add(c)); current_balance.saturating_add(total_to_add)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/address_sync/mod.rs` around lines 383 - 392, In the BlockAwareCreditOperation::AddToCreditsOperations arm replace the intermediate Iterator::sum() (the total_to_add calculation) with a saturating fold so it cannot overflow in debug builds; keep the filter(|(height, _)| **height >= current_height) and map to the credits but fold with acc.saturating_add(credits) (starting from 0u64) before applying current_balance.saturating_add(total_to_add) to ensure consistent, non-panicking overflow behavior.
411-413: Bare+ 1onu64block heights — usesaturating_add(1)for consistency.Lines 411-412 (
entry.end_block_height + 1) and 493-494 (entry.block_height + 1) use unchecked addition. Whileu64::MAXis unreachable in practice, the rest of this file uses saturating arithmetic. These should match.Proposed fix (line 411-412)
- if entry.end_block_height + 1 > current_height { - current_height = entry.end_block_height + 1; + if entry.end_block_height.saturating_add(1) > current_height { + current_height = entry.end_block_height.saturating_add(1); }Proposed fix (line 493-494)
- if entry.block_height + 1 > current_height { - current_height = entry.block_height + 1; + if entry.block_height.saturating_add(1) > current_height { + current_height = entry.block_height.saturating_add(1); }Also applies to: 493-495
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/address_sync/mod.rs` around lines 411 - 413, Replace unchecked u64 additions with saturating arithmetic: where the code uses entry.end_block_height + 1 and entry.block_height + 1 in this module (within the logic that updates current_height and when computing next heights), change those expressions to entry.end_block_height.saturating_add(1) and entry.block_height.saturating_add(1) respectively so they match the rest of the file's saturating arithmetic and avoid potential overflow.
🧹 Nitpick comments (2)
packages/rs-sdk/src/platform/address_sync/mod.rs (1)
319-324: Minor: redundant clone in address key lookup construction.Each entry clones
keytwice — once for the HashMap key and once inside the value tuple. Sincekey_to_indexalready owns the keys, you could avoid the extra clone:Proposed optimization
let address_key_lookup: HashMap<Vec<u8>, (AddressIndex, AddressKey)> = key_to_index .iter() - .map(|(key, &index)| (key.clone(), (index, key.clone()))) + .map(|(key, &index)| { + let k = key.clone(); + (k.clone(), (index, k)) + }) .collect();This is cosmetic — the data set is small. No action needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/address_sync/mod.rs` around lines 319 - 324, The map construction duplicates clones of each key; change the iteration to consume ownership of the entries (use into_iter on key_to_index) so you only clone the key once and reuse the moved key in the value tuple (adjusting the closure from |(key, &index)| to something like |(key, index)| and produce (key.clone(), (index, key))). Update the code around address_key_lookup, key_to_index, AddressIndex and AddressKey accordingly so the HashMap<Vec<u8>, (AddressIndex, AddressKey)> is built with a single clone per entry.packages/rs-sdk-ffi/src/address_sync/mod.rs (1)
366-372: Test should assert the newfull_rescan_after_time_sdefault.The
test_config_defaulttest verifies three of the four config fields but omits the newly addedfull_rescan_after_time_s.Proposed fix
fn test_config_default() { let config = DashSDKAddressSyncConfig::default(); assert_eq!(config.min_privacy_count, 32); assert_eq!(config.max_concurrent_requests, 10); assert_eq!(config.max_iterations, 50); + assert_eq!(config.full_rescan_after_time_s, 7 * 24 * 60 * 60); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/mod.rs` around lines 366 - 372, The test test_config_default currently omits the new DashSDKAddressSyncConfig field full_rescan_after_time_s; update the test to assert that config.full_rescan_after_time_s equals the configured default by adding an assertion (e.g. assert_eq!(config.full_rescan_after_time_s, <expected_default>)); use the same literal value used in DashSDKAddressSyncConfig::default() (or replace <expected_default> with that literal) so the test reflects the struct's actual default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk/src/platform/address_sync/mod.rs`:
- Around line 373-407: The current behavior where BlockAwareCreditOperation
carries no nonce is intentional; for incremental-only sync the nonce comes from
provider.current_balances() and thus freshness depends on the AddressProvider
implementation—add an explicit comment near the incremental seeding logic
(reference symbols: BlockAwareCreditOperation, AddressFunds::try_from,
provider.current_balances(), last_sync_timestamp) stating this contract and
requirement that any AddressProvider must maintain up-to-date nonces for
incremental-only mode, and update docs/tests to validate providers preserve
nonce state between syncs.
In `@packages/rs-sdk/src/platform/address_sync/provider.rs`:
- Around line 117-122: The doc link to AddressSyncResult::new_sync_height is
unresolved because AddressSyncResult is not imported into this module; fix by
either importing AddressSyncResult into the file or using a fully-qualified
intra-doc path. Specifically, add a use for AddressSyncResult (or change the doc
link to crate::platform::address_sync::AddressSyncResult::new_sync_height) so
the rustdoc link resolves; update the module where AddressFunds, AddressIndex,
AddressKey are declared to also reference AddressSyncResult.
In `@packages/rs-sdk/src/platform/address_sync/types.rs`:
- Around line 110-118: The doc for the field new_sync_height is misleading: it
says the value should be passed back via last_sync_timestamp (which is a Unix
timestamp), conflating height and time; update the comment for new_sync_height
to state that callers should store this block height for the provider's
last_sync_height() on the next call, and separately store the current wall-clock
time as last_sync_timestamp when invoking the next sync; reference the
new_sync_height field and the last_sync_timestamp parameter and the provider
method last_sync_height() so readers know which values are stored and passed
back.
---
Outside diff comments:
In `@packages/rs-sdk-ffi/src/address_sync/types.rs`:
- Around line 55-76: The FFI structs are missing new fields: add
compacted_queries and recent_queries to DashSDKAddressSyncMetrics and add
new_sync_height to DashSDKAddressSyncResult (or add a TODO if you intentionally
defer); ensure you update the struct definitions (DashSDKAddressSyncMetrics and
AddressSyncResult), keep #[repr(C)] and the derives, choose the same numeric
types as the platform-level types (e.g., u32/u64 to match AddressSyncMetrics and
AddressSyncResult), and update Default/Copy/Clone implementations if needed so
the FFI surface can expose the new counters and the new_sync_height for
incremental syncs.
---
Duplicate comments:
In `@packages/rs-sdk/src/platform/address_sync/mod.rs`:
- Around line 383-392: In the BlockAwareCreditOperation::AddToCreditsOperations
arm replace the intermediate Iterator::sum() (the total_to_add calculation) with
a saturating fold so it cannot overflow in debug builds; keep the
filter(|(height, _)| **height >= current_height) and map to the credits but fold
with acc.saturating_add(credits) (starting from 0u64) before applying
current_balance.saturating_add(total_to_add) to ensure consistent, non-panicking
overflow behavior.
- Around line 411-413: Replace unchecked u64 additions with saturating
arithmetic: where the code uses entry.end_block_height + 1 and
entry.block_height + 1 in this module (within the logic that updates
current_height and when computing next heights), change those expressions to
entry.end_block_height.saturating_add(1) and
entry.block_height.saturating_add(1) respectively so they match the rest of the
file's saturating arithmetic and avoid potential overflow.
---
Nitpick comments:
In `@packages/rs-sdk-ffi/src/address_sync/mod.rs`:
- Around line 366-372: The test test_config_default currently omits the new
DashSDKAddressSyncConfig field full_rescan_after_time_s; update the test to
assert that config.full_rescan_after_time_s equals the configured default by
adding an assertion (e.g. assert_eq!(config.full_rescan_after_time_s,
<expected_default>)); use the same literal value used in
DashSDKAddressSyncConfig::default() (or replace <expected_default> with that
literal) so the test reflects the struct's actual default.
In `@packages/rs-sdk/src/platform/address_sync/mod.rs`:
- Around line 319-324: The map construction duplicates clones of each key;
change the iteration to consume ownership of the entries (use into_iter on
key_to_index) so you only clone the key once and reuse the moved key in the
value tuple (adjusting the closure from |(key, &index)| to something like |(key,
index)| and produce (key.clone(), (index, key))). Update the code around
address_key_lookup, key_to_index, AddressIndex and AddressKey accordingly so the
HashMap<Vec<u8>, (AddressIndex, AddressKey)> is built with a single clone per
entry.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/rs-sdk-ffi/src/address_sync/mod.rspackages/rs-sdk-ffi/src/address_sync/types.rspackages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-sdk/src/platform/address_sync/provider.rspackages/rs-sdk/src/platform/address_sync/types.rspackages/rs-sdk/tests/fetch/address_sync.rs
Contributor
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-sdk-ffi/src/address_sync/mod.rs (1)
396-401:⚠️ Potential issue | 🟡 Minor
test_config_defaultdoesn't cover the newfull_rescan_after_time_sfield.The test validates three existing fields but omits an assertion for
full_rescan_after_time_s, which was added by this PR. A regression in its default value (expected7 * 24 * 60 * 60 = 604800) would go undetected.🐛 Proposed fix
assert_eq!(config.max_iterations, 50); + assert_eq!(config.full_rescan_after_time_s, 7 * 24 * 60 * 60); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/mod.rs` around lines 396 - 401, The test test_config_default in DashSDKAddressSyncConfig currently asserts three default fields but omits the newly added full_rescan_after_time_s; update test_config_default to assert that config.full_rescan_after_time_s equals 7 * 24 * 60 * 60 (604800) so the default for DashSDKAddressSyncConfig.full_rescan_after_time_s is verified and future regressions are caught.
🧹 Nitpick comments (2)
packages/rs-sdk-ffi/src/address_sync/mod.rs (2)
60-77: Duplicate config/timestamp conversion — extract into helpers.The config-conversion block and the
last_sync_tssentinel conversion (0 → None, non-zero →Some) are identically duplicated between both FFI functions. If a new field is added toAddressSyncConfigor the sentinel semantics change, both copies will need updating.♻️ Suggested refactor
+fn build_rust_config(config: *const DashSDKAddressSyncConfig) -> Option<AddressSyncConfig> { + if config.is_null() { + return None; + } + unsafe { + Some(AddressSyncConfig { + min_privacy_count: (*config).min_privacy_count, + max_concurrent_requests: (*config).max_concurrent_requests as usize, + max_iterations: (*config).max_iterations as usize, + full_rescan_after_time_s: (*config).full_rescan_after_time_s, + request_settings: RequestSettings::default(), + }) + } +} + +fn to_last_sync_ts(last_sync_timestamp: u64) -> Option<u64> { + if last_sync_timestamp == 0 { None } else { Some(last_sync_timestamp) } +}Then replace both identical blocks in
dash_sdk_sync_address_balancesanddash_sdk_sync_address_balances_with_result:- // Convert config - let rust_config = if config.is_null() { - None - } else { - Some(AddressSyncConfig { - min_privacy_count: (*config).min_privacy_count, - max_concurrent_requests: (*config).max_concurrent_requests as usize, - max_iterations: (*config).max_iterations as usize, - full_rescan_after_time_s: (*config).full_rescan_after_time_s, - request_settings: RequestSettings::default(), - }) - }; - - // Convert timestamp: 0 means no previous sync (full scan) - let last_sync_ts = if last_sync_timestamp == 0 { - None - } else { - Some(last_sync_timestamp) - }; + let rust_config = build_rust_config(config); + let last_sync_ts = to_last_sync_ts(last_sync_timestamp);Also applies to: 154-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/mod.rs` around lines 60 - 77, Extract the duplicated config and timestamp conversion into reusable helper functions: create a function (e.g., build_address_sync_config) that takes *config pointer and returns Option<AddressSyncConfig> (setting min_privacy_count, max_concurrent_requests as usize, max_iterations as usize, full_rescan_after_time_s and request_settings: RequestSettings::default()), and another helper (e.g., convert_last_sync_ts) that maps last_sync_timestamp: u64 with the sentinel 0 → None, non-zero → Some(last_sync_timestamp); then replace the duplicated blocks in dash_sdk_sync_address_balances and dash_sdk_sync_address_balances_with_result to call these helpers (use the same helper names or similar) so future changes to AddressSyncConfig fields or sentinel logic are made in one place.
173-182: Missingdebug!log forlast_sync_tsin thewith_resultvariant.
dash_sdk_sync_address_balanceslogs the resolved config andlast_sync_tsbefore executing the sync, butdash_sdk_sync_address_balances_with_resulthas no equivalent. This makes it harder to distinguish incremental from full-scan runs when reading logs from the_with_resultpath.♻️ Proposed addition
// Create the callback-based provider wrapper let mut callback_provider = CallbackAddressProvider::new(provider_ffi); + + debug!( + "dash_sdk_sync_address_balances_with_result: running sync with config: {:?}, last_sync_timestamp: {:?}", + rust_config, last_sync_ts + ); // Execute the sync🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/mod.rs` around lines 173 - 182, Add a debug log for last_sync_ts in the dash_sdk_sync_address_balances_with_result path before executing the sync: locate the with_result variant where CallbackAddressProvider::new(provider_ffi) is created and wrapper.runtime.block_on(...) calls wrapper.sdk.sync_address_balances(..., rust_config, last_sync_ts). Insert a debug! (matching the existing dash_sdk_sync_address_balances variant) that logs the resolved rust_config (or its identifying fields) and last_sync_ts so incremental vs full-scan runs are distinguishable in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/rs-sdk-ffi/src/address_sync/mod.rs`:
- Around line 396-401: The test test_config_default in DashSDKAddressSyncConfig
currently asserts three default fields but omits the newly added
full_rescan_after_time_s; update test_config_default to assert that
config.full_rescan_after_time_s equals 7 * 24 * 60 * 60 (604800) so the default
for DashSDKAddressSyncConfig.full_rescan_after_time_s is verified and future
regressions are caught.
---
Nitpick comments:
In `@packages/rs-sdk-ffi/src/address_sync/mod.rs`:
- Around line 60-77: Extract the duplicated config and timestamp conversion into
reusable helper functions: create a function (e.g., build_address_sync_config)
that takes *config pointer and returns Option<AddressSyncConfig> (setting
min_privacy_count, max_concurrent_requests as usize, max_iterations as usize,
full_rescan_after_time_s and request_settings: RequestSettings::default()), and
another helper (e.g., convert_last_sync_ts) that maps last_sync_timestamp: u64
with the sentinel 0 → None, non-zero → Some(last_sync_timestamp); then replace
the duplicated blocks in dash_sdk_sync_address_balances and
dash_sdk_sync_address_balances_with_result to call these helpers (use the same
helper names or similar) so future changes to AddressSyncConfig fields or
sentinel logic are made in one place.
- Around line 173-182: Add a debug log for last_sync_ts in the
dash_sdk_sync_address_balances_with_result path before executing the sync:
locate the with_result variant where CallbackAddressProvider::new(provider_ffi)
is created and wrapper.runtime.block_on(...) calls
wrapper.sdk.sync_address_balances(..., rust_config, last_sync_ts). Insert a
debug! (matching the existing dash_sdk_sync_address_balances variant) that logs
the resolved rust_config (or its identifying fields) and last_sync_ts so
incremental vs full-scan runs are distinguishable in logs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-sdk-ffi/src/address_sync/mod.rspackages/rs-sdk-ffi/src/address_sync/types.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk-ffi/src/address_sync/types.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-sdk-ffi/src/address_sync/types.rs (1)
55-76:⚠️ Potential issue | 🟡 MinorStale doc on
trunk_queries: no longer "always 1 for a successful sync".With incremental-only mode,
trunk_queriesis 0 (no trunk query is issued). The doc is misleading for FFI consumers relying on it to detect the sync mode.📝 Suggested doc fix
- /// Number of trunk queries (always 1 for a successful sync) + /// Number of trunk queries (0 for incremental-only, 1 for full tree scan) pub trunk_queries: u32,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/types.rs` around lines 55 - 76, The doc comment for DashSDKAddressSyncMetrics.trunk_queries is stale: update the comment to state that trunk_queries can be 0 in incremental-only mode (no trunk query issued), may be 1 for a full sync, and consumers should not assume a value of 1 implies success; adjust the field doc on the DashSDKAddressSyncMetrics struct to clearly describe these possible values and intended use by FFI clients.packages/rs-sdk-ffi/src/address_sync/mod.rs (1)
396-411:⚠️ Potential issue | 🟡 Minor
test_config_defaultdoesn't assert thefull_rescan_after_time_sdefault.The new field is the key control for incremental vs. full-scan behavior; leaving it out of the unit test makes the default easy to accidentally change.
✅ Suggested addition
assert_eq!(config.max_iterations, 50); + assert_eq!(config.full_rescan_after_time_s, 7 * 24 * 60 * 60);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/mod.rs` around lines 396 - 411, The test_config_default is missing an assertion for the new DashSDKAddressSyncConfig::full_rescan_after_time_s field; update the test to assert the configured default for full_rescan_after_time_s (e.g. add assert_eq!(config.full_rescan_after_time_s, <expected_default>)); if there is a named constant (like DEFAULT_FULL_RESCAN_AFTER_TIME_S) use that constant in the assertion, otherwise use the literal value used in DashSDKAddressSyncConfig::default() so the test will catch accidental changes to the default.
🧹 Nitpick comments (4)
packages/rs-sdk-ffi/src/address_sync/types.rs (1)
55-76:DashSDKAddressSyncMetricsomitscompacted_queriesandrecent_queries.FFI consumers cannot distinguish between a sync that did no incremental queries versus one that paginated through many. The Rust
AddressSyncMetricsalready tracks both fields. Consider exposing them here.📝 Proposed addition
/// Number of iterations (0 = trunk only, 1+ = trunk plus branch rounds) pub iterations: u32, + + /// Number of compacted incremental queries issued. + pub compacted_queries: u32, + + /// Number of recent incremental queries issued. + pub recent_queries: u32,And in
convert_sync_result(mod.rs):let metrics = DashSDKAddressSyncMetrics { trunk_queries: result.metrics.trunk_queries as u32, branch_queries: result.metrics.branch_queries as u32, total_elements_seen: result.metrics.total_elements_seen as u32, total_proof_bytes: result.metrics.total_proof_bytes as u32, iterations: result.metrics.iterations as u32, + compacted_queries: result.metrics.compacted_queries as u32, + recent_queries: result.metrics.recent_queries as u32, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/types.rs` around lines 55 - 76, The FFI struct DashSDKAddressSyncMetrics is missing the compacted_queries and recent_queries fields present on the Rust AddressSyncMetrics; add two u32 fields named compacted_queries and recent_queries to the DashSDKAddressSyncMetrics struct and update the convert_sync_result function to populate these new fields from the source AddressSyncMetrics so FFI consumers can distinguish incremental/paginated queries from none. Ensure field names exactly match compacted_queries and recent_queries and that convert_sync_result assigns the corresponding values when building DashSDKAddressSyncMetrics.packages/rs-sdk/src/platform/address_sync/mod.rs (2)
600-615:_countis used — drop the underscore prefix to avoid misreading and potentialclippy::used_underscore_bindingwarning.The leading underscore conventionally signals "intentionally unused", but
_countis passed directly toLeafInfo { count: Some(_count) }. Rename it tocountto match the field name and signal intent correctly.📝 Suggested fix
- if let Some((levels_up, _count, ancestor_key, ancestor_hash)) = + if let Some((levels_up, count, ancestor_key, ancestor_hash)) = trunk_result.get_ancestor(&leaf_key, min_privacy_count) { if seen_ancestors.insert(ancestor_key.clone()) { let ancestor_info = LeafInfo { hash: ancestor_hash, - count: Some(_count), + count: Some(count), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/address_sync/mod.rs` around lines 600 - 615, The binding named `_count` returned from trunk_result.get_ancestor is used (passed into LeafInfo { count: Some(_count) }) but has a leading underscore which implies unused; change the pattern to bind it as `count` (e.g., replace `_count` with `count`) in the match for trunk_result.get_ancestor so the variable is used explicitly and then pass `count` into LeafInfo::count, leaving the rest of the logic (leaf_key, min_privacy_count, seen_ancestors insertion, ancestor_key, ancestor_hash, tree_depth depth calculation, and pushing to result) unchanged.
332-427: Unbounded pagination loops — consider a max-iterations guard.Both
loopblocks break onentries.is_empty()orentry_count < batch_limit, relying on the server to behave correctly. If a misbehaving server consistently returns exactlyCOMPACTED_BATCH_LIMIT/RECENT_BATCH_LIMITentries whoseend_block_height/block_heightnever advancescurrent_height, the loop will not terminate. The tree-scan phase already has amax_iterationsguard; applying the same pattern here would be consistent.🔒 Sketch of a guard (compacted phase; same applies to recent)
+ let mut compacted_iterations = 0usize; + let max_compacted_iterations = 1000; // or a configurable value loop { + compacted_iterations += 1; + if compacted_iterations > max_compacted_iterations { + warn!("Compacted catch-up reached iteration limit; stopping early"); + break; + } let request = GetRecentCompactedAddressBalanceChangesRequest { ... };Also applies to: 430-514
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/address_sync/mod.rs` around lines 332 - 427, The compacted and recent pagination loops can become infinite if the server returns full-size pages without advancing current_height; add a max-iterations guard like the tree-scan phase: introduce a local counter (e.g., compacted_iterations / recent_iterations), increment it each outer-loop iteration (the loop that builds GetRecentCompactedAddressBalanceChangesRequest and its sibling for recent), and if it exceeds a sensible constant (e.g., MAX_PAGINATION_ITERATIONS) break or return an error; ensure you reference and update/inspect current_height (and entry.end_block_height / entry.block_height) as before and reset/stop the loop when entries.is_empty() or entry_count < COMPACTED_BATCH_LIMIT / RECENT_BATCH_LIMIT, so the new guard only triggers for pathological servers.packages/rs-sdk-ffi/src/address_sync/mod.rs (1)
59-77: Duplicated config conversion and timestamp mapping across both FFI entry points.The config-to-
AddressSyncConfigconstruction and theu64 → Option<u64>timestamp conversion are identical in both functions. Extract them into private helpers to reduce the maintenance surface.♻️ Suggested helpers
+fn convert_ffi_config(config: *const DashSDKAddressSyncConfig) -> Option<AddressSyncConfig> { + if config.is_null() { + return None; + } + // SAFETY: caller guarantees config is a valid pointer when non-null + let c = unsafe { &*config }; + Some(AddressSyncConfig { + min_privacy_count: c.min_privacy_count, + max_concurrent_requests: c.max_concurrent_requests as usize, + max_iterations: c.max_iterations as usize, + full_rescan_after_time_s: c.full_rescan_after_time_s, + request_settings: RequestSettings::default(), + }) +} + +fn convert_ffi_timestamp(ts: u64) -> Option<u64> { + if ts == 0 { None } else { Some(ts) } +}Also applies to: 153-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/mod.rs` around lines 59 - 77, Extract the duplicated FFI-to-Rust conversions into two private helpers: one that converts the raw config pointer (named config in the diff) into Option<AddressSyncConfig> building AddressSyncConfig (copying min_privacy_count, max_concurrent_requests -> usize, max_iterations -> usize, full_rescan_after_time_s and using RequestSettings::default()), and another that maps last_sync_timestamp (u64) into Option<u64> (treat 0 as None). Replace the inline blocks in both FFI entry points with calls to these helpers so both the AddressSyncConfig construction and the u64 → Option<u64> timestamp mapping are centralized and reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk/src/platform/address_sync/mod.rs`:
- Around line 13-20: The rustdoc link is pointing to a non-existent member
AddressSyncConfig::last_sync_height; update the documentation to reference the
actual controlling fields by replacing that link with
AddressSyncConfig::last_sync_timestamp and, where relevant,
AddressProvider::last_sync_height(), and adjust the surrounding text to say the
behavior depends on AddressSyncConfig::last_sync_timestamp together with
AddressProvider::last_sync_height() so rustdoc links resolve and the description
matches code.
---
Outside diff comments:
In `@packages/rs-sdk-ffi/src/address_sync/mod.rs`:
- Around line 396-411: The test_config_default is missing an assertion for the
new DashSDKAddressSyncConfig::full_rescan_after_time_s field; update the test to
assert the configured default for full_rescan_after_time_s (e.g. add
assert_eq!(config.full_rescan_after_time_s, <expected_default>)); if there is a
named constant (like DEFAULT_FULL_RESCAN_AFTER_TIME_S) use that constant in the
assertion, otherwise use the literal value used in
DashSDKAddressSyncConfig::default() so the test will catch accidental changes to
the default.
In `@packages/rs-sdk-ffi/src/address_sync/types.rs`:
- Around line 55-76: The doc comment for DashSDKAddressSyncMetrics.trunk_queries
is stale: update the comment to state that trunk_queries can be 0 in
incremental-only mode (no trunk query issued), may be 1 for a full sync, and
consumers should not assume a value of 1 implies success; adjust the field doc
on the DashSDKAddressSyncMetrics struct to clearly describe these possible
values and intended use by FFI clients.
---
Nitpick comments:
In `@packages/rs-sdk-ffi/src/address_sync/mod.rs`:
- Around line 59-77: Extract the duplicated FFI-to-Rust conversions into two
private helpers: one that converts the raw config pointer (named config in the
diff) into Option<AddressSyncConfig> building AddressSyncConfig (copying
min_privacy_count, max_concurrent_requests -> usize, max_iterations -> usize,
full_rescan_after_time_s and using RequestSettings::default()), and another that
maps last_sync_timestamp (u64) into Option<u64> (treat 0 as None). Replace the
inline blocks in both FFI entry points with calls to these helpers so both the
AddressSyncConfig construction and the u64 → Option<u64> timestamp mapping are
centralized and reused.
In `@packages/rs-sdk-ffi/src/address_sync/types.rs`:
- Around line 55-76: The FFI struct DashSDKAddressSyncMetrics is missing the
compacted_queries and recent_queries fields present on the Rust
AddressSyncMetrics; add two u32 fields named compacted_queries and
recent_queries to the DashSDKAddressSyncMetrics struct and update the
convert_sync_result function to populate these new fields from the source
AddressSyncMetrics so FFI consumers can distinguish incremental/paginated
queries from none. Ensure field names exactly match compacted_queries and
recent_queries and that convert_sync_result assigns the corresponding values
when building DashSDKAddressSyncMetrics.
In `@packages/rs-sdk/src/platform/address_sync/mod.rs`:
- Around line 600-615: The binding named `_count` returned from
trunk_result.get_ancestor is used (passed into LeafInfo { count: Some(_count) })
but has a leading underscore which implies unused; change the pattern to bind it
as `count` (e.g., replace `_count` with `count`) in the match for
trunk_result.get_ancestor so the variable is used explicitly and then pass
`count` into LeafInfo::count, leaving the rest of the logic (leaf_key,
min_privacy_count, seen_ancestors insertion, ancestor_key, ancestor_hash,
tree_depth depth calculation, and pushing to result) unchanged.
- Around line 332-427: The compacted and recent pagination loops can become
infinite if the server returns full-size pages without advancing current_height;
add a max-iterations guard like the tree-scan phase: introduce a local counter
(e.g., compacted_iterations / recent_iterations), increment it each outer-loop
iteration (the loop that builds GetRecentCompactedAddressBalanceChangesRequest
and its sibling for recent), and if it exceeds a sensible constant (e.g.,
MAX_PAGINATION_ITERATIONS) break or return an error; ensure you reference and
update/inspect current_height (and entry.end_block_height / entry.block_height)
as before and reset/stop the loop when entries.is_empty() or entry_count <
COMPACTED_BATCH_LIMIT / RECENT_BATCH_LIMIT, so the new guard only triggers for
pathological servers.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rs-sdk-ffi/src/address_sync/mod.rspackages/rs-sdk-ffi/src/address_sync/types.rspackages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-sdk/src/platform/address_sync/types.rs
Contributor
|
Reviewed the latest CodeRabbit feedback threads on this PR. All actionable items appear already addressed in the branch ( |
QuantumExplorer
changed the title
Contributor
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-sdk/src/platform/address_sync/mod.rs (1)
124-166:⚠️ Potential issue | 🟠 MajorAllow incremental-only updates when
last_sync_timestampis present, regardless of pending status.If a provider distinguishes between "pending addresses" (new/changed) and "current balances" (known state from previous sync), the early return at line 128 prevents incremental-only mode when there are no pending addresses but incremental state exists. This skips balance updates to existing addresses. Additionally, when entering incremental-only mode,
current_balances()are seeded into results but their keys are never added tokey_to_index, so the subsequent incremental catch-up phase (line 325) buildsaddress_key_lookupwithout them, preventing balance changes from being matched to those addresses.Modify the early return to allow incremental-only when
last_sync_timestampis present, and ensure keys fromcurrent_balances()are added tokey_to_index:Fix
- // If no pending addresses, return early - if !provider.has_pending() { - return Ok(result); - } + // If no pending addresses and no incremental state, return early + if !provider.has_pending() && last_sync_timestamp.is_none() { + return Ok(result); + } @@ - for (index, key, funds) in provider.current_balances() { - result.found.insert((index, key), funds); - } + for (index, key, funds) in provider.current_balances() { + key_to_index.insert(key.clone(), index); + result.found.insert((index, key), funds); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/address_sync/mod.rs` around lines 124 - 166, The early return that exits when provider.has_pending() is false prevents entering incremental-only mode when last_sync_timestamp is present and also fails to populate key_to_index for seeded current_balances(); modify the logic around provider.has_pending() and last_sync_timestamp so that we only return early when there are no pending changes AND no last_sync_timestamp (i.e., allow incremental-only when last_sync_timestamp.is_some()), and when seeding result.found from provider.current_balances() inside the incremental-only branch also insert each key into key_to_index (the same mapping used later when building address_key_lookup) so subsequent catch-up changes can be matched to those seeded balances (update uses of provider.has_pending(), last_sync_timestamp, current_balances(), result.found, and key_to_index/catch_up_from accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/rs-sdk/src/platform/address_sync/mod.rs`:
- Around line 124-166: The early return that exits when provider.has_pending()
is false prevents entering incremental-only mode when last_sync_timestamp is
present and also fails to populate key_to_index for seeded current_balances();
modify the logic around provider.has_pending() and last_sync_timestamp so that
we only return early when there are no pending changes AND no
last_sync_timestamp (i.e., allow incremental-only when
last_sync_timestamp.is_some()), and when seeding result.found from
provider.current_balances() inside the incremental-only branch also insert each
key into key_to_index (the same mapping used later when building
address_key_lookup) so subsequent catch-up changes can be matched to those
seeded balances (update uses of provider.has_pending(), last_sync_timestamp,
current_balances(), result.found, and key_to_index/catch_up_from accordingly).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-sdk/src/platform/address_sync/provider.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk/src/platform/address_sync/provider.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
This update introduces incremental address balance synchronization to enhance the efficiency of address balance updates.
What was done?
AddressSyncConfigto includelast_sync_heightandfull_rescan_after_time_sparameters, allowing for more flexible synchronization strategies.sync_address_balancesfunction to conditionally perform either a full tree scan or incremental catch-up based on the provided configuration.AddressProvidertrait to support fetching current known balances during incremental syncs.How Has This Been Tested?
Breaking Changes
None
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Public API
Tests