feat(sdk)!: return last meaningful error on no available addresses #2958
Conversation
✅ gRPC Query Coverage Report |
📝 WalkthroughWalkthroughAdds per-execution last-error tracking and a new non-retriable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as DapiClient
participant Addr as AddressList
participant TransportFactory as TransportFactory
participant Transport as TransportClient
participant Timer as Sleep
Note over Client,Addr: execute starts — loop-based retry with shared last_error
Client->>Addr: pick_next_address()
Client->>TransportFactory: create_transport_client(address)
TransportFactory->>Transport: new client
Client->>Transport: execute(request)
alt transport OK
Transport-->>Client: success(response)
Client->>Addr: unban/update_success(address)
Client-->>Caller: return response (with retries)
else transport Err
Transport-->>Client: TransportError
Client->>Client: record last_error (Arc<Mutex>)
Client->>Addr: ban/update_failure(address)
Client->>Timer: sleep(retry_delay)
Timer-->>Client: wake
Client->>Client: increment retry count
alt addresses exhausted AND last_error present
Client-->>Caller: NoAvailableAddressesToRetry(last_error)
else addresses exhausted WITHOUT last_error
Client-->>Caller: NoAvailableAddresses
else non-retriable or retries exhausted
Client-->>Caller: propagate error
end
end
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)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (16)📚 Learning: 2024-10-25T10:35:05.071ZApplied to files:
📚 Learning: 2024-10-22T10:53:12.111ZApplied to files:
📚 Learning: 2025-10-09T15:59:33.162ZApplied to files:
📚 Learning: 2024-10-18T13:52:36.233ZApplied to files:
📚 Learning: 2024-10-29T14:44:01.184ZApplied to files:
📚 Learning: 2024-10-29T14:40:54.727ZApplied to files:
📚 Learning: 2024-11-20T14:14:54.721ZApplied to files:
📚 Learning: 2025-10-09T15:59:41.943ZApplied to files:
📚 Learning: 2025-01-23T09:23:32.740ZApplied to files:
📚 Learning: 2024-10-10T10:30:24.653ZApplied to files:
📚 Learning: 2024-10-10T10:30:19.883ZApplied to files:
📚 Learning: 2024-10-10T10:30:25.283ZApplied to files:
📚 Learning: 2025-01-23T09:23:32.740ZApplied to files:
📚 Learning: 2024-10-28T10:38:49.538ZApplied to files:
📚 Learning: 2024-10-29T14:16:40.972ZApplied to files:
📚 Learning: 2024-10-18T15:43:32.447ZApplied to files:
🧬 Code graph analysis (1)packages/rs-sdk/src/sync.rs (3)
⏰ 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)
🔇 Additional comments (5)
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
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rs-dapi-client/src/dapi_client.rspackages/rs-dapi-client/src/executor.rspackages/rs-dapi-client/src/lib.rspackages/rs-sdk/src/error.rspackages/rs-sdk/src/platform/transition/broadcast.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-dapi-client/src/lib.rspackages/rs-dapi-client/src/executor.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/platform/transition/broadcast.rspackages/rs-sdk/src/error.rs
🧠 Learnings (13)
📓 Common learnings
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-dapi-client/src/dapi_client.rs:267-267
Timestamp: 2024-10-18T13:52:36.233Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when handling retries, if `can_retry()` returns `None`, we should treat it as non-retryable and not retry. This is the preferred approach for safety, and future implementations of `CanRetry` may return `None` less frequently.
📚 Learning: 2024-10-18T13:52:36.233Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-dapi-client/src/dapi_client.rs:267-267
Timestamp: 2024-10-18T13:52:36.233Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when handling retries, if `can_retry()` returns `None`, we should treat it as non-retryable and not retry. This is the preferred approach for safety, and future implementations of `CanRetry` may return `None` less frequently.
Applied to files:
packages/rs-dapi-client/src/lib.rspackages/rs-dapi-client/src/executor.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/error.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-dapi-client/src/lib.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/error.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-dapi-client/src/lib.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/platform/transition/broadcast.rspackages/rs-sdk/src/error.rs
📚 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-dapi-client/src/lib.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-dapi-client/src/lib.rspackages/rs-dapi-client/src/executor.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/error.rs
📚 Learning: 2024-11-28T14:08:53.428Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2351
File: packages/rs-dapi-client/src/address_list.rs:241-250
Timestamp: 2024-11-28T14:08:53.428Z
Learning: In the `IntoIterator` implementation for `AddressList` in `packages/rs-dapi-client/src/address_list.rs`, since `self` is consumed, holding a write lock while converting to an iterator does not block other operations.
Applied to files:
packages/rs-dapi-client/src/lib.rs
📚 Learning: 2024-10-25T10:35:05.071Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2266
File: packages/rs-sdk/src/sync.rs:171-221
Timestamp: 2024-10-25T10:35:05.071Z
Learning: In `packages/rs-sdk/src/sync.rs`, within the `retry` function, the closure passed to `.when()` in `backon::Retryable::retry` is not executed concurrently, so using a mutable variable like `retries` inside it does not lead to data races.
Applied to files:
packages/rs-dapi-client/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-dapi-client/src/executor.rspackages/rs-dapi-client/src/dapi_client.rs
📚 Learning: 2024-11-20T14:14:54.721Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2337
File: packages/rs-dapi-client/src/executor.rs:161-212
Timestamp: 2024-11-20T14:14:54.721Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, the `WrapWithExecutionResult` trait supports on-the-fly type conversions using the `From` trait, which is useful when using the `?` operator to return errors.
Applied to files:
packages/rs-dapi-client/src/executor.rs
📚 Learning: 2024-10-22T10:51:16.910Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/transport.rs:51-51
Timestamp: 2024-10-22T10:51:16.910Z
Learning: In implementations of `TransportClient`, only `DapiClientError` is used as the `Error` type, and it implements `std::error::Error`.
Applied to files:
packages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/error.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-dapi-client/src/dapi_client.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/transition/broadcast.rs
🧬 Code graph analysis (3)
packages/rs-dapi-client/src/lib.rs (3)
packages/rs-dapi-client/src/dapi_client.rs (1)
is_no_available_addresses(59-61)packages/rs-dapi-client/src/executor.rs (1)
is_no_available_addresses(83-85)packages/rs-sdk/src/error.rs (1)
is_no_available_addresses(253-258)
packages/rs-dapi-client/src/executor.rs (3)
packages/rs-dapi-client/src/dapi_client.rs (1)
is_no_available_addresses(59-61)packages/rs-dapi-client/src/lib.rs (1)
is_no_available_addresses(92-94)packages/rs-sdk/src/error.rs (1)
is_no_available_addresses(253-258)
packages/rs-dapi-client/src/dapi_client.rs (3)
packages/rs-dapi-client/src/executor.rs (1)
is_no_available_addresses(83-85)packages/rs-dapi-client/src/lib.rs (1)
is_no_available_addresses(92-94)packages/rs-sdk/src/error.rs (1)
is_no_available_addresses(253-258)
⏰ 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). (10)
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- 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: Rust crates security audit
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
🔇 Additional comments (7)
packages/rs-sdk/src/platform/transition/broadcast.rs (4)
24-24: LGTM! Send bound is necessary for async safety.The
+ Sendbound on the generic typeTis required for async methods where the returned future may be moved across threads. This ensures thread safety in async contexts.Based on learnings, when passing data into asynchronous code, data structures must be
Send + Synccompatible.
29-29: LGTM! Consistent with trait definition.The
+ Sendbound is correctly applied to maintain consistency with the trait method signature at Line 24.
94-94: LGTM! Implementation matches trait signature.The
+ Sendbound is correctly applied to the implementation, matching the trait definition.
263-263: LGTM! Complete and consistent implementation.The
+ Sendbound is correctly applied across all async methods that use this generic parameter, ensuring thread safety.packages/rs-dapi-client/src/executor.rs (1)
82-85: LGTM! Proper delegation pattern.The
is_no_available_addressesmethod correctly delegates to the inner error's implementation, maintaining consistency with the existingcan_retrydelegation pattern at lines 79-81.packages/rs-dapi-client/src/dapi_client.rs (1)
58-61: LGTM! Correct identification of NoAvailableAddresses.The implementation correctly uses
matches!to identify theDapiClientError::NoAvailableAddressesvariant, consistent with the trait's default implementation and the broader retry logic pattern.packages/rs-sdk/src/error.rs (1)
252-258: LGTM! Proper SDK-level integration.The implementation correctly identifies the
NoAvailableAddresseserror through the SDK's error hierarchy, using nested pattern matching to check forError::DapiClientError(DapiClientError::NoAvailableAddresses). This enables proper retry logic at the SDK layer.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/rs-dapi-client/src/dapi_client.rs (2)
231-236: Fixed retry delay may need to be configurable.The
retry_delayis hardcoded to 10ms. While the comment at line 258-259 insync.rssuggests "we use different server, so no real delay needed," consider whether this should be configurable viaRequestSettingsfor scenarios where longer backoff might be beneficial (e.g., rate limiting).
286-319: Consider simplifying the error handling flow.The pattern of creating an
ExecutionError, wrapping it inErr, passing toupdate_address_ban_status, then unwrapping it back is functional but awkward. Consider extracting the ban update into a helper that takes the error directly.♻️ Suggested simplification
- let execution_error = ExecutionError { - inner: DapiClientError::Transport(transport_error), - retries, - address: Some(address.clone()), - }; - - // Update ban status - let error_result: ExecutionResult<R::Response, DapiClientError> = - Err(execution_error); - update_address_ban_status::<R::Response, DapiClientError>( - &self.address_list, - &error_result, - &applied_settings, - ); - - // Unwrap the error back - let execution_error = error_result.unwrap_err(); + let execution_error = ExecutionError { + inner: DapiClientError::Transport(transport_error), + retries, + address: Some(address.clone()), + }; + + // Update ban status + update_address_ban_status::<R::Response, DapiClientError>( + &self.address_list, + &Err(execution_error.clone()), + &applied_settings, + );This would require
ExecutionErrorto implementClone, which may not be desirable. Alternatively, a helper function that accepts the error components directly could be cleaner.packages/rs-sdk/src/sync.rs (1)
15-25: Duplicatesleephelpers across crates.This
sleepfunction is duplicated in three locations:
packages/rs-sdk/src/sync.rs(here)packages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/src/transport/wasm_channel.rsConsider re-exporting from
rs-dapi-clientto avoid duplication, or document why each crate needs its own copy.♻️ Alternative: re-export from rs-dapi-client
In
packages/rs-sdk/src/sync.rs:-/// Sleep for the given duration. -#[cfg(not(target_arch = "wasm32"))] -pub async fn sleep(duration: Duration) { - tokio::time::sleep(duration).await; -} - -/// Sleep for the given duration. -#[cfg(target_arch = "wasm32")] -pub async fn sleep(duration: Duration) { - gloo_timers::future::sleep(duration).await; -} +pub use rs_dapi_client::transport::sleep;This would require making
sleeppublic inrs_dapi_client::transport.
📜 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 (6)
packages/rs-dapi-client/Cargo.tomlpackages/rs-dapi-client/src/dapi_client.rspackages/rs-dapi-client/src/transport.rspackages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/error.rspackages/rs-sdk/src/sync.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk/src/error.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/sync.rspackages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/src/dapi_client.rs
🧠 Learnings (26)
📚 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-sdk/Cargo.tomlpackages/rs-sdk/src/sync.rspackages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/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-sdk/Cargo.tomlpackages/rs-dapi-client/Cargo.toml
📚 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/Cargo.tomlpackages/rs-sdk/src/sync.rspackages/rs-dapi-client/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 WASM bindings to connect Rust and JavaScript code
Applied to files:
packages/rs-sdk/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 WebAssembly as a bridge between Rust and JavaScript implementations
Applied to files:
packages/rs-sdk/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/Cargo.tomlpackages/rs-sdk/src/sync.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/sync.rspackages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-dapi-client/Cargo.toml
📚 Learning: 2024-10-25T10:35:05.071Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2266
File: packages/rs-sdk/src/sync.rs:171-221
Timestamp: 2024-10-25T10:35:05.071Z
Learning: In `packages/rs-sdk/src/sync.rs`, within the `retry` function, the closure passed to `.when()` in `backon::Retryable::retry` is not executed concurrently, so using a mutable variable like `retries` inside it does not lead to data races.
Applied to files:
packages/rs-sdk/src/sync.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/sync.rspackages/rs-dapi-client/src/transport.rs
📚 Learning: 2025-10-09T15:59:33.162Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/clients/tenderdash_client.rs:0-0
Timestamp: 2025-10-09T15:59:33.162Z
Learning: In the rs-dapi codebase, the `Workers::spawn` method (in `packages/rs-dapi/src/sync.rs`) accepts futures that return `Result<O, E>`, not `()`. Its signature is `fn spawn<F, O, E>(&self, fut: F) where F: Future<Output = Result<O, E>> + Send + 'static`. The method handles error logging internally, so spawned tasks should return Results directly without additional error handling wrappers.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2025-10-09T15:59:41.943Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/clients/tenderdash_client.rs:0-0
Timestamp: 2025-10-09T15:59:41.943Z
Learning: In packages/rs-dapi, the Workers::spawn method accepts futures that return Result<O, E> where E: Into<DapiError>. It handles error logging internally, so spawned tasks should return Result types rather than handling errors inside the task. The Workers implementation automatically logs worker failures.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2025-01-23T09:23:32.740Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2405
File: packages/rs-sdk/src/sync.rs:88-95
Timestamp: 2025-01-23T09:23:32.740Z
Learning: The `block_on` function in `packages/rs-sdk/src/sync.rs` requires true blocking behavior, which is why a non-blocking WebAssembly implementation using `wasm_bindgen_futures::spawn_local` is not suitable.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2024-10-10T10:30:24.653Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2232
File: packages/rs-sdk/src/sync.rs:68-71
Timestamp: 2024-10-10T10:30:24.653Z
Learning: In synchronous code within a Tokio runtime, we cannot await spawned task handles (`JoinHandle`), so it's acceptable to check if the task is finished and abort it if necessary.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2024-10-10T10:30:25.283Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2232
File: packages/rs-sdk/src/mock/sdk.rs:129-130
Timestamp: 2024-10-10T10:30:25.283Z
Learning: In the codebase `packages/rs-sdk/src/mock/sdk.rs`, `tokio::sync::Mutex` is used instead of `std::sync::Mutex` because `std::sync::MutexGuard` is not `Send` and cannot be used in async contexts.
Applied to files:
packages/rs-sdk/src/sync.rspackages/rs-dapi-client/src/transport.rs
📚 Learning: 2024-10-18T13:52:36.233Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-dapi-client/src/dapi_client.rs:267-267
Timestamp: 2024-10-18T13:52:36.233Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when handling retries, if `can_retry()` returns `None`, we should treat it as non-retryable and not retry. This is the preferred approach for safety, and future implementations of `CanRetry` may return `None` less frequently.
Applied to files:
packages/rs-sdk/src/sync.rspackages/rs-dapi-client/src/dapi_client.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/sync.rspackages/rs-dapi-client/src/dapi_client.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/sync.rspackages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/src/dapi_client.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/sync.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/sync.rs
📚 Learning: 2024-10-22T10:51:16.910Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/transport.rs:51-51
Timestamp: 2024-10-22T10:51:16.910Z
Learning: In implementations of `TransportClient`, only `DapiClientError` is used as the `Error` type, and it implements `std::error::Error`.
Applied to files:
packages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/src/dapi_client.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-dapi-client/src/dapi_client.rspackages/rs-dapi-client/Cargo.toml
📚 Learning: 2024-11-20T14:14:54.721Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2337
File: packages/rs-dapi-client/src/executor.rs:161-212
Timestamp: 2024-11-20T14:14:54.721Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, the `WrapWithExecutionResult` trait supports on-the-fly type conversions using the `From` trait, which is useful when using the `?` operator to return errors.
Applied to files:
packages/rs-dapi-client/src/dapi_client.rs
📚 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-dapi-client/src/dapi_client.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-dapi-client/src/dapi_client.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-dapi-client/src/dapi_client.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-dapi-client/Cargo.toml
🧬 Code graph analysis (2)
packages/rs-sdk/src/sync.rs (5)
packages/rs-dapi-client/src/dapi_client.rs (4)
update_address_ban_status(141-200)update_address_ban_status(295-295)update_address_ban_status(352-352)address_list(134-136)packages/rs-dapi-client/src/transport.rs (2)
sleep(29-31)sleep(35-37)packages/rs-dapi-client/src/transport/wasm_channel.rs (1)
sleep(100-102)packages/rs-dapi-client/src/executor.rs (1)
default(101-107)packages/rs-dapi-client/src/request_settings.rs (1)
default(39-46)
packages/rs-dapi-client/src/dapi_client.rs (4)
packages/rs-sdk/src/error.rs (1)
is_no_available_addresses(258-264)packages/rs-dapi-client/src/lib.rs (1)
is_no_available_addresses(92-94)packages/rs-dapi-client/src/transport.rs (4)
method_name(62-62)request_name(52-54)response_name(57-59)with_uri_and_settings(145-149)packages/rs-dapi-client/src/transport/grpc.rs (2)
with_uri_and_settings(30-49)with_uri_and_settings(67-86)
⏰ 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). (15)
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- 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: Rust crates security audit
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
🔇 Additional comments (11)
packages/rs-sdk/Cargo.toml (1)
48-50: LGTM!The
gloo-timersdependency withfeatures = ["futures"]is correctly added for wasm32 targets, consistent with the same dependency inrs-dapi-client/Cargo.toml.packages/rs-dapi-client/Cargo.toml (1)
24-28: LGTM!The platform-specific
tokiodependency withfeatures = ["time"]is correctly added for non-wasm32 targets, enablingtokio::time::sleepused by the new sleep helper intransport.rs.packages/rs-dapi-client/src/transport.rs (3)
27-37: LGTM!The platform-specific
sleephelpers correctly delegate totokio::time::sleepfor non-wasm andgloo_timers::future::sleepfor wasm32 targets.
85-101: Clone implementation loses the original error source.The comment at line 89-90 correctly documents that this manual Clone loses the original error source from
tonic::Status. This is acceptable given thattonic::Statusdoesn't implement Clone, but callers should be aware that chained error context may be lost when cloning transport errors.
126-137: LGTM!The
Mockableimplementation forBox<TransportError>correctly delegates to the underlyingTransportErrorimplementation.packages/rs-dapi-client/src/dapi_client.rs (3)
34-40: LGTM!The new
NoAvailableAddressesToRetryvariant correctly usesBox<TransportError>to preserve the last meaningful error while keeping the enum size reasonable.
63-70: LGTM!The
is_no_available_addressesimplementation correctly identifies both address exhaustion variants.
238-382: LGTM on the retry loop refactor.The loop-based retry mechanism correctly:
- Tracks
last_transport_errorto preserve meaningful errors- Returns
NoAvailableAddressesToRetrywhen addresses are exhausted after a transport error- Properly updates address ban status after each attempt
- Respects
max_retrieslimit with appropriate early exitspackages/rs-sdk/src/sync.rs (3)
176-184: Breaking change:retryfunction signature now uses concreteErrortype.The function signature changed from a generic error type
Eto the concreteErrortype. This is a breaking change for callers that used custom error types. The PR is already marked as breaking (!in title), so this is expected.
194-269: LGTM on the retry logic refactor.The loop-based retry mechanism correctly:
- Tracks
last_meaningful_errorto preserve the last retryable error before address exhaustion- Returns
NoAvailableAddressesToRetrywrapping the last meaningful error whenis_no_available_addresses()is true- Properly updates settings for lower layers via
current_settings.retries- Maintains accurate retry counting across nested retry attempts
418-514: LGTM on new test cases.The new tests
test_retry_returns_last_meaningful_error_on_no_addressesandtest_retry_returns_no_addresses_if_no_previous_errorprovide excellent coverage for the new error wrapping behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-dapi-client/src/dapi_client.rs (1)
229-230: Consider making retry delay configurable with backoff.The hardcoded 10ms delay is quite aggressive for network retries. This could lead to:
- Rapid retry storms if multiple clients fail simultaneously
- Unnecessary load on recovering nodes
Consider either:
- Making this configurable via
RequestSettings- Adding exponential backoff (e.g.,
retry_delay * 2^retries)- Adding jitter to prevent thundering herd
Example with exponential backoff
let max_retries = applied_settings.retries; -let retry_delay = Duration::from_millis(10); +let base_retry_delay = Duration::from_millis(10); // ... later in the retry logic: -transport::sleep(retry_delay).await; +let backoff_delay = base_retry_delay * (1 << retries.min(5)); // cap at 320ms +transport::sleep(backoff_delay).await;
📜 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 (8)
packages/rs-dapi-client/src/address_list.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-dapi-client/src/mock.rspackages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/src/transport/wasm_channel.rspackages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/sync.rspackages/wasm-sdk/src/error.rs
💤 Files with no reviewable changes (1)
- packages/rs-sdk/Cargo.toml
🧰 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-dapi-client/src/mock.rspackages/rs-dapi-client/src/address_list.rspackages/wasm-sdk/src/error.rspackages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/src/transport/wasm_channel.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.rs
🧠 Learnings (24)
📓 Common learnings
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.
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-dapi-client/src/dapi_client.rs:267-267
Timestamp: 2024-10-18T13:52:36.233Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when handling retries, if `can_retry()` returns `None`, we should treat it as non-retryable and not retry. This is the preferred approach for safety, and future implementations of `CanRetry` may return `None` less frequently.
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/transport.rs:51-51
Timestamp: 2024-10-22T10:51:16.910Z
Learning: In implementations of `TransportClient`, only `DapiClientError` is used as the `Error` type, and it implements `std::error::Error`.
Learnt from: shumkov
Repo: dashpay/platform PR: 2266
File: packages/rs-sdk/src/sync.rs:171-221
Timestamp: 2024-10-25T10:35:05.071Z
Learning: In `packages/rs-sdk/src/sync.rs`, within the `retry` function, the closure passed to `.when()` in `backon::Retryable::retry` is not executed concurrently, so using a mutable variable like `retries` inside it does not lead to data races.
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.
📚 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-dapi-client/src/mock.rspackages/rs-dapi-client/src/address_list.rspackages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.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-dapi-client/src/mock.rspackages/rs-dapi-client/src/dapi_client.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-dapi-client/src/mock.rspackages/rs-sdk/src/sync.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-dapi-client/src/address_list.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.rs
📚 Learning: 2024-11-28T14:08:53.428Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2351
File: packages/rs-dapi-client/src/address_list.rs:241-250
Timestamp: 2024-11-28T14:08:53.428Z
Learning: In the `IntoIterator` implementation for `AddressList` in `packages/rs-dapi-client/src/address_list.rs`, since `self` is consumed, holding a write lock while converting to an iterator does not block other operations.
Applied to files:
packages/rs-dapi-client/src/address_list.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-dapi-client/src/address_list.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.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-dapi-client/src/address_list.rspackages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/src/transport/wasm_channel.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.rs
📚 Learning: 2024-10-22T10:51:16.910Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/transport.rs:51-51
Timestamp: 2024-10-22T10:51:16.910Z
Learning: In implementations of `TransportClient`, only `DapiClientError` is used as the `Error` type, and it implements `std::error::Error`.
Applied to files:
packages/wasm-sdk/src/error.rspackages/rs-dapi-client/src/transport.rspackages/rs-dapi-client/src/dapi_client.rs
📚 Learning: 2024-10-18T13:52:36.233Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-dapi-client/src/dapi_client.rs:267-267
Timestamp: 2024-10-18T13:52:36.233Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when handling retries, if `can_retry()` returns `None`, we should treat it as non-retryable and not retry. This is the preferred approach for safety, and future implementations of `CanRetry` may return `None` less frequently.
Applied to files:
packages/wasm-sdk/src/error.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.rs
📚 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-dapi-client/src/transport.rspackages/rs-dapi-client/src/transport/wasm_channel.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-dapi-client/src/transport.rspackages/rs-sdk/src/sync.rs
📚 Learning: 2024-10-10T10:30:25.283Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2232
File: packages/rs-sdk/src/mock/sdk.rs:129-130
Timestamp: 2024-10-10T10:30:25.283Z
Learning: In the codebase `packages/rs-sdk/src/mock/sdk.rs`, `tokio::sync::Mutex` is used instead of `std::sync::Mutex` because `std::sync::MutexGuard` is not `Send` and cannot be used in async contexts.
Applied to files:
packages/rs-dapi-client/src/transport.rspackages/rs-sdk/src/sync.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-dapi-client/src/transport.rs
📚 Learning: 2025-10-09T15:59:33.162Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/clients/tenderdash_client.rs:0-0
Timestamp: 2025-10-09T15:59:33.162Z
Learning: In the rs-dapi codebase, the `Workers::spawn` method (in `packages/rs-dapi/src/sync.rs`) accepts futures that return `Result<O, E>`, not `()`. Its signature is `fn spawn<F, O, E>(&self, fut: F) where F: Future<Output = Result<O, E>> + Send + 'static`. The method handles error logging internally, so spawned tasks should return Results directly without additional error handling wrappers.
Applied to files:
packages/rs-dapi-client/src/transport/wasm_channel.rspackages/rs-sdk/src/sync.rs
📚 Learning: 2025-01-23T09:23:32.740Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2405
File: packages/rs-sdk/src/sync.rs:88-95
Timestamp: 2025-01-23T09:23:32.740Z
Learning: The `block_on` function in `packages/rs-sdk/src/sync.rs` is currently only used in tests, and its WebAssembly implementation is deferred until there's a user request for it.
Applied to files:
packages/rs-dapi-client/src/transport/wasm_channel.rspackages/rs-sdk/src/sync.rs
📚 Learning: 2025-01-23T09:23:32.740Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2405
File: packages/rs-sdk/src/sync.rs:88-95
Timestamp: 2025-01-23T09:23:32.740Z
Learning: The `block_on` function in `packages/rs-sdk/src/sync.rs` requires true blocking behavior, which is why a non-blocking WebAssembly implementation using `wasm_bindgen_futures::spawn_local` is not suitable.
Applied to files:
packages/rs-dapi-client/src/transport/wasm_channel.rspackages/rs-sdk/src/sync.rs
📚 Learning: 2024-11-20T14:14:54.721Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2337
File: packages/rs-dapi-client/src/executor.rs:161-212
Timestamp: 2024-11-20T14:14:54.721Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, the `WrapWithExecutionResult` trait supports on-the-fly type conversions using the `From` trait, which is useful when using the `?` operator to return errors.
Applied to files:
packages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.rs
📚 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-dapi-client/src/dapi_client.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-dapi-client/src/dapi_client.rs
📚 Learning: 2024-10-25T10:35:05.071Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2266
File: packages/rs-sdk/src/sync.rs:171-221
Timestamp: 2024-10-25T10:35:05.071Z
Learning: In `packages/rs-sdk/src/sync.rs`, within the `retry` function, the closure passed to `.when()` in `backon::Retryable::retry` is not executed concurrently, so using a mutable variable like `retries` inside it does not lead to data races.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2025-10-09T15:59:41.943Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/clients/tenderdash_client.rs:0-0
Timestamp: 2025-10-09T15:59:41.943Z
Learning: In packages/rs-dapi, the Workers::spawn method accepts futures that return Result<O, E> where E: Into<DapiError>. It handles error logging internally, so spawned tasks should return Result types rather than handling errors inside the task. The Workers implementation automatically logs worker failures.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2024-10-10T10:30:24.653Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2232
File: packages/rs-sdk/src/sync.rs:68-71
Timestamp: 2024-10-10T10:30:24.653Z
Learning: In synchronous code within a Tokio runtime, we cannot await spawned task handles (`JoinHandle`), so it's acceptable to check if the task is finished and abort it if necessary.
Applied to files:
packages/rs-sdk/src/sync.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/sync.rs
🧬 Code graph analysis (3)
packages/rs-dapi-client/src/transport.rs (1)
packages/rs-dapi-client/src/transport/wasm_channel.rs (3)
sleep(100-102)into_send_sleep(108-110)new(40-43)
packages/rs-dapi-client/src/transport/wasm_channel.rs (1)
packages/rs-dapi-client/src/transport.rs (2)
sleep(29-31)sleep(35-37)
packages/rs-dapi-client/src/dapi_client.rs (2)
packages/rs-dapi-client/src/executor.rs (1)
is_no_available_addresses(83-85)packages/rs-sdk/src/error.rs (1)
is_no_available_addresses(258-264)
⏰ 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 (wasm-sdk) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / 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: 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 Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
🔇 Additional comments (16)
packages/rs-dapi-client/src/mock.rs (1)
208-208: LGTM! Clone derive added for error propagation.Adding
ClonetoMockErroraligns with the broader pattern across this PR where error types are equipped withCloneto support propagation through retry logic and error handling.packages/rs-dapi-client/src/address_list.rs (1)
101-101: LGTM! Clone derive added for error handling.Adding
ClonetoAddressListErroris consistent with the error type enhancements across this PR, enabling error propagation through the new retry mechanism.packages/rs-sdk/src/sync.rs (3)
165-174: Function signature correctly reflects breaking change.The signature change from generic error
Eto concreteErrortype aligns with the PR's breaking change objective. The addition of theR: Sendbound andmutqualifier onfuture_factory_fnare appropriate for the new loop-based retry implementation.
175-259: Excellent retry loop implementation with proper error tracking.The manual retry loop correctly implements the new behavior:
- Address ban status updates (line 187) are performed after each attempt, ensuring failed addresses are properly banned.
- NoAvailableAddresses handling (lines 193-210) correctly wraps the last meaningful error or returns as-is if no previous error exists.
- Retry counting (lines 212-214) properly accounts for both the initial request and lower-layer retries.
- Lower-layer retry limiting (line 245) uses
saturating_subto ensure the total retry budget isn't exceeded across layers.- Error preservation (line 253) stores each meaningful error before the next iteration.
The logic handles all edge cases correctly and the flow is clear and maintainable.
408-503: Comprehensive test coverage for new error wrapping behavior.The two new tests thoroughly validate the NoAvailableAddresses error handling:
test_retry_returns_last_meaningful_error_on_no_addresses(lines 408-472) verifies that when addresses are exhausted after a meaningful error, the result wraps the last meaningful error inNoAvailableAddressesToRetry.
test_retry_returns_no_addresses_if_no_previous_error(lines 474-503) ensures that if no addresses are available on the first attempt, the error is returned as-is without wrapping.These tests cover the critical edge cases introduced by this PR.
packages/wasm-sdk/src/error.rs (1)
180-185: Correct error mapping for NoAvailableAddressesToRetry.The mapping to
WasmSdkErrorKind::DapiClientErroris appropriate, and the message format clearly communicates both the address exhaustion and the underlying cause. Preserving theretriableflag maintains correct retry semantics.packages/rs-dapi-client/src/transport/wasm_channel.rs (1)
105-110: Send-compatible sleep helper correctly implemented.The
into_send_sleepfunction properly wrapsgloo_timers::future::sleepto provide aSend-compatible future for WASM targets. This complements the existingWasmBackonSleeperand supports thetransport::sleepabstraction used in the retry logic.packages/rs-dapi-client/src/transport.rs (3)
27-37: LGTM! Clean platform-specific sleep abstractions.The sleep helpers correctly abstract over platform differences, using
tokio::time::sleepfor native andwasm_channel::into_send_sleepfor WASM. This aligns with the existing pattern inwasm_channel.rs(lines 99-101, 107-109) and provides a unified API for the retry logic.
85-101: Reasonable workaround for cloningtonic::Status.The reconstruction approach is sound, preserving the key fields (code, message, details, metadata). The comment on line 90 appropriately documents that the original error source is lost, which is an acceptable tradeoff for enabling the
Clonerequirement needed by the new retry mechanism.
126-137: LGTM!Clean delegation to the underlying
TransportErrorimplementation, properly supporting the boxed error variant used inNoAvailableAddressesToRetry.packages/rs-dapi-client/src/dapi_client.rs (6)
22-40: LGTM! Well-designed error variant for improved error reporting.The new
NoAvailableAddressesToRetryvariant preserves the last meaningful error, which is the core improvement of this PR. Boxing theTransportErroris appropriate to avoid bloating the enum size. The serde attribute correctly reuses the existingserde_mockableapproach.
51-70: LGTM!The
CanRetryimplementation correctly treatsNoAvailableAddressesToRetryas non-retryable and theis_no_available_addresses()method properly identifies both exhaustion variants. This aligns with the pattern inpackages/rs-sdk/src/error.rs(lines 257-263).
236-257: LGTM! Clean address exhaustion handling.The loop correctly tracks
last_transport_errorand returnsNoAvailableAddressesToRetrywhen addresses are exhausted after a transport failure, orNoAvailableAddresseswhen no addresses were ever available. The use oftake()on line 241 properly moves ownership.
276-312: LGTM! Correct handling of transport client creation failures.The error handling properly clones the transport error before moving it into
ExecutionError, updates ban status, and stores the error for potential wrapping if addresses are exhausted. The retry logic correctly checks bothcan_retry_errorandretries < max_retries.
314-348: LGTM! Well-instrumented transport execution.The transport execution is properly wrapped with tracing spans for observability. The result handling correctly builds
ExecutionResponseorExecutionErrorand updates the address ban status appropriately.
350-375: LGTM! Correct retry and error propagation logic.The result handling correctly:
- Returns immediately on success
- Stores transport errors for potential wrapping on address exhaustion
- Respects retry limits and error retryability
- Uses consistent sleep/log pattern with the client creation failure path
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-dapi-client/src/dapi_client.rs (1)
352-357: Consider logging on poisoned lock.The code silently ignores lock poisoning. While this is defensive and unlikely to occur in practice, consider adding a trace/debug log when the lock fails to aid debugging in edge cases.
♻️ Optional: Add trace log for poisoned lock
if let DapiClientError::Transport(ref transport_error) = error.inner { - if let Ok(mut last) = last_error.lock() { - *last = Some(transport_error.clone()); + match last_error.lock() { + Ok(mut last) => *last = Some(transport_error.clone()), + Err(_) => tracing::trace!("failed to acquire last_error lock (poisoned)"), } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/sync.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-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.rs
🧠 Learnings (29)
📓 Common learnings
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-dapi-client/src/dapi_client.rs:267-267
Timestamp: 2024-10-18T13:52:36.233Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when handling retries, if `can_retry()` returns `None`, we should treat it as non-retryable and not retry. This is the preferred approach for safety, and future implementations of `CanRetry` may return `None` less frequently.
📚 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-sdk/Cargo.tomlpackages/rs-sdk/src/sync.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/Cargo.toml
📚 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/Cargo.toml
📚 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-sdk/Cargo.toml
📚 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/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-sdk/Cargo.toml
📚 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/Cargo.tomlpackages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.rs
📚 Learning: 2025-01-23T09:23:32.740Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2405
File: packages/rs-sdk/src/sync.rs:88-95
Timestamp: 2025-01-23T09:23:32.740Z
Learning: The `block_on` function in `packages/rs-sdk/src/sync.rs` is currently only used in tests, and its WebAssembly implementation is deferred until there's a user request for it.
Applied to files:
packages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/sync.rs
📚 Learning: 2024-10-22T10:51:16.910Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/transport.rs:51-51
Timestamp: 2024-10-22T10:51:16.910Z
Learning: In implementations of `TransportClient`, only `DapiClientError` is used as the `Error` type, and it implements `std::error::Error`.
Applied to files:
packages/rs-dapi-client/src/dapi_client.rs
📚 Learning: 2024-10-18T13:52:36.233Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2254
File: packages/rs-dapi-client/src/dapi_client.rs:267-267
Timestamp: 2024-10-18T13:52:36.233Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when handling retries, if `can_retry()` returns `None`, we should treat it as non-retryable and not retry. This is the preferred approach for safety, and future implementations of `CanRetry` may return `None` less frequently.
Applied to files:
packages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.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-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.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-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.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-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.rs
📚 Learning: 2024-11-20T14:14:54.721Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2337
File: packages/rs-dapi-client/src/executor.rs:161-212
Timestamp: 2024-11-20T14:14:54.721Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, the `WrapWithExecutionResult` trait supports on-the-fly type conversions using the `From` trait, which is useful when using the `?` operator to return errors.
Applied to files:
packages/rs-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.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-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.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-dapi-client/src/dapi_client.rspackages/rs-sdk/src/sync.rs
📚 Learning: 2024-10-25T10:35:05.071Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2266
File: packages/rs-sdk/src/sync.rs:171-221
Timestamp: 2024-10-25T10:35:05.071Z
Learning: In `packages/rs-sdk/src/sync.rs`, within the `retry` function, the closure passed to `.when()` in `backon::Retryable::retry` is not executed concurrently, so using a mutable variable like `retries` inside it does not lead to data races.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2025-10-09T15:59:33.162Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/clients/tenderdash_client.rs:0-0
Timestamp: 2025-10-09T15:59:33.162Z
Learning: In the rs-dapi codebase, the `Workers::spawn` method (in `packages/rs-dapi/src/sync.rs`) accepts futures that return `Result<O, E>`, not `()`. Its signature is `fn spawn<F, O, E>(&self, fut: F) where F: Future<Output = Result<O, E>> + Send + 'static`. The method handles error logging internally, so spawned tasks should return Results directly without additional error handling wrappers.
Applied to files:
packages/rs-sdk/src/sync.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/sync.rs
📚 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/sync.rs
📚 Learning: 2025-10-09T15:59:41.943Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/clients/tenderdash_client.rs:0-0
Timestamp: 2025-10-09T15:59:41.943Z
Learning: In packages/rs-dapi, the Workers::spawn method accepts futures that return Result<O, E> where E: Into<DapiError>. It handles error logging internally, so spawned tasks should return Result types rather than handling errors inside the task. The Workers implementation automatically logs worker failures.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2024-10-10T10:30:24.653Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2232
File: packages/rs-sdk/src/sync.rs:68-71
Timestamp: 2024-10-10T10:30:24.653Z
Learning: In synchronous code within a Tokio runtime, we cannot await spawned task handles (`JoinHandle`), so it's acceptable to check if the task is finished and abort it if necessary.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2025-01-23T09:23:32.740Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2405
File: packages/rs-sdk/src/sync.rs:88-95
Timestamp: 2025-01-23T09:23:32.740Z
Learning: The `block_on` function in `packages/rs-sdk/src/sync.rs` requires true blocking behavior, which is why a non-blocking WebAssembly implementation using `wasm_bindgen_futures::spawn_local` is not suitable.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2024-10-10T10:30:25.283Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2232
File: packages/rs-sdk/src/mock/sdk.rs:129-130
Timestamp: 2024-10-10T10:30:25.283Z
Learning: In the codebase `packages/rs-sdk/src/mock/sdk.rs`, `tokio::sync::Mutex` is used instead of `std::sync::Mutex` because `std::sync::MutexGuard` is not `Send` and cannot be used in async contexts.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2024-11-25T01:17:02.001Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2347
File: packages/rs-drive/tests/query_tests.rs:438-460
Timestamp: 2024-11-25T01:17:02.001Z
Learning: In Rust test files (`packages/rs-drive/tests/query_tests.rs`), when code is used only in tests, defining explicit enums for fields (like the `status` field in the `Withdrawal` struct) may not be necessary; using primitive types is acceptable.
Applied to files:
packages/rs-sdk/src/sync.rs
📚 Learning: 2024-10-30T11:19:59.163Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/tests/fetch/config.rs:233-233
Timestamp: 2024-10-30T11:19:59.163Z
Learning: In the Rust SDK's `rs-sdk/tests` integration tests (e.g., in `packages/rs-sdk/tests/fetch/config.rs`), we cannot create objects during tests because there is no support for object creation in this context. Therefore, hardcoded values for test identities must be used.
Applied to files:
packages/rs-sdk/src/sync.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/sync.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/sync.rs
🧬 Code graph analysis (1)
packages/rs-sdk/src/sync.rs (1)
packages/rs-dapi-client/src/dapi_client.rs (3)
update_address_ban_status(144-203)update_address_ban_status(328-328)address_list(137-139)
⏰ 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). (5)
- 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: JS NPM security audit
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
🔇 Additional comments (14)
packages/rs-dapi-client/src/dapi_client.rs (6)
10-10: LGTM!Using
std::sync::Mutexis appropriate here since the lock is held only briefly for cloning/storing the error, avoiding the overhead of an async mutex.
25-25: LGTM!Adding
ClonetoDapiClientErroris necessary for the new error-wrapping functionality and enables storing/propagating the last transport error.
37-43: Design choice acknowledged.The separation between
NoAvailableAddresses(empty list) andNoAvailableAddressesToRetry(exhausted after failures) provides distinct semantics. UsingBox<TransportError>appropriately keeps the enum size balanced.
54-73: LGTM!The
CanRetryimplementation correctly:
- Marks
NoAvailableAddressesToRetryas non-retryable- Provides
is_no_available_addresses()to uniformly detect both address exhaustion variantsBased on learnings, treating these as non-retryable is the safe approach.
241-243: LGTM!The
Arc<Mutex<Option<TransportError>>>pattern correctly enables shared mutable state across retry iterations, consistent with howretries_counter_arcis used. Based on learnings,Arcis necessary for passing data into asynchronous code.
369-386: LGTM!The error transformation logic correctly:
- Detects
NoAvailableAddresseserrors- Wraps them with the last transport error when available
- Preserves retry count and address metadata
- Falls back gracefully when no last error exists
packages/rs-sdk/Cargo.toml (1)
43-43: LGTM! Dependency bump aligns with custom sleep implementation.The backon dependency has been bumped to version 1.3, maintaining
default-features = falsewithout explicit feature flags. This configuration is intentional and aligns with the PR's introduction of custom platform sleep helpers for wasm/non-wasm environments.Based on learnings, a previous setup required the
tokio-sleepfeature for backon. However, backon supports custom sleep implementations via the.sleep()method when default features are disabled, which appears to be the approach taken in this PR.packages/rs-sdk/src/sync.rs (7)
7-7: LGTM: Import changes align with the new concrete Error type.The addition of the concrete
Errortype import and theStdMutexalias are appropriate for the new last-error tracking mechanism. Usingstd::sync::Mutex(viaStdMutex) instead oftokio::sync::Mutexis correct here since we're only storing aStringand not holding the lock across await points.Also applies to: 11-12, 17-17
175-183: LGTM: Function signature updated to use concrete Error type.The signature change from generic
Eto concreteErroris a breaking change as noted in the PR objectives. This enables the new behavior of preserving and returning the last meaningful error when no addresses are available. The addition of theR: Sendbound is necessary for cross-thread operations within the retry mechanism.
193-196: LGTM: Last error tracking implementation is sound.The use of
Arc<StdMutex<Option<String>>>to track the last meaningful error is appropriate given thatErrordoesn't implementClone. Converting errors to strings trades type information for the ability to preserve error details, which is acceptable for this use case where we're providing better diagnostic information to the user.
252-256: LGTM: Last error tracking in notify closure handles lock failures gracefully.The implementation stores the last error message on each retry attempt. Lock acquisition failures (due to poisoning or contention) are handled gracefully by continuing without storing the error, which is acceptable since the retry logic itself continues normally and this is just for improving error messages.
289-289: LGTM: Atomic imports for test counter.The atomic imports are appropriate for tracking retry attempts across closure invocations in the tests. Based on learnings, using
Arc<AtomicUsize>is the correct approach for counters in async code.
260-277: Wrapping logic correctly implements the PR objective. All requested Error API elements have been verified to exist:
is_no_available_addresses()method exists atpackages/rs-sdk/src/error.rs:258NoAvailableAddressesToRetry(Box<Error>)variant exists atpackages/rs-sdk/src/error.rs:102Generic(String)variant exists atpackages/rs-sdk/src/error.rs:81The implementation correctly preserves the retry count and address from the original error while wrapping the stored error message.
367-394: LGTM: Test function correctly uses concrete Error type for retry testing.The test function properly uses
Error::StaleNodewith theHeightvariant, providing the required fields (expected_height,received_height,tolerance_blocks). SinceStaleNodereturnstrueforcan_retry(), it's the appropriate error type for testing the retry logic.
shumkov
force-pushed
the
feat/last-meaningful-error
branch
from
87e95df to
491e20d
Compare
github-project-automation
bot
moved this from In review / testing
to Done
in Platform team
Issue being fixed or feature implemented
When SDK doesn't have enough live addresses to retry a faulty request it returns "no available addresses" error which is actaully hiding the real issue.
What was done?
How Has This Been Tested?
With existing and additional unit tests
Breaking Changes
In case if live addresses are less than expected retries you get and error of the last request failure instead of "no available addresses"
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.