feat(sdk): add NFT actions in the JS Dash SDK #2444
Conversation
WalkthroughThis pull request updates the document state transition functionality across several modules. The changes introduce an optional parameter, encapsulated as Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DF as DocumentFactory/V0
participant WASM as DocumentFactoryWASM
participant Transition as Transition Handler
Client->>DF: create_state_transition(documents, nonce_counter, params)
alt Action Type: Transfer
DF->>Transition: document_transfer_transitions(documents, params, ...)
else Action Type: UpdatePrice
DF->>Transition: document_update_price_transitions(documents, params, ...)
else Action Type: Purchase
DF->>Transition: document_purchase_transitions(documents, params, ...)
else Other
DF->>DF: Process default transition
end
Client->>WASM: create_state_transition(documents, nonce_counter, recipient, price)
WASM->>DF: Forwards call with new parameters
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (18)
🔇 Additional comments (1)
✨ Finishing Touches
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 (
|
![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.
| recipient: Option<IdentifierWrapper>, | ||
| price: Option<Credits>, |
There was a problem hiding this comment.
This looks strange here, to be honest. For which exact document it applies? I think these params must be a part of document information
There was a problem hiding this comment.
For NFT operations, like transfer, updatePrice or purchase. Neither Document or ExtendedDocument have such fields
There was a problem hiding this comment.
Then we need to accept another structure that will contain document and params
There was a problem hiding this comment.
I just wondering why do we need document for transfer? We need just ID, isn't ?
There was a problem hiding this comment.
Then we need to accept another structure that will contain document and params
Should I revert back to this approach?
I just wondering why do we need document for transfer? We need just ID, isn't ?
Yes, it is possible, however there are no such methods in the rs-dpp yet (only from_document)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/js-dash-sdk/docs/platform/documents/broadcast.md (1)
1-1: Update method signature to include options parameter.The method signature in the usage section should be updated to reflect the new optional parameter.
-**Usage**: `client.platform.document.broadcast(documents, identity)` +**Usage**: `client.platform.document.broadcast(documents, identity, options)`Also applies to: 16-16
🧹 Nitpick comments (6)
packages/js-dash-sdk/docs/platform/documents/transfer.md (2)
4-11: Table Formatting: Replace Hard Tabs with Spaces
The parameter table is informative; however, static analysis indicates that hard tabs are used (see line 7). To adhere to markdownlint conventions, please replace any hard tabs with spaces to ensure consistent table formatting.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Hard tabs
Column: 50(MD010, no-hard-tabs)
29-31: Informative Note with Return Information
The note at the end effectively clarifies that the transfer transition alters document ownership. The return type ("DocumentsBatchTransition") is clearly stated. Optionally, you might expand on any side effects or subsequent steps for users after invoking the broadcast method.packages/rs-dpp/src/document/document_factory/v0/mod.rs (1)
702-702: Remove commented out code.The commented line
//document.set_owner_id(recipient.unwrap());should be removed if it's no longer needed.packages/js-dash-sdk/docs/platform/documents/updatePrice.md (2)
14-16: Add type annotations in example code.The example should include type annotations for better clarity:
-const identityId = '';// Your identity identifier -const documentId = '' // Your document id -const price = BigInt(1000000) +const identityId: string = '';// Your identity identifier +const documentId: string = '' // Your document id +const price: bigint = BigInt(1000000)
27-27: Clarify batch limitation in note.The note about batch limitation should be more specific about why only one price is possible and whether this is a temporary limitation.
-**Note**: This method sets the same price on all documents in the batch (only one is possible right now) +**Note**: This method sets the same price on all documents in the batch. Currently, the platform only supports setting one price per batch. This limitation may be removed in future versions.packages/js-dash-sdk/docs/platform/documents/broadcast.md (1)
31-32: Add examples for new NFT actions.The example section only shows document creation. Consider adding examples for transfer, updatePrice, and purchase actions.
Would you like me to generate example code for the new NFT actions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/js-dash-sdk/docs/platform/documents/broadcast.md(1 hunks)packages/js-dash-sdk/docs/platform/documents/purchase.md(1 hunks)packages/js-dash-sdk/docs/platform/documents/transfer.md(1 hunks)packages/js-dash-sdk/docs/platform/documents/updatePrice.md(1 hunks)packages/rs-dpp/src/data_contract/document_type/property/array.rs(1 hunks)packages/rs-dpp/src/data_contract/document_type/property/mod.rs(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/state_transition/state_transitions/document/documents_batch_transition/document_transition/action_type.rs(1 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/documents_batch_transition/document_transition/mod.rs(1 hunks)packages/rs-dpp/src/tests/fixtures/get_document_transitions_fixture.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/factory.rs(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/rs-dpp/src/state_transition/state_transitions/document/documents_batch_transition/document_transition/mod.rs
- packages/rs-dpp/src/data_contract/document_type/property/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/wasm-dpp/src/document/errors/mod.rs
- packages/rs-dpp/src/state_transition/state_transitions/document/documents_batch_transition/document_transition/action_type.rs
- packages/wasm-dpp/src/document/document_facade.rs
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/js-dash-sdk/docs/platform/documents/transfer.md
7-7: Hard tabs
Column: 50
(MD010, no-hard-tabs)
packages/js-dash-sdk/docs/platform/documents/purchase.md
7-7: Hard tabs
Column: 55
(MD010, no-hard-tabs)
packages/js-dash-sdk/docs/platform/documents/updatePrice.md
7-7: Hard tabs
Column: 58
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (13)
packages/js-dash-sdk/docs/platform/documents/transfer.md (2)
1-3: Clear and Concise Overview for API Usage
The "Usage" and "Description" sections clearly communicate how to invoke the new broadcast method for transferring documents. The instructions are straightforward and provide a good entry point for users.
12-27: Example Code Block is Well-Structured
The provided example clearly demonstrates how to retrieve identities/documents and subsequently call the broadcast method. This helps in understanding the flow of NFT document transfer. Consider, if possible, mentioning what the expected return (DocumentsBatchTransition) looks like or linking to further documentation on its structure.packages/rs-dpp/src/tests/fixtures/get_document_transitions_fixture.rs (1)
26-27: LGTM!The test fixture has been correctly updated to support the new NFT actions by adding optional parameters for recipient and price.
packages/rs-dpp/src/data_contract/document_type/property/array.rs (1)
72-119: LGTM!The changes consistently use the
to_*method naming pattern, improving code readability while maintaining the same functionality.🧰 Tools
🪛 GitHub Check: Rust packages (drive) / Linting
[failure] 72-72: mismatched types
error[E0308]: mismatched types
--> packages/rs-dpp/src/data_contract/document_type/property/array.rs:72:64
|
72 | pub fn encode_value_ref_with_size(&self, value: &Value) -> Result<Vec, ProtocolError> {
| -------------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expectedResult<Vec<u8>, ProtocolError>, found()
| |
| implicitly returns()as its body has no tail orreturnexpression
...
119 | };
| - help: remove this semicolon to return this value
|
= note: expected enumstd::result::Result<std::vec::Vec<u8>, errors::protocol_error::ProtocolError>
found unit type()🪛 GitHub Check: Rust packages (dpp) / Linting
[failure] 72-72: mismatched types
error[E0308]: mismatched types
--> packages/rs-dpp/src/data_contract/document_type/property/array.rs:72:64
|
72 | pub fn encode_value_ref_with_size(&self, value: &Value) -> Result<Vec, ProtocolError> {
| -------------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expectedResult<Vec<u8>, ProtocolError>, found()
| |
| implicitly returns()as its body has no tail orreturnexpression
...
119 | };
| - help: remove this semicolon to return this value
|
= note: expected enumstd::result::Result<std::vec::Vec<u8>, errors::protocol_error::ProtocolError>
found unit type()🪛 GitHub Check: Rust packages (dash-sdk) / Linting
[failure] 72-72: mismatched types
error[E0308]: mismatched types
--> packages/rs-dpp/src/data_contract/document_type/property/array.rs:72:64
|
72 | pub fn encode_value_ref_with_size(&self, value: &Value) -> Result<Vec, ProtocolError> {
| -------------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expectedResult<Vec<u8>, ProtocolError>, found()
| |
| implicitly returns()as its body has no tail orreturnexpression
...
119 | };
| - help: remove this semicolon to return this value
|
= note: expected enumstd::result::Result<std::vec::Vec<u8>, errors::protocol_error::ProtocolError>
found unit type()packages/rs-dpp/src/document/document_factory/mod.rs (1)
114-126: LGTM!The changes correctly implement the NFT actions by:
- Adding optional parameters for recipient and price
- Properly delegating to the versioned factory implementation
The implementation aligns well with the PR objectives for supporting NFT document transactions.
Also applies to: 127-131
packages/wasm-dpp/src/document/factory.rs (3)
119-120: Address the TODO comment about error handling.The comment indicates that error handling needs improvement. Consider implementing proper error handling instead of using unwrap.
Would you like me to help implement proper error handling for the nonce counter parsing?
289-292: LGTM! New NFT actions added.The implementation correctly adds support for the three NFT actions:
- transfer: For transferring documents between identities
- updatePrice: For setting document prices
- purchase: For purchasing documents
297-302: LGTM! Action mapping implemented correctly.The new NFT actions are properly mapped to their corresponding transition types in the documents_by_action hashmap.
packages/rs-dpp/src/document/document_factory/v0/mod.rs (4)
619-624: Avoid forced unwrap forrecipient_owner_idin Transfer transitions.Similar to previous review comments, using
unwrap()onrecipient_owner_idcan cause a panic. Return a descriptive error instead.- recipient_owner_id.unwrap(), + recipient_owner_id.ok_or_else(|| ProtocolError::GenericError( + "Recipient is required for a transfer transition".to_string() + ))?,
663-667: Avoid forced unwrap forpricein UpdatePrice transitions.Similar to previous review comments, using
unwrap()onpricecan cause a panic. Return a descriptive error instead.- price.unwrap(), + price.ok_or_else(|| ProtocolError::GenericError( + "Price is required for an update price transition".to_string() + ))?,
698-700: Avoid forced unwrap forrecipientin Purchase transitions.Using
unwrap()onrecipientcan cause a panic. Return a descriptive error instead.- entry((recipient.unwrap(), document_type.data_contract_id())) + entry((recipient.ok_or_else(|| ProtocolError::GenericError( + "Recipient is required for a purchase transition".to_string() + ))?, document_type.data_contract_id()))
706-710: Avoid forced unwrap forpricein Purchase transitions.Similar to previous review comments, using
unwrap()onpricecan cause a panic. Return a descriptive error instead.- price.unwrap(), + price.ok_or_else(|| ProtocolError::GenericError( + "Price is required for a purchase transition".to_string() + ))?,packages/js-dash-sdk/docs/platform/documents/purchase.md (1)
22-22:⚠️ Potential issueRemove duplicate identity initialization.
The identity is initialized twice in the example code.
-const identity = await client.platform.identities.get(identityId);Likely invalid or redundant comment.
| { where: [['$id', '==', documentId]] }, | ||
| ); | ||
|
|
||
| await dash.platform.documents.broadcast({ updatePrice: [document], }, identity, { price, receiver: receiverIdentity.getId() }); |
There was a problem hiding this comment.
Fix incorrect action type in example code.
The example uses updatePrice instead of purchase in the broadcast call.
-await dash.platform.documents.broadcast({ updatePrice: [document], }, identity, { price, receiver: receiverIdentity.getId() });
+await dash.platform.documents.broadcast({ purchase: [document], }, identity, { price, receiver: receiverIdentity.getId() });📝 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.
| await dash.platform.documents.broadcast({ updatePrice: [document], }, identity, { price, receiver: receiverIdentity.getId() }); | |
| await dash.platform.documents.broadcast({ purchase: [document], }, identity, { price, receiver: receiverIdentity.getId() }); |
# Conflicts: # .yarnrc.yml # packages/rs-dpp/src/document/document_factory/v0/mod.rs # packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition_action_type.rs # packages/rs-dpp/src/tests/fixtures/get_document_transitions_fixture.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/rs-dpp/src/document/document_factory/v0/mod.rs (4)
287-319:⚠️ Potential issueHandle new document transition types safely.
The logic for handling new document transition types looks good. However, there are several forced unwraps (
unwrap()) that could cause panics if the expected parameters are missing.Consider replacing the forced unwraps with proper error handling:
DocumentTransitionActionType::Transfer => Self::document_transfer_transitions( documents .into_iter() .map(|(document, document_params, document_type, _)| { - (document, document_params.unwrap(), document_type) + (document, document_params.ok_or_else(|| ProtocolError::GenericError( + "DocumentTransitionParams is required for transfer transitions".to_string() + ))?, document_type) }) .collect(), nonce_counter, platform_version, ),Apply similar changes to the UpdatePrice and Purchase sections as well.
642-643:⚠️ Potential issueAvoid forced unwrap for
receiverin Transfer transitions.Using
unwrap()will panic ifreceiverisNone. It would be better to return a descriptive error.Replace the forced unwrap:
- document_params.receiver.unwrap(), + document_params.receiver.ok_or_else(|| ProtocolError::GenericError( + "Recipient is required for a transfer transition".to_string() + ))?,
684-685:⚠️ Potential issueAvoid forced unwrap for
pricein UpdatePrice transitions.Using
unwrap()will panic ifpriceisNone. It would be better to return a descriptive error.Replace the forced unwrap:
- document_params.price.unwrap(), + document_params.price.ok_or_else(|| ProtocolError::GenericError( + "Price is required for an UpdatePrice transition".to_string() + ))?,
716-718:⚠️ Potential issueAvoid forced unwraps in Purchase transitions.
Both
receiverandpriceunwraps could panic if the values are missing.Replace the forced unwraps:
- document_params.receiver.unwrap(), + document_params.receiver.ok_or_else(|| ProtocolError::GenericError( + "Receiver is required for a Purchase transition".to_string() + ))?, // And later: - document_params.price.unwrap(), + document_params.price.ok_or_else(|| ProtocolError::GenericError( + "Price is required for a Purchase transition".to_string() + ))?,Also applies to: 727-728
🧹 Nitpick comments (8)
packages/js-dash-sdk/docs/platform/documents/transfer.md (2)
1-32: Well-documented transfer document functionalityThe documentation for the new NFT transfer feature is clear and provides a good example of usage. This aligns with the PR objectives of implementing document transfer functionality.
Fix the hard tab in the table header row 7:
-|-------------------|---------|------------------ |-----------------------------------------------------------------------| +|-------------------|---------|-------------------|-----------------------------------------------------------------------|🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Hard tabs
Column: 50(MD010, no-hard-tabs)
14-26: Consider enhancing the example with more contextWhile the example is functional, it would be more helpful to include comments explaining what each identity and document ID represents in an NFT transfer context.
-const identityId = '';// Your identity identifier -const receiverId = ''; // Receiver identity identifier -const documentId = '' // Your document id +// The identity that currently owns the NFT document +const identityId = ''; // Your identity identifier +// The identity that will receive the NFT document after transfer +const receiverId = ''; // Receiver identity identifier +// The ID of the NFT document being transferred +const documentId = ''; // Your document idpackages/js-dash-sdk/docs/platform/documents/purchase.md (2)
14-23: Fix duplicate identity initialization in example.There's a duplicate retrieval of the identity at lines 19 and 22.
const identityId = '';// Your identity identifier const receiverId = ''; // Receiver identity identifier const documentId = '' // Your document id const price = BigInt(1000000) const identity = await client.platform.identities.get(identityId); const receiverIdentity = await client.platform.identities.get(receiverId); -const identity = await client.platform.identities.get(identityId); - const [document] = await dash.platform.documents.get( 'helloWorldContract.note', { where: [['$id', '==', documentId]] }, );
31-31: Fix missing apostrophe in "seller identity".The possessive form should be used here.
-**Note**: This method will change the ownership of the document to your identity, and seller identity will be credited with the amount specified in the updatePrice deducted from your balance. +**Note**: This method will change the ownership of the document to your identity, and seller's identity will be credited with the amount specified in the updatePrice deducted from your balance.🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: It looks like there might be a possessive apostrophe missing. Consider inserting it.
Context: ...p of the document to your identity, and seller identity will be credited with the amou...(AI_EN_LECTOR_MISSING_NOUN_POSSESSIVE)
packages/js-dash-sdk/docs/platform/documents/updatePrice.md (2)
6-10: Fix hard tab character in parameter table.The markdown table contains a hard tab character that should be replaced with spaces for consistency.
| parameters | type | required | Description | -|------------------------|---------|------------------ |-------------------------------------------------------------------| +|------------------------|---------|-------------------|-------------------------------------------------------------------| | **documents.updatePrice** | ExtendedDocument[] | no | array of valid [created document](../documents/create.md) to update price | | **identity** | Identity | yes | A valid [registered identity](../identities/register.md) | | **options** | DocumentTransitionParams | no | An object with field `price` (BigInt) |🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Hard tabs
Column: 58(MD010, no-hard-tabs)
27-27: Consider clarifying the batch limitation.The note about setting the same price for all documents in a batch is useful, but could be more specific about current limitations.
-**Note**: This method sets the same price on all documents in the batch (only one is possible right now) +**Note**: This method sets the same price on all documents in the batch. Currently, the implementation only supports broadcasting one document at a time.packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts (2)
71-74: Improve validation for updatePrice operations.Similar to the transfer validation, this could be enhanced with a more descriptive error message and validation when price is provided without documents.
Consider applying these changes:
- if (documents.updatePrice?.length && documents.updatePrice - .some(({ params }) => !params?.price)) { - throw new Error('Price must be provided for UpdatePrice operation'); + if (documents.updatePrice?.length) { + if (documents.updatePrice.some(({ params }) => !params?.price)) { + throw new Error('Price must be provided for document price update operations'); + } + } else if (options?.price !== undefined) { + throw new Error('Price provided but no documents to update price'); }
123-128: Add purchase to final logging statement.The final logging statement includes counts for create, replace, delete, and transfer operations, but is missing updatePrice and purchase operations.
Apply this change to include all operation types in the final log:
this.logger.debug('[Document#broadcast] Broadcasted documents', { create: documents.create?.length || 0, replace: documents.replace?.length || 0, delete: documents.delete?.length || 0, transfer: documents.transfer?.length || 0, + updatePrice: documents.updatePrice?.length || 0, + purchase: documents.purchase?.length || 0, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/js-dash-sdk/docs/platform/documents/broadcast.md(1 hunks)packages/js-dash-sdk/docs/platform/documents/purchase.md(1 hunks)packages/js-dash-sdk/docs/platform/documents/transfer.md(1 hunks)packages/js-dash-sdk/docs/platform/documents/updatePrice.md(1 hunks)packages/js-dash-sdk/src/SDK/Client/Client.spec.ts(2 hunks)packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts(4 hunks)packages/js-dash-sdk/src/SDK/Client/Platform/methods/names/register.ts(2 hunks)packages/platform-test-suite/test/e2e/contacts.spec.js(6 hunks)packages/platform-test-suite/test/e2e/dpns.spec.js(1 hunks)packages/platform-test-suite/test/e2e/withdrawals.spec.js(4 hunks)packages/platform-test-suite/test/functional/platform/DataContract.spec.js(1 hunks)packages/platform-test-suite/test/functional/platform/Document.spec.js(8 hunks)packages/platform-test-suite/test/functional/platform/Identity.spec.js(2 hunks)packages/platform-test-suite/test/functional/platform/featureFlags.spec.js(1 hunks)packages/rs-dpp/src/document/document_factory/mod.rs(2 hunks)packages/rs-dpp/src/document/document_factory/v0/mod.rs(5 hunks)packages/rs-dpp/src/document/errors.rs(1 hunks)packages/rs-dpp/src/document/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-dpp/src/document/document_factory/mod.rs
- packages/rs-dpp/src/document/errors.rs
- packages/js-dash-sdk/docs/platform/documents/broadcast.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/js-dash-sdk/docs/platform/documents/transfer.md
7-7: Hard tabs
Column: 50
(MD010, no-hard-tabs)
packages/js-dash-sdk/docs/platform/documents/updatePrice.md
7-7: Hard tabs
Column: 58
(MD010, no-hard-tabs)
🪛 LanguageTool
packages/js-dash-sdk/docs/platform/documents/purchase.md
[uncategorized] ~31-~31: It looks like there might be a possessive apostrophe missing. Consider inserting it.
Context: ...p of the document to your identity, and seller identity will be credited with the amou...
(AI_EN_LECTOR_MISSING_NOUN_POSSESSIVE)
🔇 Additional comments (28)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/names/register.ts (2)
77-77: Document structure updated to support new NFT functionality.The document is now wrapped in an object with a
documentproperty. This change is consistent with the new SDK structure needed to support NFT operations like transfer, update price, and purchase documents.
102-102: Document structure updated to support new NFT functionality.Similar to line 77, document objects are now wrapped in a structure that supports NFT operations. The change is consistent with the broadcast method signature updates across the SDK.
packages/platform-test-suite/test/functional/platform/DataContract.spec.js (1)
209-210: Updated document structure in broadcast method.The document is now properly wrapped in an object with a
documentproperty to match the updated broadcast method signature. This change correctly aligns with the SDK modifications for NFT functionality.packages/platform-test-suite/test/e2e/dpns.spec.js (1)
263-264: Updated document structure in delete operation.The document deletion now uses the updated structure where documents are wrapped with a
documentproperty. This change is consistent with the NFT functionality improvements in the SDK.packages/platform-test-suite/test/e2e/contacts.spec.js (6)
148-149: Document creation updated to new structure format.The document creation is now using the updated structure required for NFT operations support. Documents are properly wrapped in an object with the
documentproperty.
247-248: Updated document structure for Alice's profile creation.Alice's profile creation correctly implements the new document structure format needed for NFT support in the SDK.
268-269: Document replacement structure updated.The replace operation now correctly uses the updated document structure, maintaining consistency with the SDK changes for NFT functionality.
302-303: Contact request document structure updated.Bob's contact request document creation follows the updated structure pattern, consistent with other changes in this PR.
327-328: Contact approval document structure updated.Alice's contact approval document creation correctly uses the updated document structure format.
347-348: Document deletion structure updated.The document deletion operation now properly utilizes the new structure format with the
documentproperty wrapper.packages/js-dash-sdk/src/SDK/Client/Client.spec.ts (2)
316-317: API format change for document broadcastingThe update wraps each document in an object structure with
documentandparamsproperties, supporting the new NFT functionality mentioned in the PR objectives.
335-336: API format change consistently appliedThis change follows the same pattern as line 316, ensuring consistent API usage throughout the test cases.
packages/platform-test-suite/test/functional/platform/featureFlags.spec.js (2)
106-107: Document format change for broadcast APIConsistent with changes in other files, documents are now wrapped in objects with a
documentproperty when passed tobroadcast.
111-112: Document format change consistently appliedSame change as line 106, ensuring consistency in how documents are passed to the broadcast method.
packages/rs-dpp/src/document/mod.rs (2)
63-67: New structure to support NFT operationsThe
DocumentTransitionParamsstructure provides the necessary fields to support the NFT operations mentioned in the PR objectives (transfer, update price, purchase).The structure is well-designed with:
receiver: Optional identifier for the transfer recipientprice: Optional value for price-related operationsThis implementation correctly uses Rust's
Optiontype for both fields, making them truly optional.
48-48: Appropriate import for Identifier typeThe added import for
Identifierfromplatform_valueis necessary for the newDocumentTransitionParamsstructure.packages/js-dash-sdk/docs/platform/documents/purchase.md (1)
29-29: LGTM! The broadcast call correctly uses the purchase action.The example correctly shows how to broadcast a purchase transaction with the document.
packages/platform-test-suite/test/functional/platform/Document.spec.js (1)
104-104: Updated document object structure in broadcast method calls.The document structure has been updated to use
{ document: documentObj }format, which aligns with the API changes for NFT-related operations described in the PR objectives.Also applies to: 137-137, 168-168, 187-187, 216-216, 265-265, 295-295, 371-371
packages/js-dash-sdk/docs/platform/documents/updatePrice.md (1)
25-25: LGTM! The broadcast call correctly implements updatePrice functionality.This example correctly demonstrates how to use the updatePrice functionality with the appropriate parameters.
packages/platform-test-suite/test/functional/platform/Identity.spec.js (1)
361-361: Updated document object structure in broadcast method calls.The document structure has been updated to use
{ document: documentObj }format, which aligns with the API changes for NFT-related operations described in the PR objectives.Also applies to: 448-448
packages/platform-test-suite/test/e2e/withdrawals.spec.js (4)
117-118: Format update consistent with new API structure.The change properly adapts to the new
DocumentSubmittableinterface where documents need to be wrapped with adocumentkey.
219-220: Format update consistent with new API structure.The change properly adapts to the new
DocumentSubmittableinterface where documents need to be wrapped with adocumentkey.
263-264: Format update consistent with new API structure.The change properly adapts to the new
DocumentSubmittableinterface where documents need to be wrapped with adocumentkey.
286-287: Format update consistent with new API structure.The change properly adapts to the new
DocumentSubmittableinterface where documents need to be wrapped with adocumentkey.packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts (3)
6-14: Well-structured interfaces for NFT functionality.The new interfaces clearly define the parameters needed for NFT operations, making the code more type-safe and self-documenting.
16-37: Updated documentation reflects new structure.The JSDoc has been properly updated to document the new parameter structure, making it clear how to use the expanded functionality.
66-69: Enhance error handling for transfer operations.While the basic recipient validation is good, the error handling could be more robust:
- The error message could be more descriptive
- Missing validation when recipient is provided but transfer array is empty
Consider applying these changes:
- if (documents.transfer?.length && documents.transfer - .some(({ params }) => !params?.receiver)) { - throw new Error('Receiver identity is not found for transfer transition'); + if (documents.transfer?.length) { + if (documents.transfer.some(({ params }) => !params?.receiver)) { + throw new Error('Recipient identity ID is required for document transfer operations'); + } + } else if (options?.recipient) { + throw new Error('Transfer recipient provided but no documents to transfer'); }packages/rs-dpp/src/document/document_factory/v0/mod.rs (1)
679-680: Consider ifset_transferred_atis appropriate for UpdatePrice.The
set_transferred_atmethod is called during price updates, but this may not be semantically correct as the document isn't being transferred, just having its price updated.Is setting
transferred_atintentional for price updates? This timestamp would typically indicate when ownership changed, not when price changed.
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: 1
♻️ Duplicate comments (3)
packages/rs-dpp/src/document/document_factory/v0/mod.rs (3)
641-642:⚠️ Potential issueAvoid forced unwrap for
recipientin Transfer transitions.Using
document_params.receiver.unwrap()will panic ifreceiverisNone. This could lead to runtime crashes if a transfer is attempted without specifying a recipient.- document_params.receiver.unwrap(), + document_params.receiver.ok_or_else(|| ProtocolError::GenericError( + "Recipient is required for a transfer transition".to_string() + ))?,
683-684:⚠️ Potential issueAvoid forced unwrap for
pricein UpdatePrice transitions.Using
document_params.price.unwrap()will panic ifpriceisNone. Return a more explicit error rather than allowing potential runtime crashes.- document_params.price.unwrap(), + document_params.price.ok_or_else(|| ProtocolError::GenericError( + "Price is required for an UpdatePrice transition".to_string() + ))?,
726-727:⚠️ Potential issueAvoid forced unwrap for
pricein Purchase transitions.Using
document_params.price.unwrap()will panic ifpriceisNone. Similar to other transitions, return an explicit error message.- document_params.price.unwrap(), + document_params.price.ok_or_else(|| ProtocolError::GenericError( + "Price is required for a purchase transition".to_string() + ))?,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-dpp/src/document/document_factory/v0/mod.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Formatting
- 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 (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 (3)
packages/rs-dpp/src/document/document_factory/v0/mod.rs (3)
608-652: The new Transfer transition functionality looks good.The implementation properly checks document transferability status before proceeding and handles document revision increments correctly. Just fix the unwrap issue noted above.
654-695: The new UpdatePrice transition functionality looks good.The implementation properly handles document revision increments and timestamp updates. Just fix the unwrap issue noted above.
697-738: The new Purchase transition functionality looks good.The implementation correctly increments document revision and updates timestamps. Just fix the unwrap issues noted above.
| document_params.receiver.unwrap(), | ||
| document_type.data_contract_id(), |
There was a problem hiding this comment.
Avoid forced unwrap for receiver in Purchase transitions.
Using document_params.receiver.unwrap() will panic if receiver is None when calculating the nonce.
- document_params.receiver.unwrap(),
+ document_params.receiver.ok_or_else(|| ProtocolError::GenericError(
+ "Recipient is required for a purchase transition".to_string()
+ ))?,📝 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.
| document_params.receiver.unwrap(), | |
| document_type.data_contract_id(), | |
| document_params.receiver.ok_or_else(|| ProtocolError::GenericError( | |
| "Recipient is required for a purchase transition".to_string() | |
| ))?, | |
| document_type.data_contract_id(), |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts (1)
123-128: 🛠️ Refactor suggestionInclude all new operations in final debug log.
The final debug log only includes the 'transfer' operation but not 'updatePrice' and 'purchase'.
Add the missing operations to the debug log:
this.logger.debug('[Document#broadcast] Broadcasted documents', { create: documents.create?.length || 0, replace: documents.replace?.length || 0, delete: documents.delete?.length || 0, transfer: documents.transfer?.length || 0, + updatePrice: documents.updatePrice?.length || 0, + purchase: documents.purchase?.length || 0, });
♻️ Duplicate comments (3)
packages/rs-dpp/src/document/document_factory/v0/mod.rs (3)
716-717:⚠️ Potential issueAvoid forced unwrap for receiver in Purchase transitions.
Using
document_params.receiver.unwrap()will panic ifreceiverisNone.Replace the unwrap with proper error handling:
- document_params.receiver.unwrap(), + document_params.receiver.ok_or_else(|| ProtocolError::GenericError( + "Receiver identity is required for a purchase transition".to_string() + ))?,
727-728:⚠️ Potential issueAvoid forced unwrap for price in Purchase transitions.
Using
document_params.price.unwrap()will panic ifpriceisNone.Replace the unwrap with proper error handling:
- document_params.price.unwrap(), + document_params.price.ok_or_else(|| ProtocolError::GenericError( + "Price is required for a purchase transition".to_string() + ))?,
684-685:⚠️ Potential issueAvoid forced unwrap for price in UpdatePrice transitions.
Using
document_params.price.unwrap()will panic ifpriceisNone.Replace the unwrap with proper error handling:
- document_params.price.unwrap(), + document_params.price.ok_or_else(|| ProtocolError::GenericError( + "Price is required for an update price transition".to_string() + ))?,
🧹 Nitpick comments (4)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts (3)
66-69: Improve type validation for receiver identity.The validation checks for the presence of the receiver but doesn't validate its type or format.
Consider enhancing the validation:
- if (documents.transfer?.length && documents.transfer - .some(({ params }) => !params?.receiver)) { - throw new Error('Receiver Identity is not found for Transfer transition'); - } + if (documents.transfer?.length) { + if (documents.transfer.some(({ params }) => !params?.receiver)) { + throw new Error('Receiver Identity is not found for Transfer transition'); + } + if (documents.transfer.some(({ params }) => params?.receiver && !(params.receiver instanceof Identifier))) { + throw new Error('Receiver must be a valid Identifier for Transfer transition'); + } + }
71-74: Add validation for price in UpdatePrice operations.The code checks if price is provided but doesn't validate that it's a positive value.
Consider adding validation for the price value:
- if (documents.updatePrice?.length && documents.updatePrice - .some(({ params }) => !params?.price)) { - throw new Error('Price must be provided for UpdatePrice operation'); - } + if (documents.updatePrice?.length) { + if (documents.updatePrice.some(({ params }) => !params?.price)) { + throw new Error('Price must be provided for UpdatePrice operation'); + } + if (documents.updatePrice.some(({ params }) => params?.price && params.price < 0n)) { + throw new Error('Price must be a non-negative value for UpdatePrice operation'); + } + }
76-83: Validate both receiver and price in Purchase operations.The validation is good, but could be structured more clearly with separate checks.
Consider restructuring for better readability:
if (documents.purchase?.length) { - if (documents.purchase - .some(({ params }) => !params?.price || !params?.receiver)) { - throw new Error('Receiver and Price must be provided for Purchase operation'); - } else { - documents.purchase.forEach(({ document, params }) => document.setOwnerId(params!.receiver)); - } + // Check required parameters + if (documents.purchase.some(({ params }) => !params?.price)) { + throw new Error('Price must be provided for Purchase operation'); + } + if (documents.purchase.some(({ params }) => !params?.receiver)) { + throw new Error('Receiver must be provided for Purchase operation'); + } + // Validate parameter types + if (documents.purchase.some(({ params }) => params?.price && params.price < 0n)) { + throw new Error('Price must be a non-negative value for Purchase operation'); + } + if (documents.purchase.some(({ params }) => params?.receiver && !(params.receiver instanceof Identifier))) { + throw new Error('Receiver must be a valid Identifier for Purchase operation'); + } + // Set owner ID for purchase documents + documents.purchase.forEach(({ document, params }) => document.setOwnerId(params!.receiver)); }packages/rs-dpp/src/document/document_factory/v0/mod.rs (1)
637-653: Add transaction validation to document_transfer_transitions method.The method should verify that the receiver is different from the current owner.
Consider adding validation to prevent transferring to the same owner:
let transition = DocumentTransferTransition::from_document( document, document_type, *nonce, - document_params.receiver.unwrap(), + { + let receiver = document_params.receiver.ok_or_else(|| ProtocolError::GenericError( + "Receiver identity is required for a transfer transition".to_string() + ))?; + // Prevent transferring to self + if receiver == document.owner_id() { + return Err(ProtocolError::GenericError( + "Cannot transfer document to the same owner".to_string() + )); + } + receiver + }, platform_version, None, None, )?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts(4 hunks)packages/rs-dpp/src/document/document_factory/v0/mod.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / 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: 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
- GitHub Check: Rust crates security audit
🔇 Additional comments (1)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts (1)
6-14: Well-defined TypeScript interfaces for document transitions.The interfaces
DocumentTransitionParamsandDocumentSubmittableare well-structured and provide proper type safety for the NFT functionality.
| (document, document_params.unwrap(), document_type) | ||
| }) |
There was a problem hiding this comment.
Avoid forced unwrap for document_params in UpdatePrice transitions.
Using document_params.unwrap() will panic if document_params is None.
Replace the unwrap with proper error handling:
- (document, document_params.unwrap(), document_type)
+ (document, document_params.ok_or_else(|| ProtocolError::GenericError(
+ "Document params are required for an update price transition".to_string()
+ ))?, document_type)📝 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.
| (document, document_params.unwrap(), document_type) | |
| }) | |
| (document, document_params.ok_or_else(|| ProtocolError::GenericError( | |
| "Document params are required for an update price transition".to_string() | |
| ))?, document_type) |
| (document, document_params.unwrap(), document_type) | ||
| }) |
There was a problem hiding this comment.
Avoid forced unwrap for document_params in Purchase transitions.
Using document_params.unwrap() will panic if document_params is None.
Replace the unwrap with proper error handling:
- (document, document_params.unwrap(), document_type)
+ (document, document_params.ok_or_else(|| ProtocolError::GenericError(
+ "Document params are required for a purchase transition".to_string()
+ ))?, document_type)📝 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.
| (document, document_params.unwrap(), document_type) | |
| }) | |
| (document, document_params.ok_or_else(|| ProtocolError::GenericError( | |
| "Document params are required for a purchase transition".to_string() | |
| ))?, document_type) |
| (document, document_params.unwrap(), document_type) | ||
| }) |
There was a problem hiding this comment.
Avoid forced unwrap for document_params in Transfer transitions.
Using document_params.unwrap() will panic if document_params is None.
Replace the unwrap with a proper error handling:
- (document, document_params.unwrap(), document_type)
+ (document, document_params.ok_or_else(|| ProtocolError::GenericError(
+ "Document params are required for a transfer transition".to_string()
+ ))?, document_type)📝 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.
| (document, document_params.unwrap(), document_type) | |
| }) | |
| (document, document_params.ok_or_else(|| ProtocolError::GenericError( | |
| "Document params are required for a transfer transition".to_string() | |
| ))?, document_type) |
| document_params.receiver.unwrap(), | ||
| platform_version, |
There was a problem hiding this comment.
Avoid forced unwrap for receiver identity in Transfer transitions.
Using document_params.receiver.unwrap() will panic if receiver is None.
Replace the unwrap with proper error handling:
- document_params.receiver.unwrap(),
+ document_params.receiver.ok_or_else(|| ProtocolError::GenericError(
+ "Receiver identity is required for a transfer transition".to_string()
+ ))?,📝 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.
| document_params.receiver.unwrap(), | |
| platform_version, | |
| document_params.receiver.ok_or_else(|| ProtocolError::GenericError( | |
| "Receiver identity is required for a transfer transition".to_string() | |
| ))?, | |
| platform_version, |
| @@ -208,33 +213,43 @@ impl DocumentFactoryV0 { | |||
| documents_iter: impl IntoIterator< | |||
| Item = ( | |||
| DocumentTransitionActionType, | |||
| Vec<(Document, DocumentTypeRef<'a>, Bytes32)>, | |||
| Vec<( | |||
| Document, | |||
There was a problem hiding this comment.
This is too complex tuple. Please introduce a structure
| @@ -47,3 +47,19 @@ impl TryFrom<&str> for DocumentTransitionActionType { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| impl From<DocumentTransitionActionType> for &str { | |||
There was a problem hiding this comment.
You better implement Display and then call to_string
| use crate::state_transition::batch_transition::accessors::DocumentsBatchTransitionAccessorsV0; | ||
| use crate::state_transition::batch_transition::batched_transition::BatchedTransition; | ||
| use crate::state_transition::batch_transition::batched_transition::document_transition_action_type::DocumentTransitionActionType; | ||
| pub fn get_batched_transitions_fixture<'a>( | ||
| documents: impl IntoIterator< | ||
| Item = ( | ||
| DocumentTransitionActionType, | ||
| Vec<(Document, DocumentTypeRef<'a>, Bytes32)>, | ||
| Vec<( |
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
Documentation
Tests