fix(dpp): add toJSON() serialization to TokenContractInfoWasm#3089
fix(dpp): add toJSON() serialization to TokenContractInfoWasm#3089thepastaclaw wants to merge 4 commits intodashpay:v3.1-devfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds wasm-bindgen JSON serialization support for TokenContractInfo in wasm bindings (new Changes
Sequence Diagram(s)mermaid 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
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
Summary by CodeRabbit