fix(sdk)!: failed address sync on invalid proof #2967
Conversation
✅ gRPC Query Coverage Report |
📝 WalkthroughWalkthroughAdds a PlatformAddressTrunkState wrapper and FromProof impl; threads per-request RequestSettings through address-sync trunk/branch flows; updates Fetch/Query implementations and SDK mocks; converts branch execution to a retry-enabled ExecutionResponse flow. Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as AddressSync (SDK)
participant DAPI as DAPI Client
participant Verifier as Drive Proof Verifier
participant Store as Local Store
SDK->>DAPI: fetch GetAddressesTrunkStateRequest (with RequestSettings)
DAPI-->>SDK: trunk response + proof
SDK->>Verifier: verify trunk proof (maybe_from_proof_with_metadata)
Verifier-->>SDK: PlatformAddressTrunkState + metadata
SDK->>Store: extract inner trunk state, update metrics
SDK->>SDK: for each branch -> execute_single_branch_query(settings) (retry wrapper)
SDK->>DAPI: fetch branch request/proof (per retry)
DAPI-->>SDK: branch response + proof
SDK->>Verifier: verify branch proof
Verifier-->>SDK: branch result
SDK->>Store: persist branch state / metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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). (7)
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: 1
🤖 Fix all issues with AI agents
In @packages/rs-sdk/src/platform/address_sync/types.rs:
- Around line 47-48: The manual AddressSyncConfig struct literals are missing
the newly required request_settings field; update each AddressSyncConfig
construction in the FFI module (the two places that build AddressSyncConfig with
min_privacy_count, max_concurrent_requests, max_iterations) to include
request_settings: RequestSettings::default() so the struct initializes all
required fields.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive-proof-verifier/src/types.rspackages/rs-sdk/src/mock/requests.rspackages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-sdk/src/platform/address_sync/types.rspackages/rs-sdk/src/platform/fetch.rspackages/rs-sdk/src/platform/query.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/address_sync/types.rspackages/rs-sdk/src/platform/query.rspackages/rs-drive-proof-verifier/src/types.rspackages/rs-sdk/src/platform/fetch.rspackages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-sdk/src/mock/requests.rs
🧠 Learnings (21)
📚 Learning: 2024-12-05T12:59:22.044Z
Learnt from: lklimek
Repo: dashpay/platform PR: 1924
File: packages/rs-dapi-client/src/request_settings.rs:74-74
Timestamp: 2024-12-05T12:59:22.044Z
Learning: The `AppliedRequestSettings` struct in `packages/rs-dapi-client/src/request_settings.rs` is intended for internal use within the monorepo and is not part of the public API.
Applied to files:
packages/rs-sdk/src/platform/address_sync/types.rspackages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2024-10-28T10:38:49.538Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/executor.rs:49-56
Timestamp: 2024-10-28T10:38:49.538Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, the `ExecutionResponse` struct always has a non-optional `address` field of type `Address` because an address is always present when there's a successful response.
Applied to files:
packages/rs-sdk/src/platform/address_sync/types.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-proof-verifier/src/types.rspackages/rs-sdk/src/platform/fetch.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-sdk/src/mock/requests.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-drive-proof-verifier/src/types.rspackages/rs-sdk/src/platform/address_sync/mod.rspackages/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-drive-proof-verifier/src/types.rspackages/rs-sdk/src/platform/address_sync/mod.rspackages/rs-sdk/src/mock/requests.rs
📚 Learning: 2024-10-04T14:16:05.798Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2207
File: packages/rs-drive-proof-verifier/src/proof.rs:1646-1664
Timestamp: 2024-10-04T14:16:05.798Z
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-drive-proof-verifier/src/types.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-sdk/src/mock/requests.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-sdk/src/platform/fetch.rspackages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2024-10-30T11:13:22.398Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/platform/fetch_many.rs:0-0
Timestamp: 2024-10-30T11:13:22.398Z
Learning: In the `fetch_many` method in `packages/rs-sdk/src/platform/fetch_many.rs`, we cannot move `request` into the async closure, as moving `request` into the async context will not work correctly.
Applied to files:
packages/rs-sdk/src/platform/fetch.rspackages/rs-sdk/src/platform/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/src/platform/address_sync/mod.rspackages/rs-sdk/src/mock/requests.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-sdk/src/platform/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/src/platform/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: Use Rust for core platform components (Drive, DAPI server, DPP implementation)
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2024-10-29T14:44:01.184Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-dapi-client/src/executor.rs:38-38
Timestamp: 2024-10-29T14:44:01.184Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, for the structs `ExecutionError<E>` and `ExecutionResponse<R>`, only the serde derives (`Serialize`, `Deserialize`) should be conditional based on the "mocks" feature; the other derives (`Debug`, `Clone`, `Eq`, `PartialEq`) should always be present.
Applied to files:
packages/rs-sdk/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: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-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.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: 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-sdk/src/mock/requests.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-sdk/src/mock/requests.rs
📚 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-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
🧬 Code graph analysis (5)
packages/rs-sdk/src/platform/query.rs (1)
packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (7)
GetIdentityKeysRequestV0(959-991)AllKeys(854-863)GetAddressInfoRequest(9505-9520)GetAddressesInfosRequest(9702-9717)GetAddressesTrunkStateRequest(9827-9842)Version(5599-5618)GetAddressesTrunkStateRequestV0(9849-9858)
packages/rs-sdk/src/platform/fetch.rs (1)
packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (1)
GetAddressesTrunkStateRequest(9827-9842)
packages/rs-sdk/src/platform/address_sync/mod.rs (4)
packages/rs-sdk/src/sync.rs (1)
retry(174-254)packages/rs-drive-proof-verifier/src/proof.rs (1)
Drive(915-918)packages/rs-sdk/src/platform/fetch.rs (1)
fetch_with_metadata(121-129)packages/dashmate/src/core/waitForCoreStart.js (1)
retries(13-13)
packages/rs-drive-proof-verifier/src/proof.rs (2)
packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs (1)
maybe_from_proof_with_metadata(34-122)packages/rs-drive-proof-verifier/src/proof/token_direct_purchase.rs (2)
maybe_from_proof_with_metadata(28-74)request(46-56)
packages/rs-sdk/src/mock/requests.rs (1)
packages/rs-sdk/src/platform/types/evonode.rs (2)
mock_serialize(50-52)mock_deserialize(45-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build JS packages / Build JS
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Rust crates security audit
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (11)
packages/rs-sdk/src/platform/query.rs (1)
303-315: LGTM! Parameterless trunk state query follows established patterns.The implementation correctly uses the unit type
()for a parameterless query, consistent with other similar queries in this file (e.g.,GetProtocolVersionUpgradeStateRequestat line 404). The proof enforcement is appropriate.packages/rs-sdk/src/platform/fetch.rs (1)
328-330: LGTM! Fetch implementation follows established patterns.The implementation correctly associates
PlatformAddressTrunkStatewith its request type, following the same pattern as other types in this file.packages/rs-drive-proof-verifier/src/types.rs (1)
728-761: LGTM! Clean newtype wrapper with proper trait implementations.The
PlatformAddressTrunkStatewrapper provides:
- Clear semantic meaning for trunk state queries
- Type safety in the API
- Transparent access via Deref/DerefMut
- Comprehensive documentation
The implementation follows Rust best practices for newtype wrappers.
packages/rs-sdk/src/mock/requests.rs (1)
526-539: LGTM! Mock implementation correctly handles non-serializable type.The implementation is consistent with the existing
GroveTrunkQueryResultmock (lines 513-524) and correctly usesunimplemented!for a type that cannot be serialized due to the underlying Tree type.Based on learnings, panicking in mock implementations is acceptable, and the error messages clearly explain why serialization is not supported.
packages/rs-drive-proof-verifier/src/proof.rs (1)
1086-1110: Clean delegation pattern for the new wrapper type.The implementation correctly delegates to the existing
GroveTrunkQueryResult::maybe_from_proof_with_metadataand wraps the result. This follows the established pattern seen elsewhere in this file (e.g.,Votedelegating toResourceVotesByIdentityat lines 2079-2134).packages/rs-sdk/src/platform/address_sync/mod.rs (6)
50-67: Imports align with the new retry and settings integration.The added imports (
Fetch,retry,PlatformAddressTrunkState, and thers_dapi_clienttypes) are all utilized in the refactored code paths.
224-243: Clean refactor to useFetchtrait with settings propagation.The trunk query now leverages
PlatformAddressTrunkState::fetch_with_metadata, which handles proof verification internally. Theinto_inner()call correctly extracts the underlyingGroveTrunkQueryResult.
330-392: Settings correctly threaded through parallel branch queries.The
RequestSettingsparameter is properly passed to eachexecute_single_branch_querycall. The existing concurrency control and error handling (log and continue) remain appropriate for parallel query execution.
394-469: Well-structured retry logic for branch queries.The implementation correctly:
- Wraps execution and proof verification in a retryable closure
- Preserves address context in errors for retry decisions
- Applies settings override in the correct order (line 466)
- Uses the
retryfunction frompackages/rs-sdk/src/sync.rsOne consideration: the closure captures
expected_hash,depth, andplatform_versionby value/reference, whilerequestandkeyare cloned inside the closure. This is the correct approach since the closure may be called multiple times during retries.
121-122: Request settings properly integrated into the sync flow.The
config.request_settingsis passed to both trunk and branch query functions, enabling per-request configuration (retries, timeouts) as described in the PR objectives.
426-428: No changes needed—the error context is preserved.The
inner_into()method onExecutionErroris explicitly designed to convert the inner error type while preserving bothretriesandaddressfields (see the implementation atpackages/rs-dapi-client/src/executor.rs:54-62). At line 428, the transport error's address and retry context are maintained through this conversion, so the retry logic will have the necessary information for ban decisions.Likely an incorrect or invalid review comment.
| /// Tree type doesn't support serialization. Address sync operations should not be mocked. | ||
| impl MockResponse for PlatformAddressTrunkState { | ||
| fn mock_serialize(&self, _sdk: &MockDashPlatformSdk) -> Vec<u8> { | ||
| unimplemented!("PlatformAddressTrunkState does not support mock serialization - the Tree type is not serializable") |
There was a problem hiding this comment.
sounds like feature request to grovedb?
| }; | ||
|
|
||
| // Verify the proof | ||
| let branch_result = Drive::verify_address_funds_branch_query( |
There was a problem hiding this comment.
why not use FromProof trait?
This skips metadata + signature verification, etc.
We should use FromProof and interpret returned errors as needed.
There was a problem hiding this comment.
We don't need to verify tenderdash proof and metadata here because it's already validted and we are passing here root has to check with merk proof
There was a problem hiding this comment.
I would prefer use of FromProof if it's easy to ensure it will not change in future in a direction that introduces security issue. However, if you are sure we don't want it here, please add a comment explaining that.
There was a problem hiding this comment.
I'm not sure, that's wha @QuantumExplorer told me: the idea is that they must be on the same app hash and we verify it here. @QuantumExplorer please confirm. But you still can get other invalid parts of the proof and metadata and we just ignore this. I don't like this neigher.
There was a problem hiding this comment.
We don't want it here. This query does not need to prove a tenderdash proof, because well... we already did for that root hash.
There was a problem hiding this comment.
Let's add a comment to the fn that root hash must have been already verified, so that AI will see it and hopefully respect when it changes that part of code in 2 years from now ;-)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-sdk/src/platform/address_sync/mod.rs (1)
224-464: Add tracking fortotal_proof_bytesmetric in branch query execution.The refactored code updates
trunk_queries,branch_queries,total_elements_seen, anditerationsmetrics, buttotal_proof_bytesis not being tracked despite the field existing inAddressSyncMetricsand theaverage_proof_bytes()method depending on it.In
execute_single_branch_query, after successfully verifying the proof, add:metrics.total_proof_bytes += proof_bytes.len();This will ensure the average proof size metric is correctly calculated and observability is complete.
🧹 Nitpick comments (1)
packages/rs-sdk-ffi/src/address_sync/mod.rs (1)
56-61: Consider exposing RequestSettings configuration to FFI callers.The FFI layer currently hardcodes
RequestSettings::default()for all address sync operations. This prevents FFI users from customizing retry behavior, timeouts, and other request-level settings that may be important for production use cases.Consider either:
- Adding optional RequestSettings fields to
DashSDKAddressSyncConfigfor FFI exposure- Documenting this limitation for FFI users
Also applies to: 135-140
📜 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/src/platform/address_sync/mod.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/mod.rspackages/rs-sdk/src/platform/address_sync/mod.rs
🧠 Learnings (16)
📓 Common learnings
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`.
📚 Learning: 2024-12-05T12:59:22.044Z
Learnt from: lklimek
Repo: dashpay/platform PR: 1924
File: packages/rs-dapi-client/src/request_settings.rs:74-74
Timestamp: 2024-12-05T12:59:22.044Z
Learning: The `AppliedRequestSettings` struct in `packages/rs-dapi-client/src/request_settings.rs` is intended for internal use within the monorepo and is not part of the public API.
Applied to files:
packages/rs-sdk-ffi/src/address_sync/mod.rspackages/rs-sdk/src/platform/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.rspackages/rs-sdk/src/platform/address_sync/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-sdk/src/platform/address_sync/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-sdk/src/platform/address_sync/mod.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/platform/address_sync/mod.rs
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2024-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/src/platform/address_sync/mod.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-sdk/src/platform/address_sync/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-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2024-10-04T14:16:05.798Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2207
File: packages/rs-drive-proof-verifier/src/proof.rs:1646-1664
Timestamp: 2024-10-04T14:16:05.798Z
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/platform/address_sync/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-sdk/src/platform/address_sync/mod.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-sdk/src/platform/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: Use `rs-drive-proof-verifier` for cryptographic proof verification
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
📚 Learning: 2024-10-30T11:13:22.398Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/platform/fetch_many.rs:0-0
Timestamp: 2024-10-30T11:13:22.398Z
Learning: In the `fetch_many` method in `packages/rs-sdk/src/platform/fetch_many.rs`, we cannot move `request` into the async closure, as moving `request` into the async context will not work correctly.
Applied to files:
packages/rs-sdk/src/platform/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: Use Rust for core platform components (Drive, DAPI server, DPP implementation)
Applied to files:
packages/rs-sdk/src/platform/address_sync/mod.rs
🧬 Code graph analysis (1)
packages/rs-sdk/src/platform/address_sync/mod.rs (6)
packages/rs-sdk/src/sync.rs (1)
retry(174-254)packages/rs-sdk/src/sdk.rs (1)
version(606-612)packages/rs-drive-proof-verifier/src/proof.rs (1)
Drive(915-918)packages/rs-sdk/src/platform/fetch.rs (1)
fetch_with_metadata(121-129)packages/dashmate/src/core/waitForCoreStart.js (1)
retries(13-13)packages/rs-drive/src/verify/address_funds/verify_address_funds_branch_query/mod.rs (1)
verify_address_funds_branch_query(33-60)
⏰ 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). (17)
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (rs-sdk-ffi) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (rs-sdk-ffi) / Linting
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- 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)
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (6)
packages/rs-sdk-ffi/src/address_sync/mod.rs (1)
17-17: LGTM!The import is correctly placed and necessary for the
RequestSettings::default()usage in the config initialization.packages/rs-sdk/src/platform/address_sync/mod.rs (5)
50-51: LGTM!The new imports are necessary for the retry logic and request settings propagation. They follow Rust conventions and align with the PR objectives to add retry support for address sync operations.
Also applies to: 63-63, 65-67
121-122: LGTM!The trunk query execution has been correctly updated to:
- Accept and propagate
RequestSettingsfor per-request configuration- Use the new
PlatformAddressTrunkStatewrapper with theFetchtrait- Properly handle the optional return value
- Unwrap the result with
into_inner()to extract the underlyingGroveTrunkQueryResultAlso applies to: 224-239
179-179: LGTM!The request settings are correctly propagated through the branch query execution chain, enabling per-request retry and timeout configuration for each branch query.
Also applies to: 333-333, 351-351
391-401: LGTM!The function signature and documentation correctly reflect the new retry capability when proof verification fails.
404-464: Well-implemented retry logic with proper error handling.The function has been correctly refactored to support automatic retries when proof verification fails:
- Request cloning: Line 415 correctly clones the request before execution, as
execute()consumes it (as per learnings)- Error wrapping: All error paths (lines 425, 431-436, 447-451) properly wrap errors in
ExecutionErrorto enable retry logic- Metadata preservation:
ExecutionResponsecorrectly tracks the responding address and retry count- Settings merge: Line 461 properly merges SDK defaults with provided settings using
override_by- Retry integration: The closure pattern allows the
retry()function to re-execute the entire request+verification flow on failure
3 participants
Issue being fixed or feature implemented
During address syn, invalid proof will lead to sync failure
What was done?
FetchforPlatfromAddressTrunkStateHow Has This Been Tested?
With existing tests
Breaking Changes
AddressSyncMetricsstruct is changed.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Refactor
Tests/Mocks
✏️ Tip: You can customize this high-level summary in your review settings.