feat(wasm-sdk): add prepare_* APIs for idempotent document state transitions#3091
feat(wasm-sdk): add prepare_* APIs for idempotent document state transitions#3091thepastaclaw wants to merge 7 commits intodashpay:v3.1-devfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a two‑phase Prepare+Execute API to the WASM SDK for document state transitions by introducing prepare_document_create, prepare_document_replace, and prepare_document_delete which build, sign, and return signed StateTransition objects without broadcasting for idempotent retries. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant SDK as WASM SDK
participant Signer as Signer
participant Platform as Platform
App->>SDK: prepare_document_create(document, identityKey, signer, options)
SDK->>SDK: build_document_create_or_replace_transition(document, entropy?, ...)
SDK->>Signer: sign(stateTransition)
Signer-->>SDK: signed StateTransition
SDK-->>App: return signed StateTransition
App->>SDK: broadcastStateTransition(signedST)
SDK->>Platform: submit signed ST
Platform-->>SDK: accept/confirm
SDK-->>App: broadcast acknowledgment
App->>SDK: waitForResponse(signedST)
SDK->>Platform: query ST status
Platform-->>SDK: confirmed/result
SDK-->>App: final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/wasm-sdk/src/state_transitions/document.rs (2)
465-521: Consider extracting shared option-parsing logic to reduce duplication.
prepare_document_create(lines 469–506) duplicates nearly all of the extraction logic fromdocument_create(lines 111–147): document, entropy, identity key, signer, contract fetch, document type, and settings. The same pattern applies toprepare_document_replacevsdocument_replace, andprepare_document_deletevsdocument_delete.A private helper (e.g.,
extract_create_options(options) → (Document, [u8;32], IdentityPublicKey, Signer, DataContract, DocumentType, Option<PutSettings>)) for each operation variant would let both the all-in-one and prepare methods share the parsing/validation code, reducing the surface area for divergence bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/document.rs` around lines 465 - 521, Multiple methods duplicate option parsing/validation (prepare_document_create vs document_create and similar prepare_/document_ pairs); extract the repeated logic into a private helper (e.g., extract_document_create_options) that performs DocumentWasm::try_from_options + Document conversion, entropy validation and conversion to [u8;32], IdentityPublicKeyWasm::try_from_options -> IdentityPublicKey, IdentitySignerWasm::try_from_options (or signer wrapper), calls self.get_or_fetch_contract(contract_id).await, resolves document_type via get_document_type, and parses settings via try_from_options_optional; have both prepare_document_create and document_create call this helper and return the tuple (Document, [u8;32], IdentityPublicKey, IdentitySignerWasm/Signer, DataContract, DocumentType, Option<PutSettingsInput/Into>) to eliminate duplication and keep behavior identical.
1121-1122: Minor: prefermap_oroveris_some()+unwrap()for the revision check.The double-call to
document.revision()with anunwrap()is safe (guarded byis_some()), but a more idiomatic pattern avoids the rawunwrap():♻️ Suggested simplification
- let transition = if document.revision().is_some() - && document.revision().unwrap() != INITIAL_REVISION - { + let transition = if document + .revision() + .map_or(false, |rev| rev != INITIAL_REVISION) + {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/document.rs` around lines 1121 - 1122, Replace the is_some() + unwrap() pattern when building the transition with a single map_or call on document.revision(): compute the boolean condition as document.revision().map_or(false, |r| r != INITIAL_REVISION) (or equivalent map_or_else) and use that in the if that assigns transition so you no longer call revision() twice or unwrap; update the branch that currently reads the revision check to use this mapped result (refer to document.revision() and INITIAL_REVISION in the transition assignment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/wasm-sdk/src/state_transitions/document.rs`:
- Around line 465-521: Multiple methods duplicate option parsing/validation
(prepare_document_create vs document_create and similar prepare_/document_
pairs); extract the repeated logic into a private helper (e.g.,
extract_document_create_options) that performs DocumentWasm::try_from_options +
Document conversion, entropy validation and conversion to [u8;32],
IdentityPublicKeyWasm::try_from_options -> IdentityPublicKey,
IdentitySignerWasm::try_from_options (or signer wrapper), calls
self.get_or_fetch_contract(contract_id).await, resolves document_type via
get_document_type, and parses settings via try_from_options_optional; have both
prepare_document_create and document_create call this helper and return the
tuple (Document, [u8;32], IdentityPublicKey, IdentitySignerWasm/Signer,
DataContract, DocumentType, Option<PutSettingsInput/Into>) to eliminate
duplication and keep behavior identical.
- Around line 1121-1122: Replace the is_some() + unwrap() pattern when building
the transition with a single map_or call on document.revision(): compute the
boolean condition as document.revision().map_or(false, |r| r !=
INITIAL_REVISION) (or equivalent map_or_else) and use that in the if that
assigns transition so you no longer call revision() twice or unwrap; update the
branch that currently reads the revision check to use this mapped result (refer
to document.revision() and INITIAL_REVISION in the transition assignment).
shumkov
left a comment
shumkov
left a comment
There was a problem hiding this comment.
please provide a snippet of code which doesn't work
|
@shumkov — here's the code snippet showing what doesn't work: The Problem// Current API: documentCreate() is atomic (nonce bump + sign + broadcast + wait)
try {
await sdk.documents.create({ document, identityKey, signer });
} catch (err) {
if (isTimeoutError(err)) {
// The ST was broadcast, but waitForResponse timed out (504 from DAPI gateway).
// Did it land on Platform? We don't know.
//
// Our only option is to retry — but documentCreate() will:
// 1. Fetch a NEW nonce (old nonce + 1)
// 2. Build a NEW StateTransition with different bytes
// 3. Sign and broadcast this NEW ST
//
// If the first ST DID land, we now have TWO documents (double post).
// There is no way to rebroadcast the original ST because
// documentCreate() never exposes it to the caller.
await sdk.documents.create({ document, identityKey, signer }); // DUPLICATE
}
}This is the exact bug PastaPastaPasta/yappr#260 hit in production — DAPI gateway 504s caused double-posting. The workaround was ~200 lines of manual ST construction: // What the app had to do: manually build the full ST chain
const createTransition = new DocumentCreateTransition(document, nonce + 1n, null, null);
const batched = new BatchedTransition(createTransition.toDocumentTransition());
const batchTransition = BatchTransition.fromBatchedTransitions([batched], ownerId, 0);
const st = batchTransition.toStateTransition();
st.setIdentityContractNonce(nonce + 1n);
st.sign(PrivateKey.fromWIF(wif), identityKey);
// Cache bytes before broadcasting for safe retry
const stBytes = st.toBytes();
localStorage.setItem(cacheKey, base64Encode(stBytes));
await sdk.wasm.broadcastStateTransition(st);
await sdk.wasm.waitForResponse(st);
// On timeout: reload cached bytes, rebroadcast SAME ST (idempotent)
const cached = StateTransition.fromBytes(localStorage.getItem(cacheKey));
await sdk.wasm.broadcastStateTransition(cached);With the const st = await sdk.prepareDocumentCreate({ document, identityKey, signer });
const stBytes = st.toBytes(); // cache for retry safety
await sdk.broadcastStateTransition(st);
await sdk.waitForResponse(st);
// On timeout: StateTransition.fromBytes(stBytes) → rebroadcast |
|
@shumkov Here's the concrete problem — the high-level // Current API — double-post on timeout
try {
const result = await sdk.documentCreate({
document, identityKey, signer,
settings: { timeoutMs: 30_000 }
});
} catch (e) {
// If this was a 504 timeout, did the ST land on Platform?
// We don't know, and we can't rebroadcast the same ST.
//
// Retrying calls documentCreate() again, which:
// 1. Fetches a NEW nonce (bumped)
// 2. Builds a NEW StateTransition
// 3. Signs and broadcasts it
// → If the first one DID land, we now have a DUPLICATE document.
const result2 = await sdk.documentCreate({
document, identityKey, signer,
settings: { timeoutMs: 30_000 }
});
}This actively causes double-posting in production (see yappr#260 for the ~200-line manual workaround). With // New two-phase API — idempotent retry
const st = await sdk.prepareDocumentCreate({
document, identityKey, signer
});
const stBytes = st.toBytes(); // cache for retry safety
try {
await sdk.broadcastStateTransition(st);
await sdk.waitForResponse(st);
} catch (e) {
if (isTimeout(e)) {
// Rebroadcast the IDENTICAL ST — same nonce, same signature
// Platform will either accept it (first didn't land) or
// reject as duplicate (first succeeded). No double-post possible.
const cached = StateTransition.fromBytes(stBytes);
await sdk.broadcastStateTransition(cached);
await sdk.waitForResponse(cached);
}
}The key difference: 🤖 This was generated by an automated review bot. |
|
Addressed the For the option-parsing duplication extraction: I agree there's shared logic between the 🤖 This was generated by an automated review bot. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/wasm-sdk/src/state_transitions/document.rs (2)
1108-1108: Move the inlineusedeclaration to module-level importsPlacing
useinside a function body is valid Rust but non-idiomatic and inconsistent with the rest of the file where all trait imports are at the top.♻️ Proposed fix
-async fn build_document_create_or_replace_transition(...) ... { - use dash_sdk::dpp::data_contract::document_type::accessors::DocumentTypeV0Getters; - - let new_identity_contract_nonce = ...Move to the module-level import block (near the other
use dash_sdk::dpp::data_contract::...imports):+use dash_sdk::dpp::data_contract::document_type::accessors::DocumentTypeV0Getters;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/document.rs` at line 1108, The inline use statement for the trait DocumentTypeV0Getters should be moved from inside the function body to the module-level import block alongside the other dash_sdk::dpp::data_contract::... imports; update the top-of-file use imports to include use dash_sdk::dpp::data_contract::document_type::accessors::DocumentTypeV0Getters and remove the inline declaration so trait methods resolve consistently and match the file's import style.
465-521: Consider extracting shared option-parsing logic to eliminate duplication across prepare/non-prepare variants
prepare_document_create(lines 470–506) anddocument_create(lines 111–147) contain identical blocks for: document extraction, entropy validation, identity-key extraction, signer extraction, contract fetch, document-type lookup, and settings extraction. Same duplication exists betweendocument_replace/prepare_document_replaceanddocument_delete/prepare_document_delete. Extracting these into small helpers (e.g.parse_document_create_opts,parse_delete_document_spec) would make the diverging parts (broadcast vs. return ST) obvious and reduce maintenance surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/document.rs` around lines 465 - 521, Extract the duplicated option-parsing and validation logic into small helper functions and call them from both prepare and non-prepare variants; e.g., add a helper parse_document_create_opts that accepts PrepareDocumentCreateOptionsJs (or the shared options type) and returns the parsed Document (or DocumentWasm), a 32-byte entropy array, IdentityPublicKey, IdentitySignerWasm (or signer), the fetched DataContract, the DocumentType, and optional PutSettingsInput/converted settings; then replace the duplicated blocks in prepare_document_create and document_create to call parse_document_create_opts and use its results (similarly introduce parse_document_replace_opts and parse_document_delete_opts and use them from prepare_document_replace/document_replace and prepare_document_delete/document_delete) so the prepare* functions only differ by building/returning the state transition while non-prepare variants handle broadcasting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/wasm-sdk/src/state_transitions/document.rs`:
- Around line 564-607: prepare_document_replace currently delegates to
build_document_create_or_replace_transition which will treat a Document with
revision == None or INITIAL_REVISION as a create (mutating ID/entropy); add an
explicit guard in prepare_document_replace that reads the Document's revision
(from Document or DocumentWasm) and returns a WasmSdkError (or appropriate error
variant) if revision is None or equals INITIAL_REVISION, so only documents with
a non‑initial revision are allowed to proceed to
build_document_create_or_replace_transition; reference prepare_document_replace,
Document/DocumentWasm.revision(), INITIAL_REVISION, and
build_document_create_or_replace_transition when adding the check and error
return.
---
Nitpick comments:
In `@packages/wasm-sdk/src/state_transitions/document.rs`:
- Line 1108: The inline use statement for the trait DocumentTypeV0Getters should
be moved from inside the function body to the module-level import block
alongside the other dash_sdk::dpp::data_contract::... imports; update the
top-of-file use imports to include use
dash_sdk::dpp::data_contract::document_type::accessors::DocumentTypeV0Getters
and remove the inline declaration so trait methods resolve consistently and
match the file's import style.
- Around line 465-521: Extract the duplicated option-parsing and validation
logic into small helper functions and call them from both prepare and
non-prepare variants; e.g., add a helper parse_document_create_opts that accepts
PrepareDocumentCreateOptionsJs (or the shared options type) and returns the
parsed Document (or DocumentWasm), a 32-byte entropy array, IdentityPublicKey,
IdentitySignerWasm (or signer), the fetched DataContract, the DocumentType, and
optional PutSettingsInput/converted settings; then replace the duplicated blocks
in prepare_document_create and document_create to call
parse_document_create_opts and use its results (similarly introduce
parse_document_replace_opts and parse_document_delete_opts and use them from
prepare_document_replace/document_replace and
prepare_document_delete/document_delete) so the prepare* functions only differ
by building/returning the state transition while non-prepare variants handle
broadcasting.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/wasm-sdk/src/state_transitions/document.rs (1)
466-523: Substantial duplication betweenprepare_document_create/prepare_document_deleteand their non-prepare counterpartsThe option-extraction preamble (document, entropy, identity_key, signer, contract_id, document_type_name, data_contract, document_type, settings) is ~40 lines repeated verbatim in each prepare/non-prepare pair. Similarly
prepare_document_deleteanddocument_deleteshare identical document-field extraction and builder construction.A small private helper that returns the extracted options as a struct (or tuple) would eliminate the duplication and reduce the maintenance surface. The PR comments note this as a follow-up, but it's worth tracking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/document.rs` around lines 466 - 523, The prepare/document functions duplicate option extraction and validation; create a small private helper (e.g., extract_document_options or DocumentOptions) that, given &self and the PrepareDocumentCreateOptionsJs (or generic options), performs: DocumentWasm::try_from_options -> Document, extract and validate entropy into a [u8;32], IdentityPublicKeyWasm::try_from_options -> IdentityPublicKey, IdentitySignerWasm::try_from_options -> signer, derive contract_id and document_type_name, fetch data_contract via self.get_or_fetch_contract(contract_id).await, resolve document_type via get_document_type(&data_contract, &document_type_name)?, and parse optional settings; return a struct with fields (document, document_type, entropy_array, identity_key, signer, data_contract, settings) and update prepare_document_create (and prepare_document_delete/document_create/document_delete) to call this helper and use its returned values when building the state transition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/wasm-sdk/src/state_transitions/document.rs`:
- Around line 466-523: The prepare/document functions duplicate option
extraction and validation; create a small private helper (e.g.,
extract_document_options or DocumentOptions) that, given &self and the
PrepareDocumentCreateOptionsJs (or generic options), performs:
DocumentWasm::try_from_options -> Document, extract and validate entropy into a
[u8;32], IdentityPublicKeyWasm::try_from_options -> IdentityPublicKey,
IdentitySignerWasm::try_from_options -> signer, derive contract_id and
document_type_name, fetch data_contract via
self.get_or_fetch_contract(contract_id).await, resolve document_type via
get_document_type(&data_contract, &document_type_name)?, and parse optional
settings; return a struct with fields (document, document_type, entropy_array,
identity_key, signer, data_contract, settings) and update
prepare_document_create (and
prepare_document_delete/document_create/document_delete) to call this helper and
use its returned values when building the state transition.
|
@thepastaclaw @PastaPastaPasta it's already retring on timeouts. you can configure retries and timeouts passing settings platform/packages/wasm-sdk/src/settings.rs Line 192 in 3f49766 |
|
Here's the problem scenario: // Current API — atomic, no way to retry
try {
await sdk.documentCreate(document, identity, documentType, entropy);
} catch (e) {
if (e.message.includes("timeout")) {
// Cannot retry! Calling documentCreate again would:
// 1. Fetch a new nonce (incremented from the first attempt)
// 2. Build a NEW state transition with different nonce
// 3. The original ST might still be in mempool — now we have a conflict
//
// We also cannot rebroadcast the original ST because
// we never had access to it.
}
}
// With prepare_* API — caller controls retry
const signedST = await sdk.prepareDocumentCreate(document, identity, documentType, entropy);
const txid = signedST.transactionId();
try {
await sdk.broadcastAndWait(signedST);
} catch (e) {
if (e.message.includes("timeout")) {
// Safe to rebroadcast the SAME signed ST
await sdk.broadcastAndWait(signedST);
// Or check if the original went through:
// await sdk.waitForStateTransitionResult(txid);
}
}The core issue is that |
|
Ah, I see — the SDK already has built-in retry logic with configurable The remaining question would be whether there are cases where the caller needs the signed ST for reasons beyond retry (e.g., storing the txid for tracking before broadcast, or broadcasting through a different channel). But if the existing retry mechanism covers the practical use cases thephez was hitting, this PR may not be needed. @PastaPastaPasta @thephez — does the existing retry config in |
|
@shumkov — I looked at the retry mechanism you linked ( But it doesn't cover the case this PR addresses: application-level failure recovery. Consider this scenario:
The SDK's built-in retry can't help here because the retry loop is inside the same With
This is a standard two-phase pattern (prepare + commit) that any app dealing with unreliable networks needs. The SDK's retry covers the happy path; I'm keeping this PR open — it solves a real problem that |
|
I think in my usage, the timeout that bites us is the |
This isn't very consistent with what I've seen in using the sdk in yappr; is the default configuration setup to retry? |
|
Re: your questions about retry behavior: Does Default config: What's actually retried: The retry loop in But — if the So if you're hitting timeouts in yappr where the wait never completes even with retries, it might be that the timeout per-attempt is too short, or the retries are exhausted. The This is exactly why I think |
|
Re: your questions about retry/timeout behavior: Does PutSettingsInput affect wait_for_response? Are retries enabled by default? The real gap (which this PR was trying to address) is that even with retries configured, the retry happens at the DAPI request level — if the broadcast succeeds but |
thepastaclaw
force-pushed
the
feat/sdk-prepare-document-apis
branch
from
67793d7 to
a4ad291
Compare
Claw please back this up with code references @shumkov if this is the case, this is a big problem for clients like yappr, and may have been the cause of many of our double posting issues. We also had some issues where the sdk itself would just timeout / fail, and then user would click post again, and then it turns out that the first one worked. IDK how this case was happening if sdk had retries as shown here... (claw maybe look if yappr needs to manually configure in sdk options this retrying? or maybe we are setting some other option and that is clobbering it or something? don't look in latest yappr, look a bit further back in history before our hacky fix for double posts) |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/wasm-sdk/src/state_transitions/document.rs (3)
465-465: Register newprepare*methods inapi-definitions.jsonand regenerate docs.The three new
#[wasm_bindgen]exports (prepareDocumentCreate,prepareDocumentReplace,prepareDocumentDelete) are now part of the public JS/TS API surface but won't appear in the generateddocs.htmlorAI_REFERENCE.mdunless they're added toapi-definitions.jsonand docs are regenerated withpython3 generate_docs.py. Based on learnings, both files are auto-generated from that JSON config and manual edits would be overwritten.Based on learnings: "In
packages/wasm-sdk/, theAI_REFERENCE.mdfile is auto-generated fromapi-definitions.json. Any documentation fixes should be made inapi-definitions.jsonrather than directly inAI_REFERENCE.md", and "WASM SDK (packages/wasm-sdk) WebAssembly bindings must be built with./build.shand documentation kept in sync usingpython3 generate_docs.py".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/document.rs` at line 465, Add the three new wasm exports to the SDK docs config: update api-definitions.json to register prepareDocumentCreate, prepareDocumentReplace, and prepareDocumentDelete (the #[wasm_bindgen(js_name = "...")] exports in document.rs) with appropriate signatures and descriptions, then regenerate the WebAssembly bindings/docs by running ./build.sh and python3 generate_docs.py so docs.html and AI_REFERENCE.md reflect the new public API surface.
466-522: Consider extracting shared option-parsing logic to reduce duplication.
prepare_document_create(lines 466–522) duplicates ~30 lines of option extraction that are also present indocument_create(lines 107–164). The only meaningful difference between them is the final call —build_document_create_or_replace_transitionvsput_to_platform_and_wait_for_response. Extracting the shared preamble into a private helper struct or a typedDocumentCreateArgswould halve the maintenance surface.This duplication also exists between
prepare_document_replaceanddocument_replace.(Already noted in PR discussion as a known follow-up — flagging here for tracking.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/document.rs` around lines 466 - 522, prepare_document_create duplicates the same options-parsing preamble found in document_create (and similarly for prepare_document_replace/document_replace); extract that shared logic into a small private helper (e.g., a DocumentCreateArgs or parse_document_create_args function) that returns a struct containing the parsed Document/DocumentWasm, contract_id, document_type_name, entropy_array ([u8;32]), IdentityPublicKey (from IdentityPublicKeyWasm), IdentitySignerWasm, and optional PutSettingsInput, reusing existing helpers like DocumentWasm::try_from_options, IdentityPublicKeyWasm::try_from_options, IdentitySignerWasm::try_from_options, get_or_fetch_contract, get_document_type and try_from_options_optional; then refactor prepare_document_create to call the new helper and pass its fields into build_document_create_or_replace_transition, and refactor document_create to call the same helper and pass its fields into put_to_platform_and_wait_for_response so both flows share the parsed arguments.
1147-1161: Fallback random-entropy path is unreachable from all current callers.When
document_state_transition_entropyisNoneand the document is in create mode (revision == Noneorrevision == INITIAL_REVISION), the code falls into a random-entropy branch that generates a newStdRngand rewrites the document ID. This path is currently dead:
prepare_document_createalways suppliesSome(entropy_array)(validated and required).prepare_document_replacealways suppliesNoneentropy but the revision guard ensures the replace branch is taken.This is not a bug—the helper preserves the flexibility of the original
put_to_platformlogic. However, ifStdRng::from_entropy()has platform-specific behaviour in the WASM target (i.e.,getrandomwith thejsfeature), it's worth a quick sanity check in case a future caller accidentally hits this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/document.rs` around lines 1147 - 1161, The fallback branch that generates entropy with StdRng::from_entropy() (used in the document_state_transition_entropy None path and Document::generate_document_id_v0) is effectively unreachable today but may hit WASM where StdRng::from_entropy() behavior differs; update the fallback to be explicit: either (A) mark it unreachable/assert (so callers must supply entropy) or (B) replace StdRng::from_entropy() with a platform-safe entropy source (e.g., OsRng/getrandom) guarded by cfg(target_arch = "wasm32") and return a clear error if secure entropy is unavailable; adjust callers or document this in prepare_document_create / prepare_document_replace and keep references to Document::generate_document_id_v0, document_state_transition_entropy, StdRng::from_entropy, prepare_document_create, prepare_document_replace, and put_to_platform to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/wasm-sdk/src/state_transitions/document.rs`:
- Line 465: Add the three new wasm exports to the SDK docs config: update
api-definitions.json to register prepareDocumentCreate, prepareDocumentReplace,
and prepareDocumentDelete (the #[wasm_bindgen(js_name = "...")] exports in
document.rs) with appropriate signatures and descriptions, then regenerate the
WebAssembly bindings/docs by running ./build.sh and python3 generate_docs.py so
docs.html and AI_REFERENCE.md reflect the new public API surface.
- Around line 466-522: prepare_document_create duplicates the same
options-parsing preamble found in document_create (and similarly for
prepare_document_replace/document_replace); extract that shared logic into a
small private helper (e.g., a DocumentCreateArgs or parse_document_create_args
function) that returns a struct containing the parsed Document/DocumentWasm,
contract_id, document_type_name, entropy_array ([u8;32]), IdentityPublicKey
(from IdentityPublicKeyWasm), IdentitySignerWasm, and optional PutSettingsInput,
reusing existing helpers like DocumentWasm::try_from_options,
IdentityPublicKeyWasm::try_from_options, IdentitySignerWasm::try_from_options,
get_or_fetch_contract, get_document_type and try_from_options_optional; then
refactor prepare_document_create to call the new helper and pass its fields into
build_document_create_or_replace_transition, and refactor document_create to
call the same helper and pass its fields into
put_to_platform_and_wait_for_response so both flows share the parsed arguments.
- Around line 1147-1161: The fallback branch that generates entropy with
StdRng::from_entropy() (used in the document_state_transition_entropy None path
and Document::generate_document_id_v0) is effectively unreachable today but may
hit WASM where StdRng::from_entropy() behavior differs; update the fallback to
be explicit: either (A) mark it unreachable/assert (so callers must supply
entropy) or (B) replace StdRng::from_entropy() with a platform-safe entropy
source (e.g., OsRng/getrandom) guarded by cfg(target_arch = "wasm32") and return
a clear error if secure entropy is unavailable; adjust callers or document this
in prepare_document_create / prepare_document_replace and keep references to
Document::generate_document_id_v0, document_state_transition_entropy,
StdRng::from_entropy, prepare_document_create, prepare_document_replace, and
put_to_platform to locate the code.
thepastaclaw
force-pushed
the
feat/sdk-prepare-document-apis
branch
from
64ee784 to
a868b10
Compare
thepastaclaw
force-pushed
the
feat/sdk-prepare-document-apis
branch
from
a868b10 to
1cee881
Compare
…sitions Add prepare variants for document create, replace, and delete operations that build and sign a StateTransition without broadcasting. This enables idempotent retry patterns where callers can cache the signed ST bytes and rebroadcast on timeout instead of creating duplicates with new nonces. New methods: - prepareDocumentCreate() — build, sign, return ST - prepareDocumentReplace() — build, sign, return ST - prepareDocumentDelete() — build, sign, return ST These pair with the existing broadcastStateTransition() and waitForResponse() methods already exposed in broadcast.rs. Closes dashpay#3090
Addresses CodeRabbit nitpick - more idiomatic Rust pattern that avoids calling revision() twice and the unnecessary unwrap().
…eplace Add a guard in prepare_document_replace to reject documents with no revision or INITIAL_REVISION, which would otherwise silently produce a create transition instead of a replace. Also move the inline DocumentTypeV0Getters import to module-level. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Clippy 1.92 treats unnecessary_map_or as a warning, which CI promotes to error via -D warnings.
thepastaclaw
force-pushed
the
feat/sdk-prepare-document-apis
branch
from
1cee881 to
24fbe7d
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The new prepare_* APIs are well-structured and faithfully mirror upstream rs-sdk logic. The main gap is the omission of client-side structure validation that the upstream put_to_platform performs before broadcast. Platform will catch invalid STs server-side, so this is not blocking, but it degrades error quality and wastes nonces. No blocking issues found.
Reviewed commit: 24fbe7d
🟡 4 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 1174-1177: Missing client-side structure validation before returning signed ST
The upstream `PutDocument::put_to_platform` (rs-sdk `put_document.rs:113`) calls `ensure_valid_state_transition_structure(&transition, sdk.version())?` after building the transition. The `build_document_create_or_replace_transition` helper skips this, so `prepareDocumentCreate` and `prepareDocumentReplace` can return structurally invalid signed transitions to JS callers.
While `ensure_valid_state_transition_structure` is `pub(crate)` in rs-sdk and inaccessible, the underlying `StateTransitionStructureValidation` trait is public in `rs-dpp` and available via `dash-sdk` (through the `dash-sdk-features` → `state-transition-signing` → `state-transition-validation` feature chain). The fix is to call `validate_structure` directly.
Impact: invalid STs would be caught server-side on broadcast, but the error message would be confusing, and the nonce would already be consumed.
- [SUGGESTION] lines 466-522: prepareDocumentCreate lacks symmetric revision guard
`prepareDocumentReplace` (lines 574-584) explicitly guards against documents with no revision or `INITIAL_REVISION`, rejecting create-eligible documents with a clear error. However, `prepareDocumentCreate` has no corresponding guard — a document with `revision > INITIAL_REVISION` silently produces a *replace* transition via the helper's branching logic at line 1132-1134.
This matches upstream `put_to_platform` behavior (which also silently switches between create/replace), but the explicit `prepare*` API names create a stronger expectation that the method does what it says. The asymmetry between the two prepare methods is surprising.
- [SUGGESTION] lines 448-750: No test coverage for new prepare_* APIs
Three new public WASM entry points (`prepareDocumentCreate`, `prepareDocumentReplace`, `prepareDocumentDelete`) were added with zero test coverage. Grepping `packages/wasm-sdk/tests/` for `prepare` returns no matches. Key untested paths: serialization roundtrip of the returned ST, retry idempotency (rebroadcasting identical bytes), the revision guard in `prepareDocumentReplace`, and the Document-vs-plain-object union input in `prepareDocumentDelete`.
- [SUGGESTION] lines 1-17: Module docs should mention nonce consumption semantics
Each `prepare_*` call invokes `get_identity_contract_nonce` with `bump=true` (line 1125), consuming a nonce. If the caller discards the returned ST (e.g., due to an application error between prepare and broadcast), that nonce is permanently wasted. The module-level docs (lines 1-17) explain the retry pattern but don't mention this cost. Users who call prepare speculatively or discard STs will silently accumulate nonce gaps.
Review GateCommit:
|
|
Re-checked this draft today on head I replied inline to my own non-blocking follow-up comments and resolved those threads so the PR thread state matches the intended severity. That leaves the older Feb 17 The concrete snippets + later discussion about |
|
Small correction / code-backed clarification to my earlier retry comments: Inside a single Code path on this branch:
So the gap this PR is addressing is narrower and more specific:
That is what the two-phase API fixes:
So: built-in retries help for in-call transient transport failures; |
|
Code-backed follow-up on the old yappr behavior @PastaPastaPasta asked about: I checked yappr history before the manual ST-caching workaround landed. In commit |
|
Re-checking this on the current draft head The original Feb 17 requested-change ask was “please provide a snippet of code which doesn't work”. That ask is already answered in-thread with concrete examples and later code-backed clarification:
At this point I do not see an unresolved implementation change tied to that review on the current head:
So unless there is a specific remaining blocking objection to the narrower claim here — built-in retries help within one high-level call, while |
|
@shumkov re-checked this draft on the current head At this point I do not see an unresolved code change request on this head:
So unless there is still a specific blocking objection to the narrower claim here — built-in retries help within one high-level call, while If there is still a concrete blocker on the current head, point me at it directly and I’ll address it. Otherwise, could you clear the old |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR adds the intended two-phase prepareDocument* WASM APIs, but introduces an asymmetric validation gap: prepareDocumentCreate accepts documents whose revision > INITIAL_REVISION and silently routes them through the REPLACE branch of the shared helper, producing a wrongly-typed signed ST. The shared helper also skips the ensure_valid_state_transition_structure check that the existing PutDocument::put_to_platform performs before broadcast, so structurally invalid transitions may be cached for retry and only fail at broadcast time. Minor issues: the replace guard also lets revision=0 through, there are no tests for any new prepare-specific validation branches, the helper contains an unreachable random-entropy fallback, and the nonce-bumping side effect of every prepare call is undocumented.
Reviewed commit: 24fbe7d
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [BLOCKING] lines 466-522: prepareDocumentCreate silently produces a REPLACE transition when the input document has a non-INITIAL revision
`prepare_document_create` validates entropy but never checks `document.revision()`. It forwards the call to `build_document_create_or_replace_transition`, whose branch decision at lines 1132-1134 is `document.revision().is_some_and(|rev| rev != INITIAL_REVISION)` → REPLACE. A caller that passes a Document with `revision = 2` (e.g., re-hydrated from storage) will receive a signed *replacement* state transition; the supplied `entropy` is ignored, and the resulting ST will fail at broadcast with DocumentNotFound — or, worse, mutate an existing document that happens to share the ID. Commit 466c8255b2 added the inverse guard in `prepare_document_replace` (rejecting None/INITIAL_REVISION); the symmetric guard is missing on the create side. Reject any document whose revision is `Some(rev)` with `rev != INITIAL_REVISION` so the API name matches the produced transition kind.
- [SUGGESTION] lines 1112-1177: Prepared create/replace transitions skip ensure_valid_state_transition_structure
`PutDocument::put_to_platform` (packages/rs-sdk/src/platform/transition/put_document.rs:113) calls `ensure_valid_state_transition_structure(&transition, sdk.version())?` after building the transition and before broadcasting. `build_document_create_or_replace_transition` omits this step, so `prepareDocumentCreate`/`prepareDocumentReplace` can return a signed, cacheable ST whose structure is invalid — the failure only surfaces later at broadcast, after the caller has already burned an identity-contract nonce and may have persisted the bytes for retry. This defeats a core reason to prepare-then-broadcast. Mirror the `ensure_valid_state_transition_structure` call inside the helper (and apply the same invariant to `prepare_document_delete` for parity) so the prepare path enforces the same structural invariants as the one-shot path.
- [SUGGESTION] lines 574-584: prepareDocumentReplace accepts revision 0, which can never produce a valid replace
The guard at lines 580-584 only rejects `None` and `INITIAL_REVISION` (1). Because Revision is `u64`, a caller can still pass `revision = 0`, which falls through to `build_document_create_or_replace_transition` — where `is_some_and(|rev| rev != INITIAL_REVISION)` routes it to REPLACE. The resulting ST can never be valid against platform state (no document in platform has revision 0), so the prepare API hands the caller a signed object that is guaranteed to fail at broadcast. Collapse the two-step guard into `revision <= INITIAL_REVISION` so the API fails fast on this degenerate input.
- [SUGGESTION] lines 411-754: New prepare_* public API has no tests covering its validation branches
The PR adds ~430 lines of new public WASM API surface (`prepareDocumentCreate`, `prepareDocumentReplace`, `prepareDocumentDelete`, plus the shared helper) with no unit or `wasm_bindgen_test` coverage. The prepare-specific branches have no equivalents in the all-in-one path: the entropy presence/length check (479-487), the revision guard in `prepareDocumentReplace` (574-584), the Document-vs-plain-object branch in `prepareDocumentDelete` (689-709), and the helper's branch selection (1132-1174) are all untested. Given that a regression here silently produces a wrong-typed transition (see the create/replace asymmetry finding), at minimum add tests that assert: (a) `prepareDocumentReplace` errors on `revision = None` and `revision = INITIAL_REVISION`; (b) `prepareDocumentCreate` errors on missing/non-32-byte entropy; (c) the produced transition's batch transition kind matches the API called.
- [SUGGESTION] lines 1121-1128: Prepare API permanently advances the identity-contract nonce on every call
`get_identity_contract_nonce(..., bump_first_nonce=true, ...)` at 1121-1128 advances the on-network nonce as part of returning the next value, so every successful `prepareDocument*` call consumes a nonce slot regardless of whether the caller ever broadcasts. A plausible use of a prepare API is 'build a few candidate transitions and pick one to broadcast' — under this implementation unbroadcast prepares waste nonce slots and can cause subsequent legitimate transitions to land out of order. This is inherent to producing a signable ST in advance, but it is not documented in the TS interface comment or the Rust doc comments. Either document the invariant ('every prepare call advances the identity-contract nonce; do not call prepare unless you intend to broadcast') or expose the nonce as a caller-supplied parameter so the SDK does not have to bump it on every prepare.
| pub async fn prepare_document_create( | ||
| &self, | ||
| options: PrepareDocumentCreateOptionsJs, | ||
| ) -> Result<StateTransitionWasm, WasmSdkError> { | ||
| // Extract document from options | ||
| let document_wasm = DocumentWasm::try_from_options(&options, "document")?; | ||
| let document: Document = document_wasm.clone().into(); | ||
|
|
||
| // Get metadata from document | ||
| let contract_id: Identifier = document_wasm.data_contract_id().into(); | ||
| let document_type_name = document_wasm.document_type_name(); | ||
|
|
||
| // Get entropy from document | ||
| let entropy = document_wasm.entropy().ok_or_else(|| { | ||
| WasmSdkError::invalid_argument("Document must have entropy set for creation") | ||
| })?; | ||
|
|
||
| if entropy.len() != 32 { | ||
| return Err(WasmSdkError::invalid_argument( | ||
| "Document entropy must be exactly 32 bytes", | ||
| )); | ||
| } | ||
|
|
||
| let mut entropy_array = [0u8; 32]; | ||
| entropy_array.copy_from_slice(&entropy); | ||
|
|
||
| // Extract identity key from options | ||
| let identity_key_wasm = IdentityPublicKeyWasm::try_from_options(&options, "identityKey")?; | ||
| let identity_key: IdentityPublicKey = identity_key_wasm.into(); | ||
|
|
||
| // Extract signer from options | ||
| let signer = IdentitySignerWasm::try_from_options(&options, "signer")?; | ||
|
|
||
| // Fetch the data contract (using cache) | ||
| let data_contract = self.get_or_fetch_contract(contract_id).await?; | ||
|
|
||
| // Get document type (owned) | ||
| let document_type = get_document_type(&data_contract, &document_type_name)?; | ||
|
|
||
| // Extract settings from options | ||
| let settings = | ||
| try_from_options_optional::<PutSettingsInput>(&options, "settings")?.map(Into::into); | ||
|
|
||
| // Build and sign the state transition without broadcasting | ||
| let state_transition = build_document_create_or_replace_transition( | ||
| &document, | ||
| &document_type, | ||
| Some(entropy_array), | ||
| &identity_key, | ||
| &signer, | ||
| self.inner_sdk(), | ||
| settings, | ||
| ) | ||
| .await?; | ||
|
|
||
| Ok(state_transition.into()) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: prepareDocumentCreate silently produces a REPLACE transition when the input document has a non-INITIAL revision
prepare_document_create validates entropy but never checks document.revision(). It forwards the call to build_document_create_or_replace_transition, whose branch decision at lines 1132-1134 is document.revision().is_some_and(|rev| rev != INITIAL_REVISION) → REPLACE. A caller that passes a Document with revision = 2 (e.g., re-hydrated from storage) will receive a signed replacement state transition; the supplied entropy is ignored, and the resulting ST will fail at broadcast with DocumentNotFound — or, worse, mutate an existing document that happens to share the ID. Commit 466c825 added the inverse guard in prepare_document_replace (rejecting None/INITIAL_REVISION); the symmetric guard is missing on the create side. Reject any document whose revision is Some(rev) with rev != INITIAL_REVISION so the API name matches the produced transition kind.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [BLOCKING] lines 466-522: prepareDocumentCreate silently produces a REPLACE transition when the input document has a non-INITIAL revision
`prepare_document_create` validates entropy but never checks `document.revision()`. It forwards the call to `build_document_create_or_replace_transition`, whose branch decision at lines 1132-1134 is `document.revision().is_some_and(|rev| rev != INITIAL_REVISION)` → REPLACE. A caller that passes a Document with `revision = 2` (e.g., re-hydrated from storage) will receive a signed *replacement* state transition; the supplied `entropy` is ignored, and the resulting ST will fail at broadcast with DocumentNotFound — or, worse, mutate an existing document that happens to share the ID. Commit 466c8255b2 added the inverse guard in `prepare_document_replace` (rejecting None/INITIAL_REVISION); the symmetric guard is missing on the create side. Reject any document whose revision is `Some(rev)` with `rev != INITIAL_REVISION` so the API name matches the produced transition kind.
| async fn build_document_create_or_replace_transition( | ||
| document: &Document, | ||
| document_type: &DocumentType, | ||
| document_state_transition_entropy: Option<[u8; 32]>, | ||
| identity_public_key: &IdentityPublicKey, | ||
| signer: &IdentitySignerWasm, | ||
| sdk: &dash_sdk::Sdk, | ||
| settings: Option<dash_sdk::platform::transition::put_settings::PutSettings>, | ||
| ) -> Result<dash_sdk::dpp::state_transition::StateTransition, WasmSdkError> { | ||
| let new_identity_contract_nonce = sdk | ||
| .get_identity_contract_nonce( | ||
| document.owner_id(), | ||
| document_type.data_contract_id(), | ||
| true, | ||
| settings, | ||
| ) | ||
| .await?; | ||
|
|
||
| let put_settings = settings.unwrap_or_default(); | ||
|
|
||
| let transition = if document | ||
| .revision() | ||
| .is_some_and(|rev| rev != INITIAL_REVISION) | ||
| { | ||
| BatchTransition::new_document_replacement_transition_from_document( | ||
| document.clone(), | ||
| document_type.as_ref(), | ||
| identity_public_key, | ||
| new_identity_contract_nonce, | ||
| put_settings.user_fee_increase.unwrap_or_default(), | ||
| None, // token_payment_info | ||
| signer, | ||
| sdk.version(), | ||
| put_settings.state_transition_creation_options, | ||
| ) | ||
| } else { | ||
| let (doc, entropy) = document_state_transition_entropy | ||
| .map(|entropy| (document.clone(), entropy)) | ||
| .unwrap_or_else(|| { | ||
| let mut rng = StdRng::from_entropy(); | ||
| let mut doc = document.clone(); | ||
| let entropy = rng.gen::<[u8; 32]>(); | ||
| doc.set_id(Document::generate_document_id_v0( | ||
| &document_type.data_contract_id(), | ||
| &doc.owner_id(), | ||
| document_type.name(), | ||
| entropy.as_slice(), | ||
| )); | ||
| (doc, entropy) | ||
| }); | ||
| BatchTransition::new_document_creation_transition_from_document( | ||
| doc, | ||
| document_type.as_ref(), | ||
| entropy, | ||
| identity_public_key, | ||
| new_identity_contract_nonce, | ||
| put_settings.user_fee_increase.unwrap_or_default(), | ||
| None, // token_payment_info | ||
| signer, | ||
| sdk.version(), | ||
| put_settings.state_transition_creation_options, | ||
| ) | ||
| }?; | ||
|
|
||
| Ok(transition) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Prepared create/replace transitions skip ensure_valid_state_transition_structure
PutDocument::put_to_platform (packages/rs-sdk/src/platform/transition/put_document.rs:113) calls ensure_valid_state_transition_structure(&transition, sdk.version())? after building the transition and before broadcasting. build_document_create_or_replace_transition omits this step, so prepareDocumentCreate/prepareDocumentReplace can return a signed, cacheable ST whose structure is invalid — the failure only surfaces later at broadcast, after the caller has already burned an identity-contract nonce and may have persisted the bytes for retry. This defeats a core reason to prepare-then-broadcast. Mirror the ensure_valid_state_transition_structure call inside the helper (and apply the same invariant to prepare_document_delete for parity) so the prepare path enforces the same structural invariants as the one-shot path.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 1112-1177: Prepared create/replace transitions skip ensure_valid_state_transition_structure
`PutDocument::put_to_platform` (packages/rs-sdk/src/platform/transition/put_document.rs:113) calls `ensure_valid_state_transition_structure(&transition, sdk.version())?` after building the transition and before broadcasting. `build_document_create_or_replace_transition` omits this step, so `prepareDocumentCreate`/`prepareDocumentReplace` can return a signed, cacheable ST whose structure is invalid — the failure only surfaces later at broadcast, after the caller has already burned an identity-contract nonce and may have persisted the bytes for retry. This defeats a core reason to prepare-then-broadcast. Mirror the `ensure_valid_state_transition_structure` call inside the helper (and apply the same invariant to `prepare_document_delete` for parity) so the prepare path enforces the same structural invariants as the one-shot path.
| // Guard: reject documents with no revision or INITIAL_REVISION — those are creates, not replaces | ||
| let revision = document.revision().ok_or_else(|| { | ||
| WasmSdkError::invalid_argument( | ||
| "Document must have a revision set for replace. Use prepareDocumentCreate for new documents.", | ||
| ) | ||
| })?; | ||
| if revision == INITIAL_REVISION { | ||
| return Err(WasmSdkError::invalid_argument( | ||
| "Document revision is INITIAL_REVISION (1). Replace requires revision > 1. Use prepareDocumentCreate for new documents.", | ||
| )); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: prepareDocumentReplace accepts revision 0, which can never produce a valid replace
The guard at lines 580-584 only rejects None and INITIAL_REVISION (1). Because Revision is u64, a caller can still pass revision = 0, which falls through to build_document_create_or_replace_transition — where is_some_and(|rev| rev != INITIAL_REVISION) routes it to REPLACE. The resulting ST can never be valid against platform state (no document in platform has revision 0), so the prepare API hands the caller a signed object that is guaranteed to fail at broadcast. Collapse the two-step guard into revision <= INITIAL_REVISION so the API fails fast on this degenerate input.
💡 Suggested change
| // Guard: reject documents with no revision or INITIAL_REVISION — those are creates, not replaces | |
| let revision = document.revision().ok_or_else(|| { | |
| WasmSdkError::invalid_argument( | |
| "Document must have a revision set for replace. Use prepareDocumentCreate for new documents.", | |
| ) | |
| })?; | |
| if revision == INITIAL_REVISION { | |
| return Err(WasmSdkError::invalid_argument( | |
| "Document revision is INITIAL_REVISION (1). Replace requires revision > 1. Use prepareDocumentCreate for new documents.", | |
| )); | |
| } | |
| let revision = document.revision().ok_or_else(|| { | |
| WasmSdkError::invalid_argument( | |
| "Document must have a revision set for replace. Use prepareDocumentCreate for new documents.", | |
| ) | |
| })?; | |
| if revision <= INITIAL_REVISION { | |
| return Err(WasmSdkError::invalid_argument( | |
| "Document revision must be greater than INITIAL_REVISION (1). Use prepareDocumentCreate for new documents.", | |
| )); | |
| } |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 574-584: prepareDocumentReplace accepts revision 0, which can never produce a valid replace
The guard at lines 580-584 only rejects `None` and `INITIAL_REVISION` (1). Because Revision is `u64`, a caller can still pass `revision = 0`, which falls through to `build_document_create_or_replace_transition` — where `is_some_and(|rev| rev != INITIAL_REVISION)` routes it to REPLACE. The resulting ST can never be valid against platform state (no document in platform has revision 0), so the prepare API hands the caller a signed object that is guaranteed to fail at broadcast. Collapse the two-step guard into `revision <= INITIAL_REVISION` so the API fails fast on this degenerate input.
| } | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Prepare Document Create (Two-Phase API) | ||
| // ============================================================================ | ||
|
|
||
| /// TypeScript interface for prepare document create options | ||
| #[wasm_bindgen(typescript_custom_section)] | ||
| const PREPARE_DOCUMENT_CREATE_OPTIONS_TS: &'static str = r#" | ||
| /** | ||
| * Options for preparing a document creation state transition without broadcasting. | ||
| * | ||
| * Use this for idempotent retry patterns: | ||
| * 1. Call `prepareDocumentCreate()` to get a signed `StateTransition` | ||
| * 2. Cache `stateTransition.toBytes()` for retry safety | ||
| * 3. Call `broadcastStateTransition(st)` + `waitForResponse(st)` | ||
| * 4. On timeout, deserialize cached bytes and rebroadcast the **identical** ST | ||
| */ | ||
| export interface PrepareDocumentCreateOptions { | ||
| /** The document to create. */ | ||
| document: Document; | ||
| /** The identity public key to use for signing. */ | ||
| identityKey: IdentityPublicKey; | ||
| /** Signer containing the private key for the identity key. */ | ||
| signer: IdentitySigner; | ||
| /** Optional settings (retries, timeouts, userFeeIncrease). */ | ||
| settings?: PutSettings; | ||
| } | ||
| "#; | ||
|
|
||
| #[wasm_bindgen] | ||
| extern "C" { | ||
| #[wasm_bindgen(typescript_type = "PrepareDocumentCreateOptions")] | ||
| pub type PrepareDocumentCreateOptionsJs; | ||
| } | ||
|
|
||
| #[wasm_bindgen] | ||
| impl WasmSdk { | ||
| /// Prepare a document creation state transition without broadcasting. | ||
| /// | ||
| /// This method handles nonce management, ST construction, and signing, but does | ||
| /// **not** broadcast or wait for a response. The returned `StateTransition` can be: | ||
| /// | ||
| /// - Serialized with `toBytes()` and cached for retry safety | ||
| /// - Broadcast with `broadcastStateTransition(st)` | ||
| /// - Awaited with `waitForResponse(st)` | ||
| /// | ||
| /// This is the "prepare" half of the two-phase API. Use it when you need | ||
| /// idempotent retry behavior — on timeout, you can rebroadcast the exact same | ||
| /// signed transition instead of creating a new one with a new nonce. | ||
| /// | ||
| /// @param options - Creation options including document, identity key, and signer | ||
| /// @returns The signed StateTransition ready for broadcasting | ||
| #[wasm_bindgen(js_name = "prepareDocumentCreate")] | ||
| pub async fn prepare_document_create( | ||
| &self, | ||
| options: PrepareDocumentCreateOptionsJs, | ||
| ) -> Result<StateTransitionWasm, WasmSdkError> { | ||
| // Extract document from options | ||
| let document_wasm = DocumentWasm::try_from_options(&options, "document")?; | ||
| let document: Document = document_wasm.clone().into(); | ||
|
|
||
| // Get metadata from document | ||
| let contract_id: Identifier = document_wasm.data_contract_id().into(); | ||
| let document_type_name = document_wasm.document_type_name(); | ||
|
|
||
| // Get entropy from document | ||
| let entropy = document_wasm.entropy().ok_or_else(|| { | ||
| WasmSdkError::invalid_argument("Document must have entropy set for creation") | ||
| })?; | ||
|
|
||
| if entropy.len() != 32 { | ||
| return Err(WasmSdkError::invalid_argument( | ||
| "Document entropy must be exactly 32 bytes", | ||
| )); | ||
| } | ||
|
|
||
| let mut entropy_array = [0u8; 32]; | ||
| entropy_array.copy_from_slice(&entropy); | ||
|
|
||
| // Extract identity key from options | ||
| let identity_key_wasm = IdentityPublicKeyWasm::try_from_options(&options, "identityKey")?; | ||
| let identity_key: IdentityPublicKey = identity_key_wasm.into(); | ||
|
|
||
| // Extract signer from options | ||
| let signer = IdentitySignerWasm::try_from_options(&options, "signer")?; | ||
|
|
||
| // Fetch the data contract (using cache) | ||
| let data_contract = self.get_or_fetch_contract(contract_id).await?; | ||
|
|
||
| // Get document type (owned) | ||
| let document_type = get_document_type(&data_contract, &document_type_name)?; | ||
|
|
||
| // Extract settings from options | ||
| let settings = | ||
| try_from_options_optional::<PutSettingsInput>(&options, "settings")?.map(Into::into); | ||
|
|
||
| // Build and sign the state transition without broadcasting | ||
| let state_transition = build_document_create_or_replace_transition( | ||
| &document, | ||
| &document_type, | ||
| Some(entropy_array), | ||
| &identity_key, | ||
| &signer, | ||
| self.inner_sdk(), | ||
| settings, | ||
| ) | ||
| .await?; | ||
|
|
||
| Ok(state_transition.into()) | ||
| } | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Prepare Document Replace (Two-Phase API) | ||
| // ============================================================================ | ||
|
|
||
| /// TypeScript interface for prepare document replace options | ||
| #[wasm_bindgen(typescript_custom_section)] | ||
| const PREPARE_DOCUMENT_REPLACE_OPTIONS_TS: &'static str = r#" | ||
| /** | ||
| * Options for preparing a document replace state transition without broadcasting. | ||
| * | ||
| * Use this for idempotent retry patterns. See `prepareDocumentCreate` for the full pattern. | ||
| */ | ||
| export interface PrepareDocumentReplaceOptions { | ||
| /** The document with updated data (same ID, incremented revision). */ | ||
| document: Document; | ||
| /** The identity public key to use for signing. */ | ||
| identityKey: IdentityPublicKey; | ||
| /** Signer containing the private key for the identity key. */ | ||
| signer: IdentitySigner; | ||
| /** Optional settings (retries, timeouts, userFeeIncrease). */ | ||
| settings?: PutSettings; | ||
| } | ||
| "#; | ||
|
|
||
| #[wasm_bindgen] | ||
| extern "C" { | ||
| #[wasm_bindgen(typescript_type = "PrepareDocumentReplaceOptions")] | ||
| pub type PrepareDocumentReplaceOptionsJs; | ||
| } | ||
|
|
||
| #[wasm_bindgen] | ||
| impl WasmSdk { | ||
| /// Prepare a document replace state transition without broadcasting. | ||
| /// | ||
| /// This method handles nonce management, ST construction, and signing, but does | ||
| /// **not** broadcast or wait for a response. See `prepareDocumentCreate` for | ||
| /// the full two-phase usage pattern. | ||
| /// | ||
| /// @param options - Replace options including document, identity key, and signer | ||
| /// @returns The signed StateTransition ready for broadcasting | ||
| #[wasm_bindgen(js_name = "prepareDocumentReplace")] | ||
| pub async fn prepare_document_replace( | ||
| &self, | ||
| options: PrepareDocumentReplaceOptionsJs, | ||
| ) -> Result<StateTransitionWasm, WasmSdkError> { | ||
| // Extract document from options | ||
| let document_wasm = DocumentWasm::try_from_options(&options, "document")?; | ||
| let document: Document = document_wasm.clone().into(); | ||
|
|
||
| // Guard: reject documents with no revision or INITIAL_REVISION — those are creates, not replaces | ||
| let revision = document.revision().ok_or_else(|| { | ||
| WasmSdkError::invalid_argument( | ||
| "Document must have a revision set for replace. Use prepareDocumentCreate for new documents.", | ||
| ) | ||
| })?; | ||
| if revision == INITIAL_REVISION { | ||
| return Err(WasmSdkError::invalid_argument( | ||
| "Document revision is INITIAL_REVISION (1). Replace requires revision > 1. Use prepareDocumentCreate for new documents.", | ||
| )); | ||
| } | ||
|
|
||
| // Get metadata from document | ||
| let contract_id: Identifier = document_wasm.data_contract_id().into(); | ||
| let document_type_name = document_wasm.document_type_name(); | ||
|
|
||
| // Extract identity key from options | ||
| let identity_key_wasm = IdentityPublicKeyWasm::try_from_options(&options, "identityKey")?; | ||
| let identity_key: IdentityPublicKey = identity_key_wasm.into(); | ||
|
|
||
| // Extract signer from options | ||
| let signer = IdentitySignerWasm::try_from_options(&options, "signer")?; | ||
|
|
||
| // Fetch the data contract (using cache) | ||
| let data_contract = self.get_or_fetch_contract(contract_id).await?; | ||
|
|
||
| // Get document type (owned) | ||
| let document_type = get_document_type(&data_contract, &document_type_name)?; | ||
|
|
||
| // Extract settings from options | ||
| let settings = | ||
| try_from_options_optional::<PutSettingsInput>(&options, "settings")?.map(Into::into); | ||
|
|
||
| // Build and sign the state transition without broadcasting | ||
| let state_transition = build_document_create_or_replace_transition( | ||
| &document, | ||
| &document_type, | ||
| None, // entropy not needed for replace | ||
| &identity_key, | ||
| &signer, | ||
| self.inner_sdk(), | ||
| settings, | ||
| ) | ||
| .await?; | ||
|
|
||
| Ok(state_transition.into()) | ||
| } | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Prepare Document Delete (Two-Phase API) | ||
| // ============================================================================ | ||
|
|
||
| /// TypeScript interface for prepare document delete options | ||
| #[wasm_bindgen(typescript_custom_section)] | ||
| const PREPARE_DOCUMENT_DELETE_OPTIONS_TS: &'static str = r#" | ||
| /** | ||
| * Options for preparing a document delete state transition without broadcasting. | ||
| * | ||
| * Use this for idempotent retry patterns. See `prepareDocumentCreate` for the full pattern. | ||
| */ | ||
| export interface PrepareDocumentDeleteOptions { | ||
| /** | ||
| * The document to delete — either a Document instance or an object with identifiers. | ||
| */ | ||
| document: Document | { | ||
| id: IdentifierLike; | ||
| ownerId: IdentifierLike; | ||
| dataContractId: IdentifierLike; | ||
| documentTypeName: string; | ||
| }; | ||
| /** The identity public key to use for signing. */ | ||
| identityKey: IdentityPublicKey; | ||
| /** Signer containing the private key for the identity key. */ | ||
| signer: IdentitySigner; | ||
| /** Optional settings (retries, timeouts, userFeeIncrease). */ | ||
| settings?: PutSettings; | ||
| } | ||
| "#; | ||
|
|
||
| #[wasm_bindgen] | ||
| extern "C" { | ||
| #[wasm_bindgen(typescript_type = "PrepareDocumentDeleteOptions")] | ||
| pub type PrepareDocumentDeleteOptionsJs; | ||
| } | ||
|
|
||
| #[wasm_bindgen] | ||
| impl WasmSdk { | ||
| /// Prepare a document delete state transition without broadcasting. | ||
| /// | ||
| /// This method handles nonce management, ST construction, and signing, but does | ||
| /// **not** broadcast or wait for a response. See `prepareDocumentCreate` for | ||
| /// the full two-phase usage pattern. | ||
| /// | ||
| /// @param options - Delete options including document identifiers, identity key, and signer | ||
| /// @returns The signed StateTransition ready for broadcasting | ||
| #[wasm_bindgen(js_name = "prepareDocumentDelete")] | ||
| pub async fn prepare_document_delete( | ||
| &self, | ||
| options: PrepareDocumentDeleteOptionsJs, | ||
| ) -> Result<StateTransitionWasm, WasmSdkError> { | ||
| // Extract document field - can be either a Document instance or plain object | ||
| let document_js = js_sys::Reflect::get(&options, &JsValue::from_str("document")) | ||
| .map_err(|_| WasmSdkError::invalid_argument("document is required"))?; | ||
|
|
||
| if document_js.is_undefined() || document_js.is_null() { | ||
| return Err(WasmSdkError::invalid_argument("document is required")); | ||
| } | ||
|
|
||
| // Check if it's a Document instance or a plain object with fields | ||
| let (document_id, owner_id, contract_id, document_type_name): ( | ||
| Identifier, | ||
| Identifier, | ||
| Identifier, | ||
| String, | ||
| ) = if get_class_type(&document_js).ok().as_deref() == Some("Document") { | ||
| let doc: DocumentWasm = document_js | ||
| .to_wasm::<DocumentWasm>("Document") | ||
| .map(|boxed| (*boxed).clone())?; | ||
| let doc_inner: Document = doc.clone().into(); | ||
| ( | ||
| doc.id().into(), | ||
| doc_inner.owner_id(), | ||
| doc.data_contract_id().into(), | ||
| doc.document_type_name(), | ||
| ) | ||
| } else { | ||
| ( | ||
| IdentifierWasm::try_from_options(&document_js, "id")?.into(), | ||
| IdentifierWasm::try_from_options(&document_js, "ownerId")?.into(), | ||
| IdentifierWasm::try_from_options(&document_js, "dataContractId")?.into(), | ||
| try_from_options_with(&document_js, "documentTypeName", |v| { | ||
| try_to_string(v, "documentTypeName") | ||
| })?, | ||
| ) | ||
| }; | ||
|
|
||
| // Extract identity key from options | ||
| let identity_key_wasm = IdentityPublicKeyWasm::try_from_options(&options, "identityKey")?; | ||
| let identity_key: IdentityPublicKey = identity_key_wasm.into(); | ||
|
|
||
| // Extract signer from options | ||
| let signer = IdentitySignerWasm::try_from_options(&options, "signer")?; | ||
|
|
||
| // Fetch the data contract (using cache) | ||
| let data_contract = self.get_or_fetch_contract(contract_id).await?; | ||
|
|
||
| // Extract settings from options | ||
| let settings = | ||
| try_from_options_optional::<PutSettingsInput>(&options, "settings")?.map(Into::into); | ||
|
|
||
| // Build the delete transition using the builder's sign method (which does NOT broadcast) | ||
| let builder = DocumentDeleteTransitionBuilder::new( | ||
| Arc::new(data_contract), | ||
| document_type_name, | ||
| document_id, | ||
| owner_id, | ||
| ); | ||
|
|
||
| let builder = if let Some(s) = settings { | ||
| builder.with_settings(s) | ||
| } else { | ||
| builder | ||
| }; | ||
|
|
||
| let state_transition = builder | ||
| .sign( | ||
| self.inner_sdk(), | ||
| &identity_key, | ||
| &signer, | ||
| self.inner_sdk().version(), | ||
| ) | ||
| .await?; | ||
|
|
||
| Ok(state_transition.into()) | ||
| } | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Document Transfer | ||
| // ============================================================================ |
There was a problem hiding this comment.
🟡 Suggestion: New prepare_ public API has no tests covering its validation branches*
The PR adds ~430 lines of new public WASM API surface (prepareDocumentCreate, prepareDocumentReplace, prepareDocumentDelete, plus the shared helper) with no unit or wasm_bindgen_test coverage. The prepare-specific branches have no equivalents in the all-in-one path: the entropy presence/length check (479-487), the revision guard in prepareDocumentReplace (574-584), the Document-vs-plain-object branch in prepareDocumentDelete (689-709), and the helper's branch selection (1132-1174) are all untested. Given that a regression here silently produces a wrong-typed transition (see the create/replace asymmetry finding), at minimum add tests that assert: (a) prepareDocumentReplace errors on revision = None and revision = INITIAL_REVISION; (b) prepareDocumentCreate errors on missing/non-32-byte entropy; (c) the produced transition's batch transition kind matches the API called.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 411-754: New prepare_* public API has no tests covering its validation branches
The PR adds ~430 lines of new public WASM API surface (`prepareDocumentCreate`, `prepareDocumentReplace`, `prepareDocumentDelete`, plus the shared helper) with no unit or `wasm_bindgen_test` coverage. The prepare-specific branches have no equivalents in the all-in-one path: the entropy presence/length check (479-487), the revision guard in `prepareDocumentReplace` (574-584), the Document-vs-plain-object branch in `prepareDocumentDelete` (689-709), and the helper's branch selection (1132-1174) are all untested. Given that a regression here silently produces a wrong-typed transition (see the create/replace asymmetry finding), at minimum add tests that assert: (a) `prepareDocumentReplace` errors on `revision = None` and `revision = INITIAL_REVISION`; (b) `prepareDocumentCreate` errors on missing/non-32-byte entropy; (c) the produced transition's batch transition kind matches the API called.
| let new_identity_contract_nonce = sdk | ||
| .get_identity_contract_nonce( | ||
| document.owner_id(), | ||
| document_type.data_contract_id(), | ||
| true, | ||
| settings, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
🟡 Suggestion: Prepare API permanently advances the identity-contract nonce on every call
get_identity_contract_nonce(..., bump_first_nonce=true, ...) at 1121-1128 advances the on-network nonce as part of returning the next value, so every successful prepareDocument* call consumes a nonce slot regardless of whether the caller ever broadcasts. A plausible use of a prepare API is 'build a few candidate transitions and pick one to broadcast' — under this implementation unbroadcast prepares waste nonce slots and can cause subsequent legitimate transitions to land out of order. This is inherent to producing a signable ST in advance, but it is not documented in the TS interface comment or the Rust doc comments. Either document the invariant ('every prepare call advances the identity-contract nonce; do not call prepare unless you intend to broadcast') or expose the nonce as a caller-supplied parameter so the SDK does not have to bump it on every prepare.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 1121-1128: Prepare API permanently advances the identity-contract nonce on every call
`get_identity_contract_nonce(..., bump_first_nonce=true, ...)` at 1121-1128 advances the on-network nonce as part of returning the next value, so every successful `prepareDocument*` call consumes a nonce slot regardless of whether the caller ever broadcasts. A plausible use of a prepare API is 'build a few candidate transitions and pick one to broadcast' — under this implementation unbroadcast prepares waste nonce slots and can cause subsequent legitimate transitions to land out of order. This is inherent to producing a signable ST in advance, but it is not documented in the TS interface comment or the Rust doc comments. Either document the invariant ('every prepare call advances the identity-contract nonce; do not call prepare unless you intend to broadcast') or expose the nonce as a caller-supplied parameter so the SDK does not have to bump it on every prepare.
| let (doc, entropy) = document_state_transition_entropy | ||
| .map(|entropy| (document.clone(), entropy)) | ||
| .unwrap_or_else(|| { | ||
| let mut rng = StdRng::from_entropy(); | ||
| let mut doc = document.clone(); | ||
| let entropy = rng.gen::<[u8; 32]>(); | ||
| doc.set_id(Document::generate_document_id_v0( | ||
| &document_type.data_contract_id(), | ||
| &doc.owner_id(), | ||
| document_type.name(), | ||
| entropy.as_slice(), | ||
| )); | ||
| (doc, entropy) | ||
| }); |
There was a problem hiding this comment.
💬 Nitpick: Random-entropy fallback in the shared helper is unreachable from current callers
The fallback that calls StdRng::from_entropy() and regenerates the document ID only fires when the helper is invoked with document_state_transition_entropy: None AND a revision of None or INITIAL_REVISION. prepare_document_create requires entropy upfront (lines 479-487), so it never passes None. prepare_document_replace rejects None/INITIAL_REVISION revisions (574-584), so it always takes the REPLACE branch. In the original put_to_platform this fallback makes sense because it's a single entry point, but here it is silent dead code. Either remove the fallback and require explicit entropy on the create branch, or add a comment documenting the contract so a future caller can rely on it without accidentally broadcasting a transition whose document ID is non-deterministic from the caller's perspective.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Findings verify correctly against the checkout. The structure-validation helper is indeed a no-op for document Batch transitions (confirmed via rs-dpp state_transition/mod.rs:1409-1426 returning UnsupportedFeatureError, which validate_state_transition_structure explicitly passes through), but since this exactly matches rs-sdk's pre-broadcast behavior it is not a regression — we downgrade from blocking to suggestion and combine the two reports. The other issues (nonce leak on mid-pipeline failure, asymmetric revision guards vs one-shot APIs, shallow functional tests, owner_id asymmetry in prepare_document_delete) are all real and kept as suggestions/nitpicks.
Reviewed commit: 3d84f1b
🟡 4 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 1258-1287: validate_state_transition_structure is a no-op for every document Batch transition
`validate_state_transition_structure` calls `StateTransition::validate_structure` and then deliberately treats `BasicError::UnsupportedFeatureError` as success. In `packages/rs-dpp/src/state_transition/mod.rs:1409-1426`, the `StateTransition::Batch(_)` arm returns exactly that error, and every transition produced by `prepareDocumentCreate/Replace/Delete` is a `Batch`. So the validation call added at document.rs:553 and 663 (and the delete site) always short-circuits to `Ok(())` for the prepare paths despite comments stating that structure is validated. The included unit test `validate_accepts_unsupported_feature_errors` exercises an `IdentityCreditTransfer` transition, so it cannot catch this. This matches rs-sdk's `ensure_valid_state_transition_structure`, which is also a no-op for these variants, so it is not a regression — but the comments and module docs over-promise. Either drop the call and the misleading comment, or call Batch-specific structural validators (e.g. validating each `DocumentBaseTransition`) so the guard provides real value.
- [SUGGESTION] lines 1175-1228: Identity-contract nonce is consumed even when the prepare call fails after the bump
`build_document_create_or_replace_transition` calls `sdk.get_identity_contract_nonce(..., true, ...)` at line 1175, which advances the cached/server-known nonce before the transition is built and signed. If `BatchTransition::new_document_*_transition_from_document` fails (signer error, internal validation) or the post-build `validate_state_transition_structure` returns an error, the caller never receives a usable transition but the nonce has already been bumped. The module docs flag this for the case where the caller discards a successful result, but in-process failure paths leak silently with no recovery. Since this API is explicitly designed for retry workflows where nonce leaks matter more than usual, consider calling `sdk.refresh_identity_nonce(&owner_id)` (or equivalent) on the error path so the next attempt re-fetches and doesn't widen the gap.
- [SUGGESTION] lines 487-626: Revision guards are inconsistent between prepareDocument* and documentCreate/documentReplace
`prepare_document_create` rejects documents with `revision != INITIAL_REVISION` (lines 495-502) and `prepare_document_replace` rejects documents with `revision <= INITIAL_REVISION` (lines 616-626). These guards are correct — `build_document_create_or_replace_transition` routes by revision and silently picks the wrong branch otherwise. The existing `document_create` (lines 119-176) and `document_replace` (lines 236-) call `put_to_platform_and_wait_for_response` with no equivalent guards, so the same misuse silently produces the wrong transition kind in the all-in-one path. Lift the same guards into the one-shot APIs (or factor them into a shared validator) so both paths reject the same inputs.
In `packages/wasm-sdk/tests/functional/transitions/documents.spec.ts`:
- [SUGGESTION] lines 284-409: Functional tests don't exercise the PR's central idempotency claim
The new functional tests only assert that `prepareDocument*` returns a Batch transition with the expected inner action type. That is necessary but not sufficient for the PR's central claim — that a prepared ST can be cached, broadcast, and (on timeout) rebroadcast without the platform creating a duplicate or rejecting it. There are no assertions that `broadcastStateTransition(st)` succeeds for a prepared ST, that `st.toBytes()` → `StateTransition.fromBytes(...)` → broadcast yields the same on-chain effect, or that a second broadcast of the same ST is accepted as a duplicate. These behaviors are what users will rely on; the manual yappr testing referenced in the PR description should be encoded as automated tests.
| fn validate_state_transition_structure( | ||
| state_transition: &dash_sdk::dpp::state_transition::StateTransition, | ||
| platform_version: &dash_sdk::dpp::version::PlatformVersion, | ||
| ) -> Result<(), WasmSdkError> { | ||
| use dash_sdk::dpp::consensus::basic::BasicError; | ||
| use dash_sdk::dpp::consensus::ConsensusError; | ||
| use dash_sdk::dpp::state_transition::StateTransitionStructureValidation; | ||
|
|
||
| let validation_result = state_transition.validate_structure(platform_version); | ||
| if validation_result.is_valid() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let all_unsupported = validation_result.errors.iter().all(|e| { | ||
| matches!( | ||
| e, | ||
| ConsensusError::BasicError(BasicError::UnsupportedFeatureError(_)) | ||
| ) | ||
| }); | ||
| if all_unsupported { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Surface the first error through the ProtocolError → WasmSdkError conversion chain. | ||
| // `is_valid()` returned false, so `errors` is non-empty; fall back defensively. | ||
| let first = validation_result.errors.into_iter().next().ok_or_else(|| { | ||
| WasmSdkError::generic("state transition structure validation failed without an error") | ||
| })?; | ||
| Err(dash_sdk::dpp::ProtocolError::from(first).into()) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: validate_state_transition_structure is a no-op for every document Batch transition
validate_state_transition_structure calls StateTransition::validate_structure and then deliberately treats BasicError::UnsupportedFeatureError as success. In packages/rs-dpp/src/state_transition/mod.rs:1409-1426, the StateTransition::Batch(_) arm returns exactly that error, and every transition produced by prepareDocumentCreate/Replace/Delete is a Batch. So the validation call added at document.rs:553 and 663 (and the delete site) always short-circuits to Ok(()) for the prepare paths despite comments stating that structure is validated. The included unit test validate_accepts_unsupported_feature_errors exercises an IdentityCreditTransfer transition, so it cannot catch this. This matches rs-sdk's ensure_valid_state_transition_structure, which is also a no-op for these variants, so it is not a regression — but the comments and module docs over-promise. Either drop the call and the misleading comment, or call Batch-specific structural validators (e.g. validating each DocumentBaseTransition) so the guard provides real value.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 1258-1287: validate_state_transition_structure is a no-op for every document Batch transition
`validate_state_transition_structure` calls `StateTransition::validate_structure` and then deliberately treats `BasicError::UnsupportedFeatureError` as success. In `packages/rs-dpp/src/state_transition/mod.rs:1409-1426`, the `StateTransition::Batch(_)` arm returns exactly that error, and every transition produced by `prepareDocumentCreate/Replace/Delete` is a `Batch`. So the validation call added at document.rs:553 and 663 (and the delete site) always short-circuits to `Ok(())` for the prepare paths despite comments stating that structure is validated. The included unit test `validate_accepts_unsupported_feature_errors` exercises an `IdentityCreditTransfer` transition, so it cannot catch this. This matches rs-sdk's `ensure_valid_state_transition_structure`, which is also a no-op for these variants, so it is not a regression — but the comments and module docs over-promise. Either drop the call and the misleading comment, or call Batch-specific structural validators (e.g. validating each `DocumentBaseTransition`) so the guard provides real value.
| let new_identity_contract_nonce = sdk | ||
| .get_identity_contract_nonce( | ||
| document.owner_id(), | ||
| document_type.data_contract_id(), | ||
| true, | ||
| settings, | ||
| ) | ||
| .await?; | ||
|
|
||
| let put_settings = settings.unwrap_or_default(); | ||
|
|
||
| let transition = if document | ||
| .revision() | ||
| .is_some_and(|rev| rev != INITIAL_REVISION) | ||
| { | ||
| BatchTransition::new_document_replacement_transition_from_document( | ||
| document.clone(), | ||
| document_type.as_ref(), | ||
| identity_public_key, | ||
| new_identity_contract_nonce, | ||
| put_settings.user_fee_increase.unwrap_or_default(), | ||
| None, // token_payment_info | ||
| signer, | ||
| sdk.version(), | ||
| put_settings.state_transition_creation_options, | ||
| ) | ||
| } else { | ||
| let (doc, entropy) = document_state_transition_entropy | ||
| .map(|entropy| (document.clone(), entropy)) | ||
| .unwrap_or_else(|| { | ||
| let mut rng = StdRng::from_entropy(); | ||
| let mut doc = document.clone(); | ||
| let entropy = rng.gen::<[u8; 32]>(); | ||
| doc.set_id(Document::generate_document_id_v0( | ||
| &document_type.data_contract_id(), | ||
| &doc.owner_id(), | ||
| document_type.name(), | ||
| entropy.as_slice(), | ||
| )); | ||
| (doc, entropy) | ||
| }); | ||
| BatchTransition::new_document_creation_transition_from_document( | ||
| doc, | ||
| document_type.as_ref(), | ||
| entropy, | ||
| identity_public_key, | ||
| new_identity_contract_nonce, | ||
| put_settings.user_fee_increase.unwrap_or_default(), | ||
| None, // token_payment_info | ||
| signer, | ||
| sdk.version(), | ||
| put_settings.state_transition_creation_options, | ||
| ) | ||
| }?; |
There was a problem hiding this comment.
🟡 Suggestion: Identity-contract nonce is consumed even when the prepare call fails after the bump
build_document_create_or_replace_transition calls sdk.get_identity_contract_nonce(..., true, ...) at line 1175, which advances the cached/server-known nonce before the transition is built and signed. If BatchTransition::new_document_*_transition_from_document fails (signer error, internal validation) or the post-build validate_state_transition_structure returns an error, the caller never receives a usable transition but the nonce has already been bumped. The module docs flag this for the case where the caller discards a successful result, but in-process failure paths leak silently with no recovery. Since this API is explicitly designed for retry workflows where nonce leaks matter more than usual, consider calling sdk.refresh_identity_nonce(&owner_id) (or equivalent) on the error path so the next attempt re-fetches and doesn't widen the gap.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 1175-1228: Identity-contract nonce is consumed even when the prepare call fails after the bump
`build_document_create_or_replace_transition` calls `sdk.get_identity_contract_nonce(..., true, ...)` at line 1175, which advances the cached/server-known nonce before the transition is built and signed. If `BatchTransition::new_document_*_transition_from_document` fails (signer error, internal validation) or the post-build `validate_state_transition_structure` returns an error, the caller never receives a usable transition but the nonce has already been bumped. The module docs flag this for the case where the caller discards a successful result, but in-process failure paths leak silently with no recovery. Since this API is explicitly designed for retry workflows where nonce leaks matter more than usual, consider calling `sdk.refresh_identity_nonce(&owner_id)` (or equivalent) on the error path so the next attempt re-fetches and doesn't widen the gap.
| let document_wasm = DocumentWasm::try_from_options(&options, "document")?; | ||
| let document: Document = document_wasm.clone().into(); | ||
|
|
||
| // Guard: reject documents with any explicit revision other than INITIAL_REVISION. | ||
| // `build_document_create_or_replace_transition` routes any `revision != INITIAL_REVISION` | ||
| // (including `0`) to the replace branch, so the only explicit values that are safe for | ||
| // create are `None` and `INITIAL_REVISION` (1). Without this guard, a `revision = 0` | ||
| // document would silently produce a replace transition, which is not what the caller asked for. | ||
| if let Some(revision) = document.revision() { | ||
| if revision != INITIAL_REVISION { | ||
| return Err(WasmSdkError::invalid_argument(format!( | ||
| "Document revision is {} but create requires revision to be unset or {}. Use prepareDocumentReplace for existing documents.", | ||
| revision, INITIAL_REVISION, | ||
| ))); | ||
| } | ||
| } | ||
|
|
||
| // Get metadata from document | ||
| let contract_id: Identifier = document_wasm.data_contract_id().into(); | ||
| let document_type_name = document_wasm.document_type_name(); | ||
|
|
||
| // Get entropy from document | ||
| let entropy = document_wasm.entropy().ok_or_else(|| { | ||
| WasmSdkError::invalid_argument("Document must have entropy set for creation") | ||
| })?; | ||
|
|
||
| if entropy.len() != 32 { | ||
| return Err(WasmSdkError::invalid_argument( | ||
| "Document entropy must be exactly 32 bytes", | ||
| )); | ||
| } | ||
|
|
||
| let mut entropy_array = [0u8; 32]; | ||
| entropy_array.copy_from_slice(&entropy); | ||
|
|
||
| // Extract identity key from options | ||
| let identity_key_wasm = IdentityPublicKeyWasm::try_from_options(&options, "identityKey")?; | ||
| let identity_key: IdentityPublicKey = identity_key_wasm.into(); | ||
|
|
||
| // Extract signer from options | ||
| let signer = IdentitySignerWasm::try_from_options(&options, "signer")?; | ||
|
|
||
| // Fetch the data contract (using cache) | ||
| let data_contract = self.get_or_fetch_contract(contract_id).await?; | ||
|
|
||
| // Get document type (owned) | ||
| let document_type = get_document_type(&data_contract, &document_type_name)?; | ||
|
|
||
| // Extract settings from options | ||
| let settings = | ||
| try_from_options_optional::<PutSettingsInput>(&options, "settings")?.map(Into::into); | ||
|
|
||
| // Build and sign the state transition without broadcasting | ||
| let state_transition = build_document_create_or_replace_transition( | ||
| &document, | ||
| &document_type, | ||
| Some(entropy_array), | ||
| &identity_key, | ||
| &signer, | ||
| self.inner_sdk(), | ||
| settings, | ||
| ) | ||
| .await?; | ||
|
|
||
| // Validate structure before handing the ST back to the caller, matching | ||
| // the check rs-sdk's `put_to_platform` performs before broadcasting. | ||
| validate_state_transition_structure(&state_transition, self.inner_sdk().version())?; | ||
|
|
||
| Ok(state_transition.into()) | ||
| } | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Prepare Document Replace (Two-Phase API) | ||
| // ============================================================================ | ||
|
|
||
| /// TypeScript interface for prepare document replace options | ||
| #[wasm_bindgen(typescript_custom_section)] | ||
| const PREPARE_DOCUMENT_REPLACE_OPTIONS_TS: &'static str = r#" | ||
| /** | ||
| * Options for preparing a document replace state transition without broadcasting. | ||
| * | ||
| * Use this for idempotent retry patterns. See `prepareDocumentCreate` for the full pattern. | ||
| */ | ||
| export interface PrepareDocumentReplaceOptions { | ||
| /** The document with updated data (same ID, incremented revision). */ | ||
| document: Document; | ||
| /** The identity public key to use for signing. */ | ||
| identityKey: IdentityPublicKey; | ||
| /** Signer containing the private key for the identity key. */ | ||
| signer: IdentitySigner; | ||
| /** Optional settings (retries, timeouts, userFeeIncrease). */ | ||
| settings?: PutSettings; | ||
| } | ||
| "#; | ||
|
|
||
| #[wasm_bindgen] | ||
| extern "C" { | ||
| #[wasm_bindgen(typescript_type = "PrepareDocumentReplaceOptions")] | ||
| pub type PrepareDocumentReplaceOptionsJs; | ||
| } | ||
|
|
||
| #[wasm_bindgen] | ||
| impl WasmSdk { | ||
| /// Prepare a document replace state transition without broadcasting. | ||
| /// | ||
| /// This method handles nonce management, ST construction, and signing, but does | ||
| /// **not** broadcast or wait for a response. See `prepareDocumentCreate` for | ||
| /// the full two-phase usage pattern. | ||
| /// | ||
| /// **Nonce consumption:** A successful call consumes (advances) the identity-contract | ||
| /// nonce. Only call this when you intend to broadcast / persist-and-retry the returned | ||
| /// transition — discarding it leaks the nonce. See module docs for details. | ||
| /// | ||
| /// @param options - Replace options including document, identity key, and signer | ||
| /// @returns The signed StateTransition ready for broadcasting | ||
| #[wasm_bindgen(js_name = "prepareDocumentReplace")] | ||
| pub async fn prepare_document_replace( | ||
| &self, | ||
| options: PrepareDocumentReplaceOptionsJs, | ||
| ) -> Result<StateTransitionWasm, WasmSdkError> { | ||
| // Extract document from options | ||
| let document_wasm = DocumentWasm::try_from_options(&options, "document")?; | ||
| let document: Document = document_wasm.clone().into(); | ||
|
|
||
| // Guard: replace requires revision > INITIAL_REVISION. Revisions 0 and 1 (and a | ||
| // missing revision) all indicate the caller really wants create semantics and are | ||
| // rejected here so we never accidentally produce a create transition from a | ||
| // replace call. | ||
| let revision = document.revision().ok_or_else(|| { | ||
| WasmSdkError::invalid_argument( | ||
| "Document must have a revision set for replace. Use prepareDocumentCreate for new documents.", | ||
| ) | ||
| })?; | ||
| if revision <= INITIAL_REVISION { | ||
| return Err(WasmSdkError::invalid_argument(format!( | ||
| "Document revision is {} but replace requires revision > {}. Use prepareDocumentCreate for new documents.", | ||
| revision, INITIAL_REVISION, | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Revision guards are inconsistent between prepareDocument and documentCreate/documentReplace*
prepare_document_create rejects documents with revision != INITIAL_REVISION (lines 495-502) and prepare_document_replace rejects documents with revision <= INITIAL_REVISION (lines 616-626). These guards are correct — build_document_create_or_replace_transition routes by revision and silently picks the wrong branch otherwise. The existing document_create (lines 119-176) and document_replace (lines 236-) call put_to_platform_and_wait_for_response with no equivalent guards, so the same misuse silently produces the wrong transition kind in the all-in-one path. Lift the same guards into the one-shot APIs (or factor them into a shared validator) so both paths reject the same inputs.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 487-626: Revision guards are inconsistent between prepareDocument* and documentCreate/documentReplace
`prepare_document_create` rejects documents with `revision != INITIAL_REVISION` (lines 495-502) and `prepare_document_replace` rejects documents with `revision <= INITIAL_REVISION` (lines 616-626). These guards are correct — `build_document_create_or_replace_transition` routes by revision and silently picks the wrong branch otherwise. The existing `document_create` (lines 119-176) and `document_replace` (lines 236-) call `put_to_platform_and_wait_for_response` with no equivalent guards, so the same misuse silently produces the wrong transition kind in the all-in-one path. Lift the same guards into the one-shot APIs (or factor them into a shared validator) so both paths reject the same inputs.
| describe('prepareDocument* transition kind selection', () => { | ||
| // The prepare* APIs return a signed StateTransition without broadcasting. | ||
| // For document ops this is always a Batch state transition; the inner | ||
| // BatchedTransition encodes whether it's a create / replace / delete. | ||
| // DocumentTransitionActionType numeric codes (see wasm-dpp2 | ||
| // document_transition.rs): Create=0, Replace=1, Delete=2. | ||
| const DOC_TRANSITION_CREATE = 0; | ||
| const DOC_TRANSITION_REPLACE = 1; | ||
| const DOC_TRANSITION_DELETE = 2; | ||
|
|
||
| function firstDocTransition(st) { | ||
| expect(st.actionType).to.equal('Batch'); | ||
| const batch = sdk.BatchTransition.fromStateTransition(st); | ||
| const inner = batch.transitions[0].toTransition(); | ||
| return inner; | ||
| } | ||
|
|
||
| it('prepareDocumentCreate produces a Create batched transition', async () => { | ||
| expect(testContractId).to.exist(); | ||
| const { signer, identityKey } = createTestSignerAndKey(sdk, 1, 2); | ||
|
|
||
| const document = new sdk.Document({ | ||
| properties: { message: 'prepare create' }, | ||
| documentTypeName: 'mutableNote', | ||
| revision: 1, | ||
| dataContractId: testContractId, | ||
| ownerId: testData.identityId, | ||
| }); | ||
|
|
||
| const st = await client.prepareDocumentCreate({ | ||
| document, | ||
| identityKey, | ||
| signer, | ||
| }); | ||
|
|
||
| const docTransition = firstDocTransition(st); | ||
| expect(docTransition.actionTypeNumber).to.equal(DOC_TRANSITION_CREATE); | ||
| }); | ||
|
|
||
| it('prepareDocumentReplace produces a Replace batched transition', async () => { | ||
| expect(testContractId).to.exist(); | ||
| const { signer, identityKey } = createTestSignerAndKey(sdk, 1, 2); | ||
|
|
||
| // Create a document first so we have a real ID to target. | ||
| const seedDoc = new sdk.Document({ | ||
| properties: { message: 'prepare replace seed' }, | ||
| documentTypeName: 'mutableNote', | ||
| revision: 1, | ||
| dataContractId: testContractId, | ||
| ownerId: testData.identityId, | ||
| }); | ||
| await client.documentCreate({ document: seedDoc, identityKey, signer }); | ||
| await new Promise((resolve) => { setTimeout(resolve, 2000); }); | ||
|
|
||
| const replaceDoc = new sdk.Document({ | ||
| id: seedDoc.id, | ||
| properties: { message: 'prepare replace updated' }, | ||
| documentTypeName: 'mutableNote', | ||
| revision: 2, | ||
| dataContractId: testContractId, | ||
| ownerId: testData.identityId, | ||
| }); | ||
|
|
||
| const st = await client.prepareDocumentReplace({ | ||
| document: replaceDoc, | ||
| identityKey, | ||
| signer, | ||
| }); | ||
|
|
||
| const docTransition = firstDocTransition(st); | ||
| expect(docTransition.actionTypeNumber).to.equal(DOC_TRANSITION_REPLACE); | ||
| }); | ||
|
|
||
| it('prepareDocumentDelete accepts a Document instance and produces a Delete transition', async () => { | ||
| expect(testContractId).to.exist(); | ||
| const { signer, identityKey } = createTestSignerAndKey(sdk, 1, 2); | ||
|
|
||
| const seedDoc = new sdk.Document({ | ||
| properties: { message: 'prepare delete seed (Document)' }, | ||
| documentTypeName: 'mutableNote', | ||
| revision: 1, | ||
| dataContractId: testContractId, | ||
| ownerId: testData.identityId, | ||
| }); | ||
| await client.documentCreate({ document: seedDoc, identityKey, signer }); | ||
| await new Promise((resolve) => { setTimeout(resolve, 2000); }); | ||
|
|
||
| const st = await client.prepareDocumentDelete({ | ||
| document: seedDoc, | ||
| identityKey, | ||
| signer, | ||
| }); | ||
|
|
||
| const docTransition = firstDocTransition(st); | ||
| expect(docTransition.actionTypeNumber).to.equal(DOC_TRANSITION_DELETE); | ||
| }); | ||
|
|
||
| it('prepareDocumentDelete accepts a plain identifier object and produces a Delete transition', async () => { | ||
| expect(testContractId).to.exist(); | ||
| const { signer, identityKey } = createTestSignerAndKey(sdk, 1, 2); | ||
|
|
||
| const seedDoc = new sdk.Document({ | ||
| properties: { message: 'prepare delete seed (object)' }, | ||
| documentTypeName: 'mutableNote', | ||
| revision: 1, | ||
| dataContractId: testContractId, | ||
| ownerId: testData.identityId, | ||
| }); | ||
| await client.documentCreate({ document: seedDoc, identityKey, signer }); | ||
| await new Promise((resolve) => { setTimeout(resolve, 2000); }); | ||
|
|
||
| const st = await client.prepareDocumentDelete({ | ||
| document: { | ||
| id: seedDoc.id, | ||
| ownerId: testData.identityId, | ||
| dataContractId: testContractId, | ||
| documentTypeName: 'mutableNote', | ||
| }, | ||
| identityKey, | ||
| signer, | ||
| }); | ||
|
|
||
| const docTransition = firstDocTransition(st); | ||
| expect(docTransition.actionTypeNumber).to.equal(DOC_TRANSITION_DELETE); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 Suggestion: Functional tests don't exercise the PR's central idempotency claim
The new functional tests only assert that prepareDocument* returns a Batch transition with the expected inner action type. That is necessary but not sufficient for the PR's central claim — that a prepared ST can be cached, broadcast, and (on timeout) rebroadcast without the platform creating a duplicate or rejecting it. There are no assertions that broadcastStateTransition(st) succeeds for a prepared ST, that st.toBytes() → StateTransition.fromBytes(...) → broadcast yields the same on-chain effect, or that a second broadcast of the same ST is accepted as a duplicate. These behaviors are what users will rely on; the manual yappr testing referenced in the PR description should be encoded as automated tests.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/tests/functional/transitions/documents.spec.ts`:
- [SUGGESTION] lines 284-409: Functional tests don't exercise the PR's central idempotency claim
The new functional tests only assert that `prepareDocument*` returns a Batch transition with the expected inner action type. That is necessary but not sufficient for the PR's central claim — that a prepared ST can be cached, broadcast, and (on timeout) rebroadcast without the platform creating a duplicate or rejecting it. There are no assertions that `broadcastStateTransition(st)` succeeds for a prepared ST, that `st.toBytes()` → `StateTransition.fromBytes(...)` → broadcast yields the same on-chain effect, or that a second broadcast of the same ST is accepted as a duplicate. These behaviors are what users will rely on; the manual yappr testing referenced in the PR description should be encoded as automated tests.
| // Check if it's a Document instance or a plain object with fields | ||
| let (document_id, owner_id, contract_id, document_type_name): ( | ||
| Identifier, | ||
| Identifier, | ||
| Identifier, | ||
| String, | ||
| ) = if get_class_type(&document_js).ok().as_deref() == Some("Document") { | ||
| let doc: DocumentWasm = document_js | ||
| .to_wasm::<DocumentWasm>("Document") | ||
| .map(|boxed| (*boxed).clone())?; | ||
| let doc_inner: Document = doc.clone().into(); | ||
| ( | ||
| doc.id().into(), | ||
| doc_inner.owner_id(), | ||
| doc.data_contract_id().into(), | ||
| doc.document_type_name(), | ||
| ) | ||
| } else { | ||
| ( | ||
| IdentifierWasm::try_from_options(&document_js, "id")?.into(), | ||
| IdentifierWasm::try_from_options(&document_js, "ownerId")?.into(), | ||
| IdentifierWasm::try_from_options(&document_js, "dataContractId")?.into(), | ||
| try_from_options_with(&document_js, "documentTypeName", |v| { | ||
| try_to_string(v, "documentTypeName") | ||
| })?, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
💬 Nitpick: prepareDocumentDelete reads ownerId from Document instance but requires it in options for plain object
When the caller passes a Document instance, owner_id is read from the inner Document via doc_inner.owner_id() and any wrapper-level ownerId is ignored. When the caller passes a plain object, ownerId must be set explicitly. This asymmetry is not documented in the TypeScript interface at document.rs:681-697 — the comment says "either a Document instance or an object with identifiers" but does not note that the Document branch ignores any options-level ownerId. Add a one-line clarification in the TS doc.
source: ['claude']
|
Addressed the follow-up review items on the branch in ed7dc8c. What changed:
I did not add custom Batch-specific validation in wasm-sdk because that would diverge from current rs-sdk behavior; the fix there was to make the docs/comments honest about today's behavior. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR is largely sound and the new prepare-document APIs mirror the existing rs-sdk flow closely. I verified the flagged paths against the current source and found one real nonce-cache issue in prepareDocumentDelete; the other findings either describe intentionally unreachable/private code, a documented test choice, or behavior inherited from the pre-existing one-shot delete boundary rather than a new defect in this PR.
Reviewed commit: ed7dc8c
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 785-798: prepareDocumentDelete leaks the local nonce cache on post-bump failures
`prepareDocumentDelete` advances the identity-contract nonce inside `DocumentDeleteTransitionBuilder::sign()` via `get_identity_contract_nonce(..., true, ...)`, but unlike the create/replace prepare paths it never refreshes that cache if a later step fails. There are two leak paths here: `builder.sign(...).await?` can return an error after the nonce has already been bumped, and `validate_state_transition_structure(...)` also returns directly without a refresh. `prepareDocumentCreate` and `prepareDocumentReplace` explicitly refresh on both construction/signing failure and structure-validation failure, so delete is now the only asymmetric path. Today `Batch` structure validation is an `UnsupportedFeatureError` no-op in DPP, but the bug becomes user-visible as soon as batch validation is implemented or if delete signing fails after nonce allocation.
| let state_transition = builder | ||
| .sign( | ||
| self.inner_sdk(), | ||
| &identity_key, | ||
| &signer, | ||
| self.inner_sdk().version(), | ||
| ) | ||
| .await?; | ||
|
|
||
| // Validate structure before handing the ST back, mirroring rs-sdk's | ||
| // pre-broadcast check. For document Batch transitions this currently | ||
| // ends up as a no-op because DPP returns UnsupportedFeatureError until | ||
| // that structure validation is implemented there. | ||
| validate_state_transition_structure(&state_transition, self.inner_sdk().version())?; |
There was a problem hiding this comment.
🟡 Suggestion: prepareDocumentDelete leaks the local nonce cache on post-bump failures
prepareDocumentDelete advances the identity-contract nonce inside DocumentDeleteTransitionBuilder::sign() via get_identity_contract_nonce(..., true, ...), but unlike the create/replace prepare paths it never refreshes that cache if a later step fails. There are two leak paths here: builder.sign(...).await? can return an error after the nonce has already been bumped, and validate_state_transition_structure(...) also returns directly without a refresh. prepareDocumentCreate and prepareDocumentReplace explicitly refresh on both construction/signing failure and structure-validation failure, so delete is now the only asymmetric path. Today Batch structure validation is an UnsupportedFeatureError no-op in DPP, but the bug becomes user-visible as soon as batch validation is implemented or if delete signing fails after nonce allocation.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [SUGGESTION] lines 785-798: prepareDocumentDelete leaks the local nonce cache on post-bump failures
`prepareDocumentDelete` advances the identity-contract nonce inside `DocumentDeleteTransitionBuilder::sign()` via `get_identity_contract_nonce(..., true, ...)`, but unlike the create/replace prepare paths it never refreshes that cache if a later step fails. There are two leak paths here: `builder.sign(...).await?` can return an error after the nonce has already been bumped, and `validate_state_transition_structure(...)` also returns directly without a refresh. `prepareDocumentCreate` and `prepareDocumentReplace` explicitly refresh on both construction/signing failure and structure-validation failure, so delete is now the only asymmetric path. Today `Batch` structure validation is an `UnsupportedFeatureError` no-op in DPP, but the bug becomes user-visible as soon as batch validation is implemented or if delete signing fails after nonce allocation.
|
Addressed the latest automated follow-up on the current head in
So delete is no longer the odd one out if a prepare attempt fails after the nonce bump. |
3 participants
Issue
Closes #3090
Problem
The high-level document APIs (
documentCreate,documentReplace,documentDelete) in the WASM SDK atomically bundle nonce management, ST construction, signing, broadcasting, and waiting. On timeout, callers cannot rebroadcast the same signed ST — retrying creates a duplicate with a new nonce.Solution
Implements Option A (Two-Phase API) from the issue: add
prepare_*variants for each document operation that return a signedStateTransitionwithout broadcasting:prepareDocumentCreate()— build, sign, return STprepareDocumentReplace()— build, sign, return STprepareDocumentDelete()— build, sign, return STThese pair with the already-existing
broadcastStateTransition()andwaitForResponse()methods inbroadcast.rs.Usage Pattern
This gives applications full control over retry and caching strategy while leveraging Platform's built-in duplicate ST rejection.
Changes
packages/wasm-sdk/src/state_transitions/document.rs:prepareDocumentCreate()— builds and signs a create ST without broadcastingprepareDocumentReplace()— builds and signs a replace ST without broadcastingprepareDocumentDelete()— builds and signs a delete ST without broadcastingbuild_document_create_or_replace_transition()helper that replicates the ST construction logic fromPutDocument::put_to_platform(nonce fetch, transition creation, signing) without the broadcast stepTesting
The existing document operation tests validate the build/sign/broadcast pipeline. The prepare variants reuse the same construction logic, stopping before broadcast. The
broadcastStateTransitionandwaitForResponsemethods are already tested inbroadcast.rs. Manual testing with the yappr application (which prompted this issue) confirms the two-phase pattern works correctly.Validation
Build verification
# WASM SDK builds successfully with new prepare_* APIs cargo check -p wasm-sdkExisting test coverage
The
prepare_*methods reuse the same internal construction paths as the existing all-in-one document operations:prepareDocumentCreate/prepareDocumentReplacedelegate to the newbuild_document_create_or_replace_transition()helper, which mirrors the nonce-fetch → transition-build → sign pipeline fromPutDocument::put_to_platformprepareDocumentDeleteusesDocumentDeleteTransitionBuilder::sign(), the same builder used by the existingdocumentDeleteflowbroadcastStateTransition()andwaitForResponse()(the "execute" half) are already tested inbroadcast.rsExisting CI test suites (
cargo test -p wasm-sdk, platform integration tests) validate these shared code paths.Manual / integration testing
prepareDocumentCreate()returns a signedStateTransitiontoBytes(), cached, then broadcast viabroadcastStateTransition()Input validation
prepareDocumentReplacerejects documents withINITIAL_REVISION(guards against accidental create-as-replace)prepareDocumentCreatevalidates entropy is present and exactly 32 bytesprepareDocumentDeleteaccepts both fullDocumentinstances and minimal identifier objectsSummary by CodeRabbit