fix(sdk): failed to deserialize consensus error #2410
Conversation
WalkthroughThis pull request introduces improvements to error handling in the Rust SDK. A new dependency Changes
Sequence DiagramsequenceDiagram
participant SDK as Rust SDK
participant Error as Error Handler
participant Metadata as Consensus Metadata
SDK->>Error: Process DapiClientError
Error->>Metadata: Convert to bytes
Metadata-->>Error: Byte representation
Error->>Error: Attempt deserialization
alt Deserialization Successful
Error-->>SDK: Specific Error Variant
else Deserialization Fails
Error-->>SDK: Generic Error Message
end
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rs-sdk/src/error.rs (2)
91-109: Consider introducing a specialized error variant or detailed message where indicated by the TODO.You have a
TODOat lines 107-108 hinting at creating a specific error to handle invalid consensus error encodings. Instead of defaulting toSelf::Generic(...), introducing a dedicated variant likeError::InvalidConsensusError(...)can improve clarity and make error handling more robust across the codebase.
234-255: Supplement coverage of fixture-basedConsensusErrorwith negative test scenarios.While this fixture-based test is valuable, consider adding a scenario where the fixture data is corrupted or truncated, ensuring that the deserialization fails gracefully and returns the correct fallback error path. This can confirm the defensive behavior in real-world edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
packages/rs-sdk/Cargo.toml(1 hunks)packages/rs-sdk/src/error.rs(2 hunks)
🔇 Additional comments (3)
packages/rs-sdk/src/error.rs (2)
186-194: Confirm coverage scope for the AlreadyExists variant.
The test checks if the AlreadyExists variant is returned when the error code is AlreadyExists. Consider adding an assertion about the error message itself to ensure it matches any expected message format. This can help detect unexpected message changes or translations in the future.
196-232: Good usage of assert_matches to validate consensus error transformations.
The test correctly validates that valid bytes produce the expected ConsensusError, ensuring robust error handling. This is a good demonstration of using assert_matches. Keep up the comprehensive testing approach to ensure new error types don't regress.
packages/rs-sdk/Cargo.toml (1)
59-59: New test dependency is appropriate.
The assert_matches crate is well-suited for pattern-matching-based assertions and keeps tests more expressive and concise. This addition aligns with Rust testing best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rs-sdk/src/error.rs (2)
91-109: LGTM! Robust error handling implementation.The improved error handling for consensus error deserialization is well-structured with:
- Proper byte conversion before deserialization
- Appropriate fallback paths for deserialization failures
- Debug logging for troubleshooting
Consider addressing the TODO comment.
The TODO comment on line 107 suggests introducing a specific error for invalid consensus error encoding cases.
Would you like me to help create a specific error variant for this case? This would improve error handling granularity and make it easier for clients to handle this specific failure mode.
170-257: LGTM! Comprehensive test coverage.The test module is well-structured with:
- Clear test organization in submodules
- Specific test cases for each error scenario
- Good use of fixtures for real-world cases
- Proper use of assert_matches for cleaner assertions
Consider documenting the test fixture.
The base64 encoded test fixture on line 236 would benefit from a comment explaining what consensus error case it represents and how it was generated.
Add a comment before the fixture:
+ // This fixture represents a consensus error for an IdentityAssetLockProofLockedTransactionMismatch + // Generated from a real-world example where... let consensus_error_bytes = base64::engine::general_purpose::STANDARD.decode("ATUgJOJEYbuHBqyTeApO/ptxQ8IAw8nm9NbGROu1nyE/kqcgDTlFeUG0R4wwVcbZJMFErL+VSn63SUpP49cequ3fsKw=").expect("decode base64");
There was a problem hiding this comment.
Tested in Dash Evo Tool
I pushed this fix to test/testWithoutSpan2 as well without the additional tests 8a000df
Issue being fixed or feature implemented
When SDK receives consensus error we get another error:
What was done?
How Has This Been Tested?
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Dependencies
assert_matcheslibrary version 1.5.0 to support testingTesting
AlreadyExistsand consensus errors