fix(rs-dapi,sdk): decode base64 CBOR error messages from Tenderdash#3350
fix(rs-dapi,sdk): decode base64 CBOR error messages from Tenderdash#3350
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds permissive base64+CBOR decoding to extract embedded "message" strings from Tenderdash JSON Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TenderdashJSON as Tenderdash JSON handler
participant DapiGRPC as DapiClient error converter
participant Metadata as JSON "data" / gRPC metadata
participant Decoder as Base64+CBOR Decoder
participant ErrorMapper as Error Mapper
Client->>TenderdashJSON: deliver JSON error (contains "message" and/or "data")
TenderdashJSON->>Metadata: inspect top-level "message" and "data"
alt top-level "message" missing/empty or "Internal error"
TenderdashJSON->>Decoder: attempt permissive base64 -> CBOR decode of "data"
Decoder-->>TenderdashJSON: optional "message" string
TenderdashJSON->>ErrorMapper: map using decoded message or fallback raw "data"
else top-level "message" present
TenderdashJSON->>ErrorMapper: map using top-level "message"
end
Client->>DapiGRPC: deliver gRPC Status + metadata
DapiGRPC->>Metadata: inspect `drive-error-data-bin` when Code::Internal
alt `drive-error-data-bin` present
DapiGRPC->>Decoder: CBOR-decode metadata bytes
Decoder-->>DapiGRPC: optional "message" string
DapiGRPC->>ErrorMapper: return Error::DriveInternalError(message) if found
else
DapiGRPC->>ErrorMapper: continue existing mapping / fallback
end
ErrorMapper-->>Client: mapped Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Improves dash-sdk’s error conversion so callers can recover structured ConsensusError information even when DAPI does not provide the primary dash-serialized-consensus-error-bin metadata, by falling back to decoding drive-error-data-bin.
Changes:
- Add a fallback path in
From<DapiClientError> for Errorto parse CBORdrive-error-data-binand extract embeddedconsensus_errorbytes. - Add explicit defensive limits (8 KiB max metadata size, CBOR recursion limit 16) before deserializing untrusted metadata.
- Add unit tests covering the fallback behavior and precedence.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lklimek
force-pushed
the
fix/sdk-cbor-security-hardening
branch
from
b173ca5 to
4aa4910
Compare
When Tenderdash returns an error with `message: "Internal error"`, rs-dapi falls through to the `data` field. Previously, the raw base64 string was stored as the message, producing unintelligible gRPC status messages like "oWdtZXNzYWdleMVzdG9yYWdl...". Now `decode_data_message()` tries to decode the `data` field as base64 → CBOR and extract the human-readable `message` text. If decoding fails (plain text data), the raw string is preserved as before. This fixes the 99% case from production DET logs: GroveDB duplicate-key errors (code 13) where the actual message "storage: identity: a unique key with that hash already exists..." was buried in double-encoded CBOR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
changed the title
lklimek
force-pushed
the
fix/sdk-cbor-security-hardening
branch
from
0fc9aee to
bfba2ef
Compare
There was a problem hiding this comment.
Pull request overview
Improves rs-dapi’s Tenderdash JSON-RPC error mapping by decoding base64-encoded CBOR stored in the Tenderdash data field (when the top-level message is unhelpful like "Internal error"), so gRPC clients receive human-readable error messages instead of base64 gibberish.
Changes:
- Added
decode_data_message()helper to decodedataas base64 → CBOR and extract the"message"field. - Updated
TenderdashStatus::from(serde_json::Value)to prefer decodeddata.messagewhen appropriate, while preserving prior fallback behavior. - Added unit tests for decoding behavior plus real-world DET log fixtures.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3350 +/- ##
=========================================
Coverage 84.83% 84.83%
=========================================
Files 2476 2476
Lines 267733 267733
=========================================
Hits 227123 227123
Misses 40610 40610
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-scoped PR that adds base64→CBOR decoding for the Tenderdash error.data field, with good test coverage including real-world DET log fixtures. The new decode_data_message() function follows existing patterns in the module, reuses base64_decode() and walk_cbor_for_key(), and gracefully falls back on any decode failure. No blocking issues. Two suggestions worth considering: the pre-existing is_ascii() filter silently drops non-ASCII UTF-8 data strings (contradicting the 'raw string preserved' claim), and the other Tenderdash error ingestion paths (wait_for_state_transition_result) still pass data through without attempting CBOR decoding.
Reviewed commit: 7ae8846
🟡 3 suggestion(s)
1 additional finding
🟡 suggestion: Other Tenderdash error paths still pass `data` through without CBOR decoding
packages/rs-dapi/src/services/platform_service/wait_for_state_transition_result.rs (lines 151-161)
The new decode_data_message() is only invoked inside impl From<serde_json::Value>, which handles JSON-RPC error responses. The wait_for_state_transition_result path constructs TenderdashStatus::new() directly from ABCI tx_result.data (lines 151–161, 243) without attempting CBOR decoding.
This may be intentional — the ABCI responses could use a different encoding format than JSON-RPC errors. But if they can also carry the same base64-CBOR payloads, clients would still receive unreadable strings on those paths. Worth investigating whether TenderdashStatus::new() should optionally run the same decoding on its message parameter.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dapi/src/services/platform_service/error_mapping.rs`:
- [SUGGESTION] lines 313-314: Pre-existing `is_ascii()` filter silently discards non-ASCII UTF-8 data
The `.filter(|s| s.is_ascii())` on line 313 (pre-existing, not introduced by this PR) means that if Tenderdash ever returns a valid UTF-8 `data` string containing non-ASCII characters (e.g. localized or user-provided error text), it gets silently dropped — `message` becomes `None` and later code falls back to a generic message instead of preserving the server detail.
`serde_json::Value::as_str()` already guarantees well-formed UTF-8, and `base64_decode()` will simply return `None` for non-base64 input, so the `is_ascii()` guard provides no safety benefit here. Removing it would make the "raw string preserved on decode failure" claim fully accurate.
Since this PR is already touching this exact code path, it would be a good opportunity to fix this.
- [SUGGESTION] lines 830-841: No integration test for valid-base64-but-not-CBOR data field
The existing `from_json_value_preserves_plain_text_data` test uses text that fails at the base64 step. Adding a test for a `data` field that base64-decodes successfully but is NOT valid CBOR would confirm the end-to-end fallback through the full `From<serde_json::Value>` conversion, not just the unit function.
In `packages/rs-dapi/src/services/platform_service/wait_for_state_transition_result.rs`:
- [SUGGESTION] lines 151-161: Other Tenderdash error paths still pass `data` through without CBOR decoding
The new `decode_data_message()` is only invoked inside `impl From<serde_json::Value>`, which handles JSON-RPC error responses. The `wait_for_state_transition_result` path constructs `TenderdashStatus::new()` directly from ABCI `tx_result.data` (lines 151–161, 243) without attempting CBOR decoding.
This may be intentional — the ABCI responses could use a different encoding format than JSON-RPC errors. But if they can also carry the same base64-CBOR payloads, clients would still receive unreadable strings on those paths. Worth investigating whether `TenderdashStatus::new()` should optionally run the same decoding on its `message` parameter.
- T4: Use non-logging base64 decode in decode_data_message() to avoid noisy debug logs when data field is plain text (expected failure) - T5: Fix misleading test comment — decode_data_message returns None, caller falls back to raw string - T6: Remove pre-existing .filter(|s| s.is_ascii()) that silently discarded valid non-ASCII UTF-8 data strings - T7: Add test for valid-base64-but-not-CBOR data field fallback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
added
the
ready for final review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Incremental Review
Single-commit push (25de8d2) addressing 2 of 3 prior suggestions. Changes are minimal, correct, and well-tested:
-
is_ascii()filter removed ✅ — resolves the pre-existing filter that silently dropped non-ASCII UTF-8 data strings. Thedecode_data_message()fallback now correctly handles all valid UTF-8. -
base64-non-CBOR integration test added ✅ — new
from_json_value_preserves_base64_non_cbor_dataconfirms end-to-end fallback for data that is valid base64 but not CBOR. -
Inline base64 engine — good call avoiding
base64_decode()debug logging for expected failures on plain-text data fields. The inlineGeneralPurposeengine keeps the same decode config.
All 36 tests pass (cargo test -p rs-dapi --lib error_mapping). No new findings.
Reviewed commit: 25de8d2
…-bin metadata When Drive returns a raw GroveDB storage error for a duplicate identity key (without a serialized ConsensusError), the SDK now checks the drive-error-data-bin gRPC metadata for CBOR-encoded error details. If the decoded message matches the identity key uniqueness pattern, the error is surfaced as Error::AlreadyExists instead of falling through to the opaque Error::DapiClientError. Also adds test fixtures for both the DAPI-level CBOR data field decoding and the SDK-level drive-error-data-bin handling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lklimek
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk/src/error.rs (1)
193-194: Make duplicate-key matching resilient to casing changes.Line 193 and Line 194 use case-sensitive
contains(...). If upstream wording changes capitalization, this mapping silently falls back toError::DapiClientError(_).♻️ Suggested patch
- if message.contains("unique key") && message.contains("already exists") { + let normalized = message.to_ascii_lowercase(); + if normalized.contains("unique key") + && normalized.contains("already exists") + { return Self::AlreadyExists(message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/error.rs` around lines 193 - 194, The duplicate-key detection is using case-sensitive contains on the error message, so capitalization changes cause falls back to DapiClientError; update the matching in the function that returns Self::AlreadyExists (the branch that currently does if message.contains("unique key") && message.contains("already exists")) to perform case-insensitive matching — e.g. normalize the message with message.to_lowercase() (or use a case-insensitive regex) and then check for "unique key" and "already exists", ensuring the same Self::AlreadyExists variant is returned when those substrings appear in any casing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-sdk/src/error.rs`:
- Around line 193-194: The duplicate-key detection is using case-sensitive
contains on the error message, so capitalization changes cause falls back to
DapiClientError; update the matching in the function that returns
Self::AlreadyExists (the branch that currently does if message.contains("unique
key") && message.contains("already exists")) to perform case-insensitive
matching — e.g. normalize the message with message.to_lowercase() (or use a
case-insensitive regex) and then check for "unique key" and "already exists",
ensuring the same Self::AlreadyExists variant is returned when those substrings
appear in any casing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 557e7cca-577e-488e-8ca0-2ba378c1e050
📒 Files selected for processing (2)
packages/rs-dapi/src/services/platform_service/error_mapping.rspackages/rs-sdk/src/error.rs
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "8096689fc50b9520370c382dfee3d22a4da8933da0eaf2e8311808e6f0b247f4"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The new drive-error-data-bin detection code has a semantic conflict with existing recovery logic. When a duplicate identity key error (key belongs to a different identity) is mapped to Error::AlreadyExists, the Identity::wait_for_response() handler misinterprets it as 'this identity was already created', tries to fetch an identity that doesn't exist, and produces a misleading 'identity was proved to not exist but was said to exist' error. The helper function and tests are well-structured but this classification issue needs to be resolved before merging.
Reviewed commit: b18c173
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/error.rs`:
- [BLOCKING] lines 190-198: Mapping duplicate-key errors to AlreadyExists triggers wrong recovery path in Identity::wait_for_response
This new branch converts any `drive-error-data-bin` message containing 'unique key' and 'already exists' into `Error::AlreadyExists`. However, `Identity::wait_for_response()` in `waitable.rs:98-115` catches *every* `Error::AlreadyExists` and assumes the identity was previously created — it extracts the identity ID from the state transition and calls `Identity::fetch(sdk, identity_id)`. For a duplicate-key conflict the *key* belongs to a different identity, but the identity being created does not yet exist. The fetch returns `None` and the user receives the misleading error 'identity was proved to not exist but was said to exist', losing the actual duplicate-key cause entirely.
Either introduce a dedicated variant like `Error::DuplicateIdentityKey(String)` that is not caught by the existing `AlreadyExists` recovery path, or keep the error as `Error::DapiClientError` and let the caller inspect it. If `AlreadyExists` must be reused, update `waitable.rs` to distinguish between 'identity already exists' and 'key already exists' before attempting the fetch.
- [SUGGESTION] lines 193-195: Brittle string matching creates invisible coupling to Drive error format
The pattern `message.contains("unique key") && message.contains("already exists")` matches against Drive/GroveDB's human-readable error text (`UniqueKeyAlreadyExists` display string). If GroveDB or Drive ever changes the wording — even a minor phrasing tweak — this detection silently breaks with no compile-time or test-time signal. A more durable approach would be a machine-readable error code in the CBOR payload (the `code` field is already present but not used for this classification).
- [SUGGESTION] lines 190-192: Silent error swallowing when to_bytes() fails — inconsistent with adjacent code
When `drive_error_value.to_bytes()` returns `Err`, the failure is silently discarded via `if let Ok(bytes)`. The adjacent `consensus_error_value.to_bytes()` path at lines 181-185 logs at debug level and returns a `Generic` error. Adding a `tracing::debug!` log here would maintain consistency and aid debugging.
- [SUGGESTION] lines 190-198: Detection only covers UniqueKeyAlreadyExists, not IdentityKeyAlreadyExists
Drive defines two duplicate-key error variants: `UniqueKeyAlreadyExists` (display: 'a unique key with that hash already exists: ...') and `IdentityKeyAlreadyExists` (display: 'identity key already exists for user error: ...'). The string heuristic only matches the first variant. The second variant has different semantics (same-identity duplicate vs. cross-identity conflict) so this may be intentional, but it's worth documenting the distinction.
…ror-data-bin metadata" This reverts commit b18c173.
…-bin metadata When a gRPC Internal status contains drive-error-data-bin metadata, extract the human-readable message from the CBOR payload and return Error::DriveInternalError instead of opaque Error::DapiClientError. This gives downstream consumers (like DET) a structured error variant for Drive-level failures that bypassed consensus validation. Related: #3396 (pre-validation gap for non-unique key set) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This push correctly addresses the prior blocking issue by replacing the error-prone AlreadyExists mapping with a dedicated DriveInternalError variant, which no longer triggers the wrong recovery path in waitable.rs. The rs-dapi decoding logic and test coverage are solid. One gap: the SDK-side DriveInternalError conversion path and extract_drive_error_message() helper have no unit tests — the prior push's tests were removed along with the string-matching logic.
Reviewed commit: c27a959
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/error.rs`:
- [SUGGESTION] lines 194-210: SDK-side DriveInternalError conversion has no unit tests
The prior push had tests for the `drive-error-data-bin` handling path (`test_drive_error_data_bin_duplicate_identity_key` and `test_drive_error_data_bin_unrelated_message_falls_through`), but they were removed when the string-matching approach was replaced. The new `DriveInternalError` mapping and `extract_drive_error_message()` helper at lines 226-240 have zero test coverage in the SDK.
The rs-dapi side has thorough tests for `decode_data_message()`, but the SDK's `From<DapiClientError>` conversion through `drive-error-data-bin` metadata is a separate code path that deserves its own tests — particularly to verify that:
1. A gRPC `Internal` status with valid CBOR in `drive-error-data-bin` maps to `DriveInternalError`
2. A gRPC `Internal` status *without* `drive-error-data-bin` falls through to `DapiClientError`
3. Non-`Internal` status codes with `drive-error-data-bin` are NOT intercepted (the `Code::Internal` guard)
- [SUGGESTION] lines 198-204: Silent failure when to_bytes() fails on drive-error-data-bin metadata
When `drive_error_value.to_bytes()` returns `Err`, the failure is silently discarded via `if let Ok(bytes)`. The adjacent consensus error path (lines 181-190) handles `to_bytes()` failure by logging at debug level and returning a `Generic` error. Adding a `tracing::debug!` log here would maintain consistency and aid debugging — even a `trace!` level would be better than complete silence.
…ently discarding Addresses PR review feedback: the adjacent consensus error path logs at debug level on to_bytes() failure, but the drive-error-data-bin path silently swallowed the error. Now consistent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Improves error propagation across rs-dapi → gRPC metadata → rs-sdk by decoding Tenderdash data payloads that are base64-wrapped CBOR and surfacing Drive internal error messages as a dedicated SDK error variant.
Changes:
- Decode Tenderdash JSON-RPC
datafield as base64 → CBOR and extract a human-readablemessagewhen the top-level message is empty/“Internal error”. - Add
DriveInternalErrorto the SDK error enum and parsedrive-error-data-binmetadata onInternalgRPC responses to populate it. - Add unit tests in rs-dapi covering plain-text, base64-non-CBOR, and real-world fixtures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/rs-sdk/src/error.rs | Adds DriveInternalError and extracts message from CBOR drive-error-data-bin metadata for Internal gRPC statuses. |
| packages/rs-dapi/src/services/platform_service/error_mapping.rs | Decodes Tenderdash data base64 CBOR to recover readable messages and adds tests for this behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn decode_data_message(data: &str) -> Option<String> { | ||
| // Silently try base64 — failure is expected for plain-text data fields, | ||
| // so we intentionally avoid `base64_decode()` which logs at debug level. | ||
| let decoded_bytes = engine::GeneralPurpose::new( | ||
| &base64::alphabet::STANDARD, | ||
| engine::GeneralPurposeConfig::new() | ||
| .with_decode_allow_trailing_bits(true) | ||
| .with_decode_padding_mode(engine::DecodePaddingMode::Indifferent), | ||
| ) | ||
| .decode(data) | ||
| .ok()?; | ||
|
|
There was a problem hiding this comment.
decode_data_message duplicates the base64 engine configuration that already exists in base64_decode(). To avoid config drift and make future changes safer, consider reusing the same engine (e.g., a shared static/helper) while still keeping this path silent (no debug logging) for expected non-base64 inputs.
| fn decode_data_message(data: &str) -> Option<String> { | |
| // Silently try base64 — failure is expected for plain-text data fields, | |
| // so we intentionally avoid `base64_decode()` which logs at debug level. | |
| let decoded_bytes = engine::GeneralPurpose::new( | |
| &base64::alphabet::STANDARD, | |
| engine::GeneralPurposeConfig::new() | |
| .with_decode_allow_trailing_bits(true) | |
| .with_decode_padding_mode(engine::DecodePaddingMode::Indifferent), | |
| ) | |
| .decode(data) | |
| .ok()?; | |
| /// Shared base64 engine configuration for Tenderdash error payloads. | |
| /// This mirrors the configuration used by other base64 helpers, so that | |
| /// future changes only need to be made in one place. | |
| fn tenderdash_base64_engine() -> engine::GeneralPurpose { | |
| engine::GeneralPurpose::new( | |
| &base64::alphabet::STANDARD, | |
| engine::GeneralPurposeConfig::new() | |
| .with_decode_allow_trailing_bits(true) | |
| .with_decode_padding_mode(engine::DecodePaddingMode::Indifferent), | |
| ) | |
| } | |
| fn decode_data_message(data: &str) -> Option<String> { | |
| // Silently try base64 — failure is expected for plain-text data fields, | |
| // so we intentionally avoid `base64_decode()` which logs at debug level. | |
| let decoded_bytes = tenderdash_base64_engine().decode(data).ok()?; |
There was a problem hiding this comment.
The inline engine in decode_data_message() is intentional — it avoids the debug! logging in base64_decode() which would fire on every plain-text data field (an expected/normal case). A shared tenderdash_base64_engine() helper as suggested could work, but the two call sites have different logging requirements, so extracting a shared engine without also sharing the silence/log behavior doesn't buy much. Leaving as-is for now; the duplication is minimal (one config block) and well-commented.
🤖 Co-authored by Claudius the Magnificent AI Agent
|
⏳ Review in progress (commit 86fbeea) |
… path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lklimek
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk/src/error.rs`:
- Around line 94-103: The new public variant DriveInternalError added to the
Error enum is a semver-breaking change for downstream crates that match
exhaustively; to fix, mark the public enum Error with the #[non_exhaustive]
attribute (above the enum declaration) so future variants (like
DriveInternalError) won’t break consumers, update any internal exhaustive
matches in this crate if necessary, and run cargo check to ensure compilation;
reference the enum named Error and the variant DriveInternalError when making
this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94ce607d-3d24-43e9-b268-e4719b011abe
📒 Files selected for processing (1)
packages/rs-sdk/src/error.rs
Add DriveInternalError to WasmSdkErrorKind enum and match arms to fix compilation after the new variant was added to dash_sdk::Error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The core rs-dapi/rs-sdk fix looks right: the new decoding path matches the existing drive-error-data-bin metadata flow, and the targeted rs-dapi + dash-sdk tests pass locally. One SDK surface still misses the new error kind, though: DriveInternalError is not wired through rs-sdk-ffi, so C/Swift consumers still fall through to the generic NetworkError path instead of seeing the clearer internal/storage failure this PR is trying to surface.
Reviewed commit: f7833a2
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/error.rs`:
- [SUGGESTION] lines 94-102: Adding `DriveInternalError` here leaves the FFI bridge returning `NetworkError`
`rs-sdk-ffi/src/error.rs` still classifies `FFIError::SDKError` by substring matching on `sdk_err.to_string()`. The new `DriveInternalError` variant formats as `"Drive internal error: ..."`, which matches none of the existing branches and therefore falls through to the default `DashSDKErrorCode::NetworkError` / `"Failed to fetch balances: ..."` path. That means the exact duplicate-key / storage errors this PR is trying to decode cleanly are still mislabeled once they cross the C/Swift boundary. Please update the FFI mapper for the new variant (or match the enum directly instead of stringifying it) so all SDK surfaces report the same error class.
| /// Drive returned an internal error that was not classified as a consensus | ||
| /// error. Contains the decoded human-readable message from the | ||
| /// `drive-error-data-bin` gRPC metadata (CBOR → message extraction). | ||
| /// | ||
| /// This typically indicates a storage-level error (e.g., GroveDB constraint | ||
| /// violation) that bypassed the consensus validation layer. If pre-validation | ||
| /// is working correctly, these should be rare. | ||
| #[error("Drive internal error: {0}")] | ||
| DriveInternalError(String), |
There was a problem hiding this comment.
🟡 Suggestion: Adding DriveInternalError here leaves the FFI bridge returning NetworkError
rs-sdk-ffi/src/error.rs still classifies FFIError::SDKError by substring matching on sdk_err.to_string(). The new DriveInternalError variant formats as "Drive internal error: ...", which matches none of the existing branches and therefore falls through to the default DashSDKErrorCode::NetworkError / "Failed to fetch balances: ..." path. That means the exact duplicate-key / storage errors this PR is trying to decode cleanly are still mislabeled once they cross the C/Swift boundary. Please update the FFI mapper for the new variant (or match the enum directly instead of stringifying it) so all SDK surfaces report the same error class.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/error.rs`:
- [SUGGESTION] lines 94-102: Adding `DriveInternalError` here leaves the FFI bridge returning `NetworkError`
`rs-sdk-ffi/src/error.rs` still classifies `FFIError::SDKError` by substring matching on `sdk_err.to_string()`. The new `DriveInternalError` variant formats as `"Drive internal error: ..."`, which matches none of the existing branches and therefore falls through to the default `DashSDKErrorCode::NetworkError` / `"Failed to fetch balances: ..."` path. That means the exact duplicate-key / storage errors this PR is trying to decode cleanly are still mislabeled once they cross the C/Swift boundary. Please update the FFI mapper for the new variant (or match the enum directly instead of stringifying it) so all SDK surfaces report the same error class.
Store protocol version as Arc<AtomicU32> on Sdk (shared between clones). On each gRPC response, compare metadata.protocol_version against stored value and atomically update if newer and known to this binary. Unknown versions are logged at WARN and ignored. Downgrades and zero values are silently skipped. Closes #3410 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nc tests Cover the two remaining test cases for protocol version auto-detection: concurrent thread races converge to highest version, and PlatformVersionCurrentVersion::get_current() is synced after update. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Relax test_global_dpp_version_synced_after_update assertion to account for concurrent test execution — global PlatformVersion is process-wide, so parallel tests may push it higher. Assert >= instead of ==. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
changed the title
lklimek
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I validated the completed reviewer outputs directly against the checked-out PR diff because the Claude verifier lane stalled during ACP startup. The previous cross-language gap from the earlier #3350 review is fixed on this SHA: DriveInternalError now reaches both the FFI and WASM surfaces. One edge case remains in the changed FFI classifier, where some DriveInternalError values are still downgraded to NotFound based on substring order.
Reviewed commit: 60c4d32
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-ffi/src/error.rs`:
- [SUGGESTION] lines 133-138: DriveInternalError can still be reclassified as NotFound in the new FFI mapping
The new `DriveInternalError` case is checked after the generic `"not found"` branch. Because `FFIError::SDKError` classifies errors from the rendered string, a real `dash_sdk::Error::DriveInternalError("...not found...")` will hit `DashSDKErrorCode::NotFound` first and never reach the new `DriveInternalError` code path. That means C/Swift consumers still lose the stronger error type for some internal storage failures, which undermines the purpose of adding code 10 in this PR. The fix is to detect the enum variant before any substring-based fallbacks, or at minimum move the DriveInternalError check ahead of the generic `not found` branch.
| (DashSDKErrorCode::ProtocolError, error_str) | ||
| } else if error_str.contains("not found") || error_str.contains("Not found") { | ||
| (DashSDKErrorCode::NotFound, error_str) | ||
| } else if error_str.contains("Drive internal error") { | ||
| (DashSDKErrorCode::DriveInternalError, error_str) |
There was a problem hiding this comment.
🟡 Suggestion: DriveInternalError can still be reclassified as NotFound in the new FFI mapping
The new DriveInternalError case is checked after the generic "not found" branch. Because FFIError::SDKError classifies errors from the rendered string, a real dash_sdk::Error::DriveInternalError("...not found...") will hit DashSDKErrorCode::NotFound first and never reach the new DriveInternalError code path. That means C/Swift consumers still lose the stronger error type for some internal storage failures, which undermines the purpose of adding code 10 in this PR. The fix is to detect the enum variant before any substring-based fallbacks, or at minimum move the DriveInternalError check ahead of the generic not found branch.
💡 Suggested change
| (DashSDKErrorCode::ProtocolError, error_str) | |
| } else if error_str.contains("not found") || error_str.contains("Not found") { | |
| (DashSDKErrorCode::NotFound, error_str) | |
| } else if error_str.contains("Drive internal error") { | |
| (DashSDKErrorCode::DriveInternalError, error_str) | |
| let (code, detailed_msg) = if matches!(sdk_err, dash_sdk::Error::DriveInternalError(_)) { | |
| (DashSDKErrorCode::DriveInternalError, error_str) | |
| } else if error_str.contains("timeout") | |
| || error_str.contains("Timeout") | |
| { | |
| (DashSDKErrorCode::Timeout, error_str) | |
| } else if error_str.contains("I/O error") || error_str.contains("connection") { | |
| ( | |
| DashSDKErrorCode::NetworkError, | |
| format!("Network connection failed: {}", error_str), | |
| ) | |
| } else if error_str.contains("DAPI") || error_str.contains("dapi") { | |
| if error_str.contains("No available addresses") | |
| || error_str.contains("empty address list") | |
| { | |
| (DashSDKErrorCode::NetworkError, | |
| "Cannot connect to network: No DAPI addresses configured. The SDK needs masternode quorum information to connect to the network.".to_string()) | |
| } else { | |
| ( | |
| DashSDKErrorCode::NetworkError, | |
| format!("DAPI error: {}", error_str), | |
| ) | |
| } | |
| } else if error_str.contains("protocol") || error_str.contains("Protocol") { | |
| (DashSDKErrorCode::ProtocolError, error_str) | |
| } else if error_str.contains("not found") || error_str.contains("Not found") { | |
| (DashSDKErrorCode::NotFound, error_str) | |
| } else { | |
| ( | |
| DashSDKErrorCode::NetworkError, | |
| format!("Failed to fetch balances: {}", error_str), | |
| ) | |
| }; |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-ffi/src/error.rs`:
- [SUGGESTION] lines 133-138: DriveInternalError can still be reclassified as NotFound in the new FFI mapping
The new `DriveInternalError` case is checked after the generic `"not found"` branch. Because `FFIError::SDKError` classifies errors from the rendered string, a real `dash_sdk::Error::DriveInternalError("...not found...")` will hit `DashSDKErrorCode::NotFound` first and never reach the new `DriveInternalError` code path. That means C/Swift consumers still lose the stronger error type for some internal storage failures, which undermines the purpose of adding code 10 in this PR. The fix is to detect the enum variant before any substring-based fallbacks, or at minimum move the DriveInternalError check ahead of the generic `not found` branch.
Issue being fixed or feature implemented
When Tenderdash returns errors with
message: "Internal error", rs-dapi falls through to thedatafield. Tenderdash wraps Drive's CBOR error payload in the JSON-RPCdatafield as a base64 string, but rs-dapi stored it raw — producing unintelligible gRPC status messages likeoWdtZXNzYWdleMVzdG9yYWdl...on the wire.Related: dashpay/dash-evo-tool#766 (DET workaround using message-text matching)
Evidence from DET production logs
144 gRPC errors across all DET logs. Four patterns:
drive-error-data-bindash-serialized-consensus-error-binconsensus_errorin CBORThe majority of errors (99/144) are GroveDB duplicate-key errors during identity create/update where the actual human-readable message was buried in double-encoded CBOR. The 2 consensus errors (DPP 10530) already work correctly via the existing
dash-serialized-consensus-error-binpath.What was done?
rs-dapi (error_mapping.rs)
Added
decode_data_message()helper that tries to decode thedatafield as base64 → CBOR → extract human-readablemessagetext. If decoding fails (plain text data), the raw string is preserved — no regression for existing behavior.rs-sdk (error.rs)
Added
DriveInternalError(String)variant to the SDKErrorenum. When gRPC status isCode::Internalanddrive-error-data-binmetadata is present, the CBOR payload is decoded and the human-readable message is surfaced viaDriveInternalErrorinstead of being lost in a genericDapiClientError.rs-sdk-ffi (error.rs)
Added
DashSDKErrorCode::DriveInternalError = 10and string match branch so FFI consumers (C/Swift) get a dedicated error code instead of falling through toNetworkError.wasm-sdk (error.rs)
Added
WasmSdkErrorKind::DriveInternalErrormapping for the new variant.How Has This Been Tested?
Automated
cargo test -p rs-dapi --lib error_mapping— all 36 tests pass (29 existing + 7 new)cargo test -p dash-sdk --lib— 111 tests pass (4 new fordrive-error-data-binpath)cargo clippy -p rs-dapi -p dash-sdk -p dash-sdk-ffi— cleancargo fmt --all— cleanSDK test coverage added
test_drive_error_data_bin_maps_to_drive_internal_errorCode::Internal→DriveInternalErrortest_internal_error_without_drive_metadata_falls_throughCode::Internalwithout metadata →DapiClientErrortest_non_internal_code_with_drive_metadata_not_interceptedCode::Unavailable+ metadata → NOT interceptedtest_malformed_cbor_in_drive_error_data_bin_falls_throughManual integration test
To reproduce with a real network:
IdentityCreatestate transitionInternalerror with messageoWdtZXNzYWdleMVzdG9yYWdl...(base64 CBOR gibberish)Internalerror with messagestorage: identity: a unique key with that hash already exists: the key already exists in the unique set [...]Breaking Changes
None. Plain text
datafields are preserved as-is. Only base64-encoded CBOR with amessagekey is decoded. The newDriveInternalErrorvariant extends theErrorenum (pre-1.0 SDK, follows platform release cycle).Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent