Skip to content

fix: re-enable shielded transactions now that mainnet supports protocol v12 - #906

Merged
lklimek merged 6 commits into
v1.0-devfrom
fix/reenable-shielded-transactions
Jul 21, 2026
Merged

fix: re-enable shielded transactions now that mainnet supports protocol v12#906
lklimek merged 6 commits into
v1.0-devfrom
fix/reenable-shielded-transactions

Conversation

@Claudius-Maginificent

@Claudius-Maginificent Claudius-Maginificent commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Why this PR exists

  • Problem: Shielded (Orchard) send/shield/unshield operations have been hardcoded closed in every build since PR feat(user-role): persona/capability-gating — role model, callsite reclassification, and role-picker UI #879, pending upstream shipping the shielded state transitions. They've since shipped — the connected network's protocol version is what actually gates them.
  • What breaks without it: A user on a network that has activated shielded state transitions (confirmed live on testnet at protocol v12 during verification for this PR, and per the reporter already active on mainnet) still sees the Shielded tab's Shield/Send/Unshield controls hidden, with no way to use functionality the network already supports. All the shielded UI/backend code has been built and working since PR feat: shielded transactions (ZK) integration #793 — only this one runtime gate was closed.
  • Blocking relationship: none.

What was done

  • src/context/feature_gate.rs: SHIELDED_ACTIVATION_PROTOCOL_VERSION now reads Some(SHIELDED_POOL_INITIAL_PROTOCOL_VERSION) (upstream rs-platform-version's own constant, protocol v12) instead of the placeholder None. Capability::ShieldedProtocol is met once the connected network's live protocol version reaches that value.
  • Replaced the tripwire test that was designed to fail the moment this constant changed with real coverage of the v12 activation boundary across roles (Developer below/at v12, Everyday at v12).
  • Added FeatureGate::first_unmet_check, a small generic addition that reports which check in a gate's conjunction is failing rather than a single opaque bool.
  • src/ui/wallets/shielded_tab.rs: the "shielded sending unavailable" notice now uses first_unmet_check to show one of two accurate messages instead of always blaming the network — distinguishing "your network doesn't support this yet" (capability) from "your interface mode doesn't unlock this yet" (still Power/Developer-only, unchanged gating — this PR only fixes the messaging).
  • CHANGELOG.md entries for both changes.

Testing

  • cargo test -p dash-evo-tool -- feature_gate: 14 tests pass, including the new/changed activation-boundary and first_unmet_check coverage (confirmed by name in the test log, not just an aggregate count).
  • cargo test -p dash-evo-tool -- shielded_tab: passes, including the two renamed/new i18n-label tests.
  • cargo clippy --all-features --all-targets -p dash-evo-tool -- -D warnings: clean.
  • Live GUI verification against the real compiled app (docs/gui-testing tier): built and ran the binary, confirmed all three states render correctly —
    • boot state (protocol v0, Developer role): shielded controls correctly hidden with the network-unavailable message
    • live testnet, which is already at protocol v12, Developer role: Shield/Send/Unshield controls appear and are usable
    • live testnet at v12, Everyday role: controls correctly stay hidden (role gate still holds), now showing the role-specific message
    • no panics or crashes in the app log across any of the above

Breaking changes

None. Shielded operations remain additionally gated behind the existing Power/Developer interface-mode requirement — this only unlocks the network-capability half of the gate, and only on networks that have actually activated the feature.

Checklist

  • Tests added/updated and passing
  • Clippy clean
  • Verified against the real running app, not just unit tests
  • CHANGELOG updated

Attribution

🤖 Co-authored by Claudius the Magnificent AI Agent

Summary by CodeRabbit

  • New Features

    • Enabled shielded sending, receiving, shielding, and unshielding on supported networks, including mainnet.
    • Added clearer availability notices distinguishing network support from interface permissions.
  • Bug Fixes

    • Shielded operations now report the specific reason they are unavailable.
    • Prevented unavailable shielded actions from attempting to access wallet functionality.

lklimek and others added 2 commits July 17, 2026 09:32
Shielded state transitions are live on mainnet as of upstream protocol
v12 (rs-platform-version::SHIELDED_POOL_INITIAL_PROTOCOL_VERSION).
Point the capability gate at that constant instead of the placeholder
`None` set while the feature was unshipped, and replace the tripwire
test with real coverage of the v12 activation boundary across roles.

Co-Authored-By: Codex Sol <noreply@openai.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@lklimek, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 55 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5e81a4bf-a08d-4df3-9ece-7cb8dab43241

📥 Commits

Reviewing files that changed from the base of the PR and between 0bae807 and 1dd79d4.

📒 Files selected for processing (5)
  • docs/user-stories.md
  • src/mcp/resolve.rs
  • src/mcp/tools/shielded.rs
  • tests/backend-e2e/framework/harness.rs
  • tests/backend-e2e/framework/shielded_helpers.rs
📝 Walkthrough

Walkthrough

Walkthrough

Shielded operations now activate on supported protocol versions. Feature gates identify whether availability fails because of network capability or interface role, and backend errors and UI notices report the corresponding reason.

Changes

Shielded Operations Availability

Layer / File(s) Summary
Protocol activation and unmet-check reporting
src/context/feature_gate.rs
Shielded capability activation uses SHIELDED_POOL_INITIAL_PROTOCOL_VERSION, while FeatureGate::first_unmet_check identifies the first failing condition with updated coverage.
Backend error mapping and dispatch
src/backend_task/error.rs, src/backend_task/mod.rs, src/backend_task/shielded/mod.rs
Shielded task gating maps network and role restrictions to distinct errors and prevents blocked role-gated operations from reaching the wallet backend.
Shielded tab availability notices
src/ui/wallets/shielded_tab.rs, CHANGELOG.md
The shielded tab renders separate network and role notices, preserves action locking, and documents the supported-network behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ShieldedTabView
  participant FeatureGate
  participant AppContext
  participant WalletBackend
  ShieldedTabView->>FeatureGate: first_unmet_check(AppContext)
  FeatureGate->>AppContext: inspect protocol and interface role
  AppContext-->>FeatureGate: unmet check or available
  alt Network capability unavailable
    FeatureGate-->>ShieldedTabView: Capability::ShieldedProtocol
    ShieldedTabView-->>ShieldedTabView: render network notice
  else Role unavailable
    FeatureGate-->>ShieldedTabView: ExperimentalFeature::Shielded
    ShieldedTabView-->>ShieldedTabView: render role notice
  else Available
    FeatureGate-->>ShieldedTabView: no unmet check
    ShieldedTabView->>WalletBackend: execute shielded operation
  end
Loading

Suggested reviewers: lklimek

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: re-enabling shielded transactions after protocol v12 activation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/reenable-shielded-transactions

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lklimek
lklimek marked this pull request as ready for review July 17, 2026 11:15
@thepastaclaw

thepastaclaw commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

🕓 Ready for review — 27 ahead in queue (commit 1dd79d4)
Queue position: 28/29

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary review — Codex only

The protocol-v12 activation boundary and role-specific UI messaging are implemented consistently, but the protocol-version value is only populated by a one-shot epoch-info request. A failed request or an app session spanning activation can therefore leave supported shielded operations disabled until reconnection or a manual refresh. The backend also discards the typed unavailability reason, and the tests do not directly pin the Power-role activation boundary.

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 — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
  • Verifier: gpt-5.6-sol — verifier
  • Sonnet: not run (deferred by blocker gate)

🔴 1 blocking | 🟡 2 suggestion(s)

2 additional finding(s) omitted (not in diff).

🤖 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 `src/context/feature_gate.rs`:
- [BLOCKING] src/context/feature_gate.rs:41: Protocol-version cache is never refreshed after initial sync
  This gate treats `platform_protocol_version()` as current network state, but the only production setter runs after a successful `CurrentEpochInfo` request. `ConnectionBanner` dispatches that request only when the connection transitions into `OverallConnectionState::Synced`; it does not retry a failed request while the state remains synced or refresh the value when a later epoch activates protocol v12. Consequently, a transient failure during initial sync or an app session spanning activation leaves both the UI and authoritative backend guards rejecting shielded operations on a supported network until reconnection or a manual Platform Info request. This became an in-scope functional bug when this PR changed the cached value from informational state into the condition enabling shielded fund movements. Retry failed initial requests and refresh epoch information periodically or when the epoch changes.
- [SUGGESTION] src/context/feature_gate.rs:326-332: Test the Power role at the activation boundary
  The positive activation test covers Developer only, although the actual experimental threshold is `UserRole::Power` and the new UI message promises that Expert view unlocks shielded sending. The Everyday negative test does not verify this lower unlocked boundary, so changing the shielded gate to require Developer would leave all activation-specific tests passing while breaking the advertised Power-role behavior. Exercise both unlocked roles at protocol v12.

In `src/backend_task/error.rs`:
- [SUGGESTION] src/backend_task/error.rs:1859-1864: Preserve the shielded unavailability reason in the backend error
  The PR introduces `first_unmet_check()` so callers can distinguish an unsupported network from an insufficient interface role, but both authoritative backend guards still return the same fieldless `ShieldedOperationsUnavailable` variant. This is directly observable through the registered MCP shielded tools, which expose the variant's `Display` text: an Everyday user on protocol v12 is told to use a regular payment or wait for a future update even though switching to Expert view enables the operation, while a Power or Developer user below v12 receives application-update advice for a network capability restriction. Carry a typed unavailability reason in `TaskError`, construct it from the failed gate check in both backend guards, and provide an accurate actionable message for each reason.

Comment thread src/context/feature_gate.rs

@lklimek lklimek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grumpy review — consolidated findings

The core funds-safety logic holds up under adversarial review. This is the part that matters, since this PR gates real funds-relevant functionality (shielded transactions on mainnet), so it was verified rather than taken on faith — two independent reviewers read the pinned upstream source and ran the suites, and neither found a version-check or bypass defect:

  • SHIELDED_ACTIVATION_PROTOCOL_VERSION sources upstream's own SHIELDED_POOL_INITIAL_PROTOCOL_VERSION (= 12) directly, matching drive-abci's consensus is_allowed comparison (activation at v12, rejection at v11). No off-by-one, no inverted logic — the operator is >=.
  • Fails closed at boot: the atomic starts at 0, so every role is blocked until a live epoch fetch overwrites it.
  • Per-network, not global: ctx.platform_protocol_version() is read fresh from the live AppContext, so mainnet stays gated until its own connection reports v12, regardless of testnet/devnet state. No cross-network leakage.
  • Conjunctive AND semantics: capability and role checks are independent — a Developer role cannot buy past a failing capability check. No privilege-escalation path.
  • No stray or duplicate hardcoded shielded-version comparison exists anywhere else in the tree — one source of truth, and all four production call sites read the same predicate.
  • shielded_capability_tracks_the_activation_boundary sweeps every upstream-defined protocol version against the exact >= predicate — the strongest boundary coverage short of a property test.

CHANGELOG accuracy is also worth crediting: the "Fixed" bullet scopes itself to the shielded_tab.rs notice split rather than claiming all messaging now distinguishes both reasons, and the "Changed" bullet's phrasing is conditional ("when the connected network's protocol version supports them") rather than asserting mainnet is live today.

The one substantive finding

CALL-001 (medium) — the PR's own advertised behavior is only half-applied. The commit title is "distinguish network vs role reason," and first_unmet_check() was added specifically for that, but only 1 of 4 production call sites uses it. The three backend sites — including every MCP shielded tool — still return the undifferentiated TaskError::ShieldedOperationsUnavailable, whose text says "try again after a future update." For MCP/CLI callers (which default to UserRole::Everyday and have no role-switching tool at all), that guidance is simply false: no protocol upgrade fixes a role gate. Inline detail below. Worth fixing or explicitly deferring with a tracked follow-up — it's a messaging accuracy issue, not a safety one.

Two findings that can't be inlined (files outside this diff)

  • CODE-002 (low, pre-existing, not caused by this PR)tests/backend-e2e/framework/shielded_helpers.rs:14-22: is_shielded_available claims to test whether the network supports shielded state transitions, but calls FeatureGate::Shielded, whose check list is intentionally empty so balances stay viewable everywhere. It therefore returns true before v12. Callers in tc_074_shielded_lifecycle and tc_079_shield_from_balance don't skip unsupported networks, so TC-079 can fund a Platform address before the (correctly fail-closed) backend rejects the write. This predates the PR — pre-PR the gate was unconditionally closed, so the same wasted setup already happened — and production still fails closed, so it defeats an E2E preflight and wastes funded setup, nothing more. Suggest switching it to FeatureGate::ShieldedOperations.is_available(app_context).
  • PROJ-001 (low)docs/user-stories.md:276-284 (WAL-029) and 368-454 (SND-007/009/010/015/016): these six stories are tagged [Implemented] and describe the flows as simply reachable, with no acceptance criterion for the two-axis gating (network protocol v12 AND Expert view or higher) that now actually governs them. A reader can't learn from the doc that these were non-functional everywhere until this PR. CLAUDE.md's catalog policy covers adding stories and flipping [Gap] tags, not refreshing criteria on already-[Implemented] ones — so this is a suggested improvement, not a process violation.

Dropped as duplicate

One finding (no direct UserRole::Power assertion at the activation boundary) was dropped — @thepastaclaw already filed it on feature_gate.rs:332 with a suggestion patch. Independent agreement on that one; the existing thread stands.

Verdict: no funds-safety defect in the activation gate. Resolve CALL-001 or defer it explicitly; the rest are non-blocking follow-ups.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)

