feat(sdk): add NFT actions in the JS Dash SDK #2444
Conversation
…/document-transfer
…/document-transfer
|
Warning Rate limit exceeded@pshenmic has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
WalkthroughThis pull request introduces comprehensive changes to document handling across multiple packages, focusing on enhancing document state transitions. The modifications enable new capabilities for document transfers, price updates, and purchases. Key changes include updating method signatures in document factories, adding new error handling for non-transferable documents, and extending WebAssembly (WASM) bindings to support these new document transition types. Changes
Possibly related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
# Conflicts: # packages/js-dash-sdk/src/SDK/Client/Platform/Platform.ts # packages/wasm-dpp/src/document/extended_document.rs
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/js-dash-sdk/src/SDK/Client/Platform/Platform.ts(3 hunks)packages/wasm-dpp/src/document/extended_document.rs(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/js-dash-sdk/src/SDK/Client/Platform/Platform.ts
[error] 64-64: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 65-65: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 66-66: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (2)
packages/js-dash-sdk/src/SDK/Client/Platform/Platform.ts (2)
12-14: LGTM! New document method imports are properly organized.The imports for NFT-related document methods are correctly added and follow the existing import pattern.
174-176: LGTM! Method bindings are correctly implemented.The new NFT-related methods are properly bound to the Platform instance in the constructor.
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
packages/wasm-dpp/src/document/factory.rs (1)
Line range hint
117-141: Consider improving nonce counter error handling.The TODO comment and unwrap usage in the nonce counter processing could lead to runtime panics. Consider implementing proper error handling for robustness.
- // TODO: move to a function and handle errors instead of doing unwraps - { - js_sys::Object::entries(nonce_counter_value) - .iter() - .for_each(|entry| { - let key_value = js_sys::Array::from(&entry); - let identity_id = identifier_from_js_value(&key_value.get(0)).unwrap(); + fn process_nonce_counter( + nonce_counter_value: &js_sys::Object, + nonce_counter: &mut BTreeMap<(Identifier, Identifier), u64> + ) -> Result<(), JsValue> { + for entry in js_sys::Object::entries(nonce_counter_value).iter() { + let key_value = js_sys::Array::from(&entry); + let identity_id = identifier_from_js_value(&key_value.get(0)) + .map_err(|_| JsValue::from_str("Invalid identity ID in nonce counter"))?;
🧹 Nitpick comments (11)
packages/rs-dpp/src/document/document_factory/v0/mod.rs (4)
35-36: Consider gating these new imports under the same feature flags.
Although these imports are needed for state transition operations, you may want to wrap them under#[cfg(feature = "state-transitions")]to avoid potential compilation issues when that feature isn’t enabled.
582-624: Consider incrementing revision or updatedAt when transferring documents.
Currently, the code does not increment revision or update any timestamps when transferring a document. If transfers logically constitute a mutation, you may wish to track them via the document’s revision or updatedAt fields, or clarify if it is intentional to leave them as is.
625-661: Add revision increment for price updates.
A document’s price is part of its data, implying a change that typically warrants incrementing revision or updating timestamps. Not accounting for it here could create confusion or conflicts.
662-698: Add revision increment for purchases or clarify immutability.
Purchasing a document might constitute an ownership or data change. Consider incrementing revision or marking updatedAt for clarity.packages/wasm-dpp/src/document/errors/trying_to_transfer_nontransferable_document_error.rs (1)
1-19: Improve error message phrasing and reduce stored data.
- Minor grammar: "Trying to transfer a non-transferable document" would be clearer.
- Consider storing only the document ID in the error (instead of the entire
DocumentWasm) unless you need the entire document. This can help minimize memory usage.packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/updatePrice.ts (2)
5-12: JSDoc block references “Transfer document” instead of “Update price.”
Update the JSDoc to accurately reflect that this function updates the price, not that it transfers a document.- /** - * Transfer document in the platform - * + /** + * Update price of the document in the platform + *
13-31: Rename debug message for clarity.
Although the function properly updates a document’s price, the debug log uses[Document#transfer]. Consider reflecting the method name for consistency.- this.logger.debug(`[Document#transfer] Update price for document ${document.getId().toString()} to ${amount}`); + this.logger.debug(`[Document#updatePrice] Update price for document ${document.getId().toString()} to ${amount}`);packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/transfer.ts (1)
13-18: Consider using a more specific return type.The function currently returns
Promise<any>. Consider defining a specific return type that represents the result of the state transition broadcast.- ): Promise<any> { + ): Promise<StateTransitionBroadcastResult> {packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/purchase.ts (1)
13-19: Consider using a more specific return type.The function currently returns
Promise<any>. Consider defining a specific return type that represents the result of the state transition broadcast.- ): Promise<any> { + ): Promise<StateTransitionBroadcastResult> {packages/wasm-dpp/src/document/state_transition/document_batch_transition/document_transition/document_transfer_transition.rs (2)
1-8: Add documentation for the WASM wrapper struct.Consider adding documentation comments to explain:
- The purpose of this WASM wrapper
- How it relates to NFT document transfers
- Usage examples from JavaScript
Add this documentation above the struct:
+/// WASM wrapper for DocumentTransferTransition to enable NFT document transfer operations +/// from JavaScript/TypeScript code. +/// +/// # Example (JavaScript) +/// ```javascript +/// const transfer = new DocumentTransferTransition(); +/// // ... configure transfer properties +/// ``` #[wasm_bindgen(js_name=DocumentTransferTransition)] #[derive(Debug, Clone)] pub struct DocumentTransferTransitionWasm {
10-20: Document conversion behavior and potential failure cases.The From implementations look correct, but consider documenting any potential edge cases or data loss scenarios that might occur during conversion.
Add documentation to both From implementations:
+/// Converts from the DPP DocumentTransferTransition to its WASM wrapper. +/// This conversion is infallible and preserves all data. impl From<DocumentTransferTransition> for DocumentTransferTransitionWasm { +/// Converts from the WASM wrapper back to the DPP DocumentTransferTransition. +/// This conversion is infallible and preserves all data. impl From<DocumentTransferTransitionWasm> for DocumentTransferTransition {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/purchase.ts(1 hunks)packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/transfer.ts(1 hunks)packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/updatePrice.ts(1 hunks)packages/rs-dpp/src/document/document_factory/mod.rs(2 hunks)packages/rs-dpp/src/document/document_factory/v0/mod.rs(4 hunks)packages/rs-dpp/src/document/errors.rs(1 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/documents_batch/transformer/v0/mod.rs(1 hunks)packages/wasm-dpp/src/document/document_facade.rs(2 hunks)packages/wasm-dpp/src/document/errors/mod.rs(3 hunks)packages/wasm-dpp/src/document/errors/trying_to_transfer_nontransferable_document_error.rs(1 hunks)packages/wasm-dpp/src/document/extended_document.rs(2 hunks)packages/wasm-dpp/src/document/factory.rs(4 hunks)packages/wasm-dpp/src/document/state_transition/document_batch_transition/document_transition/document_transfer_transition.rs(1 hunks)packages/wasm-dpp/src/document/state_transition/document_batch_transition/document_transition/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wasm-dpp/src/document/extended_document.rs
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Formatting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (14)
packages/rs-dpp/src/document/document_factory/v0/mod.rs (1)
214-216: Validate optional parameters better.
recipient.unwrap()andprice.unwrap()are used later but they are only assigned as optional here. It might be safer to explicitly match on the presence or absence of these parameters rather than unwrapping, to prevent runtime panics.packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/updatePrice.ts (2)
1-4: Imports are consistent and straightforward.
No issues found with these import statements.
33-33: Default export is appropriate.
Exporting this function as the default aligns with your naming conventions for other methods in the directory.packages/rs-dpp/src/document/errors.rs (1)
50-51: LGTM!The new error variant follows the existing pattern and provides clear error messaging.
packages/wasm-dpp/src/document/errors/mod.rs (1)
9-9: LGTM!The new error handling implementation follows the existing pattern and correctly integrates with the error handling system.
Also applies to: 36-36, 82-85
packages/wasm-dpp/src/document/document_facade.rs (2)
3-4: LGTM! New imports support NFT functionality.The addition of
CreditsandIdentifiertypes, along withIdentifierWrapper, provides the necessary types for handling NFT operations (prices and recipients).Also applies to: 9-9
100-102: LGTM! Method signature enhancement for NFT support.The
create_state_transitionmethod signature has been updated to include:
recipient: For document transfer operationsprice: For document pricing operationsThese changes align well with the PR objectives for NFT functionality.
packages/wasm-dpp/src/document/state_transition/document_batch_transition/document_transition/mod.rs (1)
2-2: LGTM! Added document transfer transition module.The addition of
document_transfer_transitionmodule aligns with the PR objectives by providing the necessary infrastructure for NFT transfer operations.packages/rs-dpp/src/document/document_factory/mod.rs (2)
21-21: LGTM! Added Credits type for NFT pricing.The addition of the
Creditsimport supports the price-related functionality for NFTs.
124-125: LGTM! Enhanced state transition creation for NFT support.The
create_state_transitionmethod has been updated to:
- Accept recipient information for transfers
- Handle price information for NFT operations
- Properly propagate these parameters to the V0 implementation
The changes maintain consistency with the WASM interface.
Also applies to: 128-128
packages/wasm-dpp/src/document/factory.rs (3)
20-20: LGTM! Added necessary imports for NFT support.The addition of
CreditsandIdentifierWrapperimports supports the new NFT functionality.Also applies to: 28-28
113-114: LGTM! Enhanced state transition creation with NFT parameters.The
create_state_transitionmethod now properly handles:
- Optional recipient for transfers
- Optional price for NFT operations
- Correct conversion of IdentifierWrapper to Identifier
Also applies to: 182-182
284-286: LGTM! Added NFT-specific document actions.Successfully implemented the three new document actions required by the PR objectives:
- transfer: For NFT ownership transfers
- updatePrice: For updating NFT prices
- purchase: For NFT purchases
The implementation follows the existing pattern for document actions.
Also applies to: 291-293
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/documents_batch/transformer/v0/mod.rs (1)
723-723: Improved error message clarity for revision mismatch.The change enhances error reporting by including the expected revision number instead of the actual revision. This makes it clearer to developers what revision number they should use to fix the error.
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/transfer.ts
Outdated
Show resolved
Hide resolved
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/transfer.ts
Outdated
Show resolved
Hide resolved
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/purchase.ts
Outdated
Show resolved
Hide resolved
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/purchase.ts
Outdated
Show resolved
Hide resolved
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/purchase.ts
Outdated
Show resolved
Hide resolved
...ate_transition/document_batch_transition/document_transition/document_transfer_transition.rs
Outdated
Show resolved
Hide resolved
...ate_transition/document_batch_transition/document_transition/document_transfer_transition.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Formatting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (2)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts (2)
32-32: LGTM! Logging changes are consistent.The debug logging has been properly updated to include transfer operations tracking.
Also applies to: 91-91
43-43: Verify integration with DPP state transition creation.The changes look correct, but let's verify the integration with the DPP package:
Also applies to: 61-61
✅ Verification successful
Transfer documents are properly integrated with DPP state transition creation
The implementation correctly:
- Includes transfer documents in the state transition array
- Validates recipient presence for transfer operations
- Follows consistent patterns with other document types (create, replace, delete)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the DPP package properly handles the new parameters # Check if the DPP document.createStateTransition method accepts recipient and price ast-grep --pattern 'createStateTransition($$$, $_, $recipient, $price)' # Check for any related tests rg -l "createStateTransition.*recipient.*price" -g "*.spec.ts" -g "*.test.ts"Length of output: 153
Script:
#!/bin/bash # Find state transition creation implementation with broader patterns ast-grep --pattern 'createStateTransition($_)' # Also search for files containing state transition creation rg -l "createStateTransition" --type ts # Look for document array handling rg "documents\." -A 5 -B 5 --type tsLength of output: 33146
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts
Outdated
Show resolved
Hide resolved
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/rs-dpp/src/document/document_factory/v0/mod.rs (1)
214-216: Consider documenting new parameters.The newly added parameters
recipientandpricewould benefit from doc comments clarifying their purpose and expected usage.packages/rs-dpp/src/state_transition/state_transitions/document/documents_batch_transition/document_transition/action_type.rs (1)
52-64: Ensure consistency for variant-to-string mappings.Some variants return uppercase version names (e.g.
"Create","Replace"), whereas the string keys intry_fromare lowercase. Consider standardizing for clarity.packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts (1)
6-10: Add doc comments forDocumentTransitionParams.Consider explaining the intended usage of
bigintforpriceversusnumber, to reduce confusion for callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts(3 hunks)packages/rs-dpp/src/document/document_factory/v0/mod.rs(4 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/documents_batch_transition/document_transition/action_type.rs(1 hunks)
🔇 Additional comments (18)
packages/rs-dpp/src/document/document_factory/v0/mod.rs (5)
35-36: Imports look consistent with new functionality.The newly added imports for
Creditsand the new transition types are aligned with the changes below.
269-297: Graceful handling of optional values is recommended.These lines invoke transition methods (transfer, update price, purchase) with optional
recipient/price. Ensure that cases where these options are missing trigger a safe and descriptive error rather than relying on usage in upstream code.
586-631: Avoid forced unwrap forrecipient_owner_id.Calling
recipient_owner_id.unwrap()inDocumentTransferTransition::from_documentcan trigger a panic ifrecipientisNone.
633-675: Avoid forced unwrap forpriceindocument_update_price_transitions.The code calls
price.unwrap()at line 663, which will panic ifpriceis absent.
677-718: Avoid forced unwrap for bothrecipientandpriceindocument_purchase_transitions.Lines 696 and 706 (
recipient.unwrap()andprice.unwrap()) risk panics. Also verify whether the commented-out ownership change (line 699) is intended.packages/rs-dpp/src/state_transition/state_transitions/document/documents_batch_transition/document_transition/action_type.rs (1)
42-43: String matching for new action types looks correct.Mapping
"updatePrice"and"purchase"aligns well with the newly introduced transitions.packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts (12)
1-1: Import statements appear valid.Importing
ExtendedDocumentandIdentifierfrom@dashevo/wasm-dppis consistent with the new NFT functionalities.
19-20: Documentation updated properly.The new JSDoc params for identity and options provide clarity for these arguments.
27-30: Additional operations recognized (transfer, updatePrice, purchase).Expanding the documents object to handle these new arrays is consistent with the underlying transitions.
33-33: Optional parameters integrated smoothly.Optional
optionsparameter properly referencesDocumentTransitionParams.
39-41: Enhanced logging to include new operations.Providing counts for transfer, update price, and purchase is helpful for debugging.
52-54: Merging new document types into a single list is coherent.Ensures consistent handling of all document operations in a unified flow.
61-63: Receiver check prevents runtime errors.Throwing an error when
documents.transferis non-empty butoptions?.receiveris missing protects from invalid usage.
65-67: Enforces requiredpricefor updatePrice.Ensures users cannot proceed without specifying the price for update operations.
69-75: Purchase operation validation is precise.Checking both
priceandreceiverfields ensures correct usage, and setting the new owner properly.
80-85: Correctly manages the identity contract nonce.Mapping the
identityContractNonceto the identity and contract ensures correct sequence for transitions.
86-91: State transition creation respects new parameters.
createStateTransitionpassingoptions?.receiverandoptions?.priceis aligned with the Rust side changes.
121-121: Logging of transfer documents is consistent with earlier logs.Matching the new operation logs fosters debugging consistency.
Issue being fixed or feature implemented
WASM DPP and JS Dash SDK is missing functions for creating NFT document transactions, there are 3 of them:
This PR implements all three missing functions making it able to perform such operations from the browsers (Web).
What was done?
tbd.
How Has This Been Tested?
In the testnet, with JS Dash SDK
https://testnet.platform-explorer.com/transaction/42175A63E25A316B5E0666317A3FF36A407EDDBA01D844982F68A6D10CBBFAF7
https://testnet.platform-explorer.com/transaction/36F3002686CA5ED23ED1FEFAD58803493055EFE55FC07517779AB267C8151747
https://testnet.platform-explorer.com/transaction/5CA156E676FA503CA4A520831484BE73B16EBE15599BB307605DE520B623AC1A
Breaking Changes
No
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor