-
Notifications
You must be signed in to change notification settings - Fork 44
fix(wasm-sdk): fix nft transitions #2751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Use dedicated price update transition method instead of generic replacement - Properly increment document revision for purchase and price updates - Create separate document instances with updated revisions for each operation - Fix transition creation to use appropriate specialized methods 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
When creating document transfer transitions, the code now properly increments the document's revision number before creating the transition. This ensures the transfer transition uses revision + 1 instead of the current revision, which is required for proper state transition validation on the platform. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
WalkthroughRefactors document transition flows to compute a next revision, construct a new DocumentV0 with revision = current_revision + 1, and pass that document into dedicated BatchTransition constructors for transfer, purchase, and price-update flows; removes in-place document mutations and entropy generation for price updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Docs as Document logic
participant Builder as StateTransitionBuilder
participant Batch as BatchTransition
rect rgba(200,235,255,0.18)
note over Caller,Docs: Transfer / Purchase / Set Price flows
Caller->>Docs: fetch current document
Docs->>Docs: get_next_revision(document)
Docs->>Docs: create new DocumentV0 (revision = current + 1)
Docs->>Builder: build transition from new document
alt Transfer
Builder->>Batch: new_document_transfer_transition_from_document(new_doc)
else Purchase
Builder->>Batch: new_document_purchase_transition_from_document(new_doc)
else Set Price
Builder->>Batch: new_document_update_price_transition_from_document(new_doc)
end
Batch-->>Caller: transition ready
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/wasm-sdk/src/state_transitions/documents/mod.rs (2)
1305-1318: Be explicit about Credits type for forward-compat
Passing u64 relies on Credits being a type alias; cast explicitly to avoid a future break if Credits becomes a newtype.Apply:
- price, + price as Credits,
968-987: DRY the “clone-and-bump revision” pattern
This block repeats across transfer, purchase, and price-set. Extract a helper to reduce duplication and ensure consistent safety checks.Example:
fn clone_with_bumped_revision(src: &Document) -> Result<Document, JsValue> { let cur = src.revision().ok_or_else(|| JsValue::from_str("Document revision is required"))?; let next = cur.checked_add(1).ok_or_else(|| JsValue::from_str("Document revision overflow"))?; Ok(Document::V0(DocumentV0 { id: src.id(), owner_id: src.owner_id(), properties: src.properties().clone(), revision: Some(next), created_at: src.created_at(), updated_at: src.updated_at(), transferred_at: src.transferred_at(), created_at_block_height: src.created_at_block_height(), updated_at_block_height: src.updated_at_block_height(), transferred_at_block_height: src.transferred_at_block_height(), created_at_core_block_height: src.created_at_core_block_height(), updated_at_core_block_height: src.updated_at_core_block_height(), transferred_at_core_block_height: src.transferred_at_core_block_height(), })) }Then call it where needed.
Also applies to: 1112-1131, 1273-1287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/wasm-sdk/src/state_transitions/documents/mod.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/wasm-sdk/src/**/*.rs
📄 CodeRabbit inference engine (packages/wasm-sdk/CLAUDE.md)
packages/wasm-sdk/src/**/*.rs: When implementing WASM SDK functionality, always refer to AI_REFERENCE.md first for accurate method signatures and examples.
For WASM builds, fix 'time not implemented on this platform' errors by using js_sys::Date::now().
Token functions are methods on WasmSdk, not standalone functions; avoid importing them as standalone.
The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.
Files:
packages/wasm-sdk/src/state_transitions/documents/mod.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:05.082Z
Learning: Documents and data contracts should use state transitions for updates
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: build-wasm-sdk
Replace unsafe revision defaulting with explicit error handling in document state transitions. Add helper method for safe revision increment with overflow protection. Affects transfer, purchase, delete, and price update operations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/wasm-sdk/src/state_transitions/documents/mod.rs (1)
1194-1196: Avoid unwrap_or(0) when reporting revision in resultEven for UI, defaulting to 0 can mask issues. Push the revision only if present.
Apply:
- additional_fields.push(("documentUpdated", JsValue::from_bool(true))); - additional_fields.push(("revision", JsValue::from_f64(doc.revision().unwrap_or(0) as f64))); + additional_fields.push(("documentUpdated", JsValue::from_bool(true))); + if let Some(rev) = doc.revision() { + additional_fields.push(("revision", JsValue::from_f64(rev as f64))); + }
🧹 Nitpick comments (3)
packages/wasm-sdk/src/state_transitions/documents/mod.rs (3)
188-196: Safe revision bump helper is solid; minor nit on error contextCentralizing the checked increment is great. Consider adding optional operation context (e.g., "transfer"/"purchase"/"price update") to the error to aid debugging, or let callers augment it.
1316-1321: Nit: pass matching_key by reference consistentlyElsewhere you pass a borrowed key directly (matching_key). Here you pass &matching_key (an extra reference). Both compile due to auto-deref, but consistency helps.
Apply:
- price, - &matching_key, + price, + matching_key,
554-571: Optional: guard replace-path increment with checked_add for paritydocument_replace still does
revision + 1. For parity with other flows and to avoid silent overflow in release builds, use checked_add.Apply:
- // Create the document using the DocumentV0 constructor - let platform_version = sdk.version(); + // Create the document using the DocumentV0 constructor + let platform_version = sdk.version(); + let next_revision = revision + .checked_add(1) + .ok_or_else(|| JsValue::from_str("Document revision overflow"))?; let document = Document::V0(DocumentV0 { id: doc_id, owner_id: owner_identifier, properties: document_data_platform_value .into_btree_string_map() .map_err(|e| JsValue::from_str(&format!("Failed to convert document data: {}", e)))?, - revision: Some(revision + 1), + revision: Some(next_revision), created_at: None, updated_at: None, transferred_at: None,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/wasm-sdk/src/state_transitions/documents/mod.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/wasm-sdk/src/**/*.rs
📄 CodeRabbit inference engine (packages/wasm-sdk/CLAUDE.md)
packages/wasm-sdk/src/**/*.rs: When implementing WASM SDK functionality, always refer to AI_REFERENCE.md first for accurate method signatures and examples.
For WASM builds, fix 'time not implemented on this platform' errors by using js_sys::Date::now().
Token functions are methods on WasmSdk, not standalone functions; avoid importing them as standalone.
The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.
Files:
packages/wasm-sdk/src/state_transitions/documents/mod.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:05.082Z
Learning: Documents and data contracts should use state transitions for updates
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: build-wasm-sdk
🔇 Additional comments (6)
packages/wasm-sdk/src/state_transitions/documents/mod.rs (6)
847-849: Delete uses actual current revision (no silent default)Correct to require Some(revision) without incrementing for deletion transitions.
978-996: Transfer: correct use of next revision and derived docGood: next_revision via helper and passing a cloned properties map. Please confirm that any sale-related fields (like $price) are handled by the transfer transition (cleared if needed) to avoid stale metadata on the new owner.
1015-1017: Transfer transition now uses revised documentLooks correct and consistent with the API.
1122-1140: Purchase: safe bump + derived doc — verify platform-side field updatesGood: next_revision and keeping owner_id as the current owner while the transition applies the change. Please verify DPP clears $price and updates ownership in the transition to prevent property drift.
1159-1161: Purchase transition call uses revised documentLooks good.
1279-1297: Price update: consistent safe bump and derived docGood refactor toward dedicated update flow using a new document with incremented revision.
Issue being fixed or feature implemented
Fixes issues with document state transitions in the WASM SDK where document revisions were not being properly incremented for transfer, purchase, and price update operations, causing validation failures on the platform.
What was done?
update_pricemethod instead of generic replacementThere may be a better way to do this, but it's how I was able to get wasm-sdk to produce transitions that actually work.
How Has This Been Tested?
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
Refactor