Comment thread src/ui/wallets/shielded_tab.rs Outdated
Comment thread src/ui/wallets/shielded_tab.rs Outdated
Comment thread src/ui/wallets/shielded_tab.rs Outdated
Preserve the first unmet shielded feature-gate check through both backend refusal layers, and keep the UI classification single-evaluation and exhaustive.

Co-Authored-By: OpenAI Codex <noreply@openai.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary review — Codex only

Live PR HEAD and local HEAD both resolve to ecf9874. The latest delta fixes the Power-role boundary coverage and typed backend unavailability errors, but the prior blocking protocol-cache lifecycle issue remains: GUI refresh is one-shot on a Synced transition, while standalone MCP/CLI never populates the cache. Supported shielded fund-moving operations can therefore remain disabled indefinitely.

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 — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
  • Verifier: gpt-5.6-sol — verifier
  • Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

1 additional finding(s) omitted (not in diff).

🤖 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 `src/context/feature_gate.rs`:
- [BLOCKING] src/context/feature_gate.rs:41: Protocol-version cache is never refreshed after initial sync
  The shielded capability now treats `AppContext::platform_protocol_version()` as authoritative network state, but the cache lifecycle does not maintain that invariant. The cache starts at `0`, and the only production setter is the successful `CurrentEpochInfo` path in `src/backend_task/platform_info.rs`. In the GUI, `ConnectionBanner` dispatches that request only when the overall connection state transitions to `Synced`; a transient fetch failure is not retried while the connection remains synced, and a session spanning a later protocol activation retains its previous version. Standalone MCP/CLI initialization and `ensure_spv_synced` never request current epoch information, so those contexts retain `0` and reject every shielded fund-moving tool as network-unavailable even on protocol v12. Because this PR makes the cache the authoritative guard that re-enables these operations, initialize it in every runtime path and refresh or retry it when epoch information changes or fetching fails.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary review — Codex only

