Skip to content

Conversation

pauldelucia
Copy link
Member

@pauldelucia pauldelucia commented Sep 24, 2025

Issue being fixed or feature implemented

There are updates in the newer revision of rust-dashcore that are important for SPV clients.

What was done?

Update revision of rust-dashcore in dpp Cargo.toml and remove the feature gating for DapiMocksError in wasm-sdk for compilation.

How Has This Been Tested?

By using DET with this branch.

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • Bug Fixes

    • Improved mock DAPI error handling to surface clearer messages while preserving retry behavior and compatibility with the mocks feature.
  • Chores

    • Updated multiple Dash-related dependencies to newer revisions for improved stability and compatibility.

Copy link
Contributor

coderabbitai bot commented Sep 24, 2025

Walkthrough

Updates git revision pins for dashcore-related dependencies in packages/rs-dpp/Cargo.toml. Changes error representations and conversions: maps SdkError::DapiMocksError to WasmSdkErrorKind::DapiMocksError in packages/wasm-sdk/src/error.rs, and replaces DapiMocksError(#[from] MockError) with DapiMocksError(String) plus a feature-gated From<MockError> impl in packages/rs-sdk/src/error.rs. No other public APIs or runtime control flow changes.

Changes

Cohort / File(s) Summary
Dashcore git rev updates
packages/rs-dpp/Cargo.toml
Bumps git rev for dashcore-related dependencies (dashcore, key-wallet, key-wallet-manager, dash-spv, dashcore-rpc) to 8b5ac9170668722ac0bdddb5fae710f167f2e582. No feature or dependency set changes.
WASM SDK error mapping
packages/wasm-sdk/src/error.rs
Adds handling of SdkError::DapiMocksError(...) in From<SdkError> for WasmSdkError, mapping to WasmSdkErrorKind::DapiMocksError with the message and existing retriable flag preserved.
RS SDK mock error refactor
packages/rs-sdk/src/error.rs
Changes enum variant from DapiMocksError(#[from] rs_dapi_client::mock::MockError) to DapiMocksError(String). Adds #[cfg(feature = "mocks")] impl From<rs_dapi_client::mock::MockError> for Error converting the mock error via to_string().

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant RS_SDK as rs-sdk
  participant WASM_SDK as wasm-sdk
  participant ErrorConv as Error conversion

  Caller->>RS_SDK: Invoke SDK function (mocks enabled)
  RS_SDK-->>RS_SDK: Encounter MockError
  alt previous (typed variant)
    RS_SDK-->>Caller: Error::DapiMocksError(MockError) -- typed
  else new (string variant)
    RS_SDK->>Caller: Error::DapiMocksError(String) (MockError.to_string())
  end

  Caller->>WASM_SDK: Call WASM function that returns SdkError
  WASM_SDK-->>ErrorConv: From<SdkError> conversion
  alt SdkError::DapiMocksError(msg)
    ErrorConv-->>WASM_SDK: WasmSdkError{ kind: DapiMocksError, message: msg, retriable }
  else other SdkError
    ErrorConv-->>WASM_SDK: Existing WasmSdkError mapping
  end
  WASM_SDK-->>Caller: Return mapped WasmSdkError
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

cleanup

Suggested reviewers

  • QuantumExplorer
  • lklimek

Poem

I nibble at revisions, neat and spry,
Pins retuned beneath moonlit sky.
Mocks now speak in strings, not types,
Conversions hop through error pipes.
Thump-thump—builds sing, the tests reply. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly and concisely summarizes the primary change—updating the dashcore revision in the dpp package—using a conventional commit format that makes the intent and scope immediately apparent to reviewers. It focuses on the most significant update without unnecessary detail, which aligns with best practices for PR titles. Although some supporting error-handling adjustments were made, they serve to enable the main revision update and need not be called out in the title.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/update-dashcore

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fcb3d97 and 7f3edc7.

📒 Files selected for processing (2)
  • packages/rs-sdk/src/error.rs (2 hunks)
  • packages/wasm-sdk/src/error.rs (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code

Files:

  • packages/rs-sdk/src/error.rs
  • packages/wasm-sdk/src/error.rs
packages/rs-*/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

packages/rs-*/**/*.rs: Use rustfmt defaults for Rust code (format via cargo fmt --all)
Keep Rust code clippy-clean (cargo clippy)
Name Rust modules in snake_case
Use PascalCase for Rust types
Use SCREAMING_SNAKE_CASE for Rust constants

Files:

  • packages/rs-sdk/src/error.rs
packages/wasm-sdk/**

📄 CodeRabbit inference engine (CLAUDE.md)

Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK

Files:

  • packages/wasm-sdk/src/error.rs
🧠 Learnings (3)
📚 Learning: 2024-10-29T14:44:01.184Z
Learnt from: lklimek
PR: dashpay/platform#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/error.rs
  • packages/wasm-sdk/src/error.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
PR: dashpay/platform#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/error.rs
📚 Learning: 2024-10-22T10:51:16.910Z
Learnt from: shumkov
PR: dashpay/platform#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-sdk/src/error.rs
🧬 Code graph analysis (1)
packages/rs-sdk/src/error.rs (1)
packages/wasm-sdk/src/error.rs (3)
  • from (87-169)
  • from (172-174)
  • from (178-185)
⏰ 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). (14)
  • GitHub Check: Rust packages (dash-sdk) / Formatting
  • GitHub Check: Rust packages (drive-abci) / Tests
  • GitHub Check: Rust packages (wasm-sdk) / Tests
  • GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
  • GitHub Check: Rust packages (dash-sdk) / Check each feature
  • GitHub Check: Rust packages (dash-sdk) / Tests
  • GitHub Check: Rust packages (wasm-dpp) / Tests
  • GitHub Check: Rust packages (drive) / Tests
  • GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
  • GitHub Check: Rust crates security audit
  • GitHub Check: Build JS packages / Build JS
  • GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
  • GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
  • GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (3)
packages/wasm-sdk/src/error.rs (1)

114-116: String payload mapping for DapiMocksError looks good.

Thanks for forwarding the pre-formatted message into the JS-facing error; the retriable flag is preserved and no additional conversions are required here.

packages/rs-sdk/src/error.rs (2)

43-43: String payload for DapiMocksError makes sense

Swapping the payload to String keeps the enum available even when the mocks feature (and its types) are compiled out, which aligns with the wasm error mapping. Looks good.


168-173: Manual From<MockError> conversion looks good

Gating the conversion behind the mocks feature while preserving the easy Error::from(mock_err) path is tidy and keeps the feature surface balanced.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/wasm-sdk/src/error.rs (1)

114-120: Duplicate error handling for DapiMocksError.

The code contains two identical match arms for DapiMocksError(e) at lines 115-120 and 172-177. The second occurrence (lines 172-177) should be removed as it's unreachable and creates duplicate handling.

Apply this diff to remove the duplicate match arm:

            StateTransitionBroadcastError(e) => WasmSdkError::from(e),
-            DapiMocksError(e) => Self::new(
-                WasmSdkErrorKind::DapiMocksError,
-                e.to_string(),
-                None,
-                retriable,
-            ),
        }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 23d6612 and fcb3d97.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • packages/rs-dpp/Cargo.toml (1 hunks)
  • packages/wasm-sdk/src/error.rs (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code

Files:

  • packages/wasm-sdk/src/error.rs
packages/wasm-sdk/**

📄 CodeRabbit inference engine (CLAUDE.md)

Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK

Files:

  • packages/wasm-sdk/src/error.rs
🧠 Learnings (5)
📓 Common learnings
Learnt from: shumkov
PR: dashpay/platform#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.
📚 Learning: 2025-03-11T09:39:23.071Z
Learnt from: shumkov
PR: dashpay/platform#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-dpp/Cargo.toml
📚 Learning: 2025-09-03T16:37:11.605Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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-dpp/Cargo.toml
📚 Learning: 2025-01-19T07:36:46.042Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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-dpp/Cargo.toml
📚 Learning: 2024-10-29T14:13:35.584Z
Learnt from: lklimek
PR: dashpay/platform#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/wasm-sdk/src/error.rs
⏰ 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). (18)
  • GitHub Check: Rust packages (wasm-dpp) / Tests
  • GitHub Check: Rust packages (dpp) / Linting
  • GitHub Check: Rust packages (dpp) / Tests
  • GitHub Check: Rust packages (dpp) / Check each feature
  • GitHub Check: Rust packages (dpp) / Unused dependencies
  • GitHub Check: Rust packages (wasm-sdk) / Tests
  • GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
  • GitHub Check: Rust packages (wasm-sdk) / Formatting
  • GitHub Check: Rust packages (wasm-sdk) / Linting
  • GitHub Check: Rust packages (rs-sdk-ffi) / Tests
  • GitHub Check: Rust packages (dash-sdk) / Tests
  • GitHub Check: Rust packages (drive-abci) / Tests
  • GitHub Check: Rust packages (drive) / Tests
  • GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
  • GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build 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
🔇 Additional comments (1)
packages/rs-dpp/Cargo.toml (1)

26-26: Verify dashcore revision includes desired SPV client code changes.
Revision 8b5ac917 exists and adds only CI workflow enhancements for SPV test logging and segfault isolation—no SPV client code updates. Ensure the intended SPV client improvements are actually included in this revision.

Copy link

✅ gRPC Query Coverage Report

================================================================================
gRPC Query Coverage Report - NEW QUERIES ONLY
================================================================================

Total queries in proto: 47
Previously known queries: 47
New queries found: 0


================================================================================
Summary:
--------------------------------------------------------------------------------
No new queries found

Total known queries: 47
  - Implemented: 44
  - Not implemented: 2
  - Excluded: 1

Not implemented queries:
  - getConsensusParams
  - getTokenPreProgrammedDistributions

@pauldelucia pauldelucia requested a review from shumkov September 25, 2025 06:31
/// DAPI mocks error
#[error("Dapi mocks error: {0}")]
DapiMocksError(#[from] rs_dapi_client::mock::MockError),
DapiMocksError(String),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not supposed to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants