fix(sdk)!: tenderdash broadcast errors mapped to Internal error #2853
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughIntroduces a centralized Tenderdash-to-Dapi message mapper ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Broadcaster
participant Mapper as map_tenderdash_message
participant Handler as BroadcastErrorHandler
Client->>Broadcaster: submit state transition
Broadcaster->>Broadcaster: attempt broadcast
alt Tenderdash error returned
Broadcaster->>Mapper: map_tenderdash_message(message)
alt Mapper returns Some(DapiError)
Mapper-->>Broadcaster: DapiError
Broadcaster->>Handler: short-circuit with mapped DapiError
Handler-->>Client: return mapped error status
else Mapper returns None
Broadcaster->>Handler: fallback consensus/error parsing
Handler-->>Client: return original/parsed error status
end
else success
Broadcaster-->>Client: success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
✅ 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: "5e6f1c0e23e918ffa1efa12b7af15b6b8e4324c481412e7961a2ed01f1eb9bbf"
)Xcode manual integration:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs(2 hunks)packages/rs-dapi/src/services/platform_service/error_mapping.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-dapi/src/services/platform_service/error_mapping.rspackages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs
🧠 Learnings (13)
📓 Common learnings
Learnt from: shumkov
Repo: dashpay/platform PR: 2255
File: packages/dashmate/src/status/scopes/platform.js:112-116
Timestamp: 2024-10-19T06:10:00.808Z
Learning: In the Dashmate project, when fetching from Tenderdash RPC endpoints like `/status`, `/net_info`, and `/abci_info`, if one request fails, it's likely that the others will fail as well, so individual error handling for each fetch may not be necessary.
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js:33-37
Timestamp: 2024-10-24T04:57:23.753Z
Learning: JSON-RPC errors from Tenderdash can have the same error code even when the error messages are different.
📚 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/src/services/platform_service/error_mapping.rspackages/rs-dapi/src/services/platform_service/broadcast_state_transition.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/src/services/platform_service/error_mapping.rspackages/rs-dapi/src/services/platform_service/broadcast_state_transition.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/src/services/platform_service/error_mapping.rs
📚 Learning: 2025-10-15T14:45:30.856Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-11-20T10:01:50.837Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs:94-94
Timestamp: 2024-11-20T10:01:50.837Z
Learning: In `packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs`, when converting with `PublicKey::try_from`, it's acceptable to use `.expect()` to handle potential conversion errors.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rspackages/rs-dapi/src/services/platform_service/broadcast_state_transition.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/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-10-24T04:57:23.753Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js:33-37
Timestamp: 2024-10-24T04:57:23.753Z
Learning: JSON-RPC errors from Tenderdash can have the same error code even when the error messages are different.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rspackages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs
📚 Learning: 2024-10-19T06:10:00.808Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2255
File: packages/dashmate/src/status/scopes/platform.js:112-116
Timestamp: 2024-10-19T06:10:00.808Z
Learning: In the Dashmate project, when fetching from Tenderdash RPC endpoints like `/status`, `/net_info`, and `/abci_info`, if one request fails, it's likely that the others will fail as well, so individual error handling for each fetch may not be necessary.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rspackages/rs-dapi/src/services/platform_service/broadcast_state_transition.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-dapi/src/services/platform_service/broadcast_state_transition.rs
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs
📚 Learning: 2024-10-24T05:01:51.097Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:90-96
Timestamp: 2024-10-24T05:01:51.097Z
Learning: In `broadcastStateTransitionHandlerFactory.js`, error handling when querying transaction status is handled in the API script.
Applied to files:
packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs
📚 Learning: 2024-10-24T04:59:20.436Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:75-77
Timestamp: 2024-10-24T04:59:20.436Z
Learning: Tenderdash's `tx` RPC method accepts transaction hashes in base64 encoding, or in hex encoding if prefixed with `0x`. Therefore, in `packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js`, it's acceptable to use `stHash.toString('base64')` when calling `requestTenderRpc('tx', { hash: stHash.toString('base64') })`.
Applied to files:
packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs
🧬 Code graph analysis (1)
packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs (1)
packages/rs-dapi/src/services/platform_service/error_mapping.rs (1)
map_tenderdash_message(322-355)
⏰ 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). (2)
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (6)
packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs (2)
13-13: LGTM!The import correctly brings in the centralized error mapping helper.
222-224: Good refactoring to centralize error message mapping.This change effectively consolidates multiple ad-hoc error message checks into a single helper function, improving maintainability. The early return pattern is clean and reduces code duplication.
packages/rs-dapi/src/services/platform_service/error_mapping.rs (4)
9-9: LGTM!The import is necessary for the new
map_tenderdash_messagefunction.
44-50: Good integration with appropriate recursion prevention.The logic correctly maps Tenderdash messages to DapiError variants before falling back to the default status construction. The recursion check prevents infinite loops when
map_tenderdash_messagereturns aTenderdashClientError.
286-296: Good enhancement for extracting informative error messages.Falling back to the "data" field when "message" is empty or generic improves error diagnostics. The ASCII filter is a sensible safety check.
321-355: Request manual verification of Rust formatting and linting.The sandbox environment cannot execute Cargo commands due to
/proclimitations. Please verify the changes meet project standards by running the following locally:cd packages/rs-dapi cargo fmt --check cargo clippy --all-targets --all-features -- -D warningsConfirm that both commands pass without warnings or formatting issues before merging.
lklimek
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-dapi/src/services/platform_service/error_mapping.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2439
File: packages/rs-dpp/src/errors/consensus/state/token/mod.rs:4-22
Timestamp: 2025-01-23T02:10:08.979Z
Learning: The user QuantumExplorer prefers to handle documentation of breaking changes separately from the code changes, particularly for token-related error types and validation rules.
Learnt from: shumkov
Repo: dashpay/platform PR: 2255
File: packages/dashmate/src/status/scopes/platform.js:112-116
Timestamp: 2024-10-19T06:10:00.808Z
Learning: In the Dashmate project, when fetching from Tenderdash RPC endpoints like `/status`, `/net_info`, and `/abci_info`, if one request fails, it's likely that the others will fail as well, so individual error handling for each fetch may not be necessary.
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js:33-37
Timestamp: 2024-10-24T04:57:23.753Z
Learning: JSON-RPC errors from Tenderdash can have the same error code even when the error messages are different.
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:75-77
Timestamp: 2024-10-24T04:59:20.436Z
Learning: Tenderdash's `tx` RPC method accepts transaction hashes in base64 encoding, or in hex encoding if prefixed with `0x`. Therefore, in `packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js`, it's acceptable to use `stHash.toString('base64')` when calling `requestTenderRpc('tx', { hash: stHash.toString('base64') })`.
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:90-96
Timestamp: 2024-10-24T05:01:51.097Z
Learning: In `broadcastStateTransitionHandlerFactory.js`, error handling when querying transaction status is handled in the API script.
📚 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/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-11-20T10:01:50.837Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs:94-94
Timestamp: 2024-11-20T10:01:50.837Z
Learning: In `packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs`, when converting with `PublicKey::try_from`, it's acceptable to use `.expect()` to handle potential conversion errors.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.rs
📚 Learning: 2025-10-15T14:45:30.856Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.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-dapi/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-11-03T10:39:11.242Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2305
File: packages/rs-drive-abci/src/abci/handler/finalize_block.rs:81-0
Timestamp: 2024-11-03T10:39:11.242Z
Learning: In `packages/rs-drive-abci/src/abci/handler/finalize_block.rs`, the use of `.expect("commit transaction")` after `app.commit_transaction(platform_version)` is intentional due to the provided comment explaining its necessity. Do not flag this usage in future reviews.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-10-24T04:57:23.753Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js:33-37
Timestamp: 2024-10-24T04:57:23.753Z
Learning: JSON-RPC errors from Tenderdash can have the same error code even when the error messages are different.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-10-19T06:10:00.808Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2255
File: packages/dashmate/src/status/scopes/platform.js:112-116
Timestamp: 2024-10-19T06:10:00.808Z
Learning: In the Dashmate project, when fetching from Tenderdash RPC endpoints like `/status`, `/net_info`, and `/abci_info`, if one request fails, it's likely that the others will fail as well, so individual error handling for each fetch may not be necessary.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
🔇 Additional comments (2)
packages/rs-dapi/src/services/platform_service/error_mapping.rs (2)
44-50: LGTM - thoughtful recursion guard.The centralized mapping integration correctly short-circuits with mapped DapiError variants. The recursion guard for
TenderdashClientErrorprevents infinite loops if that variant internally wraps a TenderdashStatus.
321-355: LGTM - centralizes Tenderdash error mapping.The new function consolidates scattered error message checks into a single location, improving maintainability. The case-insensitive matching (Line 323) correctly handles variations in Tenderdash error messages.
packages/rs-dapi/src/services/platform_service/error_mapping.rs
Outdated
Show resolved
Hide resolved
…fix/rs-dapi-invalid-error-mapping
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-dapi/src/services/platform_service/error_mapping.rs (1)
341-375: Consider preserving original message casing for user-facing errors.The function lowercases the message on line 343 for case-insensitive comparison, but then returns the lowercased version in some error variants (lines 345, 356, 372). For user-facing error messages, preserving the original casing would be more professional.
To preserve original casing while maintaining case-insensitive comparisons, consider this pattern:
-pub(super) fn map_tenderdash_message(message: &str) -> Option<DapiError> { - let msg = message.trim().to_lowercase(); +pub(super) fn map_tenderdash_message(message: &str) -> Option<DapiError> { + let original = message.trim(); + let msg = original.to_lowercase(); if msg == "tx already exists in cache" { - return Some(DapiError::AlreadyExists(msg.to_string())); + return Some(DapiError::AlreadyExists(original.to_string())); } // ... (similar changes for lines 356 and 372)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/platform-test-suite/test/functional/platform/DataContract.spec.js(1 hunks)packages/rs-dapi/src/services/platform_service/error_mapping.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
🧠 Learnings (12)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2439
File: packages/rs-dpp/src/errors/consensus/state/token/mod.rs:4-22
Timestamp: 2025-01-23T02:10:08.979Z
Learning: The user QuantumExplorer prefers to handle documentation of breaking changes separately from the code changes, particularly for token-related error types and validation rules.
Learnt from: shumkov
Repo: dashpay/platform PR: 2255
File: packages/dashmate/src/status/scopes/platform.js:112-116
Timestamp: 2024-10-19T06:10:00.808Z
Learning: In the Dashmate project, when fetching from Tenderdash RPC endpoints like `/status`, `/net_info`, and `/abci_info`, if one request fails, it's likely that the others will fail as well, so individual error handling for each fetch may not be necessary.
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js:33-37
Timestamp: 2024-10-24T04:57:23.753Z
Learning: JSON-RPC errors from Tenderdash can have the same error code even when the error messages are different.
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:75-77
Timestamp: 2024-10-24T04:59:20.436Z
Learning: Tenderdash's `tx` RPC method accepts transaction hashes in base64 encoding, or in hex encoding if prefixed with `0x`. Therefore, in `packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js`, it's acceptable to use `stHash.toString('base64')` when calling `requestTenderRpc('tx', { hash: stHash.toString('base64') })`.
📚 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/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-11-20T10:01:50.837Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs:94-94
Timestamp: 2024-11-20T10:01:50.837Z
Learning: In `packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs`, when converting with `PublicKey::try_from`, it's acceptable to use `.expect()` to handle potential conversion errors.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.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-dapi/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.rs
📚 Learning: 2025-10-15T14:45:30.856Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-11-03T10:39:11.242Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2305
File: packages/rs-drive-abci/src/abci/handler/finalize_block.rs:81-0
Timestamp: 2024-11-03T10:39:11.242Z
Learning: In `packages/rs-drive-abci/src/abci/handler/finalize_block.rs`, the use of `.expect("commit transaction")` after `app.commit_transaction(platform_version)` is intentional due to the provided comment explaining its necessity. Do not flag this usage in future reviews.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-10-24T04:57:23.753Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js:33-37
Timestamp: 2024-10-24T04:57:23.753Z
Learning: JSON-RPC errors from Tenderdash can have the same error code even when the error messages are different.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-10-19T06:10:00.808Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2255
File: packages/dashmate/src/status/scopes/platform.js:112-116
Timestamp: 2024-10-19T06:10:00.808Z
Learning: In the Dashmate project, when fetching from Tenderdash RPC endpoints like `/status`, `/net_info`, and `/abci_info`, if one request fails, it's likely that the others will fail as well, so individual error handling for each fetch may not be necessary.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
🧬 Code graph analysis (2)
packages/rs-dapi/src/services/platform_service/error_mapping.rs (1)
packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (1)
StateTransitionBroadcastError(97-117)
packages/platform-test-suite/test/functional/platform/DataContract.spec.js (3)
packages/platform-test-suite/test/functional/platform/Document.spec.js (3)
waitForSTPropagated(10-10)client(35-35)getDataContractFixture(7-7)packages/platform-test-suite/test/functional/platform/Identity.spec.js (3)
waitForSTPropagated(8-8)client(36-36)getDataContractFixture(6-6)packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (1)
StateTransitionBroadcastError(97-117)
⏰ 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). (2)
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (4)
packages/platform-test-suite/test/functional/platform/DataContract.spec.js (1)
62-85: LGTM! Well-structured test case for validation error handling.The test properly validates that non-contiguous document property positions are caught and reported with the correct error code (10411) and descriptive message. The use of
skipValidation: trueon line 72 ensures the validation occurs server-side, which aligns with testing the new error mapping behavior introduced in this PR.packages/rs-dapi/src/services/platform_service/error_mapping.rs (3)
44-50: Good defensive recursion prevention.The check for
TenderdashClientErrorprevents potential infinite recursion. Whilemap_tenderdash_messagecurrently never returnsTenderdashClientError, this guard is a good defensive practice for future-proofing.
152-166: LGTM! Clear message extraction priority.The fallback chain (explicit message → consensus error → "Unknown error") is logical and properly handles all cases with appropriate debug logging.
302-317: LGTM! Past review concern addressed.The condition on line 308 now correctly handles empty messages in addition to "Internal error", falling back to the
datafield when the message is uninformative. The ASCII filter ensures safe string handling.
6 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-dapi/src/services/platform_service/error_mapping.rs (1)
326-360: Excellent! Case sensitivity bug from previous review is now fixed.Line 334 now correctly uses lowercase
"tx too large."to match the already-lowercasedmsg, resolving the previous replacement issue.Recommended: Add unit tests for the mapping function.
The
map_tenderdash_messagefunction would benefit from test coverage to verify each error pattern is correctly mapped to the appropriateDapiErrorvariant.Example test structure:
#[test] fn test_map_tenderdash_message() { assert!(matches!( map_tenderdash_message("tx already exists in cache"), Some(DapiError::AlreadyExists(_)) )); assert!(matches!( map_tenderdash_message("TX TOO LARGE. some details"), Some(DapiError::InvalidArgument(_)) )); assert!(matches!( map_tenderdash_message("mempool is full"), Some(DapiError::ResourceExhausted(_)) )); // ... additional cases }Optional: Consider preserving original message casing.
Currently, line 328 converts the entire message to lowercase for matching, but some error variants return this lowercased version (e.g., line 330). For user-facing messages, preserving the original casing might be more professional. You could match on lowercase but extract details from the original message where appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-dapi/src/services/platform_service/error_mapping.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
🧠 Learnings (14)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2439
File: packages/rs-dpp/src/errors/consensus/state/token/mod.rs:4-22
Timestamp: 2025-01-23T02:10:08.979Z
Learning: The user QuantumExplorer prefers to handle documentation of breaking changes separately from the code changes, particularly for token-related error types and validation rules.
Learnt from: shumkov
Repo: dashpay/platform PR: 2255
File: packages/dashmate/src/status/scopes/platform.js:112-116
Timestamp: 2024-10-19T06:10:00.808Z
Learning: In the Dashmate project, when fetching from Tenderdash RPC endpoints like `/status`, `/net_info`, and `/abci_info`, if one request fails, it's likely that the others will fail as well, so individual error handling for each fetch may not be necessary.
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:75-77
Timestamp: 2024-10-24T04:59:20.436Z
Learning: Tenderdash's `tx` RPC method accepts transaction hashes in base64 encoding, or in hex encoding if prefixed with `0x`. Therefore, in `packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js`, it's acceptable to use `stHash.toString('base64')` when calling `requestTenderRpc('tx', { hash: stHash.toString('base64') })`.
📚 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/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-11-20T10:01:50.837Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs:94-94
Timestamp: 2024-11-20T10:01:50.837Z
Learning: In `packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs`, when converting with `PublicKey::try_from`, it's acceptable to use `.expect()` to handle potential conversion errors.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.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-dapi/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.rs
📚 Learning: 2025-10-15T14:45:30.856Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-11-03T10:39:11.242Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2305
File: packages/rs-drive-abci/src/abci/handler/finalize_block.rs:81-0
Timestamp: 2024-11-03T10:39:11.242Z
Learning: In `packages/rs-drive-abci/src/abci/handler/finalize_block.rs`, the use of `.expect("commit transaction")` after `app.commit_transaction(platform_version)` is intentional due to the provided comment explaining its necessity. Do not flag this usage in future reviews.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.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/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-10-24T04:57:23.753Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js:33-37
Timestamp: 2024-10-24T04:57:23.753Z
Learning: JSON-RPC errors from Tenderdash can have the same error code even when the error messages are different.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.rs
📚 Learning: 2024-10-19T06:10:00.808Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2255
File: packages/dashmate/src/status/scopes/platform.js:112-116
Timestamp: 2024-10-19T06:10:00.808Z
Learning: In the Dashmate project, when fetching from Tenderdash RPC endpoints like `/status`, `/net_info`, and `/abci_info`, if one request fails, it's likely that the others will fail as well, so individual error handling for each fetch may not be necessary.
Applied to files:
packages/rs-dapi/src/services/platform_service/error_mapping.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). (1)
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (3)
packages/rs-dapi/src/services/platform_service/error_mapping.rs (3)
45-51: LGTM! Clean integration of centralized error mapping.The pre-mapping step with recursion guard is well-designed. This ensures Tenderdash messages are mapped to appropriate DapiError variants before falling back to generic status construction.
155-155: Good improvement in consistency.Using
grpc_message()ensures the message derivation logic (including consensus error fallback) is applied consistently across all conversions.
287-302: Well done! Previous concern about empty messages is now addressed.The condition at lines 292-293 now correctly handles empty/whitespace-only messages in addition to
Noneand "Internal error", properly falling back to thedatafield when themessagefield is uninformative.
| let status_message = self.grpc_message(); | ||
|
|
||
| // check if we can map to a DapiError first | ||
| if let Some(dapi_error) = map_tenderdash_message(&status_message) { |
Member
There was a problem hiding this comment.
can we not do this by code? This seems very brittle.
Contributor
Author
There was a problem hiding this comment.
We would need to also refactor tenderdash. Right now, it returns -32603 which is CodeInternalError for multiple error types.
I'd prefer to leave it behind and fix it separately.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
rs-dapi hides some error details, returning "internal error" message.
What was done?
Fixed error handling to map errors to correct error codes, and include respective error messages.
How Has This Been Tested?
Using dash-evo-tool
Breaking Changes
Some error codes/messages might have changed.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
Tests