The protocol-v12 boundary, Power-role coverage, and typed backend refusal errors are correct at the current head. However, the protocol-version cache remains a one-shot GUI-only update and is never populated by standalone MCP/CLI, so supported shielded writes can remain disabled indefinitely.

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 — general (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
  • Verifier: gpt-5.6-sol — verifier
  • Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

1 additional finding(s) omitted (not in diff).

🤖 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 `src/context/feature_gate.rs`:
- [BLOCKING] src/context/feature_gate.rs:41: Maintain the protocol-version cache as authoritative network state
  Shielded operations now depend on `platform_protocol_version()`, but the cache starts at 0 and its only production setter is the successful `CurrentEpochInfo` path in `src/backend_task/platform_info.rs`. The GUI requests that information only when `ConnectionReconciler` transitions into `Synced`, so a failed request is not retried while the connection remains synced and a session spanning a later protocol activation retains its old value. Standalone MCP/CLI creates the context with version 0, and `ensure_spv_synced()` never fetches epoch information, causing every shielded write to be rejected as network-unavailable even on protocol v12. Populate this state in every runtime path and refresh or retry it when fetching fails or the current epoch changes.

The shielded-operations feature gate depends on platform_protocol_version,
which was only ever populated by the GUI reconciler on a Synced-state edge.
Headless MCP/CLI callers never triggered that fetch, so the version stayed
at its zero boot value forever and shielded operations stayed refused even
on a live mainnet running protocol v12.

ensure_spv_synced now fetches/refreshes CurrentEpochInfo when the protocol
version is unpopulated, with retry on failure, and every shielded
fund-moving MCP tool requires fresh protocol metadata before dispatch.

Also fixes the backend-e2e preflight helper to check the correct
FeatureGate variant (ShieldedOperations, not the always-true Shielded gate)
and adds the missing protocol/role acceptance-criteria axis to the
SND-007/009/010/015/016 user stories.

Co-Authored-By: Codex GPT-5 <noreply@openai.com>
@lklimek lklimek added the claudius-review Triggers automated code review using claudius plugin, runs as a CI job label Jul 21, 2026
@Claudius-Maginificent

Copy link
Copy Markdown
Collaborator Author

Pushed a fix for the headless shielded-protocol-version gap plus two smaller follow-ups:

  • MCP/CLI's ensure_spv_synced now fetches/refreshes CurrentEpochInfo (with retry) when the protocol version is still unpopulated, so shielded operations actually become available headlessly on a v12 network instead of staying gated forever.
  • tests/backend-e2e/framework/shielded_helpers.rs now checks the correct FeatureGate::ShieldedOperations variant.
  • docs/user-stories.md SND-007/009/010/015/016 acceptance criteria updated to cover both the protocol-version and role axes.

Commit: 31029372

🤖 Co-authored by Claudius the Magnificent AI Agent

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary review — Codex only

The latest push correctly refreshes protocol metadata before headless shielded fund movements and fixes the prior Power-role and typed-unavailability gaps. The GUI still treats a transition-only protocol-version cache as live network state, so supported shielded operations can remain hidden or rejected after an initial epoch-query failure or an in-session activation; the new best-effort headless refresh also adds avoidable DAPI latency to Core-only tools.

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 — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
  • Verifier: gpt-5.6-sol — verifier
  • Sonnet: not run (deferred by blocker gate)

🔴 1 blocking | 🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 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 `src/context/feature_gate.rs`:
- [BLOCKING] src/context/feature_gate.rs:41: Maintain the GUI protocol-version cache as live network state
  This PR activates `ShieldedOperations` by reading `platform_protocol_version()` as authoritative network state, but the GUI still updates that cache only when `ConnectionBanner::update` observes a transition into `OverallConnectionState::Synced` (`src/app/reconcilers.rs:330-337`). The reconciler records `Synced` whether the dispatched `CurrentEpochInfo` request succeeds or fails, so a failed request is not retried while the state stays synced; likewise, a GUI session that cached a pre-v12 epoch never observes a later v12 activation without a connection cycle, a manual epoch query, or a restart. The new refresh in `mcp::resolve` covers MCP/CLI calls only, while GUI controls and both backend shielded guards continue reading the stale atomic value, causing the PR's newly supported operations to remain hidden and be rejected on a supported network.

In `src/mcp/resolve.rs`:
- [SUGGESTION] src/mcp/resolve.rs:316-319: Bound the best-effort refresh for unrelated Core tools
  `BestEffortIfUnpopulated` awaits the full `CurrentEpochInfo` backend task before returning, even though `ensure_spv_synced` is called by Core-only operations such as `core_address_create`, `core_balances_get`, and `core_funds_send` that do not consume the protocol cache. The SDK is configured with a 10-second request timeout and six retries (`src/sdk_wrapper.rs:14-18`), and the error is discarded only after that wait; if the request fails, the cache remains zero and every later Core call repeats the delay. This is a non-blocking latency regression rather than a correctness blocker because the Core operation is still allowed to proceed after the refresh attempt completes, but the best-effort path should be bounded, single-flight/background, or limited to Platform-dependent callers.

Comment thread src/mcp/resolve.rs
Comment on lines +316 to +319
match protocol_refresh {
ProtocolRefresh::BestEffortIfUnpopulated if ctx.platform_protocol_version() == 0 => {
let _ = refresh_platform_protocol_version(ctx).await;
Ok(())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Bound the best-effort refresh for unrelated Core tools

BestEffortIfUnpopulated awaits the full CurrentEpochInfo backend task before returning, even though ensure_spv_synced is called by Core-only operations such as core_address_create, core_balances_get, and core_funds_send that do not consume the protocol cache. The SDK is configured with a 10-second request timeout and six retries (src/sdk_wrapper.rs:14-18), and the error is discarded only after that wait; if the request fails, the cache remains zero and every later Core call repeats the delay. This is a non-blocking latency regression rather than a correctness blocker because the Core operation is still allowed to proceed after the refresh attempt completes, but the best-effort path should be bounded, single-flight/background, or limited to Platform-dependent callers.

source: ['codex']

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed. ProtocolRefresh::BestEffortIfUnpopulated (src/mcp/resolve.rs:316-319) still .awaits the full refresh_platform_protocol_version dispatch and only discards the error afterward — it doesn't return early. Since ensure_spv_synced (which uses this best-effort path) is the same gate used by Core-only tools (core_address_create, core_balances_get, core_funds_send, etc.), any of them can pay the SDK's full 10s-timeout × 6-retry latency on every call while platform_protocol_version() is unpopulated or the query keeps failing. Non-blocking (the Core operation still proceeds after), but agreed it should be bounded, single-flight/background, or skipped entirely for callers that don't consume the protocol cache.

Verified by Claudius the Magnificent AI Agent

@lklimek
lklimek enabled auto-merge (squash) July 21, 2026 07:12
@lklimek
lklimek disabled auto-merge July 21, 2026 07:13
@lklimek
lklimek enabled auto-merge (squash) July 21, 2026 07:13
@lklimek
lklimek merged commit 0812761 into v1.0-dev Jul 21, 2026
3 of 5 checks passed
@lklimek
lklimek deleted the fix/reenable-shielded-transactions branch July 21, 2026 07:17
@Claudius-Maginificent

Copy link
Copy Markdown
Collaborator Author

Ran a comment-verification pass on this PR. One note: thepastaclaw's latest review (2026-07-21T03:30:01Z, head 31029372) describes a blocking finding in src/context/feature_gate.rs:41 that doesn't appear to have a corresponding inline review thread, so flagging it here directly:

The GUI treats a one-shot protocol-version cache as live network state. FeatureGate::ShieldedOperations reads ctx.platform_protocol_version() as authoritative (feature_gate.rs:41), but the GUI only refreshes that value once, on the first transition into OverallConnectionState::Synced (src/app/reconcilers.rs:328-337) — confirmed by reading the code: self.previous_state is set to Synced regardless of whether the dispatched CurrentEpochInfo request succeeds, and there's no re-dispatch on later frames while the connection stays Synced. So a failed initial epoch query, or a later in-session protocol activation (e.g. crossing v12), never gets re-observed by the GUI without a full connection cycle or restart — leaving newly-supported shielded operations hidden or wrongly rejected. The new mcp::resolve refresh only covers MCP/CLI, not the GUI path or the backend shielded guards.

The one existing inline thread (best-effort refresh latency for Core-only tools) was also verified still open — replied there directly.

🤖 Co-authored by Claudius the Magnificent AI Agent

orchardpaytl pushed a commit to orchardpaytl/orchardpay that referenced this pull request Jul 21, 2026
Shielded state transitions are live on mainnet as of upstream protocol
v12 (rs-platform-version::SHIELDED_POOL_INITIAL_PROTOCOL_VERSION).
Point the capability gate at that constant instead of the placeholder
`None` set while the feature was unshipped, and replace the tripwire
test with real coverage of the v12 activation boundary across roles.

Cherry-picked from dashpay/dash-evo-tool PR dashpay#906 (51e74f9).

Co-Authored-By: Lukasz Klimek <842586+lklimek@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
orchardpaytl pushed a commit to orchardpaytl/orchardpay that referenced this pull request Jul 21, 2026
…otice

Split the single "shielded sending is not available" label into a
network-gated variant and a role-gated (Expert view required) variant,
so the notice tells the user which of the two closed gates actually
applies to them instead of a generic message.

Cherry-picked from dashpay/dash-evo-tool PR dashpay#906 (3a0a6cc).

Co-Authored-By: Lukasz Klimek <842586+lklimek@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
orchardpaytl pushed a commit to orchardpaytl/orchardpay that referenced this pull request Jul 21, 2026
Preserve the first unmet shielded feature-gate check through both
backend refusal layers (the pre-dispatch check in run_backend_task and
the in-handler check in run_shielded_task), so a network-gated refusal
and a role-gated refusal report distinct TaskError variants instead of
collapsing into one generic "unavailable" error. Keeps the UI
classification single-evaluation and exhaustive.

Cherry-picked from dashpay/dash-evo-tool PR dashpay#906 (ecf9874).

Co-Authored-By: Lukasz Klimek <842586+lklimek@users.noreply.github.com>
Co-Authored-By: OpenAI Codex <noreply@openai.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
orchardpaytl pushed a commit to orchardpaytl/orchardpay that referenced this pull request Jul 21, 2026
Brings in: shutdown fix (dashpay#905), duplicate-DPNS-name error message
(dashpay#915), startup banner clearing (dashpay#916), nav pointer cursor + tooltips
and wallet-less masternode indication (dashpay#917), onboarding disconnected-
banner suppression (dashpay#907), masternode dialog/nav/passphrase fixes
(dashpay#913), DAPI auto-refresh during pre-1.0 migration (dashpay#908), "Add
Receiving Address" wiring + its test hardening (dashpay#914, dashpay#920), and a CI
timeout bump (dashpay#912). dashpay#906 (shielded re-enable) was already pulled in
individually last session, so its squashed commit merged as a no-op.

Conflicts (6 files) were rebrand-naming overlaps (dash_evo_tool::  vs
orchardpay::  imports) plus one real merge in left_panel.rs, where
OrchardPay's green-icon tint had to combine with upstream's new nav
tooltip. Also fixed 5 files upstream's auto-merged (non-conflicting)
additions left un-rebranded: a stray DASH_EVO_DATA_DIR_LOCK/env-var
name in a new app.rs test, and dash_evo_tool:: references in three
kittest test files.

Added tooltip strings for OrchardPay's own nav entries (OrchardPay,
DashPay) so the new every_nav_entry_has_a_tooltip test covers them —
upstream's version only knows its own nav items. Fixed the new
nav_label_hover_shows_pointer_cursor kittest test: OrchardPay's nav
rail carries two more always-visible entries than upstream's, pushing
"Settings" below the scrollable list's default-size visible viewport;
scroll it into view first, matching what a real user would do.

Verified: cargo check (both feature modes), cargo clippy --all-features
--all-targets -- -D warnings, cargo fmt --all, cargo test --all-features
--workspace (2066 lib + 257 kittest + doc tests, 0 failed), all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claudius-review Triggers automated code review using claudius plugin, runs as a CI job

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants