refactor(sdk): comprehansive Evo SDK refactroring #2999
Conversation
📝 WalkthroughWalkthroughLarge wasm-bindgen and TypeScript interop overhaul: added many "LikeJs" extern types and TS interfaces, replaced JsValue params with typed LikeJs inputs and options-object constructors, renamed getters/setters to JS-friendly properties, added conversion/type-info macros, and updated wasm-sdk query signatures and tests to use initWasm/wasm. Changes
Sequence Diagram(s)sequenceDiagram
participant JSApp as JS/TS App
participant Init as initWasm()
participant Wasm as wasm module
participant Conv as conversion helpers (utils)
participant RustCore as Rust domain types
JSApp->>Init: call initWasm()
Init->>Wasm: instantiate & export `wasm` namespace
JSApp->>Wasm: call constructor(optionsObj or LikeJs)
Wasm->>Conv: try_from_options / try_into conversions
Conv->>RustCore: produce Rust types (Identifier, PlatformVersion, etc.)
RustCore-->>Wasm: construct inner V0 types
Wasm-->>JSApp: return Wasm wrapper (typed extern object/JSON)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
thephez
changed the title
shumkov
changed the title
|
@CodeRabbit review |
✅ Actions performedReview 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/wasm-dpp2/src/state_transitions/batch/token_payment_info.rs (1)
153-156: Fix copy-paste bug: calling wrong setter method.The
set_minimum_token_costfunction incorrectly callsset_maximum_token_coston the inner type, which would cause the minimum cost value to be set as the maximum cost instead.🐛 Proposed fix
#[wasm_bindgen(setter = "minimumTokenCost")] pub fn set_minimum_token_cost(&mut self, minimum_cost: Option<TokenAmount>) { - self.0.set_maximum_token_cost(minimum_cost); + self.0.set_minimum_token_cost(minimum_cost); }packages/wasm-dpp2/src/enums/batch/gas_fees_paid_by.rs (1)
52-55: Incorrect error message references "batch type" instead of "gas fees".The error messages say
"unknown batch type value"but this enum isGasFeesPaidBy. This appears to be a copy-paste error.Fix error message
- _ => Err(WasmDppError::invalid_argument(format!( - "unknown batch type value: {}", - enum_val - ))), + _ => Err(WasmDppError::invalid_argument(format!( + "unknown gas fees paid by value: {}", + enum_val + ))),Apply the same fix at line 64-67.
Also applies to: 64-67
packages/wasm-dpp2/src/tokens/configuration/distribution_recipient.rs (1)
45-55: recipientType should stay a stable variant name.
Returning"Identity(<id>)"couples type and value, which breaks checks likerecipientType === "Identity". The identifier is already exposed viavalue, so this should return just"Identity".Proposed fix
- TokenDistributionRecipient::Identity(identity) => { - format!("Identity({})", IdentifierWasm::from(identity).to_base58()) - } + TokenDistributionRecipient::Identity(_) => String::from("Identity"),packages/wasm-dpp2/src/state_transitions/base/state_transition.rs (1)
101-112: Changekey_typeparameter to useOption<KeyTypeWasm>to properly reflect optional behavior.The method currently accepts
key_type: JsValueand checksis_undefined()for the default, but this pattern doesn't ensure the generated TypeScript signature treatskeyTypeas optional. UseOption<KeyTypeWasm>instead, matching the pattern already established inverify_public_key(line 131-132) for optional parameters, then convert using.map_or()or similar to preserve theECDSA_SECP256K1default.packages/wasm-dpp2/src/data_contract/transitions/update.rs (1)
166-190: Inconsistent parameter ordering withcreate.rs.The parameter order here is
(full_validation, platform_version), but increate.rs(line 182-183) it's(platform_version, full_validation). This inconsistency could confuse API consumers.Suggested fix to align parameter ordering
#[wasm_bindgen(js_name = "getDataContract")] pub fn get_data_contract( &self, - full_validation: Option<bool>, platform_version: PlatformVersionLikeJs, + full_validation: Option<bool>, ) -> WasmDppResult<DataContractWasm> {packages/wasm-dpp2/src/asset_lock_proof/outpoint.rs (1)
106-128: Guard fromBytes against oversized input.Line 111 slices with
lenfrom user input;len > 36will panic in WASM. Prefer explicit validation and return a structured error.🐛 Suggested fix (validate length and return error)
#[wasm_bindgen(js_name = "fromBytes")] -pub fn from_bytes(buffer: Vec<u8>) -> OutPointWasm { +pub fn from_bytes(buffer: Vec<u8>) -> WasmDppResult<OutPointWasm> { + if buffer.len() != 36 { + return Err(WasmDppError::invalid_argument(format!( + "OutPoint bytes must be 36 bytes, got {}", + buffer.len() + ))); + } let mut out_buffer = [0u8; 36]; - let bytes = buffer.as_slice(); - let len = bytes.len(); - out_buffer[..len].copy_from_slice(bytes); - - OutPointWasm(OutPoint::from(out_buffer)) + out_buffer.copy_from_slice(&buffer); + + Ok(OutPointWasm(OutPoint::from(out_buffer))) } @@ - Ok(OutPointWasm::from_bytes(bytes)) + OutPointWasm::from_bytes(bytes) @@ - Ok(OutPointWasm::from_bytes(bytes)) + OutPointWasm::from_bytes(bytes)packages/wasm-dpp2/src/identity/partial_identity.rs (1)
279-323: Accept numeric string keys inObject::keysiteration
Object::keysreturns strings regardless of key type, sokey.as_f64()will always fail. Parse string keys (or numeric values if somehow present) intou32.🐛 Suggested fix (accept numeric string keys)
- let key_val = key.as_f64().ok_or_else(|| { - WasmDppError::invalid_argument("Key identifier must be numeric") - })?; - - if key_val > u32::MAX as f64 { - return Err(WasmDppError::invalid_argument(format!( - "Key id '{:?}' exceeds the maximum limit for u32.", - key.as_string() - ))); - } - - let key_id = KeyID::from(key_val as u32); + let key_id_u32: u32 = if let Some(n) = key.as_f64() { + if n.fract() != 0.0 || n < 0.0 || n > u32::MAX as f64 { + return Err(WasmDppError::invalid_argument( + "Key identifier must be a valid u32", + )); + } + n as u32 + } else if let Some(s) = key.as_string() { + s.parse().map_err(|_| { + WasmDppError::invalid_argument("Key identifier must be numeric") + })? + } else { + return Err(WasmDppError::invalid_argument("Key identifier must be numeric")); + }; + + let key_id = KeyID::from(key_id_u32); @@ - key_val as u32, message + key_id_u32, message
🤖 Fix all issues with AI agents
In `@packages/wasm-dpp2/src/core/pro_tx_hash.rs`:
- Around line 34-67: The TryFrom implementation for ProTxHashLikeJs currently
only routes to ProTxHashWasm::try_from(JsValue) and therefore rejects an input
that is already a ProTxHash JS object; add a fast-path that detects a JS object
convertible via the IntoWasm trait (as done in IdentifierWasm) and convert it
directly to ProTxHashWasm before falling back to the hex/Uint8Array parsing;
update TryFrom<ProTxHashLikeJs> for ProTxHashWasm to call IntoWasm::into_wasm
when the JsValue is an object representing a ProTxHash, and mirror the same
pattern in TryFrom<ProTxHashLikeNullableJs> path so ProTxHash JS objects and
nullable/empty-string cases are handled correctly.
In `@packages/wasm-dpp2/src/data_contract/document/model.rs`:
- Around line 173-210: The constructor currently converts revision_js via
as_f64() and casts to u64 which can silently accept fractional/invalid values;
replace that logic to call crate::utils::try_to_u64(revision_js) and wrap the
result with Revision::from(...) (mirror the pattern used in
partial_identity.rs). Ensure you reference the local variable revision_js,
remove the direct as_f64() cast, and propagate any error from try_to_u64 so
invalid/NaN/negative/fractional values are rejected.
In `@packages/wasm-dpp2/src/enums/keys/purpose.rs`:
- Around line 6-11: The extern type PurposeLikeJs references a missing
TypeScript type alias "PurposeLike"; add a TS_TYPES block that defines
PurposeLike (e.g., a union of allowed values or string | number as used in other
key enum files) so the generated bindings include the TypeScript declaration;
update the same file's wasm-bindgen TS_TYPES section to declare export type
PurposeLike = /* match other enums' pattern, e.g. */ "AUTH" | "ENCRYPT" |
string; ensuring the identifier PurposeLike matches the typescript_type used on
the extern type PurposeLikeJs.
In `@packages/wasm-dpp2/src/enums/keys/security_level.rs`:
- Around line 6-11: The extern type SecurityLevelLikeJs declares typescript_type
= "SecurityLevelLike" but no TypeScript definition is emitted; add a matching
typescript_custom_section for "SecurityLevelLike" (similar to
gas_fees_paid_by.rs and key_type.rs) so consumers get the TypeScript
alias/interface. Locate the extern type SecurityLevelLikeJs and add a
#[wasm_bindgen(typescript_custom_section = r#"/* TS_TYPES */ ..."#)] attribute
that defines the SecurityLevelLike type (matching expected shape/union used
elsewhere) to ensure the generated d.ts includes the declaration.
In `@packages/wasm-dpp2/src/epoch/extended_epoch_info.rs`:
- Around line 90-94: The use of Reflect::get in the ExtendedEpochInfo
constructor (e.g., the options_obj -> Reflect::get(...)->as_f64() chain that
produces index as u16) is incorrect to treat Reflect::get errors as "missing
key" because Reflect::get returns Ok(JsValue::UNDEFINED) for absent properties;
update each property extraction (e.g., the index flow) to explicitly check for
JsValue::UNDEFINED or call .is_undefined() on the result and return
WasmDppError::invalid_argument with a clear "index is required" message when
undefined, then convert the defined value to number (using as_f64() and map to
invalid_argument "index must be a number" if None); apply the same pattern for
all other Reflect::get uses in the constructor and keep error creation via
WasmDppError::invalid_argument for consistent messages.
In `@packages/wasm-dpp2/src/state_transitions/batch/token_base_transition.rs`:
- Around line 61-66: The current extraction of tokenContractPosition casts a f64
to u16 with `as u16`, which can silently truncate; replicate the safe pattern
used for `identityContractNonce` by retrieving the value from `options_obj`, get
it as f64, convert it to an integer using the existing `try_to_u64` helper (or
the same conversion function used for `identityContractNonce`), validate range,
then explicitly cast to u16 only after successful conversion to prevent
truncation or out-of-range values when setting `token_contract_position`.
- Around line 76-87: The code currently uses
Reflect::get(...).unwrap_or(JsValue::UNDEFINED), which hides property access
errors; change it to propagate Reflect::get errors instead of swallowing them:
call Reflect::get(&options_obj, &"usingGroupInfo".into()) and propagate any Err
via the function's Result (using ? or mapping the JsValue error into the
function error), then use the returned JsValue (check is_undefined to
distinguish absence) before converting with to_wasm on
GroupStateTransitionInfoWasm and converting into GroupStateTransitionInfo;
ensure you reference the using_group_info_js variable, Reflect::get, to_wasm,
GroupStateTransitionInfoWasm, and GroupStateTransitionInfo when making the
change.
In `@packages/wasm-dpp2/src/version.rs`:
- Around line 16-41: The numeric path for PlatformVersionLike is accepting f64s
but PlatformVersionWasm::try_from currently casts f64 to u8 directly; update
PlatformVersionWasm::try_from to validate numeric inputs like NetworkWasm does:
if the incoming JsValue is a number, ensure it is an integer (no fractional
part) and within the valid u8 range (0..=255) before casting, and return a
WasmDppError for non-integer or out-of-range values; locate the conversion logic
in PlatformVersionWasm::try_from (and the TryFrom<PlatformVersionLikeJs> impl
that delegates to it) and mirror the integer validation pattern used by
NetworkWasm.
🧹 Nitpick comments (29)
packages/wasm-dpp2/JSVALUE_TYPE_AUDIT.md (1)
8-8: Clarify category numbering for readability.
Starting at “Category 2” without a preceding “Category 1” is a bit confusing in a standalone doc; consider renaming to a neutral header (e.g., “Return Types Using JsValue”) or adding Category 1 for completeness.packages/wasm-dpp2/src/state_transitions/batch/token_transitions/token_emergency_action.rs (2)
48-51: Unnecessary clone of entire struct.
self.clone()clones the whole wrapper unnecessarily. The method only needs the inner value'spublic_note_owned().♻️ Suggested fix
#[wasm_bindgen(getter = "publicNote")] pub fn get_public_note(&self) -> Option<String> { - self.clone().0.public_note_owned() + self.0.public_note().cloned() }If
public_note_owned()requires ownership, consider usingself.0.public_note().cloned()or checking if there's a borrowing accessor available.
53-56: Asymmetric getter/setter types foremergencyAction.The getter returns
Stringwhile the setter acceptsTokenEmergencyActionWasm. This creates an inconsistent API. Compare withbasewhich usesTokenBaseTransitionWasmfor both getter and setter.Consider returning
TokenEmergencyActionWasmfrom the getter for consistency:♻️ Suggested fix
#[wasm_bindgen(getter = "emergencyAction")] - pub fn get_emergency_action(&self) -> String { - TokenEmergencyActionWasm::from(self.0.emergency_action()).into() + pub fn get_emergency_action(&self) -> TokenEmergencyActionWasm { + self.0.emergency_action().into() }packages/wasm-dpp2/src/state_transitions/batch/token_transitions/token_claim.rs (1)
75-90: Consider extracting duplicated conversion logic.Lines 83-86 duplicate the same conversion pattern from the constructor (lines 36-39). If this pattern appears elsewhere in the codebase, a small helper function could reduce repetition:
fn parse_distribution_type(value: &JsValue) -> WasmDppResult<TokenDistributionTypeWasm> { match value.is_undefined() { true => Ok(TokenDistributionTypeWasm::default()), false => TokenDistributionTypeWasm::try_from(value.clone()), } }This is a minor nit given it's only two occurrences in this file.
packages/wasm-dpp2/src/state_transitions/batch/prefunded_voting_balance.rs (1)
29-35: Unconventional Rust function nameconstructor.While the
#[wasm_bindgen(constructor)]attribute correctly enables JSnew ClassName()syntax, the Rust function nameconstructoris non-idiomatic. The standard Rust convention is to name this functionnew, which would still work correctly with the wasm_bindgen attribute.That said, if this is a deliberate PR-wide convention for consistency with the JS/TS API surface, it's acceptable.
packages/wasm-sdk/src/wallet/key_generation.rs (1)
118-133: Pre-allocate the vector to avoid small reallocations.
This is minor, but cheap and clean sincecountis bounded.♻️ Suggested tweak
- let mut pairs = Vec::new(); + let mut pairs = Vec::with_capacity(count as usize);packages/wasm-dpp2/src/tokens/configuration/pre_programmed_distribution.rs (1)
131-132: Consider validating that inner value is a Map.
Map::from(value)will attempt to create a Map from any JsValue. If the caller passes a malformed structure where the inner value is not a Map (e.g., an object or array), this won't fail immediately but will produce incorrect/empty results, making debugging difficult.♻️ Proposed validation
- let inner_map = Map::from(value); + let inner_map: Map = value.dyn_into().map_err(|_| { + WasmDppError::invalid_argument(format!( + "Expected Map for distribution amounts at timestamp '{}', got: {:?}", + timestamp_str, + value.js_typeof() + )) + })?;This requires adding
dyn_intofromJsCast(already imported).packages/wasm-dpp2/src/identity/transitions/masternode_vote_transition.rs (1)
116-132: Consider adding null checks for optional fields.The code checks for
undefinedbut notnullfor optional fields. While the TypeScript types don't allownull, JavaScript callers could still pass it:
- For
signaturePublicKeyId: passingnullwould trigger "must be a number" error (misleading but safe)- For
signature:Uint8Array::from(null)may produce unexpected resultsThis follows the existing codebase pattern, so it's a minor edge case, but adding explicit null checks would improve robustness.
♻️ Suggested improvement
- let signature_public_key_id: KeyID = if signature_public_key_js.is_undefined() { + let signature_public_key_id: KeyID = if signature_public_key_js.is_undefined() || signature_public_key_js.is_null() { 0 } else { ... }; - let signature: Vec<u8> = if signature_js.is_undefined() { + let signature: Vec<u8> = if signature_js.is_undefined() || signature_js.is_null() { Vec::new() } else { Uint8Array::from(signature_js).to_vec() };packages/wasm-dpp2/src/tokens/configuration/reward_distribution_type.rs (1)
60-82: Redundant clone onfunctionin each match arm.The
functionis already owned after destructuring fromself.0.clone(), so the additional.clone()on lines 66, 72, and 78 is unnecessary.♻️ Suggested refactor to remove redundant clones
#[wasm_bindgen(getter = "distribution")] pub fn distribution(&self) -> JsValue { match self.0.clone() { RewardDistributionType::BlockBasedDistribution { interval, function } => { JsValue::from(BlockBasedDistributionWasm { interval, - function: function.clone().into(), + function: function.into(), }) } RewardDistributionType::TimeBasedDistribution { interval, function } => { JsValue::from(TimeBasedDistributionWasm { interval, - function: function.clone().into(), + function: function.into(), }) } RewardDistributionType::EpochBasedDistribution { interval, function } => { JsValue::from(EpochBasedDistributionWasm { interval, - function: function.clone().into(), + function: function.into(), }) } } }packages/wasm-dpp2/src/epoch/finalized_epoch_info.rs (1)
182-243: Consider adding field context totry_to_u64error messages.When
try_to_u64fails (e.g., forfirstBlockTime), the error message fromtry_to_u64won't include the field name, making it harder for SDK users to identify which field caused the issue.Example improvement for one field:
let first_block_time = try_to_u64( Reflect::get(&options_obj, &"firstBlockTime".into()) .map_err(|_| WasmDppError::invalid_argument("firstBlockTime is required"))?, -)? ; +).map_err(|e| WasmDppError::invalid_argument(format!("firstBlockTime: {}", e)))?;packages/wasm-dpp2/src/state_transitions/batch/token_transition.rs (2)
148-185: Consider avoiding unnecessary clone in getter.The
transitiongetter callsself.clone().0which clones the entireTokenTransitionWasmstruct. If theFromimplementations for the specific transition Wasm types can work with references or if the innerTokenTransitioncan be cloned directly, this could be optimized.♻️ Potential optimization
#[wasm_bindgen(getter = "transition")] pub fn transition(&self) -> JsValue { - match self.clone().0 { + match &self.0 { TokenTransition::Burn(token_transition) => { - TokenBurnTransitionWasm::from(token_transition).into() + TokenBurnTransitionWasm::from(token_transition.clone()).into() } // ... similar for other variants } }This avoids cloning the wrapper struct and only clones the specific inner transition that's actually needed.
187-202: Consider deriving transition type numbers to prevent protocol contract misalignment.The numeric values 0-10 are hardcoded in the match statement without any compile-time enforcement that they stay synchronized with the
TokenTransitionenum variants. Since this is exposed as a WASM binding (JavaScript API), these numbers form part of the protocol contract. If enum variants are reordered or added, the numbers here won't automatically update, risking silent breakage.Consider using explicit enum discriminants (
#[repr(u8)]) or a constant mapping to ensure type numbers remain synchronized with enum changes.packages/wasm-dpp2/src/tokens/configuration/configuration_convention.rs (1)
82-121: Validatelocalizationsis a plain object before coercion.
Object::fromwill coerce primitives/arrays into objects, which can yield confusing errors. Consider rejecting non-object inputs up front to keep error messages clear and prevent accidental acceptance of invalid shapes.♻️ Suggested guard (plus import)
-use js_sys::{Object, Reflect}; +use js_sys::{Array, Object, Reflect};fn value_to_localizations( localizations_value: &JsValue, ) -> WasmDppResult<BTreeMap<String, TokenConfigurationLocalization>> { + if localizations_value.is_null() || localizations_value.is_undefined() { + return Err(WasmDppError::invalid_argument( + "localizations must be an object", + )); + } + if !localizations_value.is_object() || Array::is_array(localizations_value) { + return Err(WasmDppError::invalid_argument( + "localizations must be a Record<string, TokenConfigurationLocalization>", + )); + } let js_object = Object::from(localizations_value.clone()); let mut localizations = BTreeMap::new();packages/wasm-dpp2/src/state_transitions/batch/token_pricing_schedule.rs (1)
81-86: Consider using bigint for SetPrices values for consistency.
SinglePricereturns values asbigint(line 76), butSetPricesserializes values using.into()which produces a JavaScriptnumber. For large credit values that exceed JavaScript's safe integer range, this could cause precision loss.♻️ Suggested fix for consistency
for (key, value) in prices.iter() { Reflect::set( &price_object, &JsValue::from(key.to_string()), - &(*value).into(), + &JsValue::bigint_from_str(&value.to_string()), )packages/wasm-dpp2/src/epoch/extended_epoch_info.rs (1)
90-94: Silent truncation on numeric casts may lose precision.The
as u16,as u32casts will silently truncate values exceeding their ranges:
- Line 94:
indexcast tou16(max 65,535)- Line 111:
first_core_block_heightcast tou32(max ~4.2 billion)- Line 122:
protocol_versioncast tou32If a JS caller passes a value like
100000forindex, it will be silently truncated.Consider adding range validation, especially for
index:let index_f64 = index_js.as_f64() .ok_or_else(|| WasmDppError::invalid_argument("index must be a number"))?; if index_f64 < 0.0 || index_f64 > u16::MAX as f64 { return Err(WasmDppError::invalid_argument( format!("index must be between 0 and {}", u16::MAX) )); } let index = index_f64 as u16;Also applies to: 106-111, 118-122
packages/wasm-dpp2/src/state_transitions/batch/batched_transition.rs (1)
100-103: Consider simplifying the identifier conversion path.The current conversion goes through an intermediate
JsValue:let id_value: JsValue = contract_id.into(); let contract_id: Identifier = IdentifierWasm::try_from(&id_value)?.into();Based on the
TryFrom<IdentifierLikeJs> for Identifierimplementation inidentifier.rs(lines 46-51), this could be simplified to:let contract_id: Identifier = contract_id.try_into()?;♻️ Suggested simplification
#[wasm_bindgen(setter = "dataContractId")] pub fn set_data_contract_id(&mut self, contract_id: IdentifierLikeJs) -> WasmDppResult<()> { - let id_value: JsValue = contract_id.into(); - let contract_id: Identifier = IdentifierWasm::try_from(&id_value)?.into(); + let contract_id: Identifier = contract_id.try_into()?;packages/wasm-dpp2/src/voting/vote_poll.rs (1)
82-112: Consider validating that required fields are present, not just that Reflect::get succeeds.
Reflect::gettypically doesn't throw an error when a property doesn't exist—it returnsOk(JsValue::UNDEFINED). The current error messages suggest the fields are "missing" when Reflect fails, but Reflect failure usually indicates a different issue (e.g., the target is not an object).For robustness, consider explicitly checking if the retrieved values are undefined:
♻️ Suggested validation improvement
// Extract contractId (required) let js_contract_id = Reflect::get(&object, &JsValue::from_str("contractId")) .map_err(|e| WasmDppError::invalid_argument(format!("Missing contractId: {:?}", e)))?; + if js_contract_id.is_undefined() { + return Err(WasmDppError::invalid_argument("contractId is required")); + } let contract_id = IdentifierWasm::try_from(&js_contract_id)?.into(); // Extract indexValues (required) let js_index_values = Reflect::get(&object, &JsValue::from_str("indexValues")) .map_err(|e| WasmDppError::invalid_argument(format!("Missing indexValues: {:?}", e)))?; + if js_index_values.is_undefined() { + return Err(WasmDppError::invalid_argument("indexValues is required")); + }packages/wasm-dpp2/src/identifier.rs (1)
339-355: Consider simplifying the error mapping inidentifiers_from_js_array.The error is already a
WasmDppErrorfromIdentifierWasm::try_from, so wrapping it in anotherinvalid_argumentadds redundant context. The inner error already contains "Invalid identifier" messaging.♻️ Suggested simplification
pub fn identifiers_from_js_array(array: IdentifierLikeArrayJs) -> WasmDppResult<Vec<Identifier>> { let js_value: JsValue = array.into(); let js_array = js_sys::Array::from(&js_value); js_array .iter() - .map(|v| { - IdentifierWasm::try_from(v) - .map(Identifier::from) - .map_err(|err| { - WasmDppError::invalid_argument(format!("Invalid identifier: {}", err)) - }) - }) + .map(|v| IdentifierWasm::try_from(v).map(Identifier::from)) .collect() }packages/wasm-dpp2/src/tokens/configuration_change_item/items/new_tokens_destination_identity.rs (1)
2-20: Align optional identity semantics with the signature.
The function acceptsIdentifierLikeJs(required) but still treatsundefinedasNone, whilenullwill error. If the value is intentionally optional, consider usingIdentifierLikeOrUndefinedJs(orOption<IdentifierLikeJs>) so the signature matches behavior andnull/undefinedare handled consistently.Proposed refactor
-use crate::identifier::{IdentifierLikeJs, IdentifierWasm}; +use crate::identifier::{IdentifierLikeOrUndefinedJs, IdentifierWasm}; @@ #[wasm_bindgen(js_name = "NewTokensDestinationIdentityItem")] pub fn new_tokens_destination_identity_item( - identity_id: IdentifierLikeJs, + identity_id: IdentifierLikeOrUndefinedJs, ) -> WasmDppResult<TokenConfigurationChangeItemWasm> { - let id_value: JsValue = identity_id.into(); - let identity_id: Option<Identifier> = match id_value.is_undefined() { - true => None, - false => Some(IdentifierWasm::try_from(&id_value)?.into()), - }; + let identity_id: Option<Identifier> = identity_id.try_into()?;packages/wasm-sdk/src/queries/group.rs (2)
798-835: Consider hoisting the inline import to the top-level imports.The function correctly uses
identifiers_from_js_arrayfor batch conversion. However, the inlineusestatement on line 803 could be moved to the top-level imports (line 20) for consistency with Rust conventions and to avoid redundant imports if this pattern is used elsewhere.♻️ Suggested refactor
Move the import to the top of the file:
-use wasm_dpp2::identifier::{IdentifierLikeArrayJs, IdentifierLikeJs, IdentifierWasm}; +use wasm_dpp2::identifier::{identifiers_from_js_array, IdentifierLikeArrayJs, IdentifierLikeJs, IdentifierWasm};Then remove the inline import on line 803.
1092-1139: Duplicate inline import - reinforces need to hoist.This function duplicates the inline
use wasm_dpp2::identifier::identifiers_from_js_arraystatement. Moving it to the top-level imports would eliminate this duplication.packages/wasm-dpp2/src/state_transitions/batch/token_transitions/token_transfer.rs (1)
178-216: Consider handlingnullin addition toundefinedfor note setters.The setters for
sharedEncryptedNoteandprivateEncryptedNoteonly checkis_undefined(). In JavaScript, callers might passnullto explicitly clear a value. Currently, passingnullwould attempt conversion and likely fail.♻️ Suggested fix
pub fn set_shared_encrypted_note( &mut self, shared_encrypted_note: &JsValue, ) -> WasmDppResult<()> { let shared_encrypted_note: Option<SharedEncryptedNote> = - match shared_encrypted_note.is_undefined() { + match shared_encrypted_note.is_undefined() || shared_encrypted_note.is_null() { true => None, false => Some( shared_encrypted_note .to_wasm::<SharedEncryptedNoteWasm>("SharedEncryptedNote")? .clone() .into(), ), };Apply the same pattern to
set_private_encrypted_note.packages/wasm-dpp2/src/identity/transitions/credit_withdrawal_transition.rs (1)
218-231: Consider handlingnullfor output script setter.Similar to the token transfer note setters,
set_output_scriptonly checksis_undefined(). JavaScript callers might passnullto clear the output script.♻️ Suggested fix
pub fn set_output_script( &mut self, #[wasm_bindgen(unchecked_param_type = "CoreScript | undefined")] script: &JsValue, ) -> WasmDppResult<()> { - if script.is_undefined() { + if script.is_undefined() || script.is_null() { self.0.set_output_script(None); } else {packages/wasm-dpp2/src/identity/transitions/update_transition.rs (1)
227-233: Consider simplifying identifier conversion.The setter converts
IdentifierLikeJs→JsValue→IdentifierWasm. Based on theTryFrom<IdentifierLikeJs>impl inidentifier.rs(lines 39-42), you can convert directly.♻️ Simplified conversion
pub fn set_identity_identifier(&mut self, identity_id: IdentifierLikeJs) -> WasmDppResult<()> { - let id_value: JsValue = identity_id.into(); - let identity_id = IdentifierWasm::try_from(&id_value)?.into(); + let identity_id: dpp::platform_value::Identifier = identity_id.try_into()?; self.0.set_identity_id(identity_id); Ok(()) }packages/wasm-dpp2/src/public_key.rs (1)
88-117: Consider documenting the auto-detection behavior inset_inner.The setter infers compression from byte length (33 = compressed, 65 = uncompressed), which differs from the constructor's explicit
compressedparameter. This is pragmatic but could surprise callers. Consider adding a JSDoc comment to clarify this behavior.packages/wasm-dpp2/src/group/action_event.rs (1)
67-72: Consider returningOption<TokenEventWasm>for future-proofing.Currently
token_event()unconditionally returnsTokenEventWasmsince there's only one variant. If additional variants are added toGroupActionEventin the future, this method would need updating. ReturningOption<TokenEventWasm>would be more defensive.Optional: Return Option for variant-specific accessor
#[wasm_bindgen(js_name = "tokenEvent")] - pub fn token_event(&self) -> TokenEventWasm { + pub fn token_event(&self) -> Option<TokenEventWasm> { match &self.0 { - GroupActionEvent::TokenEvent(event) => TokenEventWasm::from(event.clone()), + GroupActionEvent::TokenEvent(event) => Some(TokenEventWasm::from(event.clone())), } }packages/wasm-dpp2/src/identity/model.rs (1)
122-129: Consider keeping a backward-compatiblegetPublicKeysalias.If older JS consumers rely on
getPublicKeys, an alias eases migration; otherwise please document the break.♻️ Optional compatibility alias
#[wasm_bindgen(getter = "publicKeys")] pub fn public_keys(&self) -> Vec<IdentityPublicKeyWasm> { self.0 .public_keys() .values() .map(|key| IdentityPublicKeyWasm::from(key.clone())) .collect() } + +#[wasm_bindgen(js_name = "getPublicKeys")] +pub fn get_public_keys(&self) -> Vec<IdentityPublicKeyWasm> { + self.public_keys() +}packages/wasm-dpp2/src/identity/public_key.rs (2)
124-128: Missing validation for non-string input types could cause silent failures.When the input is not a string,
js_sys::Uint8Array::new(&js_value)is called without verifying the input is actually aUint8Array. This can produce unexpected results:
- A number
ncreates a zero-filled array of lengthn- An object or
nullmay create an empty arrayWhile callers like
get_identity_by_public_key_hashvalidate the 20-byte length afterward, this function could silently accept invalid inputs and return misleading data.♻️ Suggested validation
} else { - // Try to convert as Uint8Array - let array = js_sys::Uint8Array::new(&js_value); - Ok(array.to_vec()) + // Try to convert as Uint8Array + if js_value.is_instance_of::<js_sys::Uint8Array>() { + let array = js_sys::Uint8Array::unchecked_from_js(js_value); + Ok(array.to_vec()) + } else { + Err(WasmDppError::invalid_argument( + "Public key hash must be a hex string or Uint8Array", + )) + } }
154-169: Consider clarifying error handling for missing required fields.
Reflect::getreturnsOk(JsValue::UNDEFINED)for missing properties, notErr. The current error mapping on lines 156, 161-163, and 168 only catches cases where the target isn't an object. Ifpurpose,securityLevel, orkeyTypeare missing, the error will come from the subsequentTryFromconversion (e.g.,PurposeWasm::try_from(undefined)), not from these error handlers.This isn't a bug since invalid inputs will still be rejected, but the error messages may differ from expectations.
♻️ Optional: Explicit undefined check
let js_purpose = Reflect::get(&object, &JsValue::from_str("purpose")) - .map_err(|e| WasmDppError::invalid_argument(format!("Missing purpose: {:?}", e)))?; + .unwrap_or(JsValue::UNDEFINED); + if js_purpose.is_undefined() { + return Err(WasmDppError::invalid_argument("Missing required field: purpose")); + } let purpose = PurposeWasm::try_from(js_purpose)?;
packages/wasm-dpp2/src/state_transitions/batch/token_base_transition.rs
Outdated
Show resolved
Hide resolved
packages/wasm-dpp2/src/state_transitions/batch/token_base_transition.rs
Outdated
Show resolved
Hide resolved
| #[wasm_bindgen(typescript_custom_section)] | ||
| const PLATFORM_VERSION_LIKE_TS: &'static str = r#" | ||
| export type PlatformVersionLike = PlatformVersion | string | number; | ||
| "#; | ||
|
|
||
| #[wasm_bindgen] | ||
| extern "C" { | ||
| #[wasm_bindgen(typescript_type = "PlatformVersionLike")] | ||
| pub type PlatformVersionLikeJs; | ||
| } | ||
|
|
||
| impl TryFrom<PlatformVersionLikeJs> for PlatformVersionWasm { | ||
| type Error = WasmDppError; | ||
| fn try_from(value: PlatformVersionLikeJs) -> Result<Self, Self::Error> { | ||
| let js_value: JsValue = value.into(); | ||
| PlatformVersionWasm::try_from(js_value) | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<PlatformVersionLikeJs> for PlatformVersion { | ||
| type Error = WasmDppError; | ||
| fn try_from(value: PlatformVersionLikeJs) -> Result<Self, Self::Error> { | ||
| let wasm: PlatformVersionWasm = value.try_into()?; | ||
| Ok(PlatformVersion::from(wasm)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate numeric PlatformVersionLike inputs to avoid truncation.
Line 18 now advertises numeric inputs, but PlatformVersionWasm::try_from (Lines 85-100) casts f64 directly to u8. A value like 1.5 would silently become PLATFORM_V1. Please add integer validation (similar to NetworkWasm) before casting.
🛠️ Proposed fix
@@
- false => match value.as_f64() {
+ false => match value.as_f64() {
None => Err(WasmDppError::invalid_argument(
"cannot read value from enum",
)),
- Some(enum_val) => match enum_val as u8 {
- 1 => Ok(PlatformVersionWasm::PLATFORM_V1),
- 2 => Ok(PlatformVersionWasm::PLATFORM_V2),
- 3 => Ok(PlatformVersionWasm::PLATFORM_V3),
- 4 => Ok(PlatformVersionWasm::PLATFORM_V4),
- 5 => Ok(PlatformVersionWasm::PLATFORM_V5),
- 6 => Ok(PlatformVersionWasm::PLATFORM_V6),
- 7 => Ok(PlatformVersionWasm::PLATFORM_V7),
- 8 => Ok(PlatformVersionWasm::PLATFORM_V8),
- 9 => Ok(PlatformVersionWasm::PLATFORM_V9),
- 10 => Ok(PlatformVersionWasm::PLATFORM_V10),
- _ => Err(WasmDppError::invalid_argument(format!(
- "unknown platform version value: {}",
- enum_val
- ))),
- },
+ Some(enum_val) => {
+ if enum_val.fract() != 0.0 || enum_val < 1.0 || enum_val > u8::MAX as f64 {
+ return Err(WasmDppError::invalid_argument(format!(
+ "platform version value must be an integer >= 1, got {}",
+ enum_val
+ )));
+ }
+ match enum_val as u8 {
+ 1 => Ok(PlatformVersionWasm::PLATFORM_V1),
+ 2 => Ok(PlatformVersionWasm::PLATFORM_V2),
+ 3 => Ok(PlatformVersionWasm::PLATFORM_V3),
+ 4 => Ok(PlatformVersionWasm::PLATFORM_V4),
+ 5 => Ok(PlatformVersionWasm::PLATFORM_V5),
+ 6 => Ok(PlatformVersionWasm::PLATFORM_V6),
+ 7 => Ok(PlatformVersionWasm::PLATFORM_V7),
+ 8 => Ok(PlatformVersionWasm::PLATFORM_V8),
+ 9 => Ok(PlatformVersionWasm::PLATFORM_V9),
+ 10 => Ok(PlatformVersionWasm::PLATFORM_V10),
+ _ => Err(WasmDppError::invalid_argument(format!(
+ "unknown platform version value: {}",
+ enum_val
+ ))),
+ }
+ }
},🤖 Prompt for AI Agents
In `@packages/wasm-dpp2/src/version.rs` around lines 16 - 41, The numeric path for
PlatformVersionLike is accepting f64s but PlatformVersionWasm::try_from
currently casts f64 to u8 directly; update PlatformVersionWasm::try_from to
validate numeric inputs like NetworkWasm does: if the incoming JsValue is a
number, ensure it is an integer (no fractional part) and within the valid u8
range (0..=255) before casting, and return a WasmDppError for non-integer or
out-of-range values; locate the conversion logic in
PlatformVersionWasm::try_from (and the TryFrom<PlatformVersionLikeJs> impl that
delegates to it) and mirror the integer validation pattern used by NetworkWasm.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
packages/wasm-dpp2/src/enums/batch/gas_fees_paid_by.rs (1)
59-68: Validate numeric JsValue before casting to enum.
as_f64()followed byas u8will silently coerce values like1.7to1and-1to255, producing incorrect variants or mismapped errors. Please validate that the number is finite and an integer within range before casting.🛠️ Proposed fix
- if let Some(enum_val) = value.as_f64() { - return match enum_val as u8 { - 0 => Ok(GasFeesPaidByWasm::DocumentOwner), - 1 => Ok(GasFeesPaidByWasm::ContractOwner), - 2 => Ok(GasFeesPaidByWasm::PreferContractOwner), - _ => Err(WasmDppError::invalid_argument(format!( - "unknown batch type value: {}", - enum_val - ))), - }; - } + if let Some(enum_val) = value.as_f64() { + if !enum_val.is_finite() || enum_val.fract() != 0.0 { + return Err(WasmDppError::invalid_argument(format!( + "unknown batch type value: {}", + enum_val + ))); + } + return match enum_val as i64 { + 0 => Ok(GasFeesPaidByWasm::DocumentOwner), + 1 => Ok(GasFeesPaidByWasm::ContractOwner), + 2 => Ok(GasFeesPaidByWasm::PreferContractOwner), + _ => Err(WasmDppError::invalid_argument(format!( + "unknown batch type value: {}", + enum_val + ))), + }; + }packages/wasm-dpp2/src/state_transitions/batch/token_pricing_schedule.rs (1)
78-96: Inconsistent JS number representation forCreditsvalues may cause precision loss.In the
SinglePricebranch (line 76),Creditsis correctly serialized as a bigint usingbigint_from_str. However, in theSetPricesbranch (line 85),(*value).into()convertsCredits(u64) to a JavaScript number viaJsValue::from(u64), which uses f64 internally. This can lose precision for values exceedingNumber.MAX_SAFE_INTEGER(2^53 - 1).For consistency and correctness, both branches should serialize
Creditsas bigint.Proposed fix
for (key, value) in prices.iter() { Reflect::set( &price_object, &JsValue::from(key.to_string()), - &(*value).into(), + &JsValue::bigint_from_str(&value.to_string()), ) .map_err(|err| {packages/wasm-dpp2/src/state_transitions/batch/token_payment_info.rs (1)
153-156: Bug:set_minimum_token_costcallsset_maximum_token_costinstead.This is a copy-paste error. The setter for
minimumTokenCostincorrectly mutates the maximum cost field.🐛 Proposed fix
#[wasm_bindgen(setter = "minimumTokenCost")] pub fn set_minimum_token_cost(&mut self, minimum_cost: Option<TokenAmount>) { - self.0.set_maximum_token_cost(minimum_cost); + self.0.set_minimum_token_cost(minimum_cost); }packages/wasm-dpp2/src/core/core_script.rs (1)
36-60: Add input validation for hash lengths to match PlatformAddress pattern.Both
from_p2pkhandfrom_p2shsilently truncate inputs longer than 20 bytes via.min(). This is inconsistent with similar functions inpackages/wasm-dpp2/src/platform_address/address.rs(from_p2pkh_hashandfrom_p2sh_hash), which validate the input length and return aWasmDppErrorif the hash is not exactly 20 bytes.If a caller passes an incorrect length (e.g., 32-byte SHA256 instead of 20-byte HASH160), the current implementation silently truncates without error, potentially hiding bugs. Add explicit length validation and return an error for incorrect sizes:
pub fn from_p2pkh(key_hash: Vec<u8>) -> WasmDppResult<Self> { if key_hash.len() != 20 { return Err(WasmDppError::invalid_argument( "key_hash must be exactly 20 bytes".to_string() )); } // ... rest of implementation }packages/wasm-dpp2/src/core/private_key.rs (1)
101-104: Alignto_hex()output case with the parallel method inpublic_key.rs, which returns lowercase hex.The
to_hex()method inprivate_key.rsusesCase::Upper(uppercase), while the parallelto_hex()method inpublic_key.rs(line 355) useshex::encode(), which defaults to lowercase. This inconsistency within the same module violates the principle of least surprise. Additionally, the codebase predominantly usesCase::Lowerfor hex encoding (3 out of 3 other instances). Change toCase::Lowerfor consistency with both the parallel method and codebase convention.packages/wasm-dpp2/src/tokens/configuration/distribution_function.rs (1)
68-103: Validatesteps_with_amountis an object before iterating.With an unchecked param type, non-object inputs can silently yield an empty map and an unexpected distribution function. Consider an explicit guard.
🛠️ Suggested validation guard
pub fn stepwise( #[wasm_bindgen(unchecked_param_type = "Record<string, bigint | number>")] steps_with_amount: JsValue, ) -> WasmDppResult<DistributionFunctionWasm> { - let obj = Object::from(steps_with_amount); + if steps_with_amount.is_null() + || steps_with_amount.is_undefined() + || !steps_with_amount.is_object() + { + return Err(WasmDppError::invalid_argument( + "steps_with_amount must be an object mapping step to amount", + )); + } + let obj = Object::from(steps_with_amount);packages/wasm-dpp2/src/state_transitions/batch/document_base_transition.rs (1)
60-101: Explicitly guard missingdocumentId/dataContractId.
Reflect::getreturnsundefinedwithout error when a property is missing. The current flow then defers toIdentifierWasm::try_from, which yields a less clear error. Add a direct null/undefined check for clearer diagnostics.🛠️ Suggested guard
let js_document_id = Reflect::get(&object, &JsValue::from_str("documentId")) .map_err(|e| WasmDppError::invalid_argument(format!("Missing documentId: {:?}", e)))?; +if js_document_id.is_null() || js_document_id.is_undefined() { + return Err(WasmDppError::invalid_argument("documentId is required")); +} let document_id = IdentifierWasm::try_from(&js_document_id)?.into(); let js_data_contract_id = Reflect::get(&object, &JsValue::from_str("dataContractId")) .map_err(|e| { WasmDppError::invalid_argument(format!("Missing dataContractId: {:?}", e)) })?; +if js_data_contract_id.is_null() || js_data_contract_id.is_undefined() { + return Err(WasmDppError::invalid_argument("dataContractId is required")); +} let data_contract_id = IdentifierWasm::try_from(&js_data_contract_id)?.into();packages/wasm-dpp2/src/identity/partial_identity.rs (1)
279-354: Add validation to reject negative or fractional key IDs before casting tou32.The code casts floats to
u32without validation, allowing fractional values to truncate silently (e.g.,1.5→1) and negative values to wrap, potentially causing key ID collisions. This pattern is already properly validated elsewhere in the codebase (seepackages/wasm-dpp2/src/core/network.rs).Proposed fix
let key_val = key.as_f64().ok_or_else(|| { WasmDppError::invalid_argument("Key identifier must be numeric") })?; + if key_val.fract() != 0.0 || key_val < 0.0 { + return Err(WasmDppError::invalid_argument( + "Key identifier must be a non-negative integer", + )); + } if key_val > u32::MAX as f64 { return Err(WasmDppError::invalid_argument(format!( "Key id '{:?}' exceeds the maximum limit for u32.", key.as_string() ))); }let key_val = key.as_f64().ok_or_else(|| { WasmDppError::invalid_argument("Key id must be a numeric value") })?; + if key_val.fract() != 0.0 || key_val < 0.0 { + return Err(WasmDppError::invalid_argument( + "Key id must be a non-negative integer", + )); + } if key_val > u32::MAX as f64 { return Err(WasmDppError::invalid_argument(format!( "Key id '{:?}' exceeds the maximum limit for u32.", key.as_string() ))); }packages/wasm-dpp2/src/data_contract/document/model.rs (1)
500-542:fromObjectalways usesPlatformVersion::latest()while similar methods accept a version parameter.Unlike
fromBytes,fromHex, andfromBase64which accept aplatform_versionparameter,fromObject(line 534) andfromJSON(line 587) hardcodePlatformVersion::latest(). This inconsistency could cause issues when deserializing documents created with older platform versions.Consider adding an optional
platform_versionparameter tofromObjectandfromJSONfor consistency with the binary deserialization methods.
🤖 Fix all issues with AI agents
In `@packages/wasm-dpp2/src/asset_lock_proof/outpoint.rs`:
- Around line 106-114: The from_bytes implementation on OutPointWasm must
validate the input length instead of unconditionally slicing; change from_bytes
to return a Result (e.g., Result<OutPointWasm, JsValue>), check that
buffer.len() == 36 and return an Err with a clear message for invalid lengths,
otherwise construct the 36-byte array and create OutPoint::from; also update the
related from_hex and from_base64 helper functions to propagate this Result
instead of assuming success so callers receive proper errors.
In `@packages/wasm-dpp2/src/asset_lock_proof/proof.rs`:
- Around line 150-158: The getters instant_lock_proof and chain_lock_proof
currently call the panic-prone From<AssetLockProof> conversion (which panics for
the wrong variant); change each getter to perform a safe runtime check of the
inner AssetLockProof variant (use the same enum/method used by the From impl)
and return a non-panicking value: either return
Option<InstantAssetLockProofWasm>/Option<ChainAssetLockProofWasm> (annotated
with #[wasm_bindgen(js_name = ...)] so JS sees null when absent) or return
JsValue::NULL when the variant doesn't match; ensure you do not call the From
conversion unless the variant matches and reference the AssetLockProof
enum/variant matching logic used in the From<AssetLockProof> implementation.
In `@packages/wasm-dpp2/src/data_contract/document/model.rs`:
- Around line 177-196: The Reflect::get calls (producing properties_js,
document_type_name retrieval, data_contract_id_js, owner_id_js) can return
JsValue::UNDEFINED for missing properties so add explicit undefined checks after
each Reflect::get (e.g., if properties_js.is_undefined() { return
Err(WasmDppError::invalid_argument("Missing 'properties' field")) } and
similarly for documentTypeName, dataContractId, ownerId) before further
conversions; for documentTypeName check undefined first then call
.as_string().ok_or_else(...) as currently done, and for data_contract_id_js and
owner_id_js return the specific "Missing '...'" error if undefined before
calling IdentifierWasm::try_from.
In `@packages/wasm-dpp2/src/data_contract/model.rs`:
- Around line 180-188: Reflect::get returns Ok(JsValue::UNDEFINED) for missing
properties, so update the ownerId and schemas extraction to explicitly check the
returned JsValue for undefined (e.g., js_owner_id.is_undefined() /
js_schema.is_undefined()) and return a clear WasmDppError::invalid_argument when
missing; for ownerId keep using try_into() only after confirming js_owner_id is
not undefined, and for schemas call
serialization::platform_value_from_object(js_schema) only after confirming
js_schema is not undefined so you produce explicit "missing ownerId" / "missing
schemas" errors instead of obscure try_into/serialization errors.
In `@packages/wasm-dpp2/src/epoch/extended_epoch_info.rs`:
- Around line 106-122: The conversions for firstCoreBlockHeight and
protocolVersion currently cast f64 to u32 without bounds checks; add validation
like you did for index (or implement a reusable helper) to ensure the f64 is
finite, non-negative, an integer, and <= u32::MAX before casting. Update the
parsing of "firstCoreBlockHeight" and "protocolVersion" (and/or introduce a
helper used by both) to return a clear WasmDppError on invalid range or type
instead of silent truncation.
- Around line 90-94: The current extraction of index via
Reflect::get(...).as_f64().ok_or(...)? as u16 can silently truncate large or
non-integer JS numbers; modify the logic that reads `index` from `options_obj`
(the Reflect::get call and the `index` binding) to: convert to f64, verify it's
finite, an integer (e.g., floor equality), and within 0..=u16::MAX (65535); if
any check fails, return a WasmDppError::invalid_argument with a clear message,
otherwise cast safely to u16. This ensures `index` cannot overflow or be a
fractional/NaN/Infinity value when assigned to the `u16` variable.
In `@packages/wasm-dpp2/src/identity/public_key.rs`:
- Around line 115-129: The public_key_hash_from_js function accepts arbitrary
JsValue and can coerce non-Uint8Array values into empty arrays and doesn't
enforce HASH160's 20-byte length; update public_key_hash_from_js to first check
if js_value.as_string() yields a hex string and decode it, else verify
js_value.is_instance_of::<js_sys::Uint8Array>() before calling
js_sys::Uint8Array::new(&js_value), convert to Vec<u8>, and then validate the
resulting Vec length equals 20 (HASH160) returning a
WasmDppError::invalid_argument on failure; follow the same input-type check and
length validation pattern used in pro_tx_hash.rs to ensure callers no longer
need to duplicate this logic.
In `@packages/wasm-dpp2/src/identity/transitions/masternode_vote_transition.rs`:
- Around line 116-124: Replace the loose as_f64() parsing for
signaturePublicKeyId with the same robust validation used for nonce: call
try_to_u64 on signature_public_key_js (the variable currently named
signature_public_key_js) when it's not undefined, return a
WasmDppError::invalid_argument if conversion fails, then convert the resulting
u64 into the KeyID type (u32) with an explicit range check to ensure it fits;
update the variable signature_public_key_id to use this validated conversion so
fractional/negative/NaN/Infinity values are rejected consistently.
- Around line 99-115: After each Reflect::get call for pro_tx_hash_js,
voter_identity_id_js, vote_js and the nonce JsValue, add an explicit
undefined/null check (like the existing signaturePublicKeyId handling) and
return WasmDppError::invalid_argument("<field> is required") if the value is
undefined or null; then proceed to call IdentifierWasm::try_from for
pro_tx_hash_js and voter_identity_id_js, vote_js.to_wasm::<VoteWasm>("Vote") for
vote_js, and try_to_u64 for the nonce value so the conversions only run on
present values and produce your intended "X is required" errors.
In
`@packages/wasm-dpp2/src/state_transitions/batch/token_transitions/token_transfer.rs`:
- Around line 75-81: The code currently treats any non-string publicNote as
undefined and silently drops it; update the parsing logic around
Reflect::get(&options_obj, &"publicNote".into()) so that if
public_note_js.is_undefined() you return None, if public_note_js.is_string() you
set public_note = Some(public_note_js.as_string().unwrap()), and otherwise
return a descriptive type error (e.g., throw/return a Js TypeError) indicating
"publicNote must be a string" instead of converting to None; use the existing
variables public_note_js and public_note to locate and change the logic.
In `@packages/wasm-dpp2/src/tokens/configuration/distribution_rules.rs`:
- Around line 194-197: The getter name is inconsistent with the TypeScript
constructor option; change the wasm getter and Rust method to match the TS
property `mintingAllowChoosingDestination`: rename the #[wasm_bindgen(getter =
"...")] attribute and the Rust method `is_minting_allowing_choosing_destination`
(and any corresponding setter method/attribute) to use
`mintingAllowChoosingDestination`, and update any references to the old name so
the wasm interface and the TS constructor option use the same identifier.
- Around line 74-80: Reflect::get returns Ok(undefined) for missing properties,
so update the extraction for perpetual_distribution_rules (and the other
required fields with the same pattern) to explicitly check for undefined before
calling to_wasm: call Reflect::get(&options_obj,
&"perpetualDistributionRules".into())?, bind the JsValue, if
js_value.is_undefined() return
WasmDppError::invalid_argument("perpetualDistributionRules is required"),
otherwise call js_value.to_wasm::<ChangeControlRulesWasm>("ChangeControlRules")
and clone; apply the same pattern to the other fields referenced in this file
(the other Reflect::get usages around the noted line ranges) so missing
properties yield clear invalid_argument errors instead of relying on to_wasm.
In `@packages/wasm-dpp2/src/tokens/configuration/group.rs`:
- Around line 125-135: The get_members function currently returns a JS Map keyed
by Identifier JS objects which breaks value-based lookups; update the API so
keys are stable strings (e.g., base58 of the Identifier) or provide a helper
lookup method: modify get_members (and the GroupMembersMapJs return
type/contract) to insert keys as base58 strings via IdentifierWasm (use
IdentifierWasm -> base58 representation) instead of JS Identifier objects, or
alternatively add a new method like get_member_required_power(identifier:
&IdentifierWasm) / getMemberRequiredPower(identifier) that performs a
value-based lookup against the internal members map and returns the number, and
keep setMemberRequiredPower as-is to avoid reference-equality pitfalls.
In `@packages/wasm-dpp2/src/tokens/configuration/reward_distribution_type.rs`:
- Around line 144-148: The file contains duplicate impl blocks caused by the
macro invocations impl_wasm_type_info!(RewardDistributionTypeWasm,
RewardDistributionType); impl_wasm_type_info!(BlockBasedDistributionWasm,
BlockBasedDistribution); impl_wasm_type_info!(TimeBasedDistributionWasm,
TimeBasedDistribution); impl_wasm_type_info!(EpochBasedDistributionWasm,
EpochBasedDistribution); which duplicate the existing #[wasm_bindgen(js_class =
...)] impls for RewardDistributionTypeWasm, BlockBasedDistributionWasm,
TimeBasedDistributionWasm, and EpochBasedDistributionWasm; fix this by removing
the redundant macro invocations (or conversely remove the explicit
#[wasm_bindgen] impl blocks) so each type has exactly one impl block—preferably
delete the four impl_wasm_type_info!(...) lines to avoid conflicting
wasm_bindgen attributes.
In `@packages/wasm-dpp2/src/voting/vote_poll.rs`:
- Around line 87-98: Reflect::get does not error for missing properties, so
update the parsing of "contractId" and "indexValues" to explicitly check the
returned JsValue for undefined/null before attempting conversions: after calling
Reflect::get for "contractId" and "indexValues" validate with
JsValue::is_undefined() or is_null() and return a WasmDppError::invalid_argument
with a clear message if missing; only then call
IdentifierWasm::try_from(&js_contract_id) and
js_index_values.with_serde_to_platform_value()/into_array() to avoid downstream
panics and confusing errors.
In `@packages/wasm-sdk/src/queries/protocol.rs`:
- Around line 20-21: The field is_threshold_reached currently serializes as
"thresholdReached" (#[serde(rename = "thresholdReached")]) but the public getter
is named isThresholdReached, causing inconsistent external APIs between
toJSON()/serde output and the getter; pick one approach and make them
consistent: either rename the serde key to "isThresholdReached" or rename the
getter to thresholdReached (or add a documented alias) so both serde (the field
is_threshold_reached) and the public accessor (isThresholdReached or
thresholdReached) produce the same JSON/property name; apply the same fix for
the other occurrences referenced around lines 64–67 to keep naming consistent
across the struct.
🧹 Nitpick comments (25)
packages/wasm-dpp2/src/tokens/status.rs (1)
23-27: Potential JS API break:paused→isPausedrename.Line 23 renames the JS-visible getter. If backwards compatibility is required, consider keeping a deprecated alias (or ensure all downstream JS/TS usages and docs are updated in this PR).
♻️ Optional compatibility alias
impl TokenStatusWasm { #[wasm_bindgen(getter = "isPaused")] pub fn is_paused(&self) -> bool { match &self.0 { TokenStatus::V0(v0) => v0.paused(), } } + + // Deprecated alias for compatibility; remove in a future major release. + #[wasm_bindgen(getter = "paused")] + pub fn paused(&self) -> bool { + self.is_paused() + } }packages/wasm-dpp2/src/state_transitions/batch/token_transitions/set_price_for_direct_purchase.rs (1)
42-50: Consider handlingnullin addition toundefined.The constructor checks
price.is_undefined()to determine if a price was provided. In JavaScript, bothundefinedandnullare commonly used to indicate "no value". If a caller passesnull,is_undefined()returnsfalse, causing the code to attemptto_wasmconversion onnull, which will likely produce a confusing error.Proposed fix
- let price: Option<TokenPricingSchedule> = match price.is_undefined() { - true => None, - false => Some( + let price: Option<TokenPricingSchedule> = match price.is_undefined() || price.is_null() { + true => None, + false => Some( price .to_wasm::<TokenPricingScheduleWasm>("TokenPricingSchedule")? .clone() .into(), ), };packages/wasm-sdk/src/queries/mod.rs (1)
303-321: Consider using typed extern types for stricter TypeScript signatures.The change from
JsValuetojs_sys::Objectimproves type safety. However, the relevant code snippets inpackages/wasm-dpp2/src/asset_lock_proof/proof.rsshow usage of custom typed extern types (e.g.,AssetLockProofJSONJs,AssetLockProofObjectJs) which generate more descriptive TypeScript signatures.If full consistency with wasm-dpp2 patterns is desired, you could define typed extern types like:
#[wasm_bindgen] extern "C" { #[wasm_bindgen(typescript_type = "ProofMetadataResponseJSON")] pub type ProofMetadataResponseJSONJs; #[wasm_bindgen(typescript_type = "ProofMetadataResponseObject")] pub type ProofMetadataResponseObjectJs; }That said, using
js_sys::Objectis a valid intermediate step and the current implementation is functionally correct.packages/wasm-dpp2/src/tokens/configuration/configuration_convention.rs (1)
96-99: Consider adding input type validation.
Object::from(localizations_value.clone())will wrap anyJsValue(includingnull,undefined, or primitives) without error. If a non-object value is passed,Object::keys()will return an empty iterator, causing the function to silently return an emptyBTreeMapinstead of an error.While the
unchecked_param_typeattribute provides TypeScript compile-time checking, adding a runtime check would make debugging easier if invalid values slip through:🔧 Optional: Add type validation
fn value_to_localizations( localizations_value: &JsValue, ) -> WasmDppResult<BTreeMap<String, TokenConfigurationLocalization>> { + if !localizations_value.is_object() || localizations_value.is_null() { + return Err(WasmDppError::invalid_argument( + "Localizations must be an object", + )); + } let js_object = Object::from(localizations_value.clone()); let mut localizations = BTreeMap::new();packages/wasm-dpp2/src/state_transitions/base/state_transition.rs (1)
340-345: Consider accepting ownedJsValueto avoid clone.The parameter is
&JsValuebuttry_fromrequires an owned value, necessitating the clone. Acceptingpurpose: JsValuedirectly would be more efficient.♻️ Suggested change
#[wasm_bindgen(js_name = "getKeyLevelRequirement")] pub fn get_key_level_requirement( &self, - purpose: &JsValue, + purpose: JsValue, ) -> WasmDppResult<Option<Vec<String>>> { - let purpose = PurposeWasm::try_from(purpose.clone())?; + let purpose = PurposeWasm::try_from(purpose)?;packages/wasm-dpp2/src/epoch/finalized_epoch_info.rs (2)
236-238: Consider explicit Map type check for clearer error messages.
Map::from(block_proposers_js)wraps the JsValue without validating it's actually a Map. If a consumer bypasses TypeScript and passes a non-Map value, the error will occur during iteration with a less clear message.♻️ Suggested improvement
let block_proposers_js = Reflect::get(&options_obj, &"blockProposers".into()) .map_err(|_| WasmDppError::invalid_argument("blockProposers is required"))?; - let block_proposers = block_proposers_from_map(&Map::from(block_proposers_js))?; + let block_proposers_map = block_proposers_js + .dyn_into::<Map>() + .map_err(|_| WasmDppError::invalid_argument("blockProposers must be a Map"))?; + let block_proposers = block_proposers_from_map(&block_proposers_map)?;
382-391: Same Map validation suggestion applies here.For consistency and clearer error handling, consider using
dyn_into::<Map>()as suggested for the constructor.♻️ Suggested improvement
pub fn set_block_proposers( &mut self, block_proposers: BlockProposersMapJs, ) -> WasmDppResult<()> { - let block_proposers_map = - block_proposers_from_map(&Map::from(JsValue::from(block_proposers)))?; + let js_map = JsValue::from(block_proposers) + .dyn_into::<Map>() + .map_err(|_| WasmDppError::invalid_argument("blockProposers must be a Map"))?; + let block_proposers_map = block_proposers_from_map(&js_map)?; self.v0_mut().block_proposers = block_proposers_map; Ok(()) }packages/wasm-dpp2/src/tokens/configuration/keeps_history_rules.rs (1)
12-45: Consider rejecting unknown option fields to avoid silent typos.Right now, misspelled JS option keys are silently ignored and default to
false. If you want stricter input validation, consider denying unknown fields (confirm this matches desired JS leniency).♻️ Optional strict-validation tweak
-#[derive(Deserialize)] -#[serde(rename_all = "camelCase")] +#[derive(Deserialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] struct TokenKeepsHistoryRulesOptions {packages/wasm-dpp2/src/voting/winner_info.rs (2)
7-33: Tighten TS typings foridentityIdbased onkind.
identityIdis optional for all kinds, so TS won’t enforce it forWonByIdentity. Consider a discriminated union to align with runtime validation.♻️ Proposed refactor
-export interface ContestedDocumentVotePollWinnerInfoObject { - kind: "NoWinner" | "WonByIdentity" | "Locked"; - identityId?: Uint8Array; -} +export type ContestedDocumentVotePollWinnerInfoObject = + | { kind: "NoWinner" | "Locked" } + | { kind: "WonByIdentity"; identityId: Uint8Array }; @@ -export interface ContestedDocumentVotePollWinnerInfoJSON { - kind: "NoWinner" | "WonByIdentity" | "Locked"; - identityId?: string; -} +export type ContestedDocumentVotePollWinnerInfoJSON = + | { kind: "NoWinner" | "Locked" } + | { kind: "WonByIdentity"; identityId: string };
53-78: RejectidentityIdfor non-identity kinds to avoid silent misuse.Right now,
identityIdis ignored forNoWinner/Locked, which can mask caller mistakes. Consider validating and erroring when it’s provided.♻️ Suggested validation
- match kind { - "NoWinner" | "noWinner" | "no_winner" | "none" | "NO_WINNER" => { + match kind { + "NoWinner" | "noWinner" | "no_winner" | "none" | "NO_WINNER" => { + if identity_id.is_some() { + return Err(JsValue::from_str( + "identityId must be omitted when kind is 'NoWinner'", + )); + } Ok(ContestedDocumentVotePollWinnerInfo::NoWinner.into()) } @@ - "Locked" | "locked" | "LOCKED" => { + "Locked" | "locked" | "LOCKED" => { + if identity_id.is_some() { + return Err(JsValue::from_str( + "identityId must be omitted when kind is 'Locked'", + )); + } Ok(ContestedDocumentVotePollWinnerInfo::Locked.into()) }packages/wasm-dpp2/src/tokens/configuration/pre_programmed_distribution.rs (2)
93-138: Validate nested map values before conversion.If a distribution entry value is not a
Map,Map::from(value)can lead to confusing runtime errors. A small guard improves error clarity.🔧 Proposed guard for nested map values
- let inner_map = Map::from(value); + if !value.is_instance_of::<Map>() { + return Err(WasmDppError::invalid_argument( + "Distribution amounts must be a Map", + )); + } + let inner_map = Map::from(value);
168-196: Guard against non-Map inputs in constructor/setter.
Map::fromuses unchecked casts; adding a fast validation improves error messages and avoids runtime TypeError on invalid inputs.🔧 Proposed guard for top-level map inputs
- let distributions_map = distributions_from_map(&Map::from(JsValue::from(distributions)))?; + let distributions_value = JsValue::from(distributions); + if distributions_value.is_null() + || distributions_value.is_undefined() + || !distributions_value.is_instance_of::<Map>() + { + return Err(WasmDppError::invalid_argument( + "distributions must be a Map", + )); + } + let distributions_map = distributions_from_map(&Map::from(distributions_value))?;- let distributions_map = distributions_from_map(&Map::from(JsValue::from(distributions)))?; + let distributions_value = JsValue::from(distributions); + if distributions_value.is_null() + || distributions_value.is_undefined() + || !distributions_value.is_instance_of::<Map>() + { + return Err(WasmDppError::invalid_argument( + "distributions must be a Map", + )); + } + let distributions_map = distributions_from_map(&Map::from(distributions_value))?;packages/wasm-dpp2/JSVALUE_TYPE_AUDIT.md (1)
3-41: Add concrete recommendations or tracking links to keep this audit actionable.Each item currently has an empty “Recommendation:” field. Consider adding a short recommendation, owner, or issue link (or mark TODO) so the checklist can be executed.
packages/wasm-dpp2/src/group/token_event.rs (1)
6-32: Consider more specific TypeScript interface definitions for better type safety.The current interfaces use
[key: string]: unknownwhich is very permissive. Given thatTokenEventis a discriminated union with known variants (Mint, Burn, Freeze, etc.), consider defining variant-specific properties or a discriminated union type for improved TypeScript type checking.For example:
export type TokenEventObject = | { variant: TokenEventVariant.Mint; amount: bigint; recipient: Uint8Array; ... } | { variant: TokenEventVariant.Burn; amount: bigint; ... } // ... other variantsHowever, if the loose typing is intentional for flexibility or due to implementation constraints, this is acceptable.
packages/wasm-dpp2/src/state_transitions/batch/token_transitions/token_emergency_action.rs (1)
53-71: Getter/setter type asymmetry may cause confusion in TypeScript.The
emergencyActiongetter returnsString(line 54-55) while the setter acceptsTokenEmergencyActionWasm(line 69). This asymmetry could lead to confusing TypeScript types where reading and writing the same property requires different types.Consider either:
- Having the getter return
TokenEmergencyActionWasmfor consistency, or- Having the setter accept
String(if it can be parsed back to the enum)♻️ Suggested fix: Return enum type from getter
#[wasm_bindgen(getter = "emergencyAction")] - pub fn get_emergency_action(&self) -> String { - TokenEmergencyActionWasm::from(self.0.emergency_action()).into() + pub fn get_emergency_action(&self) -> TokenEmergencyActionWasm { + TokenEmergencyActionWasm::from(self.0.emergency_action()) }packages/wasm-sdk/src/wallet/key_generation.rs (1)
115-135: Consider pre-allocating the vector capacity.The loop creates key pairs one at a time. Since
countis known upfront, pre-allocating would avoid reallocations.♻️ Suggested improvement
let network_wasm: NetworkWasm = network.try_into()?; let net: Network = network_wasm.into(); - let mut pairs = Vec::new(); + let mut pairs = Vec::with_capacity(count as usize); for _ in 0..count { pairs.push(Self::generate_key_pair_internal(net, &network_wasm)?); }packages/wasm-dpp2/src/state_transitions/batch/token_transition.rs (2)
148-185: Consider avoiding unnecessary clone intransition()getter.The getter uses
self.clone().0which clones the entire wrapper. Since you're only reading and converting the inner value, you could match on&self.0and clone only the specific variant data being returned.♻️ Suggested optimization
#[wasm_bindgen(getter = "transition")] pub fn transition(&self) -> JsValue { - match self.clone().0 { + match &self.0 { TokenTransition::Burn(token_transition) => { - TokenBurnTransitionWasm::from(token_transition).into() + TokenBurnTransitionWasm::from(token_transition.clone()).into() } // ... similar for other variants } }
187-221: Same unnecessary clone pattern intransition_type_number()andtransition_type().These getters only read the discriminant and don't need the inner data, so
self.clone().0is wasteful. Matching on&self.0would avoid the allocation.♻️ Suggested optimization
#[wasm_bindgen(getter = "transitionTypeNumber")] pub fn transition_type_number(&self) -> u8 { - match self.clone().0 { + match &self.0 { TokenTransition::Burn(_) => 0, TokenTransition::Mint(_) => 1, // ... etc } } #[wasm_bindgen(getter = "transitionType")] pub fn transition_type(&self) -> String { - match self.clone().0 { + match &self.0 { TokenTransition::Burn(_) => "Burn".to_string(), // ... etc } }packages/wasm-dpp2/src/state_transitions/batch/token_base_transition.rs (1)
163-171: Consider using conventional boolean ordering for readability.The match arms have inverted ordering (
false => Some,true => None) compared to the typical pattern. While correct, consider reordering for consistency with the constructor pattern at lines 78-87.♻️ Suggested reorder for consistency
let group_info: Option<GroupStateTransitionInfo> = match using_group_info.is_undefined() { - false => Some( - using_group_info - .to_wasm::<GroupStateTransitionInfoWasm>("GroupStateTransitionInfo")? - .clone() - .into(), - ), true => None, + false => Some( + using_group_info + .to_wasm::<GroupStateTransitionInfoWasm>("GroupStateTransitionInfo")? + .clone() + .into(), + ), };packages/wasm-dpp2/src/tokens/configuration/change_control_rules.rs (1)
116-135: Minor: Unnecessary clone in getters.The getters clone
self.0to match on it, which is functional but slightly inefficient. Consider borrowing instead if the enum supports it.♻️ Suggested improvement
- pub fn is_changing_authorized_action_takers_to_no_one_allowed(&self) -> bool { - match self.0.clone() { - ChangeControlRules::V0(v0) => v0.changing_authorized_action_takers_to_no_one_allowed, - } - } + pub fn is_changing_authorized_action_takers_to_no_one_allowed(&self) -> bool { + match &self.0 { + ChangeControlRules::V0(v0) => v0.changing_authorized_action_takers_to_no_one_allowed, + } + }Apply the same pattern to the other two getters.
packages/wasm-dpp2/src/core/pro_tx_hash.rs (1)
70-86: Consider removing redundant error wrapping.The error from
ProTxHashWasm::try_from(v)is already aWasmDppError. Wrapping it again withWasmDppError::invalid_argumentloses the original error details and creates a nested message.♻️ Suggested simplification
pub fn pro_tx_hashes_from_js_array( array: ProTxHashLikeArrayJs, ) -> Result<Vec<ProTxHash>, WasmDppError> { let js_value: JsValue = array.into(); let js_array = js_sys::Array::from(&js_value); js_array .iter() - .map(|v| { - ProTxHashWasm::try_from(v) - .map(ProTxHash::from) - .map_err(|err| { - WasmDppError::invalid_argument(format!("Invalid ProTxHash: {}", err)) - }) - }) + .enumerate() + .map(|(i, v)| { + ProTxHashWasm::try_from(v) + .map(ProTxHash::from) + .map_err(|err| { + WasmDppError::invalid_argument(format!("Invalid ProTxHash at index {}: {}", i, err)) + }) + }) .collect() }This adds the array index to the error message for better debugging while still providing context.
packages/wasm-dpp2/src/identifier.rs (1)
339-355: Consider simplifying the error message inidentifiers_from_js_array.The error wrapping adds "Invalid identifier:" but the underlying
WasmDppErrorfromIdentifierWasm::try_fromlikely already contains descriptive context. This may result in redundant messaging.♻️ Suggested simplification
js_array .iter() .map(|v| { IdentifierWasm::try_from(v) .map(Identifier::from) - .map_err(|err| { - WasmDppError::invalid_argument(format!("Invalid identifier: {}", err)) - }) }) .collect()Alternatively, keep the wrapper but make it add index context:
.enumerate() .map(|(i, v)| { IdentifierWasm::try_from(v) .map(Identifier::from) .map_err(|err| WasmDppError::invalid_argument(format!("Invalid identifier at index {}: {}", i, err))) })packages/wasm-sdk/src/queries/group.rs (1)
798-835: Consider moving the local import to module level.The
use wasm_dpp2::identifier::identifiers_from_js_array;import is duplicated insideget_groups_data_contractsandget_groups_data_contracts_with_proof_info. Moving it to the top-level imports (line 20) would be cleaner and avoid duplication.♻️ Suggested refactor
-use wasm_dpp2::identifier::{IdentifierLikeArrayJs, IdentifierLikeJs, IdentifierWasm}; +use wasm_dpp2::identifier::{IdentifierLikeArrayJs, IdentifierLikeJs, IdentifierWasm, identifiers_from_js_array};Then remove the local
usestatements fromget_groups_data_contracts(line 803) andget_groups_data_contracts_with_proof_info(line 1097).packages/wasm-dpp2/src/data_contract/model.rs (1)
193-197: Use logical OR (||) instead of bitwise OR (|) for boolean conditions.The expression
js_definitions.is_undefined() | js_definitions.is_null()uses bitwise OR which doesn't short-circuit. While functionally equivalent here since both sides are cheap boolean checks, using||is idiomatic and clearer for boolean logic.Suggested fix
let definitions: Option<Value> = - match js_definitions.is_undefined() | js_definitions.is_null() { + match js_definitions.is_undefined() || js_definitions.is_null() { true => None, false => Some(serialization::platform_value_from_object(js_definitions)?), };Same applies at line 203 for
js_tokens.packages/wasm-dpp2/src/data_contract/document/model.rs (1)
545-566:toJSONdoesn't accept aplatform_versionparameter unliketoObject.
toJSONusesPlatformVersion::latest()at line 548, whiletoObjectaccepts aplatform_versionparameter. For API consistency, consider adding the platform version parameter totoJSONas well.
packages/wasm-dpp2/src/tokens/configuration/distribution_rules.rs
Outdated
Show resolved
Hide resolved
…ctionProposer Makes the constructor parameter JS name consistent with the getter/setter which use isActionProposer. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…n-existent ones Replace non-existent get_required_property and get_optional_property_with imports with existing try_from_options, try_from_options_with, and try_from_options_optional_with helpers from wasm-dpp2. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Added try_to_string helper to wasm-dpp2/src/utils.rs for consistent string extraction from JsValue with proper error handling. Updated usages in: - wasm-dpp2/src/data_contract/document/model.rs (documentTypeName) - wasm-dpp2/src/state_transitions/batch/token_transitions/token_transfer.rs (publicNote) - wasm-sdk/src/dpns.rs (label) - removed duplicate extract_string_from_options - wasm-sdk/src/state_transitions/document.rs (documentTypeName) - removed duplicate This improves DRY compliance by eliminating duplicate string extraction logic across packages. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Made WasmDppError constructors public (invalid_argument, serialization, conversion, generic) so wasm-sdk can use them with try_from_options helpers - Added TryFrom<&JsValue> for PutSettingsInput to enable use with try_from_options_optional - Removed extract_settings_from_options wrapper function - Removed duplicate extract_settings_from_options from addresses.rs - Updated all state transition files to use try_from_options_optional directly This improves DRY compliance by using the shared utility functions from wasm-dpp2 instead of custom extraction logic. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add `JsMapExt::from_entries` extension trait for building js_sys::Map from iterators, similar to JavaScript's `new Map(entries)` constructor. Replace repeated Map building patterns across wasm-dpp2 and wasm-sdk: - wasm-sdk/src/queries/epoch.rs (8 instances) - wasm-sdk/src/queries/group.rs (10 instances) - wasm-dpp2/src/epoch/finalized_epoch_info.rs (1 instance) - wasm-dpp2/src/tokens/configuration/group.rs (1 instance) - wasm-dpp2/src/tokens/configuration/pre_programmed_distribution.rs (2 instances) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove unnecessary wrapper utils.rs and use wasm-dpp2's serialization helpers (to_object, from_object, js_value_to_platform_value) directly for consistency and BigInt support. - Delete wasm-sdk/src/utils.rs wrapper module - Replace serde_wasm_bindgen::from_value with from_object - Replace serde_wasm_bindgen::to_value with to_object - Update imports to use wasm_dpp2::serialization::conversions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace three nearly identical array converter functions with a single generic try_to_vec function that converts JS arrays to Vec<T> via WASM wrapper types. This eliminates ~60 lines of duplicate code. Removed functions: - identifiers_from_js_array - pro_tx_hashes_from_js_array - platform_addresses_from_js_array Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace verbose manual loop pattern with idiomatic for loop using js_sys::Iterator::into_iter(). This removes ~47 lines of boilerplate code across 4 functions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…s extraction Add a macro to generate try_from_options and try_from_optional_options static methods on WASM types. This standardizes the pattern for extracting values from JS option objects. - Add impl_try_from_options! macro to utils.rs - Apply macro to 14 WASM types (Identifier, Identity, IdentityPublicKey, etc.) - Add custom try_from_options for signer types with "signer" default field - Convert generic try_from_options::<T>() calls to T::try_from_options() - Clean up unused imports Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use dereference instead of clone for Copy type ProTxHashWasm - Remove useless Revision::from conversions (Revision is u64) - Remove unused Revision import - Remove redundant .map_err(Into::into) calls Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
shumkov
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/wasm-dpp2/src/identity/transitions/pooling.rs (1)
126-140: Unsafe float-to-integer cast can silently produce incorrect results.The
enum_val as u8cast on line 131 has problematic edge cases:
- Negative values like
-1.0saturate to0, silently matchingNever- Non-integer values like
1.5truncate to1, matchingIfAvailable- Out-of-range values saturate (e.g.,
300.0→255)Consider validating the value is a non-negative integer within range before casting.
🔧 Proposed fix with validation
} else { match value.as_f64() { None => Err(WasmDppError::invalid_argument( - "cannot read value from enum", + "pooling must be a string or number", )), - Some(enum_val) => match enum_val as u8 { - 0 => Ok(PoolingWasm::Never), - 1 => Ok(PoolingWasm::IfAvailable), - 2 => Ok(PoolingWasm::Standard), - _ => Err(WasmDppError::invalid_argument(format!( - "unsupported pooling value ({})", - enum_val - ))), + Some(enum_val) => { + if enum_val.fract() != 0.0 || enum_val < 0.0 || enum_val > u8::MAX as f64 { + return Err(WasmDppError::invalid_argument(format!( + "unsupported pooling value ({})", + enum_val + ))); + } + match enum_val as u8 { + 0 => Ok(PoolingWasm::Never), + 1 => Ok(PoolingWasm::IfAvailable), + 2 => Ok(PoolingWasm::Standard), + _ => Err(WasmDppError::invalid_argument(format!( + "unsupported pooling value ({})", + enum_val + ))), + } }, } }packages/wasm-dpp2/src/tokens/configuration/distribution_rules.rs (1)
56-103: Handlenullconsistently for optional fields.Optional fields in the constructor check only
is_undefined(), but the setter fornewTokensDestinationIdentity(lines 244–252) acceptsnullthrough itsIdentifierLikeOrUndefinedJsparameter, which includes|| js_value.is_null()in its conversion logic. This divergence between constructor and setter semantics should be unified—either treat both as absent consistently or accept both everywhere. Apply the sameis_null()check toperpetualDistribution,preProgrammedDistribution, andnewTokensDestinationIdentityin the constructor, and extend it to the setters at lines 199–238 for consistency.packages/wasm-dpp2/src/state_transitions/batch/token_transitions/token_transfer.rs (1)
159-199: Treatnullthe same asundefinedfor optional encrypted notes.The constructor treats null as “absent” via the options helpers, but these setters only check
is_undefined(). Passingnullcurrently errors instead of clearing the field.🛠️ Proposed fix
- let shared_encrypted_note: Option<SharedEncryptedNote> = - if shared_encrypted_note.is_undefined() { + let shared_encrypted_note: Option<SharedEncryptedNote> = + if shared_encrypted_note.is_undefined() || shared_encrypted_note.is_null() { None } else { Some( shared_encrypted_note .to_wasm::<SharedEncryptedNoteWasm>("SharedEncryptedNote")? .clone() .into(), ) }; @@ - let private_encrypted_note: Option<PrivateEncryptedNote> = - if private_encrypted_note.is_undefined() { + let private_encrypted_note: Option<PrivateEncryptedNote> = + if private_encrypted_note.is_undefined() || private_encrypted_note.is_null() { None } else { Some( private_encrypted_note .to_wasm::<PrivateEncryptedNoteWasm>("PrivateEncryptedNote")? .clone() .into(), ) };packages/wasm-dpp2/src/data_contract/document/model.rs (1)
539-548: Validate$entropylength instead of silently dropping.
fromObjectignores invalid$entropylengths, which hides input errors and diverges from constructor/setter validation. Prefer returning a clear error when length ≠ 32.🛠 Suggested validation
- let entropy = map.remove_optional_key("$entropy").and_then(|v| { - v.into_bytes().ok().and_then(|bytes| { - if bytes.len() == 32 { - let mut arr = [0u8; 32]; - arr.copy_from_slice(&bytes); - Some(arr) - } else { - None - } - }) - }); + let entropy = match map.remove_optional_key("$entropy") { + None => None, + Some(v) => { + let bytes = v + .into_bytes() + .map_err(|e| WasmDppError::invalid_argument(e.to_string()))?; + if bytes.len() != 32 { + return Err(WasmDppError::invalid_argument(format!( + "Entropy must be exactly 32 bytes, got {}", + bytes.len() + ))); + } + let mut arr = [0u8; 32]; + arr.copy_from_slice(&bytes); + Some(arr) + } + };
🤖 Fix all issues with AI agents
In `@packages/wasm-dpp2/.mocharc.yml`:
- Around line 1-3: The mocha config uses the unsupported node flag
"experimental-strip-types" under the node-option list which requires Node
>=22.6.0 while package.json engines target >=18.18; either remove
"experimental-strip-types" from the node-option array in .mocharc.yml to
maintain compatibility with Node 18.18–22.5, or update package.json engines to
">=22.6.0" (and ensure CI/test runners use that runtime); keep the
"disable-warning=ExperimentalWarning" entry if desired.
In `@packages/wasm-dpp2/scripts/bundle.cjs`:
- Around line 121-128: The added type block currently imports the wasm namespace
from './dpp.js' and declares initWasm, which creates a circular/self-reference
and mismatches the runtime (which imports raw bindings from
'./raw/wasm_dpp2.no_url.js'). Update the additionalDts content so the namespace
import points to the raw JS binding module used at runtime (e.g., import * as
_wasm from './raw/wasm_dpp2.no_url.js'; export { _wasm as wasm };) and make the
init declaration match the actual exported initializer name/signature used at
runtime (align the declared initWasm to the real init/InitInput/InitOutput types
or re-export the real init name), removing any references that cause dpp.d.ts to
import itself.
In `@packages/wasm-dpp2/src/data_contract/document/model.rs`:
- Around line 29-93: Update the TypeScript typedefs in the DOCUMENT_OPTIONS_TS
section so they match the actual serialization behavior: change
DocumentOptions.revision to accept number | bigint (so constructors can take
either), change DocumentObject.$revision to bigint (since toObject() emits
BigInt when serialize_large_number_types_as_bigints(true) is used), and change
DocumentJSON.$revision to string (BigInt is stringified in JSON). Update the
interfaces DocumentOptions, DocumentObject, and DocumentJSON accordingly.
In `@packages/wasm-dpp2/src/identity/partial_identity.rs`:
- Around line 291-341: The key ID parsing in value_to_loaded_public_keys (and
similarly in option_array_to_not_found, value_to_loaded_public_keys_from_object,
value_to_loaded_public_keys_from_json) only checks the upper bound and allows
negative, fractional, or non-finite values; replace the manual parse+u32 cast
with the existing try_to_u32() helper from utils.rs to fully validate each
Object.keys() entry: call try_to_u32(key_str) (or pass the parsed JsValue as
appropriate) to obtain a validated u32, handle its error via
WasmDppError::invalid_argument with the same contextual message, then use
KeyID::from(validated_u32) instead of casting from f64; update the Reflect::get
and error messages to reference the validated u32 where needed.
In `@packages/wasm-dpp2/src/identity/transitions/update_transition.rs`:
- Around line 236-249: The setters set_public_key_ids_to_disable and
set_user_fee_increase currently take already-coerced Rust types and therefore
allow invalid JS inputs; change their signatures to accept JsValue (or Array for
the keys) and use the same try_to_* helper functions used in the constructor to
parse and validate the incoming JS values before calling
self.0.set_public_key_ids_to_disable(...) and self.0.set_user_fee_increase(...);
ensure invalid inputs produce the same rejection/throw behavior as the
constructor (return/throw an error) rather than silently accepting bad values.
In `@packages/wasm-dpp2/src/state_transitions/base/state_transition.rs`:
- Around line 436-444: In set_owner_id (Wasm state transition) avoid the
unnecessary JsValue round-trip: convert the IdentifierLikeJs directly to
Identifier by calling owner_id.try_into()? (or Identifier::try_from(owner_id) as
used elsewhere) instead of doing let owner_id_value: JsValue = owner_id.into();
let owner_id: Identifier = IdentifierWasm::try_from(&owner_id_value)?.into();
replace that sequence with the direct try_into conversion so types align and
follow the idiomatic pattern used across the codebase.
In `@packages/wasm-dpp2/tests/unit/ChainLockProof.spec.ts`:
- Around line 115-131: The test currently named "should allow to get outPoint"
is misleading because it exercises the setter; rename the test to reflect
setting (e.g., "should allow to set outPoint" or "should update outPoint via
setter") and keep the existing assertions that assign chainlock.outPoint =
newOutpoint and verify chainlock.outPoint.vout and newOutpoint instance type;
locate the spec using the wasm.ChainAssetLockProof, chainlock, outPoint and
newOutpoint identifiers and update only the it() description string accordingly.
♻️ Duplicate comments (6)
packages/wasm-dpp2/src/state_transitions/batch/document_transitions/create.rs (1)
1-23:DocumentTransitionDataJsneedsFrom<JsValue>for.into().
data()returnsDocumentTransitionDataJsvia.into(). Without aFrom<JsValue>impl for the extern type, this will fail. Add the extern-type conversion macro or cast explicitly.🔧 Suggested fix
-use crate::impl_wasm_type_info; +use crate::impl_from_for_extern_type; +use crate::impl_wasm_type_info; @@ #[wasm_bindgen] extern "C" { #[wasm_bindgen(typescript_type = "Record<string, unknown>")] pub type DocumentTransitionDataJs; } + +impl_from_for_extern_type!(DocumentTransitionDataJs);Also applies to: 65-69
packages/wasm-dpp2/src/identity/model.rs (1)
2-6: Ensure extern JS types can be constructed fromJsValue.
to_object/to_jsonreturnIdentityObjectJs/IdentityJSONJsvia.into(). That requires aFrom<JsValue>impl, which wasm-bindgen doesn’t provide for extern types by default. Please add the extern-type conversion macro (or cast explicitly).🔧 Suggested fix
-use crate::impl_try_from_js_value; +use crate::impl_from_for_extern_type; +use crate::impl_try_from_js_value; @@ #[wasm_bindgen] extern "C" { #[wasm_bindgen(typescript_type = "IdentityObject")] pub type IdentityObjectJs; #[wasm_bindgen(typescript_type = "IdentityJSON")] pub type IdentityJSONJs; } + +impl_from_for_extern_type!(IdentityObjectJs; IdentityJSONJs);Also applies to: 42-49, 169-182
packages/wasm-dpp2/src/state_transitions/batch/batch_transition.rs (1)
2-3: Extern JS types needFrom<JsValue>for.into()conversions.
to_object/to_jsonreturnBatchTransitionObjectJs/BatchTransitionJSONJsvia.into(). Without an explicitFrom<JsValue>impl for these extern types, this will fail. Add the extern conversion macro or cast explicitly.🔧 Suggested fix
-use crate::impl_wasm_type_info; +use crate::impl_from_for_extern_type; +use crate::impl_wasm_type_info; @@ #[wasm_bindgen] extern "C" { #[wasm_bindgen(typescript_type = "BatchTransitionObject")] pub type BatchTransitionObjectJs; @@ #[wasm_bindgen(typescript_type = "BatchTransitionJSON")] pub type BatchTransitionJSONJs; } + +impl_from_for_extern_type!(BatchTransitionObjectJs; BatchTransitionJSONJs);Also applies to: 52-59, 213-248
packages/wasm-dpp2/src/asset_lock_proof/proof.rs (2)
137-150: Avoid movingAssetLockProofin getters.
Match on&self.0to avoid moving a large enum in&selfmethods (and keep clippy happy).♻️ Proposed refactor
#[wasm_bindgen(getter = "lockType")] pub fn lock_type(&self) -> AssetLockProofTypeWasm { - match self.0 { + match &self.0 { AssetLockProof::Chain(_) => AssetLockProofTypeWasm::Chain, AssetLockProof::Instant(_) => AssetLockProofTypeWasm::Instant, } } #[wasm_bindgen(getter = "lockTypeName")] pub fn lock_type_name(&self) -> String { - match self.0 { + match &self.0 { AssetLockProof::Chain(_) => AssetLockProofTypeWasm::Chain.into(), AssetLockProof::Instant(_) => AssetLockProofTypeWasm::Instant.into(), } }
153-166: Prevent panic when accessing the wrong proof variant.
instant_lock_proof/chain_lock_proofcurrently call the panic-proneFrom<AssetLockProof>conversion. ReturningOptionavoids runtime panics for mismatched types.🛠️ Safer getters (JS sees null when absent)
#[wasm_bindgen(getter = "instantLockProof")] - pub fn instant_lock_proof(&self) -> InstantAssetLockProofWasm { - self.clone().0.into() - } + pub fn instant_lock_proof(&self) -> Option<InstantAssetLockProofWasm> { + match &self.0 { + AssetLockProof::Instant(instant) => { + Some(InstantAssetLockProofWasm::from(instant.clone())) + } + _ => None, + } + } #[wasm_bindgen(getter = "chainLockProof")] - pub fn chain_lock_proof(&self) -> ChainAssetLockProofWasm { - self.clone().0.into() - } + pub fn chain_lock_proof(&self) -> Option<ChainAssetLockProofWasm> { + match &self.0 { + AssetLockProof::Chain(chain) => { + Some(ChainAssetLockProofWasm::from(chain.clone())) + } + _ => None, + } + }packages/wasm-dpp2/src/identity/transitions/create_transition.rs (1)
95-128: Fix polymorphicassetLockProofconversion.
to_wasm::<AssetLockProofWasm>("AssetLockProof")will rejectChainAssetLockProof/InstantAssetLockProofobjects. Use the constructor that already dispatches by runtime type.🛠️ Proposed fix
- let asset_lock: AssetLockProofWasm = - try_from_options_with(&object, "assetLockProof", |v| { - v.to_wasm::<AssetLockProofWasm>("AssetLockProof") - .map(|r| r.clone()) - })?; + let asset_lock: AssetLockProofWasm = + try_from_options_with(&object, "assetLockProof", |v| { + AssetLockProofWasm::constructor(v) + })?;
🧹 Nitpick comments (14)
packages/wasm-dpp2/tests/unit/DistributionFunction.spec.ts (3)
1-2: Consider renaming file to kebab-case.Per coding guidelines, files within JS packages should use kebab-case. Consider renaming to
distribution-function.spec.ts.
131-251: Consider using descriptive 'should ...' test names.Per coding guidelines, test names should start with 'should ...'. Currently these tests just use the variant name (e.g.,
it('FixedAmountDistribution', ...)). Consider renaming to be more descriptive:- it('FixedAmountDistribution', () => { + it('should return "FixedAmount" for FixedAmountDistribution', () => {This applies to all tests in the
function nameandfunction valuedescribe blocks.
293-303: Add edge case test for Stepwise with multiple entries.The
Stepwisetests only verify a single key-value pair. Consider adding a test with multiple entries to verify iteration order and correct serialization of the full map:🧪 Suggested additional test
it('should handle Stepwise with multiple entries', () => { const distributionFunction = wasm.DistributionFunction.Stepwise({ }); expect(distributionFunction.functionValue).to.deep.equal({ }); });packages/wasm-dpp2/tests/unit/ContestedDocumentVotePollWinnerInfo.spec.ts (1)
53-55: Consider renaming variable for clarity.The variable
identityis misleading since it holds aContestedDocumentVotePollWinnerInfoobject, not anIdentity. A name likewonByIdentityInfoorinfoWithIdentitywould better convey its actual type.♻️ Suggested fix
const identityId = wasm.Identifier.fromBytes(Buffer.from(identityIdHex, 'hex')); - const identity = new wasm.ContestedDocumentVotePollWinnerInfo('Identity', identityId); - expect(identity.isWonByIdentity).to.be.true(); + const wonByIdentityInfo = new wasm.ContestedDocumentVotePollWinnerInfo('Identity', identityId); + expect(wonByIdentityInfo.isWonByIdentity).to.be.true();packages/wasm-dpp2/src/state_transitions/batch/document_transitions/purchase.rs (1)
73-78: Consider using a trait method forset_priceif available.Unlike
set_baseandset_revisionwhich use trait methods,set_pricemanually matches on theV0variant. If new versions are added toDocumentPurchaseTransition, this match arm would need updating. If a setter trait method exists (similar toDocumentPurchaseTransitionV0Methods), consider using it for consistency.packages/wasm-dpp2/src/voting/vote_poll.rs (1)
168-231: Consider eliminating unnecessary clones in setters.All four setters clone
self.0before modification, which is inefficient. Since you're mutating the inner poll and reassigning toself.0, you can modify in place using&mut self.0.♻️ Proposed refactor for set_contract_id (apply similar pattern to other setters)
#[wasm_bindgen(setter = "contractId")] pub fn set_contract_id( &mut self, #[wasm_bindgen(js_name = "contractId")] contract_id: IdentifierLikeJs, ) -> WasmDppResult<()> { let contract_id = contract_id.try_into()?; - self.0 = match self.0.clone() { - VotePoll::ContestedDocumentResourceVotePoll(mut poll) => { - poll.contract_id = contract_id; - - VotePoll::ContestedDocumentResourceVotePoll(poll) - } - }; + match &mut self.0 { + VotePoll::ContestedDocumentResourceVotePoll(poll) => { + poll.contract_id = contract_id; + } + } Ok(()) }packages/wasm-dpp2/src/tokens/configuration/pre_programmed_distribution.rs (1)
83-115: Consider narrowing visibility topub(crate)if not used externally.The function is marked
pubwhile its companiondistribution_amounts_from_mapis private. If this function is only used within the crate, considerpub(crate)to minimize the public API surface.The implementation itself is solid—error handling properly covers iteration failures, timestamp parsing, and nested map conversion using
try_to_mapfor checked conversion.packages/wasm-dpp2/tests/unit/AuthorizedActionTakers.spec.ts (1)
12-12: Fix grammar in test titles (“should allow …”).Optional polish to make the test names read cleanly.
♻️ Suggested edits
- it('should allows to create AuthorizedActionTakers with NoOne', () => { + it('should allow creating AuthorizedActionTakers with NoOne', () => { - it('should allows to create AuthorizedActionTakers with ContractOwner', () => { + it('should allow creating AuthorizedActionTakers with ContractOwner', () => { - it('should allows to create AuthorizedActionTakers with Identity', () => { + it('should allow creating AuthorizedActionTakers with Identity', () => { - it('should allows to create AuthorizedActionTakers with MainGroup', () => { + it('should allow creating AuthorizedActionTakers with MainGroup', () => { - it('should allows to create AuthorizedActionTakers with Group', () => { + it('should allow creating AuthorizedActionTakers with Group', () => { - it('should allows to get value with NoOne', () => { + it('should allow getting value with NoOne', () => { - it('should allows to get value with ContractOwner', () => { + it('should allow getting value with ContractOwner', () => { - it('should allows to get value with Identity', () => { + it('should allow getting value with Identity', () => { - it('should allows to get value with MainGroup', () => { + it('should allow getting value with MainGroup', () => { - it('should allows to get value with Group', () => { + it('should allow getting value with Group', () => {Also applies to: 19-19, 26-26, 33-33, 40-40, 49-49, 55-55, 61-61, 67-67, 73-73
packages/wasm-dpp2/tests/unit/AssetLockProof.spec.ts (1)
34-42: Tighten the error assertion (optional).Consider asserting on the thrown error type/message to make the test more specific and reduce false positives.
packages/wasm-dpp2/src/state_transitions/batch/token_base_transition.rs (1)
152-172: Setter only checksis_undefined(), missingis_null()check.The setter at line 158 only checks
using_group_info.is_undefined(), but the constructor'stry_from_options_optional_withhelper checks bothis_undefined()andis_null(). For consistency with JavaScript semantics (where bothnullandundefinedcommonly represent "no value"), consider checking both.♻️ Suggested fix
- let group_info: Option<GroupStateTransitionInfo> = if using_group_info.is_undefined() { + let group_info: Option<GroupStateTransitionInfo> = if using_group_info.is_undefined() || using_group_info.is_null() { None } else {packages/wasm-dpp2/src/identity/transitions/credit_withdrawal_transition.rs (1)
197-210: Setter only checksis_undefined(), missingis_null()check.Similar to
token_base_transition.rs, this setter only checksscript.is_undefined()but notis_null(). For consistency with JavaScript conventions and thetry_from_options_optional_withhelper (which treats both as "absent"), consider checking both.♻️ Suggested fix
- if script.is_undefined() { + if script.is_undefined() || script.is_null() { self.0.set_output_script(None); } else {packages/wasm-dpp2/tests/unit/CoreScript.spec.ts (1)
1-49: Consider renaming test file to follow kebab-case convention.Per coding guidelines, files in
packages/**/!(node_modules)/**/*.{js,jsx,ts,tsx}should use kebab-case. This fileCoreScript.spec.tsshould be renamed tocore-script.spec.ts. Additionally, test names should ideally start with "should ..." (e.g.,it('should allow to create from bytes', ...)could beit('should create from bytes', ...)).packages/wasm-dpp2/tests/unit/BatchTransition.spec.ts (1)
12-48: Widen identifier option types to match actual inputs.The helpers default to base58 string IDs, but the option interfaces only accept
Identifierinstances. Consider using an Identifier-like union (or the wasm-provided alias) so callers can pass strings/Uint8Array/Identifier consistently.♻️ Suggested refactor
+type IdentifierLike = InstanceType<typeof wasm.Identifier> | string | Uint8Array; + interface DocumentOptions { properties?: Record<string, unknown>; documentTypeName?: string; - dataContractId?: InstanceType<typeof wasm.Identifier>; - ownerId?: InstanceType<typeof wasm.Identifier>; + dataContractId?: IdentifierLike; + ownerId?: IdentifierLike; revision?: bigint; - id?: InstanceType<typeof wasm.Identifier>; + id?: IdentifierLike; } @@ interface TokenBaseTransitionOptions { identityContractNonce?: bigint; tokenContractPosition?: number; - dataContractId?: InstanceType<typeof wasm.Identifier>; - tokenId?: InstanceType<typeof wasm.Identifier>; + dataContractId?: IdentifierLike; + tokenId?: IdentifierLike; }packages/wasm-dpp2/tests/unit/DataContractUpdateStateTransition.spec.ts (1)
74-79: Improve error assertion pattern.The current pattern is a test anti-pattern:
expect(true).to.equal(false)to fail on missing error is unclearexpect(false).to.equal(false)always passes and doesn't verify the errorConsider using Chai's
expect(...).to.throw()or at minimum verify the actual error message/type in the catch block.♻️ Proposed fix
it('should allow to verify incorrect protocol version', () => { const dataContract = wasm.DataContract.fromJSON(value, false, PlatformVersion.PLATFORM_V1); const dataContractTransition = new wasm.DataContractUpdateTransition(dataContract, BigInt(1)); - try { - dataContractTransition.verifyProtocolVersion(20); - expect(true).to.equal(false); - } catch (error) { - expect(false).to.equal(false); - } + expect(() => dataContractTransition.verifyProtocolVersion(20)).to.throw(); });
| node-option: | ||
| - experimental-strip-types | ||
| - disable-warning=ExperimentalWarning |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check package.json in wasm-dpp2 for engine requirements
cat packages/wasm-dpp2/package.json | grep -A 5 "engines"Repository: dashpay/platform
Length of output: 262
🏁 Script executed:
# Also check root package.json for any global engine specs
cat package.json | grep -A 5 "engines"Repository: dashpay/platform
Length of output: 42
🌐 Web query:
What Node.js version introduced the --experimental-strip-types flag?
💡 Result:
Node.js introduced the --experimental-strip-types flag in v22.6.0 (released 2024-08-06). [1]
Node.js version mismatch: --experimental-strip-types requires v22.6.0+, but engines declare >=18.18.
The --experimental-strip-types flag was introduced in Node.js v22.6.0, but package.json declares support for Node 18.18 and above. Test runs on Node 18.18–22.5 will fail with an unknown option error. Either bump the engine requirement to >=22.6.0 or remove the flag.
🤖 Prompt for AI Agents
In `@packages/wasm-dpp2/.mocharc.yml` around lines 1 - 3, The mocha config uses
the unsupported node flag "experimental-strip-types" under the node-option list
which requires Node >=22.6.0 while package.json engines target >=18.18; either
remove "experimental-strip-types" from the node-option array in .mocharc.yml to
maintain compatibility with Node 18.18–22.5, or update package.json engines to
">=22.6.0" (and ensure CI/test runners use that runtime); keep the
"disable-warning=ExperimentalWarning" entry if desired.
| const additionalDts = ` | ||
| // Namespace export for convenient access to all WASM bindings | ||
| import * as _wasm from './dpp.js'; | ||
| export { _wasm as wasm }; | ||
| // Alias for init() with clearer naming | ||
| export declare function initWasm(module_or_path?: { module_or_path: InitInput | Promise<InitInput> } | InitInput | Promise<InitInput>): Promise<InitOutput>; | ||
| `; |
There was a problem hiding this comment.
Type declaration mismatch: wasm namespace imports from wrong source.
The JS wrapper imports from ./raw/wasm_dpp2.no_url.js for the wasm namespace, but the type declaration imports from ./dpp.js. This creates:
- A circular self-reference (
dpp.d.tsimporting itself) - Type/runtime mismatch — the
wasmnamespace at runtime contains only the raw WASM bindings, but the types would includeinit,initWasm,wasmitself, etc.
Proposed fix to align type declaration with JS runtime
// Namespace export for convenient access to all WASM bindings
-import * as _wasm from './dpp.js';
+import * as _wasm from './raw/wasm_dpp2.js';
export { _wasm as wasm };🤖 Prompt for AI Agents
In `@packages/wasm-dpp2/scripts/bundle.cjs` around lines 121 - 128, The added type
block currently imports the wasm namespace from './dpp.js' and declares
initWasm, which creates a circular/self-reference and mismatches the runtime
(which imports raw bindings from './raw/wasm_dpp2.no_url.js'). Update the
additionalDts content so the namespace import points to the raw JS binding
module used at runtime (e.g., import * as _wasm from
'./raw/wasm_dpp2.no_url.js'; export { _wasm as wasm };) and make the init
declaration match the actual exported initializer name/signature used at runtime
(align the declared initWasm to the real init/InitInput/InitOutput types or
re-export the real init name), removing any references that cause dpp.d.ts to
import itself.
| /// TypeScript interface for Document constructor options | ||
| #[wasm_bindgen(typescript_custom_section)] | ||
| const DOCUMENT_OPTIONS_TS: &str = r#" | ||
| /** | ||
| * Options for creating a new Document. | ||
| */ | ||
| export interface DocumentOptions { | ||
| /** Document properties/data */ | ||
| properties: Record<string, unknown>; | ||
| /** Document type name from the data contract */ | ||
| documentTypeName: string; | ||
| /** Data contract ID this document belongs to */ | ||
| dataContractId: IdentifierLike; | ||
| /** Owner identity ID */ | ||
| ownerId: IdentifierLike; | ||
| /** Document revision (default: 1) */ | ||
| revision?: number; | ||
| /** Document ID (auto-generated if not provided) */ | ||
| id?: IdentifierLike; | ||
| /** Entropy bytes (32 bytes, auto-generated if not provided) */ | ||
| entropy?: Uint8Array; | ||
| } | ||
| /** | ||
| * Document serialized as a plain object. | ||
| */ | ||
| export interface DocumentObject { | ||
| $id: Identifier; | ||
| $ownerId: Identifier; | ||
| $revision?: number; | ||
| $createdAt?: number; | ||
| $updatedAt?: number; | ||
| $transferredAt?: number; | ||
| $createdAtBlockHeight?: number; | ||
| $updatedAtBlockHeight?: number; | ||
| $transferredAtBlockHeight?: number; | ||
| $createdAtCoreBlockHeight?: number; | ||
| $updatedAtCoreBlockHeight?: number; | ||
| $transferredAtCoreBlockHeight?: number; | ||
| $dataContractId: Identifier; | ||
| $type: string; | ||
| [key: string]: unknown; | ||
| } | ||
| /** | ||
| * Document serialized as JSON (with string identifiers). | ||
| */ | ||
| export interface DocumentJSON { | ||
| $id: string; | ||
| $ownerId: string; | ||
| $revision?: number; | ||
| $createdAt?: number; | ||
| $updatedAt?: number; | ||
| $transferredAt?: number; | ||
| $createdAtBlockHeight?: number; | ||
| $updatedAtBlockHeight?: number; | ||
| $transferredAtBlockHeight?: number; | ||
| $createdAtCoreBlockHeight?: number; | ||
| $updatedAtCoreBlockHeight?: number; | ||
| $transferredAtCoreBlockHeight?: number; | ||
| $dataContractId: string; | ||
| $type: string; | ||
| [key: string]: unknown; | ||
| } | ||
| "#; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the model.rs file and check its size
wc -l packages/wasm-dpp2/src/data_contract/document/model.rsRepository: dashpay/platform
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Examine the specific lines mentioned (29-93) and surrounding context
sed -n '1,120p' packages/wasm-dpp2/src/data_contract/document/model.rs | cat -nRepository: dashpay/platform
Length of output: 4774
🏁 Script executed:
#!/bin/bash
# Search for how revision is used in the Rust code
rg -n "revision" packages/wasm-dpp2/src/data_contract/document/model.rs -A 3 -B 3Repository: dashpay/platform
Length of output: 1724
🏁 Script executed:
#!/bin/bash
# Look for toObject method and how it handles revision
rg -n "to_object|toObject" packages/wasm-dpp2/src/data_contract/document/ -A 5Repository: dashpay/platform
Length of output: 1933
🏁 Script executed:
#!/bin/bash
# Check test files for revision usage
fd -t f "spec.ts" packages/wasm-dpp2/tests/ | head -5Repository: dashpay/platform
Length of output: 334
🏁 Script executed:
#!/bin/bash
# Find the serialization module and check how it handles u64
fd -t f "serialization.rs" packages/wasm-dpp2/src/ | head -5Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for platform_value_to_object function
rg -n "platform_value_to_object" packages/wasm-dpp2/src/ -A 10 | head -50Repository: dashpay/platform
Length of output: 4443
🏁 Script executed:
#!/bin/bash
# Look for how u64 values are converted in serialization
rg -n "U64|u64" packages/wasm-dpp2/src/serialization/ -B 2 -A 2 | head -100Repository: dashpay/platform
Length of output: 4117
🏁 Script executed:
#!/bin/bash
# Check if there's a Document test spec file
fd -t f "Document.*spec.ts" packages/wasm-dpp2/tests/Repository: dashpay/platform
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Check the Document test file for actual revision usage
head -200 packages/wasm-dpp2/tests/unit/Document.spec.ts | cat -nRepository: dashpay/platform
Length of output: 8254
🏁 Script executed:
#!/bin/bash
# Search for revision usage in Document tests
rg -n "revision" packages/wasm-dpp2/tests/unit/Document.spec.ts -B 2 -A 2Repository: dashpay/platform
Length of output: 2141
Align revision TypeScript types with actual BigInt serialization.
The toObject() method serializes revision as BigInt (due to serialize_large_number_types_as_bigints(true)), but the TypeScript interfaces incorrectly type it as number. This creates a type mismatch: DocumentOptions.revision should accept both input types, while DocumentObject.$revision should reflect the BigInt output.
🛠 Suggested TS type fix
export interface DocumentOptions {
/** Document revision (default: 1) */
- revision?: number;
+ revision?: number | bigint;
/** Document ID (auto-generated if not provided) */
id?: IdentifierLike;
/** Entropy bytes (32 bytes, auto-generated if not provided) */
entropy?: Uint8Array;
}
export interface DocumentObject {
$id: Identifier;
$ownerId: Identifier;
- $revision?: number;
+ $revision?: bigint;🤖 Prompt for AI Agents
In `@packages/wasm-dpp2/src/data_contract/document/model.rs` around lines 29 - 93,
Update the TypeScript typedefs in the DOCUMENT_OPTIONS_TS section so they match
the actual serialization behavior: change DocumentOptions.revision to accept
number | bigint (so constructors can take either), change
DocumentObject.$revision to bigint (since toObject() emits BigInt when
serialize_large_number_types_as_bigints(true) is used), and change
DocumentJSON.$revision to string (BigInt is stringified in JSON). Update the
interfaces DocumentOptions, DocumentObject, and DocumentJSON accordingly.
| impl_wasm_type_info!(PartialIdentityWasm, PartialIdentity); | ||
|
|
||
| pub fn value_to_loaded_public_keys( | ||
| loaded_public_keys: &JsValue, | ||
| ) -> WasmDppResult<BTreeMap<KeyID, IdentityPublicKey>> { | ||
| match js_loaded_public_keys.is_object() { | ||
| false => Err(WasmDppError::invalid_argument( | ||
| if !loaded_public_keys.is_object() { | ||
| return Err(WasmDppError::invalid_argument( | ||
| "loaded_public_keys must be an object", | ||
| )), | ||
| true => { | ||
| let mut map = BTreeMap::new(); | ||
|
|
||
| let pub_keys_object = Object::from(js_loaded_public_keys.clone()); | ||
| let keys = Object::keys(&pub_keys_object); | ||
|
|
||
| for key in keys.iter() { | ||
| let key_val = key.as_f64().ok_or_else(|| { | ||
| WasmDppError::invalid_argument("Key identifier must be numeric") | ||
| })?; | ||
|
|
||
| if key_val > u32::MAX as f64 { | ||
| return Err(WasmDppError::invalid_argument(format!( | ||
| "Key id '{:?}' exceeds the maximum limit for u32.", | ||
| key.as_string() | ||
| ))); | ||
| } | ||
|
|
||
| let key_id = KeyID::from(key_val as u32); | ||
|
|
||
| let js_key = Reflect::get(&pub_keys_object, &key).map_err(|err| { | ||
| let message = err.error_message(); | ||
| WasmDppError::invalid_argument(format!( | ||
| "unable to access loaded public key '{}': {}", | ||
| key_val as u32, message | ||
| )) | ||
| })?; | ||
|
|
||
| let key = js_key | ||
| .to_wasm::<IdentityPublicKeyWasm>("IdentityPublicKey")? | ||
| .clone(); | ||
|
|
||
| map.insert(key_id, IdentityPublicKey::from(key)); | ||
| } | ||
|
|
||
| Ok(map) | ||
| )); | ||
| } | ||
|
|
||
| let mut map = BTreeMap::new(); | ||
|
|
||
| let pub_keys_object = Object::from(loaded_public_keys.clone()); | ||
| let keys = Object::keys(&pub_keys_object); | ||
|
|
||
| for key in keys.iter() { | ||
| // Object.keys() returns strings, so we need to parse them | ||
| let key_str = key | ||
| .as_string() | ||
| .ok_or_else(|| WasmDppError::invalid_argument("Key identifier must be a string"))?; | ||
| let key_val: f64 = key_str.parse().map_err(|_| { | ||
| WasmDppError::invalid_argument(format!("Key identifier '{}' must be numeric", key_str)) | ||
| })?; | ||
|
|
||
| if key_val > u32::MAX as f64 { | ||
| return Err(WasmDppError::invalid_argument(format!( | ||
| "Key id '{:?}' exceeds the maximum limit for u32.", | ||
| key.as_string() | ||
| ))); | ||
| } | ||
|
|
||
| let key_id = KeyID::from(key_val as u32); | ||
|
|
||
| let js_key = Reflect::get(&pub_keys_object, &key).map_err(|err| { | ||
| let message = err.error_message(); | ||
| WasmDppError::invalid_argument(format!( | ||
| "unable to access loaded public key '{}': {}", | ||
| key_val as u32, message | ||
| )) | ||
| })?; | ||
|
|
||
| let key = js_key | ||
| .to_wasm::<IdentityPublicKeyWasm>("IdentityPublicKey")? | ||
| .clone(); | ||
|
|
||
| map.insert(key_id, IdentityPublicKey::from(key)); | ||
| } | ||
|
|
||
| Ok(map) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find KeyID type definition and check for existing parsing helpers
rg -n "pub type KeyID|^type KeyID" -g '*.rs' packages/wasm-dpp2
rg -n "pub type KeyID|^type KeyID" -g '*.rs' packagesRepository: dashpay/platform
Length of output: 902
🏁 Script executed:
# Read the partial_identity.rs file to see the full context
wc -l packages/wasm-dpp2/src/identity/partial_identity.rsRepository: dashpay/platform
Length of output: 115
🏁 Script executed:
# Check the actual code at the specified lines
sed -n '280,470p' packages/wasm-dpp2/src/identity/partial_identity.rs | cat -nRepository: dashpay/platform
Length of output: 7685
🏁 Script executed:
# Check if there are existing key ID parsing helpers in the codebase
rg -n "parse.*key|key.*id.*parse" packages/wasm-dpp2 -g '*.rs' -A 3 | head -40Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Verify the actual line numbers in the file
grep -n "impl_wasm_type_info!(PartialIdentityWasm" packages/wasm-dpp2/src/identity/partial_identity.rs
grep -n "pub fn value_to_loaded_public_keys" packages/wasm-dpp2/src/identity/partial_identity.rs
grep -n "pub fn option_array_to_not_found" packages/wasm-dpp2/src/identity/partial_identity.rsRepository: dashpay/platform
Length of output: 303
🏁 Script executed:
# Test Rust's f64 parsing behavior to confirm it accepts negative/fractional strings
cat > /tmp/test_f64_parse.rs << 'EOF'
fn main() {
// Test negative values
let neg = "-1.5".parse::<f64>();
println!("Parsing '-1.5' as f64: {:?}", neg);
if let Ok(val) = neg {
println!(" As u32: {}", val as u32);
}
// Test fractional
let frac = "1.9".parse::<f64>();
println!("Parsing '1.9' as f64: {:?}", frac);
if let Ok(val) = frac {
println!(" As u32: {}", val as u32);
}
// Test infinity
let inf = "inf".parse::<f64>();
println!("Parsing 'inf' as f64: {:?}", inf);
// Test NaN
let nan = "NaN".parse::<f64>();
println!("Parsing 'NaN' as f64: {:?}", nan);
if let Ok(val) = nan {
println!(" As u32: {}", val as u32);
}
}
EOF
rustc /tmp/test_f64_parse.rs -o /tmp/test_f64_parse && /tmp/test_f64_parseRepository: dashpay/platform
Length of output: 106
🏁 Script executed:
# Search for similar validation patterns elsewhere in the wasm-dpp2 codebase
rg -n "is_finite|fract|negative|fraction" packages/wasm-dpp2 -g '*.rs' -B 2 -A 2Repository: dashpay/platform
Length of output: 4908
🏁 Script executed:
# Read the try_to_u32 and try_to_u16 functions from utils.rs
sed -n '494,530p' packages/wasm-dpp2/src/utils.rs | cat -n
sed -n '540,580p' packages/wasm-dpp2/src/utils.rs | cat -nRepository: dashpay/platform
Length of output: 3008
🏁 Script executed:
# Check if try_to_u32 is already used in partial_identity.rs
rg -n "try_to_u32|from utils" packages/wasm-dpp2/src/identity/partial_identity.rsRepository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Check imports at the top of partial_identity.rs
head -50 packages/wasm-dpp2/src/identity/partial_identity.rs | cat -nRepository: dashpay/platform
Length of output: 1972
🏁 Script executed:
# Look at the complete line ranges mentioned in "Also applies to"
sed -n '343,365p' packages/wasm-dpp2/src/identity/partial_identity.rs | cat -n
sed -n '374,415p' packages/wasm-dpp2/src/identity/partial_identity.rs | cat -nRepository: dashpay/platform
Length of output: 2990
🏁 Script executed:
# Check if there's any existing string-to-u32 parsing helper
rg -n "parse.*u32|parse_key|parse.*from.*str" packages/wasm-dpp2 -g '*.rs' -B 2 -A 5 | head -60Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Look at how network.rs validates the f64 after parsing
sed -n '65,80p' packages/wasm-dpp2/src/core/network.rs | cat -nRepository: dashpay/platform
Length of output: 844
🏁 Script executed:
# Check the full context of the lines review mentioned vs actual line numbers
sed -n '343,366p' packages/wasm-dpp2/src/identity/partial_identity.rs | cat -nRepository: dashpay/platform
Length of output: 1158
🏁 Script executed:
# Verify that try_to_u32 from utils is available but not used in partial_identity.rs
grep -n "try_to_u32\|try_to_u16" packages/wasm-dpp2/src/identity/partial_identity.rsRepository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Check the complete signature of the functions to understand parameter types
sed -n '293,298p' packages/wasm-dpp2/src/identity/partial_identity.rs | cat -n
sed -n '343,348p' packages/wasm-dpp2/src/identity/partial_identity.rs | cat -nRepository: dashpay/platform
Length of output: 623
🏁 Script executed:
# Verify Rust string parsing behavior with a simple test to confirm negative values parse
python3 << 'EOF'
# Simulate what happens with f64 casting in Rust
import struct
# Test -1.0 cast to u32 (undefined behavior in Rust, but typically wraps)
test_cases = [
(-1.0, "negative"),
(1.9, "fractional"),
(float('inf'), "infinity"),
(float('nan'), "nan"),
]
# Show what happens with f64 to u32 casting behavior
for val, desc in test_cases:
# In Rust, casting f64 to u32 when out of range is undefined behavior
# But truncation toward zero is typical
if val < 0 or val > 4294967295 or val != val: # NaN check
print(f"{desc}: {val} -> undefined/wrapping in Rust")
else:
print(f"{desc}: {val} -> {int(val)} as u32")
EOFRepository: dashpay/platform
Length of output: 215
Use the existing try_to_u32() helper to validate key IDs across all four parsing functions.
Current validation only checks the upper bound (key_val > u32::MAX), allowing:
- Negative values to coerce to wrong
u32values - Fractional values to silently truncate (e.g.,
1.9→1) - Non-finite values (NaN, Infinity) to pass through
This silently corrupts key ID mappings. The try_to_u32() utility in utils.rs already provides complete validation (finite + integer + non-negative + range bounds) and is consistent with patterns used elsewhere in the codebase. Apply it to:
value_to_loaded_public_keys(line 293)option_array_to_not_found(line 343)value_to_loaded_public_keys_from_object(line 374)value_to_loaded_public_keys_from_json(line 421)
🤖 Prompt for AI Agents
In `@packages/wasm-dpp2/src/identity/partial_identity.rs` around lines 291 - 341,
The key ID parsing in value_to_loaded_public_keys (and similarly in
option_array_to_not_found, value_to_loaded_public_keys_from_object,
value_to_loaded_public_keys_from_json) only checks the upper bound and allows
negative, fractional, or non-finite values; replace the manual parse+u32 cast
with the existing try_to_u32() helper from utils.rs to fully validate each
Object.keys() entry: call try_to_u32(key_str) (or pass the parsed JsValue as
appropriate) to obtain a validated u32, handle its error via
WasmDppError::invalid_argument with the same contextual message, then use
KeyID::from(validated_u32) instead of casting from f64; update the Reflect::get
and error messages to reference the validated u32 where needed.
| #[wasm_bindgen(setter = "publicKeyIdsToDisable")] | ||
| pub fn set_public_key_ids_to_disable(&mut self, public_keys: Vec<KeyID>) { | ||
| pub fn set_public_key_ids_to_disable( | ||
| &mut self, | ||
| #[wasm_bindgen(js_name = "publicKeys")] public_keys: Vec<KeyID>, | ||
| ) { | ||
| self.0.set_public_key_ids_to_disable(public_keys) | ||
| } | ||
|
|
||
| #[wasm_bindgen(setter = "userFeeIncrease")] | ||
| pub fn set_user_fee_increase(&mut self, user_fee_increase: UserFeeIncrease) { | ||
| pub fn set_user_fee_increase( | ||
| &mut self, | ||
| #[wasm_bindgen(js_name = "userFeeIncrease")] user_fee_increase: UserFeeIncrease, | ||
| ) { | ||
| self.0.set_user_fee_increase(user_fee_increase) |
There was a problem hiding this comment.
Add validation in setters to match constructor safeguards.
Right now these setters accept already‑coerced JS inputs, so invalid values (e.g., floats/NaN/negatives) can slip through silently. Consider parsing from JsValue/Array with the same try_to_* helpers used in the constructor to keep behavior consistent and reject invalid input early.
🛠️ Proposed fix
#[wasm_bindgen(setter = "publicKeyIdsToDisable")]
pub fn set_public_key_ids_to_disable(
&mut self,
- #[wasm_bindgen(js_name = "publicKeys")] public_keys: Vec<KeyID>,
-) {
- self.0.set_public_key_ids_to_disable(public_keys)
-}
+ #[wasm_bindgen(js_name = "publicKeyIdsToDisable")] public_keys: &js_sys::Array,
+) -> WasmDppResult<()> {
+ let public_keys: Vec<KeyID> = public_keys
+ .iter()
+ .enumerate()
+ .map(|(i, v)| try_to_u32(v, &format!("publicKeyIdsToDisable[{}]", i)))
+ .collect::<WasmDppResult<Vec<KeyID>>>()?;
+ self.0.set_public_key_ids_to_disable(public_keys);
+ Ok(())
+}
#[wasm_bindgen(setter = "userFeeIncrease")]
pub fn set_user_fee_increase(
&mut self,
- #[wasm_bindgen(js_name = "userFeeIncrease")] user_fee_increase: UserFeeIncrease,
-) {
- self.0.set_user_fee_increase(user_fee_increase)
-}
+ #[wasm_bindgen(js_name = "userFeeIncrease")] user_fee_increase: JsValue,
+) -> WasmDppResult<()> {
+ let user_fee_increase = try_to_u16(user_fee_increase, "userFeeIncrease")?;
+ self.0.set_user_fee_increase(user_fee_increase);
+ Ok(())
+}🤖 Prompt for AI Agents
In `@packages/wasm-dpp2/src/identity/transitions/update_transition.rs` around
lines 236 - 249, The setters set_public_key_ids_to_disable and
set_user_fee_increase currently take already-coerced Rust types and therefore
allow invalid JS inputs; change their signatures to accept JsValue (or Array for
the keys) and use the same try_to_* helper functions used in the constructor to
parse and validate the incoming JS values before calling
self.0.set_public_key_ids_to_disable(...) and self.0.set_user_fee_increase(...);
ensure invalid inputs produce the same rejection/throw behavior as the
constructor (return/throw an error) rather than silently accepting bad values.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/js-dash-sdk/package.json`:
- Around line 57-60: The peer dependency mismatch is due to `@typescript-eslint`
v8 requiring ESLint "^8.57.0 || ^9.0.0"; update the "eslint" entry in
package.json from "^8.53.0" to a compatible range (e.g., "^8.57.0" or "^9.0.0")
so the `@typescript-eslint` dependency resolves cleanly—ensure the updated
"eslint" version is applied wherever "eslint" is declared (the same package.json
and any duplicated dependency blocks) and run install/lockfile update to persist
the change.
- Around line 63-64: Update the eslint devDependency in package.json to a
version compatible with `@typescript-eslint` v8 (bump "eslint" to at least
"8.57.0" or a caret range like "^8.57.0"); modify the package.json entry that
currently pins "eslint" (alongside the existing
"@typescript-eslint/eslint-plugin" and "@typescript-eslint/parser" entries) and
then reinstall dependencies and run lint to verify no compatibility errors.
In `@packages/js-evo-sdk/package.json`:
- Around line 34-35: The package.json currently pins
"@typescript-eslint/eslint-plugin" and "@typescript-eslint/parser" to v8 while
"eslint-config-airbnb-typescript" v17 only supports v5–v6 and ESLint v8.53.0 is
too old for `@typescript-eslint` v8; update package.json to either bump
"eslint-config-airbnb-typescript" to v18+ (or switch to a config compatible with
`@typescript-eslint` v8) and also upgrade "eslint" to >=8.57.0 (or v9) so the
toolchain versions align; specifically modify the dependency entries for
"eslint-config-airbnb-typescript", "eslint", "@typescript-eslint/eslint-plugin",
and "@typescript-eslint/parser" and then run install (or add a package-lock/yarn
resolution) to ensure consistent, compatible versions.
In `@packages/wasm-dpp2/.eslintrc.cjs`:
- Around line 16-18: Update the ESLint config's parserOptions block so the
TypeScript project path is resolved relative to the config file: add a
tsconfigRootDir property (set to __dirname) alongside parserOptions.project in
the .eslintrc.cjs parserOptions object so the ['./tests/tsconfig.json'] entry is
resolved from the config file location regardless of process.cwd(); modify the
parserOptions object where parserOptions.project is defined to include
tsconfigRootDir = __dirname.
In `@packages/wasm-dpp2/tests/unit/BatchTransition.spec.ts`:
- Around line 52-91: The two tests for "v0 transition" and "v1 transition" are
duplicates; either consolidate them into a single test or make the second
genuinely exercise a v1 path. To fix, either (A) remove one test and keep a
single spec that asserts instances for createTransition
(DocumentCreateTransition), documentTransition (DocumentTransition),
batchedTransition (BatchedTransition) and batch
(BatchTransition.fromBatchedTransitions), or (B) update the "v1 transition" test
to construct a real v1 transition (e.g., set the version on
DocumentCreateTransition or pass a v1 indicator when creating
BatchedTransition/documentTransition) and add assertions for v1-specific
behavior in BatchedTransition and BatchTransition.fromBatchedTransitions so the
two tests differ.
♻️ Duplicate comments (1)
packages/wasm-dpp2/tests/unit/ChainLockProof.spec.ts (1)
114-129: Rename test to reflect setter behavior.Line 114 sets
chainlock.outPoint, so the description should mention setting rather than getting.📝 Proposed fix
- it('should allow to get outPoint', () => { + it('should allow to set outPoint', () => {
🧹 Nitpick comments (7)
packages/js-dash-sdk/src/SDK/Client/Client.spec.ts (1)
120-122: Consider applying consistent error typing across all catch blocks.This change adds
: anyto handle TypeScript's strict catch variable typing, which is fine for test code. However, other catch blocks in this file (lines 168, 226, 291, 318, 369) still use untypedcatch (e), which creates inconsistency. If the project enablesuseUnknownInCatchVariables, those blocks would also need updating.For consistency, consider applying the same pattern to all catch blocks in this file.
packages/js-dash-sdk/src/SDK/Client/Platform/methods/names/register.spec.ts (1)
117-127: Typing change is acceptable; consider improving test reliability.The
catch (e: any)annotation resolves TypeScript's strict handling of caught errors asunknown. This aligns with the broader TS interop improvements in this PR.However, this test lacks a fail assertion to ensure the error is actually thrown. If
register.callsucceeds unexpectedly, the test passes silently without validating the expected error behavior.💡 Suggested improvement for test reliability
it('should fail if DPNS app have no contract set up', async () => { delete platformMock.client.getApps().get('dpns').contractId; try { await register.call(platformMock, 'user.dash', { identity: await generateRandomIdentifier(), }, identityMock); + + expect.fail('Expected an error to be thrown'); } catch (e: any) { expect(e.message).to.equal('DPNS is required to register a new name.'); } });packages/js-dash-sdk/src/SDK/Client/Platform/broadcastStateTransition.ts (1)
60-73: Prefer a typed error shape overanyforcause.
anyturns off safety forgetCode()andmessage. A lightweight local type keeps the intent while still allowinginstanceofchecks.🔧 Suggested refactor
- if (error instanceof ResponseError) { - let cause: any = error; + if (error instanceof ResponseError) { + type ErrorWithCode = { getCode(): number; message: string }; + let cause: ErrorWithCode = error as ErrorWithCode; // Pass DPP consensus error directly to avoid // additional wrappers if (cause instanceof InvalidRequestDPPError) { - cause = cause.getConsensusError(); + cause = cause.getConsensusError() as ErrorWithCode; }packages/wasm-dpp2/tests/unit/AssetLockProof.spec.ts (1)
32-41: Consider using Chai'sthrowassertion for cleaner error testing.The current try/catch pattern with
expect(true).to.be.ok()doesn't verify the error type or message. A more idiomatic approach would be:♻️ Suggested refactor
- it("shouldn't allow to get chain lock proof via constructor", () => { - try { - // eslint-disable-next-line - new (wasm.AssetLockProof as any)('chain'); - } catch { - expect(true).to.be.ok(); - return; - } - expect.fail('Expected an error to be thrown'); + it('should not allow to get chain lock proof via constructor', () => { + expect(() => { + // eslint-disable-next-line + new (wasm.AssetLockProof as any)('chain'); + }).to.throw(); });packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/internal/waitForCoreChainLockedHeight.ts (1)
3-6: Align return type withPromise<void>to avoid misleading consumers.Line 6 still exposes
Promise<any>even though the internal promise isPromise<void>(line 18). This inaccurate type signature suggests a resolved value exists when the promise resolvesvoid. Tighten the public signature and thecanceltype to match the actual behavior.🔧 Suggested type alignment
-): Promise<{ promise: Promise<any>, cancel: Function }> { +): Promise<{ promise: Promise<void>, cancel: () => void }> {packages/wasm-dpp2/tests/unit/ContractBounds.spec.ts (1)
120-122: Consider asserting the error type/message.Line 120 uses a generic
to.throw()which can pass for unrelated exceptions; asserting the expected error (type or message) makes the test more precise.packages/wasm-dpp2/tests/unit/BatchTransition.spec.ts (1)
40-48: Minor:tokenIddefaults toownerId- verify this is intentional.Using
ownerIdas the default fortokenId(line 46) works since both areIdentifiertypes, but it may cause confusion when debugging tests. Consider using a dedicated mocktokenIdfor clarity, or add a brief comment explaining the choice.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/js-evo-sdk/tests/.eslintrc.cjs`:
- Around line 15-17: The ESLint config's parserOptions.project is pointing to
the wrong path; update the parserOptions.project value (the property named
"parserOptions.project" in the .eslintrc.cjs file) to use './tsconfig.json'
instead of './tests/tsconfig.json' so the parser resolves the actual tests
tsconfig.
🧹 Nitpick comments (4)
packages/js-evo-sdk/tests/tsconfig.json (1)
8-8: Consider enabling stricter type checking in the future.While
strict: falseis pragmatic during initial migration from JavaScript to TypeScript, consider enabling stricter settings incrementally (e.g.,strictNullChecks,noImplicitAny) once the migration stabilizes to catch more type-related issues in tests.packages/js-evo-sdk/tests/unit/facades/addresses.spec.ts (1)
171-177: Use a valid P2PKH script payload to avoid brittle tests.
new Uint8Array(25).fill(5)is unlikely to be a valid script. Ifwithdraw()starts validating/decoding scripts, this test may fail. Prefer a minimal valid P2PKH script byte pattern (or a wasm-sdk helper if that’s now required).♻️ Proposed tweak (valid P2PKH script bytes)
- outputScript: new Uint8Array(25).fill(5), // Mock P2PKH script bytes + // OP_DUP OP_HASH160 <20-byte pubKeyHash> OP_EQUALVERIFY OP_CHECKSIG + outputScript: new Uint8Array([ + 0x76, 0xa9, 0x14, + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, + 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14, + 0x88, 0xac, + ]),packages/js-evo-sdk/src/.eslintrc.cjs (2)
12-15: Consider keeping Airbnb/TypeScript rules instead of droppingairbnb-typescript/base.This change likely reduces lint coverage vs. the repo’s stated ESLint baseline. If the swap is intentional, it would help to document the rationale; otherwise, consider reverting. Based on learnings, this repo expects Airbnb/TypeScript rules for JS/TS.
♻️ Suggested adjustment
extends: [ 'airbnb-base', - 'plugin:`@typescript-eslint/recommended`', + 'airbnb-typescript/base', ],
20-23: Avoid turning offimport/no-unresolvedglobally.Disabling this can hide broken imports and module resolution regressions. Prefer enabling TS resolver support or scoping exceptions to generated/test files via
overrides.♻️ Suggested adjustment
rules: { - 'import/no-unresolved': 'off', + 'import/no-unresolved': 'error', }, +settings: { + 'import/resolver': { + typescript: { project: ['../tsconfig.json'] }, + }, +},
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/js-evo-sdk/tests/unit/facades/documents.spec.ts (1)
58-203: Rename tests to start with “should …”.Test names in
**/tests/**should start with “should …”. As per coding guidelines.Proposed renames
- it('query() fetches documents matching criteria', async () => { + it('should query documents matching criteria', async () => { ... - it('queryWithProof() fetches documents with proof metadata', async () => { + it('should queryWithProof documents with proof metadata', async () => { ... - it('get() fetches a single document by ID', async () => { + it('should get a single document by ID', async () => { ... - it('getWithProof() fetches a single document with proof', async () => { + it('should getWithProof a single document with proof', async () => { ... - it('create() creates a new document', async () => { + it('should create a new document', async () => { ... - it('replace() replaces an existing document', async () => { + it('should replace an existing document', async () => { ... - it('delete() deletes a document', async () => { + it('should delete a document', async () => { ... - it('delete() accepts document identifiers instead of Document instance', async () => { + it('should delete using document identifiers instead of Document instance', async () => { ... - it('transfer() transfers document ownership to another identity', async () => { + it('should transfer document ownership to another identity', async () => { ... - it('purchase() purchases a document from another identity', async () => { + it('should purchase a document from another identity', async () => { ... - it('setPrice() sets a price on a document for sale', async () => { + it('should setPrice on a document for sale', async () => {packages/js-evo-sdk/tests/unit/facades/contracts.spec.ts (1)
58-143: Rename tests to start with “should …”.Test names in
**/tests/**should start with “should …”. As per coding guidelines.Proposed renames
- it('fetch() returns a DataContract for valid ID', async () => { + it('should fetch a DataContract for a valid ID', async () => { ... - it('fetchWithProof() returns DataContract with proof metadata', async () => { + it('should fetchWithProof a DataContract with proof metadata', async () => { ... - it('getHistory() fetches contract version history', async () => { + it('should getHistory fetch contract version history', async () => { ... - it('getHistoryWithProof() fetches contract version history with proof', async () => { + it('should getHistoryWithProof fetch contract version history with proof', async () => { ... - it('getMany() fetches multiple contracts by IDs', async () => { + it('should getMany fetch multiple contracts by IDs', async () => { ... - it('getManyWithProof() fetches multiple contracts with proof', async () => { + it('should getManyWithProof fetch multiple contracts with proof', async () => { ... - it('publish() publishes a new data contract', async () => { + it('should publish a new data contract', async () => { ... - it('update() updates an existing data contract', async () => { + it('should update an existing data contract', async () => {
🤖 Fix all issues with AI agents
In `@packages/js-evo-sdk/tests/unit/facades/dpns.spec.ts`:
- Around line 37-76: Rename the two test descriptions so they start with "should
...": change the it(...) titled
"convertToHomographSafe/isValidUsername/isContestedUsername await wasm statics"
to something like "should await wasm statics for
convertToHomographSafe/isValidUsername/isContestedUsername" and change "name
resolution and registration forward correctly" to "should forward name
resolution and registration correctly"; keep the test bodies and assertions
intact (these tests call client.dpns.convertToHomographSafe,
client.dpns.isValidUsername, client.dpns.isContestedUsername,
client.dpns.isNameAvailable, client.dpns.resolveName, client.dpns.registerName,
client.dpns.usernames, client.dpns.username, client.dpns.usernamesWithProof,
client.dpns.usernameWithProof, client.dpns.getUsernameByName,
client.dpns.getUsernameByNameWithProof and the corresponding stub expectations).
In `@packages/js-evo-sdk/tests/unit/facades/epoch.spec.ts`:
- Around line 39-76: Update the three test titles so they start with "should
...": rename the it(...) descriptions for the epochs tests (currently
"epochsInfo and finalizedInfos forward queries untouched"), the current tests
(currently "current and currentWithProof forward"), and the evonodes tests
(currently "evonodesProposedBlocks* forward with args") to begin with "should"
and keep the rest of the wording (e.g., "should forward epochsInfo and
finalizedInfos ...", "should forward current and currentWithProof", "should
forward evonodesProposedBlocks* with args") ensuring the strings passed to the
it(...) calls for these tests are updated accordingly.
In `@packages/js-evo-sdk/tests/unit/facades/protocol.spec.ts`:
- Around line 27-47: Rename the three test case descriptions so they start with
"should ...": change "versionUpgradeState and versionUpgradeStateWithProof
forward" to "should forward versionUpgradeState and
versionUpgradeStateWithProof"; change "versionUpgradeVoteStatus and withProof
forward with positional args" to "should forward versionUpgradeVoteStatus and
versionUpgradeVoteStatusWithProof with positional args"; and change
"versionUpgradeVoteStatus accepts Uint8Array and positional args" to "should
accept Uint8Array and positional args for versionUpgradeVoteStatus and
versionUpgradeVoteStatusWithProof" — update the string literals passed to the
it(...) calls that contain those exact texts so test names follow the "should
..." convention.
In `@packages/js-evo-sdk/tests/unit/facades/system.spec.ts`:
- Around line 37-55: Rename the test description string in the failing spec so
it begins with "should ...": change the it(...) call currently labeled "forwards
all methods to instance methods" to a description like "should forward all
methods to instance methods" (update the test name in the it(...) invocation in
the test case that calls client.system.* and asserts the various stubs such as
getStatusStub, getCurrentQuorumsInfoStub, etc.).
♻️ Duplicate comments (1)
packages/wasm-dpp2/.eslintrc.cjs (1)
16-18: AddtsconfigRootDirfor reliable path resolution in monorepo.This issue was previously flagged: without
tsconfigRootDir, the parser resolvesprojectrelative toprocess.cwd(), which may vary depending on how ESLint is invoked. AddingtsconfigRootDir: __dirnameensures the tsconfig path resolves correctly regardless of the working directory.Suggested fix
parserOptions: { + tsconfigRootDir: __dirname, project: ['./tests/tsconfig.json'], },
🧹 Nitpick comments (1)
packages/js-evo-sdk/tests/unit/facades/voting.spec.ts (1)
176-207: Consider usingcalledOnceWithExactlyfor consistency.Lines 190 and 206 use
calledWithExactlywhile line 173 usescalledOnceWithExactly. SincebeforeEachcreates fresh stubs for each test, usingcalledOnceWithExactlywould be more precise and consistent, ensuring the method wasn't called more times than expected.♻️ Suggested change
- expect(masternodeVoteStub).to.be.calledWithExactly(abstainOptions); + expect(masternodeVoteStub).to.be.calledOnceWithExactly(abstainOptions);- expect(masternodeVoteStub).to.be.calledWithExactly(lockOptions); + expect(masternodeVoteStub).to.be.calledOnceWithExactly(lockOptions);
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only