Repository navigation
fix(sdk): preserve uncertain submission outcomes through shared broadcast - #5556
llbartekll wants to merge 17 commits into
Conversation
|
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:
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 29.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "fix(sdk): preserve uncertain submission outcomes through shared broadcast" matches the main change. The PR keeps an unconfirmed outcome when an earlier broadcast may have reached a node, an… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "fix(sdk): preserve uncertain submission outcomes through shared broadcast" matches the main change. The PR keeps an unconfirmed outcome when an earlier broadcast may have reached a node, and it routes the combined submission APIs through a shared broadcast path (wait_after_broadcast). The title is specific and concise, and it follows a conventional-commit format.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 29.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 112 functions across 51 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
|
✅ Final review complete — no blockers (commit dced0c1) · triage: critical |
|
Self-review completed for Reviewed the actual DAPI and outer SDK retry loops: uncertainty survives a later definite refusal, the existing dispatch budget and signed transition are retained, and the existing wait/proof path runs only after success or possible acceptance. Preparation and definite-refusal failures remain outside the top-up unconfirmed-result wrapper. A timeout/cache response alone is never promoted into a new success condition. A separate FFI/mobile pass checked both SDK error wrappers/free paths, real wallet/composite top-up propagation, and Swift/Kotlin classification. Diagnostic nonce/proof errors do not become a safe-retry verdict. Existing vote lookup and affected-state proof semantics remain unchanged. Fresh checks: 259 Rust tests pass (75 transition, 1 unconfirmed marker, 157 DAPI, 20 SDK FFI, 6 wallet FFI top-up). Rustfmt, diff checks, and all-features/all-targets Clippy for Updated the description to the actual base and companion iOS #1254. Explicitly retained the known limits: broadcast-only history exposure, ordinary identity-withdrawal typing, vote lookup behavior, generic WASM kind and textual DAPI refusal marker; no claim to fix external-recipient drain recovery, persist Orchard actions, or provide a proved drain-completion receipt. No source changes, new commits or live spends in this pass. The prior native framework evidence remains tied to its original source SHA, as documented. External bot/human review is still required; this self-review does not waive it. |
|
/self-reviewed 48490f2 |
github-actions
Bot
added
the
bot-review-skipped
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The submission-observation mechanism is generally sound, but three combined submission paths still discard ambiguous broadcast history and can return a misleading failure before verifying the original signed transition. Proxy establishment failures are also classified as possibly accepted, which can suppress safe retry even though no node received the request. Additional non-blocking issues remain around DAPI text classification, WASM error typing, Kotlin metadata masking, and feature-gating the new tests. The Rust workspace test check failed on this head, while several JS checks remain queued; no failure log was available to establish whether the Rust failure is change-related.
🔴 3 blocking | 🟡 4 suggestion(s)
3 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: DashPay profile writes still abort before waiting after an ambiguous broadcast
packages/rs-sdk/src/platform/dashpay/profile.rs:237-238
put_profile_write remains a manual combined submit-and-wait path. It calls state_transition.broadcast(...).await? and only starts wait_for_document_and_owner_balance_with_metadata afterward. If the profile transition was accepted but its acknowledgement was lost, or a later retry ends with AlreadyExists, broadcast returns the final relay error and the proof wait is skipped. The caller can then treat the update as failed and submit another signed update even though the original may have executed. Route this path through the observation-aware broadcast result and wait_after_broadcast, preserving the same signed transition.
source: gpt-6.1-sol (phase2-reviewer: general)
🔴 Blocking: Platform-wallet contract updates bypass the observation-aware combined path
packages/rs-platform-wallet/src/wallet/identity/network/contract.rs:547-554
The wallet contract-update flow still performs a raw transition.broadcast(...).await? and then calls DataContract::wait_for_response. An ambiguous acknowledgement, or a definite final error after an earlier ambiguous retry, therefore prevents the wait from running. This is a separate public combined contract-submission route from the updated SDK PutContract path, so callers can still receive a failed update result without reconciling the original signed transition. It must use the observation-aware broadcast/wait orchestration before returning the final error.
source: gpt-6.1-sol (phase2-reviewer: general)
🟡 Suggestion: Mask consensus metadata on Kotlin unconfirmed submission errors
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt:50-51
StateTransitionResultUnconfirmed is constructed with the native DashSDKException as its cause, while the base consensusError getter exposes cause.consensusError for every subclass. The current Rust FFI conversion intentionally emits no consensus metadata for this marker, but the Kotlin public type does not enforce that invariant. If a malformed, stale, or future native error carrying code 11 also carries consensus fields, Kotlin callers could observe a contradictory consensus rejection and treat the operation as a definite refusal. Make the base property overridable and return null for StateTransitionResultUnconfirmed, or clear the metadata while mapping code 11.
source: gpt-6.1-sol (phase2-reviewer: ffi-engineer)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to shared transition submission and identity top-up recovery paths that affect funds movement and cross-language error handling, particularly intop_up_identity_from_addresses.rsand the corresponding FFI boundaries. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/broadcast.rs`:
- [BLOCKING] packages/rs-sdk/src/platform/transition/broadcast.rs:413-425: Proxy establishment failures are incorrectly treated as possibly accepted broadcasts
The classifier only excludes tonic ConnectError instances before treating transport statuses as possibly accepted. The DAPI transport already exposes TransportError::is_proxy_failure(), which detects SOCKS5 ProxyError values preserved in the status source chain. A proxy connection or handshake failure occurs before the request reaches DAPI or a Platform node, so treating it as possibly accepted causes wait_after_broadcast to poll the transition and can turn an address-funded top-up into StateTransitionResultUnconfirmed. That tells callers not to retry even though no node could have executed the transition. Check is_proxy_failure() before the status-code classification and add a regression test.
- [SUGGESTION] packages/rs-sdk/src/platform/transition/broadcast.rs:426-431: Do not classify DAPI broadcast state by human-readable status text
The SDK now makes a DAPI-specific decision by comparing the complete human-readable Internal status with DAPI_PROCESSING_REJECTION. That marker is emitted by rs-dapi and is duplicated across handlers, so wording changes or another DAPI implementation can make a definite processing rejection look ambiguous and trigger an unnecessary result wait. Conversely, reusing the same text for another internal condition could skip a required wait. The owning fix belongs in rs-dapi and its gRPC contract: emit a machine-readable error detail or metadata marker for this local processing rejection, then consume that marker here. The PR links the earlier recommendation, so this can be deferred as a tracked follow-up rather than blocking this fix.
In `packages/rs-sdk/src/platform/dashpay/profile.rs`:
- [BLOCKING] packages/rs-sdk/src/platform/dashpay/profile.rs:237-238: DashPay profile writes still abort before waiting after an ambiguous broadcast
put_profile_write remains a manual combined submit-and-wait path. It calls state_transition.broadcast(...).await? and only starts wait_for_document_and_owner_balance_with_metadata afterward. If the profile transition was accepted but its acknowledgement was lost, or a later retry ends with AlreadyExists, broadcast returns the final relay error and the proof wait is skipped. The caller can then treat the update as failed and submit another signed update even though the original may have executed. Route this path through the observation-aware broadcast result and wait_after_broadcast, preserving the same signed transition.
In `packages/rs-platform-wallet/src/wallet/identity/network/contract.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contract.rs:547-554: Platform-wallet contract updates bypass the observation-aware combined path
The wallet contract-update flow still performs a raw transition.broadcast(...).await? and then calls DataContract::wait_for_response. An ambiguous acknowledgement, or a definite final error after an earlier ambiguous retry, therefore prevents the wait from running. This is a separate public combined contract-submission route from the updated SDK PutContract path, so callers can still receive a failed update result without reconciling the original signed transition. It must use the observation-aware broadcast/wait orchestration before returning the final error.
In `packages/wasm-sdk/src/error.rs`:
- [SUGGESTION] packages/wasm-sdk/src/error.rs:184-189: Preserve the unconfirmed classification through the WASM boundary
The Rust SDK has a dedicated StateTransitionResultUnconfirmed error, but the WASM adapter converts it to WasmSdkErrorKind::Generic. JavaScript callers of identity_top_up_from_addresses cannot distinguish an unconfirmed submission from an ordinary generic failure except by parsing the message; only the non-retryable flag survives. Add a dedicated WASM error kind with is_retriable set to false so the typed recovery classification remains available across this boundary. The PR links the prior recommendation, so this is a tracked non-blocking follow-up.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt:50-51: Mask consensus metadata on Kotlin unconfirmed submission errors
StateTransitionResultUnconfirmed is constructed with the native DashSDKException as its cause, while the base consensusError getter exposes cause.consensusError for every subclass. The current Rust FFI conversion intentionally emits no consensus metadata for this marker, but the Kotlin public type does not enforce that invariant. If a malformed, stale, or future native error carrying code 11 also carries consensus fields, Kotlin callers could observe a contradictory consensus rejection and treat the operation as a definite refusal. Make the base property overridable and return null for StateTransitionResultUnconfirmed, or clear the metadata while mapping code 11.
In `packages/rs-sdk/src/platform/transition/put_contract.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/transition/put_contract.rs:117-121: Gate new submission tests with the mocks feature
The new submission_tests module is enabled by #[cfg(test)] but imports crate::sdk::put_test_support::PutFixture, which is declared only under #[cfg(all(test, feature = "mocks"))] in sdk.rs. The same mismatch exists in the new submission test modules in put_document.rs, purchase_document.rs, update_price_of_document.rs, and vote.rs; vote also imports mock-gated IdentityNonceFetcher. Building tests with --no-default-features therefore leaves the consumer modules enabled while their helpers are absent, causing compilation failures. Use the same all(test, feature = "mocks") gate on every new submission_tests module.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Persist uncertain submissions for restart recovery — The SDK and wallet now preserve an in-memory unconfirmed result and prevent it from being treated as permission to create a replacement spend, but the original signed address-funded transition is not persisted for reconciliation after process restart. Without a durable recovery record, a crash after an ambiguous submission still requires external recovery handling.
- Follow-up: Track persistent operation records and proof-backed reconciliation in a separate recovery-focused issue or PR, as the current PR explicitly excludes that broader feature.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
|
@thepastaclaw Follow-up on the final review of
CI diagnosis: the failed Rust workspace job ran the synthetic merge No commit, push, review dismissal or merge in this pass. Local fixes and their validation are distinguished from published-head evidence in both PR descriptions. |
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The observation-aware submission mechanism is correctly applied to the shared SDK paths, but the platform-wallet contract update and DashPay profile writer still discard ambiguous broadcast history before waiting for proof. Kotlin also exposes consensus metadata on the new unconfirmed error if a code-11 native exception carries contradictory metadata. The Rust workspace check failed on an unrelated unchanged Drive schema assertion, while several CI checks remain pending.
🔴 2 blocking | 🟡 1 suggestion(s)
3 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Platform-wallet contract updates bypass the observation-aware combined path
packages/rs-platform-wallet/src/wallet/identity/network/contract.rs:547-554
This wallet-owned combined submission path still calls the broadcast-only API and propagates its final error before invoking DataContract::wait_for_response. If an earlier dispatched attempt may have been accepted but its acknowledgement is lost, a later refusal or exhausted retry causes the ? at line 550 to return immediately, so the existing proof lookup for the same signed transition never runs. A caller can then treat an executed contract update as failed and submit a replacement. Route this flow through the SDK's observation-preserving broadcast-and-wait helper.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality)
🔴 Blocking: DashPay profile writes still abort before waiting after an ambiguous broadcast
packages/rs-sdk/src/platform/dashpay/profile.rs:237-238
put_profile_write retains the old broadcast-then-wait sequence. An acknowledgement timeout, connection loss, or other possibly accepted broadcast error is returned at line 237, preventing the proof-backed wait_for_document_and_owner_balance_with_metadata call from checking whether that exact signed profile replacement executed. Because both profile create and update use this helper, a successful profile write can be reported as failed and later retried as a new operation. Preserve the broadcast observation and invoke the existing proof wait before returning an ambiguous failure.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality)
🟡 Suggestion: Mask consensus metadata on Kotlin unconfirmed submission errors
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt:50-51
StateTransitionResultUnconfirmed retains the native DashSDKException as its cause, while the base consensusError accessor returns any consensus metadata from that cause without considering the native error code. The supported Rust conversion correctly emits code 11 without consensus metadata, but a malformed, stale, or future native code-11 payload can still expose a definitive consensus rejection on an operation whose execution remains uncertain. Make the accessor return null for StateTransitionResultUnconfirmed so callers cannot interpret diagnostic metadata as permission to replace the submission.
val consensusError: PlatformConsensusError?
get() = if (this is StateTransitionResultUnconfirmed) {
null
} else {
(cause as? DashSDKException)?.consensusError
}
source: muse-spark-1.3-contributor (phase1-reviewer: rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to submission and recovery logic that directly affects funds movement and replacement-spend decisions, particularly in identity top-up paths such astop_up_identity_from_addresses.rsand the platform wallet top-up handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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-platform-wallet/src/wallet/identity/network/contract.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contract.rs:547-554: Platform-wallet contract updates bypass the observation-aware combined path
This wallet-owned combined submission path still calls the broadcast-only API and propagates its final error before invoking `DataContract::wait_for_response`. If an earlier dispatched attempt may have been accepted but its acknowledgement is lost, a later refusal or exhausted retry causes the `?` at line 550 to return immediately, so the existing proof lookup for the same signed transition never runs. A caller can then treat an executed contract update as failed and submit a replacement. Route this flow through the SDK's observation-preserving broadcast-and-wait helper.
In `packages/rs-sdk/src/platform/dashpay/profile.rs`:
- [BLOCKING] packages/rs-sdk/src/platform/dashpay/profile.rs:237-238: DashPay profile writes still abort before waiting after an ambiguous broadcast
`put_profile_write` retains the old broadcast-then-wait sequence. An acknowledgement timeout, connection loss, or other possibly accepted broadcast error is returned at line 237, preventing the proof-backed `wait_for_document_and_owner_balance_with_metadata` call from checking whether that exact signed profile replacement executed. Because both profile create and update use this helper, a successful profile write can be reported as failed and later retried as a new operation. Preserve the broadcast observation and invoke the existing proof wait before returning an ambiguous failure.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt:50-51: Mask consensus metadata on Kotlin unconfirmed submission errors
`StateTransitionResultUnconfirmed` retains the native `DashSDKException` as its cause, while the base `consensusError` accessor returns any consensus metadata from that cause without considering the native error code. The supported Rust conversion correctly emits code 11 without consensus metadata, but a malformed, stale, or future native code-11 payload can still expose a definitive consensus rejection on an operation whose execution remains uncertain. Make the accessor return `null` for `StateTransitionResultUnconfirmed` so callers cannot interpret diagnostic metadata as permission to replace the submission.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Preserve the unconfirmed classification through the WASM boundary — The WASM adapter still maps the unconfirmed result to its existing generic kind while preserving
is_retriable = false, so JavaScript callers cannot consume a dedicated typed classification. This is a concrete public API limitation, but the PR explicitly defers it to the tracked follow-up.- Follow-up: Implement the dedicated WASM error kind under #5511 (comment).
- Do not classify DAPI broadcast state by human-readable status text — The SDK classifies the DAPI processing-rejection marker by exact human-readable text. The producer's duplicate-transaction path can observe an earlier
AlreadyExistscondition without proving that the transition was never accepted elsewhere, so a structured DAPI error marker would make this boundary safer. The PR explicitly defers the cross-layer contract change to the tracked follow-up.- Follow-up: Replace the text comparison with a machine-readable DAPI/gRPC marker under #5511 (comment).
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
github-actions
Bot
added
the
waiting-self-review
|
/self-reviewed e87e9b4 Reviewed the merge with v5.1-dev 9766bab. Preserved the upstream resource-vote equality check and both regression suites within the shared uncertain-submission wait. Local validation: 633 SDK tests, all-features/all-targets SDK Clippy, full-workspace all-target check, formatting and PR diff checks passed. Swift/Rust FFI integration audit found no additional conflict. The new cache-processing fallback finding remains open; this attestation does not claim it resolved or carry earlier approvals forward. No PR merge. |
|
Your move: coderabbitai left review threads unresolved; resolve them; thepastaclaw left review threads unresolved; resolve them. |
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; coderabbitai left review threads unresolved; resolve them; thepastaclaw left review threads unresolved; resolve them. |
|
/self-reviewed b1e529b Reviewed the cache fallback change and the separate mock replay fix. Production send budgets, original signed bytes, proof verification and typed consensus refusal handling are preserved. Before/after regressions reproduced both defects. Final local validation: 633 SDK tests, 206 DAPI unit/integration tests + 5 doctests (1 existing ignored), replay-focused 4 tests, SDK/DAPI all-features/all-targets Clippy, rustfmt and diff checks passed. External review and CI on this SHA remain required; no old approvals carried forward. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of b1e529b confirms the cache-processing fix, but identifies a blocking path that clears submission uncertainty using an unauthenticated wait response and a remaining SDK mock-replay classification defect. Current CI shows Kotlin, Rust wasm32, Swift frozen-schema, JS build/lint, and Drive/RS-DAPI image checks passing; Rust workspace tests, Evo SDK tests, the Dashmate helper image, and PR Hygiene remain pending, with no failed code checks reported. No builds or tests were run locally.
🔴 1 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Preserve recorded submission classification during mock replay
packages/rs-sdk/src/mock/sdk.rs:668-669
The direct MockDapiClient::load fix preserves versioned classification, but the SDK's public recording loader still bypasses it. load_expectations_sync, including the SdkBuilder::with_dump_dir path, calls this helper, which deserializes DumpData and feeds the decoded result into expect(). The newly added classification in expect() then ORs request-specific classification into the recorded flag after MockableStatus has discarded connector sources and consensus metadata.
A versioned connector failure recorded with possibly_accepted = false therefore becomes a bare Unavailable status and is changed to true; metadata-backed refusals under ambiguous status codes suffer the same loss. SDK replay can enter an outcome wait instead of reproducing definite non-dispatch, despite the direct-client regressions passing. Delegate SDK recording ingestion to MockDapiClient::load so the recorded-versus-hand-built distinction remains owned by the DAPI client, and add a round-trip regression through load_expectations_sync or the builder. Keep classification before serialization for genuinely hand-built expectations; do not reclassify decoded versioned recordings.
source: gpt-6.1-sol (phase2-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate cross-layer change directly alters funds-operation submission and recovery semantics in packages/rs-sdk/src/platform/transition/top_up_identity_from_addresses.rs and shared broadcast.rs, including whether uncertain submissions permit replacement operations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/broadcast.rs`:
- [BLOCKING] packages/rs-sdk/src/platform/transition/broadcast.rs:285-287: Do not clear submission uncertainty using an unsigned wait verdict
A populated `cause` proves only that the endpoint's consensus-error bytes deserialize; it does not authenticate a rejection of the submitted transition. `wait_for_outcome_with_metadata` returns `Result::Error` at lines 503–511 before reaching quorum-signature/proof verification, and `StateTransitionBroadcastError::try_from` performs deserialization only. Consequently, this exception returns an ordinary rejection even after an uncertain broadcast.
After a valid address payment reaches node A and its acknowledgement is lost, a malicious DAPI endpoint can answer the original-hash wait with a fabricated serialized `AddressInvalidNonceError`, without a proof or quorum signature. This branch drops the unconfirmed classification, and the wallet promotes that error to `AddressNonceMismatch`—FFI code 21 rather than unconfirmed code 20. A caller following the advertised nonce-recovery path can rebuild with the advanced nonce and pay the recipient again. Requesting the original hash does not authenticate the response's claimed verdict. The new `should_preserve_exact_transition_wait_nonce_verdict_after_lost_ack` test explicitly expects this unsafe downgrade.
Keep unauthenticated result-stream errors inside the unconfirmed envelope after an uncertain send. Clear that history only with authenticated failure evidence bound to the submitted transition, and change the regression to assert that the fabricated wait error cannot enter replacement-operation recovery.
In `packages/rs-sdk/src/mock/sdk.rs`:
- [SUGGESTION] packages/rs-sdk/src/mock/sdk.rs:668-669: Preserve recorded submission classification during mock replay
The direct `MockDapiClient::load` fix preserves versioned classification, but the SDK's public recording loader still bypasses it. `load_expectations_sync`, including the `SdkBuilder::with_dump_dir` path, calls this helper, which deserializes `DumpData` and feeds the decoded result into `expect()`. The newly added classification in `expect()` then ORs request-specific classification into the recorded flag after `MockableStatus` has discarded connector sources and consensus metadata.
A versioned connector failure recorded with `possibly_accepted = false` therefore becomes a bare `Unavailable` status and is changed to `true`; metadata-backed refusals under ambiguous status codes suffer the same loss. SDK replay can enter an outcome wait instead of reproducing definite non-dispatch, despite the direct-client regressions passing. Delegate SDK recording ingestion to `MockDapiClient::load` so the recorded-versus-hand-built distinction remains owned by the DAPI client, and add a round-trip regression through `load_expectations_sync` or the builder. Keep classification before serialization for genuinely hand-built expectations; do not reclassify decoded versioned recordings.
| if matches!(&error, Error::StateTransitionBroadcastError(result) if result.cause.is_some()) | ||
| { | ||
| error |
There was a problem hiding this comment.
✅ Resolved at
67032453; see the replies below.
🔴 Blocking: Do not clear submission uncertainty using an unsigned wait verdict
A populated cause proves only that the endpoint's consensus-error bytes deserialize; it does not authenticate a rejection of the submitted transition. wait_for_outcome_with_metadata returns Result::Error at lines 503–511 before reaching quorum-signature/proof verification, and StateTransitionBroadcastError::try_from performs deserialization only. Consequently, this exception returns an ordinary rejection even after an uncertain broadcast.
After a valid address payment reaches node A and its acknowledgement is lost, a malicious DAPI endpoint can answer the original-hash wait with a fabricated serialized AddressInvalidNonceError, without a proof or quorum signature. This branch drops the unconfirmed classification, and the wallet promotes that error to AddressNonceMismatch—FFI code 21 rather than unconfirmed code 20. A caller following the advertised nonce-recovery path can rebuild with the advanced nonce and pay the recipient again. Requesting the original hash does not authenticate the response's claimed verdict. The new should_preserve_exact_transition_wait_nonce_verdict_after_lost_ack test explicitly expects this unsafe downgrade.
Keep unauthenticated result-stream errors inside the unconfirmed envelope after an uncertain send. Clear that history only with authenticated failure evidence bound to the submitted transition, and change the regression to assert that the fabricated wait error cannot enter replacement-operation recovery.
source: gpt-6.1-sol (phase2-reviewer: security-auditor)
There was a problem hiding this comment.
Fixed in 670324539c4a7ba67e87743489e534db1bc8ca56. The populated consensus cause was only decoded, not authenticated: the wire Result::Error branch exits before proof/quorum-signature verification. I removed that exception from the shared wait instead of adding another classifier.
Both a broadcast ACK and a lost ACK now keep an unsigned wait error inside the existing unconfirmed envelope, preserving the diagnostic cause without double wrapping. The public SDK→wallet→FFI top-up regression supplies serialized AddressInvalidNonceError without proof and asserts code 20 / consensus code 0, one signing/broadcast/wait, and unchanged identity balance for both send outcomes. A definite broadcast-refusal control still returns code 21.
The SDK regression failed before the fix. Validation on this head: 634 SDK library tests, 6 SDK→wallet→FFI top-up tests, SDK + wallet FFI Clippy all-features/all-targets with warnings denied, rustfmt and diff checks all passed. No new API, retry, persistence or mobile behavior was added. Please re-review this head; leaving the thread open for your confirmation.
There was a problem hiding this comment.
Resolved (re-reviewed at 67032453): Your removal of the typed-cause exception closes the reported downgrade after both ACK and lost ACK. The public wallet/FFI regression supplies an unsigned nonce verdict and checks unconfirmed code 20 with consensus code 0 rather than replacement-recovery classification.
|
Your move: coderabbitai left review threads unresolved; resolve them; thepastaclaw left review threads unresolved; resolve them. |
|
/self-reviewed 6703245 Reviewed the four-file follow-up: unsigned wait verdicts preserve unconfirmed status after ACK or lost ACK; definite broadcast refusals retain their original handling; SDK replay preserves recorded classification through both loader entry points. 634 SDK tests, 6 SDK→wallet→FFI tests, all-feature/all-target Clippy for SDK and wallet FFI, rustfmt and diff checks passed. Swift/Rust FFI boundary audit completed. No consensus-generation, ABI, retry-budget or persistence changes. Native/mobile results remain historical; CI and external review for this exact head are still required. |
|
/self-reviewed |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 6703245 and found no remaining in-scope blocking defect; two non-blocking improvements remain in legacy mock classification and public submission assertions. The unsigned-wait-verdict and SDK recording-loader fixes address the prior findings. This was static review only: current exact-head CI reports Rust workspace, wasm32 and Kotlin checks passing, with Swift, several JS/integration checks and PR Hygiene still pending; no failed code check was listed.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate shared submission rewrite changes funds-moving operation retry and confirmation behavior in packages/rs-sdk/src/platform/transition/top_up_identity_from_addresses.rs and purchase_document.rs, including whether ambiguous delivery can permit replacement operations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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-dapi-client/src/mock.rs`:
- [SUGGESTION] packages/rs-dapi-client/src/mock.rs:71-73: Classify retained transport errors when loading legacy recordings
This classification handles only DapiClientError::Transport, but production address exhaustion retains the last transport failure in DapiClientError::NoAvailableAddressesToRetry. Legacy recordings deserialize with possibly_accepted=false and load() routes them through expect(), so a wrapped Unavailable or DeadlineExceeded bypasses classification here. Since execute() now correctly preserves the stored envelope, SDK replay returns an ordinary broadcast failure and skips the original-transition wait, unlike production. Classify the retained transport cause from both variants, while leaving versioned recordings untouched. Add a legacy wrapped-error load/execute regression and an ordinary-read control.
In `packages/rs-sdk/src/sdk/put_test_support.rs`:
- [SUGGESTION] packages/rs-sdk/src/sdk/put_test_support.rs:186-191: Assert the typed unconfirmed outcome at public submission boundaries
The fixture deliberately supplies an invalid wait response, but this assertion only excludes a bare AlreadyExists error. A raw InvalidProvedResponse or a Generic error containing stringified uncertainty would also pass, although either loses the typed classification needed to suppress replacement-operation recovery. These assertions cover the public document, contract, purchase, price-update and transfer APIs, so they should verify propagation of the unconfirmed classification rather than only that the original hash was polled.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Delegate raw WASM combined submissions to Rust outcome orchestration — The unchanged broadcastAndWait and broadcastAndWaitForAffectedState exports in packages/wasm-sdk/src/state_transitions/broadcast.rs still broadcast with an early return before a separate outcome wait. An uncertain broadcast therefore skips reconciliation, and failures after an acknowledged broadcast bypass the shared unconfirmed envelope. These pre-existing raw wrappers are separate from the typed submission paths migrated here and from the deferred WASM error-kind change.
- Follow-up: Create a follow-up PR exposing the required outcome-returning orchestration in rs-sdk and delegate both WASM exports to it, preserving ownerBalance and strict-versus-affected-state guarantees. Test lost acknowledgements and unsigned or unverifiable wait responses through the public exports.
- Extend unsigned-refusal protection to remaining rejection paths — Two pre-existing paths still treat unauthenticated endpoint reports as definitive refusals: masternode_withdrawal.rs:503–516 accepts decoded wait errors through carries_consensus_rejection(), and a first broadcast response carrying consensus metadata returns directly when there is no earlier uncertainty. An endpoint can forward a valid transition and then fabricate a rejection, leaving callers exposed to replacement-payment or replacement-withdrawal recovery despite possible execution. This PR fixes accumulated uncertainty and guarded waits, not that broader refusal-authentication contract.
- Follow-up: Extend #5665 for unsigned withdrawal wait verdicts and track a separate follow-up for first-response broadcast refusals. Distinguish authenticated rejection or locally established non-dispatch from node-reported refusal, with regressions proving fabricated verdicts cannot authorize replacement operations.
- Include distinct change outputs in address-funded identity proof expectations — put_identity_with_address_funding builds expected_addresses from inputs only, while Drive verifies and returns address information for inputs plus the optional output. A distinct change-output address therefore makes collect_address_infos_from_proof reject a valid proof after identity creation executes. Both the expected-set calculation and interpretation are unchanged from the reviewed base.
- Follow-up: Create a separate Rust SDK fix including the optional output in the expected address set, with successful registration coverage using a change address distinct from every input.
| if let DapiClientError::Transport(transport) = &error.inner { | ||
| error.possibly_accepted |= request.classify_error(transport).possibly_accepted; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Classify retained transport errors when loading legacy recordings
This classification handles only DapiClientError::Transport, but production address exhaustion retains the last transport failure in DapiClientError::NoAvailableAddressesToRetry. Legacy recordings deserialize with possibly_accepted=false and load() routes them through expect(), so a wrapped Unavailable or DeadlineExceeded bypasses classification here. Since execute() now correctly preserves the stored envelope, SDK replay returns an ordinary broadcast failure and skips the original-transition wait, unlike production. Classify the retained transport cause from both variants, while leaving versioned recordings untouched. Add a legacy wrapped-error load/execute regression and an ordinary-read control.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
| if expected_waits == 1 { | ||
| assert!( | ||
| !matches!(result, Err(Error::AlreadyExists(_))), | ||
| "return the failed proof, not the ambiguous relay acknowledgement" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Assert the typed unconfirmed outcome at public submission boundaries
The fixture deliberately supplies an invalid wait response, but this assertion only excludes a bare AlreadyExists error. A raw InvalidProvedResponse or a Generic error containing stringified uncertainty would also pass, although either loses the typed classification needed to suppress replacement-operation recovery. These assertions cover the public document, contract, purchase, price-update and transfer APIs, so they should verify propagation of the unconfirmed classification rather than only that the original hash was polled.
| if expected_waits == 1 { | |
| assert!( | |
| !matches!(result, Err(Error::AlreadyExists(_))), | |
| "return the failed proof, not the ambiguous relay acknowledgement" | |
| ); | |
| } | |
| if expected_waits == 1 { | |
| assert!( | |
| result | |
| .as_ref() | |
| .is_err_and(Error::is_state_transition_result_unconfirmed), | |
| "a failed original-transition wait must preserve the typed unconfirmed outcome: {result:?}" | |
| ); | |
| } |
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
|
Your move: coderabbitai left review threads unresolved; resolve them; thepastaclaw left review threads unresolved; resolve them. |
|
/self-reviewed |
|
/self-reviewed 66243c7 Integrated v5.1-dev at e0f21d7. The only content conflict was put_contract.rs: retained upstream private-entry preflight in the shared preparation helper, before nonce fetch/signing, for both plain broadcast and combined send/wait. Original-transition uncertainty handling is unchanged. Preserved upstream tests and added a two-entry-point preflight regression with zero signatures. Validation on the final tree: 640 SDK all-feature library tests, 6 SDK→wallet→FFI submission tests, SDK/wallet FFI all-feature/all-target Clippy with warnings denied, full workspace all-target cargo check, rustfmt and diff checks passed. Read-only Swift/Rust FFI integration audit found no error-code or uncertainty-boundary regression. No new native/mobile run is claimed; fresh CI and human review remain required for this head. No PR merge. |
|
Your move: coderabbitai left review threads unresolved; resolve them; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at 66243c7 and found no remaining production-path blocker in the migrated submission flows. Three non-blocking suggestions remain: legacy recording fidelity, typed public-boundary assertions, and explicit tracking of the unchanged generic WASM combined APIs. Current CI reports all code checks successful, including Rust workspace, Swift, Kotlin, wasm32 and integration tests; this verification was static, with no local builds or tests.
🟡 3 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Explicitly track the generic WASM combined APIs that bypass the shared gate
packages/wasm-sdk/src/state_transitions/broadcast.rs:224-232
broadcastAndWait still composes broadcast().await? and wait_for_outcome_with_metadata() directly; broadcastAndWaitForAffectedState repeats this at lines 295–303. An uncertain broadcast therefore exits without checking the original transition, while an acknowledged broadcast followed by a proof failure or overall wait timeout can expose an ordinary retryable Proof or TimeoutReached error instead of the non-retryable unconfirmed envelope. These sequences are unchanged from the base, so this is a missed sibling suitable for a non-blocking follow-up, not an introduced binding-layer defect. Explicitly list both exports among the retained limitations and concrete callers tracked by #4998. The follow-up should expose a Rust-owned combined outcome primitive and delegate to it from WASM, preserving owner-balance presentation and execution-versus-affected-state guarantees rather than duplicating the submission gate in the binding.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This intricate, cross-cutting change modifies funds-movement submission and recovery semantics in packages/rs-sdk/src/platform/transition/top_up_identity_from_addresses.rs, including whether an uncertain address-funded top-up is treated as failed or checked for completion. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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] packages/wasm-sdk/src/state_transitions/broadcast.rs:224-232: Explicitly track the generic WASM combined APIs that bypass the shared gate
broadcastAndWait still composes broadcast().await? and wait_for_outcome_with_metadata() directly; broadcastAndWaitForAffectedState repeats this at lines 295–303. An uncertain broadcast therefore exits without checking the original transition, while an acknowledged broadcast followed by a proof failure or overall wait timeout can expose an ordinary retryable Proof or TimeoutReached error instead of the non-retryable unconfirmed envelope. These sequences are unchanged from the base, so this is a missed sibling suitable for a non-blocking follow-up, not an introduced binding-layer defect. Explicitly list both exports among the retained limitations and concrete callers tracked by https://github.com/dashpay/platform/issues/4998. The follow-up should expose a Rust-owned combined outcome primitive and delegate to it from WASM, preserving owner-balance presentation and execution-versus-affected-state guarantees rather than duplicating the submission gate in the binding.
In `packages/rs-dapi-client/src/mock.rs`:
- [SUGGESTION] packages/rs-dapi-client/src/mock.rs:71-73: Classify retained transport errors when loading legacy recordings
(existing thread: https://github.com/dashpay/platform/pull/5556#discussion_r4480387988)
The versioned-recording fixes are intact, but legacy classification still misses a transport-bearing sibling. Production execute() retains the last transport failure inside NoAvailableAddressesToRetry when addresses run out. Legacy deserialization initializes possibly_accepted=false, and load() delegates to expect(), whose direct-Transport match leaves that wrapped failure unclassified. A recorded broadcast with NoAvailableAddressesToRetry around an acknowledgement-loss Unavailable or DeadlineExceeded consequently replays as definite failure and can skip the original-transition wait, unlike production execution. Classify the retained transport cause too, and cover legacy load-through-execute replay plus an ordinary-read control. Preserve the existing bypass for versioned recordings so their stored flags remain authoritative. This is a mock/replay fidelity defect, not a defect in the live executor's accumulated history.
In `packages/rs-sdk/src/sdk/put_test_support.rs`:
- [SUGGESTION] packages/rs-sdk/src/sdk/put_test_support.rs:186-191: Assert the typed unconfirmed outcome at public submission boundaries
(existing thread: https://github.com/dashpay/platform/pull/5556#discussion_r4480387994)
The fixture verifies one signature, the original broadcast bytes and the original wait hash, but after its deliberately invalid wait response it only excludes a bare AlreadyExists error. A public combined method returning an unwrapped proof/decoding error or a stringified Generic error would still pass these contract, document, purchase, price-update and transfer regressions. The typed unconfirmed classification is what prevents wallet recovery and bindings from treating the diagnostic as permission to replace the operation. Assert is_state_transition_result_unconfirmed() whenever this fixture expects the failed-proof wait to run. The lower-level gate and top-up FFI tests protect their own boundaries, but do not catch a public wrapper stripping the classification.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Include the change output when interpreting address-funded identity creation — put_identity_with_address_funding builds expected_addresses from inputs only, but Drive verifies and returns both inputs and the optional output. A distinct change output therefore makes collect_address_infos_from_proof reject a valid result after the identity was created and funded. This interpretation also occurs after the combined uncertainty guard, so the failure escapes as an ordinary InvalidProvedResponse. Both defects are present unchanged on the base.
- Follow-up: A separate follow-up PR should include the optional output in the expected address set and keep post-submission interpretation within Rust-owned uncertainty handling. Cover a distinct change output with a real proof after both an acknowledged and a lost-acknowledgement submission.
- Strengthen Rust-only public submission test assertions — Already covered by the kept shared-fixture finding 221efdc14be9. Its placement outside the dedicated FFI review lane does not make it outside this PR's responsibility; no separate finding is needed.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
Your move: coderabbitai left review threads unresolved; resolve them; thepastaclaw left review threads unresolved; resolve them. |
|
/self-reviewed dced0c1 Conflict-only integration of v5.1-dev at 9b36712 after lklimek's approval of parent 66243c7. Resolved broadcast.rs by retaining upstream type-29 protocol preflight before this PR's existing broadcast retry path, plus both sets of helpers/tests. Existing transition kinds gain no additional probe; shared uncertain-result handling remains intact. Validation: 715 SDK all-feature library tests, 25 SDK FFI error tests, 6 SDK→wallet→FFI submission tests; SDK/SDK-FFI/wallet-FFI all-feature/all-target Clippy with warnings denied; full workspace all-target check; rustfmt and diff checks passed. Read-only FFI/WASM integration audit found no additional merge correction. No new native/mobile validation claimed. Fresh CI/review state applies to this exact head; no PR merge. |
|
Your move: coderabbitai left review threads unresolved; resolve them; thepastaclaw left review threads unresolved; resolve them. |
|
/self-reviewed |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at dced0c1: the shared Rust submission paths preserve delivery uncertainty, retain the original signed transition, and wrap unsuccessful confirmation attempts without promoting unsigned verdicts. Three non-blocking suggestions remain concerning generic WASM sibling coverage, public-boundary assertions, and legacy mock replay. This was a static review; current CI shows all completed code checks passing, including Rust workspace, wasm32, Swift, Kotlin, functional, and both browser suites, with only review/hygiene checks pending.
🟡 3 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Explicitly track the generic WASM combined APIs that bypass the shared gate
packages/wasm-sdk/src/state_transitions/broadcast.rs:224-232
Both exported generic combined APIs still broadcast and then wait separately: broadcastAndWait returns here on a broadcast error, and broadcastAndWaitForAffectedState repeats the sequence at lines 295–303. An uncertain send therefore skips the original-transition wait; after an acknowledged send, a failed wait also bypasses the shared unconfirmed envelope and can expose an ordinary proof error or unsigned consensus verdict. These inherited siblings need not expand this PR's implementation scope, but its retained-limitations list and the concrete checklist in #4998 name only contract updates and profile writes. Explicitly add both exported methods to that tracked exception before deferring them. Their eventual repair should delegate combined-outcome orchestration to rs-sdk, preserving ownerBalance and strict-versus-affected-state guarantees rather than adding submission policy to WASM.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, rust-quality)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate retry and outcome-classification changes in packages/rs-sdk/src/platform/transition/broadcast.rs and wallet top-up recovery directly alter funds-moving submission behavior, including whether uncertain delivery permits a replacement operation. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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] packages/wasm-sdk/src/state_transitions/broadcast.rs:224-232: Explicitly track the generic WASM combined APIs that bypass the shared gate
Both exported generic combined APIs still broadcast and then wait separately: `broadcastAndWait` returns here on a broadcast error, and `broadcastAndWaitForAffectedState` repeats the sequence at lines 295–303. An uncertain send therefore skips the original-transition wait; after an acknowledged send, a failed wait also bypasses the shared unconfirmed envelope and can expose an ordinary proof error or unsigned consensus verdict. These inherited siblings need not expand this PR's implementation scope, but its retained-limitations list and the concrete checklist in #4998 name only contract updates and profile writes. Explicitly add both exported methods to that tracked exception before deferring them. Their eventual repair should delegate combined-outcome orchestration to rs-sdk, preserving ownerBalance and strict-versus-affected-state guarantees rather than adding submission policy to WASM.
In `packages/rs-sdk/src/sdk/put_test_support.rs`:
- [SUGGESTION] packages/rs-sdk/src/sdk/put_test_support.rs:186-191: Assert the typed unconfirmed outcome at public submission boundaries
(existing thread: https://github.com/dashpay/platform/pull/5556#discussion_r4480387994)
When the fixture expects a result wait, its deliberately invalid response must produce a typed unconfirmed outcome. The current assertions require only an error other than top-level AlreadyExists, so a public wrapper returning a bare proof error, timeout, or stringified Generic error would still pass despite losing the classification that prevents replacement-operation recovery. Assert `is_state_transition_result_unconfirmed()`, non-retryability, and no promoted consensus verdict for `expected_waits == 1`, while keeping definite broadcast-refusal controls separate. The lower-level gate and top-up FFI regressions do not establish propagation through each document, contract, purchase, price-update, and transfer API exercised by this fixture.
In `packages/rs-dapi-client/src/mock.rs`:
- [SUGGESTION] packages/rs-dapi-client/src/mock.rs:71-73: Classify retained transport errors when loading legacy recordings
(existing thread: https://github.com/dashpay/platform/pull/5556#discussion_r4480387988)
Legacy recordings default their missing possibly_accepted flag to false and load through expect(), but this match classifies only the direct Transport variant. The production executor also retains its last transport failure inside NoAvailableAddressesToRetry when addresses are exhausted. A legacy broadcast recording containing that envelope around Unavailable or DeadlineExceeded consequently replays with possibly_accepted=false, allowing the SDK to skip the original-transition wait even though live execution preserves uncertainty. Classify the transport cause from both variants on the hand-built/legacy path and add a legacy load-through-execute regression with an ordinary-read control. Keep the existing versioned-recording bypass so stored classifications remain unchanged.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Authenticate first-attempt refusals before permitting replacement payments — Without earlier uncertainty, broadcast consensus metadata is still treated as a definite refusal and deserialized without execution-proof authentication. A DAPI endpoint that forwards a valid payment and then fabricates a nonce refusal can therefore expose a verdict that callers may use to construct a replacement payment. This first-attempt endpoint-trust assumption predates the reviewed change; the new protection specifically preserves uncertainty from prior attempts and unsuccessful result waits.
- Follow-up: Address rejection provenance in a separate security-focused issue or PR, including a regression where an endpoint forwards the original payment before returning fabricated nonce metadata and the caller attempts replacement recovery.
|
Your move: coderabbitai left review threads unresolved; resolve them; thepastaclaw left review threads unresolved; resolve them. |
Issue being fixed or feature implemented
A Platform transition can reach a node while its acknowledgement is lost. A later refusal must not erase that history or authorize a replacement operation. Combined submission APIs must check the outcome of the same signed transition and preserve an unresolved result when the required proof cannot be obtained or verified.
What was done?
Implemented lklimek’s simplification in production commit
c35e4fe4b8756e9f43c66403f38d3ce712cb6753;839bcba470a0312054a7e7e66c7d3f13588b2b22added the final WASM test expectations.383aa3bab632ad5dffc10d6f9c8eb0389999c124removes the reported-refusal shortcut described below.e87e9b498fbc8c67ded53eea08d75d8b15d9414dintegratesv5.1-dev(9766bab24a14af7cfbcafe33b466a777db8f868c) and resolves the vote confirmation conflict. The670324539c4a7ba67e87743489e534db1bc8ca56follow-up preserves uncertainty for unsigned wait errors and routes SDK recording replay through the existing classified loader:ExecutionError<E>carriespossibly_accepted, alongside its original typed cause, address and attempt count. Broadcast-specific classification lives beside the transport; ordinary queries retain their retry rules. DAPI and SDK retry loops accumulate uncertainty without replacing the typed error while retries are active.broadcast()is the single send path. RemovedBroadcastOutcome,BroadcastPolicy, the observed-send helpers and observer execution/callback plumbing. Existing budgets, local validation, maximum-size checks and signed bytes are retained. Success returns immediately; confirmed mempool/chain presence stops rebroadcasting; raw cache membership remains ambiguous.wait_after_broadcast(Result<(), Error>, wait_future)checks the same prepared transition after success or possible submission. A concrete rejection ends send retries. Without earlier uncertainty it returns directly; with earlier uncertainty, the existing wait checks the original transition. A server-reported consensus error type alone cannot clear earlier possible delivery or bypass that check.StateTransitionResultUnconfirmednow covers Platform submissions generally and retains its diagnostic cause without double wrapping. Inconclusive waits, missing/invalid proofs, interpretation failures and unsigned result-stream error verdicts remain unconfirmed. Decoding a consensus cause does not authenticate rejection. This applies after both a broadcast acknowledgement and a lost acknowledgement; definite broadcast refusal without earlier uncertainty still returns its original error. Existing execution-proof versus affected-state guarantees remain distinct.Intentional behavior changes: votes and document transfers now share ordinary broadcast settings/retries and nonce-cache refresh after final send failure. Concrete Drive refusals no longer trigger send retries, including the previous owner/nonce exceptions. Earlier uncertainty instead leads to checking the original operation.
Base:
v5.1-dev. Companion app integration remains iOS #1254; no iOS PR changes or merge are included here.Boundaries and retained limitations
How Has This Been Tested?
Base integration after human review — 9 November
Validated merge commit
dced0c119f9d84f873b931aa84f2e4c4e5057ade, integratingv5.1-devat9b36712770c33b2870cbe078f8b386fccb512dccwithout rewriting history:broadcast.rs. Retained upstream's type-29 private-document protocol requirement and pre-dispatch network-version check, followed by this PR's existing uncertainty-preserving broadcast retry and shared wait. Existing transition kinds receive no additional probe from this check.66243c798e069de1f919a3d83c6c85a1c6ef1932on 9 November at 14:20 UTC. That review is recorded against its actual SHA; fresh CI/review state is evaluated for this merge commit. No PR merge.Base integration — 9 November
Validated merge commit
66243c798e069de1f919a3d83c6c85a1c6ef1932, integratingv5.1-devate0f21d7137bc7e8d710d1ae56082ce0419418dd0without rewriting history:put_contract.rs: the base's private-document entry-size preflight runs in the shared preparation helper, before nonce fetching and signing. Both broadcast-only and combined send/wait retain the check; the combined path still waits on the original signed transition after uncertain submission.cargo check --workspace --all-targets, rustfmt and diff checks: passed.Unsigned wait verdict and SDK recording loader — 5 November
Validated head
670324539c4a7ba67e87743489e534db1bc8ca56:MockDapiClient::load, preserving versioned classification instead of reclassifying serialized errors. Regression coverage exercises bothload_expectations_syncandSdkBuilder::with_dump_dir, connector failures, consensus metadata and both stored flag values. Handcrafted FFI recordings now use the actual request classifier before serialization.--all-features --all-targets -- -D warnings, rustfmt and diff checks passed. Read-only Swift/Rust FFI audit confirmed the existing error boundary preserves the wrapper before nonce recovery. No new native framework or simulator run is claimed; earlier mobile results remain attributed to their original artifacts.Cache-processing and mock replay fixes — 5 November
Previously validated head
b1e529bc2fd4f8227b1648b5e3cf66fa62ed3ab4contains two focused commits:044dab53a23ce2ba2f09c050b7a30a100dbb5ce0: classify the exact cache-processing fallback as possibly accepted, without another broadcast retry. A cache hit can precede mempool insertion; a successful standalone CheckTx and earlier negative lookups do not prove rejection. Combined APIs use the existing wait for the original signed transition. If it cannot be confirmed, the existing timeout yields unconfirmed, not permission to create a replacement. Typed consensus refusals keep their existing classification.b1e529bc2fd4f8227b1648b5e3cf66fa62ed3ab4: classify hand-built mock expectations before serialization loses connector sources/metadata; preserve versioned recording flags during load/replay. Legacy recordings retain existing request-specific classification. No production API, recording format, retry budget or persistence mechanism was added.Validation: the cache regression and both new replay regressions failed before their respective fixes, then passed. 633 SDK library tests; 206 DAPI unit/integration tests plus 5 doctests passed, with 1 existing ignored test. Final replay-focused run: 4 passed. Clippy for SDK and DAPI with all features/all targets and warnings denied passed; rustfmt and diff checks passed. Tests cover both cache-message forms, no rebroadcast, a real signed original transfer, successful wait and timeout, typed connector failure, consensus metadata, retained prior uncertainty, versioned/legacy replay and unchanged read classification.
The proof-backed duplicate resource-vote decoder repair remains in #4138. These follow-ups do not claim new mobile artifact builds or simulator runs. Fresh CI and external re-review are required for this head.
Base integration — 5 November
Validated commit
e87e9b498fbc8c67ded53eea08d75d8b15d9414d, integrating base9766bab24a14af7cfbcafe33b466a777db8f868c:vote.rs: retained the base's comparison of the recorded resource vote with the submitted vote inside this PR's shared wait. Kept both regression suites. A mismatched or absent snapshot is not reported as success, and the diagnostic cause remains inside the unconfirmed envelope.cargo check --workspace --all-targets: passed. rustfmt and PR diff whitespace checks: passed.Reported-refusal fix — 5 November
Validated commit
383aa3bab632ad5dffc10d6f9c8eb0389999c124:--all-features --all-targets -- -D warnings, rustfmt and diff checks pass.c35e4feartifacts; mobile builds and simulator tests were not rerun for this Rust-only follow-up.Earlier simplification validation
Production validation used clean
c35e4fe4b8756e9f43c66403f38d3ce712cb6753. The final WASM suites and WASM all-target Clippy also cover the test-only follow-up839bcba470a0312054a7e7e66c7d3f13588b2b22; no production source changed between these commits. The additional full-workspace all-target check also passed on839bcba470a0312054a7e7e66c7d3f13588b2b22:-D warningscargo check --workspace --all-targetsRegression tests execute the real DAPI/SDK retry loops and public submission methods. They cover lost response followed by refusal, retained history across retries/conversions/address exhaustion, connection failure, raw cache versus confirmed presence, consensus metadata under an unusual gRPC code, same signed bytes and existing budgets, proof failures, conclusive business errors, single wrapping, nonce refresh and suppression of replacement recovery. Normal DAPI request tests also pass. The existing DPP Cargo manifest warning about inherited
default-featuresis unchanged.Fresh mobile artifacts were built from production commit
c35e4fe4b8756e9f43c66403f38d3ce712cb6753(clean), with iOS #1254 baseline03f781c4e3d231e360c9345b4099dfaf5e4a3b61and a local SDK-pin override only:d71a932783698cca17197b6321431959f418bb3e65dd8a80cd4ac8dedc21aa93.dashpayarm64 simulator build passed; five actual Swift error-decoder/presentation tests linked to the rebuilt C FFI passed (5/5).95d21e654d6aa25ff009743117b890b92b449b0186a66d1f965b537dabf9d761.42b8bbbbd6d120291433f11f578b4b7189d6688f30fa01402aada46b17ac842c. Balance observation alone is not claimed as an exact-transition execution proof.Full WASM testing also updated four expectations to preserve the new unconfirmed/non-retryable error after submission while retaining proof-failure diagnostics and unchanged cache/signature assertions. Under
--all-features, one existing fixture assumes the withdrawals contract is not compiled in; that assumption conflicts with--all-features. It is unchanged and passes in the complete default-feature suite; the remaining 196 tests pass with all features.Breaking Changes
Rust source compatibility:
ExecutionError<E>has a new requiredpossibly_acceptedfield, so downstream struct literals must be updated. The observer-specific APIs introduced earlier in this PR are removed. The PR’s SDK error variant/code 11 and wallet code 20 remain; existing C structure layouts and assigned numeric codes are unchanged by this simplification. Downstream exhaustive enum matches must account for the PR’s added error variant.In-place changes to shipped generations
None. Changes are client submission/error/wait orchestration, not consensus logic or protocol tables.
Checklist
No merge or application publication is authorized by this change.
PR Hygiene ·
dced0c1Cargo.lock,packages/rs-dapi-client/src/address_list.rs,packages/rs-dapi-client/src/dapi_client.rsand 7 more) — QuantumExplorer or shumkovkotlin-sdk(packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt,packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/DashSDKException.kt,packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt) — HashEngineeringrs-platform-wallet-ffi— you own itrs-platform-wallet— you own itrust-sdk-ffi(packages/rs-sdk-ffi/src/error.rs) — approved by lklimekrust-sdk(packages/rs-sdk/src/error.rs,packages/rs-sdk/src/mock/sdk.rs,packages/rs-sdk/src/platform/address_sync/mod.rsand 19 more) — approved by lklimekswift-sdk— you own itjs-wasm-sdk(packages/wasm-sdk/src/error.rs,packages/wasm-sdk/src/state_transitions/broadcast_tests.rs,packages/wasm-sdk/src/state_transitions/contract/cache_tests.rs) — shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit
New Features
Bug Fixes