fix(dpp): deserialization function for u16 group maps was broken #2621
Conversation
|
""" WalkthroughThe deserialization logic in the Changes
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (19)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
@pauldelucia won't this be the same for "fn deserialize_u16_token_configuration_map<'de, D>("? |
No, tokens don't have the V0 like groups @QuantumExplorer |
![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 (1)
packages/rs-dpp/src/data_contract/serialized_version/v1/mod.rs (1)
86-129: Consider using consistent error message formatting.The error messages are detailed, but there's a slight inconsistency in formatting. Some error messages start with phrases like "invalid group key" while others use "failed to". Consider standardizing the error message format for better consistency.
- serde::de::Error::custom(format!("invalid group key '{}': {}", key_str, e)) + serde::de::Error::custom(format!("failed to parse group key '{}': {}", key_str, e))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
packages/rs-dpp/src/data_contract/serialized_version/v1/mod.rs(2 hunks)
🔇 Additional comments (4)
packages/rs-dpp/src/data_contract/serialized_version/v1/mod.rs (4)
7-7: Good addition of the required import.The
GroupV0import is necessary for the updated deserialization logic to properly handle the group version.
73-85: Excellent documentation enhancement.The docstring clearly explains the expected JSON structure with a practical example, making it much easier for developers to understand the format that the deserialization function expects to handle.
92-129: The deserialization implementation properly handles the nested version structure.This rewrite correctly addresses the issue described in the PR by first deserializing into an intermediate structure and then handling the version before processing the group map. The implementation now:
- Deserializes into a generic
BTreeMap<String, Value>first- Parses each key string into a
GroupContractPositionwith proper error handling- Explicitly extracts and validates the "V0" variant
- Converts to
serde_json::Valueand then deserializes intoGroupV0- Properly wraps in
Group::V0enum variantThe error handling is comprehensive with specific messages for each potential failure point, which will make debugging much easier.
92-129:✅ Verification successful
Verify impact on existing data.
This implementation is more strict, requiring the "V0" variant to be present in every group entry. Please ensure this doesn't break backward compatibility with existing data contracts.
🏁 Script executed:
#!/bin/bash # Look for any existing group data formats that might not follow the new expected structure rg -A 5 '"groups"' --jsonLength of output: 7531
No backward-compatibility issues with existing “groups” data
We searched all JSON vectors in
packages/rs-sdk/tests/vectorsandpackages/rs-drive-abci/testsand only found empty"groups": {}entries. Deserializing an empty map skips the loop (no"V0"lookup), so requiring“V0”only affects non-empty group entries—which don’t exist in current contracts.
QuantumExplorer
changed the title
Issue being fixed or feature implemented
The deserialization function was trying to deserialize into the group map when it was supposed to be deserializing into the group version first and then the group map.
What was done?
Handle the version first then the group map.
How Has This Been Tested?
DET
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit