feat: sdk support for platform addresses #2933
Conversation
✅ gRPC Query Coverage Report |
📝 WalkthroughWalkthroughAdds address balance synchronization (trunk + branch queries) with FFI bindings, checkpoint-aware branch proofs and GroveDB source selection, new gRPC transport methods, trunk-proof verification, checkpoint creation flow, local grovedb path deps, and related tests and helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Sdk
participant Provider
participant TrunkExec
participant BranchExec
participant GroveDB
participant ProofVerifier
Note over Sdk,Provider: Start address sync
Client->>Sdk: sync_address_balances(provider, config)
Sdk->>Provider: pending_addresses()
Provider-->>Sdk: [(index,key),...]
rect rgb(220,230,250)
Note over Sdk,TrunkExec: Trunk query + verification
Sdk->>TrunkExec: execute_trunk_query()
TrunkExec->>GroveDB: trunk_query(leaf_paths)
GroveDB-->>TrunkExec: trunk_response + proof
TrunkExec->>ProofVerifier: verify_trunk_response(response, proof)
ProofVerifier-->>Sdk: verified_trunk_result
end
rect rgb(220,250,230)
Note over Sdk,BranchExec: Iterative branch queries (parallel)
loop while tracker not empty && iterations < max
Sdk->>BranchExec: execute_branch_queries(leaves[], checkpoint_height?)
par Parallel branches
BranchExec->>GroveDB: branch_query(leaf, depth, grove_db_selector)
GroveDB-->>BranchExec: branch_response + proof
end
BranchExec->>ProofVerifier: verify_branch_responses(responses)
ProofVerifier-->>Sdk: verified_branch_results
Sdk->>Provider: on_address_found/absent(...)
end
end
Sdk->>Provider: highest_found_index()
Provider-->>Sdk: highest_index?
Sdk-->>Client: AddressSyncResult{found, absent, metrics}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/rs-sdk/src/mock/requests.rs (1)
508-521: Intentional panic forGroveTrunkQueryResultmock serializationThe
MockResponseimpl explicitly panics viaunimplemented!()to signal that trunk state results must not be recorded/replayed in the current mocking setup. This is acceptable as long as tests and tools never expect to persist or replay these values; otherwise you’ll get a hard fail at runtime.If you want a slightly clearer failure mode, you could consider
panic!()with the same message instead ofunimplemented!(), but functionally this is fine.packages/rs-sdk/src/platform/address_sync/provider.rs (1)
50-104: Trait method set is sufficient; consider future ergonomicsThe method set (
gap_limit,pending_addresses,on_address_found,on_address_absent,has_pending,highest_found_index) is minimal yet complete for the current sync design. Returning aVecfrompending_addressesis fine for now; if large address sets become common, switching to an iterator or borrowing API could be a later optimization.packages/rs-sdk/src/platform/address_sync/tracker.rs (1)
51-55: Potential memory accumulation inleaf_infomap.When
add_keyis called multiple times with different leaves, or whenupdate_leafmoves keys to new leaves, oldLeafInfoentries remain in theleaf_infomap even after their refcount drops to zero. Over many iterations, this could accumulate stale entries.Consider cleaning up
leaf_infoentries when their refcount reaches zero, or documenting that this is intentional for potential future lookups.🔎 Optional cleanup in key_found
pub fn key_found(&mut self, key: &[u8]) { if let Some(leaf) = self.key_to_leaf.remove(key) { if let Some(count) = self.leaf_refcount.get_mut(&leaf) { *count = count.saturating_sub(1); + if *count == 0 { + self.leaf_refcount.remove(&leaf); + self.leaf_info.remove(&leaf); + } } } }packages/rs-sdk/src/platform/address_sync/mod.rs (1)
346-359: Branch query failures are silently logged but not propagated.When a branch query fails (lines 353-356, 369-371), the error is only logged with
warn!and the loop continues. While this provides resilience, it means some addresses may never be resolved if their branch queries consistently fail, and the caller has no indication of partial failures.Consider either:
- Tracking failed leaves for retry in subsequent iterations
- Including failure metrics in
AddressSyncMetrics- Documenting this behavior explicitly
packages/rs-sdk-ffi/src/address_sync/provider.rs (2)
83-86:unsafe impl Sendrequires careful consideration of callback thread-safety.The
Sendimplementation assumes that callbacks can be safely invoked from any thread. However, the actual safety depends on:
- The C caller ensuring their callbacks are thread-safe
- The
contextpointer being safe to access across threadsConsider adding documentation in the public API (or vtable comments) warning callers that callbacks must be thread-safe if the provider will be used with async operations.
🔎 Suggested documentation enhancement
// SAFETY: AddressProviderFFI only contains function pointers and an opaque context // The actual thread safety depends on the callback implementations +// IMPORTANT: Callers must ensure that all callbacks are thread-safe and that +// the context pointer can be safely accessed from multiple threads if the +// provider is used with async operations. unsafe impl Send for AddressProviderFFI {}
98-127: Vtable pointer is dereferenced without null check in trait methods.The
CallbackAddressProvidertrait implementation dereferencesself.ffi.vtablewithout checking for null. Whiledash_sdk_address_provider_createvalidates the vtable pointer, direct construction ofAddressProviderFFI(as done in tests at lines 238-241) could theoretically have a null vtable.Since
CallbackAddressProviderispub(crate)and the tests use a valid static vtable, this is acceptable, but consider adding a debug assertion inCallbackAddressProvider::new()for defensive programming.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
packages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-dapi-client/src/transport/grpc.rspackages/rs-drive-abci/src/query/address_funds/addresses_branch_state/v0/mod.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive/Cargo.tomlpackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rspackages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/mod.rspackages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/mod.rspackages/rs-platform-version/Cargo.tomlpackages/rs-sdk-ffi/src/address_sync/mod.rspackages/rs-sdk-ffi/src/address_sync/provider.rspackages/rs-sdk-ffi/src/address_sync/types.rspackages/rs-sdk-ffi/src/lib.rspackages/rs-sdk/src/mock/requests.rspackages/rs-sdk/src/platform.rspackages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-sdk/src/platform/address_sync/provider.rspackages/rs-sdk/src/platform/address_sync/tracker.rspackages/rs-sdk/src/platform/address_sync/types.rspackages/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/platform.rspackages/rs-sdk/src/mock/requests.rspackages/rs-sdk-ffi/src/lib.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/mod.rspackages/rs-sdk/src/sdk.rspackages/rs-sdk/src/platform/address_sync/provider.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rspackages/rs-sdk-ffi/src/address_sync/types.rspackages/rs-sdk/src/platform/address_sync/types.rspackages/rs-dapi-client/src/transport/grpc.rspackages/rs-drive-abci/src/query/address_funds/addresses_branch_state/v0/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/mod.rspackages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-sdk/src/platform/address_sync/tracker.rspackages/rs-sdk-ffi/src/address_sync/mod.rspackages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/mod.rspackages/rs-sdk-ffi/src/address_sync/provider.rs
🧠 Learnings (33)
📓 Common learnings
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Applies to **/swift-sdk/**/*.{swift,h,m,c} : iOS SDK must combine both Core (SPV wallet) and Platform (identity/documents) functionality in the unified SDK with proper function naming: `dash_core_sdk_*` prefix for Core functions, `dash_sdk_*` prefix for Platform functions, and `dash_unified_sdk_*` prefix for unified functions
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 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/src/platform.rspackages/rs-sdk/src/mock/requests.rspackages/rs-sdk-ffi/src/address_sync/types.rspackages/rs-sdk-ffi/src/address_sync/mod.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/platform.rspackages/rs-sdk/src/platform/address_sync/provider.rspackages/rs-sdk-ffi/src/address_sync/types.rspackages/rs-sdk/src/platform/address_sync/types.rspackages/rs-drive-abci/src/query/address_funds/addresses_branch_state/v0/mod.rspackages/rs-sdk-ffi/src/address_sync/mod.rspackages/rs-sdk-ffi/src/address_sync/provider.rs
📚 Learning: 2024-10-18T15:39:51.172Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-sdk/src/sdk.rs:585-585
Timestamp: 2024-10-18T15:39:51.172Z
Learning: The 'platform' project uses Rust version 1.80, so code in 'packages/rs-sdk' can use features available in Rust 1.80, such as the `abs_diff()` method.
Applied to files:
packages/rs-sdk/src/platform.rspackages/rs-platform-version/Cargo.toml
📚 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/src/platform.rspackages/rs-sdk-ffi/src/lib.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/src/mock/requests.rs
📚 Learning: 2024-10-29T14:13:35.584Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/dapi-grpc/src/mock.rs:95-109
Timestamp: 2024-10-29T14:13:35.584Z
Learning: In test code (e.g., `packages/dapi-grpc/src/mock.rs`), panicking on deserialization errors is acceptable behavior.
Applied to files:
packages/rs-sdk/src/mock/requests.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/src/mock/requests.rs
📚 Learning: 2024-10-29T14:16:40.972Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-dapi-client/src/address_list.rs:63-73
Timestamp: 2024-10-29T14:16:40.972Z
Learning: In the Rust SDK, it's acceptable to use `expect()` that panics on errors in the `Mockable` trait implementations during testing, and the `Mockable` trait will be refactored in a future PR.
Applied to files:
packages/rs-sdk/src/mock/requests.rs
📚 Learning: 2024-10-29T14:16:00.141Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/dapi-grpc/src/mock.rs:125-145
Timestamp: 2024-10-29T14:16:00.141Z
Learning: Using `serde_json` encoding produces deterministic output required to identify requests and responses by hash, therefore `serde_json` is used as the basic serialization algorithm for mocking.
Applied to files:
packages/rs-sdk/src/mock/requests.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-sdk/src/mock/requests.rs
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Use `rs-drive-proof-verifier` for cryptographic proof verification
Applied to files:
packages/rs-sdk/src/mock/requests.rspackages/rs-drive-proof-verifier/src/proof.rs
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2207
File: packages/rs-drive-proof-verifier/src/proof.rs:1646-1664
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In the implementation of `FromProof<platform::GetContestedResourceIdentityVotesRequest>` in `packages/rs-drive-proof-verifier/src/proof.rs`, when matching `maybe_votes`, using `.expect()` on `v.into_iter().next()` is acceptable because the prior match arm `Some(v) if v.is_empty()` ensures that the map is not empty, preventing a panic.
Applied to files:
packages/rs-sdk/src/mock/requests.rspackages/rs-drive-proof-verifier/src/proof.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/src/mock/requests.rspackages/rs-sdk-ffi/src/lib.rspackages/rs-sdk/src/platform/address_sync/types.rs
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Applies to **/swift-sdk/**/*.{swift,h,m,c} : iOS SDK must combine both Core (SPV wallet) and Platform (identity/documents) functionality in the unified SDK with proper function naming: `dash_core_sdk_*` prefix for Core functions, `dash_sdk_*` prefix for Platform functions, and `dash_unified_sdk_*` prefix for unified functions
Applied to files:
packages/rs-sdk-ffi/src/lib.rspackages/rs-sdk-ffi/src/address_sync/mod.rs
📚 Learning: 2025-09-03T16:37:11.605Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2756
File: packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:11-11
Timestamp: 2025-09-03T16:37:11.605Z
Learning: In packages/rs-dpp/Cargo.toml, the abci feature already includes core_rpc_client, and core_rpc_client is defined as ["dep:dashcore-rpc"]. This means rs-drive-abci can access dashcore-rpc types through dpp when using the abci feature.
Applied to files:
packages/rs-sdk-ffi/src/lib.rspackages/rs-drive/Cargo.tomlpackages/rs-platform-version/Cargo.toml
📚 Learning: 2025-01-15T08:09:59.365Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2422
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:152-163
Timestamp: 2025-01-15T08:09:59.365Z
Learning: In the `transition_to_version_8` function, errors from `grove_get_path_query` when retrieving active contested resource votes are intentionally logged and ignored (returning `Ok(())`) to allow the protocol upgrade to proceed despite query failures.
Applied to files:
packages/rs-drive-proof-verifier/src/proof.rs
📚 Learning: 2025-01-19T07:36:46.042Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2431
File: packages/rs-drive/Cargo.toml:55-60
Timestamp: 2025-01-19T07:36:46.042Z
Learning: The grovedb dependencies in packages/rs-drive/Cargo.toml and related files are intentionally kept at specific revisions rather than using the latest stable version, with plans to update them at a later time.
Applied to files:
packages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/mod.rspackages/rs-drive/Cargo.tomlpackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rspackages/rs-platform-version/Cargo.tomlpackages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/mod.rs
📚 Learning: 2024-10-30T11:04:33.634Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/platform/fetch_unproved.rs:0-0
Timestamp: 2024-10-30T11:04:33.634Z
Learning: In `packages/rs-sdk/src/platform/fetch_unproved.rs`, the `execute()` method consumes the request object, so cloning the request is necessary before passing it to `execute()` and `maybe_from_unproved_with_metadata`.
Applied to files:
packages/rs-drive-proof-verifier/src/proof.rs
📚 Learning: 2025-03-07T12:37:44.555Z
Learnt from: ogabrielides
Repo: dashpay/platform PR: 2486
File: packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/v0/mod.rs:1-29
Timestamp: 2025-03-07T12:37:44.555Z
Learning: GroveDB operations like `insert()` return a tuple that includes a cost value which needs to be unwrapped with `.unwrap()` before error handling with `.map_err()`.
Applied to files:
packages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rspackages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/mod.rs
📚 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.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/mod.rs
📚 Learning: 2024-12-05T09:29:38.918Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2375
File: packages/rs-drive-abci/Cargo.toml:61-63
Timestamp: 2024-12-05T09:29:38.918Z
Learning: In the `drive-abci` package, avoid adding unused dependencies like `hashbrown` to `Cargo.toml`. The team relies on CI to detect dependency version issues.
Applied to files:
packages/rs-drive/Cargo.toml
📚 Learning: 2024-10-29T10:42:00.521Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-dapi-client/Cargo.toml:22-22
Timestamp: 2024-10-29T10:42:00.521Z
Learning: In `packages/rs-dapi-client/Cargo.toml`, `backon` will not work without the `tokio-sleep` feature in our setup, so it's unnecessary to declare `tokio-sleep` as a separate feature in the `[features]` section.
Applied to files:
packages/rs-drive/Cargo.toml
📚 Learning: 2025-03-11T09:39:23.071Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2489
File: packages/rs-dpp/Cargo.toml:32-32
Timestamp: 2025-03-11T09:39:23.071Z
Learning: In the Dash Platform project, dependencies are currently managed using Git repository references with tags (repo+tag format in Cargo.toml) rather than published crates, as the team is not currently publishing crates to crates.io.
Applied to files:
packages/rs-drive/Cargo.tomlpackages/rs-platform-version/Cargo.toml
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:105-105
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In `run_block_proposal` in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, when retrieving `last_block_time_ms`, it's acceptable to use `platform_state` instead of `block_platform_state`, even after updating the protocol version.
Applied to files:
packages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rspackages/rs-drive-abci/src/query/address_funds/addresses_branch_state/v0/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/mod.rs
📚 Learning: 2024-11-22T08:19:14.448Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2345
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:93-99
Timestamp: 2024-11-22T08:19:14.448Z
Learning: In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`, the `insert_contract` method requires an owned `BlockInfo`, so cloning `block_info` is necessary when calling it.
Applied to files:
packages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rspackages/rs-drive-abci/src/query/address_funds/addresses_branch_state/v0/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/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-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rspackages/rs-drive-abci/src/query/address_funds/addresses_branch_state/v0/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/mod.rspackages/rs-platform-version/Cargo.toml
📚 Learning: 2024-10-06T16:18:07.994Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:119-120
Timestamp: 2024-10-06T16:18:07.994Z
Learning: In the `run_block_proposal` function in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, it's acceptable to pass `platform_state` to `perform_events_on_first_block_of_protocol_change`, even if `block_platform_state` has been updated.
Applied to files:
packages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rspackages/rs-drive-abci/src/query/address_funds/addresses_branch_state/v0/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/mod.rs
📚 Learning: 2024-10-22T10:53:12.111Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/dapi_client.rs:137-139
Timestamp: 2024-10-22T10:53:12.111Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when passing data into asynchronous code, ensure that data structures are `Send + Sync`. Using `Arc<AtomicUsize>` is necessary for the retry counter.
Applied to files:
packages/rs-sdk-ffi/src/address_sync/types.rspackages/rs-sdk/src/platform/address_sync/types.rs
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Applies to **/swift-sdk/**/*.swift : Swift code should properly wrap FFI functions and maintain type safety across the Swift/Rust boundary
Applied to files:
packages/rs-sdk-ffi/src/address_sync/types.rspackages/rs-sdk-ffi/src/address_sync/mod.rspackages/rs-sdk-ffi/src/address_sync/provider.rs
📚 Learning: 2025-10-09T15:59:28.329Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs:181-187
Timestamp: 2025-10-09T15:59:28.329Z
Learning: In `packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs`, the maintainer requires logging full state transition bytes (`tx_bytes = hex::encode(st_bytes)`) at debug level when a state transition passes CheckTx but is removed from the block by the proposer, to facilitate debugging of this rare edge case.
Applied to files:
packages/rs-drive-abci/src/query/address_funds/addresses_branch_state/v0/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-drive-abci/src/query/address_funds/addresses_branch_state/v0/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/mod.rs
📚 Learning: 2024-10-03T11:51:06.980Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-03T11:51:06.980Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Applied to files:
packages/rs-drive-abci/src/query/address_funds/addresses_branch_state/v0/mod.rs
🧬 Code graph analysis (5)
packages/rs-drive-proof-verifier/src/proof.rs (2)
packages/rs-drive/src/verify/address_funds/verify_address_funds_trunk_query/mod.rs (1)
verify_address_funds_trunk_query(31-49)packages/rs-drive-proof-verifier/src/verify.rs (1)
verify_tenderdash_proof(31-119)
packages/rs-sdk/src/sdk.rs (2)
packages/wasm-sdk/src/queries/mod.rs (1)
metadata(278-280)packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (1)
ResponseMetadata(57-84)
packages/rs-dapi-client/src/transport/grpc.rs (1)
packages/rs-drive-abci/src/query/service.rs (2)
get_addresses_trunk_state(832-842)get_addresses_branch_state(844-854)
packages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/mod.rs (1)
packages/rs-drive/src/error/mod.rs (4)
from(80-82)from(86-88)from(92-94)from(98-100)
packages/rs-sdk-ffi/src/address_sync/provider.rs (2)
packages/rs-sdk-ffi/src/address_sync/types.rs (1)
empty(151-156)packages/rs-sdk-ffi/src/context_provider.rs (1)
provider(37-39)
⏰ 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). (20)
- GitHub Check: Rust packages (drive-abci) / Detect immutable structure changes
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (drive) / Detect immutable structure changes
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (rs-sdk-ffi) / Linting
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-sdk-ffi) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Detect immutable structure changes
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (35)
packages/rs-drive/Cargo.toml (1)
55-60: LGTM! Local path dependencies for grovedb crates.The migration from git-based references to local path-based dependencies for grovedb crates is appropriate for local development and testing. This enables development against a local grovedb checkout while maintaining the same optional and default-features configuration.
Based on learnings, grovedb dependencies are intentionally managed at specific versions with plans for coordinated updates.
packages/rs-platform-version/Cargo.toml (1)
14-14: LGTM! Consistent local path dependency migration.The grovedb-version dependency migration to local path is consistent with the broader pattern of using local grovedb crates for development, as seen in rs-drive/Cargo.toml.
packages/rs-dapi-client/src/transport/grpc.rs (1)
645-661: LGTM! Standard transport implementations for address state queries.The new gRPC transport implementations for
GetAddressesTrunkStateandGetAddressesBranchStatefollow the established pattern used throughout this file. The use of default request settings is appropriate for these query operations, consistent with similar address-related endpoints likeget_address_infoandget_addresses_infos.packages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/mod.rs (2)
9-9: LGTM! Added GroveDBToUse support.The import enables the new checkpoint-based query functionality.
15-52: LGTM! Well-documented API extension for checkpoint support.The addition of the
grove_db_to_useparameter enables querying from different GroveDB instances (current, latest checkpoint, or specific checkpoint). The documentation clearly describes the new parameter and its purpose, and the version dispatch correctly propagates it to the v0 implementation.packages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/mod.rs (2)
1-9: LGTM! Necessary imports for checkpoint support.The new imports enable checkpoint-based GroveDB selection and error handling.
11-59: LGTM! Well-structured checkpoint-based query implementation.The implementation correctly handles three GroveDB sources:
- Current: Standard path using the main grove instance
- LatestCheckpoint: Retrieves the most recent checkpoint
- Checkpoint(height): Retrieves a specific checkpoint by height
Key strengths:
- Consistent cost tracking across all branches
- Appropriate error handling for missing checkpoints
- Clean match-based branching logic
- Each path follows the same prove_branch_chunk → push cost → return pattern
packages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/mod.rs (3)
9-9: LGTM! Required import for checkpoint support.
12-60: LGTM! Well-documented API extension for checkpoint-based proofs.The addition of the
checkpoint_heightparameter enables querying address funds at specific checkpoints or the latest checkpoint. The documentation clearly explains:
- The parameter's purpose (consistency across trunk/branch queries)
- When to use it (should match trunk query response height)
- The new error condition (CheckpointNotFound)
The version dispatch correctly propagates the new parameter.
62-113: LGTM! Consistent parameter addition to operations variant.The
checkpoint_heightparameter is consistently added to the operations-tracking variant with the same documentation pattern as the main method.packages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rs (2)
1-9: LGTM! Required imports for checkpoint functionality.
10-68: LGTM! Clean checkpoint-to-GroveDBToUse translation.The implementation correctly:
- Adds
checkpoint_heightparameter to both method variants- Translates the optional checkpoint height to
GroveDBToUseenum:
Some(height)→GroveDBToUse::Checkpoint(height)for specific checkpointNone→GroveDBToUse::LatestCheckpointfor most recent- Passes
platform_version.drive(notplatform_version) togrove_get_proved_branch_chunk_query, which is correct since that method expects&DriveVersionThe depth validation logic remains unchanged and appropriate.
packages/rs-sdk-ffi/src/lib.rs (2)
8-8: LGTM! New address_sync module added.The module declaration follows the established pattern in this file.
37-41: LGTM! Consistent public re-exports.The re-export of
address_syncfollows the established pattern of re-exporting all modules at the crate root, making the FFI surface accessible to external consumers.packages/rs-sdk/src/platform.rs (1)
8-8: Exposeaddress_syncmodule looks correctPublicly re‑exporting the new
address_syncmodule here is consistent with the existing platform surface and keeps SDK consumers’ import paths coherent.packages/rs-sdk/src/mock/requests.rs (1)
35-41: ImportingGroveTrunkQueryResultinto mocks is coherentBringing
GroveTrunkQueryResultinto this mocking module aligns with the new FromProof support and keeps all MockResponse impls centralized.packages/dapi-grpc/protos/platform/v0/platform.proto (1)
2019-2024: Checkpoint height field is a clean, additive extensionAdding
optional uint64 checkpoint_height = 3;toGetAddressesBranchStateRequestV0is backward compatible and documents the coupling to trunk metadata clearly. This wires well into the updated Drive branch‑proof APIs that take an optional checkpoint height.packages/rs-drive-proof-verifier/src/proof.rs (2)
54-54: Import aligns with new trunk-state verificationBringing
GroveTrunkQueryResultinto scope here is expected given the newFromProofimplementation below and matches how other Drive/Grove types are imported.
944-971: Trunk-stateFromProofimplementation follows existing verification patternThe
FromProof<platform::GetAddressesTrunkStateRequest> for GroveTrunkQueryResultimplementation:
- Extracts
ProofandResponseMetadata,- Delegates Grovedb verification to
Drive::verify_address_funds_trunk_query,- Runs
verify_tenderdash_proof,- Returns the verified
GroveTrunkQueryResultplus metadata/proof.This mirrors other FromProof implementations (identities, balances, etc.) and looks correct for integrating trunk proofs into the verifier.
packages/rs-sdk/src/platform/address_sync/types.rs (4)
6-17: Address key/index aliases are reasonable and flexibleUsing
Vec<u8>for keys andu32for indices keeps the types generic enough for different wallet schemes while still documenting the intended 32‑byte key size in comments.
18-55:AddressSyncConfigdefaults and semantics look sensibleThe configuration fields (privacy count, max concurrent requests, iteration cap) are clearly documented and the
Defaultimpl matches the documented values (32, 10, 50). This should be a good starting point for most callers.
57-106:AddressSyncResultstructure and helpers are straightforwardThe result type cleanly separates found vs absent addresses, tracks the highest found index, and exposes convenience methods (
new,total_balance,non_zero_count). The customDefaultdelegating tonew()keeps construction consistent.
107-144: Metrics API is minimal but useful
AddressSyncMetricstracks trunk/branch counts, elements, bytes, and iterations, with helpers for total query count and average proof size. Theaverage_proof_bytesguard for zero queries avoids division by zero and is clippy‑clean.packages/rs-sdk/src/sdk.rs (2)
285-299: Exposingverify_response_metadataas public is a good integration pointMaking
verify_response_metadatapublic allows higher‑level components (like the new address sync flows or external callers) to reuse the SDK’s staleness checks instead of duplicating height/time validation logic. The implementation remains unchanged and still respects the configured tolerances.
635-665:verify_metadata_timevisibility change is safe and keeps behavior intactPromoting
verify_metadata_timetopub(crate)doesn’t alter behavior and still backs the existing tests andverify_response_metadatapath. The use ofabs_diffwithu64metadata andnow_msis correct, and stale vs valid logging remains unchanged.packages/rs-drive-abci/src/query/address_funds/addresses_branch_state/v0/mod.rs (1)
12-25: Checkpoint height plumbed correctly into branch-state queryDestructuring
checkpoint_heightfromGetAddressesBranchStateRequestV0and forwarding it toself.drive.prove_address_funds_branch_queryis the expected glue for checkpoint‑aware branch proofs. The rest of the query/response handling is unchanged.packages/rs-sdk/src/platform/address_sync/provider.rs (1)
1-49: AddressProvider trait cleanly abstracts wallet-specific logicThe trait documentation and example explain the intended gap‑limit behaviour and pending‑address lifecycle well, and the methods cover all the hooks the sync engine needs (supply addresses, react to found/absent, report highest index). Marking it
Sendis appropriate for async/address‑sync usage.packages/rs-sdk/src/platform/address_sync/tracker.rs (1)
1-131: Well-structured tracking implementation.The
KeyLeafTrackerprovides a clean abstraction for managing the key-to-leaf mappings with proper reference counting. The use ofsaturating_subprevents underflow, and the API is well-documented with clear method purposes.packages/rs-sdk-ffi/src/address_sync/types.rs (2)
66-81: Raw pointer types lackSend/Syncimpls - verify thread safety requirements.
DashSDKFoundAddresscontains raw*mut u8pointers. If these types are used across threads (e.g., passed between async tasks or FFI callbacks on different threads), explicitSend/Syncimplementations with appropriate safety documentation would be needed.The current design appears correct for single-threaded FFI usage, but verify this matches the intended usage pattern.
1-157: FFI types are well-designed for C interoperability.The types follow proper FFI conventions with
#[repr(C)], clear documentation of field semantics, and appropriate use of raw pointers with length fields. TheDefaultimplementations provide sensible values.packages/rs-sdk/src/platform/address_sync/mod.rs (1)
94-198: Well-designed privacy-preserving synchronization workflow.The implementation correctly handles:
- Iterative trunk/branch query strategy
- Privacy adjustment for small subtrees
- Parallel branch query execution with concurrency limits
- Metrics tracking for observability
- Gap limit behavior with provider callbacks
The documentation is comprehensive and the code follows Rust idioms.
packages/rs-sdk-ffi/src/address_sync/mod.rs (4)
160-225: Memory allocation inconvert_sync_resultis correctly paired withdash_sdk_address_sync_result_free.The allocation pattern using
Box::into_raw(into_boxed_slice())for arrays and correspondingBox::from_raw(slice::from_raw_parts_mut(...))for deallocation is consistent. The key data is also properly allocated withBox::into_raw(into_boxed_slice())and freed in the matching pattern.
297-312: Provider cleanup correctly handles the destroy callback.The
dash_sdk_address_provider_freefunction properly:
- Handles null input
- Checks for null vtable before dereferencing
- Invokes the optional
destroycallback for custom cleanupThis follows good FFI patterns.
70-76: Blocking async execution is appropriate for FFI.The use of
runtime.block_on()to execute the asyncsync_address_balancesis the correct pattern for exposing async Rust code to C/FFI consumers. Callers should be aware this is a blocking call.
1-376: Comprehensive FFI bindings with proper memory management.The module provides a complete FFI surface for address synchronization with:
- Clear safety documentation for all public functions
- Consistent null pointer handling
- Proper memory lifecycle management
- Both raw-pointer and result-based error handling variants
- Utility functions for querying results
The naming follows the
dash_sdk_*prefix convention as per coding guidelines for Platform functions.
| pub unsafe extern "C" fn dash_sdk_pending_address_list_free(list: *mut DashSDKPendingAddressList) { | ||
| if list.is_null() { | ||
| return; | ||
| } | ||
|
|
||
| let list = Box::from_raw(list); | ||
| if !list.addresses.is_null() && list.count > 0 { | ||
| // Only free the addresses array, not the key data (which is owned by caller) | ||
| let _ = Box::from_raw(std::slice::from_raw_parts_mut(list.addresses, list.count)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Incorrect memory deallocation pattern for array.
Using Box::from_raw(std::slice::from_raw_parts_mut(...)) to free an array is incorrect. Box<[T]> and raw slices have different memory layouts. If the array was allocated via Vec::into_boxed_slice() then Box::into_raw(), this would work, but the current pattern is fragile and may cause undefined behavior depending on how the C caller allocated the memory.
Since the doc comment states "if the caller allocated the list dynamically," consider clarifying what allocation pattern is expected, or use Vec::from_raw_parts for deallocation.
🔎 Suggested fix
#[no_mangle]
pub unsafe extern "C" fn dash_sdk_pending_address_list_free(list: *mut DashSDKPendingAddressList) {
if list.is_null() {
return;
}
let list = Box::from_raw(list);
if !list.addresses.is_null() && list.count > 0 {
// Only free the addresses array, not the key data (which is owned by caller)
- let _ = Box::from_raw(std::slice::from_raw_parts_mut(list.addresses, list.count));
+ // Drop the Vec to deallocate the addresses array
+ let _ = Vec::from_raw_parts(list.addresses, list.count, list.count);
}
}Note: This assumes the array was allocated with the same allocator. If the C side allocates memory differently, this function should not be used, and the C caller should handle deallocation instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub unsafe extern "C" fn dash_sdk_pending_address_list_free(list: *mut DashSDKPendingAddressList) { | |
| if list.is_null() { | |
| return; | |
| } | |
| let list = Box::from_raw(list); | |
| if !list.addresses.is_null() && list.count > 0 { | |
| // Only free the addresses array, not the key data (which is owned by caller) | |
| let _ = Box::from_raw(std::slice::from_raw_parts_mut(list.addresses, list.count)); | |
| } | |
| } | |
| pub unsafe extern "C" fn dash_sdk_pending_address_list_free(list: *mut DashSDKPendingAddressList) { | |
| if list.is_null() { | |
| return; | |
| } | |
| let list = Box::from_raw(list); | |
| if !list.addresses.is_null() && list.count > 0 { | |
| // Only free the addresses array, not the key data (which is owned by caller) | |
| // Drop the Vec to deallocate the addresses array | |
| let _ = Vec::from_raw_parts(list.addresses, list.count, list.count); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/rs-sdk-ffi/src/address_sync/provider.rs around lines 184 to 194, the
current deallocation uses Box::from_raw on a raw slice which is an
unsafe/incorrect layout assumption; instead reconstruct the allocation as a Vec
from raw parts and let it drop (e.g., Vec::from_raw_parts(list.addresses as *mut
T, list.count, list.count)) so the allocator and layout match, and update the
doc comment to specify the expected allocation pattern/ownership (or require the
C caller to free if different allocator is used).
| // Check if provider has extended pending addresses (gap limit behavior) | ||
| for (index, key) in provider.pending_addresses() { | ||
| if !key_to_index.contains_key(&key) { | ||
| key_to_index.insert(key.clone(), index); | ||
| // New key needs to be traced - it will be picked up in next iteration | ||
| // For now, trace it using the trunk result if possible | ||
| if let Some((leaf_key, info)) = trunk_result.trace_key_to_leaf(&key) { | ||
| tracker.add_key(key, leaf_key, info); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Newly added addresses may not be synchronized if trunk lacks their path.
When the provider extends pending addresses (gap limit behavior), new keys are traced via trunk_result.trace_key_to_leaf(). If the trunk doesn't contain the path to the new key's location (e.g., if it falls outside the trunk's depth), the key is silently not added to the tracker and won't be synchronized.
Consider handling this case by either re-executing a trunk query or documenting this limitation.
🤖 Prompt for AI Agents
In packages/rs-sdk/src/platform/address_sync/mod.rs around lines 173 to 183,
newly added provider pending addresses are skipped if
trunk_result.trace_key_to_leaf(&key) returns None (trunk lacks the path),
causing those keys to never be tracked; modify the logic so that when
trace_key_to_leaf returns None you do not silently drop the key — instead either
(a) enqueue the key in a “pending_unresolved” set so it will be retried after
the next trunk query, or (b) trigger/re-execute a trunk query with increased
depth and retry tracing immediately, and only skip when retry fails; ensure the
key_to_index insertion remains and add a clear code-path that records unresolved
keys so they are synchronized later.
| fn extract_balance_from_element(element: &Element) -> u64 { | ||
| match element { | ||
| Element::SumItem(value, _) => *value as u64, | ||
| _ => 0, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and the specific function
cat -n packages/rs-sdk/src/platform/address_sync/mod.rs | sed -n '460,475p'Repository: dashpay/platform
Length of output: 655
🏁 Script executed:
# Search for Element and SumItem definitions to understand the structure
rg -n "enum Element" packages/rs-sdk/src/ -A 10 --type rustRepository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Check how SumItem values are created and used for balances
rg -n "SumItem" packages/rs-sdk/src/platform/address_sync/ --type rust -B 2 -A 2Repository: dashpay/platform
Length of output: 912
🏁 Script executed:
# Look for how balance values are inserted into the tree
rg -n "extract_balance_from_element" packages/rs-sdk/src/ --type rust -B 5 -A 5Repository: dashpay/platform
Length of output: 4041
🏁 Script executed:
# Search for where balances are stored/inserted to see if there's validation
rg -n "address.*balance\|balance.*tree" packages/rs-sdk/src/platform/address_sync/ --type rust -i | head -30Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Find where Element is defined
fd -e rs -x grep -l "enum Element" {} \; | head -20Repository: dashpay/platform
Length of output: 121
🏁 Script executed:
# Search for SumItem creation in the codebase to see if there's validation
rg -n "SumItem\(" --type rust | head -30Repository: dashpay/platform
Length of output: 4873
🏁 Script executed:
# Check how balance values flow into the address tree
rg -n "address.*fund\|fund.*tree" packages/rs-sdk/src/platform/ --type rust -i -C 2 | head -50Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Look for balance validation or assertions
rg -n "balance.*<.*0\|balance.*negative\|assert.*balance" packages/rs-sdk/src/platform/ --type rust -i | head -20Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Look at the explicit >= 0 check in verify_token_pre_programmed_distributions
cat -n packages/rs-drive/src/verify/tokens/verify_token_pre_programmed_distributions/v0/mod.rs | sed -n '75,90p'Repository: dashpay/platform
Length of output: 907
🏁 Script executed:
# Also check the grove_get_raw_value_u64 for comparison
cat -n packages/rs-drive/src/util/grove_operations/grove_get_raw_value_u64_from_encoded_var_vec/v0/mod.rs | sed -n '30,45p'Repository: dashpay/platform
Length of output: 940
🏁 Script executed:
# Check if there are any other similar balance extraction functions
rg -n "Element::SumItem.*as u64" --type rust -B 2 -A 2Repository: dashpay/platform
Length of output: 1281
🏁 Script executed:
# Look at identity balance specifically to see if negatives are handled
rg -n "identity_balance\|identity.*balance" packages/rs-drive/src/verify/identity/ --type rust -B 3 -A 3 | head -40Repository: dashpay/platform
Length of output: 42
Add explicit validation for negative SumItem values to match patterns used elsewhere in the codebase.
Element::SumItem stores an i64, and the cast *value as u64 will wrap negative values to large positive numbers. While address balances should never be negative in normal operation, the codebase shows that negative SumItem values are possible (see packages/rs-drive/src/verify/tokens/verify_token_pre_programmed_distributions/v0/mod.rs:80-84, which explicitly checks for and rejects negative values as data corruption). Consider adding a defensive check here to match that pattern:
fn extract_balance_from_element(element: &Element) -> u64 {
match element {
- Element::SumItem(value, _) => *value as u64,
+ Element::SumItem(value, _) => {
+ debug_assert!(*value >= 0, "Unexpected negative balance: {}", value);
+ (*value).max(0) as u64
+ }
_ => 0,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn extract_balance_from_element(element: &Element) -> u64 { | |
| match element { | |
| Element::SumItem(value, _) => *value as u64, | |
| _ => 0, | |
| } | |
| } | |
| fn extract_balance_from_element(element: &Element) -> u64 { | |
| match element { | |
| Element::SumItem(value, _) => { | |
| debug_assert!(*value >= 0, "Unexpected negative balance: {}", value); | |
| (*value).max(0) as u64 | |
| } | |
| _ => 0, | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/rs-sdk/src/platform/address_sync/mod.rs around lines 464-469, the
current cast of Element::SumItem i64 to u64 will wrap negative values; add an
explicit check for negative values used elsewhere in the codebase: if *value < 0
treat it as data corruption (log or report the error) and return 0 (or propagate
an Err if you prefer to change the signature), otherwise cast to u64 and return
the value.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs (3)
817-886: Consider extracting common test setup.This test duplicates significant setup code from
run_chain_address_transitions_with_checkpoints(lines 428-606). Both tests use nearly identicalNetworkStrategyandPlatformConfigwith only minor differences (block_signingandblock_commit_signature_verificationflags).Consider extracting a helper function to reduce duplication:
fn create_address_transfer_strategy() -> NetworkStrategy { NetworkStrategy { strategy: Strategy { start_contracts: vec![], operations: vec![ Operation { op_type: OperationType::AddressTransfer( dash_to_credits!(5)..=dash_to_credits!(5), 1..=4, Some(0.2), None, ), frequency: Frequency { times_per_block_range: 1..3, chance_per_block: None, }, }, Operation { op_type: OperationType::AddressFundingFromCoreAssetLock( dash_to_credits!(20)..=dash_to_credits!(20), ), frequency: Frequency { times_per_block_range: 1..3, chance_per_block: None, }, }, ], start_identities: StartIdentities::default(), start_addresses: StartAddresses::default(), identity_inserts: IdentityInsertInfo { frequency: Frequency { times_per_block_range: 1..2, chance_per_block: None, }, ..Default::default() }, identity_contract_nonce_gaps: None, signer: None, }, total_hpmns: 100, extra_normal_mns: 0, validator_quorum_count: 24, chain_lock_quorum_count: 24, upgrading_info: None, proposer_strategy: Default::default(), rotate_quorums: false, failure_testing: None, query_testing: None, verify_state_transition_results: true, sign_instant_locks: true, ..Default::default() } } fn create_checkpoint_platform_config(enable_signing: bool) -> PlatformConfig { PlatformConfig { validator_set: ValidatorSetConfig::default_100_67(), chain_lock: ChainLockConfig::default_100_67(), instant_lock: InstantLockConfig::default_100_67(), execution: ExecutionConfig { verify_sum_trees: true, ..Default::default() }, block_spacing_ms: 180000, // 3 mins testing_configs: PlatformTestConfig { block_signing: enable_signing, block_commit_signature_verification: enable_signing, disable_checkpoints: false, ..PlatformTestConfig::default_minimal_verifications() }, ..Default::default() } }
812-813: Consider moving imports to module level.While function-scoped imports are valid in Rust, it's more conventional to declare imports at the module level for better discoverability and consistency with the rest of the file.
🔎 Proposed refactor
Move these imports to the top of the file with the other imports:
use dpp::state_transition::StateTransition; use drive::drive::Drive; +use drive::grovedb::GroveTrunkQueryResult; use drive_abci::config::{ ChainLockConfig, ExecutionConfig, InstantLockConfig, PlatformConfig, PlatformTestConfig, ValidatorSetConfig, }; +use dpp::dashcore::Network;And remove from the function:
#[test] fn run_chain_address_transitions_with_proof_signature_verification() { - use dpp::dashcore::Network; - use drive::grovedb::GroveTrunkQueryResult; - drive_abci::logging::init_for_tests(LogLevel::Debug);
891-899: Remove unnecessary clone.The
configis cloned on line 895 but not used afterward. The clone is unnecessary sincerun_chain_for_strategydoes not consume the config.🔎 Proposed fix
let outcome = run_chain_for_strategy( &mut platform, 13, strategy, - config.clone(), + config, 15, &mut None, &mut None, );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/rs-drive-abci/Cargo.tomlpackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rspackages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/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-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.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-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Applies to **/swift-sdk/**/*.{swift,h,m,c} : iOS SDK must combine both Core (SPV wallet) and Platform (identity/documents) functionality in the unified SDK with proper function naming: `dash_core_sdk_*` prefix for Core functions, `dash_sdk_*` prefix for Platform functions, and `dash_unified_sdk_*` prefix for unified functions
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 Learning: 2024-12-05T09:29:38.918Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2375
File: packages/rs-drive-abci/Cargo.toml:61-63
Timestamp: 2024-12-05T09:29:38.918Z
Learning: In the `drive-abci` package, avoid adding unused dependencies like `hashbrown` to `Cargo.toml`. The team relies on CI to detect dependency version issues.
Applied to files:
packages/rs-drive-abci/Cargo.toml
📚 Learning: 2025-01-19T07:36:46.042Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2431
File: packages/rs-drive/Cargo.toml:55-60
Timestamp: 2025-01-19T07:36:46.042Z
Learning: The grovedb dependencies in packages/rs-drive/Cargo.toml and related files are intentionally kept at specific revisions rather than using the latest stable version, with plans to update them at a later time.
Applied to files:
packages/rs-drive-abci/Cargo.tomlpackages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/mod.rs
📚 Learning: 2025-09-03T16:37:11.605Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2756
File: packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:11-11
Timestamp: 2025-09-03T16:37:11.605Z
Learning: In packages/rs-dpp/Cargo.toml, the abci feature already includes core_rpc_client, and core_rpc_client is defined as ["dep:dashcore-rpc"]. This means rs-drive-abci can access dashcore-rpc types through dpp when using the abci feature.
Applied to files:
packages/rs-drive-abci/Cargo.toml
📚 Learning: 2025-01-24T07:54:55.723Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2440
File: packages/rs-drive-abci/Cargo.toml:76-76
Timestamp: 2025-01-24T07:54:55.723Z
Learning: In the drive-abci package, bls-signatures is intentionally declared twice:
1. As an optional dependency in [dependencies] to make it an optional feature in production
2. As a non-optional dependency in [dev-dependencies] to ensure it's always available for tests
Applied to files:
packages/rs-drive-abci/Cargo.toml
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Use `rs-drive-proof-verifier` for cryptographic proof verification
Applied to files:
packages/rs-drive-abci/Cargo.toml
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2206
File: packages/rs-drive-abci/tests/strategy_tests/main.rs:89-89
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In tests located in the `packages/rs-drive-abci/tests` directory, the team prefers to use specific protocol versions (e.g., `PROTOCOL_VERSION_1`) instead of `PROTOCOL_VERSION_LATEST`.
Applied to files:
packages/rs-drive-abci/Cargo.toml
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2206
File: packages/rs-drive-abci/tests/strategy_tests/main.rs:1162-1162
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In the Rust test file `packages/rs-drive-abci/tests/strategy_tests/main.rs`, specific protocol versions like `PROTOCOL_VERSION_1` are intentionally used in tests instead of `PROTOCOL_VERSION_LATEST`.
Applied to files:
packages/rs-drive-abci/Cargo.tomlpackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 Learning: 2025-03-07T12:37:44.555Z
Learnt from: ogabrielides
Repo: dashpay/platform PR: 2486
File: packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/v0/mod.rs:1-29
Timestamp: 2025-03-07T12:37:44.555Z
Learning: GroveDB operations like `insert()` return a tuple that includes a cost value which needs to be unwrapped with `.unwrap()` before error handling with `.map_err()`.
Applied to files:
packages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/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-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs:148-151
Timestamp: 2024-11-20T16:05:40.200Z
Learning: In `packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs`, when converting public keys from `QuorumForSavingV0` to `VerificationQuorum`, it's acceptable to use `.expect()` for public key conversion, as the conversion has been verified and panics are acceptable in this context.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.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-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 Learning: 2024-10-21T01:03:42.458Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:299-299
Timestamp: 2024-10-21T01:03:42.458Z
Learning: In the `test_serialize_deserialize_validator_set_v0` test within `packages/rs-dpp/src/core_types/validator_set/v0/mod.rs`, deterministic BLS keys cannot be easily used; therefore, using `BlsPublicKey::generate()` is acceptable.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 Learning: 2024-10-06T16:18:07.994Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:119-120
Timestamp: 2024-10-06T16:18:07.994Z
Learning: In the `run_block_proposal` function in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, it's acceptable to pass `platform_state` to `perform_events_on_first_block_of_protocol_change`, even if `block_platform_state` has been updated.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 Learning: 2024-10-06T16:17:34.571Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:105-105
Timestamp: 2024-10-06T16:17:34.571Z
Learning: In `run_block_proposal` in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, when retrieving `last_block_time_ms`, it's acceptable to use `platform_state` instead of `block_platform_state`, even after updating the protocol version.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
🧬 Code graph analysis (1)
packages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/mod.rs (1)
packages/rs-drive/src/error/mod.rs (4)
from(80-82)from(86-88)from(92-94)from(98-100)
⏰ 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). (20)
- GitHub Check: Rust packages (rs-sdk-ffi) / Formatting
- GitHub Check: Rust packages (rs-sdk-ffi) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Formatting
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (rs-sdk-ffi) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / Unused dependencies
- GitHub Check: Rust crates security audit
🔇 Additional comments (6)
packages/rs-drive/src/util/grove_operations/grove_get_proved_branch_chunk_query/v0/mod.rs (1)
1-59: LGTM! Clean implementation of checkpoint-aware proof retrieval.The branching logic for
GroveDBToUseis well-structured with consistent cost tracking across all three paths. Error handling appropriately distinguishes between "no checkpoints available" and "specific checkpoint not found" cases. The documentation clearly describes the new parameter.packages/rs-drive-abci/Cargo.toml (1)
98-98: No action needed:drive-proof-verifieris correctly placed in[dev-dependencies]only.Verification confirms that
drive-proof-verifieris intentionally declared only in the[dev-dependencies]section. Production code has no imports of this crate; the only reference found is a tracing log level string in test configuration. This is the correct design for a test-only dependency.packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs (4)
12-14: LGTM!The new imports appropriately support the proof verification test infrastructure, including quorum key management, context provision, and proof verification via FromProof.
Also applies to: 16-16, 24-24, 27-27, 29-30
35-62: LGTM!The
TestContextProviderstruct and constructor are well-designed test scaffolding. The use of.expect()for the public key conversion is acceptable in test code, as the conversion should always succeed for valid test quorum data.
64-101: LGTM!The
ContextProviderimplementation provides the minimal test infrastructure needed. Theget_quorum_public_keymethod correctly looks up keys from the test quorum map, while the other methods appropriately return stub values (None or 1) since they're not needed for this test scenario.
958-1018: LGTM! Comprehensive proof signature verification.The core test logic is excellent. It properly:
- Creates a
TestContextProviderwith quorum public keys from the test execution- Verifies proof signatures using
FromProof::maybe_from_proof_with_metadata- Validates trunk query results (elements, leaf_keys, chunk_depths)
- Verifies proof structure (quorum_hash length, signature length, quorum_type)
- Includes helpful logging for debugging
The assertion on line 968 correctly identifies this as "the key test" for proof signature verification, and the detailed verification steps that follow provide good coverage of the proof validation pathway.
| #[test] | ||
| fn run_chain_address_transitions_with_proof_signature_verification() { |
There was a problem hiding this comment.
Rename test to follow naming convention.
Per coding guidelines, test names should start with should_. Consider renaming to something like should_verify_proof_signatures_when_querying_address_trunk_state_with_checkpoints.
🔎 Proposed fix
- #[test]
- fn run_chain_address_transitions_with_proof_signature_verification() {
+ #[test]
+ fn should_verify_proof_signatures_when_querying_address_trunk_state_with_checkpoints() {As per coding guidelines, test names should start with 'should …'
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn run_chain_address_transitions_with_proof_signature_verification() { | |
| #[test] | |
| fn should_verify_proof_signatures_when_querying_address_trunk_state_with_checkpoints() { |
🤖 Prompt for AI Agents
In packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
around lines 810-811, the test function name
run_chain_address_transitions_with_proof_signature_verification does not follow
the project's test naming convention (must start with should_); rename the
function to a descriptive should_* name (for example
should_verify_proof_signatures_when_querying_address_trunk_state_with_checkpoints),
update any internal references or documentation that call the old name, and run
cargo test to confirm the renamed test compiles and executes.
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.