fix(wasm-sdk): include groups and tokens in data contract serialization #2794
Conversation
DataContractWasm was using PlatformVersion::first() which defaults to V0 serialization format that excludes groups and tokens fields. Updated to use the SDK's platform version for proper V1 serialization support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Store as DataContract Store
participant DCW as DataContractWasm
Caller->>Store: fetch_by_identifier(...)
alt found
Store-->>Caller: DataContract
Caller->>DCW: DataContractWasm::from_data_contract(contract)
DCW-->>Caller: DataContractWasm
else not found
Store-->>Caller: NotFound
end
sequenceDiagram
autonumber
participant Caller as Caller
participant DCW as DataContractWasm
participant PV as PlatformVersion
Caller->>DCW: to_json(platform_version: u32)
DCW->>PV: PlatformVersion::get(platform_version)
alt known
PV-->>DCW: &PlatformVersion
DCW-->>Caller: JsValue (serialized)
else unknown
PV-->>DCW: Err
DCW-->>Caller: WasmSdkError::invalid_argument
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (6)
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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/wasm-sdk/src/dpp.rs(2 hunks)packages/wasm-sdk/src/queries/data_contract.rs(1 hunks)packages/wasm-sdk/src/verify.rs(1 hunks)
🔇 Additional comments (3)
packages/wasm-sdk/src/verify.rs (1)
125-125: Consider consistent platform version selection across verification and runtime.This function uses
PlatformVersion::latest()whereasget_data_contractindata_contract.rsusesPlatformVersion::get(self.version()). While this may be acceptable for verification/testing functions, the inconsistency could lead to subtle bugs if verification logic diverges from runtime behavior.Consider either:
- Making verify functions accept a platform version parameter for consistency
- Documenting why
latest()is appropriate for verification contextsBased on the PR comments discussion about inconsistent platform version usage.
packages/wasm-sdk/src/dpp.rs (2)
298-298: Document the safety of the'staticlifetime requirement.The
&'static PlatformVersionreference requires that allPlatformVersioninstances outlive theDataContractWasm. This is likely safe ifPlatformVersioninstances are global singletons, but this constraint should be documented.Consider adding a doc comment explaining:
- Why
'staticis safe (e.g., "PlatformVersion instances are global singletons")- Or using a generic lifetime
'aif the'staticconstraint is too restrictiveBased on the PR comments about the
'staticlifetime concern.
320-326: Core fix correctly implemented.The
to_jsonmethod now uses the stored platform version (self.1) instead of the hardcodedPlatformVersion::first(). This ensures that version-specific fields likegroupsandtokensare included when serializing token contracts.
packages/wasm-sdk/src/dpp.rs
Outdated
| fn from(value: DataContract) -> Self { | ||
| Self(value) | ||
| // Use first version as fallback for backward compatibility | ||
| Self(value, PlatformVersion::first()) |
There was a problem hiding this comment.
I don't think this is a safe approach. For now, I would just pass platfrom version to all methods that require it (as rust dpp does). There are non-instance methods and you would need to do it anyway.
There was a problem hiding this comment.
Pushed some updates, but I see you've been working on dpp.rs in #2791 so maybe I will pause (since I don't exactly know what I'm doing here 🤪)
… methods Remove unsafe platform version fallback from DataContractWasm: - Remove stored &'static PlatformVersion field from struct - Remove From<DataContract> trait implementation using PlatformVersion::first() - Update toJSON method to accept platform_version parameter - Add internal from_data_contract helper for codebase use This ensures platform version is always passed explicitly to methods that require it, following the same pattern as Rust DPP implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/wasm-sdk/src/verify.rs (1)
114-125: Propagate platform version through verify_data_contract
verify_data_contractverifies the proof withPlatformVersion::latest()but returns aDataContractWasmwithout storing that version, whileDataContractWasm.toJSON()requires the caller to supply a version. This risks version mismatches.Consider updating
DataContractWasm::from_data_contract(or its signature) to accept and store the platform version, returning it alongside the contract, or documenting that callers must forward the same version intotoJSON().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/wasm-sdk/src/dpp.rs(2 hunks)packages/wasm-sdk/src/queries/data_contract.rs(1 hunks)packages/wasm-sdk/src/verify.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wasm-sdk/src/queries/data_contract.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/wasm-sdk/src/verify.rspackages/wasm-sdk/src/dpp.rs
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/src/verify.rspackages/wasm-sdk/src/dpp.rs
🧬 Code graph analysis (2)
packages/wasm-sdk/src/verify.rs (1)
packages/wasm-sdk/src/dpp.rs (1)
from_data_contract(301-303)
packages/wasm-sdk/src/dpp.rs (1)
packages/wasm-sdk/src/error.rs (1)
invalid_argument(73-75)
⏰ 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). (6)
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (2)
packages/wasm-sdk/src/dpp.rs (2)
313-318: Platform version parameter correctly addresses root cause.The updated signature requiring an explicit
platform_version: u32parameter directly fixes the original bug where serialization used a hardcodedPlatformVersion::first(). The error handling for unknown platform versions is appropriate.
300-330: Changes align with maintainer guidance and resolve prior concerns.The removal of the
From<DataContract>implementation and the requirement to passplatform_versionexplicitly toto_json()address both:
- The dangerous fallback that could reintroduce the bug (past review comment about
PlatformVersion::first())- The maintainer's preference (shumkov's comment) to pass platform version to methods as needed rather than storing it in the struct
This approach correctly ensures that serialization uses the intended platform version for each call, preventing omission of version-specific fields like
groupsandtokens.Based on past review comments.
| impl From<DataContract> for DataContractWasm { | ||
| fn from(value: DataContract) -> Self { | ||
| Self(value) | ||
| impl DataContractWasm { |
There was a problem hiding this comment.
I went through DPP methods. Actually, I think your idea to pass the platfrom version to the constructor was the best option we have. But all constructors should always require platform version then. We shouldn't create with default or first version in any case. I'm sorry, that I wasn't clear enough in my first comment, so you reverted half correct code.
Issue being fixed or feature implemented
The wasm-sdk was not returning
groupsandtokensfields when fetching data contracts that define tokens, making it impossible to properly work with token contracts through the SDK.What was done?
Fixed the data contract serialization in wasm-sdk to use the SDK's platform version instead of hardcoded
PlatformVersion::first(). This ensures that contracts are serialized using the V1 format which includesgroupsandtokensfields when the platform version supports it.Changes made:
DataContractWasmstruct to store the platform version referenceto_json()method to use the stored platform versionHow Has This Been Tested?
PlatformVersion::first()when neededBreaking Changes
None - This is a backward-compatible fix that preserves existing behavior while enabling proper serialization for newer platform versions.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes