feat(sdk): add platform address transition WASM bindings#3147
Open
feat(sdk): add platform address transition WASM bindings#3147
Conversation
Contributor
|
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. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughAdds six new wasm-bindgen transition bindings under Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript Client
participant WASM as wasm-dpp2 bindings
participant Core as Rust Transition (V0)
participant ST as StateTransitionWasm
JS->>WASM: call Constructor(options)
WASM->>WASM: parse JS options (inputs/outputs/feeStrategy/fields)
WASM->>Core: build V0 transition struct
Core-->>WASM: return transition instance
WASM->>WASM: to_bytes()/to_hex()/to_base64()
WASM->>ST: to_state_transition()
ST-->>WASM: state transition wrapper
WASM-->>JS: return constructed TransitionWasm or serialized data
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/wasm-dpp2/src/platform_address/transitions/identity_credit_transfer_to_addresses_transition.rs (1)
159-175: Sameoutputs_to_btree_mappanic risk applies here.
set_recipient_addresses(Line 174) and the constructor (Line 96) both useoutputs_to_btree_map, which internally callsinto_inner()with an.expect(). If aPlatformAddressOutputwithout an amount reaches this path, it will panic. This is the same root cause flagged in theaddress_credit_withdrawal_transition.rsreview.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/src/platform_address/transitions/identity_credit_transfer_to_addresses_transition.rs` around lines 159 - 175, The setter set_recipient_addresses calls outputs_to_btree_map which currently uses into_inner().expect() and can panic if a PlatformAddressOutput lacks an amount; change outputs_to_btree_map to return a Result<BTreeMap<...>, JsValue> (or custom error) instead of unwrapping, update set_recipient_addresses (and the constructor that also calls outputs_to_btree_map) to handle the Result by propagating the error to JS (e.g., return Result<(), JsValue> or use wasm_bindgen::throw_str) and validate each PlatformAddressOutputWasm for a present amount before converting so no .expect() is used. Ensure references are updated for outputs_to_btree_map, set_recipient_addresses, and the constructor that calls outputs_to_btree_map.
🧹 Nitpick comments (9)
packages/wasm-dpp2/tests/unit/AddressFundsTransfer.spec.ts (1)
42-45: Filename uses PascalCase instead of kebab-case.Per coding guidelines, files matching
packages/**/!(node_modules)/**/*.{js,jsx,ts,tsx}should use kebab-case for filenames. This file isAddressFundsTransfer.spec.tsbut should beaddress-funds-transfer.spec.ts. The same applies to the other new test files in this PR. However, if PascalCase is the established convention inwasm-dpp2tests, feel free to disregard.As per coding guidelines: "Use kebab-case for filenames within JS packages"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/tests/unit/AddressFundsTransfer.spec.ts` around lines 42 - 45, The test filename uses PascalCase (AddressFundsTransfer.spec.ts) but should follow the repo's kebab-case rule; rename the file to address-funds-transfer.spec.ts and update any imports or test references that point to AddressFundsTransfer.spec.ts (and do the same for other new test files in this PR) so test runner and imports resolve correctly; verify references in test suites or any index files that mention AddressFundsTransferTransition or the skipped describe('AddressFundsTransferTransition (.build API)') remain valid after the rename.packages/wasm-dpp2/src/platform_address/input_output.rs (1)
186-265: Consider extracting a generic helper to reduce duplication betweeninputs_from_js_optionsandoutputs_from_js_options.Both functions share identical scaffolding (Reflect::get → null check → array check → enumerate →
to_wasm→ clone →map_err), differing only in the Wasm type and its name string. A generic helper parameterized on the target type would eliminate this ~40-line duplication. Note thatfee_strategy_from_js_optionsalready uses thetry_from_options_optional_with+try_to_arrayutils for similar work but with optional semantics.Not blocking — deferring is fine if the current clarity is preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/src/platform_address/input_output.rs` around lines 186 - 265, Extract a generic helper (e.g., items_from_js_options<T>) that encapsulates the common scaffolding in inputs_from_js_options and outputs_from_js_options: read the property via js_sys::Reflect::get, check undefined/null, ensure it's an array, iterate/enumerate, call item.to_wasm::<T>(type_name) and clone the inner value, and map errors with the same "{}[{}] is not a <Type>" message; make the helper accept the JsValue options, field_name: &str, type_name: &str and a generic T used in to_wasm (with the same trait bounds required for cloning), then replace inputs_from_js_options and outputs_from_js_options to call this helper and return its result (keep the existing WasmDppError messages and signatures).packages/wasm-dpp2/tests/unit/AddressFundingFromAssetLockTransition.spec.ts (1)
164-172: Consider asserting the outputamountafter the setter.The
outputssetter test verifies the array length but not the updatedamountvalue (BigInt(50000)). A value assertion would strengthen this test.✏️ Suggested addition
transition.outputs = [newOutput]; const outputs = transition.outputs; expect(outputs).to.have.lengthOf(1); + expect(outputs[0].amount).to.equal(BigInt(50000));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/tests/unit/AddressFundingFromAssetLockTransition.spec.ts` around lines 164 - 172, The test currently only checks the outputs array length; after setting transition.outputs to [newOutput] also assert the amount on the stored output matches the intended BigInt(50000). Locate the test using createTransition(), new wasm.PlatformAddressOutput(newAddr, BigInt(50000)) and transition.outputs, then add an assertion that the retrieved outputs[0] (or outputs.at(0)) has an amount equal to BigInt(50000) to validate the setter preserved the value.packages/wasm-dpp2/tests/unit/IdentityCreateFromAddressesTransition.spec.ts (1)
81-100: MissingtoHex/fromHexround-trip coverage — same gap asIdentityTopUpFromAddressesTransition.spec.ts. Consider adding atoHex() / fromHex()describe block for consistency with the other transition suites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/tests/unit/IdentityCreateFromAddressesTransition.spec.ts` around lines 81 - 100, Add missing hex round-trip tests for IdentityCreateFromAddressesTransition: add a describe('toHex() / fromHex()') block mirroring the existing toBase64/toBytes tests, create a transition via createTransition(), call transition.toHex(), verify Buffer.from(hex, 'hex') equals Buffer.from(transition.toBytes()), then restore with wasm.IdentityCreateFromAddressesTransition.fromHex(hex) and assert Buffer.from(restored.toBytes()) equals the original bytes; reference the existing createTransition(), transition.toHex(), wasm.IdentityCreateFromAddressesTransition.fromHex(), transition.toBytes(), and restored.toBytes() methods when locating where to insert the new test.packages/wasm-dpp2/tests/unit/IdentityTopUpFromAddressesTransition.spec.ts (1)
76-95: MissingtoHex/fromHexround-trip coverage.The other transition spec files (
AddressFundsTransferTransition.spec.ts,IdentityCreditTransferToAddresses.spec.ts,AddressFundingFromAssetLockTransition.spec.ts) all include atoHex() / fromHex()suite. Consider adding one here for consistency:✏️ Suggested addition
+ describe('toHex() / fromHex()', () => { + it('should round-trip via hex', () => { + const transition = createTransition(); + const hex = transition.toHex(); + const bytes = transition.toBytes(); + + const restored = wasm.IdentityTopUpFromAddressesTransition.fromHex(hex); + expect(Buffer.from(restored.toBytes())).to.deep.equal(Buffer.from(bytes)); + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/tests/unit/IdentityTopUpFromAddressesTransition.spec.ts` around lines 76 - 95, Add a toHex()/fromHex() round-trip test for IdentityTopUpFromAddressesTransition similar to other transition specs: create a transition via createTransition(), call transition.toHex(), assert Buffer.from(hex, 'hex') equals Buffer.from(transition.toBytes()), then restore with wasm.IdentityTopUpFromAddressesTransition.fromHex(hex) and assert Buffer.from(restored.toBytes()) equals the original bytes; place this in a new describe('toHex() / fromHex()') / it('should round-trip via hex') block alongside the existing toBytes/toBase64 tests.packages/wasm-dpp2/tests/unit/AddressCreditWithdrawalTransition.spec.ts (1)
78-97: MissingtoHex/fromHexround-trip coverage — same gap as two other spec files in this PR. Consider adding atoHex() / fromHex()describe block for consistency across the transition suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/tests/unit/AddressCreditWithdrawalTransition.spec.ts` around lines 78 - 97, Add a new describe block testing toHex()/fromHex() round-trip for AddressCreditWithdrawalTransition: call createTransition(), get hex via transition.toHex(), assert Buffer.from(hex, 'hex') equals Buffer.from(transition.toBytes()), then restore via wasm.AddressCreditWithdrawalTransition.fromHex(hex) and assert Buffer.from(restored.toBytes()) equals the original bytes; place the test alongside the existing toBytes/toBase64 blocks and use the same createTransition helper and wasm.AddressCreditWithdrawalTransition methods.packages/wasm-dpp2/src/platform_address/transitions/identity_create_from_addresses_transition.rs (2)
163-179: Same unnecessary clone pattern inset_public_keys.
keysis a localVecthat's not reused —into_iter()avoids the per-element clone.♻️ Suggested optimization
IdentityCreateFromAddressesTransition::V0(v0) => { - v0.public_keys = keys - .iter() - .map(|k| IdentityPublicKeyInCreation::from(k.clone())) + v0.public_keys = keys + .into_iter() + .map(|k| IdentityPublicKeyInCreation::from(k)) .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/src/platform_address/transitions/identity_create_from_addresses_transition.rs` around lines 163 - 179, The setter set_public_keys currently iterates over the local Vec keys with keys.iter() and calls k.clone() for each element; replace that clone pattern by consuming the vector with keys.into_iter() and mapping each IdentityPublicKeyInCreationWasm directly into IdentityPublicKeyInCreation (i.e., use IdentityPublicKeyInCreation::from(k) inside the map) when assigning to v0.public_keys in the IdentityCreateFromAddressesTransition::V0 arm to avoid unnecessary per-element cloning of IdentityPublicKeyInCreationWasm.
94-105: Unnecessary.clone()—public_keysis not used after this point.
public_keys.iter().map(|k| k.clone().into())borrows the vec and clones each element. Sincepublic_keysis a local that's not used afterwards,.into_iter()avoids the clone.♻️ Suggested optimization
IdentityCreateFromAddressesTransitionV0 { - public_keys: public_keys.iter().map(|k| k.clone().into()).collect(), + public_keys: public_keys.into_iter().map(|k| k.into()).collect(), inputs: inputs_map,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/src/platform_address/transitions/identity_create_from_addresses_transition.rs` around lines 94 - 105, The public_keys vector is cloned unnecessarily when constructing IdentityCreateFromAddressesTransitionV0; update the construction in IdentityCreateFromAddressesTransitionWasm / IdentityCreateFromAddressesTransition::V0 / IdentityCreateFromAddressesTransitionV0 to consume public_keys instead of iterating and cloning it by replacing public_keys.iter().map(|k| k.clone().into()) with an into-iterator that converts each element (e.g., public_keys.into_iter().map(Into::into)), since public_keys is not used after this point.packages/wasm-dpp2/src/platform_address/transitions/address_funding_from_asset_lock_transition.rs (1)
153-204: Minor inconsistency:inputsuses manual match whileasset_lock_proofandoutputsuse accessor trait methods.The
inputsgetter/setter (lines 163–184) manually matches onV0, whileasset_lock_proofandoutputsuse theAddressFundingFromAssetLockTransitionAccessorsV0trait methods. This works fine, but if an accessor forinputsbecomes available on the trait in the future, consider aligning for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/src/platform_address/transitions/address_funding_from_asset_lock_transition.rs` around lines 153 - 204, The inputs getter/setter manually pattern-match on AddressFundingFromAssetLockTransition::V0 while asset_lock_proof and outputs use the AddressFundingFromAssetLockTransitionAccessorsV0 trait; change inputs() and set_inputs() to call the same accessor methods on self.0 (e.g., use AddressFundingFromAssetLockTransitionAccessorsV0::inputs and ::set_inputs or the impl-provided inputs()/set_inputs() methods) and convert between PlatformAddressInputWasm and the inner types as before so the code consistently uses the transition accessor API (update the inputs() getter to iterate over self.0.inputs() and the set_inputs() to call self.0.set_inputs(inputs_map)).
🤖 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/wasm-dpp2/src/platform_address/transitions/address_credit_withdrawal_transition.rs`:
- Around line 179-206: The setter AddressCreditWithdrawalTransition::set_output
currently calls PlatformAddressOutputWasm::into_inner(), which can panic if the
PlatformAddressOutput has no amount; replace this with a fallible conversion
(e.g., add and call a try_into_inner() on PlatformAddressOutputWasm that returns
WasmDppResult<PlatformAddressOutput>) or explicitly validate the output_wasm has
an amount before converting and return a WasmDppError (invalid_argument) instead
of panicking. Update callers that use into_inner() (including
outputs_to_btree_map and implementations in AddressFundsTransferTransition and
other transition setters) to use the new try_into_inner() or the
validation+error pattern so all setters return graceful WasmDppResult errors
instead of causing a WASM panic.
In `@packages/wasm-dpp2/src/platform_address/transitions/mod.rs`:
- Line 10: The import line uses the misspelled module name
address_funds_transfer_transsition (extra "s"), causing an unresolved import for
AddressFundsTransferTransitionWasm; change the module path to the correct
address_funds_transfer_transition so the pub use
AddressFundsTransferTransitionWasm; resolves to the declared module name.
---
Duplicate comments:
In
`@packages/wasm-dpp2/src/platform_address/transitions/identity_credit_transfer_to_addresses_transition.rs`:
- Around line 159-175: The setter set_recipient_addresses calls
outputs_to_btree_map which currently uses into_inner().expect() and can panic if
a PlatformAddressOutput lacks an amount; change outputs_to_btree_map to return a
Result<BTreeMap<...>, JsValue> (or custom error) instead of unwrapping, update
set_recipient_addresses (and the constructor that also calls
outputs_to_btree_map) to handle the Result by propagating the error to JS (e.g.,
return Result<(), JsValue> or use wasm_bindgen::throw_str) and validate each
PlatformAddressOutputWasm for a present amount before converting so no .expect()
is used. Ensure references are updated for outputs_to_btree_map,
set_recipient_addresses, and the constructor that calls outputs_to_btree_map.
---
Nitpick comments:
In `@packages/wasm-dpp2/src/platform_address/input_output.rs`:
- Around line 186-265: Extract a generic helper (e.g., items_from_js_options<T>)
that encapsulates the common scaffolding in inputs_from_js_options and
outputs_from_js_options: read the property via js_sys::Reflect::get, check
undefined/null, ensure it's an array, iterate/enumerate, call
item.to_wasm::<T>(type_name) and clone the inner value, and map errors with the
same "{}[{}] is not a <Type>" message; make the helper accept the JsValue
options, field_name: &str, type_name: &str and a generic T used in to_wasm (with
the same trait bounds required for cloning), then replace inputs_from_js_options
and outputs_from_js_options to call this helper and return its result (keep the
existing WasmDppError messages and signatures).
In
`@packages/wasm-dpp2/src/platform_address/transitions/address_funding_from_asset_lock_transition.rs`:
- Around line 153-204: The inputs getter/setter manually pattern-match on
AddressFundingFromAssetLockTransition::V0 while asset_lock_proof and outputs use
the AddressFundingFromAssetLockTransitionAccessorsV0 trait; change inputs() and
set_inputs() to call the same accessor methods on self.0 (e.g., use
AddressFundingFromAssetLockTransitionAccessorsV0::inputs and ::set_inputs or the
impl-provided inputs()/set_inputs() methods) and convert between
PlatformAddressInputWasm and the inner types as before so the code consistently
uses the transition accessor API (update the inputs() getter to iterate over
self.0.inputs() and the set_inputs() to call self.0.set_inputs(inputs_map)).
In
`@packages/wasm-dpp2/src/platform_address/transitions/identity_create_from_addresses_transition.rs`:
- Around line 163-179: The setter set_public_keys currently iterates over the
local Vec keys with keys.iter() and calls k.clone() for each element; replace
that clone pattern by consuming the vector with keys.into_iter() and mapping
each IdentityPublicKeyInCreationWasm directly into IdentityPublicKeyInCreation
(i.e., use IdentityPublicKeyInCreation::from(k) inside the map) when assigning
to v0.public_keys in the IdentityCreateFromAddressesTransition::V0 arm to avoid
unnecessary per-element cloning of IdentityPublicKeyInCreationWasm.
- Around line 94-105: The public_keys vector is cloned unnecessarily when
constructing IdentityCreateFromAddressesTransitionV0; update the construction in
IdentityCreateFromAddressesTransitionWasm /
IdentityCreateFromAddressesTransition::V0 /
IdentityCreateFromAddressesTransitionV0 to consume public_keys instead of
iterating and cloning it by replacing public_keys.iter().map(|k|
k.clone().into()) with an into-iterator that converts each element (e.g.,
public_keys.into_iter().map(Into::into)), since public_keys is not used after
this point.
In `@packages/wasm-dpp2/tests/unit/AddressCreditWithdrawalTransition.spec.ts`:
- Around line 78-97: Add a new describe block testing toHex()/fromHex()
round-trip for AddressCreditWithdrawalTransition: call createTransition(), get
hex via transition.toHex(), assert Buffer.from(hex, 'hex') equals
Buffer.from(transition.toBytes()), then restore via
wasm.AddressCreditWithdrawalTransition.fromHex(hex) and assert
Buffer.from(restored.toBytes()) equals the original bytes; place the test
alongside the existing toBytes/toBase64 blocks and use the same createTransition
helper and wasm.AddressCreditWithdrawalTransition methods.
In `@packages/wasm-dpp2/tests/unit/AddressFundingFromAssetLockTransition.spec.ts`:
- Around line 164-172: The test currently only checks the outputs array length;
after setting transition.outputs to [newOutput] also assert the amount on the
stored output matches the intended BigInt(50000). Locate the test using
createTransition(), new wasm.PlatformAddressOutput(newAddr, BigInt(50000)) and
transition.outputs, then add an assertion that the retrieved outputs[0] (or
outputs.at(0)) has an amount equal to BigInt(50000) to validate the setter
preserved the value.
In `@packages/wasm-dpp2/tests/unit/AddressFundsTransfer.spec.ts`:
- Around line 42-45: The test filename uses PascalCase
(AddressFundsTransfer.spec.ts) but should follow the repo's kebab-case rule;
rename the file to address-funds-transfer.spec.ts and update any imports or test
references that point to AddressFundsTransfer.spec.ts (and do the same for other
new test files in this PR) so test runner and imports resolve correctly; verify
references in test suites or any index files that mention
AddressFundsTransferTransition or the skipped
describe('AddressFundsTransferTransition (.build API)') remain valid after the
rename.
In `@packages/wasm-dpp2/tests/unit/IdentityCreateFromAddressesTransition.spec.ts`:
- Around line 81-100: Add missing hex round-trip tests for
IdentityCreateFromAddressesTransition: add a describe('toHex() / fromHex()')
block mirroring the existing toBase64/toBytes tests, create a transition via
createTransition(), call transition.toHex(), verify Buffer.from(hex, 'hex')
equals Buffer.from(transition.toBytes()), then restore with
wasm.IdentityCreateFromAddressesTransition.fromHex(hex) and assert
Buffer.from(restored.toBytes()) equals the original bytes; reference the
existing createTransition(), transition.toHex(),
wasm.IdentityCreateFromAddressesTransition.fromHex(), transition.toBytes(), and
restored.toBytes() methods when locating where to insert the new test.
In `@packages/wasm-dpp2/tests/unit/IdentityTopUpFromAddressesTransition.spec.ts`:
- Around line 76-95: Add a toHex()/fromHex() round-trip test for
IdentityTopUpFromAddressesTransition similar to other transition specs: create a
transition via createTransition(), call transition.toHex(), assert
Buffer.from(hex, 'hex') equals Buffer.from(transition.toBytes()), then restore
with wasm.IdentityTopUpFromAddressesTransition.fromHex(hex) and assert
Buffer.from(restored.toBytes()) equals the original bytes; place this in a new
describe('toHex() / fromHex()') / it('should round-trip via hex') block
alongside the existing toBytes/toBase64 tests.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/wasm-dpp2/src/lib.rspackages/wasm-dpp2/src/platform_address/address.rspackages/wasm-dpp2/src/platform_address/fee_strategy.rspackages/wasm-dpp2/src/platform_address/input_output.rspackages/wasm-dpp2/src/platform_address/mod.rspackages/wasm-dpp2/src/platform_address/transitions/address_credit_withdrawal_transition.rspackages/wasm-dpp2/src/platform_address/transitions/address_funding_from_asset_lock_transition.rspackages/wasm-dpp2/src/platform_address/transitions/address_funds_transfer_transition.rspackages/wasm-dpp2/src/platform_address/transitions/identity_create_from_addresses_transition.rspackages/wasm-dpp2/src/platform_address/transitions/identity_credit_transfer_to_addresses_transition.rspackages/wasm-dpp2/src/platform_address/transitions/identity_top_up_from_addresses_transition.rspackages/wasm-dpp2/src/platform_address/transitions/mod.rspackages/wasm-dpp2/tests/unit/AddressCreditWithdrawalTransition.spec.tspackages/wasm-dpp2/tests/unit/AddressFundingFromAssetLockTransition.spec.tspackages/wasm-dpp2/tests/unit/AddressFundsTransfer.spec.tspackages/wasm-dpp2/tests/unit/AddressFundsTransferTransition.spec.tspackages/wasm-dpp2/tests/unit/IdentityCreateFromAddressesTransition.spec.tspackages/wasm-dpp2/tests/unit/IdentityCreditTransferToAddresses.spec.tspackages/wasm-dpp2/tests/unit/IdentityTopUpFromAddressesTransition.spec.ts
Comment on lines
+179
to
+206
| #[wasm_bindgen(getter = "output")] | ||
| pub fn output(&self) -> Option<PlatformAddressOutputWasm> { | ||
| let output = match &self.0 { | ||
| AddressCreditWithdrawalTransition::V0(v0) => &v0.output, | ||
| }; | ||
| output.map(|(address, credits)| PlatformAddressOutputWasm::new(address, credits)) | ||
| } | ||
|
|
||
| #[wasm_bindgen(setter = "output")] | ||
| pub fn set_output(&mut self, output: JsValue) -> WasmDppResult<()> { | ||
| let new_output = if output.is_undefined() || output.is_null() { | ||
| None | ||
| } else { | ||
| let output_wasm = output | ||
| .to_wasm::<PlatformAddressOutputWasm>("PlatformAddressOutput") | ||
| .map(|r| (*r).clone()) | ||
| .map_err(|_| { | ||
| WasmDppError::invalid_argument("'output' is not a PlatformAddressOutput") | ||
| })?; | ||
| Some(output_wasm.into_inner()) | ||
| }; | ||
| match &mut self.0 { | ||
| AddressCreditWithdrawalTransition::V0(v0) => { | ||
| v0.output = new_output; | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
into_inner() will panic in WASM if the output has no amount.
Line 198 calls output_wasm.into_inner() which uses .expect(...) internally. If a user constructs a PlatformAddressOutput without an amount and passes it to the output setter, this will panic rather than returning a graceful error to JavaScript.
Consider using a fallible conversion (e.g., a try_into_inner that returns WasmDppResult) or validating the amount before calling into_inner.
This same risk applies to outputs_to_btree_map used in AddressFundsTransferTransition and other transitions.
Proposed fix in input_output.rs (root cause)
- pub fn into_inner(self) -> (PlatformAddress, Credits) {
- (
- self.address.into(),
- self.amount.expect("amount is required for this operation"),
- )
- }
+ pub fn into_inner(self) -> (PlatformAddress, Credits) {
+ (
+ self.address.into(),
+ self.amount.expect("amount is required for this operation"),
+ )
+ }
+
+ pub fn try_into_inner(self) -> WasmDppResult<(PlatformAddress, Credits)> {
+ let amount = self.amount.ok_or_else(|| {
+ WasmDppError::invalid_argument("amount is required for this output")
+ })?;
+ Ok((self.address.into(), amount))
+ }Then callers like outputs_to_btree_map and the output setter can use try_into_inner() to return errors instead of panicking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/wasm-dpp2/src/platform_address/transitions/address_credit_withdrawal_transition.rs`
around lines 179 - 206, The setter AddressCreditWithdrawalTransition::set_output
currently calls PlatformAddressOutputWasm::into_inner(), which can panic if the
PlatformAddressOutput has no amount; replace this with a fallible conversion
(e.g., add and call a try_into_inner() on PlatformAddressOutputWasm that returns
WasmDppResult<PlatformAddressOutput>) or explicitly validate the output_wasm has
an amount before converting and return a WasmDppError (invalid_argument) instead
of panicking. Update callers that use into_inner() (including
outputs_to_btree_map and implementations in AddressFundsTransferTransition and
other transition setters) to use the new try_into_inner() or the
validation+error pattern so all setters return graceful WasmDppResult errors
instead of causing a WASM panic.
shumkov
changed the title
Fix module name typo (transsition -> transition) and apply cargo fmt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/wasm-dpp2/src/platform_address/input_output.rs`:
- Around line 231-265: outputs_from_js_options currently accepts
PlatformAddressOutputWasm entries with amount == None which later causes panics
in outputs_to_btree_map()/into_inner(); add a fallible validation step that
returns a WasmDppError when any PlatformAddressOutputWasm has no amount instead
of allowing None to propagate. Implement this by either adding a new function
(e.g., outputs_from_js_options_required) or a boolean parameter to
outputs_from_js_options that, when enabled, iterates the parsed
Vec<PlatformAddressOutputWasm> and returns
Err(WasmDppError::invalid_argument(...)) for items where output.amount is None
(include index and field_name in the message), and then use this fallible path
in required-output flows (funds transfer / credit transfer / identity create
output setter) in place of the current callers that lead to
outputs_to_btree_map()/into_inner() panics.
In
`@packages/wasm-dpp2/src/platform_address/transitions/identity_create_from_addresses_transition.rs`:
- Around line 207-214: The set_output function currently calls
output_wasm.into_inner() which can panic when required fields like amount are
missing; change this to a safe conversion that returns a
WasmDppError::invalid_argument on missing/invalid fields: instead of calling
into_inner() directly, validate or attempt a fallible conversion from
PlatformAddressOutputWasm to the internal PlatformAddressOutput (e.g.
implement/use a try_from/try_into or an into_inner_result method on
PlatformAddressOutputWasm) and map any missing amount or deserialization error
to WasmDppError::invalid_argument; update set_output to use that Result and
return the mapped WasmDppError rather than allowing a panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e13b3276-55c7-460a-892e-fab469514b16
📒 Files selected for processing (8)
packages/wasm-dpp2/src/lib.rspackages/wasm-dpp2/src/platform_address/input_output.rspackages/wasm-dpp2/src/platform_address/transitions/address_funding_from_asset_lock_transition.rspackages/wasm-dpp2/src/platform_address/transitions/address_funds_transfer_transition.rspackages/wasm-dpp2/src/platform_address/transitions/identity_create_from_addresses_transition.rspackages/wasm-dpp2/src/platform_address/transitions/identity_credit_transfer_to_addresses_transition.rspackages/wasm-dpp2/src/platform_address/transitions/identity_top_up_from_addresses_transition.rspackages/wasm-dpp2/src/platform_address/transitions/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/wasm-dpp2/src/platform_address/transitions/mod.rs
- packages/wasm-dpp2/src/platform_address/transitions/identity_top_up_from_addresses_transition.rs
Comment on lines
+231
to
+265
| pub fn outputs_from_js_options( | ||
| options: &JsValue, | ||
| field_name: &str, | ||
| ) -> WasmDppResult<Vec<PlatformAddressOutputWasm>> { | ||
| let value = js_sys::Reflect::get(options, &JsValue::from_str(field_name)).map_err(|_| { | ||
| WasmDppError::invalid_argument(format!("failed to read '{}' from options", field_name)) | ||
| })?; | ||
| if value.is_undefined() || value.is_null() { | ||
| return Err(WasmDppError::invalid_argument(format!( | ||
| "'{}' is required", | ||
| field_name | ||
| ))); | ||
| } | ||
| if !js_sys::Array::is_array(&value) { | ||
| return Err(WasmDppError::invalid_argument(format!( | ||
| "'{}' must be an array", | ||
| field_name | ||
| ))); | ||
| } | ||
| let array = js_sys::Array::from(&value); | ||
| array | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, item)| { | ||
| item.to_wasm::<PlatformAddressOutputWasm>("PlatformAddressOutput") | ||
| .map(|r| (*r).clone()) | ||
| .map_err(|_| { | ||
| WasmDppError::invalid_argument(format!( | ||
| "{}[{}] is not a PlatformAddressOutput", | ||
| field_name, i | ||
| )) | ||
| }) | ||
| }) | ||
| .collect() | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid panic paths when required outputs are missing amount.
This helper accepts PlatformAddressOutput entries with amount = None. In required-output transitions, those values later flow into outputs_to_btree_map()/into_inner() and panic instead of returning WasmDppError.
💡 Suggested fix: add a fallible required-output converter
+pub fn outputs_to_btree_map_required(
+ outputs: Vec<PlatformAddressOutputWasm>,
+ field_name: &str,
+) -> WasmDppResult<BTreeMap<PlatformAddress, Credits>> {
+ outputs
+ .into_iter()
+ .enumerate()
+ .map(|(i, output)| {
+ let (address, amount_opt) = output.into_inner_optional();
+ let amount = amount_opt.ok_or_else(|| {
+ WasmDppError::invalid_argument(format!(
+ "{}[{}].amount is required",
+ field_name, i
+ ))
+ })?;
+ Ok((address, amount))
+ })
+ .collect()
+}Then use this fallible path in required-output transitions (funds transfer / credit transfer / identity create output setter).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/wasm-dpp2/src/platform_address/input_output.rs` around lines 231 -
265, outputs_from_js_options currently accepts PlatformAddressOutputWasm entries
with amount == None which later causes panics in
outputs_to_btree_map()/into_inner(); add a fallible validation step that returns
a WasmDppError when any PlatformAddressOutputWasm has no amount instead of
allowing None to propagate. Implement this by either adding a new function
(e.g., outputs_from_js_options_required) or a boolean parameter to
outputs_from_js_options that, when enabled, iterates the parsed
Vec<PlatformAddressOutputWasm> and returns
Err(WasmDppError::invalid_argument(...)) for items where output.amount is None
(include index and field_name in the message), and then use this fallible path
in required-output flows (funds transfer / credit transfer / identity create
output setter) in place of the current callers that lead to
outputs_to_btree_map()/into_inner() panics.
...ages/wasm-dpp2/src/platform_address/transitions/identity_create_from_addresses_transition.rs
Outdated
Show resolved
Hide resolved
…ss setters Replace JsValue with Option<PlatformAddressOutputWasm> for set_output and CoreScriptWasm for set_output_script to generate proper TypeScript types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…oss wasm-dpp2 Use js_sys::BigInt for u64 setters (balance, revision, nonce, amount, price, supply) and js_sys::Number for u32/u16 setters (version, userFeeIncrease, tokenContractPosition) to generate proper TypeScript types (bigint/number) instead of any. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/wasm-dpp2/src/platform_address/transitions/address_credit_withdrawal_transition.rs (1)
100-101:⚠️ Potential issue | 🔴 CriticalPrevent WASM panic by using fallible output conversion.
Line 100 and Line 188 call
into_inner(), which can panic whenoutput.amountis missing. This should return aWasmDppError::invalid_argumentinstead of trapping at runtime.Suggested fix
- let output = output.map(|o| o.into_inner()); + let output = output + .map(|o| o.try_into_inner()) + .transpose()?; @@ - pub fn set_output(&mut self, output: Option<PlatformAddressOutputWasm>) { - let new_output = output.map(|o| o.into_inner()); + pub fn set_output(&mut self, output: Option<PlatformAddressOutputWasm>) -> WasmDppResult<()> { + let new_output = output + .map(|o| o.try_into_inner()) + .transpose()?; match &mut self.0 { AddressCreditWithdrawalTransition::V0(v0) => { v0.output = new_output; } } + Ok(()) }Also applies to: 187-189
🤖 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/wasm-dpp2/src/platform_address/transitions/identity_top_up_from_addresses_transition.rs`:
- Around line 210-217: Change the setter signature for set_user_fee_increase to
accept a primitive numeric type instead of &js_sys::Number (e.g., amount: u32)
and perform explicit bounds checking before assigning to the inner field; locate
IdentityTopUpFromAddressesTransition::V0 handling in set_user_fee_increase,
replace the try_to_u16(amount, "userFeeIncrease") call by validating that amount
<= u16::MAX (or use u16::try_from(amount) with map_err) and then set
v0.user_fee_increase = amount_as_u16, so JavaScript callers can pass plain
number primitives and the value is safely converted/validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64e2a8c9-2704-4856-abe4-5272c8c852e5
📒 Files selected for processing (3)
packages/wasm-dpp2/src/platform_address/transitions/address_credit_withdrawal_transition.rspackages/wasm-dpp2/src/platform_address/transitions/identity_create_from_addresses_transition.rspackages/wasm-dpp2/src/platform_address/transitions/identity_top_up_from_addresses_transition.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wasm-dpp2/src/platform_address/transitions/identity_create_from_addresses_transition.rs
...ages/wasm-dpp2/src/platform_address/transitions/identity_top_up_from_addresses_transition.rs
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
JavaScript/TypeScript applications need WASM bindings for the 6 platform address-based state transitions to create, serialize, and
interact with them from the browser and Node.js.
What was done?
New transition bindings (
platform_address/transitions/)Added WASM bindings for all 6 address-based state transitions:
AddressFundsTransferTransition— transfer credits between platform addressesAddressCreditWithdrawalTransition— withdraw credits to Core via output scriptAddressFundingFromAssetLockTransition— fund platform addresses via asset lock proofsIdentityCreateFromAddressesTransition— create identity from platform address inputsIdentityTopUpFromAddressesTransition— top up identity from platform addressesIdentityCreditTransferToAddressesTransition— transfer credits from identity to platform addressesEach binding exposes:
toBytes/fromBytes,toBase64/fromBase64,toHex/fromHextoObject/fromObject,toJSON/fromJSON(viaimpl_wasm_conversions!)toStateTransition/fromStateTransitionfor enum wrappingSupporting changes
input_output.rs— addedPlatformAddressOutput::new/new_optional, helper functionsinputs_from_js_options,optional_output_from_js_options,fee_strategy_from_js_options,outputs_from_js_optionsfee_strategy.rs— addedFeeStrategyStepWasmwithdeductFromInput/reduceOutputconstructors andto_wasmconversionaddress.rs— addedPlatformAddressWasm::newconstructor,impl_wasm_type_info!lib.rs— exported new transition types andFeeStrategyStepWasmHow Has This Been Tested?
yarn workspace @dashevo/wasm-dpp2 test— 838 passing, 0 failing.Added 7 test files:
AddressFundsTransferTransition.spec.ts— constructor, bytes/base64/hex round-trips, inputs/outputs getters/setters,userFeeIncrease, StateTransition conversion
AddressCreditWithdrawalTransition.spec.ts— constructor (with/without output),inputs/output/outputScript/pooling/coreFeePerByte getters/setters, StateTransition conversion
AddressFundingFromAssetLockTransition.spec.ts— constructor with asset lock proof, inputs/outputs/assetLockProofgetters/setters, StateTransition conversion
IdentityCreateFromAddressesTransition.spec.ts— constructor with public keys, inputs getter/setter, StateTransition conversionIdentityTopUpFromAddressesTransition.spec.ts— constructor with identityId (string and Identifier), inputs/output/identityIdgetters/setters, StateTransition conversion
IdentityCreditTransferToAddresses.spec.ts— constructor, recipientAddresses/senderId/nonce/signature/signaturePublicKeyIdgetters/setters, StateTransition conversion
AddressFundsTransfer.spec.ts—FeeStrategyStepWasmdeductFromInput/reduceOutput testsNote:
toObject/fromObject/toJSON/fromJSONconversion tests deferred to a separate PR to address a pre-existingis_human_readablemismatch in theimpl_wasm_conversions!macro.Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Enhancements
Tests