feat(platform): sdk support for platform addresses and checkpoint fix #2933
Conversation
✅ gRPC Query Coverage Report |
📝 WalkthroughWalkthroughThreads a new checkpoint_height through branch proofs and GroveDB selection, adds trunk-state proof handling, introduces checkpoint decision/creation in drive-abci, implements privacy-preserving address-balance sync (trunk + parallel branch queries) with Rust FFI, adds gRPC transport stubs, and bumps grovedb revisions. 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 rgba(235,245,255,0.9)
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 rgba(232,255,240,0.9)
Note over Sdk,BranchExec: Iterative branch queries (parallel, checkpoint selection)
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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs (1)
810-811: Rename test to follow naming convention.The test name should start with
should_per coding guidelines. Consider renaming toshould_verify_proof_signatures_when_querying_address_trunk_state_with_checkpoints.As per coding guidelines, test names should start with 'should …'
🧹 Nitpick comments (2)
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs (1)
64-101: Consider documenting the limited scope of stub methods.The
ContextProviderimplementation is correct for the address trunk query test. However, the stub implementations (get_data_contract,get_token_configuration,get_platform_activation_height) return minimal values that work for address queries but may cause silent failures if this provider is reused for tests involving contracts or tokens.🔎 Suggested documentation addition
/// A test ContextProvider that provides quorum public keys from a map of test quorums. -/// This is used to verify proof signatures in strategy tests. +/// This is used to verify proof signatures in strategy tests. +/// +/// **Note**: This provider only implements `get_quorum_public_key` fully. +/// Other methods return stub values suitable for address trunk queries only. +/// Do not reuse for tests requiring contracts, tokens, or specific activation heights. struct TestContextProvider { quorum_public_keys: BTreeMap<([u8; 32], u32), [u8; 48]>, }packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rs (1)
30-30: Consider removinginline(always)for a function with I/O.The
inline(always)attribute is aggressive for a function that loads checkpoints from storage. Since this function performs I/O operations (line 60), forced inlining may not provide benefits and could increase code size. Consider using#[inline]or removing the attribute entirely to let the compiler decide.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/rs-drive-abci/src/abci/handler/finalize_block.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rspackages/rs-drive-abci/src/platform_types/block_execution_outcome/v0/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v1.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v2.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v3.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v4.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v7.rspackages/rs-platform-version/src/version/mocks/v3_test.rs
💤 Files with no reviewable changes (1)
- packages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/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-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rspackages/rs-drive-abci/src/platform_types/block_execution_outcome/v0/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v2.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rspackages/rs-drive-abci/src/abci/handler/finalize_block.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v4.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v7.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v1.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rspackages/rs-platform-version/src/version/mocks/v3_test.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v3.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 (18)
📓 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-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-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rspackages/rs-drive-abci/src/platform_types/block_execution_outcome/v0/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v2.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v4.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v7.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v1.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rspackages/rs-platform-version/src/version/mocks/v3_test.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v3.rs
📚 Learning: 2025-05-28T16:22:26.334Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2644
File: packages/rs-drive/src/cache/system_contracts.rs:18-19
Timestamp: 2025-05-28T16:22:26.334Z
Learning: In packages/rs-drive/src/cache/system_contracts.rs, the `active_since_protocol_version` field in `ActiveSystemDataContract` struct is intentionally added for future use, not current implementation. QuantumExplorer confirmed this is "meant for later" when questioned about the `#[allow(unused)]` attribute.
Applied to files:
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v2.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v4.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v1.rspackages/rs-platform-version/src/version/mocks/v3_test.rs
📚 Learning: 2024-11-20T10:01:50.837Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs:94-94
Timestamp: 2024-11-20T10:01:50.837Z
Learning: In `packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs`, when converting with `PublicKey::try_from`, it's acceptable to use `.expect()` to handle potential conversion errors.
Applied to files:
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rs
📚 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-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rspackages/rs-platform-version/src/version/mocks/v3_test.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 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-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rspackages/rs-drive-abci/src/platform_types/block_execution_outcome/v0/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rspackages/rs-drive-abci/src/abci/handler/finalize_block.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v7.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.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-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rspackages/rs-drive-abci/src/platform_types/block_execution_outcome/v0/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rspackages/rs-drive-abci/src/abci/handler/finalize_block.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v7.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/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-abci/src/platform_types/block_execution_outcome/v0/mod.rspackages/rs-drive-abci/src/abci/handler/finalize_block.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.rs
📚 Learning: 2024-11-03T10:39:11.242Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2305
File: packages/rs-drive-abci/src/abci/handler/finalize_block.rs:81-0
Timestamp: 2024-11-03T10:39:11.242Z
Learning: In `packages/rs-drive-abci/src/abci/handler/finalize_block.rs`, the use of `.expect("commit transaction")` after `app.commit_transaction(platform_version)` is intentional due to the provided comment explaining its necessity. Do not flag this usage in future reviews.
Applied to files:
packages/rs-drive-abci/src/platform_types/block_execution_outcome/v0/mod.rspackages/rs-drive-abci/src/abci/handler/finalize_block.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.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/abci/handler/finalize_block.rspackages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.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-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/mod.rs
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_owner_identity/v1/mod.rs:19-30
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In the Rust file `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_owner_identity/v1/mod.rs`, within the `create_owner_identity_v1` function, the `add_public_keys` method of the `Identity` struct cannot fail and does not require explicit error handling.
Applied to files:
packages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/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/execution/engine/finalize_block_proposal/v0/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-09T00:22:57.778Z
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/execution/platform_events/block_end/update_state_cache/v0/mod.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.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 Rust for core platform components (Drive, DAPI server, DPP implementation)
Applied to files:
packages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.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: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
🧬 Code graph analysis (2)
packages/rs-drive-abci/src/execution/platform_events/block_end/mod.rs (3)
packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.rs (1)
should_checkpoint(20-39)packages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/mod.rs (1)
update_checkpoints(33-52)packages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rs (1)
create_grovedb_checkpoint(26-44)
packages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rs (1)
packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.rs (1)
should_checkpoint(20-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). (19)
- GitHub Check: Rust packages (rs-sdk-ffi) / Unused dependencies
- GitHub Check: Rust packages (rs-sdk-ffi) / Linting
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (rs-sdk-ffi) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Formatting
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Unused dependencies
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust crates security audit
🔇 Additional comments (28)
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rs (1)
135-135: LGTM! Field addition is well-placed.The new
should_checkpointfield is correctly positioned betweenvalidator_set_updateandupdate_checkpoints, which makes semantic sense for the checkpoint flow.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v7.rs (1)
121-121: LGTM! Checkpoint feature enabled for v7.The initialization of
should_checkpoint: Some(0)correctly enables the checkpoint feature for protocol version 11 (v7), as documented in the file comment on Line 15.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rs (1)
124-124: LGTM! Backward compatibility maintained for v5.The
should_checkpoint: Nonecorrectly disables the checkpoint feature for this version, maintaining backward compatibility.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v3.rs (1)
120-120: LGTM! Backward compatibility maintained for v3.The
should_checkpoint: Nonecorrectly disables the checkpoint feature for this version.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v4.rs (1)
120-120: LGTM! Backward compatibility maintained for v4.The
should_checkpoint: Nonecorrectly disables the checkpoint feature for this version.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rs (1)
122-122: LGTM! Backward compatibility maintained for v6.The
should_checkpoint: Nonecorrectly disables the checkpoint feature for v6, which was introduced for tokens (protocol version 9), while checkpoints are introduced later in v7.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v2.rs (1)
121-121: LGTM! Backward compatibility maintained for v2.The
should_checkpoint: Nonecorrectly disables the checkpoint feature for this version.packages/rs-platform-version/src/version/mocks/v3_test.rs (1)
155-155: Test mock correctly mirrors v3 behavior.The test mock initialization in
v3_test.rsat line 155 correctly reflects the production v3 configuration, withshould_checkpoint: None. All version files (v1 through v7) consistently include theshould_checkpointfield in theirDriveAbciBlockEndMethodVersionsstruct initialization.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v1.rs (1)
116-122: LGTM! Checkpoint field addition follows existing pattern.The addition of
should_checkpoint: Noneto theDriveAbciBlockEndMethodVersionsin v1 is consistent with the existingupdate_checkpoints: Nonefield and appropriately initializes the baseline version before the checkpoint feature is enabled in later versions.packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs (4)
12-30: LGTM! Imports properly support proof verification infrastructure.The new imports (Hash, QuorumHash, ContextProvider traits, BTreeMap, Arc, etc.) are all utilized in the TestContextProvider implementation and the new proof signature verification test.
35-62: LGTM! Test context provider constructor is well-structured.The
TestContextProvider::from_quorumsconstructor correctly builds a lookup map of quorum public keys from the test quorum info. The use of.expect()for the public key conversion is acceptable in test code, and the composite key(quorum_hash, quorum_type)properly supports multiple quorum types.
593-594: LGTM! Trunk query element count updated for checkpoint feature.The assertion update from 29 to 32 expected elements in trunk queries is consistent across both checkpoint tests and aligns with the broader checkpoint-aware trunk/branch query changes introduced in this PR.
Also applies to: 790-791
812-1019: LGTM! Comprehensive proof signature verification test.This test is well-structured and thoroughly validates the FromProof-based verification flow:
- Clear documentation explaining the test's purpose
- Proper setup with quorum signing enabled (
block_signing: true)- Uses
TestContextProviderto supply quorum public keys for verification- Key verification at lines 958-968 using
maybe_from_proof_with_metadatawith signature checking- Comprehensive assertions on metadata, trunk results, and proof structure
- Good debug logging for troubleshooting
The test correctly demonstrates that proof signatures can be verified using the
ContextProvidertrait andFromProofpattern.packages/rs-drive-abci/src/platform_types/block_execution_outcome/v0/mod.rs (1)
31-40: LGTM!The new
checkpoint_neededfield is well-documented and theFromimplementation correctly defaults it tofalse, which is appropriate since aSimpleValidationResultconversion represents an early return (typically an error case) where no checkpoint should be created.packages/rs-drive-abci/src/execution/engine/finalize_block_proposal/v0/mod.rs (1)
230-251: LGTM!The checkpoint decision is correctly computed before
block_execution_contextis consumed byblock_platform_state_owned(). The explicit comment on line 230 documenting this constraint is helpful for future maintainers. The conversion fromOption<CheckpointNeededInfo>toboolvia.is_some()is appropriate.packages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rs (1)
35-62: LGTM!The function has been properly refactored to focus solely on state cache updates, removing checkpoint-related logic. This separation of concerns aligns with the new architecture where checkpoints are created post-commit in the ABCI handler. The logic for updating validator set, block info, and persisting state remains correct.
packages/rs-drive-abci/src/abci/handler/finalize_block.rs (1)
99-102: Checkpoint creation correctly positioned after transaction commit.The checkpoint is created after the transaction is committed, ensuring it captures the committed state. The error is propagated, which will cause the node to restart (similar to commit failure behavior per existing patterns).
Consider whether a checkpoint creation failure should be fatal. If the main data is already committed, failing to create a checkpoint might be recoverable by retrying on the next block. However, if checkpoints are critical for SDK address synchronization, the current strict error handling may be appropriate.
packages/rs-drive-abci/src/execution/platform_events/block_end/mod.rs (1)
9-16: LGTM!The module structure is well-organized:
should_checkpointis appropriately scoped tocrate::executioncreate_grovedb_checkpointispub(crate)to allow access from the ABCI handler- The "legacy" comment on
update_checkpointsclearly communicates its rolepackages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/mod.rs (1)
26-44: LGTM!The version dispatch follows the established pattern in the codebase. Documentation correctly emphasizes that this must be called after transaction commit.
packages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rs (1)
31-75: LGTM on checkpoint creation logic.The checkpoint path construction, directory creation, GroveDB checkpoint creation, and platform state serialization are all handled correctly with appropriate error mapping.
One minor note:
#[inline(always)]on line 31 has minimal benefit for a function that performs I/O operations, but it's not harmful.packages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rs (1)
21-90: Verify whetherupdate_checkpoints_v0should still be active or if the flow has migrated to separate functions.The code comment labels
update_checkpointsas "legacy", but the function is still actively called. However,update_checkpoints_v0andcreate_grovedb_checkpoint_v0serve different purposes:update_checkpoints_v0combines checkpoint decision logic (viashould_checkpoint) with creation and returns a bool, whilecreate_grovedb_checkpoint_v0only creates checkpoints after commit, additionally persisting platform state to disk and updating thecheckpoint_platform_statescache.If both functions remain in use, there's a real inconsistency:
update_checkpoints_v0does not updatecheckpoint_platform_states, whilecreate_grovedb_checkpoint_v0does. This could cause the cache to become stale ifupdate_checkpoints_v0is the primary checkpoint creation path. Clarify whether the separation is intentional or if one should be deprecated.packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rs (5)
1-10: LGTM! Clean and necessary imports.All imports are appropriately scoped and used within the implementation.
12-20: LGTM! Well-designed data structure.The use of
Arc<BTreeMap<...>>forcurrent_checkpointsis efficient for shared ownership, and the struct clearly conveys the information needed for checkpoint creation.
36-50: LGTM! Robust configuration handling.The testing config feature gate and configuration validation (checking for zero values) provide good defensive programming to handle misconfigured or disabled checkpoints.
52-69: LGTM! Correct checkpoint interval logic.The arithmetic for computing
most_recent_checkpoint_interval_time(line 56-57) correctly floors the block time to the nearest checkpoint interval boundary. The decision logic (lines 63-69) ensures exactly one checkpoint per interval by checking that the last checkpoint preceded the current interval and the current block has reached or passed the interval boundary.
71-82: LGTM! Efficient result construction.Cloning the
Arc(line 73) is efficient as it only increments the reference count without copying the underlying checkpoint data.packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.rs (2)
1-10: LGTM! Clean module structure.The module declaration, re-export of
CheckpointNeededInfo, and imports follow standard Rust patterns for versioned implementations.
12-40: LGTM! Well-structured version dispatch.The version dispatch pattern follows best practices:
- Returns
Ok(None)when the checkpoint feature is not enabled for the platform version- Correctly delegates to
should_checkpoint_v0for version 0- Provides clear error messages with known versions for unsupported versions
The public visibility of the method is appropriate for a platform API entry point.
| let current_checkpoint_states = self.checkpoint_platform_states.load(); | ||
| let mut new_checkpoint_states = BTreeMap::new(); | ||
|
|
||
| // Add the current platform state for this new checkpoint | ||
| new_checkpoint_states.insert(block_height, Arc::clone(&platform_state)); | ||
|
|
||
| // Copy only the states for checkpoints we're keeping | ||
| for (height, state) in current_checkpoint_states.iter().skip(to_skip) { | ||
| new_checkpoint_states.insert(*height, Arc::clone(state)); | ||
| } |
There was a problem hiding this comment.
Potential inconsistency in checkpoint_platform_states pruning.
The to_skip value is calculated from current_checkpoints.len() (line 82-83), but it's also used to skip entries in current_checkpoint_states (line 117). If these two maps ever become out of sync (different heights/counts), the wrong states could be pruned.
Consider calculating to_skip independently for checkpoint_platform_states:
🔎 Proposed fix
// Update checkpoint_platform_states cache
let current_checkpoint_states = self.checkpoint_platform_states.load();
let mut new_checkpoint_states = BTreeMap::new();
+
+ // Calculate how many old checkpoint states to skip independently
+ let states_to_skip = current_checkpoint_states.len().saturating_sub(max_old_to_keep);
// Add the current platform state for this new checkpoint
new_checkpoint_states.insert(block_height, Arc::clone(&platform_state));
// Copy only the states for checkpoints we're keeping
- for (height, state) in current_checkpoint_states.iter().skip(to_skip) {
+ for (height, state) in current_checkpoint_states.iter().skip(states_to_skip) {
new_checkpoint_states.insert(*height, Arc::clone(state));
}🤖 Prompt for AI Agents
In
packages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rs
around lines 110 to 119, the code uses a to_skip value derived from
current_checkpoints to skip entries when copying from current_checkpoint_states,
which can prune the wrong entries if the two collections differ; change the
logic to compute the skip/count based on current_checkpoint_states itself (or
compute a number_to_keep and then skip
current_checkpoint_states.iter().take_while/skip accordingly) so that you prune
entries consistently from the same map you are iterating, ensuring you clone
only the most recent N states from checkpoint_platform_states rather than
relying on current_checkpoints length.
There was a problem hiding this comment.
@QuantumExplorer, understood! I'll leave the checkpoint pruning logic as is.
🧠 Learnings used
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.
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.
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.
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.
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.
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.
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
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.
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.
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
QuantumExplorer
changed the title
QuantumExplorer
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rs-sdk-ffi/src/address_sync/provider.rs (1)
184-197: Incorrect memory deallocation pattern for slice.This is a duplicate of the existing review comment. Using
Box::from_raw(std::ptr::slice_from_raw_parts_mut(...))to free an array allocated viaVec::into_boxed_slice()thenBox::into_raw()works, but the pattern is fragile. The code inmod.rs(lines 181-182) usesinto_boxed_slice()followed byBox::into_raw()which pairs correctly withBox::from_raw(slice_from_raw_parts_mut(...)).However, the doc comment states this frees memory "if the caller allocated the list dynamically" — but this function should only be used for Rust-allocated memory. Clarify ownership semantics.
🧹 Nitpick comments (4)
packages/rs-sdk-ffi/src/address_sync/provider.rs (2)
83-85:unsafe impl Sendlacks soundness guarantees.The comment acknowledges that thread safety depends on callback implementations, but providing
unsafe impl Sendunconditionally shifts the burden to callers without any mechanism to enforce it. If a caller provides non-thread-safe callbacks, this will cause undefined behavior.Consider documenting this requirement more prominently in the public API or adding a marker/assertion mechanism.
98-104: Missing vtable null check before dereference.In
gap_limit()and all other trait methods,self.ffi.vtableis dereferenced without checking for null first. While the contract may expect a valid vtable, defensive null checks would prevent undefined behavior if violated.🔎 Suggested defensive check pattern
fn gap_limit(&self) -> AddressIndex { unsafe { + if self.ffi.vtable.is_null() { + return 0; // or panic with descriptive message + } let vtable = &*self.ffi.vtable; (vtable.gap_limit)(self.ffi.context) } }packages/rs-sdk-ffi/src/address_sync/mod.rs (2)
31-92: Null return doesn't convey error details to caller.When
sdk_handleorprovideris null, or when the sync operation fails, this function returnsnull. The caller has no way to distinguish between different error conditions. Consider using the_with_resultvariant as the primary API or at minimum document that callers should check for null and use the alternate function for error details.
298-312: Provider vtable accessed after Box reconstruction.After
Box::from_raw(provider), the provider is owned by the Box. Accessingprovider.vtableafter this is safe because the Box hasn't been dropped yet. However, consider restructuring for clarity:🔎 Slightly clearer flow
pub unsafe extern "C" fn dash_sdk_address_provider_free(provider: *mut AddressProviderFFI) { if provider.is_null() { return; } let provider = Box::from_raw(provider); // Call the destroy callback if provided - if !provider.vtable.is_null() { - let vtable = &*provider.vtable; - if let Some(destroy) = vtable.destroy { - destroy(provider.context); - } - } + if let Some(destroy) = provider.vtable.as_ref().and_then(|vt| vt.destroy) { + destroy(provider.context); + } + // provider is dropped here, freeing the AddressProviderFFI struct }Note: This requires changing
vtablefrom raw pointer toOption<&AddressProviderVTable>or using a helper. The current code is correct but the null check on a raw pointer followed by dereference could be simplified.
📜 Review details
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/provider.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-ffi/src/address_sync/provider.rspackages/rs-sdk-ffi/src/address_sync/mod.rs
🧠 Learnings (6)
📓 Common learnings
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-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-ffi/src/address_sync/provider.rspackages/rs-sdk-ffi/src/address_sync/mod.rs
📚 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-ffi/src/address_sync/mod.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/mod.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/address_sync/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/mod.rs
🧬 Code graph analysis (1)
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). (16)
- GitHub Check: Rust packages (dapi-grpc) / Formatting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (rs-sdk-ffi) / Unused dependencies
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (wasm-dpp2) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- 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 Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Rust crates security audit
- GitHub Check: Build JS packages / Build JS
- 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 (6)
packages/rs-sdk-ffi/src/address_sync/provider.rs (1)
239-259: LGTM!The tests adequately verify basic callback provider functionality with a static vtable.
packages/rs-sdk-ffi/src/address_sync/mod.rs (5)
48-49: Unsafe dereference without lifetime guarantees.The code creates references from raw pointers (
&*(sdk_handle as *const SDKWrapper)and&mut *provider) that are used across an async boundary (block_on). Whileblock_onexecutes synchronously, the safety comment should document that the referenced data must remain valid for the duration of the call.
160-225: Memory allocation pattern is correct and consistent.The
convert_sync_resultfunction properly:
- Allocates key data via
into_boxed_slice()+Box::into_raw()- Allocates entry arrays via
into_boxed_slice()+Box::into_raw()- Handles empty collections with null pointers
This pairs correctly with the deallocation in
dash_sdk_address_sync_result_free.
234-268: Deallocation correctly mirrors allocation.The free function properly:
- Reconstructs and drops the result Box
- Iterates through found/absent entries to free individual key buffers
- Frees the entry arrays themselves
The use of
std::ptr::slice_from_raw_parts_mutwithBox::from_rawcorrectly matches theBox::into_raw(boxed_slice)allocation pattern used inconvert_sync_result.
319-333: Helper functions are well-implemented.Both
dash_sdk_address_sync_result_total_balanceanddash_sdk_address_sync_result_non_zero_countproperly handle null pointers and empty results, providing safe accessor patterns for FFI consumers.Also applies to: 340-354
360-375: Tests verify default configuration values.The tests confirm expected defaults for
DashSDKAddressSyncConfigandDashSDKAddressSyncMetrics. Consider adding tests for the sync result helper functions with mock data.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs (1)
812-813: Rename test to follow naming convention.This test name should start with
should_per coding guidelines. Consider renaming toshould_verify_proof_signatures_when_querying_address_trunk_state_with_checkpoints.
🧹 Nitpick comments (1)
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs (1)
814-815: Consider moving local imports to module level.The
usestatements inside the test function work but are inconsistent with the rest of the file where imports are at the module level. This is a minor style preference.🔎 Proposed fix
Move these imports to the top of the module with other imports:
use dpp::dash_to_credits; use dpp::dashcore::hashes::Hash; +use dpp::dashcore::Network; use dpp::dashcore::QuorumHash; ... +use drive::grovedb::GroveTrunkQueryResult;Then remove the local imports from the test 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);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.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-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 (12)
📓 Common learnings
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-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-04T09:08:47.901Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2206
File: packages/rs-drive-abci/tests/strategy_tests/main.rs:1162-1162
Timestamp: 2024-10-04T09:08:47.901Z
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/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-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/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: 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/tests/strategy_tests/test_cases/address_tests.rs
📚 Learning: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 Learning: 2024-11-03T10:39:11.242Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2305
File: packages/rs-drive-abci/src/abci/handler/finalize_block.rs:81-0
Timestamp: 2024-11-03T10:39:11.242Z
Learning: In `packages/rs-drive-abci/src/abci/handler/finalize_block.rs`, the use of `.expect("commit transaction")` after `app.commit_transaction(platform_version)` is intentional due to the provided comment explaining its necessity. Do not flag this usage in future reviews.
Applied to files:
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs
📚 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-abci/tests/strategy_tests/test_cases/address_tests.rs
🔇 Additional comments (4)
packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs (4)
37-103: LGTM! Well-designed test context provider.The
TestContextProvideris cleanly implemented with appropriate trait methods for testing proof signature verification. Thefrom_quorumshelper correctly extracts and maps quorum public keys, and the error handling withInvalidQuorumis appropriate.Based on retrieved learnings, the use of
.expect()for public key conversion at line 58 is acceptable in this test context.
871-892: LGTM! Comprehensive test configuration for proof signature verification.The test correctly enables
block_signingandblock_commit_signature_verificationin the test config, which is essential for the signature verification test to be meaningful. The use ofCHAIN_IDfrom the mimic module ensures consistency with test infrastructure.
938-970: LGTM! Key proof verification logic is well-structured.The test correctly:
- Constructs a
TestContextProviderfrom the chain's validator quorums- Executes a trunk state query
- Verifies the proof signature using
maybe_from_proof_with_metadataThis validates the fix described in the PR objectives where checkpoints are now created after transaction commit.
593-597: All trunk query element count assertions are consistently expecting 32 elements.The assertions at lines 593-597 and 790-794 both expect 32 elements, which aligns with a third assertion at line 995. However, the claim about a change from 29→32 cannot be verified from code inspection alone, and the correctness of the expected value of 32 requires validation against the actual trunk query structure definition or implementation logic.
Issue being fixed or feature implemented
Fix checkpoint timing issue causing proof verification failures
GroveDB checkpoints were being created BEFORE the transaction was committed, causing the checkpoint to contain uncommitted state. When using a checkpoint to verify proofs, the proof's root hash wouldn't match the signed app_hash because the checkpoint was missing the final committed changes.
What was done?
Core Changes
- should_checkpoint - New versioned function that determines if a checkpoint should be created (called before transaction commit, while block_execution_context is still available)
- create_grovedb_checkpoint - New versioned function that creates the actual GroveDB checkpoint (called AFTER transaction commit so it captures committed state)
New Files
Test Changes
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Enhancements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.