refactor(sdk)!: typed params for state transition methods #2932
Conversation
📝 WalkthroughWalkthroughAdds a macro-based options extraction utility to wasm-dpp2, converts many WASM SDK state-transition bindings to take single options objects, introduces full document/contract/identity/token WASM flows and JS facades updates, and adjusts serialization for data contract schemas to use platform values. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript caller
participant WASM as WasmSdk (wasm_bindgen)
participant Conv as wasm-dpp2 converters
participant SDK as Inner Rust SDK (signing/builder)
participant Network as Platform / Broadcast
rect rgba(200,230,255,0.4)
JS->>WASM: call contractPublish(options)
end
rect rgba(230,250,230,0.4)
WASM->>Conv: DataContractWasm::try_from_options(options.dataContract)
WASM->>Conv: IdentitySignerWasm::try_from_options(options.signer)
WASM->>WASM: extract_settings_from_options(options.settings)
end
rect rgba(255,240,220,0.35)
WASM->>SDK: build & sign publish transition
SDK->>Network: broadcast transition
Network-->>SDK: broadcast result
SDK-->>WASM: result / published DataContract
WASM-->>JS: return DataContractWasm
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (10)📚 Learning: 2024-10-18T15:37:21.329ZApplied to files:
📚 Learning: 2024-10-18T15:37:36.387ZApplied to files:
📚 Learning: 2025-07-28T20:00:08.502ZApplied to files:
📚 Learning: 2025-07-28T20:00:24.323ZApplied to files:
📚 Learning: 2025-12-29T10:53:03.792ZApplied to files:
📚 Learning: 2025-06-18T03:44:14.385ZApplied to files:
📚 Learning: 2024-10-30T11:19:59.163ZApplied to files:
📚 Learning: 2025-08-05T13:55:39.147ZApplied to files:
📚 Learning: 2025-01-23T02:10:08.979ZApplied to files:
📚 Learning: 2025-11-25T13:10:47.943ZApplied to files:
🧬 Code graph analysis (3)packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs (2)
packages/js-evo-sdk/tests/unit/facades/identities.spec.mjs (1)
packages/js-evo-sdk/tests/unit/facades/tokens.spec.mjs (1)
🪛 Gitleaks (8.30.0)packages/js-evo-sdk/tests/unit/facades/tokens.spec.mjs[high] 12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🔇 Additional comments (4)
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 |
✅ gRPC Query Coverage Report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/wasm-sdk/tests/functional/protocol.spec.mjs (1)
44-47: Same empty string verification applies here.This test uses the same fallback pattern as the previous test. The same verification for empty string handling in
getProtocolVersionUpgradeVoteStatusWithProofInfo()applies here as well.
🧹 Nitpick comments (16)
packages/rs-sdk/docs/IMPROVE_RETRY_ERROR_HANDLING.md (1)
8-8: Specify language for fenced code blocks.Markdown linting requires language identifiers on code blocks for consistency and syntax highlighting. Lines 8 and 13 contain code examples without language specifiers.
🔎 Proposed fix
-Example: `getDataContractHistory()` fails with proof verification error, but after all addresses are banned, the user sees: -``` +Example: `getDataContractHistory()` fails with proof verification error, but after all addresses are banned, the user sees: +```text Dapi client error: no available addresses to use -``` +```text Instead of the meaningful: -``` +```text Proof verification error: <actual error details> -``` +```textAlso applies to: 13-13
packages/wasm-sdk/tests/functional/epochs-blocks.spec.mjs (2)
20-24: Consider extracting the magic delay constant.The 100ms delay works, but it's a magic number. Consider extracting it as a named constant for clarity, or adding a brief comment explaining why this specific duration was chosen.
🔎 Optional: Extract named constant
+const CLEANUP_DELAY_MS = 100; // Allow pending microtasks to settle + after(async () => { - // Wait briefly to ensure any pending async operations complete - await new Promise((resolve) => { setTimeout(resolve, 100); }); + await new Promise((resolve) => { setTimeout(resolve, CLEANUP_DELAY_MS); }); if (client) { client.free(); } });
65-98: Test may be skipped entirely if no proTxHash is available.When both
evonodeProTxHashis null andbyRangereturns no results, the coregetEvonodesProposedEpochBlocksByIdsWithProofInfocall and its assertions (lines 92-97) are skipped entirely. Consider adding athis.skip()or a log message to make it explicit when this happens, so test runs are transparent about what was actually verified.🔎 Optional: Make test skip explicit
// Only test by IDs if we have a valid proTxHash if (testProTxHash) { const res = await client.getEvonodesProposedEpochBlocksByIdsWithProofInfo(epochIndex, [testProTxHash]); expect(res).to.be.ok(); expect(res.data).to.be.instanceOf(Map); expect(res.proof).to.be.ok(); expect(res.metadata).to.be.ok(); + } else { + this.skip(); // No proTxHash available to test byIds with proof }packages/wasm-sdk/src/wallet/key_generation.rs (1)
268-272: Consider moving imports to module level.These imports are used only within this function, but module-level imports are conventional in Rust. This is a minor style consideration that doesn't affect functionality.
🔎 Suggested refactor
Move these imports to the top of the file with other imports:
use dash_sdk::dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; use dash_sdk::dpp::identity::IdentityPublicKey; use dash_sdk::dpp::version::LATEST_PLATFORM_VERSION; use rand::rngs::StdRng; use rand::SeedableRng;packages/wasm-sdk/tests/functional/transitions/documents.spec.mjs (1)
192-195: Consider more meaningful assertions for replace/delete/transfer tests.
expect(true).to.be.trueis essentially a no-op. While the tests do verify success implicitly (no exception thrown), consider asserting on the operation result or fetching the document to verify the change. Based on learnings, use dirty-chai function-call style.🔎 Example improvement for documentReplace
// After replace, optionally fetch and verify the document was updated // Or at minimum, use a more descriptive assertion pattern // The current approach relies on "no throw = success" which is valid but weakpackages/wasm-sdk/src/state_transitions/document.rs (2)
29-39: Consider consolidating string extraction with other option utilities.
extract_string_from_optionsis only used once (line 373 fordocumentTypeName). Consider either:
- Moving to the shared utils module if it will be reused
- Inlining at the call site if truly single-use
105-106: Document cloning may be unnecessary.
document_wasm.clone()is called even thoughdocument_wasmis consumed immediately after. This pattern repeats in replace, transfer, purchase, and set_price methods. Consider using the value directly without cloning when possible.🔎 Example for document_create
- let document: Document = document_wasm.clone().into(); + let document: Document = document_wasm.into();Note: This depends on whether
document_wasmis used after the conversion. Check each method individually.packages/wasm-dpp2/src/private_key.rs (1)
129-151: Implementation looks correct.The
try_from_optionsmethod properly validates field presence and handles conversions with clear error messages. The pattern is consistent with similar manual implementations in this PR.Consider whether this manual implementation could use the
impl_try_from_options!macro for consistency with other types likeIdentityWasm,CoreScriptWasm, andDocumentWasm. The manual approach may be necessary due to theIntoWasmtrait usage, but verifying this would improve codebase uniformity.packages/wasm-dpp2/src/identifier.rs (1)
297-316: Implementation is correct.The
try_from_optionsmethod properly validates field presence and delegates to the existingtry_fromimplementation. Error handling is clear and consistent.Similar to
PrivateKeyWasm, consider whether theimpl_try_from_options!macro could be used here for consistency with types likeIdentityWasmandDocumentWasm, assuming the macro can accommodate the conversion logic.packages/wasm-sdk/tests/functional/transitions/contracts.spec.mjs (1)
87-156: Test dependency between suites.The
contractUpdatetest depends oncreatedContractIdbeing set by the priorcontractPublishtest. While Mocha runs tests sequentially by default (making this work), this creates an implicit ordering dependency. Consider documenting this dependency explicitly or using a shared setup if the tests ever need to run independently.The test logic itself is sound—proper security level selection (CRITICAL for updates), nonce cache refresh, schema augmentation, and version verification.
packages/wasm-sdk/tests/functional/transitions/identity.spec.mjs (1)
126-158: Test interdependency between add and disable key tests.The "disables a public key" test (line 126) depends on the previous "adds a new public key" test having successfully added a MEDIUM-level key. Line 141 asserts
keyToDisableexists, which will fail if the previous test didn't run or failed.This is acceptable for functional/integration tests where sequential operations are being validated, but consider adding a comment noting the dependency.
🔎 Suggested documentation improvement
it('disables a public key on identity', async function testDisablePublicKey() { + // Note: This test depends on the previous test having added a MEDIUM-level key // Identity update requires MASTER key (key index 0) const { signer } = createTestSignerAndKey(sdk, 1, 0);packages/wasm-dpp2/src/identity/signer.rs (1)
103-137: Multi-key-type support correctly implemented, but clippy warning should be addressed.The
get_key_hashmethod properly distinguishes betweenECDSA_HASH160(20-byte hash) andECDSA_SECP256K1(33-byte compressed pubkey that needs hashing). The logic is sound.However, clippy reports
result_large_errbecauseProtocolErroris at least 768 bytes. SinceProtocolErroris from thedppcrate, you can suppress this locally:🔎 Proposed fix to suppress clippy warning
+ #[allow(clippy::result_large_err)] fn get_key_hash(identity_public_key: &IdentityPublicKey) -> Result<[u8; 20], ProtocolError> {Alternatively, if this pattern is widespread, consider using
Box<ProtocolError>in theErrvariant at a higher level, but that would require broader changes to the dpp crate.packages/wasm-sdk/src/state_transitions/token.rs (3)
329-338: Redundant call toextract_settings_from_optionsfor user fee increase.Settings are already extracted on line 300 and stored in
settings. The code on lines 334-335 callsextract_settings_from_optionsagain, which duplicates the deserialization work.🔎 Proposed fix to reuse the already-extracted settings
// Add user fee increase from settings - let user_fee_increase = - get_user_fee_increase(extract_settings_from_options(&options_value)?.as_ref()); + let user_fee_increase = get_user_fee_increase(settings.as_ref()); if user_fee_increase > 0 { builder = builder.with_user_fee_increase(user_fee_increase); }
584-589: Same redundantextract_settings_from_optionspattern in token_burn.This pattern is repeated across multiple token operations. Consider fixing this in all places for consistency and efficiency.
🔎 Proposed fix
// Add user fee increase from settings - let user_fee_increase = - get_user_fee_increase(extract_settings_from_options(&options_value)?.as_ref()); + let user_fee_increase = get_user_fee_increase(settings.as_ref()); if user_fee_increase > 0 { builder = builder.with_user_fee_increase(user_fee_increase); }
831-836: Repeated redundantextract_settings_from_optionscalls across all token operations.This same pattern appears in token_transfer (831-836), token_freeze (1061-1066), token_unfreeze (1291-1296), token_destroy_frozen (1502-1507), token_emergency_action (1726-1731), token_claim (1935-1940), token_set_price (2177-2182), and token_direct_purchase (2399-2404).
Consider refactoring all of these to reuse the already-extracted
settingsvariable.Also applies to: 1061-1066, 1291-1296, 1502-1507, 1726-1731, 1935-1940, 2177-2182, 2399-2404
packages/wasm-sdk/src/state_transitions/addresses.rs (1)
924-931: Identity extraction inidentity_create_from_addressesuses different pattern than other methods.Lines 924-931 use manual
js_sys::Reflect::getandto_wasmfor identity extraction, while other methods in this file (lines 303-304, 511-512) useIdentityWasm::try_from_options. Consider using the consistent pattern for uniformity.🔎 Proposed fix for consistency
- // Extract identity from options - let identity_js = js_sys::Reflect::get(&options_value, &JsValue::from_str("identity")) - .map_err(|_| WasmSdkError::invalid_argument("identity is required"))?; - let identity: Identity = identity_js - .to_wasm::<wasm_dpp2::IdentityWasm>("Identity")? - .clone() - .into(); + // Extract identity from options + let identity: Identity = IdentityWasm::try_from_options(&options_value, "identity")?.into();
| impl From<TransferResult> for TokenTransferResultWasm { | ||
| fn from(result: TransferResult) -> Self { | ||
| match result { | ||
| TransferResult::IdentitiesBalances(balances) => { | ||
| // Get the first two balances (sender and recipient) | ||
| let mut iter = balances.into_values(); | ||
| let first_balance = iter.next(); | ||
| let second_balance = iter.next(); | ||
| TokenTransferResultWasm { | ||
| sender_balance: first_balance, | ||
| recipient_balance: second_balance, | ||
| group_power: None, | ||
| document: None, | ||
| } | ||
| } | ||
| TransferResult::HistoricalDocument(doc) => TokenTransferResultWasm { | ||
| sender_balance: None, | ||
| recipient_balance: None, | ||
| group_power: None, | ||
| document: Some(doc.into()), | ||
| }, | ||
| TransferResult::GroupActionWithDocument(power, doc) => TokenTransferResultWasm { | ||
| sender_balance: None, | ||
| recipient_balance: None, | ||
| group_power: Some(power as u32), | ||
| document: doc.map(|d| d.into()), | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
TransferResult::IdentitiesBalances conversion may incorrectly assign sender/recipient balances.
The conversion iterates over balances.into_values() and assigns the first value to sender_balance and the second to recipient_balance. However, BTreeMap::into_values() (or similar) iterates in key order, not insertion order. If the recipient's identifier sorts before the sender's, the balances will be swapped.
Consider returning both balances with their corresponding identifiers, or document that the order is based on identifier sorting rather than semantic sender/recipient roles.
🔎 Proposed fix - return identifiers with balances
#[wasm_bindgen(js_name = "TokenTransferResult")]
pub struct TokenTransferResultWasm {
- /// For IdentitiesBalances result - sender's new balance
- sender_balance: Option<u64>,
- /// For IdentitiesBalances result - recipient's new balance
- recipient_balance: Option<u64>,
+ /// For IdentitiesBalances result - first identity ID
+ first_identity_id: Option<IdentifierWasm>,
+ /// For IdentitiesBalances result - first identity's new balance
+ first_balance: Option<u64>,
+ /// For IdentitiesBalances result - second identity ID
+ second_identity_id: Option<IdentifierWasm>,
+ /// For IdentitiesBalances result - second identity's new balance
+ second_balance: Option<u64>,
// ... rest unchanged
}And update the From impl accordingly to preserve identity-balance associations.
Committable suggestion skipped: line range outside the PR's diff.
| export interface TokenFreezeOptions { | ||
| /** | ||
| * The ID of the data contract containing the token. | ||
| */ | ||
| dataContractId: Identifier; | ||
| /** | ||
| * The position of the token in the contract (0-indexed). | ||
| */ | ||
| tokenPosition: number; | ||
| /** | ||
| * The identity ID of the token authority performing the freeze. | ||
| */ | ||
| authorityId: Identifier; | ||
| /** | ||
| * The identity ID to freeze. | ||
| */ | ||
| frozenIdentityId: Identifier; | ||
| /** | ||
| * Optional public note for the freeze operation. | ||
| */ | ||
| publicNote?: string; | ||
| /** | ||
| * Signer containing the private key for the authority's authentication key. | ||
| * Use IdentitySigner to add the authentication key before calling. | ||
| */ | ||
| signer: IdentitySigner; | ||
| /** | ||
| * Optional group action info for group-managed token freezing. | ||
| * Use GroupStateTransitionInfoStatus.proposer() to propose a new group action, | ||
| * or GroupStateTransitionInfoStatus.otherSigner() to vote on an existing action. | ||
| */ | ||
| groupInfo?: GroupStateTransitionInfoStatus; | ||
| /** | ||
| * Optional settings for the broadcast operation. | ||
| * Includes retries, timeouts, userFeeIncrease, etc. | ||
| */ | ||
| settings?: PutSettings; | ||
| } | ||
| "#; |
There was a problem hiding this comment.
TypeScript interfaces for TokenFreezeOptions and TokenDestroyFrozenOptions are missing identityKey field.
Looking at lines 859-904 (TokenFreezeOptions) and 1313-1363 (TokenDestroyFrozenOptions), the identityKey field is not documented in the TypeScript interfaces, but the Rust code attempts to extract it (lines 1022-1024 and 1463-1465). This mismatch could cause runtime errors.
🔎 Proposed fix for TokenFreezeOptions
Add after line 889 in the TypeScript interface:
/**
* The identity public key to use for signing the transition.
*/
identityKey: IdentityPublicKey;Also applies to: 1313-1363
🤖 Prompt for AI Agents
In packages/wasm-sdk/src/state_transitions/token.rs around lines 859-904 and
1313-1363, the TypeScript interfaces TokenFreezeOptions and
TokenDestroyFrozenOptions are missing the identityKey field referenced by the
Rust extraction code; add a new documented property named identityKey of type
IdentityPublicKey to both interfaces (insert it near the signer field or where
identity-related fields are grouped) so the TypeScript definitions match the
Rust usage and prevent runtime mismatches.
| impl From<SetPriceResult> for TokenSetPriceResultWasm { | ||
| fn from(result: SetPriceResult) -> Self { | ||
| match result { | ||
| SetPriceResult::PricingSchedule(_owner_id, _schedule) => TokenSetPriceResultWasm { | ||
| group_power: None, | ||
| group_action_status: None, | ||
| document: None, | ||
| }, | ||
| SetPriceResult::HistoricalDocument(doc) => TokenSetPriceResultWasm { | ||
| group_power: None, | ||
| group_action_status: None, | ||
| document: Some(doc.into()), | ||
| }, | ||
| SetPriceResult::GroupActionWithDocument(power, doc) => TokenSetPriceResultWasm { | ||
| group_power: Some(power as u32), | ||
| group_action_status: None, | ||
| document: doc.map(|d| d.into()), | ||
| }, | ||
| SetPriceResult::GroupActionWithPricingSchedule(power, status, _schedule) => { | ||
| TokenSetPriceResultWasm { | ||
| group_power: Some(power as u32), | ||
| group_action_status: Some(format!("{:?}", status)), | ||
| document: None, | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
SetPriceResult::PricingSchedule variant discards useful information.
When the result is PricingSchedule(owner_id, schedule), both the owner ID and the pricing schedule are discarded. The WASM result returns None for all fields, which provides no useful information to the caller.
Consider exposing at least the owner_id or a boolean indicating success, so callers can distinguish this result variant from an error state.
🤖 Prompt for AI Agents
In packages/wasm-sdk/src/state_transitions/token.rs around lines 2079 to 2106,
the match arm for SetPriceResult::PricingSchedule currently returns all None and
discards owner_id and schedule; update that arm to preserve useful info by
populating at least one field: include the owner_id (e.g., group_action_status:
Some(owner_id.to_string())) or, if schedule is convertible to the WASM document
type, set document: Some(schedule.into()); ensure any necessary conversions are
available and adjust field choices to match TokenSetPriceResultWasm types so
callers can distinguish PricingSchedule from error cases.
| /** | ||
| * Convert hex string to Uint8Array | ||
| * @param {string} hex - Hex string | ||
| * @returns {Uint8Array} Byte array | ||
| */ | ||
| function hexToBytes(hex) { | ||
| const bytes = new Uint8Array(hex.length / 2); | ||
| for (let i = 0; i < hex.length; i += 2) { | ||
| bytes[i / 2] = parseInt(hex.substr(i, 2), 16); | ||
| } | ||
| return bytes; | ||
| } |
There was a problem hiding this comment.
hexToBytes function is defined but never used.
This helper function is not exported and not used within this file. Consider removing it or using it where applicable.
🔎 Suggested fix
Remove the unused function:
-/**
- * Convert hex string to Uint8Array
- * @param {string} hex - Hex string
- * @returns {Uint8Array} Byte array
- */
-function hexToBytes(hex) {
- const bytes = new Uint8Array(hex.length / 2);
- for (let i = 0; i < hex.length; i += 2) {
- bytes[i / 2] = parseInt(hex.substr(i, 2), 16);
- }
- return bytes;
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Convert hex string to Uint8Array | |
| * @param {string} hex - Hex string | |
| * @returns {Uint8Array} Byte array | |
| */ | |
| function hexToBytes(hex) { | |
| const bytes = new Uint8Array(hex.length / 2); | |
| for (let i = 0; i < hex.length; i += 2) { | |
| bytes[i / 2] = parseInt(hex.substr(i, 2), 16); | |
| } | |
| return bytes; | |
| } |
🤖 Prompt for AI Agents
In packages/wasm-sdk/tests/functional/fixtures/requiredTestData.mjs around lines
119 to 130, the helper function hexToBytes is defined but never used or
exported; remove the unused function definition to eliminate dead code, or if
it’s intended for reuse, export it (export function hexToBytes...) and update
callers to import and use it; choose removal if not required by tests to keep
the fixture file minimal.
| * Test identities: | ||
| * - Identity 1: 4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi | ||
| * - Identity 2: 8pkwN93v4wTWzzJAJkBCVPQJHMGwNa9TYfJUW24sAq11 | ||
| * - Identity 3: CktRuQ2mttgRGkXJtyksdKHjUdc2C4TgDzyB98oEzy8 | ||
| * |
There was a problem hiding this comment.
Identity 2 ID in comment doesn't match fixture.
The comment on line 12 shows 8pkwN93v4wTWzzJAJkBCVPQJHMGwNa9TYfJUW24sAq11 but the fixture in requiredTestData.mjs defines identityId2 as 8qbHbw2BbbTHBW1sbeqakYXVKRQM8Ne7pLK7m6CVfeR.
🔎 Suggested fix
* Test identities:
* - Identity 1: 4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi
-* - Identity 2: 8pkwN93v4wTWzzJAJkBCVPQJHMGwNa9TYfJUW24sAq11
+* - Identity 2: 8qbHbw2BbbTHBW1sbeqakYXVKRQM8Ne7pLK7m6CVfeR
* - Identity 3: CktRuQ2mttgRGkXJtyksdKHjUdc2C4TgDzyB98oEzy8
Issue being fixed or feature implemented
Currently, state transition methods accept many arguments instead of typed objects, which is a better UX.
What was done?
How Has This Been Tested?
With new functional and unit tests.
Enabled skiped tests.
Breaking Changes
Public API of Evo SDK is changed
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.