fix(sdk): fail closed without proof verification#4168
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds authenticated Platform proof verification to the JavaScript SDK, validates proved DAPI responses, adds an unproved contract-history path, integrates EvoSDK-backed verifiers into test clients, and regenerates Yarn Plug’n’Play dependency mappings. ChangesPlatform proof verification
Generated dependency resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Platform
participant DAPI
participant ProofVerifier
Client->>Platform: submit Platform operation
Platform->>DAPI: request proved response
DAPI-->>Platform: response with proof and metadata
Platform->>ProofVerifier: verify response
ProofVerifier-->>Platform: verification result
Platform-->>Client: return verified operation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
🕓 Ready for review — 20 ahead in queue (commit 7eab96b) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR introduces proof verification for data contract history, but the verified path still accepts an endpoint-originated gRPC NOT_FOUND before any proof is verified. Because Platform returns an authenticated absence proof for prove=true requests, this exception creates an in-scope fail-closed bypass and requires changes.
Source: reviewers gpt-5.6-sol (general, security-auditor); verifier gpt-5.6-sol; Sonnet deferred by the blocker gate.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/history.ts`:
- [BLOCKING] packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/history.ts:39-50: Endpoint-forged NOT_FOUND bypasses history proof verification
The verified history path translates gRPC NOT_FOUND into `null` before invoking `verifyDataContractHistory`, so the selected DAPI endpoint can conceal an existing contract or its history with an unauthenticated negative response. This contradicts the PR's fail-closed boundary. The Platform handler returns a GroveDB proof whenever `prove` is true, including for nonexistent contract history, and emits NOT_FOUND only for unproved queries. The verified method must therefore propagate endpoint errors and accept absence only through the authenticated proof-verifier result; `historyUnproved` can retain the endpoint-trusted NOT_FOUND behavior.
QuantumExplorer
changed the title
Mirrors the waitForStateTransitionResult guard: a proved response without metadata cannot be verified, so reject it at the client boundary too instead of deferring to the SDK layer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t factories js-dash-sdk now fails closed without an IPlatformProofVerifier, but no implementation existed and no consumer wired one, so every proved broadcast and contract-history call would throw PlatformProofVerificationUnavailableError. Add createPlatformProofVerifier to the platform test suite and the bench suite: it is backed by @dashevo/evo-sdk (the Rust/WASM SDK), which authenticates results end to end — GroveDB proof verification plus the Tenderdash quorum signature over the root hash, with quorum keys served by the dashmate quorum-list API (enabled automatically on the local_seed node). Verification re-queries Platform through the WASM SDK's proved paths rather than re-checking the transport bytes: the returned data is quorum-authenticated, so the unverified DAPI response is never the source of truth. State transition results are confirmed by waiting on the proved endpoint for the same transition; contract history entries are returned from the verified query and become the data the caller consumes. The verifier maps the suite's existing NETWORK / DAPI_SEED / DAPI_ADDRESSES configuration to EvoSDK options (regtest -> local) and is wired into createClientWithoutWallet, createClientWithFundedWallet and createFaucetClient, plus the bench-suite client factory which shares the same env conventions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the merge blocker in 7eab96b (plus a clean v4.1-dev merge). The PR made js-dash-sdk fail closed without an This adds a real verifier and wires it in:
Validation: built wasm-sdk + evo-sdk and ran a Node smoke test covering the full path short of a live network — wasm init under Node, every API the verifier touches ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/js-dash-sdk/src/SDK/Client/Platform/broadcastStateTransition.spec.ts (1)
4-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a happy-path test for verified broadcast.
Only the two failure branches (missing verifier, missing proof/metadata) are covered. Consider adding a test where the verifier resolves and
verifyStateTransitionResultis actually invoked and the result returned, to lock in the primary success flow of this security-sensitive path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/js-dash-sdk/src/SDK/Client/Platform/broadcastStateTransition.spec.ts` around lines 4 - 56, Add a success-path test alongside the existing cases for broadcastStateTransition, configuring a proof verifier whose verifyStateTransitionResult resolves and DAPI stubs that return valid proof metadata. Assert the verifier is invoked with the broadcast result and that broadcastStateTransition returns the verified result, while preserving the existing failure tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bench-suite/lib/client/createPlatformProofVerifier.js`:
- Around line 16-39: Update verifyStateTransitionResult and
verifyDataContractHistory to retain each input’s network and protocolVersion,
and validate the requested network against the EvoSDK network used by getEvoSdk.
Ensure getEvoSdk does not reuse a client for a different network; throw on any
mismatch before verification rather than silently authenticating against the
environment-selected network.
In
`@packages/js-dash-sdk/src/SDK/Client/Platform/broadcastStateTransition.spec.ts`:
- Line 5: Rename the affected test titles in the broadcast state transition spec
so both begin with “should …”, including the tests currently titled “fails
before broadcasting…” and “rejects a success response…”. Preserve the existing
test behavior and make the titles remain descriptive.
In
`@packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/history.spec.ts`:
- Around line 106-115: Rename the test case around the history.call invocation
to describe that it fetches from DAPI again rather than reading from local
cache. Keep the existing assertions and test behavior unchanged, including
askedFromDapi equaling 2 and requestedProofModes.
In `@packages/platform-test-suite/lib/test/createPlatformProofVerifier.js`:
- Around line 16-39: Update the platform proof verifier flow around
resolveNetwork and its IPlatformProofVerifier implementation to preserve and
forward each call’s network and protocolVersion options instead of relying only
on process.env.NETWORK. Ensure EvoSDK connections are memoized per resolved
network/devnet identity, not globally from the first call, and apply the same
correction to the corresponding bench-suite implementation.
---
Nitpick comments:
In
`@packages/js-dash-sdk/src/SDK/Client/Platform/broadcastStateTransition.spec.ts`:
- Around line 4-56: Add a success-path test alongside the existing cases for
broadcastStateTransition, configuring a proof verifier whose
verifyStateTransitionResult resolves and DAPI stubs that return valid proof
metadata. Assert the verifier is invoked with the broadcast result and that
broadcastStateTransition returns the verified result, while preserving the
existing failure tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46333e8f-d413-4cad-9ffa-67782dc23f8d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
.pnp.cjspackages/bench-suite/lib/client/createClientWithFundedWallet.jspackages/bench-suite/lib/client/createPlatformProofVerifier.jspackages/bench-suite/package.jsonpackages/js-dapi-client/lib/methods/platform/getDataContractHistory/getDataContractHistoryFactory.jspackages/js-dapi-client/lib/methods/platform/waitForStateTransitionResult/waitForStateTransitionResultFactory.jspackages/js-dapi-client/test/unit/methods/platform/getDataContractHistory/getDataContractHistoryFactory.spec.jspackages/js-dapi-client/test/unit/methods/platform/waitForStateTransitionResult/waitForStateTransitionResultFactory.spec.jspackages/js-dash-sdk/src/SDK/Client/Client.spec.tspackages/js-dash-sdk/src/SDK/Client/Client.tspackages/js-dash-sdk/src/SDK/Client/Platform/Fetcher/Fetcher.tspackages/js-dash-sdk/src/SDK/Client/Platform/IPlatformProofVerifier.tspackages/js-dash-sdk/src/SDK/Client/Platform/Platform.tspackages/js-dash-sdk/src/SDK/Client/Platform/broadcastStateTransition.spec.tspackages/js-dash-sdk/src/SDK/Client/Platform/broadcastStateTransition.tspackages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/history.spec.tspackages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/history.tspackages/js-dash-sdk/src/SDK/SDK.tspackages/js-dash-sdk/src/errors/PlatformProofVerificationUnavailableError.tspackages/js-dash-sdk/src/test/mocks/createDapiClientMock.tspackages/platform-test-suite/lib/test/createClientWithFundedWallet.jspackages/platform-test-suite/lib/test/createClientWithoutWallet.jspackages/platform-test-suite/lib/test/createFaucetClient.jspackages/platform-test-suite/lib/test/createPlatformProofVerifier.jspackages/platform-test-suite/package.json
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4168 +/- ##
============================================
- Coverage 87.51% 87.51% -0.01%
============================================
Files 2667 2667
Lines 336972 336972
============================================
- Hits 294903 294902 -1
- Misses 42069 42070 +1
🚀 New features to boost your workflow:
|
|
Status on the dispatched full-pipeline validation run (29951833896): it failed, but every failure traces to All of that is fixed in #4205. Once it merges I'll re-dispatch the full pipeline on this branch to get the real e2e validation of the proof verifier wiring. |
Address review feedback on the proof verifier wiring: - The verifier ignored the per-call network from IPlatformProofVerifier and memoized its EvoSDK connection from process.env.NETWORK, so a client configured for a different network would silently verify against the wrong quorum set. Every verification now asserts the caller's network (normalized the same way) matches the connected one and throws on mismatch, in both the platform-test-suite and bench-suite copies. - Add a happy-path broadcastStateTransition test locking in that the verifier is invoked with the full binding input and the proved result is returned. - Rename test titles to the "should ..." convention and retitle the history re-fetch test that claimed caching behavior history() does not have. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
historyUnprovedmethodCovers security review findings DS-CAND-025 and DS-CAND-354.
Compatibility
This intentionally changes the legacy JS SDK default from endpoint trust to fail-closed. Callers must configure
platformProofVerifierfor transition broadcasts and verified history. Existing non-security-sensitive history integrations can migrate explicitly tocontracts.historyUnproved.Validation
yarn workspace dash build:tsyarn workspace dash test:unit(66 passing)yarn workspace @dashevo/dapi-client test:unit(317 passing)Summary by CodeRabbit
New Features
Bug Fixes
Tests