fix(dpp): add toJSON() serialization to TokenContractInfoWasm#3089
fix(dpp): add toJSON() serialization to TokenContractInfoWasm#3089thepastaclaw wants to merge 5 commits intodashpay:v3.1-devfrom
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds serde-based JSON serialization and TypeScript interop for TokenContractInfo in the wasm bindings, and conditionally enables serde derives (with camelCase rename for v0) for TokenContractInfo types in rs-dpp behind feature flags. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as Client (JS)
participant WB as wasm-bindgen glue
participant R as Rust TokenContractInfoWasm
participant S as serde::to_string / conversions
JS->>WB: call obj.toJSON()
WB->>R: invoke to_json() (wasm wrapper)
R->>S: serialize inner TokenContractInfo -> TokenContractInfoJSONJs
S-->>R: TokenContractInfoJSONJs
R-->>WB: WasmDppResult<TokenContractInfoJSONJs>
WB-->>JS: return JSON object
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@packages/wasm-dpp2/src/tokens/contract_info.rs`:
- Line 53: Replace the direct cast to f64 in the JsValue conversion by using the
From conversion: instead of using "self.token_contract_position() as f64" (where
token_contract_position() returns TokenContractPosition/u16), call
f64::from(self.token_contract_position()) so clippy's cast_lossless lint is
satisfied; update the expression that constructs JsValue (the line with
&JsValue::from_f64(...)) accordingly.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@packages/wasm-dpp2/src/tokens/contract_info.rs`: - Line 53: Replace the direct cast to f64 in the JsValue conversion by using the From conversion: instead of using "self.token_contract_position() as f64" (where token_contract_position() returns TokenContractPosition/u16), call f64::from(self.token_contract_position()) so clippy's cast_lossless lint is satisfied; update the expression that constructs JsValue (the line with &JsValue::from_f64(...)) accordingly.packages/wasm-dpp2/src/tokens/contract_info.rs (1)
53-53: Replaceas f64withf64::from()to comply with clippy'scast_losslesslint.Since
token_contract_position()returnsTokenContractPosition(which isu16), the cast tof64is a lossless widening operation. Clippy'scast_losslesslint prefers explicitFromconversions for such casts.Suggested fix
- &JsValue::from_f64(self.token_contract_position() as f64), + &JsValue::from_f64(f64::from(self.token_contract_position())),🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@packages/wasm-dpp2/src/tokens/contract_info.rs` at line 53, Replace the direct cast to f64 in the JsValue conversion by using the From conversion: instead of using "self.token_contract_position() as f64" (where token_contract_position() returns TokenContractPosition/u16), call f64::from(self.token_contract_position()) so clippy's cast_lossless lint is satisfied; update the expression that constructs JsValue (the line with &JsValue::from_f64(...)) accordingly.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Can you confirm we shouldn't add this lower in the stack? we generally want to make changes like in the rs-sdk if possible, and then just expose them in the higher levels. If this is relevant here, please make the change |
|
Good point. The cleaner approach would be to add serde derives to Want me to update this PR to do it that way? |
|
Good question. I looked into this — This is the correct layer ( That said, if you'd prefer adding |
|
Good question — I looked into this and the fix is correctly scoped to the wasm layer. The underlying Rust type ( The rs-sdk layer doesn't (and shouldn't) know about JavaScript's So no changes needed lower in the stack here. |
|
Applied the |
shumkov
left a comment
There was a problem hiding this comment.
You should look on how we inmplement conversion methods for other dpp2 entities and do the same way.d
|
Thanks @shumkov — will look at the existing dpp2 entity conversion patterns and align |
|
Refactored per review feedback (commit Changed from manual #[wasm_bindgen(js_name = "toJSON")]
pub fn to_json(&self) -> WasmDppResult<TokenContractInfoJSONJs> {
let js_value = serialization::to_json(&self.0)?;
Ok(js_value.into())
}This required adding feature-gated serde derives to the underlying types:
Also added proper TypeScript type definition ( |
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-dpp/src/tokens/contract_info/mod.rs`:
- Around line 27-36: The TokenContractInfo enum's serde attributes lack
untagged, causing JSON to be wrapped with a "V0" key; update the cfg_attr on
TokenContractInfo (the same attribute block that currently derives
serde::Serialize and serde::Deserialize) to include serde(untagged) so that
TokenContractInfo serializes to the flat shape expected by
TokenContractInfoJSON; follow the same pattern used in state_transition/mod.rs
when adding serde(untagged).
|
Good catch on the missing |
|
CI failures here are all base branch ( Ready to merge once base CI stabilizes. |
03208e7 to
1b23e6b
Compare
Fix: Swift SDK build — missing SPV symbolsRoot cause: This meant the SPV library was being built successfully but never found during the merge step. The XCFramework was created with only Fix: Changed all 6 This is a platform-wide issue affecting all PRs, not just this one. Commit: 4240f62 |
4240f62 to
1b23e6b
Compare
|
Dropped the swift-sdk build_ios.sh commit — that's a pre-existing issue already fixed by #3107 (lklimek). Shouldn't have been bundled here. |
TokenContractInfoWasm was missing toJSON() support, causing it to
serialize as an empty object {}. Add a manual toJSON() method that
returns contractId (as base58 string) and tokenContractPosition.
Fixes dashpay#3027
…nContractInfo.toJSON() Replace manual js_sys::Reflect::set approach with serialization::to_json(), matching the pattern used by Identity, DataContract, and other dpp2 types. Add serde Serialize/Deserialize derives to TokenContractInfo and TokenContractInfoV0 behind feature flags. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…alization Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1b23e6b to
f12983f
Compare
…kenContractInfo Use impl_wasm_conversions! macro with #[serde(transparent)] wrapper following the same pattern as ExtendedEpochInfoWasm and other dpp2 entities. This provides toObject/fromObject/toJSON/fromJSON via the standard serialization infrastructure instead of a manual toJSON() only. Addresses review feedback from shumkov.
|
Refactored to follow the standard dpp2 conversion conventions (modeled after
Verification: |
Summary
TokenContractInfoWasmwas missingtoJSON()support, causing it to serialize as an empty object{}when called from JavaScript.Changes
Added a manual
toJSON()method toTokenContractInfoWasmthat returns:contractId— base58-encoded stringtokenContractPosition— numeric positionWhy manual instead of serde?
The inner
TokenContractInfotype fromrs-dppdoesn't derive serdeSerialize/Deserialize, so we can't use theserialization::to_json()helper or theimpl_wasm_conversions!macro. Instead, we build the JSON object manually using the existing getters, similar to the pattern used elsewhere in the codebase.Fixes #3027
Validation
Build verification
cargo check -p dpp— confirms the conditionalserde::Serialize/serde::Deserializederives onTokenContractInfoandTokenContractInfoV0compile correctly under thefixtures-and-mocksandstate-transition-serde-conversionfeaturescargo check -p wasm-dpp2— confirms the newto_json()method onTokenContractInfoWasmcompiles and theserialization::to_jsonintegration is wired up properlyTest verification
cargo test -p dpp— ensures existing rs-dpp tests still pass with the new serde derives (thecfg_attrgating means production builds without the feature flags are unaffected)cargo test -p wasm-dpp2— validates the wasm-dpp2 package tests pass with the newtoJSON()methodCI
PR CI runs the full platform test matrix including
cargo check,cargo test, clippy, and formatting across all packages — no regressions expected since the serde derives are feature-gated and the WASM method follows the existingserialization::to_jsonpattern used by other types.Known pre-existing CI failure:
JS packages (@dashevo/dapi) / TestsThe
JS packages (@dashevo/dapi) / Testscheck fails on this PR with a NaN v2 / Node.js 24 incompatibility inssh2@npm:1.11.0:This failure is pre-existing and unrelated to this PR. This PR only modifies
wasm-dpp2. The dapi test job is triggered whenever wasm-dpp is changed (CI matrix dependency), but the failure originates from a NaN v2 native addon build error in@dashevo/dapi's transitivessh2dependency against Node 24. PRs that don't touch wasm-dpp show this check as skipping rather than failing.Summary by CodeRabbit