feat(rs-sdk): expose transition hash from state transition methods#3092
feat(rs-sdk): expose transition hash from state transition methods#3092thepastaclaw wants to merge 7 commits into
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:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/rs-sdk/src/platform/transition/state_transition_result.rs (1)
11-15: Consider derivingPartialEqandEqfor testability.Adding
PartialEqandEq(bounded onT: PartialEq/T: Eq) would make it straightforward to assert equality in tests without having to decompose into parts.Suggested change
-#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/transition/state_transition_result.rs` around lines 11 - 15, The StateTransitionResult struct should derive PartialEq and Eq for easier testing: add PartialEq and Eq to the derive list on StateTransitionResult and constrain implementations so these derives require T: PartialEq (and T: Eq for Eq). Update the derive macro on the struct (StateTransitionResult) to include PartialEq and Eq so tests can directly compare instances without manual decomposition.packages/rs-sdk/src/platform/documents/transitions/create.rs (1)
174-179: Consider propagating the transition hash through the result enums.All document transition methods (create, delete, replace, purchase, set_price) currently discard the transition hash via
into_inner(). To fully surface the hash to SDK consumers — which is the goal of this PR — these per-operation result enums (e.g.,DocumentCreateResult) could carry the[u8; 32]alongside the payload, or the methods could returnStateTransitionResult<DocumentCreateResult>directly. This can be a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/documents/transitions/create.rs` around lines 174 - 179, The DocumentCreateResult enum currently drops the transition hash (via into_inner()), so update the API to propagate the transition hash: either add a hash field to the enum (e.g., change DocumentCreateResult::Document(Document) to carry (Document, [u8; 32]) or similar) and adjust all callers that construct/consume DocumentCreateResult, or instead change the create method(s) to return StateTransitionResult<DocumentCreateResult> directly so the hash is preserved; apply the same pattern for the other per-operation enums/methods (delete, replace, purchase, set_price) and ensure functions that call into_inner() are updated to handle the preserved hash accordingly.
🤖 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/rs-sdk/src/platform/tokens/transitions/unfreeze.rs`:
- Around line 69-70: The broadcast call hardcodes None for settings causing
caller-provided settings to be ignored; change the unfreeze transition to
extract settings from the builder (use
unfreeze_tokens_transition_builder.settings or the builder’s with_settings()
value) and pass that into
state_transition.broadcast_and_wait::<StateTransitionProofResult>(self,
put_settings) instead of None; apply the same fix pattern used in
destroy_frozen_funds (and replicate for mint.rs, freeze.rs,
set_price_for_direct_purchase.rs) so user_fee_increase and retry configuration
are honored.
---
Nitpick comments:
In `@packages/rs-sdk/src/platform/documents/transitions/create.rs`:
- Around line 174-179: The DocumentCreateResult enum currently drops the
transition hash (via into_inner()), so update the API to propagate the
transition hash: either add a hash field to the enum (e.g., change
DocumentCreateResult::Document(Document) to carry (Document, [u8; 32]) or
similar) and adjust all callers that construct/consume DocumentCreateResult, or
instead change the create method(s) to return
StateTransitionResult<DocumentCreateResult> directly so the hash is preserved;
apply the same pattern for the other per-operation enums/methods (delete,
replace, purchase, set_price) and ensure functions that call into_inner() are
updated to handle the preserved hash accordingly.
In `@packages/rs-sdk/src/platform/transition/state_transition_result.rs`:
- Around line 11-15: The StateTransitionResult struct should derive PartialEq
and Eq for easier testing: add PartialEq and Eq to the derive list on
StateTransitionResult and constrain implementations so these derives require T:
PartialEq (and T: Eq for Eq). Update the derive macro on the struct
(StateTransitionResult) to include PartialEq and Eq so tests can directly
compare instances without manual decomposition.
|
Addressed the CodeRabbit review in 22fc89c: Fixed:
Deferred:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/transition/state_transition_result.rs (1)
27-29: Nit: consider returning&[u8; 32]fromtransition_hash().Currently returns by value (copy). Since
[u8; 32]isCopythis is fine and arguably simpler, but returning a reference would be more conventional for accessor methods on fixed-size data and avoids a 32-byte copy on non-optimized builds. Totally optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/transition/state_transition_result.rs` around lines 27 - 29, The accessor transition_hash() currently returns [u8; 32] by value which copies the array; change its signature to return a reference -> &[u8; 32] and return &self.transition_hash to avoid the copy (update the method in the impl for the relevant struct). After changing the signature, update any call sites that rely on ownership (e.g., places that expect a copied array should explicitly clone/copy if needed) and run tests to ensure no API breakages.
🤖 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/rs-sdk/src/platform/transition/state_transition_result.rs`:
- Around line 27-29: The accessor transition_hash() currently returns [u8; 32]
by value which copies the array; change its signature to return a reference ->
&[u8; 32] and return &self.transition_hash to avoid the copy (update the method
in the impl for the relevant struct). After changing the signature, update any
call sites that rely on ownership (e.g., places that expect a copied array
should explicitly clone/copy if needed) and run tests to ensure no API
breakages.
shumkov
left a comment
shumkov
left a comment
There was a problem hiding this comment.
It doesn't make sense to me. You have already state transition around so you can store hash into varibable and reuse it. @PastaPastaPasta please review your friend's PRs first.
|
Thanks for the review @shumkov. You make a fair point — if the caller already has the state transition, they can compute the hash themselves via The motivation was convenience for the WASM SDK layer (where thephez reported needing the txid from Happy to close this if the consensus is that it's unnecessary overhead. @PastaPastaPasta thoughts? |
|
Shumkov and claw most times you use the SDK you don't actually have the transition? It gets built internally I think. I see no reason to not return txid. |
|
@shumkov — I want to respectfully push back on this. You're right that inside the SDK, the state transition exists as a local variable. But that's exactly the point — it's local to the SDK internals. The caller never sees it. Look at pub async fn document_create(...) -> Result<DocumentCreateResult, Error> {
let state_transition = builder.sign(...).await?; // built internally
let proof_result = state_transition.broadcast_and_wait(self, settings).await?;
match proof_result { ... } // consumed internally — only proof result returned
}The So "you already have the state transition around" is true for someone modifying the SDK, but false for SDK consumers (WASM SDK, js-evo-sdk, application code). Without this PR (or similar), there's no way for WASM SDK callers to get the transaction ID after a The As Pasta noted: "most times you use the SDK you don't actually have the transition. It gets built internally." This PR fixes that gap. |
|
Good catch — the Re: T including transition hash: That's a good idea. The challenge is that |
|
Re: @PastaPastaPasta's inline comments: On On including transition hash in |
thepastaclaw
force-pushed
the
feat/sdk-transition-hash
branch
from
22fc89c to
3b57def
Compare
|
Done — split into its own PR: #3132 Removed the put_settings changes from this PR. Re: T including transition hash: Looked into this. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-sdk/src/platform/transition/transfer_to_addresses.rs (1)
23-27:⚠️ Potential issue | 🟡 MinorStale duplicate bullet in the
Returnsdoc-comment.The return type is a 2-tuple
(AddressInfos, Credits)but the doc lists three bullets, repeating "Proof-backed address infos for provided recipients" as both the first and third items.📝 Proposed fix
- /// Returns tuple of: - /// * Proof-backed address infos for provided recipients - /// * Updated identity balance - /// * Proof-backed address infos for provided recipients + /// Returns tuple of: + /// * Proof-backed address infos for provided recipients + /// * Updated identity balance after the transfer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/transition/transfer_to_addresses.rs` around lines 23 - 27, The Returns doc-comment in transfer_to_addresses.rs currently lists three bullets and repeats "Proof-backed address infos for provided recipients" although the function returns a 2-tuple (AddressInfos, Credits); update the doc-comment for the function (e.g., the transfer_to_addresses function) to list exactly two return bullets matching the actual return types: 1) Proof-backed address infos for provided recipients (AddressInfos) and 2) Updated identity balance (Credits), removing the duplicated third bullet and keeping wording consistent with the return type names.
🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/transition/top_up_identity_from_addresses.rs (1)
84-116: Transition hash is silently discarded — high-level helpers won't propagate it to WASM/JS callers.All five high-level SDK methods updated in this PR (
top_up_from_addresses_with_nonce,token_freeze,document_transfer,transfer_credits_to_addresses,put_with_address_funding) call.into_inner()immediately and throw away the 32-byte transition hash. The hash is only observable from callers who invokebroadcast_and_waitdirectly — but the PR's stated motivation is specifically that "WASM SDK, js-evo-sdk, application code do not have access to the internal StateTransition". Callers using these high-level SDK methods, including the WASM SDK thin layer built on top of them, will still receive no hash.Consider either:
- Returning
(ReturnType, [u8; 32])from each high-level method, or- Defining a higher-level counterpart of
StateTransitionResult<T>that wraps the domain-specific result types (FreezeResult,DocumentTransferResult, etc.)If propagation is intentionally deferred, tracking the gap in a follow-up issue would help ensure it's not forgotten.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/transition/top_up_identity_from_addresses.rs` around lines 84 - 116, The code discards the 32-byte transition hash by calling .into_inner() on broadcast_and_wait results; update top_up_from_addresses_with_nonce (and the other high-level methods) to capture the transition hash before into_inner() and propagate it to callers—either by returning a tuple like (address_infos, balance, transition_hash) from the handler that matches StateTransitionProofResult::VerifiedIdentityWithAddressInfos (use the existing collect_address_infos_from_proof and identity.balance logic) or by introducing a small wrapper type (e.g., StateTransitionResult<T> containing result and [u8;32]) and returning that instead so the transition hash from broadcast_and_wait is not lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/rs-sdk/src/platform/transition/transfer_to_addresses.rs`:
- Around line 23-27: The Returns doc-comment in transfer_to_addresses.rs
currently lists three bullets and repeats "Proof-backed address infos for
provided recipients" although the function returns a 2-tuple (AddressInfos,
Credits); update the doc-comment for the function (e.g., the
transfer_to_addresses function) to list exactly two return bullets matching the
actual return types: 1) Proof-backed address infos for provided recipients
(AddressInfos) and 2) Updated identity balance (Credits), removing the
duplicated third bullet and keeping wording consistent with the return type
names.
---
Nitpick comments:
In `@packages/rs-sdk/src/platform/transition/top_up_identity_from_addresses.rs`:
- Around line 84-116: The code discards the 32-byte transition hash by calling
.into_inner() on broadcast_and_wait results; update
top_up_from_addresses_with_nonce (and the other high-level methods) to capture
the transition hash before into_inner() and propagate it to callers—either by
returning a tuple like (address_infos, balance, transition_hash) from the
handler that matches
StateTransitionProofResult::VerifiedIdentityWithAddressInfos (use the existing
collect_address_infos_from_proof and identity.balance logic) or by introducing a
small wrapper type (e.g., StateTransitionResult<T> containing result and
[u8;32]) and returning that instead so the transition hash from
broadcast_and_wait is not lost.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/wasm-sdk/src/state_transitions/broadcast.rs (1)
94-100:⚠️ Potential issue | 🟠 Major
transition_hashis silently discarded — consider surfacing it to JS callers.
.into_inner()correctly satisfies theconvert_proof_resulttype, but it throws away thetransition_hashthatStateTransitionResult<T>carries. The PR discussion explicitly identifies WASM SDK consumers (js-evo-sdk, browser apps) as the primary beneficiaries of having the transaction hash, yet those callers receive no way to observe it frombroadcastAndWait.A minimal approach would be to return a small two-field JS object that contains both the proof result and the hex-encoded hash, e.g.:
💡 Sketch of a hash-exposing return value
+use wasm_bindgen::JsValue; +use js_sys::Object; pub async fn broadcast_and_wait( &self, #[wasm_bindgen(js_name = "stateTransition")] state_transition: &StateTransitionWasm, settings: Option<PutSettingsJs>, -) -> Result<StateTransitionProofResultTypeJs, WasmSdkError> { +) -> Result<JsValue, WasmSdkError> { let st: StateTransition = state_transition.into(); let put_settings = parse_put_settings(settings)?; let result = st .broadcast_and_wait::<StateTransitionProofResult>(self.as_ref(), put_settings) .await .map_err(|e| WasmSdkError::generic(format!("Failed to broadcast: {}", e)))?; + let (inner, hash) = result.into_parts(); + let proof = convert_proof_result(inner).map_err(WasmSdkError::from)?; + let hash_hex = hex::encode(hash); + let obj = Object::new(); + js_sys::Reflect::set(&obj, &"result".into(), &proof.into())?; + js_sys::Reflect::set(&obj, &"transitionHash".into(), &hash_hex.into())?; + Ok(obj.into()) - convert_proof_result(result.into_inner()).map_err(WasmSdkError::from) }Alternatively, exposing a
broadcastAndWaitWithHashsibling method would avoid a breaking change tobroadcastAndWait's return type for existing callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-sdk/src/state_transitions/broadcast.rs` around lines 94 - 100, The code currently discards the transition_hash by calling result.into_inner() before convert_proof_result; update the broadcast path (the method invoking st.broadcast_and_wait::<StateTransitionProofResult> — the wasm-facing broadcastAndWait wrapper) to extract both the inner proof result and the transition_hash from the returned StateTransitionResult<T> (do not call into_inner() alone), then return both to JS either by changing the return to a small two-field object { proofResult, transitionHashHex } or by adding a sibling method broadcastAndWaitWithHash that returns that object; ensure you hex-encode the transition_hash, keep existing error handling (WasmSdkError) and still call convert_proof_result on the proof payload before packaging the result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/wasm-sdk/src/state_transitions/broadcast.rs`:
- Around line 94-100: The code currently discards the transition_hash by calling
result.into_inner() before convert_proof_result; update the broadcast path (the
method invoking st.broadcast_and_wait::<StateTransitionProofResult> — the
wasm-facing broadcastAndWait wrapper) to extract both the inner proof result and
the transition_hash from the returned StateTransitionResult<T> (do not call
into_inner() alone), then return both to JS either by changing the return to a
small two-field object { proofResult, transitionHashHex } or by adding a sibling
method broadcastAndWaitWithHash that returns that object; ensure you hex-encode
the transition_hash, keep existing error handling (WasmSdkError) and still call
convert_proof_result on the proof payload before packaging the result.
9aba70e to
07b2e5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/transition.rs (1)
19-19: Optional:state_transition_resultmodule declaration is out of alphabetical order.It is placed after
transfer_to_addresses(line 18) rather than betweenput_settingsandtop_up_address, breaking the existing alphabetical grouping. Consider moving it to maintain consistency.♻️ Suggested placement
pub mod put_settings; +pub mod state_transition_result; pub mod top_up_address; ... -pub mod state_transition_result; // remove from line 19🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/platform/transition.rs` at line 19, The module declaration for state_transition_result is out of alphabetical order relative to the other pub mod lines; move the pub mod state_transition_result; declaration so it appears between pub mod put_settings; and pub mod top_up_address; (i.e., place state_transition_result between those two) to restore the alphabetical grouping and consistent ordering with transfer_to_addresses, put_settings, and top_up_address.
🤖 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/rs-sdk/src/platform/transition/state_transition_result.rs`:
- Line 11: Add PartialEq and Eq to the derive list for the StateTransitionResult
struct so tests can use assert_eq!; update the existing #[derive(Debug, Clone)]
line to #[derive(Debug, Clone, PartialEq, Eq)] (i.e., extend the derive on the
struct defined in state_transition_result.rs) and run cargo test to verify
equality-based assertions now work.
---
Nitpick comments:
In `@packages/rs-sdk/src/platform/transition.rs`:
- Line 19: The module declaration for state_transition_result is out of
alphabetical order relative to the other pub mod lines; move the pub mod
state_transition_result; declaration so it appears between pub mod put_settings;
and pub mod top_up_address; (i.e., place state_transition_result between those
two) to restore the alphabetical grouping and consistent ordering with
transfer_to_addresses, put_settings, and top_up_address.
4e49bd2 to
b2f3b42
Compare
…3092) The into_inner() calls on broadcast results require the StateTransitionResult type from PR dashpay#3092. Since this PR targets v3.1-dev which doesn't have dashpay#3092 merged yet, these calls break compilation. Reverting until dashpay#3092 lands.
|
Addressing the review comments:
|
thepastaclaw
force-pushed
the
feat/sdk-transition-hash
branch
from
8d45834 to
261c79f
Compare
thepastaclaw
force-pushed
the
feat/sdk-transition-hash
branch
2 times, most recently
from
28721d3 to
cb08e5e
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Well-structured PR that cleanly introduces StateTransitionResult to bundle the transition hash with broadcast results. The core implementation is correct — the hash is computed deterministically before broadcast, the wrapper is simple, and all 28 RS-SDK callers are properly updated. The WASM SDK intentionally unwraps the hash for now (acknowledged in the PR as follow-up work). Two design-level suggestions around type safety and test coverage are worth addressing.
Reviewed commit: cb08e5e
🟡 3 suggestion(s) | 💬 2 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/broadcast.rs`:
- [SUGGESTION] line 99: WASM broadcastAndWait discards the transition hash before crossing the FFI boundary
The WASM SDK's public `broadcastAndWait` calls `result.into_inner()` at line 99, permanently discarding the transition hash before converting to `StateTransitionProofResultTypeJs`. JavaScript consumers have no way to access the transition hash through this path. The same pattern exists in `contract.rs:228-230` and `identity.rs:691-694`, where `broadcast_and_wait` is called and the entire `StateTransitionResult` is dropped.
If this is intentionally deferred to the #2953 follow-up, consider adding a brief `// TODO(#2953): propagate transition_hash to JS` comment so the gap is tracked alongside the code.
In `packages/rs-sdk/src/platform/transition/state_transition_result.rs`:
- [SUGGESTION] lines 1-56: No unit tests for StateTransitionResult<T>
The new type has 6 public methods (`new`, `transition_hash`, `into_parts`, `into_inner`, `map`) plus a `Deref` impl, but no `#[cfg(test)]` module. While the type is simple, unit tests would lock the contract — especially that `into_parts()` returns both components, that `map` preserves the hash, and that `Deref` provides transparent access to the inner value. This also guards against future refactors silently breaking the hash-carrying invariant.
- [SUGGESTION] line 14: Use the existing TxId newtype instead of raw [u8; 32] for transition_hash
The `transition_hash` field uses `[u8; 32]`, but a `TxId` newtype wrapping `[u8; 32]` already exists in `transition/txid.rs` and is re-exported from the same module (`pub use txid::TxId`). Using `TxId` would provide domain-level type safety, preventing accidental use of an arbitrary 32-byte array as a transition hash, and would give callers access to future `TxId` methods (like `is_confirmed`) without a conversion step.
|
Re-checked this draft on current head A couple of stale review threads are now cleaned up:
The remaining open items on this draft look like suggestions/nits rather than blockers (tests for @shumkov — your old changes-request still seems to be the only thing keeping |
|
Pushed
I left the |
Review GateCommit:
|
thepastaclaw
force-pushed
the
feat/sdk-transition-hash
branch
from
7088dae to
ffa5609
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds a StateTransitionResult<T> wrapper to BroadcastStateTransition::broadcast_and_wait so callers can recover the transition hash. Implementation at HEAD ffa5609 is sound and contained — no consensus-critical or security concerns. Validated findings are scope/quality nits: stale book signature, missing wiring test, Deref anti-pattern, and gaps where the new type isn't yet threaded through higher-level helpers (with TODO(#2953) tracking the WASM follow-up).
Reviewed commit: ffa5609
Fresh rerun of the review pipeline for this dispatcher item. An earlier same-SHA automated review already exists, so I am posting this current run as a top-level review to avoid duplicating inline threads while still recording the fresh verification.
🟡 5 suggestion(s) | 💬 2 nitpick(s)
🟡 suggestion: Book documents stale `broadcast_and_wait` signature
book/src/sdk/put-operations.md:186-190
Verified at lines 186-190: the trait excerpt still shows Result<T, Error>, but broadcast.rs:268 now returns Result<StateTransitionResult<T>, Error>. Since the book is the canonical reference for BroadcastStateTransition, leaving the old signature will mislead consumers who upgrade and try to match on T directly. Update the snippet and add a short note pointing readers at into_inner() / transition_hash() so the breaking change called out in the PR description is reflected in the docs.
💡 Suggested change
async fn broadcast_and_wait<T: TryFrom<StateTransitionProofResult> + Send>(
&self,
sdk: &Sdk,
settings: Option<PutSettings>,
) -> Result<StateTransitionResult<T>, Error>;
🟡 suggestion: No test verifies `broadcast_and_wait` returns the correct transition hash
packages/rs-sdk/src/platform/transition/broadcast.rs:264-285
Verified at state_transition_result.rs:58-103: the unit tests only cover the inert wrapper (new, into_parts, into_inner, map, Deref). The actual contract this PR introduces — that broadcast_and_wait returns transition_hash() == StateTransition::transaction_id() of the broadcast transition — is not exercised. Downstream consumers (WASM SDK, evo-sdk) will key on this hash to reconcile against wait_for_state_transition_result_request (broadcast_request.rs:81 uses the same transaction_id()), so a regression here would be silent. Add a mock-mode integration test asserting result.transition_hash() == state_transition.transaction_id()? so future refactors of transaction_id() cannot diverge unnoticed. The PR checklist also leaves the unit/integration tests box unchecked.
🟡 suggestion: `Deref` on a non-smart-pointer wrapper is a Rust API anti-pattern
packages/rs-sdk/src/platform/transition/state_transition_result.rs:50-56
Rust API Guidelines (C-DEREF) recommend Deref only for smart pointers. StateTransitionResult<T> is a value bundle, not a pointer-like wrapper. Two practical hazards: (1) any future method on T that collides with transition_hash, into_inner, into_parts, or map will silently shadow without a compiler error; (2) callers can keep using the inner result transparently and never get nudged toward the hash, partially defeating the point of the wrapper. An explicit .result() / .inner() accessor or AsRef<T> impl would surface the same ergonomics without the foot-guns.
🟡 suggestion: Higher-level `put_*_and_wait_for_response` helpers still discard the transition hash
packages/rs-sdk/src/platform/transition/put_identity.rs:88-107
Verified across put_identity.rs:88-107, put_contract.rs:81-93, put_document.rs, top_up_identity.rs, transfer.rs: these helpers do not call broadcast_and_wait — they call put_to_platform followed by Waitable::wait_for_response, returning bare domain values (Identity, DataContract, etc.). So the new StateTransitionResult<T> is stranded at the lowest layer and Rust callers of the ergonomic API still cannot retrieve the transaction ID without manually serializing the transition again. This is the natural follow-up to thread the hash through; doing it now would close the loop on the rs-sdk side rather than leaving a partial migration. Codex's two reports labelled this blocking; downgraded here because the PR scope is clearly the BroadcastStateTransition trait and the wrapper is non-breaking via into_inner()/Deref.
🟡 suggestion: WASM `broadcastAndWait` still drops the transition hash before crossing into JS
packages/wasm-sdk/src/state_transitions/broadcast.rs:94-102
Verified: the WASM binding immediately calls result.into_inner() and only converts the inner StateTransitionProofResult, so JavaScript callers cannot read the txid the new wrapper just plumbed through. There is, however, an explicit // TODO(#2953) on line 99 acknowledging this is deferred to a follow-up. Codex flagged this as blocking in two reports; downgraded to suggestion because the deferral is intentional and tracked. Worth confirming that #2953 is filed and linked in the PR description before merge so the follow-up is not lost.
💬 nitpick: `broadcast_and_wait` now serializes the transition an extra time to compute the hash
packages/rs-sdk/src/platform/transition/broadcast.rs:264-285
StateTransition::transaction_id() (rs-dpp state_transition/mod.rs:749) calls PlatformSerializable::serialize_to_bytes(self) and SHA-256s the result. With the new line 274, the same serialization runs in addition to the one inside broadcast_request_for_state_transition() and the trace/timeout paths in wait_for_response (line 250) and wait_for_state_transition_result_request (broadcast_request.rs:81). For BatchTransitions with many document operations the body is non-trivial and ends up being serialized 3-4 times per submission. Caching the hash once in broadcast_and_wait and threading it (or the bytes) into the inner calls would amortize the cost. Not blocking — the SDK is not a hot path — but the new wrapper makes the redundancy more visible.
💬 nitpick: Eager `transaction_id()?` changes failure ordering of `broadcast_and_wait`
packages/rs-sdk/src/platform/transition/broadcast.rs:274
Placing let transition_hash = self.transaction_id()?; before self.broadcast(...) means a serialization error now preempts broadcast and surfaces a ProtocolError-shaped error before any nonce/retry path runs. In practice transaction_id() cannot fail for a transition the caller could have broadcast anyway (broadcast itself serializes), so this is mostly observable in error message ordering and timing. If the eager check is intentional (so a broken transition fails fast without occupying the broadcast pipeline), a one-line comment would prevent future refactors from quietly moving it.
🤖 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 `book/src/sdk/put-operations.md`:
- [SUGGESTION] lines 186-190: Book documents stale `broadcast_and_wait` signature
Verified at lines 186-190: the trait excerpt still shows `Result<T, Error>`, but `broadcast.rs:268` now returns `Result<StateTransitionResult<T>, Error>`. Since the book is the canonical reference for `BroadcastStateTransition`, leaving the old signature will mislead consumers who upgrade and try to match on `T` directly. Update the snippet and add a short note pointing readers at `into_inner()` / `transition_hash()` so the breaking change called out in the PR description is reflected in the docs.
In `packages/rs-sdk/src/platform/transition/broadcast.rs`:
- [SUGGESTION] lines 264-285: No test verifies `broadcast_and_wait` returns the correct transition hash
Verified at `state_transition_result.rs:58-103`: the unit tests only cover the inert wrapper (`new`, `into_parts`, `into_inner`, `map`, `Deref`). The actual contract this PR introduces — that `broadcast_and_wait` returns `transition_hash() == StateTransition::transaction_id()` of the broadcast transition — is not exercised. Downstream consumers (WASM SDK, evo-sdk) will key on this hash to reconcile against `wait_for_state_transition_result_request` (`broadcast_request.rs:81` uses the same `transaction_id()`), so a regression here would be silent. Add a mock-mode integration test asserting `result.transition_hash() == state_transition.transaction_id()?` so future refactors of `transaction_id()` cannot diverge unnoticed. The PR checklist also leaves the unit/integration tests box unchecked.
In `packages/rs-sdk/src/platform/transition/state_transition_result.rs`:
- [SUGGESTION] lines 50-56: `Deref<Target = T>` on a non-smart-pointer wrapper is a Rust API anti-pattern
Rust API Guidelines (C-DEREF) recommend `Deref` only for smart pointers. `StateTransitionResult<T>` is a value bundle, not a pointer-like wrapper. Two practical hazards: (1) any future method on `T` that collides with `transition_hash`, `into_inner`, `into_parts`, or `map` will silently shadow without a compiler error; (2) callers can keep using the inner result transparently and never get nudged toward the hash, partially defeating the point of the wrapper. An explicit `.result()` / `.inner()` accessor or `AsRef<T>` impl would surface the same ergonomics without the foot-guns.
In `packages/rs-sdk/src/platform/transition/put_identity.rs`:
- [SUGGESTION] lines 88-107: Higher-level `put_*_and_wait_for_response` helpers still discard the transition hash
Verified across `put_identity.rs:88-107`, `put_contract.rs:81-93`, `put_document.rs`, `top_up_identity.rs`, `transfer.rs`: these helpers do not call `broadcast_and_wait` — they call `put_to_platform` followed by `Waitable::wait_for_response`, returning bare domain values (`Identity`, `DataContract`, etc.). So the new `StateTransitionResult<T>` is stranded at the lowest layer and Rust callers of the ergonomic API still cannot retrieve the transaction ID without manually serializing the transition again. This is the natural follow-up to thread the hash through; doing it now would close the loop on the rs-sdk side rather than leaving a partial migration. Codex's two reports labelled this `blocking`; downgraded here because the PR scope is clearly the `BroadcastStateTransition` trait and the wrapper is non-breaking via `into_inner()`/`Deref`.
In `packages/wasm-sdk/src/state_transitions/broadcast.rs`:
- [SUGGESTION] lines 94-102: WASM `broadcastAndWait` still drops the transition hash before crossing into JS
Verified: the WASM binding immediately calls `result.into_inner()` and only converts the inner `StateTransitionProofResult`, so JavaScript callers cannot read the txid the new wrapper just plumbed through. There is, however, an explicit `// TODO(#2953)` on line 99 acknowledging this is deferred to a follow-up. Codex flagged this as `blocking` in two reports; downgraded to suggestion because the deferral is intentional and tracked. Worth confirming that #2953 is filed and linked in the PR description before merge so the follow-up is not lost.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds a StateTransitionResult<T> wrapper exposing the deterministic transition hash from broadcast_and_wait and rewires all internal callers. Mechanically correct and security-clean, but the value never reaches any consumer in this PR — every site, including the WASM boundary, immediately calls .into_inner() and the higher-level put_*_and_wait_for_response helpers don't use broadcast_and_wait at all. Behavioral change is unit-tested only at the wrapper level, not at the broadcast path that actually computes the hash.
Reviewed commit: ffa5609
🟡 6 suggestion(s) | 💬 3 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/rs-sdk/src/platform/transition/top_up_identity.rs`:
- [SUGGESTION] lines 43-46: All higher-level SDK helpers immediately discard the new transition hash
`broadcast_and_wait` now returns `StateTransitionResult<T>`, but every internal caller — `top_up_identity` (line 44–46), `documents/transitions/create.rs:222–225`, the token/transfer/withdrawal helpers, and the WASM bindings — calls `.into_inner()` and returns the bare domain type. Worse, the `put_to_platform_and_wait_for_response` family used by most ergonomic call sites (`put_document.rs`, `put_contract.rs`, `put_identity.rs`, `vote.rs`, `dpns_usernames`, `dashpay::contact_request`) splits into `broadcast` + `wait_for_response` and never goes through `broadcast_and_wait`, so the wrapper never reaches them at all. Consumers using the documented top-level methods still cannot read the transaction ID, which is the original problem #2953 asks the SDK to solve. Either plumb the hash through `put_*_and_wait_for_response` so the change has at least one consumer-visible benefit, or narrow the scope/title to clarify that this PR only refactors the leaf `broadcast_and_wait` API.
In `packages/wasm-sdk/src/state_transitions/broadcast.rs`:
- [SUGGESTION] lines 85-101: Rust→WASM boundary strips the transition hash before JS can read it
`WasmSdk::broadcastAndWait` calls `result.into_inner()` and returns `Promise<StateTransitionProofResultType>`, dropping the new `[u8; 32]` before `wasm-bindgen` converts the value. The same pattern repeats in the higher-level WASM helpers (`contract.rs`, `identity.rs`) and `packages/js-evo-sdk/src/state-transitions/facade.ts` still types the result as `Promise<wasm.StateTransitionProofResultType>`. The hash is FFI-trivial (Copy, no allocation) and could be exposed today as a `Uint8Array`/hex field on the JS return type. The TODO(#2953) acknowledges this gap, but as written this PR's stated downstream consumer (the WASM SDK) gets no observable value from the change.
In `packages/rs-sdk/src/platform/transition/broadcast.rs`:
- [SUGGESTION] lines 264-285: `broadcast_and_wait`'s new hash wiring is not covered by any test
The added unit tests in `state_transition_result.rs` only exercise the wrapper's mechanical surface (`new`, `into_parts`, `into_inner`, `map`, `Deref`) using a hard-coded `[7; 32]`. None of them verify the load-bearing invariant of this PR: that the value placed in `transition_hash` is bit-for-bit `StateTransition::transaction_id()` of the broadcast transition, equal to the `state_transition_hash` keyed in `wait_for_state_transition_result_request` (broadcast_request.rs:81), and survives the `TryFrom<StateTransitionProofResult>` conversion. With every modified call site immediately calling `.into_inner()`, a future regression that drops or recomputes the hash post-broadcast would not be caught. Add a MockSdk-driven test in `broadcast.rs` (or `tests/fetch/broadcast.rs`) asserting `result.transition_hash() == st.transaction_id()?`.
- [SUGGESTION] lines 271-285: Transition hash is computed twice per `broadcast_and_wait` call
`self.transaction_id()?` at line 274 performs a full `PlatformSerializable::serialize_to_bytes` plus SHA-256. The subsequent `wait_for_response` reaches `wait_for_state_transition_result_request` (broadcast_request.rs:81), which calls `self.transaction_id()?` again — and that path runs once per retry attempt. For BatchTransition with many document/token operations or large DataContractCreate payloads this is non-trivial extra CPU and allocation in a hot path, paid by every caller even though all current callers discard the hash. Either thread the precomputed `transition_hash` into the wait-request path so the per-retry recomputation is eliminated, or keep the broadcast-path recomputation alone and have `broadcast_and_wait` reuse the value from the `WaitForStateTransitionResultRequestV0` it builds.
- [SUGGESTION] lines 277-285: Transition hash is dropped on the error path even after a successful broadcast
The hash is computed before broadcast (good — race-free), but if `wait_for_response` fails (timeout, proof error, network error, conversion failure) the precomputed `transition_hash` is silently discarded by the `?` propagation at lines 277/279 and the `result.map(...)` at 284. The single most useful application of an exposed transition hash is recovery: "the broadcast went out, but waiting failed — let me query by tx id later." That recovery path is impossible here because the hash never reaches the caller on `Err`. Consider returning the hash through a richer error variant (e.g. `Error::WaitFailed { transition_hash: [u8;32], source: Box<Error> }`) for the wait failure case, so the feature actually closes the gap it is meant to address.
In `packages/rs-sdk/src/platform/transition/state_transition_result.rs`:
- [SUGGESTION] lines 50-56: `Deref<Target = T>` on a value-bundle wrapper violates Rust API guidelines
Per the Rust API guidelines (C-DEREF), `Deref` is reserved for smart-pointer-like types (`Box`, `Arc`, `MutexGuard`, owning containers like `String`/`Vec`). `StateTransitionResult<T>` is a value bundle of `(T, [u8; 32])`, not a pointer. The hazards are well-known: method-resolution ambiguity if `T` ever gains a method whose name collides with one on the wrapper (`map`, `into_inner` are already at risk), confusing diagnostics for consumers (`&result` vs `&*result`), and silent semantic drift when a future method on `StateTransitionResult` shadows a `T` method that callers were using through deref coercion. Internally the rs-sdk doesn't rely on `Deref` — every modified call site uses `.into_inner()` explicitly — so removing it would not break anything in this PR. Prefer `AsRef<T>`/explicit `inner()`/`inner_mut()` accessors.
| let identity: PartialIdentity = state_transition | ||
| .broadcast_and_wait::<PartialIdentity>(sdk, settings) | ||
| .await? | ||
| .into_inner(); |
There was a problem hiding this comment.
🟡 Suggestion: All higher-level SDK helpers immediately discard the new transition hash
broadcast_and_wait now returns StateTransitionResult<T>, but every internal caller — top_up_identity (line 44–46), documents/transitions/create.rs:222–225, the token/transfer/withdrawal helpers, and the WASM bindings — calls .into_inner() and returns the bare domain type. Worse, the put_to_platform_and_wait_for_response family used by most ergonomic call sites (put_document.rs, put_contract.rs, put_identity.rs, vote.rs, dpns_usernames, dashpay::contact_request) splits into broadcast + wait_for_response and never goes through broadcast_and_wait, so the wrapper never reaches them at all. Consumers using the documented top-level methods still cannot read the transaction ID, which is the original problem #2953 asks the SDK to solve. Either plumb the hash through put_*_and_wait_for_response so the change has at least one consumer-visible benefit, or narrow the scope/title to clarify that this PR only refactors the leaf broadcast_and_wait API.
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/rs-sdk/src/platform/transition/top_up_identity.rs`:
- [SUGGESTION] lines 43-46: All higher-level SDK helpers immediately discard the new transition hash
`broadcast_and_wait` now returns `StateTransitionResult<T>`, but every internal caller — `top_up_identity` (line 44–46), `documents/transitions/create.rs:222–225`, the token/transfer/withdrawal helpers, and the WASM bindings — calls `.into_inner()` and returns the bare domain type. Worse, the `put_to_platform_and_wait_for_response` family used by most ergonomic call sites (`put_document.rs`, `put_contract.rs`, `put_identity.rs`, `vote.rs`, `dpns_usernames`, `dashpay::contact_request`) splits into `broadcast` + `wait_for_response` and never goes through `broadcast_and_wait`, so the wrapper never reaches them at all. Consumers using the documented top-level methods still cannot read the transaction ID, which is the original problem #2953 asks the SDK to solve. Either plumb the hash through `put_*_and_wait_for_response` so the change has at least one consumer-visible benefit, or narrow the scope/title to clarify that this PR only refactors the leaf `broadcast_and_wait` API.
| .await | ||
| .map_err(|e| WasmSdkError::generic(format!("Failed to broadcast: {}", e)))?; | ||
|
|
||
| convert_proof_result(result).map_err(WasmSdkError::from) | ||
| // TODO(#2953): The transition hash currently stops at the WASM/JS boundary here; | ||
| // propagate it through the JS-facing result type in the follow-up. | ||
| convert_proof_result(result.into_inner()).map_err(WasmSdkError::from) |
There was a problem hiding this comment.
🟡 Suggestion: Rust→WASM boundary strips the transition hash before JS can read it
WasmSdk::broadcastAndWait calls result.into_inner() and returns Promise<StateTransitionProofResultType>, dropping the new [u8; 32] before wasm-bindgen converts the value. The same pattern repeats in the higher-level WASM helpers (contract.rs, identity.rs) and packages/js-evo-sdk/src/state-transitions/facade.ts still types the result as Promise<wasm.StateTransitionProofResultType>. The hash is FFI-trivial (Copy, no allocation) and could be exposed today as a Uint8Array/hex field on the JS return type. The TODO(#2953) acknowledges this gap, but as written this PR's stated downstream consumer (the WASM SDK) gets no observable value from the change.
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/broadcast.rs`:
- [SUGGESTION] lines 85-101: Rust→WASM boundary strips the transition hash before JS can read it
`WasmSdk::broadcastAndWait` calls `result.into_inner()` and returns `Promise<StateTransitionProofResultType>`, dropping the new `[u8; 32]` before `wasm-bindgen` converts the value. The same pattern repeats in the higher-level WASM helpers (`contract.rs`, `identity.rs`) and `packages/js-evo-sdk/src/state-transitions/facade.ts` still types the result as `Promise<wasm.StateTransitionProofResultType>`. The hash is FFI-trivial (Copy, no allocation) and could be exposed today as a `Uint8Array`/hex field on the JS return type. The TODO(#2953) acknowledges this gap, but as written this PR's stated downstream consumer (the WASM SDK) gets no observable value from the change.
| Ok(_) => trace!("broadcast_and_wait: complete success"), | ||
| Err(e) => warn!(error = ?e, "broadcast_and_wait: failed"), | ||
| } | ||
| result | ||
| result.map(|inner| StateTransitionResult::new(inner, transition_hash)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: broadcast_and_wait's new hash wiring is not covered by any test
The added unit tests in state_transition_result.rs only exercise the wrapper's mechanical surface (new, into_parts, into_inner, map, Deref) using a hard-coded [7; 32]. None of them verify the load-bearing invariant of this PR: that the value placed in transition_hash is bit-for-bit StateTransition::transaction_id() of the broadcast transition, equal to the state_transition_hash keyed in wait_for_state_transition_result_request (broadcast_request.rs:81), and survives the TryFrom<StateTransitionProofResult> conversion. With every modified call site immediately calling .into_inner(), a future regression that drops or recomputes the hash post-broadcast would not be caught. Add a MockSdk-driven test in broadcast.rs (or tests/fetch/broadcast.rs) asserting result.transition_hash() == st.transaction_id()?.
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/rs-sdk/src/platform/transition/broadcast.rs`:
- [SUGGESTION] lines 264-285: `broadcast_and_wait`'s new hash wiring is not covered by any test
The added unit tests in `state_transition_result.rs` only exercise the wrapper's mechanical surface (`new`, `into_parts`, `into_inner`, `map`, `Deref`) using a hard-coded `[7; 32]`. None of them verify the load-bearing invariant of this PR: that the value placed in `transition_hash` is bit-for-bit `StateTransition::transaction_id()` of the broadcast transition, equal to the `state_transition_hash` keyed in `wait_for_state_transition_result_request` (broadcast_request.rs:81), and survives the `TryFrom<StateTransitionProofResult>` conversion. With every modified call site immediately calling `.into_inner()`, a future regression that drops or recomputes the hash post-broadcast would not be caught. Add a MockSdk-driven test in `broadcast.rs` (or `tests/fetch/broadcast.rs`) asserting `result.transition_hash() == st.transaction_id()?`.
| @@ -274,6 +281,6 @@ impl BroadcastStateTransition for StateTransition { | |||
| Ok(_) => trace!("broadcast_and_wait: complete success"), | |||
| Err(e) => warn!(error = ?e, "broadcast_and_wait: failed"), | |||
| } | |||
| result | |||
| result.map(|inner| StateTransitionResult::new(inner, transition_hash)) | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: Transition hash is computed twice per broadcast_and_wait call
self.transaction_id()? at line 274 performs a full PlatformSerializable::serialize_to_bytes plus SHA-256. The subsequent wait_for_response reaches wait_for_state_transition_result_request (broadcast_request.rs:81), which calls self.transaction_id()? again — and that path runs once per retry attempt. For BatchTransition with many document/token operations or large DataContractCreate payloads this is non-trivial extra CPU and allocation in a hot path, paid by every caller even though all current callers discard the hash. Either thread the precomputed transition_hash into the wait-request path so the per-retry recomputation is eliminated, or keep the broadcast-path recomputation alone and have broadcast_and_wait reuse the value from the WaitForStateTransitionResultRequestV0 it builds.
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/rs-sdk/src/platform/transition/broadcast.rs`:
- [SUGGESTION] lines 271-285: Transition hash is computed twice per `broadcast_and_wait` call
`self.transaction_id()?` at line 274 performs a full `PlatformSerializable::serialize_to_bytes` plus SHA-256. The subsequent `wait_for_response` reaches `wait_for_state_transition_result_request` (broadcast_request.rs:81), which calls `self.transaction_id()?` again — and that path runs once per retry attempt. For BatchTransition with many document/token operations or large DataContractCreate payloads this is non-trivial extra CPU and allocation in a hot path, paid by every caller even though all current callers discard the hash. Either thread the precomputed `transition_hash` into the wait-request path so the per-retry recomputation is eliminated, or keep the broadcast-path recomputation alone and have `broadcast_and_wait` reuse the value from the `WaitForStateTransitionResultRequestV0` it builds.
| impl<T> Deref for StateTransitionResult<T> { | ||
| type Target = T; | ||
|
|
||
| fn deref(&self) -> &T { | ||
| &self.inner | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Deref<Target = T> on a value-bundle wrapper violates Rust API guidelines
Per the Rust API guidelines (C-DEREF), Deref is reserved for smart-pointer-like types (Box, Arc, MutexGuard, owning containers like String/Vec). StateTransitionResult<T> is a value bundle of (T, [u8; 32]), not a pointer. The hazards are well-known: method-resolution ambiguity if T ever gains a method whose name collides with one on the wrapper (map, into_inner are already at risk), confusing diagnostics for consumers (&result vs &*result), and silent semantic drift when a future method on StateTransitionResult shadows a T method that callers were using through deref coercion. Internally the rs-sdk doesn't rely on Deref — every modified call site uses .into_inner() explicitly — so removing it would not break anything in this PR. Prefer AsRef<T>/explicit inner()/inner_mut() accessors.
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/rs-sdk/src/platform/transition/state_transition_result.rs`:
- [SUGGESTION] lines 50-56: `Deref<Target = T>` on a value-bundle wrapper violates Rust API guidelines
Per the Rust API guidelines (C-DEREF), `Deref` is reserved for smart-pointer-like types (`Box`, `Arc`, `MutexGuard`, owning containers like `String`/`Vec`). `StateTransitionResult<T>` is a value bundle of `(T, [u8; 32])`, not a pointer. The hazards are well-known: method-resolution ambiguity if `T` ever gains a method whose name collides with one on the wrapper (`map`, `into_inner` are already at risk), confusing diagnostics for consumers (`&result` vs `&*result`), and silent semantic drift when a future method on `StateTransitionResult` shadows a `T` method that callers were using through deref coercion. Internally the rs-sdk doesn't rely on `Deref` — every modified call site uses `.into_inner()` explicitly — so removing it would not break anything in this PR. Prefer `AsRef<T>`/explicit `inner()`/`inner_mut()` accessors.
| @@ -274,6 +281,6 @@ impl BroadcastStateTransition for StateTransition { | |||
| Ok(_) => trace!("broadcast_and_wait: complete success"), | |||
| Err(e) => warn!(error = ?e, "broadcast_and_wait: failed"), | |||
| } | |||
| result | |||
| result.map(|inner| StateTransitionResult::new(inner, transition_hash)) | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: Transition hash is dropped on the error path even after a successful broadcast
The hash is computed before broadcast (good — race-free), but if wait_for_response fails (timeout, proof error, network error, conversion failure) the precomputed transition_hash is silently discarded by the ? propagation at lines 277/279 and the result.map(...) at 284. The single most useful application of an exposed transition hash is recovery: "the broadcast went out, but waiting failed — let me query by tx id later." That recovery path is impossible here because the hash never reaches the caller on Err. Consider returning the hash through a richer error variant (e.g. Error::WaitFailed { transition_hash: [u8;32], source: Box<Error> }) for the wait failure case, so the feature actually closes the gap it is meant to address.
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/rs-sdk/src/platform/transition/broadcast.rs`:
- [SUGGESTION] lines 277-285: Transition hash is dropped on the error path even after a successful broadcast
The hash is computed before broadcast (good — race-free), but if `wait_for_response` fails (timeout, proof error, network error, conversion failure) the precomputed `transition_hash` is silently discarded by the `?` propagation at lines 277/279 and the `result.map(...)` at 284. The single most useful application of an exposed transition hash is recovery: "the broadcast went out, but waiting failed — let me query by tx id later." That recovery path is impossible here because the hash never reaches the caller on `Err`. Consider returning the hash through a richer error variant (e.g. `Error::WaitFailed { transition_hash: [u8;32], source: Box<Error> }`) for the wait failure case, so the feature actually closes the gap it is meant to address.
| // Compute the transition hash deterministically BEFORE broadcast. | ||
| // This is a SHA-256 hash of the serialized StateTransition and does | ||
| // not depend on blockchain state, so there is no race condition. | ||
| let transition_hash = self.transaction_id()?; | ||
|
|
||
| trace!("broadcast_and_wait: step 1 - broadcasting"); | ||
| self.broadcast(sdk, settings).await?; |
There was a problem hiding this comment.
💬 Nitpick: Eager transaction_id()? changes failure ordering
Previously, all serialization happened inside broadcast / wait_for_response, where errors are wrapped in the retry/ExecutionError machinery and trigger refresh_identity_nonce recovery. With the new eager let transition_hash = self.transaction_id()?;, a serialization error surfaces before any broadcast attempt and bypasses both the retry and nonce-refresh paths. In practice both paths use PlatformSerializable::serialize_to_bytes, so failures here would also have been hit downstream — but the change in error wrapping/recovery semantics is worth confirming as intentional.
source: ['claude']
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct StateTransitionResult<T> { | ||
| inner: T, | ||
| transition_hash: [u8; 32], | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Add #[must_use] to the result wrapper
StateTransitionResult<T> is the public return type of every broadcast_and_wait call and the new public surface of this PR. Annotating the struct with #[must_use = "the transition hash and proof result should be inspected"] would catch accidentally ignored returns at the type level — relevant given the whole point of the change is that callers should be examining the wrapper.
💡 Suggested change
| #[derive(Debug, Clone, PartialEq, Eq)] | |
| pub struct StateTransitionResult<T> { | |
| inner: T, | |
| transition_hash: [u8; 32], | |
| } | |
| #[derive(Debug, Clone, PartialEq, Eq)] | |
| #[must_use = "the transition hash and proof result should be inspected"] | |
| pub struct StateTransitionResult<T> { | |
| inner: T, | |
| transition_hash: [u8; 32], | |
| } |
source: ['claude']
| /// Maps the inner value, preserving the transition hash. | ||
| pub fn map<U, F: FnOnce(T) -> U>(self, f: F) -> StateTransitionResult<U> { | ||
| StateTransitionResult { | ||
| inner: f(self.inner), | ||
| transition_hash: self.transition_hash, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: map method has no in-tree caller
StateTransitionResult::map is defined and unit-tested but no production code uses it — broadcast_and_wait constructs the wrapper at the leaf and downstream callers immediately .into_inner(). Drop it to keep the public surface minimal until a real consumer needs it.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Follow-up successfully addressed the prior major review items: Deref removed in favor of AsRef/AsMut + explicit accessors, the success path threads the precomputed transition_hash through to JS via BroadcastAndWaitResultWasm, and a new Error::WaitForStateTransitionResultFailedAfterBroadcast variant carries the hash through wait failures in broadcast_and_wait. Remaining items are non-blocking: the new *_with_transition_hash put/document/identity helpers still drop the hash on wait failure (defeating the new error variant), the WASM error mapping flattens that structured hash into a hex-encoded message string, the book documents the pre-PR signature, higher-level WASM bindings (contractPublish/Update, identityCreate/TopUp/Update) still throw the hash away, and there are several smaller wasm-bindgen / surface-area nits. No blocking issues.
Reviewed commit: 9582fc3
🟡 5 suggestion(s) | 💬 4 nitpick(s)
1 additional finding
🟡 suggestion: Book documents the pre-PR `broadcast_and_wait` signature
book/src/sdk/put-operations.md (lines 186-190)
The trait excerpt in the book still prints broadcast_and_wait -> Result<T, Error>, but packages/rs-sdk/src/platform/transition/broadcast.rs:197-201 now returns Result<StateTransitionResult<T>, Error>. Since the PR description calls this out as a breaking change and the book is the canonical narrative reference for BroadcastStateTransition, leaving the snippet stale will mislead consumers upgrading to this release and hides the new transitionHash / into_inner() behavior. The descriptive paragraph at line 200 ("Combines both -- broadcast, then wait.") also makes no mention of the hash, which is the entire point of the PR.
💡 Suggested change
async fn broadcast_and_wait<T: TryFrom<StateTransitionProofResult> + Send>(
&self,
sdk: &Sdk,
settings: Option<PutSettings>,
) -> Result<StateTransitionResult<T>, Error>;
🤖 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/rs-sdk/src/platform/transition/put_contract.rs`:
- [SUGGESTION] lines 48-57: New `*_with_transition_hash` helpers drop the hash on wait failure
The new `put_to_platform_and_wait_for_response_with_transition_hash` helpers compute `transition_hash` after the broadcast in `put_to_platform` has already gone out, then call `Waitable::wait_for_response(...).await?`. If the wait fails (timeout, proof error, conversion failure), the `?` strips the precomputed hash and returns the bare inner error, which is precisely the recovery scenario the new `Error::WaitForStateTransitionResultFailedAfterBroadcast { transition_hash, source }` variant was added to support in `broadcast.rs:271-294`. The same pattern repeats in `put_document.rs:71-75`, `put_identity.rs`, `purchase_document.rs`, `transfer_document.rs`, `update_price_of_document.rs`, plus `vote.rs` (and friends) via `Waitable::wait_for_response`. Wrap the wait error here with `wrap_wait_error_after_broadcast` (or have `Waitable::wait_for_response` return `StateTransitionResult` so the hash is preserved by construction). As-is the helpers only deliver the hash on the happy path — exactly the gap the rest of the PR was trying to close.
In `book/src/sdk/put-operations.md`:
- [SUGGESTION] lines 186-190: Book documents the pre-PR `broadcast_and_wait` signature
The trait excerpt in the book still prints `broadcast_and_wait -> Result<T, Error>`, but `packages/rs-sdk/src/platform/transition/broadcast.rs:197-201` now returns `Result<StateTransitionResult<T>, Error>`. Since the PR description calls this out as a breaking change and the book is the canonical narrative reference for `BroadcastStateTransition`, leaving the snippet stale will mislead consumers upgrading to this release and hides the new `transitionHash` / `into_inner()` behavior. The descriptive paragraph at line 200 ("Combines both -- broadcast, then wait.") also makes no mention of the hash, which is the entire point of the PR.
In `packages/wasm-sdk/src/error.rs`:
- [SUGGESTION] lines 183-195: WASM error variant flattens the structured `transition_hash` into a hex-encoded message
Rust preserves the txid as a typed `[u8; 32]` in `Error::WaitForStateTransitionResultFailedAfterBroadcast { transition_hash, source }` (`rs-sdk/src/error.rs:115-123`), which is exactly the recovery affordance this PR set out to provide ("broadcast went out, but waiting failed — query by tx id later"). At the WASM boundary, the variant is mapped to `WasmSdkError` and the hash is `hex::encode`d directly into `message`. `WasmSdkError`'s exported surface (`kind`, `name`, `message`, `code`, `isRetriable`, lines 249-313) has no slot for the hash, so JS consumers must regex-parse English text to recover it. Add a structured `transitionHash: Option<Vec<u8>>` field exposed via a `Uint8Array` getter (analogous to the existing `code` field, and mirroring `BroadcastAndWaitResultWasm.transitionHash`) and populate it for this variant. The success path crosses cleanly; the operationally most-interesting path still does not.
In `packages/wasm-sdk/src/state_transitions/contract.rs`:
- [SUGGESTION] lines 103-234: Higher-level WASM helpers still drop the transition hash before the JS boundary
The leaf `WasmSdk::broadcastAndWait` correctly forwards the hash, but every higher-level wasm-bindgen entry point still discards it before crossing into JS, with `TODO(#2953)` comments acknowledging the gap: `contractPublish` (contract.rs:103-119), `contractUpdate` (contract.rs:228-234), `identityCreate` (identity.rs:115-129), `identityTopUp` (identity.rs:206-220), `identityCreditTransfer` (identity.rs:338-353), `identityCreditWithdrawal` (identity.rs:489-503), `identityUpdate` (identity.rs:697-700). Each of these now has a hash-carrying rs-sdk helper available (the `*_with_transition_hash` variants and the `broadcast_and_wait` `StateTransitionResult`), but JS callers using the ergonomic APIs still cannot recover the txid. Either thread `transitionHash` through these return values now, or ensure #2953 is filed and linked in the PR description so the work is not lost.
In `packages/rs-sdk/src/platform/transition/broadcast.rs`:
- [SUGGESTION] lines 271-294: No test asserts `broadcast_and_wait`'s hash equals `StateTransition::transaction_id()`
The PR's load-bearing invariant is that the hash placed in `StateTransitionResult::transition_hash` is bit-for-bit equal to `StateTransition::transaction_id()` of the broadcast transition (the same hash keyed in `wait_for_state_transition_result_request_with_hash`). The added tests cover (a) the wrapper's mechanical surface with hard-coded `[7; 32]`, (b) the error wrapper, and (c) the wait-request-with-hash builder — none traverse `broadcast_and_wait` end-to-end via `MockSdk` to assert `result.transition_hash() == st.transaction_id()?` and that the wait-failure path produces `WaitForStateTransitionResultFailedAfterBroadcast` carrying the same hash. With every internal call site immediately calling `.into_inner()`, a future regression that swaps in or recomputes a different hash post-broadcast would not be caught. The PR checklist still has the test-coverage box unchecked.
| ) -> Result<StateTransitionResult<DataContract>, Error> { | ||
| let state_transition = self | ||
| .put_to_platform(sdk, identity_public_key, signer, settings) | ||
| .await?; | ||
| let transition_hash = state_transition.transaction_id()?; | ||
| let contract = | ||
| <DataContract as Waitable>::wait_for_response(sdk, state_transition, settings).await?; | ||
|
|
||
| Ok(StateTransitionResult::new(contract, transition_hash)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: New *_with_transition_hash helpers drop the hash on wait failure
The new put_to_platform_and_wait_for_response_with_transition_hash helpers compute transition_hash after the broadcast in put_to_platform has already gone out, then call Waitable::wait_for_response(...).await?. If the wait fails (timeout, proof error, conversion failure), the ? strips the precomputed hash and returns the bare inner error, which is precisely the recovery scenario the new Error::WaitForStateTransitionResultFailedAfterBroadcast { transition_hash, source } variant was added to support in broadcast.rs:271-294. The same pattern repeats in put_document.rs:71-75, put_identity.rs, purchase_document.rs, transfer_document.rs, update_price_of_document.rs, plus vote.rs (and friends) via Waitable::wait_for_response. Wrap the wait error here with wrap_wait_error_after_broadcast (or have Waitable::wait_for_response return StateTransitionResult so the hash is preserved by construction). As-is the helpers only deliver the hash on the happy path — exactly the gap the rest of the PR was trying to close.
💡 Suggested change
| ) -> Result<StateTransitionResult<DataContract>, Error> { | |
| let state_transition = self | |
| .put_to_platform(sdk, identity_public_key, signer, settings) | |
| .await?; | |
| let transition_hash = state_transition.transaction_id()?; | |
| let contract = | |
| <DataContract as Waitable>::wait_for_response(sdk, state_transition, settings).await?; | |
| Ok(StateTransitionResult::new(contract, transition_hash)) | |
| } | |
| let state_transition = self | |
| .put_to_platform(sdk, identity_public_key, signer, settings) | |
| .await?; | |
| let transition_hash = state_transition.transaction_id()?; | |
| let contract = <DataContract as Waitable>::wait_for_response(sdk, state_transition, settings) | |
| .await | |
| .map_err(|source| Error::WaitForStateTransitionResultFailedAfterBroadcast { | |
| transition_hash, | |
| source: Box::new(source), | |
| })?; | |
| Ok(StateTransitionResult::new(contract, transition_hash)) |
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/rs-sdk/src/platform/transition/put_contract.rs`:
- [SUGGESTION] lines 48-57: New `*_with_transition_hash` helpers drop the hash on wait failure
The new `put_to_platform_and_wait_for_response_with_transition_hash` helpers compute `transition_hash` after the broadcast in `put_to_platform` has already gone out, then call `Waitable::wait_for_response(...).await?`. If the wait fails (timeout, proof error, conversion failure), the `?` strips the precomputed hash and returns the bare inner error, which is precisely the recovery scenario the new `Error::WaitForStateTransitionResultFailedAfterBroadcast { transition_hash, source }` variant was added to support in `broadcast.rs:271-294`. The same pattern repeats in `put_document.rs:71-75`, `put_identity.rs`, `purchase_document.rs`, `transfer_document.rs`, `update_price_of_document.rs`, plus `vote.rs` (and friends) via `Waitable::wait_for_response`. Wrap the wait error here with `wrap_wait_error_after_broadcast` (or have `Waitable::wait_for_response` return `StateTransitionResult` so the hash is preserved by construction). As-is the helpers only deliver the hash on the happy path — exactly the gap the rest of the PR was trying to close.
| WaitForStateTransitionResultFailedAfterBroadcast { | ||
| transition_hash, | ||
| source, | ||
| } => Self::new( | ||
| WasmSdkErrorKind::WaitForStateTransitionResultFailedAfterBroadcast, | ||
| format!( | ||
| "state transition broadcast succeeded for {} but waiting for the result failed: {}", | ||
| hex::encode(transition_hash), | ||
| source | ||
| ), | ||
| None, | ||
| retriable, | ||
| ), |
There was a problem hiding this comment.
🟡 Suggestion: WASM error variant flattens the structured transition_hash into a hex-encoded message
Rust preserves the txid as a typed [u8; 32] in Error::WaitForStateTransitionResultFailedAfterBroadcast { transition_hash, source } (rs-sdk/src/error.rs:115-123), which is exactly the recovery affordance this PR set out to provide ("broadcast went out, but waiting failed — query by tx id later"). At the WASM boundary, the variant is mapped to WasmSdkError and the hash is hex::encoded directly into message. WasmSdkError's exported surface (kind, name, message, code, isRetriable, lines 249-313) has no slot for the hash, so JS consumers must regex-parse English text to recover it. Add a structured transitionHash: Option<Vec<u8>> field exposed via a Uint8Array getter (analogous to the existing code field, and mirroring BroadcastAndWaitResultWasm.transitionHash) and populate it for this variant. The success path crosses cleanly; the operationally most-interesting path still does not.
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/error.rs`:
- [SUGGESTION] lines 183-195: WASM error variant flattens the structured `transition_hash` into a hex-encoded message
Rust preserves the txid as a typed `[u8; 32]` in `Error::WaitForStateTransitionResultFailedAfterBroadcast { transition_hash, source }` (`rs-sdk/src/error.rs:115-123`), which is exactly the recovery affordance this PR set out to provide ("broadcast went out, but waiting failed — query by tx id later"). At the WASM boundary, the variant is mapped to `WasmSdkError` and the hash is `hex::encode`d directly into `message`. `WasmSdkError`'s exported surface (`kind`, `name`, `message`, `code`, `isRetriable`, lines 249-313) has no slot for the hash, so JS consumers must regex-parse English text to recover it. Add a structured `transitionHash: Option<Vec<u8>>` field exposed via a `Uint8Array` getter (analogous to the existing `code` field, and mirroring `BroadcastAndWaitResultWasm.transitionHash`) and populate it for this variant. The success path crosses cleanly; the operationally most-interesting path still does not.
|
|
||
| // Broadcast the transition | ||
| use dash_sdk::dpp::state_transition::proof_result::StateTransitionProofResult; | ||
| // TODO(#2953): The transition hash currently stops at the WASM/JS boundary for | ||
| // contract update results; propagate it through the JS-facing API in the follow-up. | ||
| state_transition | ||
| .broadcast_and_wait::<StateTransitionProofResult>(self.inner_sdk(), settings) | ||
| .await?; |
There was a problem hiding this comment.
🟡 Suggestion: Higher-level WASM helpers still drop the transition hash before the JS boundary
The leaf WasmSdk::broadcastAndWait correctly forwards the hash, but every higher-level wasm-bindgen entry point still discards it before crossing into JS, with TODO(#2953) comments acknowledging the gap: contractPublish (contract.rs:103-119), contractUpdate (contract.rs:228-234), identityCreate (identity.rs:115-129), identityTopUp (identity.rs:206-220), identityCreditTransfer (identity.rs:338-353), identityCreditWithdrawal (identity.rs:489-503), identityUpdate (identity.rs:697-700). Each of these now has a hash-carrying rs-sdk helper available (the *_with_transition_hash variants and the broadcast_and_wait StateTransitionResult), but JS callers using the ergonomic APIs still cannot recover the txid. Either thread transitionHash through these return values now, or ensure #2953 is filed and linked in the PR description so the work is not lost.
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/contract.rs`:
- [SUGGESTION] lines 103-234: Higher-level WASM helpers still drop the transition hash before the JS boundary
The leaf `WasmSdk::broadcastAndWait` correctly forwards the hash, but every higher-level wasm-bindgen entry point still discards it before crossing into JS, with `TODO(#2953)` comments acknowledging the gap: `contractPublish` (contract.rs:103-119), `contractUpdate` (contract.rs:228-234), `identityCreate` (identity.rs:115-129), `identityTopUp` (identity.rs:206-220), `identityCreditTransfer` (identity.rs:338-353), `identityCreditWithdrawal` (identity.rs:489-503), `identityUpdate` (identity.rs:697-700). Each of these now has a hash-carrying rs-sdk helper available (the `*_with_transition_hash` variants and the `broadcast_and_wait` `StateTransitionResult`), but JS callers using the ergonomic APIs still cannot recover the txid. Either thread `transitionHash` through these return values now, or ensure #2953 is filed and linked in the PR description so the work is not lost.
| async fn broadcast_and_wait<T: TryFrom<StateTransitionProofResult> + Send>( | ||
| &self, | ||
| sdk: &Sdk, | ||
| settings: Option<PutSettings>, | ||
| ) -> Result<T, Error> { | ||
| ) -> Result<StateTransitionResult<T>, Error> { | ||
| trace!(state_transition = %self.name(), "broadcast_and_wait: start"); | ||
|
|
||
| // Compute the transition hash deterministically BEFORE broadcast. | ||
| // This is a SHA-256 hash of the serialized StateTransition and does | ||
| // not depend on blockchain state, so there is no race condition. | ||
| let transition_hash = self.transaction_id()?; | ||
|
|
||
| trace!("broadcast_and_wait: step 1 - broadcasting"); | ||
| self.broadcast(sdk, settings).await?; | ||
| trace!("broadcast_and_wait: step 2 - waiting for response"); | ||
| let result = self.wait_for_response::<T>(sdk, settings).await; | ||
| let result = wait_for_response_with_hash(self, sdk, settings, transition_hash).await; | ||
| match &result { | ||
| Ok(_) => trace!("broadcast_and_wait: complete success"), | ||
| Err(e) => warn!(error = ?e, "broadcast_and_wait: failed"), | ||
| } | ||
| result | ||
| .map(|inner| StateTransitionResult::new(inner, transition_hash)) | ||
| .map_err(|e| wrap_wait_error_after_broadcast(transition_hash, e)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No test asserts broadcast_and_wait's hash equals StateTransition::transaction_id()
The PR's load-bearing invariant is that the hash placed in StateTransitionResult::transition_hash is bit-for-bit equal to StateTransition::transaction_id() of the broadcast transition (the same hash keyed in wait_for_state_transition_result_request_with_hash). The added tests cover (a) the wrapper's mechanical surface with hard-coded [7; 32], (b) the error wrapper, and (c) the wait-request-with-hash builder — none traverse broadcast_and_wait end-to-end via MockSdk to assert result.transition_hash() == st.transaction_id()? and that the wait-failure path produces WaitForStateTransitionResultFailedAfterBroadcast carrying the same hash. With every internal call site immediately calling .into_inner(), a future regression that swaps in or recomputes a different hash post-broadcast would not be caught. The PR checklist still has the test-coverage box unchecked.
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/rs-sdk/src/platform/transition/broadcast.rs`:
- [SUGGESTION] lines 271-294: No test asserts `broadcast_and_wait`'s hash equals `StateTransition::transaction_id()`
The PR's load-bearing invariant is that the hash placed in `StateTransitionResult::transition_hash` is bit-for-bit equal to `StateTransition::transaction_id()` of the broadcast transition (the same hash keyed in `wait_for_state_transition_result_request_with_hash`). The added tests cover (a) the wrapper's mechanical surface with hard-coded `[7; 32]`, (b) the error wrapper, and (c) the wait-request-with-hash builder — none traverse `broadcast_and_wait` end-to-end via `MockSdk` to assert `result.transition_hash() == st.transaction_id()?` and that the wait-failure path produces `WaitForStateTransitionResultFailedAfterBroadcast` carrying the same hash. With every internal call site immediately calling `.into_inner()`, a future regression that swaps in or recomputes a different hash post-broadcast would not be caught. The PR checklist still has the test-coverage box unchecked.
| #[wasm_bindgen(typescript_custom_section)] | ||
| const BROADCAST_AND_WAIT_RESULT_TS: &str = r#" | ||
| export interface BroadcastAndWaitResult { | ||
| result: StateTransitionProofResultType; | ||
| transitionHash: Uint8Array; | ||
| } | ||
| "#; | ||
|
|
||
| #[wasm_bindgen] | ||
| extern "C" { | ||
| #[wasm_bindgen(typescript_type = "BroadcastAndWaitResult")] | ||
| pub type BroadcastAndWaitResultJs; | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: BroadcastAndWaitResultJs extern alias and typescript_custom_section are dead code
pub type BroadcastAndWaitResultJs; (line 31) is declared with #[wasm_bindgen(typescript_type = "BroadcastAndWaitResult")] but never appears as a parameter or return type — broadcast_and_wait returns BroadcastAndWaitResultWasm (line 134) which is exported as the JS class BroadcastAndWaitResult via js_class. The typescript_custom_section interface at lines 21-26 then collides with the wasm-bindgen-generated class declaration of the same name; TypeScript only accepts both via class/interface merging while their members agree, and any future getter change has to be manually mirrored in the interface. Either drop both (the class declaration is sufficient) or wire BroadcastAndWaitResultJs into the return type so the typed interface is actually emitted.
source: ['claude']
| self, | ||
| Error::DapiClientError(DapiClientError::NoAvailableAddresses) | ||
| | Error::DapiClientError(DapiClientError::NoAvailableAddressesToRetry(_)) | ||
| ) || matches!( | ||
| self, | ||
| Error::WaitForStateTransitionResultFailedAfterBroadcast { source, .. } | ||
| if source.is_no_available_addresses() | ||
| ) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: WaitForStateTransitionResultFailedAfterBroadcast retriability semantics deserve a doc note
Including the new variant in the retriable set means external callers looping on broadcast_and_wait based on can_retry() will retry the whole call after a successful broadcast, re-broadcasting the same transition. Internal retry already happened inside broadcast / wait_for_response_with_hash, so by the time this variant escapes, the wait has exhausted its budget. For nonce-based transitions this just produces AlreadyExists on the next attempt; the recovery use case the variant was designed for is a query-by-transition_hash, not a re-broadcast. Either document that this is intentionally retriable only because the underlying broadcast is idempotent under nonce reuse, or make it non-retriable and expose an explicit wait_again_for(transition_hash) API.
source: ['claude']
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct StateTransitionResult<T> { | ||
| inner: T, | ||
| transition_hash: [u8; 32], | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: StateTransitionResult is not #[must_use]
StateTransitionResult<T> is the public return type of every broadcast_and_wait call and the entire reason this PR exists — callers ignoring the wrapper defeats its purpose. Annotating with #[must_use = "the transition hash and proof result should be inspected"] would catch accidentally dropped returns at the type level. With Deref removed, an ignored return is harder to overlook than it used to be, but let _ = ... still silently throws away the hash.
💡 Suggested change
| #[derive(Debug, Clone, PartialEq, Eq)] | |
| pub struct StateTransitionResult<T> { | |
| inner: T, | |
| transition_hash: [u8; 32], | |
| } | |
| #[derive(Debug, Clone, PartialEq, Eq)] | |
| #[must_use = "the transition hash and proof result should be inspected"] | |
| pub struct StateTransitionResult<T> { | |
| inner: T, | |
| transition_hash: [u8; 32], | |
| } |
source: ['claude']
| /// Maps the inner value, preserving the transition hash. | ||
| pub fn map<U, F: FnOnce(T) -> U>(self, f: F) -> StateTransitionResult<U> { | ||
| StateTransitionResult { | ||
| inner: f(self.inner), | ||
| transition_hash: self.transition_hash, | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: StateTransitionResult::map has no in-tree caller
map is defined and unit-tested but unused in production: broadcast_and_wait constructs the wrapper at the leaf, and every consumer calls .into_inner() or .into_parts(). Carrying public surface for hypothetical consumers tends to ossify; either drop it now or wait for a real consumer to need it. Not a defect, just unused surface.
source: ['claude']
thepastaclaw
force-pushed
the
feat/sdk-transition-hash
branch
from
9582fc3 to
127ba6e
Compare
Add StateTransitionResult<T> wrapper type that bundles the proof result with the transition hash (transaction ID). The hash is computed deterministically from the serialized StateTransition BEFORE broadcast, avoiding any race condition with blockchain state. broadcast_and_wait now returns StateTransitionResult<T> instead of T. The wrapper implements Deref<Target=T> for ergonomic access and provides into_inner() for callers that only need the result. All existing callers updated to use .into_inner() to maintain current behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…esult - Move state_transition_result module to alphabetical position in transition.rs - Update wasm-sdk broadcast.rs to unwrap StateTransitionResult with into_inner()
wait_for_response returns T directly, not StateTransitionResult<T>, so into_inner() is not needed there.
Adapt PR scope to the renamed identity APIs in v3.1-dev: - put_identity: with_transition_hash wrapper now calls put_to_platform_with_private_key (rename of put_to_platform) - wasm-sdk error: fill new transition_hash arg at remaining call sites - platform-wallet update_identity_with_signer: unwrap the new StateTransitionResult wrapper before returning the proof result
thepastaclaw
force-pushed
the
feat/sdk-transition-hash
branch
from
127ba6e to
ace6143
Compare
4 participants
Issue being fixed or feature implemented
State transition methods in the Rust SDK (
broadcast_and_wait,put_to_platform_and_wait_for_response, etc.) discard the transition hash (transaction ID) after broadcast. Downstream consumers (WASM SDK, evo-sdk) cannot access it without re-computing or restructuring their calls.This is the Rust SDK foundation for the work started in #2953, restructured per review feedback to implement the logic in the Rust SDK first (rather than only in the WASM SDK).
What was done?
Added
StateTransitionResult<T>— a thin wrapper that bundles a proof resultTwith the 32-byte transition hash (transaction ID).Core changes:
StateTransitionResult<T>inpackages/rs-sdk/src/platform/transition/state_transition_result.rstransition_hash() -> [u8; 32]— get the hashinto_parts() -> (T, [u8; 32])— destructureinto_inner() -> T— unwrap (backward compat)Deref<Target=T>— transparent access to inner resultBroadcastStateTransition::broadcast_and_waitreturnsStateTransitionResult<T>instead ofT*_with_transition_hashhelpers now wrap wait failures asWaitForStateTransitionResultFailedAfterBroadcast { transition_hash, source }after broadcast succeeds, so callers can recover the txid even when waiting failsWasmSdkErrorexposestransitionHashas aUint8Arrayfor post-broadcast wait failures, matching the successfulBroadcastAndWaitResultWasm.transitionHashpathBroadcastStateTransitionexcerpt andbroadcast_and_waitdescription forStateTransitionResult<T>Race condition avoidance
The transition hash is computed deterministically from the
StateTransitionstruct before broadcast viaStateTransition::transaction_id()(SHA-256 of serialized transition). It does not depend on blockchain state — no race condition possible. This addresses the concern raised in #2953.WASM ergonomic API follow-up
The low-level WASM
broadcastAndWaitsuccess path and post-broadcast wait-failure error path now expose the transition hash structurally. Higher-level ergonomic WASM helpers that currently return domain objects directly are intentionally left to the existing #2953 follow-up so those JS return shapes can be redesigned together instead of piecemeal in this PR.Files modified (30+ files)
broadcast.rs— core trait change and post-broadcast wait error wrapperstate_transition_result.rs— new wrapper typepackages/wasm-sdk/src/error.rs— structuredtransitionHashgetter for wait failures after broadcastbook/src/sdk/put-operations.md— updatedbroadcast_and_waitdocsHow Has This Been Tested?
cargo fmt --all— passescargo test -p dash-sdk platform::transition::broadcast --lib— passes (3 tests)cargo test -p wasm-sdk error --lib— passes (2 tests)cargo check -p wasm-sdk— passescargo check -p dash-sdk— passescargo check -p wasm-sdk --target wasm32-unknown-unknown— attempted, blocked by local clang/sysroot issue:unable to create target: 'No available targets are compatible with triple "wasm32-unknown-unknown"'Validation
Build verification
cargo check -p dash-sdk— confirms the newStateTransitionResult<T>type and all updated method signatures compile correctlycargo check -p wasm-sdk— confirms the WASM SDK consumer compiles against the updatedbroadcast_and_waitreturn type and structured error fieldType-level correctness
broadcast_and_waitreturnsStateTransitionResult<T>instead ofT, and the wrapper implementsDeref<Target=T>, so callers can inspect the hash while existing method access remains ergonomicStateTransitionResult<T>throughput_to_platform_and_wait_for_response→ callers that need onlyTuse.into_inner()wasm-sdkbroadcast layer correctly unwraps via.into_inner()forbroadcast_and_waitwhile leavingwait_for_response(which already returnsTdirectly) unchangedWaitForStateTransitionResultFailedAfterBroadcastBackward compatibility
into_inner()provides a zero-cost migration path for callers that don't need the transition hashDerefimpl means existing code that calls methods on the result works transparently without changesManual review
transaction_id()is called before broadcast (no race condition with blockchain state)Breaking Changes
broadcast_and_waitnow returnsStateTransitionResult<T>instead ofT. Callers that need justTcan use.into_inner()or rely onDeref.Checklist: