test(sdk): test sync_address_balances #2957
Conversation
…' into test_sync_address_balances
…sync_address_balances
✅ gRPC Query Coverage Report |
📝 WalkthroughWalkthroughAdds a new test for address balance synchronization (TestAddressProvider), registers the test module under the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rs-sdk/tests/fetch/address_sync.rs (1)
64-65: Consider renaming to follow test naming convention.Per coding guidelines, tests should be named descriptively starting with "should_". Consider renaming to something like
should_sync_address_balances_and_track_found_absent_addresses.Suggested rename
-#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn test_sync_address_balances() { +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn should_sync_address_balances_and_track_found_absent_addresses() {As per coding guidelines for test naming.
packages/rs-sdk/src/platform/address_sync/mod.rs (1)
130-139: Consider validating depth limits consistency.The code extracts
min_query_depthandmax_query_depthfrom platform version settings without validating thatmin_query_depth <= max_query_depth. If platform configuration is incorrect, the subsequentclampoperations could produce unexpected results.Suggested validation
let min_query_depth = platform_version .drive .methods .address_funds .address_funds_query_min_depth; let max_query_depth = platform_version .drive .methods .address_funds .address_funds_query_max_depth; + +if min_query_depth > max_query_depth { + return Err(Error::Protocol(dpp::ProtocolError::CorruptedCodeExecution( + format!( + "Invalid platform depth limits: min {} > max {}", + min_query_depth, max_query_depth + ), + ))); +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-sdk/tests/fetch/address_sync.rspackages/rs-sdk/tests/fetch/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-sdk/tests/fetch/mod.rspackages/rs-sdk/tests/fetch/address_sync.rspackages/rs-sdk/src/platform/address_sync/mod.rs
**/tests/**/*.{js,jsx,ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.{js,jsx,ts,tsx,rs}: Name tests descriptively, starting with 'should …'
Unit and integration tests should not perform network calls; mock dependencies
Files:
packages/rs-sdk/tests/fetch/mod.rspackages/rs-sdk/tests/fetch/address_sync.rs
🧠 Learnings (8)
📚 Learning: 2024-10-10T10:30:19.883Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2232
File: packages/rs-sdk/src/mock/sdk.rs:90-95
Timestamp: 2024-10-10T10:30:19.883Z
Learning: In `packages/rs-sdk/src/mock/sdk.rs`, the `load_expectations` method in `MockDashPlatformSdk` remains asynchronous (`async`) for backward compatibility, even though it now delegates to the synchronous `load_expectations_sync` method.
Applied to files:
packages/rs-sdk/tests/fetch/mod.rspackages/rs-sdk/tests/fetch/address_sync.rs
📚 Learning: 2024-10-30T11:19:59.163Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/tests/fetch/config.rs:233-233
Timestamp: 2024-10-30T11:19:59.163Z
Learning: In the Rust SDK's `rs-sdk/tests` integration tests (e.g., in `packages/rs-sdk/tests/fetch/config.rs`), we cannot create objects during tests because there is no support for object creation in this context. Therefore, hardcoded values for test identities must be used.
Applied to files:
packages/rs-sdk/tests/fetch/mod.rspackages/rs-sdk/tests/fetch/address_sync.rs
📚 Learning: 2024-10-10T10:30:25.283Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2232
File: packages/rs-sdk/src/mock/sdk.rs:129-130
Timestamp: 2024-10-10T10:30:25.283Z
Learning: In the codebase `packages/rs-sdk/src/mock/sdk.rs`, `tokio::sync::Mutex` is used instead of `std::sync::Mutex` because `std::sync::MutexGuard` is not `Send` and cannot be used in async contexts.
Applied to files:
packages/rs-sdk/tests/fetch/mod.rs
📚 Learning: 2024-10-29T14:44:01.184Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-dapi-client/src/executor.rs:38-38
Timestamp: 2024-10-29T14:44:01.184Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, for the structs `ExecutionError<E>` and `ExecutionResponse<R>`, only the serde derives (`Serialize`, `Deserialize`) should be conditional based on the "mocks" feature; the other derives (`Debug`, `Clone`, `Eq`, `PartialEq`) should always be present.
Applied to files:
packages/rs-sdk/tests/fetch/mod.rs
📚 Learning: 2025-01-23T09:23:32.740Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2405
File: packages/rs-sdk/src/sync.rs:88-95
Timestamp: 2025-01-23T09:23:32.740Z
Learning: The `block_on` function in `packages/rs-sdk/src/sync.rs` is currently only used in tests, and its WebAssembly implementation is deferred until there's a user request for it.
Applied to files:
packages/rs-sdk/tests/fetch/mod.rs
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2025-02-03T23:38:33.414Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2450
File: packages/rs-drive/src/drive/tokens/distribution/fetch/pre_programmed_distributions/mod.rs:75-81
Timestamp: 2025-02-03T23:38:33.414Z
Learning: GroveDB's `as_sum_item_value()` method returns an `i64`, so sum items cannot exceed `i64::MAX` by design.
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
🧬 Code graph analysis (1)
packages/rs-sdk/tests/fetch/address_sync.rs (1)
packages/rs-sdk/src/platform/address_sync/types.rs (1)
total_balance(91-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
🔇 Additional comments (8)
packages/rs-sdk/tests/fetch/mod.rs (1)
10-10: LGTM!The module inclusion is properly placed under the
mocksfeature gate and maintains alphabetical ordering.packages/rs-sdk/tests/fetch/address_sync.rs (3)
15-33: LGTM!The test provider structure is well-designed with appropriate fields for tracking address sync state during testing.
35-61: LGTM!The
AddressProvidertrait implementation correctly maintains state, with proper tracking of the highest found index usingmap_orandmax.
66-103: LGTM!The test comprehensively validates address sync behavior, checking found/absent tracking, balance extraction, total balance calculation, and highest found index. The use of hardcoded test addresses aligns with the testing patterns for this codebase.
packages/rs-sdk/src/platform/address_sync/mod.rs (4)
147-153: LGTM!The function call correctly passes the platform-configured depth limits to enable privacy-aware depth clamping.
286-334: LGTM!The depth clamping logic correctly ensures all query depths stay within platform limits:
- Direct leaf queries use clamped tree depth
- Ancestor queries properly reduce depth using
saturating_subbefore clamping- Fallback cases also use clamped depth
485-490: Verify the safety of the i64 to u64 cast.The cast from
value(likelyi64based on GroveDB's sum item design) tou64at line 487 could produce incorrect results ifvalueis negative. While address balances should never be negative in normal operation, consider adding a defensive check or clarification.Suggested defensive check
fn extract_balance_from_element(element: &Element) -> u64 { match element { - Element::ItemWithSumItem(_, value, _) => *value as u64, + Element::ItemWithSumItem(_, value, _) => { + if *value < 0 { + warn!("Negative balance {} found in address funds tree", value); + 0 + } else { + *value as u64 + } + } _ => 0, } }Alternatively, if negative values are impossible by platform invariants, a comment explaining this would be helpful.
545-552: LGTM!The test correctly validates balance extraction from the updated
Element::ItemWithSumItemvariant.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-sdk/src/sdk.rs (1)
267-279: Logic correctly implements conditional staleness checking.The implementation properly enables global disabling of staleness checks while maintaining special handling for checkpoint queries (trunk/branch state methods). When
metadata_time_tolerance_msis Some, these methods use a fixed 31-minute tolerance regardless of the actual tolerance value, which is appropriate for their different freshness requirements.💡 Optional: Consider using `map` for clarity
Using
mapinstead ofandmore explicitly shows the value transformation intent:"get_addresses_trunk_state" | "get_addresses_branch_state" => ( None, - self.metadata_time_tolerance_ms - .and(Some(ADDRESS_STATE_TIME_TOLERANCE_MS)), + self.metadata_time_tolerance_ms + .map(|_| ADDRESS_STATE_TIME_TOLERANCE_MS), ),Both are functionally equivalent, but
mapis more commonly used for value transformation and may be more immediately understood by readers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-sdk/src/sdk.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-sdk/src/sdk.rs
🧠 Learnings (2)
📚 Learning: 2024-10-18T15:43:32.447Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-sdk/src/sdk.rs:654-658
Timestamp: 2024-10-18T15:43:32.447Z
Learning: In `packages/rs-sdk/src/sdk.rs`, within the `verify_metadata_height` function, when using `compare_exchange` on `last_seen_height`, it's acceptable to use `Ordering::Relaxed` for the failure ordering, as any inconsistency would only trigger an extra loop iteration without affecting correctness.
Applied to files:
packages/rs-sdk/src/sdk.rs
📚 Learning: 2024-11-28T13:49:17.301Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2317
File: packages/rs-dapi-client/src/address_list.rs:175-180
Timestamp: 2024-11-28T13:49:17.301Z
Learning: In Rust code in `packages/rs-dapi-client/src/address_list.rs`, do not change the interface of deprecated methods like `add_uri`, even to fix potential panics.
Applied to files:
packages/rs-sdk/src/sdk.rs
🧬 Code graph analysis (1)
packages/rs-sdk/src/sdk.rs (3)
packages/rs-sdk/src/platform/documents/document_query.rs (1)
method_name(144-146)packages/rs-dapi-client/src/transport.rs (1)
method_name(49-49)packages/rs-sdk/src/platform/types/evonode.rs (1)
method_name(60-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
🔇 Additional comments (1)
packages/rs-sdk/src/sdk.rs (1)
264-266: Clear documentation of global staleness check disabling.The added documentation effectively explains how setting tolerance to None disables staleness checks globally, which helps users understand this important configuration option.
Issue being fixed or feature implemented
sync_address_balances not tested
What was done?
Added test for sync_address_balances.
Requires #2956 and #2955 to be merged first
How Has This Been Tested?
green GHA
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.