From bfd697b6e2605e63c6591c5de9c0a14e34275ca3 Mon Sep 17 00:00:00 2001 From: hellno Date: Fri, 3 Jul 2026 15:00:57 +0200 Subject: [PATCH 1/3] =?UTF-8?q?feat(policy):=20enforce=20the=20per-tx=20ca?= =?UTF-8?q?p=20on=20shields=20+=20honest=20authority=20line=20(E5=20=C2=B7?= =?UTF-8?q?=20#185)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The demo shield rule had no per_tx_cap_wei and per_tx_cap_for(Shield) returned None, so a 0.15 ETH shield auto-broadcast under a stated 0.1 ETH per-move cap. Rule::Shield gains per_tx_cap_wei; evaluate() — the one gate the mock and the daemon share — now enforces it on the shield path exactly as for a Send. policy.demo.json gains the 0.1 ETH shield cap; the agent's policy_get view exposes it. Every Rust Rule::Shield construction keeps per_tx_cap_wei: None, so the only behavior change is the demo file. Adds Policy::authority_for(kind, value) -> Authority so the shared Review's "Allowed by … daily left after this" line reads the SAME cap math evaluate() enforces: Authority.over_cap re-derives evaluate's over-cap test (pinned by a unit test), so the UI can never claim headroom the engine doesn't back. §6 regression: shield_over_per_tx_cap_needs_approval (+ within/never-mode) on evaluate, and the hermetic signerd e2e shield_cap_e2e.rs through the REAL daemon, prove an over-cap shield ASKS, never auto-broadcasts. Refs #179. --- crates/deckard-app/src/welcome.rs | 1 + crates/deckard-contract/src/lib.rs | 5 +- crates/deckard-contract/src/mock.rs | 5 +- crates/deckard-contract/src/policy.rs | 283 +++++++++++++++++- .../deckard-contract/tests/harness_slice.rs | 1 + crates/deckard-mcp/src/sidecar.rs | 7 +- crates/deckard-mcp/tests/common/mod.rs | 1 + crates/deckard-signerd/src/policy_store.rs | 7 +- crates/deckard-signerd/tests/common/mod.rs | 1 + crates/deckard-signerd/tests/guardrail.rs | 5 +- crates/deckard-signerd/tests/parity.rs | 5 +- .../deckard-signerd/tests/shield_cap_e2e.rs | 115 +++++++ crates/deckard-signerd/tests/swap_parity.rs | 1 + policy.demo.json | 2 +- 14 files changed, 416 insertions(+), 23 deletions(-) create mode 100644 crates/deckard-signerd/tests/shield_cap_e2e.rs diff --git a/crates/deckard-app/src/welcome.rs b/crates/deckard-app/src/welcome.rs index 95ad479..d46bfe4 100644 --- a/crates/deckard-app/src/welcome.rs +++ b/crates/deckard-app/src/welcome.rs @@ -911,6 +911,7 @@ mod tests { }, Rule::Shield { approval: ApprovalMode::OverCap, + per_tx_cap_wei: None, }, Rule::Swap { tokens: Allowlist::Any, diff --git a/crates/deckard-contract/src/lib.rs b/crates/deckard-contract/src/lib.rs index 9b21e98..726f62b 100644 --- a/crates/deckard-contract/src/lib.rs +++ b/crates/deckard-contract/src/lib.rs @@ -47,7 +47,7 @@ pub use message_signing::{ }; pub use mock::MockSigner; pub use policy::{ - evaluate, evaluate_message, evaluate_order, Allowlist, ApprovalMode, Effect, Policy, + evaluate, evaluate_message, evaluate_order, Allowlist, ApprovalMode, Authority, Effect, Policy, PolicyError, Rule, POLICY_VERSION, }; pub use read_status::ReadStatus; @@ -130,6 +130,7 @@ mod roundtrip_tests { }, Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, }, Rule::Swap { tokens: Allowlist::Only(vec![Address::repeat_byte(0xCC)]), @@ -228,6 +229,7 @@ mod roundtrip_tests { }, Rule::Shield { approval: ApprovalMode::OverCap, + per_tx_cap_wei: None, }, Rule::Swap { tokens: Allowlist::Any, @@ -336,6 +338,7 @@ mod roundtrip_tests { spent_today_wei: U256::from(321u64), rules: vec![Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, }], }; roundtrip(&live); diff --git a/crates/deckard-contract/src/mock.rs b/crates/deckard-contract/src/mock.rs index d941464..9674ebe 100644 --- a/crates/deckard-contract/src/mock.rs +++ b/crates/deckard-contract/src/mock.rs @@ -506,7 +506,10 @@ mod tests { per_tx_cap_wei: Some(U256::from(per_tx)), recipients: Allowlist::Any, }, - Rule::Shield { approval: mode }, + Rule::Shield { + approval: mode, + per_tx_cap_wei: None, + }, Rule::Swap { tokens: Allowlist::Any, }, diff --git a/crates/deckard-contract/src/policy.rs b/crates/deckard-contract/src/policy.rs index 61b9964..c08ca35 100644 --- a/crates/deckard-contract/src/policy.rs +++ b/crates/deckard-contract/src/policy.rs @@ -126,9 +126,15 @@ pub enum Rule { per_tx_cap_wei: Option, recipients: Allowlist, }, - /// Railgun deposit to one's own 0zk balance — value moves to self, so no recipient set and - /// no per-tx cap. - Shield { approval: ApprovalMode }, + /// Railgun deposit to one's own 0zk balance — value moves to self, so no recipient set. It + /// DOES carry an optional per-tx cap: a shield still moves value off the public balance, and + /// the daily wall alone let a large deposit (0.15 ETH under a stated 0.1 per-move cap) + /// auto-broadcast (#185). `evaluate` enforces this cap on the shield path exactly as it does + /// for `Send`/`Unshield`. + Shield { + approval: ApprovalMode, + per_tx_cap_wei: Option, + }, /// Railgun withdraw back to a public balance. Forward-compat; not yet reachable (see the /// type-level note above). Unshield { @@ -180,10 +186,17 @@ impl Serialize for Rule { } map.end() } - Rule::Shield { approval } => { - let mut map = serializer.serialize_map(Some(2))?; + Rule::Shield { + approval, + per_tx_cap_wei, + } => { + let len = 2 + usize::from(per_tx_cap_wei.is_some()); + let mut map = serializer.serialize_map(Some(len))?; map.serialize_entry("action", "shield")?; map.serialize_entry("approval", approval)?; + if let Some(cap) = per_tx_cap_wei { + map.serialize_entry("per_tx_cap_wei", cap)?; + } map.end() } Rule::Unshield { @@ -331,13 +344,13 @@ impl<'de> Deserialize<'de> for Rule { }) } "shield" => { - reject(per_tx_cap_wei.is_some(), "per_tx_cap_wei")?; reject(recipients.is_some(), "recipients")?; reject(tokens.is_some(), "tokens")?; reject(targets.is_some(), "targets")?; Ok(Rule::Shield { approval: approval .ok_or_else(|| M::Error::missing_field("approval"))?, + per_tx_cap_wei, }) } "unshield" => { @@ -530,6 +543,24 @@ impl core::fmt::Display for PolicyError { impl std::error::Error for PolicyError {} +/// The inputs for the shared Review's **Allowed by** authority line (DESIGN §Clear-signing) — +/// produced by [`Policy::authority_for`] so the UI renders the *same* rule + cap the engine +/// enforces, never a recomputed figure that could drift. All wei; the UI formats. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Authority { + /// The label of the rule that governs the action ([`Rule::label`]) — the line's subject. + pub rule_label: &'static str, + /// The one global daily ceiling (the "of $Y" total). + pub daily_cap_wei: U256, + /// What remains of the daily ceiling AFTER this move (the "$X"): `daily_cap − (spent + value)`, + /// saturating at zero. + pub daily_remaining_after_wei: U256, + /// `true` when the move trips a cap (per-tx OR daily) — re-derives [`evaluate`]'s cap test so + /// the UI knows the "daily left after this" clause is a breach the danger line owns, not calm + /// headroom. Pinned to `evaluate`'s verdict by a unit test so the two can't drift. + pub over_cap: bool, +} + impl Rule { /// This rule's action as its wire tag — used for duplicate-detection error messages and /// to match a rule against an [`IntentKind`]. @@ -543,6 +574,19 @@ impl Rule { } } + /// A human label for the rule — the subject of the shared Review's **Allowed by** authority + /// line (`Send rule`, `Shield rule`, …). One source so the UI never invents a rule name, and + /// the label it shows names the exact rule `evaluate` matched. + pub fn label(&self) -> &'static str { + match self { + Rule::Send { .. } => "Send rule", + Rule::Shield { .. } => "Shield rule", + Rule::Unshield { .. } => "Unshield rule", + Rule::Swap { .. } => "Swap rule", + Rule::ContractCall { .. } => "Contract-call rule", + } + } + /// Does this rule govern `kind`? (There is no `Swap` `IntentKind`; the `Swap` rule is /// reached only via [`evaluate_order`]/[`Policy::swap_tokens`].) fn matches_kind(&self, kind: &IntentKind) -> bool { @@ -583,13 +627,14 @@ impl Policy { self.rules.iter().find(|rule| rule.matches_kind(&kind)) } - /// The per-tx cap carried by the rule for `kind`, if any. Only `Send`/`Unshield` rules - /// carry one; every other action (and a no-rule) yields `None`. + /// The per-tx cap carried by the rule for `kind`, if any. `Send`/`Shield`/`Unshield` rules can + /// carry one; every other action (and a no-rule) yields `None`. Shield joined this set in #185 + /// so a large deposit can't slip past the per-move cap on the daily wall alone. pub fn per_tx_cap_for(&self, kind: IntentKind) -> Option { match self.rule_for(kind)? { - Rule::Send { per_tx_cap_wei, .. } | Rule::Unshield { per_tx_cap_wei, .. } => { - *per_tx_cap_wei - } + Rule::Send { per_tx_cap_wei, .. } + | Rule::Shield { per_tx_cap_wei, .. } + | Rule::Unshield { per_tx_cap_wei, .. } => *per_tx_cap_wei, _ => None, } } @@ -613,7 +658,7 @@ impl Policy { pub fn approval_for(&self, kind: IntentKind) -> Option { match self.rule_for(kind)? { Rule::Send { approval, .. } - | Rule::Shield { approval } + | Rule::Shield { approval, .. } | Rule::Unshield { approval, .. } | Rule::ContractCall { approval, .. } => Some(*approval), Rule::Swap { .. } => None, @@ -632,6 +677,27 @@ impl Policy { }) .unwrap_or(&DENY_ALL) } + + /// The **Allowed by** authority-line inputs for a proposed `(kind, value)` — the rule that + /// governs the action plus the daily budget remaining AFTER the move (see [`Authority`]). The + /// UI renders these verbatim, so its cap figure is the *same* one [`evaluate`] enforces and can + /// never drift. Returns `None` when no rule governs `kind` (default-deny — there is no authority + /// to cite; the review shows the deny instead). Used only for the native-value paths + /// (`Send`/`Shield`/`Unshield`); a swap always asks and cites its `Swap rule` via [`Rule::label`] + /// directly, since `evaluate_order` enforces no numeric cap the daily line could truthfully claim. + pub fn authority_for(&self, kind: IntentKind, value: U256) -> Option { + let rule = self.rule_for(kind.clone())?; + let projected = self.spent_today_wei.saturating_add(value); + // Mirror `evaluate`'s cap test (per-tx OR daily) — a unit test pins the two together. + let over_daily = projected > self.daily_cap_wei; + let over_pertx = self.per_tx_cap_for(kind).is_some_and(|cap| projected > cap); + Some(Authority { + rule_label: rule.label(), + daily_cap_wei: self.daily_cap_wei, + daily_remaining_after_wei: self.daily_cap_wei.saturating_sub(projected), + over_cap: over_pertx || over_daily, + }) + } } /// **The** decision function. A *pure* `(Intent, Policy) -> Decision` with no I/O, no @@ -728,7 +794,7 @@ pub fn evaluate(intent: &Intent, policy: &Policy) -> Decision { fn rule_approval(rule: &Rule) -> ApprovalMode { match rule { Rule::Send { approval, .. } - | Rule::Shield { approval } + | Rule::Shield { approval, .. } | Rule::Unshield { approval, .. } | Rule::ContractCall { approval, .. } => *approval, Rule::Swap { .. } => ApprovalMode::Always, @@ -882,6 +948,7 @@ mod evaluate_order_tests { }, Rule::Shield { approval: ApprovalMode::OverCap, + per_tx_cap_wei: None, }, ], } @@ -1070,6 +1137,7 @@ mod evaluate_order_tests { let p = Policy { rules: vec![Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, }], ..base_policy() }; @@ -1118,6 +1186,7 @@ mod message_signing_tests { spent_today_wei: U256::ZERO, rules: vec![Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, }], } } @@ -1241,6 +1310,7 @@ mod policy_v2_tests { // A policy with only a Shield rule denies a Send with the NEW default-deny tag. let p = policy_with(vec![Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, }]); assert_eq!( evaluate(&send_intent(Address::repeat_byte(0x22), 10), &p), @@ -1297,6 +1367,7 @@ mod policy_v2_tests { }, Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, }, Rule::Swap { tokens: Allowlist::Any, @@ -1331,6 +1402,7 @@ mod policy_v2_tests { }, Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, }, Rule::Swap { tokens: Allowlist::Only(vec![Address::repeat_byte(0xCC)]), @@ -1371,9 +1443,146 @@ mod policy_v2_tests { fn swap_tokens_floor_is_deny_all_with_no_swap_rule() { let p = policy_with(vec![Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, }]); assert_eq!(p.swap_tokens(), &Allowlist::DenyAll); } + + // ── #185 cap-enforced-on-shields (TRUST-CRITICAL) ───────────────────────────────────────── + // A Shield now carries a per-tx cap, and `evaluate` (the ONE gate the mock AND the daemon + // call) enforces it on the shield path exactly as it does for a Send. The bug this locks + // shut: a 0.15 deposit auto-broadcast under a stated 0.1 per-move cap because + // `per_tx_cap_for(Shield)` returned `None` and the per-tx check silently short-circuited. + + /// A Shield intent with the non-empty calldata `calldata_ok` requires. `to`/`chain_id` are + /// immaterial to `evaluate` (shields carry no recipient allowlist), so only `value` varies. + fn shield_intent(value: u64) -> Intent { + Intent { + chain_id: 1, + to: Address::repeat_byte(0x33), + token: None, + value: U256::from(value), + calldata: Bytes::from_static(&[0x01, 0x02, 0x03, 0x04]), + kind: IntentKind::Shield, + } + } + + /// A demo-shaped shield rule: `over_cap` approval + a per-tx cap, under a high daily wall so + /// the per-tx cap is the ONLY fence that can trip (proving per-tx enforcement on the shield + /// path, not the daily wall). + fn shield_cap_policy(per_tx: u64) -> Policy { + Policy { + daily_cap_wei: U256::from(1_000_000u64), + ..policy_with(vec![Rule::Shield { + approval: ApprovalMode::OverCap, + per_tx_cap_wei: Some(U256::from(per_tx)), + }]) + } + } + + #[test] + fn shield_over_per_tx_cap_needs_approval() { + // THE regression: a shield OVER the stated per-move cap ASKS, never auto-broadcasts. + let p = shield_cap_policy(100); + assert!( + matches!( + evaluate(&shield_intent(150), &p), + Decision::NeedsApproval { .. } + ), + "a shield over the per-tx cap must be held for approval, not auto-allowed (#185)" + ); + } + + #[test] + fn shield_within_per_tx_cap_allows() { + // The other half: a within-cap shield still auto-allows (the fix doesn't over-block). + let p = shield_cap_policy(100); + assert_eq!(evaluate(&shield_intent(50), &p), Decision::Allow); + // Boundary: exactly at the cap is within (the check is strictly `>`). + assert_eq!(evaluate(&shield_intent(100), &p), Decision::Allow); + } + + #[test] + fn shield_over_per_tx_cap_denies_under_never_mode() { + // With `never` approval there is no card to authorise an over-cap move, so it DENIES + // (fail-closed) rather than silently broadcasting. + let p = Policy { + daily_cap_wei: U256::from(1_000_000u64), + ..policy_with(vec![Rule::Shield { + approval: ApprovalMode::Never, + per_tx_cap_wei: Some(U256::from(100u64)), + }]) + }; + assert_eq!( + evaluate(&shield_intent(150), &p), + Decision::Deny { + reason: deny_reasons::OVER_CAP.into() + } + ); + } + + #[test] + fn shield_per_tx_cap_is_read_by_the_accessor() { + // Locks the `per_tx_cap_for` arm that was the bug: it must now return the shield rule's cap. + let p = shield_cap_policy(100); + assert_eq!( + p.per_tx_cap_for(IntentKind::Shield), + Some(U256::from(100u64)) + ); + } + + #[test] + fn authority_for_over_cap_matches_evaluate() { + // Pin `authority_for.over_cap` (the UI's "is this a breach?" signal) to `evaluate`'s + // verdict across a value matrix, so the Allowed-by line can never claim headroom the + // engine doesn't back (the honest-enforced-cap invariant). + let p = shield_cap_policy(100); + for value in [1u64, 50, 100, 101, 150, 1_000_000, 2_000_000] { + let auth = p + .authority_for(IntentKind::Shield, U256::from(value)) + .expect("shield rule governs the kind"); + let asks = matches!( + evaluate(&shield_intent(value), &p), + Decision::NeedsApproval { .. } + ); + assert_eq!( + auth.over_cap, asks, + "authority_for.over_cap must equal evaluate's ask-verdict at value {value}" + ); + } + } + + #[test] + fn authority_for_reports_daily_remaining_after_the_move() { + // The "$X of $Y daily left after this" figures come straight off the policy, so the UI + // never recomputes cap math. Y = daily cap; X = daily cap − (spent + value). + let p = Policy { + daily_cap_wei: U256::from(1000u64), + spent_today_wei: U256::from(200u64), + ..policy_with(vec![Rule::Shield { + approval: ApprovalMode::OverCap, + per_tx_cap_wei: Some(U256::from(500u64)), + }]) + }; + let auth = p + .authority_for(IntentKind::Shield, U256::from(300u64)) + .unwrap(); + assert_eq!(auth.rule_label, "Shield rule"); + assert_eq!(auth.daily_cap_wei, U256::from(1000u64)); + // 1000 − (200 + 300) = 500 left after this move. + assert_eq!(auth.daily_remaining_after_wei, U256::from(500u64)); + assert!( + !auth.over_cap, + "300 is within the 500 per-tx and 1000 daily caps" + ); + } + + #[test] + fn authority_for_is_none_without_a_governing_rule() { + // Default-deny: no rule for the kind ⇒ no authority to cite (the review shows the deny). + let p = policy_with(vec![]); + assert_eq!(p.authority_for(IntentKind::Shield, U256::from(1u64)), None); + } } #[cfg(test)] @@ -1503,6 +1712,12 @@ mod rule_serde_tests { }, Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, + }, + // #185: a Shield WITH a per-tx cap must also survive CBOR (the new wire field). + Rule::Shield { + approval: ApprovalMode::OverCap, + per_tx_cap_wei: Some(U256::from(7u64)), }, Rule::Unshield { approval: ApprovalMode::Always, @@ -1522,6 +1737,36 @@ mod rule_serde_tests { assert_eq!(back, rule, "rule did not survive a CBOR round-trip"); } } + + #[test] + fn shield_per_tx_cap_json_shape_and_omission() { + // #185: the shield rule's optional per-tx cap emits as a 0x-hex string when present and is + // OMITTED when `None` (matching Send/Unshield), and decodes back to the same value. + let capped = Rule::Shield { + approval: ApprovalMode::OverCap, + per_tx_cap_wei: Some(U256::from(5u64)), + }; + let json: serde_json::Value = serde_json::to_value(&capped).unwrap(); + assert_eq!(json["action"], "shield"); + assert_eq!(json["per_tx_cap_wei"], "0x5"); + let back: Rule = serde_json::from_value(json).unwrap(); + assert_eq!(back, capped); + + let uncapped: Rule = + serde_json::from_str(r#"{"action":"shield","approval":"never"}"#).unwrap(); + assert_eq!( + uncapped, + Rule::Shield { + approval: ApprovalMode::Never, + per_tx_cap_wei: None, + } + ); + let uncapped_json: serde_json::Value = serde_json::to_value(&uncapped).unwrap(); + assert!( + uncapped_json.get("per_tx_cap_wei").is_none(), + "a None shield per_tx_cap_wei must be omitted, got {uncapped_json}" + ); + } } #[cfg(test)] @@ -1529,10 +1774,11 @@ mod demo_shape_check { use super::*; #[test] fn demo_json_from_the_plan_decodes_and_validates() { - // The exact policy.demo.json shape PR1 specifies (plan §policy.demo.json). + // The exact policy.demo.json shape (#185: the shield rule now carries a per-tx cap so a + // large deposit can't auto-broadcast past the stated per-move limit). let json = r#"{ "version":1, "default":"deny", "daily_cap_wei":"500000000000000000", "auto_shield_min_wei":"10000000000000000", - "rules":[ {"action":"shield","approval":"over_cap"}, + "rules":[ {"action":"shield","approval":"over_cap","per_tx_cap_wei":"100000000000000000"}, {"action":"send","approval":"over_cap","per_tx_cap_wei":"100000000000000000","recipients":"any"}, {"action":"swap","tokens":"any"} ] }"#; let p: Policy = serde_json::from_str(json).expect("demo policy decodes"); @@ -1543,5 +1789,12 @@ mod demo_shape_check { ); assert_eq!(p.recipients_for(IntentKind::Send), &Allowlist::Any); assert_eq!(p.swap_tokens(), &Allowlist::Any); + // #185: the demo shield rule is now capped at 0.1 ETH per move (equal to the send cap), + // so a 0.15 ETH shield asks instead of auto-broadcasting. + assert_eq!( + p.per_tx_cap_for(IntentKind::Shield), + Some(U256::from(100_000_000_000_000_000u128)), + "the demo shield rule carries the 0.1 ETH per-tx cap (#185)" + ); } } diff --git a/crates/deckard-contract/tests/harness_slice.rs b/crates/deckard-contract/tests/harness_slice.rs index 8e30d6b..016354c 100644 --- a/crates/deckard-contract/tests/harness_slice.rs +++ b/crates/deckard-contract/tests/harness_slice.rs @@ -40,6 +40,7 @@ fn demo_signer() -> MockSigner { // Shield rule MUST be `OverCap` (within cap ⇒ no card ⇒ Allow), not Always. Rule::Shield { approval: ApprovalMode::OverCap, + per_tx_cap_wei: None, }, Rule::Swap { tokens: Allowlist::Any, // any token (this harness exercises sends, not swaps) diff --git a/crates/deckard-mcp/src/sidecar.rs b/crates/deckard-mcp/src/sidecar.rs index ef5ea72..1e18fde 100644 --- a/crates/deckard-mcp/src/sidecar.rs +++ b/crates/deckard-mcp/src/sidecar.rs @@ -500,9 +500,14 @@ fn rule_json(rule: &Rule) -> serde_json::Value { "per_tx_cap_eth": per_tx_cap_wei.map(format_wei_as_eth), "recipients": allowlist_json(recipients), }), - Rule::Shield { approval } => json!({ + Rule::Shield { + approval, + per_tx_cap_wei, + } => json!({ "action": "shield", "approval": approval_mode_str(*approval), + "per_tx_cap_wei": per_tx_cap_wei.map(|c| c.to_string()), + "per_tx_cap_eth": per_tx_cap_wei.map(format_wei_as_eth), }), Rule::Unshield { approval, diff --git a/crates/deckard-mcp/tests/common/mod.rs b/crates/deckard-mcp/tests/common/mod.rs index 33a5099..0ab72d1 100644 --- a/crates/deckard-mcp/tests/common/mod.rs +++ b/crates/deckard-mcp/tests/common/mod.rs @@ -122,6 +122,7 @@ pub fn demo_policy() -> Policy { }, Rule::Shield { approval: ApprovalMode::OverCap, + per_tx_cap_wei: None, }, Rule::Swap { tokens: Allowlist::Any, // any token; swap tools land in the MCP child (#26) diff --git a/crates/deckard-signerd/src/policy_store.rs b/crates/deckard-signerd/src/policy_store.rs index 3f867e6..abab652 100644 --- a/crates/deckard-signerd/src/policy_store.rs +++ b/crates/deckard-signerd/src/policy_store.rs @@ -44,6 +44,7 @@ pub fn default_policy() -> Policy { rules: vec![ Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, }, Rule::Send { approval: ApprovalMode::Always, @@ -86,6 +87,7 @@ pub fn shield_only_policy() -> Policy { spent_today_wei: U256::ZERO, rules: vec![Rule::Shield { approval: ApprovalMode::Never, + per_tx_cap_wei: None, }], } } @@ -105,6 +107,7 @@ pub fn ask_me_everything_policy() -> Policy { rules: vec![ Rule::Shield { approval: ApprovalMode::Always, + per_tx_cap_wei: None, }, Rule::Send { approval: ApprovalMode::Always, @@ -430,7 +433,7 @@ mod tests { fn shield_only_allows_only_shield() { let p = shield_only_policy(); match p.rule_for(IntentKind::Shield) { - Some(Rule::Shield { approval }) => assert_eq!(*approval, ApprovalMode::Never), + Some(Rule::Shield { approval, .. }) => assert_eq!(*approval, ApprovalMode::Never), other => panic!("expected an auto-allow Shield rule, got {other:?}"), } assert!( @@ -449,7 +452,7 @@ mod tests { fn ask_me_everything_cards_every_action() { let p = ask_me_everything_policy(); match p.rule_for(IntentKind::Shield) { - Some(Rule::Shield { approval }) => assert_eq!(*approval, ApprovalMode::Always), + Some(Rule::Shield { approval, .. }) => assert_eq!(*approval, ApprovalMode::Always), other => panic!("expected an always-card Shield rule, got {other:?}"), } match p.rule_for(IntentKind::Send) { diff --git a/crates/deckard-signerd/tests/common/mod.rs b/crates/deckard-signerd/tests/common/mod.rs index 3fd1047..c20c4ab 100644 --- a/crates/deckard-signerd/tests/common/mod.rs +++ b/crates/deckard-signerd/tests/common/mod.rs @@ -84,6 +84,7 @@ pub fn test_policy() -> Policy { }, Rule::Shield { approval: ApprovalMode::OverCap, + per_tx_cap_wei: None, }, Rule::Swap { tokens: Allowlist::Any, diff --git a/crates/deckard-signerd/tests/guardrail.rs b/crates/deckard-signerd/tests/guardrail.rs index ad17d4a..8646109 100644 --- a/crates/deckard-signerd/tests/guardrail.rs +++ b/crates/deckard-signerd/tests/guardrail.rs @@ -62,7 +62,10 @@ fn write_mode_policy(dir: &std::path::Path, mode: ApprovalMode) { per_tx_cap_wei: Some(U256::from(PER_TX_CAP)), recipients: Allowlist::Any, }, - Rule::Shield { approval: mode }, + Rule::Shield { + approval: mode, + per_tx_cap_wei: None, + }, ], }; write_policy(dir, &policy); diff --git a/crates/deckard-signerd/tests/parity.rs b/crates/deckard-signerd/tests/parity.rs index 0fd7554..c6ea18f 100644 --- a/crates/deckard-signerd/tests/parity.rs +++ b/crates/deckard-signerd/tests/parity.rs @@ -72,7 +72,10 @@ fn policy( Allowlist::Only(allow) }, }, - Rule::Shield { approval: mode }, + Rule::Shield { + approval: mode, + per_tx_cap_wei: None, + }, ], } } diff --git a/crates/deckard-signerd/tests/shield_cap_e2e.rs b/crates/deckard-signerd/tests/shield_cap_e2e.rs new file mode 100644 index 0000000..a9ace9d --- /dev/null +++ b/crates/deckard-signerd/tests/shield_cap_e2e.rs @@ -0,0 +1,115 @@ +//! #185 regression (TRUST-CRITICAL): the per-transaction cap is enforced on the SHIELD path +//! end-to-end through the REAL daemon — a shield OVER the stated per-move cap is HELD for human +//! approval, never auto-broadcast. +//! +//! The bug this pins shut: `policy.demo.json`'s shield rule carried no `per_tx_cap_wei`, and +//! `per_tx_cap_for(Shield)` returned `None`, so a 0.15 ETH deposit auto-allowed under a stated +//! 0.1 ETH per-move cap. The contract-crate unit tests prove `evaluate` (the shared gate) now +//! enforces it; THIS test proves the daemon's shield propose path actually routes through that +//! gate with the durable spend counter synced — no daemon-side bypass. +//! +//! Hermetic: `propose` never broadcasts, so a dummy RPC is enough (no anvil, no network). The +//! shield intent is shaped (targets the chain's RelayAdapt, non-empty calldata) so it clears the +//! daemon's `shield_to_mismatch` / `undecodable` pre-checks and reaches the policy gate. + +mod common; + +use alloy_primitives::{Address, Bytes, U256}; +use deckard_contract::{ + ApprovalMode, Decision, Effect, Intent, IntentKind, Policy, ProposalOrigin, Rule, + POLICY_VERSION, +}; +use deckard_signerd::SignerClient; + +use common::*; + +const DUMMY_RPC: &str = "http://127.0.0.1:1"; // propose never broadcasts +const SEPOLIA: u64 = 11_155_111; +/// The Sepolia RelayAdapt (pinned in `shield_target.rs` against `railgun::chain_config`). A shield +/// is only admitted when it targets this address, so the intent must use it to reach the gate. +const RELAY_ADAPT: &str = "0x7e3d929EbD5bDC84d02Bd3205c777578f33A214D"; + +/// A shield intent with the non-empty stand-in calldata `calldata_ok` requires (the real Railgun +/// adapter call is validated downstream; `propose` only needs a decodable shape). Only `value` +/// varies across the sub-assertions. +fn shield(to: Address, value: u128) -> Intent { + Intent { + chain_id: SEPOLIA, + to, + token: None, + value: U256::from(value), + calldata: Bytes::from_static(&[0xde, 0xad, 0xbe, 0xef]), + kind: IntentKind::Shield, + } +} + +/// A policy whose SHIELD rule carries a per-tx cap (the #185 fix), under a daily wall set far +/// above the cap so the per-tx cap is the ONLY fence that can trip — the assertions below prove +/// per-tx enforcement on the shield path, not the daily wall. +fn shield_cap_policy(per_tx: u128) -> Policy { + Policy { + version: POLICY_VERSION, + default_effect: Effect::Deny, + revoked: false, + daily_cap_wei: U256::from(10_000_000_000_000_000_000u128), // 10 ETH — never the binding fence + auto_shield_min_wei: U256::from(10_000_000_000_000_000u128), + spent_today_wei: U256::ZERO, + rules: vec![Rule::Shield { + approval: ApprovalMode::OverCap, + per_tx_cap_wei: Some(U256::from(per_tx)), + }], + } +} + +#[tokio::test] +async fn shield_over_per_tx_cap_is_held_not_broadcast() { + let relay_adapt: Address = RELAY_ADAPT.parse().unwrap(); + let dir = TempDir::new("shield-cap"); + let _ = seal_account0(dir.path()); + // The stated per-move cap the demo advertises: 0.1 ETH. + let per_tx: u128 = 100_000_000_000_000_000; + write_policy(dir.path(), &shield_cap_policy(per_tx)); + let d = spawn_daemon(dir.path(), DUMMY_RPC, SEPOLIA, &[]); + let client = SignerClient::new(d.socket_path.clone()); + client.unlock(PASS).await.unwrap(); + + // A 0.05 ETH shield is WITHIN the 0.1 per-tx cap → auto-allowed (Sepolia is guardrail-exempt, + // so a within-cap auto-allow is not downgraded to a hold). + assert_eq!( + client + .propose( + &shield(relay_adapt, 50_000_000_000_000_000), + ProposalOrigin::App + ) + .await + .unwrap(), + Decision::Allow, + "a within-cap shield still auto-allows (the fix must not over-block)" + ); + + // A 0.15 ETH shield is OVER the 0.1 per-tx cap → HELD for human approval, NOT auto-broadcast. + // Before #185 this returned `Allow` (per_tx_cap_for(Shield) was None) — the exact bug. + assert!( + matches!( + client + .propose( + &shield(relay_adapt, 150_000_000_000_000_000), + ProposalOrigin::App + ) + .await + .unwrap(), + Decision::NeedsApproval { .. } + ), + "a shield over the per-tx cap must ASK, never auto-broadcast (#185)" + ); + + // Boundary: exactly at the cap is within (the check is strictly greater-than). + assert_eq!( + client + .propose(&shield(relay_adapt, per_tx), ProposalOrigin::App) + .await + .unwrap(), + Decision::Allow, + "a shield exactly at the per-tx cap is within it" + ); +} diff --git a/crates/deckard-signerd/tests/swap_parity.rs b/crates/deckard-signerd/tests/swap_parity.rs index face21d..9f84252 100644 --- a/crates/deckard-signerd/tests/swap_parity.rs +++ b/crates/deckard-signerd/tests/swap_parity.rs @@ -61,6 +61,7 @@ fn policy(allow_swap_tokens: Vec
, revoked: bool) -> Policy { }, Rule::Shield { approval: ApprovalMode::OverCap, + per_tx_cap_wei: None, }, ], } diff --git a/policy.demo.json b/policy.demo.json index dde7c85..615f9d5 100644 --- a/policy.demo.json +++ b/policy.demo.json @@ -4,7 +4,7 @@ "daily_cap_wei": "500000000000000000", "auto_shield_min_wei": "10000000000000000", "rules": [ - { "action": "shield", "approval": "over_cap" }, + { "action": "shield", "approval": "over_cap", "per_tx_cap_wei": "100000000000000000" }, { "action": "send", "approval": "over_cap", "per_tx_cap_wei": "100000000000000000", "recipients": "any" }, { "action": "swap", "tokens": "any" } ] From a7b4a00a9508f113925024ed23f1ff1955d57499 Mon Sep 17 00:00:00 2001 From: hellno Date: Fri, 3 Jul 2026 15:01:10 +0200 Subject: [PATCH 2/3] =?UTF-8?q?feat(review):=20the=20ONE=20shared=20clear-?= =?UTF-8?q?signing=20Review,=20every=20origin=20(E5=20=C2=B7=20#185)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self Send/Shield, the agent proposal (the divergent boxed card in activity_view DELETED), and a dapp message now render ONE review body — origin_header rail → transaction-as-hero amount (tx_hero) → full recipient (tx_recipient) → one danger line "This can't be undone." → amber cautions → quiet facts (From · Network · fee/net · Allowed by). Only the origin rail changes: "You are sending" (amber), " proposes" (cyan), " requests" (neutral). Confirm is the platform-aware ⌘↵ key_cap; the speculative site-trust copy and the prose arm-delay explainer are gone. Swap wears the same rail + "Allowed by: Swap rule", keeping its honest two-token hero. The Allowed-by line reads Policy::authority_for and is omitted when there is no truthful headroom to cite (no policy, or over cap) — never a claim the engine doesn't back. No blind approve stays intact: approve_target is unchanged and remains the sole gate; the agent Approve still resolves ONLY the still-pending reviewed record (§6 regression: routed_agent_review_still_cannot_blind_approve_a_settled_record). Wires the E1 primitives (origin_header/Origin, key_cap/KeyCap, kv_row) at their first call site and removes their dead_code allows. Refs #179. --- crates/deckard-app/src/activity_view.rs | 326 +++++++++++++-------- crates/deckard-app/src/commit_view.rs | 368 ++++++++++++++++-------- crates/deckard-app/src/send_view.rs | 21 +- crates/deckard-app/src/shield_view.rs | 2 - crates/deckard-app/src/swap_view.rs | 50 +++- crates/deckard-app/src/widgets.rs | 24 +- 6 files changed, 516 insertions(+), 275 deletions(-) diff --git a/crates/deckard-app/src/activity_view.rs b/crates/deckard-app/src/activity_view.rs index edfc386..17bf9bb 100644 --- a/crates/deckard-app/src/activity_view.rs +++ b/crates/deckard-app/src/activity_view.rs @@ -35,6 +35,7 @@ use std::time::{SystemTime, UNIX_EPOCH}; +use gpui::prelude::FluentBuilder; use gpui::{ div, px, Context, FontWeight, InteractiveElement, IntoElement, ParentElement, StatefulInteractiveElement, Styled, @@ -829,9 +830,16 @@ impl Shell { } } - /// The clear-signing review for a single proposed feed row — the shared trust card, with the - /// REAL breached-fence cite from the record (per-tx vs daily), never a hardcoded - /// "per-transaction cap". Confirm is `⌘Enter`. + /// The clear-signing review for a single proposed feed row — the ONE shared Review (DESIGN + /// §Clear-signing), NOT a divergent boxed card. Only the request-origin rail changes: an agent + /// proposal shows the cyan ` proposes`, a dapp message its neutral ` requests`, + /// otherwise `You are …`. A Tx renders the same transaction-as-hero (amount + full `To`) as the + /// self Send/Shield review via the shared `tx_hero`/`tx_recipient`; a swap/approve/message keeps + /// its native rows under the same rail. One danger line `This can't be undone.`, then the amber + /// breach caution naming the fence, then the **Allowed by** line (within-cap) or the native + /// facts. Confirm is the shared ⌘↵ key-cap; **the no-blind-approve guard is unchanged** — the + /// `⌘Enter`/click still routes through `approve_activity` → `approve_target`, which resolves ONLY + /// the still-pending record this card renders. pub fn render_activity_review( &self, record: &ActivityRecord, @@ -841,121 +849,162 @@ impl Shell { let fg = theme.foreground; let muted = theme.muted_foreground; let border = theme.border; - let surface = theme.secondary; let danger = theme.danger; let is_dark = theme.is_dark(); - let agent = theme::agent(is_dark); - let agent_tint = theme::agent_tint(is_dark); + let amber = theme::amber(is_dark); + let mono = theme.mono_font_family.clone(); + let fill = theme.muted; // bg.raise2 — the confirm button fill - let is_agent = record.origin == ProposalOrigin::Agent; let agent_handle = self.agent_handle(); - let subject = origin_subject(record.origin, &agent_handle); - let cite = cite_phrase(record.reason).unwrap_or("held for your approval"); - - let band = { - let lead = if is_agent { - agent_mark( - &agent_handle, - crate::tokens::MARK_MD, - crate::tokens::RADIUS_ROW, - agent, - agent_tint, - ) + let wallet_name = self.wallet_name(); + let verb = you_verb(&record.payload); + + // The request-origin rail — the ONLY thing that changes across origins. An agent proposal + // is the cyan handle; an App-origin dapp MESSAGE carries its domain in the payload, so it + // gets the neutral dapp rail (the one dapp attribution available without the deferred + // tx-origin wire plumbing, ADR-0001); anything else is You. Trust badge is `None` — we + // don't have site reputation (DESIGN: no speculative site-trust). + let origin = match record.origin { + ProposalOrigin::Agent => crate::widgets::Origin::Agent { + handle: &agent_handle, + }, + ProposalOrigin::App => match &record.payload { + PendingPayloadView::Message(m) if !m.origin.is_empty() => { + crate::widgets::Origin::Dapp { domain: &m.origin } + } + _ => crate::widgets::Origin::You { + account: &wallet_name, + verb, + }, + }, + }; + let origin_rail = crate::widgets::origin_header(origin, None, theme); + // `theme` is not borrowed past this point — the `self.*(cx)` calls below re-borrow it. + + // A Tx renders the shared transaction-hero (amount + full To) + the Allowed-by line; a + // swap/approve/message keeps its native rows. The amount is shown in full (never masked) — + // at the moment of authorization you must see exactly what you approve. + // + // `tx_hero` formats at 18 decimals (native ETH). Every agent Tx reaching this review IS + // native ETH — `deckard_send` hardcodes `token: None` (sidecar.rs) and Shield/Unshield move + // ETH — so `intent.token` is always `None` and the scale is correct. A non-18-decimal ERC-20 + // Tx would mis-scale, but that path is not reachable today; decoding ERC-20 amount/decimals + // for clear-signing is deferred with the rest of the dapp-tx work (ADR-0001). This matches + // the pre-E5 card, which also formatted the Amount row at 18 decimals — no regression. + let (hero, to, authority) = if let PendingPayloadView::Tx(intent) = &record.payload { + let unit = if intent.token.is_none() { + Some("ETH") } else { - div() - .size(crate::tokens::MARK_MD) - .rounded(crate::tokens::RADIUS_ROW) - .bg(theme::identity_square(is_dark)) - .into_any_element() + Some("tokens") }; - let band_bg = if is_agent { agent_tint } else { surface }; - h_flex() - .w_full() - .items_center() - .gap_3() - .px_3() - .py_2p5() - .rounded_lg() - .bg(band_bg) - .child(lead) - .child( - div() - .flex_1() - .min_w_0() - .text_sm() - .text_color(fg) - .child(format!( - "{subject} wants to {} · cites: {cite}", - payload_summary(&record.payload, self.mask) - )), - ) + let hero = crate::commit_view::tx_hero( + tx_noun(&intent.kind), + intent.value, + unit, + mono.clone(), + fg, + muted, + ); + let to = crate::commit_view::tx_recipient( + &intent.to.to_checksum(None), + mono.clone(), + fg, + muted, + theme::identity_square(is_dark), + ); + let auth = self.review_authority_row(intent.kind.clone(), intent.value, cx); + (Some(hero), Some(to), auth) + } else { + (None, None, None) + }; + let is_tx = hero.is_some(); + // Non-Tx payloads keep their native detail rows (which already carry the breach cite row). + let native_rows: Vec = if is_tx { + Vec::new() + } else { + self.activity_review_detail_rows(record, cx) }; + // For a Tx, the amber caution names WHY it's held (the breached fence); a within-cap hold + // shows the Allowed-by line instead, so the two are complementary. + let breach = if is_tx { + breach_caution(record.reason) + .map(|c| crate::widgets::caution_line(amber, muted, true, c)) + } else { + None + }; + let quiet = authority.map(|a| { + v_flex() + .w_full() + .child(crate::widgets::divider(border)) + .child(a) + .child(crate::widgets::divider(border)) + }); + + // The shared ⌘↵ confirm — armed amber (the no-blind-approve guard, not an arm-delay, is the + // safety here). Deny is one-key by design (fail-safe); Cancel leaves the review. + let approve = div() + .id("activity-approve") + .w_full() + .h(px(48.0)) + .rounded(crate::tokens::RADIUS_MODAL) + .border_1() + .border_color(border) + .bg(fill) + .flex() + .items_center() + .px_4() + .cursor_pointer() + .child( + div() + .font_weight(FontWeight::SEMIBOLD) + .text_sm() + .text_color(fg) + .child("Approve"), + ) + .child(div().flex_1()) + .child(crate::widgets::key_cap( + crate::widgets::KeyCap::CmdEnter, + true, + border, + muted, + amber, + mono.clone(), + )) + .on_click(cx.listener(|this, _, _, cx| this.approve_activity(cx))); activity_shell( v_flex() .w_full() .gap_4() - .child(activity_heading_block( - "Review request", - "Confirm exactly what leaves and where, then approve or deny.", - fg, - muted, - )) - .child(band) - // Danger early — but tell the truth about WHICH fence was breached. - .child( - h_flex() - .items_center() - .gap_1p5() - .child(Icon::new(IconName::TriangleAlert).text_color(danger).small()) - .child( - div() - .text_sm() - .text_color(danger) - .child(review_danger_line(record.reason)), - ), - ) - .child( - v_flex() - .w_full() - .p_4() - .rounded_lg() - .border_1() - .border_color(border) - .bg(surface) - .children(self.activity_review_detail_rows(record, cx)), - ) + .child(origin_rail) + .children(hero) + .children(to) + .child(crate::widgets::error_line(danger, "This can't be undone.")) + .children(breach) + .when(!native_rows.is_empty(), |col| { + col.child(v_flex().w_full().children(native_rows)) + }) + .children(quiet) + .child(approve) .child( h_flex() .w_full() .gap_2() - .child( - Button::new("activity-approve") - .primary() - .label("⌘Enter Approve") - .on_click(cx.listener(|this, _, _, cx| this.approve_activity(cx))), - ) .child( Button::new("activity-deny") .ghost() - .label("x Deny") + .label("Deny") .on_click(cx.listener(|this, _, _, cx| this.deny_activity(cx))), ) .child( Button::new("activity-cancel") .ghost() - .label("Esc Cancel") + .label("Cancel") .on_click( cx.listener(|this, _, _, cx| this.cancel_activity_review(cx)), ), ), ) - .child( - div() - .text_xs() - .text_color(muted) - .child("Approving authorizes this spend. It will be signed and broadcast, and you can't undo it."), - ) .into_any_element(), ) } @@ -1327,13 +1376,43 @@ fn settled_outcome_label(record: &ActivityRecord, mask: bool) -> String { settled_label(record).to_string() } -/// The danger line at the top of the review card — names the actual fence breached. -fn review_danger_line(reason: BreachedLimit) -> &'static str { +/// The action noun for the shared Review's transaction-hero label ("Sending" / "Shielding" / …), +/// by intent kind — mirrors the self Send/Shield review's `noun` so an agent's Tx reads identically. +fn tx_noun(kind: &IntentKind) -> &'static str { + match kind { + IntentKind::Send => "Sending", + IntentKind::Shield => "Shielding", + IntentKind::Unshield => "Withdrawing", + IntentKind::ContractCall => "Calling", + } +} + +/// The lowercase verb for a `You are {verb}` origin rail on an App-origin request, by payload. +fn you_verb(payload: &PendingPayloadView) -> &'static str { + match payload { + PendingPayloadView::Tx(intent) => match intent.kind { + IntentKind::Send => "sending", + IntentKind::Shield => "shielding", + IntentKind::Unshield => "withdrawing", + IntentKind::ContractCall => "calling a contract", + }, + PendingPayloadView::Order(_) => "swapping", + PendingPayloadView::Approve { .. } => "approving a spender", + PendingPayloadView::Message(_) => "signing a message", + } +} + +/// The amber caution naming WHY a proposal is held (the breached fence), shown below the one danger +/// line on a Tx review. `None` for a within-cap hold (e.g. a guardrail hold) — there the Allowed-by +/// line shows the headroom instead, so the two never both appear. +fn breach_caution(reason: BreachedLimit) -> Option<&'static str> { match reason { - BreachedLimit::PerTxCap => "This exceeds the per-transaction limit.", - BreachedLimit::DailyCap => "This exceeds today's daily limit.", - BreachedLimit::OffAllowlist => "This recipient is not on the allow-list.", - BreachedLimit::None => "This is held for your approval.", + BreachedLimit::PerTxCap => Some("Over the per-transaction cap — held for your approval."), + BreachedLimit::DailyCap => Some("Over today's daily limit — held for your approval."), + BreachedLimit::OffAllowlist => { + Some("Recipient is not on the allow-list — held for your approval.") + } + BreachedLimit::None => None, } } @@ -1413,26 +1492,6 @@ fn skeleton_row(raise: gpui::Hsla) -> impl IntoElement { .bg(raise) } -/// The review card's heading block (H1 + muted subtitle). -fn activity_heading_block( - title: &'static str, - subtitle: &'static str, - fg: gpui::Hsla, - muted: gpui::Hsla, -) -> impl IntoElement { - v_flex() - .w_full() - .gap_1() - .child( - div() - .text_xl() - .font_weight(FontWeight::SEMIBOLD) - .text_color(fg) - .child(title), - ) - .child(div().text_sm().text_color(muted).child(subtitle)) -} - /// The shared column shell for the Activity surface — the same 760px dense-list column the /// Approvals queue uses, so the two sibling surfaces frame identically. fn activity_shell(inner: gpui::AnyElement) -> impl IntoElement { @@ -1629,6 +1688,43 @@ mod tests { assert_eq!(approve_target(None, &both), None); } + #[test] + fn routed_agent_review_still_cannot_blind_approve_a_settled_record() { + // #185 regression: the agent-approval now renders through the ONE shared Review, but the + // no-blind-approve guard is UNCHANGED — approve resolves ONLY a still-pending reviewed + // record. If the reviewed agent Tx settles under a background poll while its review is open, + // it leaves the pending set, so `approve_target` returns None and ⌘Enter re-opens a review + // rather than resolving blind. `record()` builds Agent-origin Tx records — exactly the + // payload now routed through the shared review. + let reviewed_id = B256::repeat_byte(0x0A); + let other_id = B256::repeat_byte(0x0B); + // The feed after a poll settled the reviewed row: 0x0A still EXISTS but as a settled row, + // while a different row 0x0B is still pending. + let feed = vec![ + record( + 0x0A, + ActivityLifecycle::Decided { approved: true }, + BreachedLimit::PerTxCap, + true, + ), + record( + 0x0B, + ActivityLifecycle::Proposed, + BreachedLimit::DailyCap, + false, + ), + ]; + let pending = activity_pending(&feed); + // Reviewing the now-settled 0x0A → resolve NOTHING, and never the still-pending 0x0B. + assert_eq!( + approve_target(Some(reviewed_id), &pending), + None, + "a reviewed agent record that settled must not be approvable, nor fall back to another row", + ); + // The still-pending 0x0B is resolvable only when IT is the reviewed record. + assert_eq!(approve_target(Some(other_id), &pending), Some(other_id)); + } + #[test] fn human_acted_tints_amber_for_every_human_action() { // Hands-free within-cap auto-allow → the agent's work → NOT human-acted (muted). diff --git a/crates/deckard-app/src/commit_view.rs b/crates/deckard-app/src/commit_view.rs index eeb1fe6..152b3dc 100644 --- a/crates/deckard-app/src/commit_view.rs +++ b/crates/deckard-app/src/commit_view.rs @@ -1,24 +1,24 @@ -//! commit_view — the generic "compose → review → done" renderer that drives every -//! [`CommitFlow`](crate::commit_flow) surface (Send now; Shield joins in Step 2). A single -//! [`CommitView`] descriptor (a `&'static` table of copy, button ids, the heading glyph, and a -//! few per-surface hooks) feeds [`Shell::render_commit`], which renders the shared compose → -//! review → done surface. The review step is the editorial transaction-as-hero clear-signing -//! statement (DESIGN §Clear-signing review): action label, oversized mono amount, recipient, -//! danger/caution lines, then quiet supporting facts — driven entirely by the descriptor. +//! commit_view — the generic "compose → review → done" renderer that drives every self-initiated +//! [`CommitFlow`](crate::commit_flow) surface (Send + Shield; Swap reuses the confirm + shell). A +//! single [`CommitView`] descriptor (a `&'static` table of copy, button ids, the heading glyph, and +//! a few per-surface hooks) feeds [`Shell::render_commit`], which renders the shared compose → +//! review → done surface. //! -//! The clear-signing contract is unchanged from `shield_view`/`send_view`: plain language, exact -//! mono figures, danger early, and confirm is a hold (never a tap) — the hand-built -//! [`Shell::hold_to_confirm`] sweep animates an amber fill over [`SHIELD_HOLD`] as the action -//! signs (amber = the human-confirm signal). +//! The review step is the ONE shared clear-signing Review (DESIGN §Clear-signing — E5, #185), +//! rendered for EVERY origin (here a self Send/Shield; an agent proposal and a dapp request route +//! through the same body from `activity_view`). Only the request-origin **rail** changes; the body +//! is identical: `origin_header` → the transaction-as-hero amount ([`tx_hero`]) → the full recipient +//! ([`tx_recipient`]) → the one danger line "This can't be undone." → amber cautions → the quiet +//! facts (From · Network · fee/net · **Allowed by** the rule + cap-after, via [`Shell::review_authority_row`]). //! -//! Step 1 migrates ONLY Send onto this renderer; Shield still uses its flat `shield_*` fields and -//! `shield_view.rs`. The descriptor already carries the slots Shield needs (optional fee/net rows, -//! a variable honesty-line list, an optional conditional compose-hint hook) so Step 2 is a pure -//! descriptor + handler swap with no renderer changes. +//! Confirm is the platform-aware `⌘↵` key-cap (DESIGN §The confirm pattern — a deliberate click or +//! chord, NOT a hold; the arm-delay gates it), rendered by [`Shell::hold_to_confirm`] and reused by +//! Swap's bespoke review. The clear-signing contract is unchanged: plain language, exact mono +//! figures, danger early (amber = the human-confirm signal). use gpui::prelude::FluentBuilder; use gpui::{ - div, px, ClipboardItem, Context, FontWeight, Hsla, InteractiveElement, IntoElement, + div, px, AnyElement, ClipboardItem, Context, FontWeight, Hsla, InteractiveElement, IntoElement, ParentElement, SharedString, StatefulInteractiveElement, Styled, }; use gpui_component::{ @@ -28,11 +28,13 @@ use gpui_component::{ v_flex, ActiveTheme, Disableable, Icon, IconName, }; +use deckard_contract::IntentKind; use deckard_core::U256; use crate::commit_flow::{CommitFlow, Proposal}; use crate::money::money; use crate::shell::Shell; +use crate::widgets::{key_cap, kv_row, origin_header, KeyCap, KvValue, Origin}; /// A single label/value money row in the review's quiet supporting-facts list: label left /// (muted), value right (mono). One signature shared by every commit surface. @@ -56,6 +58,74 @@ fn kv_money_row( ) } +/// The transaction-as-hero amount block shared by every Tx-shaped Review — a self Send/Shield +/// (`render_commit_review`) AND an agent's proposed Tx (`render_activity_review`). A tiny action +/// label, then the amount as the oversized mono hero (integer `fg`, decimals + ticker dimmed by +/// color, no size step). ONE builder so the self review and the agent review render the amount +/// identically — the "one review, header-rail-only difference" invariant (DESIGN §Clear-signing). +/// Never masked: at the moment of authorization you must SEE the figure you are approving. +pub(crate) fn tx_hero( + noun: &str, + value: U256, + unit: Option<&str>, + mono: SharedString, + fg: Hsla, + muted: Hsla, +) -> gpui::AnyElement { + v_flex() + .w_full() + .gap_1() + .child(crate::widgets::section_label(noun, muted)) + .child( + div() + .text_size(crate::tokens::TEXT_TX_HERO) + .font_weight(FontWeight::SEMIBOLD) + .child(money(value, 18, 6, unit, false, mono, fg, muted)), + ) + .into_any_element() +} + +/// The security-critical recipient block shared by every Tx-shaped Review: a tiny `To` label + +/// identicon + the FULL address (every character, `fg`, not dimmed, wraps for a long 0zk address) — +/// maximal distinguishability at the moment of authorization. ONE builder so the self review and +/// the agent review show the destination identically. +pub(crate) fn tx_recipient( + recipient: &str, + mono: SharedString, + fg: Hsla, + muted: Hsla, + id_fill: Hsla, +) -> gpui::AnyElement { + let r = recipient.trim(); + v_flex() + .w_full() + .gap_2() + .child(crate::widgets::section_label("To", muted)) + .child( + h_flex() + .w_full() + .items_start() + .gap_2() + .child(crate::widgets::identity_mark( + r, + px(16.0), + px(4.0), + id_fill, + fg, + )) + .child( + div() + .flex_1() + .min_w_0() + .font_family(mono) + .text_sm() + .text_color(fg) + .child(SharedString::from(r.to_string())), + ), + ) + .into_any_element() +} + /// A money figure derived from the proposal's gross value, rendered as one quiet supporting-fact /// row below the review hero (e.g. Shield's "Railgun fee" and "You'll receive (private)"). /// `compute` turns the gross intent value into the row's wei figure. @@ -106,8 +176,9 @@ pub struct CommitView { pub compose_hint_dynamic: Option &'static str>, // --- review --- - pub review_title: &'static str, - pub review_subtitle: &'static str, + // (No review title/subtitle: the shared Review leads with the request-origin rail, not a + // heading — DESIGN §Clear-signing. The action verb lives in the hero's `SENDING`/`SHIELDING` + // label, derived from `hold_label_busy`.) /// Quiet supporting-fact money rows, demoted between hairlines below the hero (Shield's fee + /// net). Empty for Send, which has nothing to demote. pub extra_rows: &'static [MoneyRow], @@ -254,12 +325,71 @@ impl Shell { ) } - /// Review: the clear-signing statement (DESIGN §Clear-signing review — transaction-as-hero). - /// NOT a bordered card: a tiny action label, then the AMOUNT as the oversized mono hero - /// (dimmed decimals), then `TO` + the recipient via `truncated_address`, then the danger / - /// caution lines, then the quiet supporting facts (any `extra_rows`) demoted between - /// hairlines, then the unchanged hold-to-confirm + Edit. Rendered from the proposal SNAPSHOT — - /// never the live input. + /// The shared Review's **Allowed by** authority line (DESIGN §Clear-signing): the rule that + /// permits this move + the daily budget left AFTER it, read from the daemon's live policy via + /// [`Policy::authority_for`](deckard_contract::Policy::authority_for) so the figure is the SAME + /// one `evaluate` enforces — the UI never recomputes cap math, so it can't drift. Returns `None` + /// (line omitted) when the policy is unavailable, no rule governs the action, or the move is + /// OVER cap: there is then no truthful headroom to cite, and the danger line carries that story + /// (never show an enforcement claim the engine doesn't back). Shared by every Tx-shaped origin. + pub(crate) fn review_authority_row( + &self, + kind: IntentKind, + value: U256, + cx: &mut Context, + ) -> Option { + let auth = self.agent_policy.as_ref()?.authority_for(kind, value)?; + if auth.over_cap { + return None; + } + let theme = cx.theme(); + let muted = theme.muted_foreground; + let primary = theme.foreground; + let mono = theme.mono_font_family.clone(); + let remaining = deckard_core::format_amount(auth.daily_remaining_after_wei, 18, 4); + let total = deckard_core::format_amount(auth.daily_cap_wei, 18, 4); + Some( + h_flex() + .w_full() + .items_baseline() + .justify_between() + .gap_4() + .py_1p5() + .text_size(crate::tokens::TEXT_BODY) + .child(div().flex_shrink_0().text_color(muted).child("Allowed by")) + .child( + h_flex() + .min_w_0() + .items_baseline() + .gap_1() + .child( + div() + .flex_shrink_0() + .text_color(primary) + .child(SharedString::from(auth.rule_label)), + ) + .child( + div() + .min_w_0() + .truncate() + .font_family(mono) + .text_color(muted) + .child(SharedString::from(format!( + "· {remaining} of {total} ETH daily left after this" + ))), + ), + ) + .into_any_element(), + ) + } + + /// Review: the ONE shared clear-signing statement (DESIGN §Clear-signing — transaction-as-hero), + /// rendered here for a self-initiated Send/Shield. NOT a bordered card: the request-origin rail + /// (`You are sending`, amber) — the ONLY thing that changes across origins — then the AMOUNT as + /// the oversized mono hero, then `TO` + the full recipient, one danger line `This can't be + /// undone.`, the descriptor's amber cautions, then the quiet facts (From · Network · any + /// fee/net · **Allowed by**) below a hairline, then the ⌘↵ key-cap confirm + Edit. Rendered from + /// the proposal SNAPSHOT — never the live input. fn render_commit_review( &self, view: &'static CommitView, @@ -271,102 +401,113 @@ impl Shell { let muted = theme.muted_foreground; let border = theme.border; let danger = theme.danger; + let success = theme.success; let is_dark = theme.is_dark(); let mono = theme.mono_font_family.clone(); let flow = (view.flow)(self); let gross = proposal.intent.value; + let kind = proposal.intent.kind.clone(); let recipient = proposal.recipient.clone(); // The action label noun ("Sending" / "Shielding") — derived from the descriptor's own // busy verb (the `&'static` `CommitView` is shared with the off-limits swap descriptor, so // a new noun field can't be added; the busy label is the descriptor's authoritative verb). let noun = view.hold_label_busy.trim_end_matches('…'); - - // The transaction-as-hero block: a tiny action label, then the amount as the oversized - // mono hero (integer `fg`, decimals + ticker dimmed by color via `money`, no size step). - let hero = v_flex() + let verb = noun.to_lowercase(); + + // Quiet-fact inputs (owned; no theme borrow) — the From identity and the network name. + let wallet_name = self.wallet_name(); + let from_value = format!( + "{} · {}", + wallet_name, + crate::widgets::short_addr(&self.wallet_address_string()) + ); + let network_name = deckard_core::for_chain(self.chain_id()) + .map(|c| c.network_name) + .unwrap_or("—"); + + // The request-origin rail: a self-initiated move is always You (amber "You are sending"). + // This rail is the ONLY thing that differs across origins — the body below is identical. + let origin_rail = origin_header( + Origin::You { + account: &wallet_name, + verb: &verb, + }, + None, + theme, + ); + // `theme` is not borrowed past this point — the `self.*(cx)` calls below re-borrow it. + + // The **Allowed by** authority line (rule + cap-after from the live policy). Built now, + // rendered inside the quiet facts; `None` when there's no truthful headroom to claim. + let authority = self.review_authority_row(kind, gross, cx); + + // The transaction-as-hero amount + the security-critical recipient — the shared builders, + // so an agent's proposed Tx renders these identically (the "one review" invariant). + let hero = tx_hero(noun, gross, Some("ETH"), mono.clone(), fg, muted); + let to = tx_recipient( + &recipient, + mono.clone(), + fg, + muted, + crate::theme::identity_square(is_dark), + ); + + // The ONE danger line for every value move (DESIGN §Clear-signing) — plain and declarative, + // no textbook blockchain explainer. The descriptor's amber cautions follow it. + let danger_line = crate::widgets::error_line(danger, "This can't be undone."); + + // Quiet supporting facts, demoted between two hairlines: From · Network · any fee/net rows + // (Shield's Railgun fee + private net) · the Allowed-by authority line. State each once. + let mut quiet = v_flex() .w_full() - .gap_1() - .child(crate::widgets::section_label(noun, muted)) - .child( - div() - .text_size(crate::tokens::TEXT_TX_HERO) - .font_weight(FontWeight::SEMIBOLD) - .child(money( - gross, - 18, - 6, - Some("ETH"), - false, - mono.clone(), - fg, - muted, - )), - ); - - // The recipient: a tiny `To` label + the identicon + the FULL address. This is the single - // most security-critical string at the moment of authorization, so unlike a tight row - // (which uses the 6+4 `short_addr`) the confirm shows EVERY character, in `fg` (not dimmed), - // and wraps for a long 0zk shield address — maximal distinguishability before signing. - let to = v_flex() - .w_full() - .gap_2() - .child(crate::widgets::section_label("To", muted)) - .child( - h_flex() - .w_full() - .items_start() - .gap_2() - .child(crate::widgets::identity_mark( - recipient.trim(), - px(16.0), - px(4.0), - crate::theme::identity_square(is_dark), - fg, - )) - .child( - div() - .flex_1() - .min_w_0() - .font_family(mono.clone()) - .text_sm() - .text_color(fg) - .child(SharedString::from(recipient.trim().to_string())), - ), - ); - - // The quiet supporting facts (Shield's Railgun fee + net), demoted between two hairlines. - // Empty for a public send, where there is nothing to demote. - let facts = (!view.extra_rows.is_empty()).then(|| { - let mut col = v_flex().w_full().child(crate::widgets::divider(border)); - for row in view.extra_rows { - col = col.child(kv_money_row( - row.label, - (row.compute)(gross), - mono.clone(), - fg, - muted, - )); - } - col.child(crate::widgets::divider(border)) - }); + .child(crate::widgets::divider(border)) + .child(kv_row( + "From", + KvValue::Sans(&from_value), + muted, + fg, + success, + mono.clone(), + )) + .child(kv_row( + "Network", + KvValue::Sans(network_name), + muted, + fg, + success, + mono.clone(), + )); + for row in view.extra_rows { + quiet = quiet.child(kv_money_row( + row.label, + (row.compute)(gross), + mono.clone(), + fg, + muted, + )); + } + let quiet = quiet + .children(authority) + .child(crate::widgets::divider(border)); self.commit_shell( view, v_flex() .w_full() .gap_4() - .child(self.commit_heading(view, view.review_title, view.review_subtitle, cx)) + .child(origin_rail) .child(hero) .child(to) + .child(danger_line) .child(self.commit_honesty(view, cx)) .children( flow.error .as_ref() .map(|e| crate::widgets::error_line(danger, e.clone())), ) - .children(facts) + .child(quiet) .child(self.hold_to_confirm(view, cx)) .child( Button::new(view.edit_button_id) @@ -500,17 +641,17 @@ impl Shell { let fg = theme.foreground; let border = theme.border; let fill = theme.muted; // bg.raise2 — the neutral primary fill - let base = theme.background; let mono = theme.mono_font_family.clone(); let amber = crate::theme::amber(theme.is_dark()); let muted = theme.muted_foreground; let flow = (view.flow)(self); let busy = flow.busy; - // The key-cap arms ~450ms after the review appears (the spam-guard). Dim it until then so a - // too-early click / ⌘↵ reads as "not ready yet" instead of a silently-dead button. The - // arm timer (`arm_commit`) wakes a re-render at the boundary so it visibly brightens. - let keycap_color = if self.commit_armed() { amber } else { muted }; + // The key-cap arms ~450ms after the review appears (the spam-guard): `key_cap`'s `armed` + // flag renders it amber once `commit_armed()`, dim before, so a too-early click / ⌘↵ reads + // as "not ready yet" rather than a silently-dead button. The arm timer (`arm_commit`) wakes + // a re-render at the boundary so it visibly brightens. + let armed = self.commit_armed(); let label = if busy { view.hold_label_busy @@ -518,28 +659,11 @@ impl Shell { view.hold_label_idle }; - // A keyboard-first key-cap confirm (DESIGN.md v2 §The confirm pattern). A deliberate - // click — or ⌘↵ — confirms; this is NOT a hold (the press-and-hold gesture was an - // anti-pattern). The ⌘↵ chord plus a short arm-delay (gated in the confirm handler) - // keep it spam-proof. The confirm handler is `on_hold_start` (kept as the trigger slot). - let keycap = move |g: &'static str| { - div() - .min_w(px(24.0)) - .h(px(24.0)) - .px_1() - .rounded(crate::tokens::RADIUS_ROW) - .bg(base) - .border_1() - .border_color(keycap_color) - .flex() - .items_center() - .justify_center() - .font_family(mono.clone()) - .text_xs() - .text_color(keycap_color) - .child(g) - }; - + // A keyboard-first key-cap confirm (DESIGN.md §The confirm pattern) via the shared, + // platform-aware `key_cap` widget (⌘↵ on macOS, Ctrl↵ on Linux, the chord as ONE cap). A + // deliberate click — or ⌘↵ — confirms; this is NOT a hold (the press-and-hold gesture was an + // anti-pattern). The ⌘↵ chord plus the arm-delay keep it spam-proof. The confirm handler is + // `on_hold_start` (kept as the trigger slot). div() .id(view.hold_id) .w_full() @@ -561,7 +685,7 @@ impl Shell { ) .child(div().flex_1()) .when(!busy, |b| { - b.child(h_flex().gap_1().child(keycap("⌘")).child(keycap("↵"))) + b.child(key_cap(KeyCap::CmdEnter, armed, border, muted, amber, mono)) }) .on_click(cx.listener(|this, _, _, cx| (view.on_hold_start)(this, cx))) } diff --git a/crates/deckard-app/src/send_view.rs b/crates/deckard-app/src/send_view.rs index 818bcc3..493814c 100644 --- a/crates/deckard-app/src/send_view.rs +++ b/crates/deckard-app/src/send_view.rs @@ -36,22 +36,15 @@ pub static SEND_VIEW: CommitView = CommitView { compose_hint_dynamic: None, // --- review --- - review_title: "Review transfer", - review_subtitle: "Confirm the amount and the destination address, then send with ⌘↵.", // No Railgun fee / private net line for a public send. extra_rows: &[], - honesty: &[ - HonestyLine { - text: "This transfer is public on Ethereum and can't be undone.", - emphasized: true, - danger: true, - }, - HonestyLine { - text: "Double-check the destination address: funds sent to the wrong address are lost.", - emphasized: false, - danger: true, - }, - ], + // The shared Review renders the ONE canonical danger line ("This can't be undone.") itself, so + // the descriptor carries only the surface-specific amber caution below it (DESIGN §Clear-signing). + honesty: &[HonestyLine { + text: "Double-check the destination address; funds sent to the wrong address are lost.", + emphasized: true, + danger: false, + }], hold_id: "send-hold", hold_label_idle: "Send", hold_label_busy: "Sending…", diff --git a/crates/deckard-app/src/shield_view.rs b/crates/deckard-app/src/shield_view.rs index 02f6459..fbb92a1 100644 --- a/crates/deckard-app/src/shield_view.rs +++ b/crates/deckard-app/src/shield_view.rs @@ -50,8 +50,6 @@ pub static SHIELD_VIEW: CommitView = CommitView { compose_hint_dynamic: Some(shield_compose_hint), // --- review --- - review_title: "Review deposit", - review_subtitle: "Confirm what leaves, where it goes, and the fee, then shield with ⌘↵.", // The Railgun fee + the net private receipt, computed from the proposal's gross value. extra_rows: &[ MoneyRow { diff --git a/crates/deckard-app/src/swap_view.rs b/crates/deckard-app/src/swap_view.rs index 0ed20b6..0cef52c 100644 --- a/crates/deckard-app/src/swap_view.rs +++ b/crates/deckard-app/src/swap_view.rs @@ -32,12 +32,14 @@ use gpui_component::{ v_flex, ActiveTheme, Disableable, Icon, IconName, }; +use deckard_contract::Rule; use deckard_core::{tokens_for, Address, U256}; use crate::commit_view::{CommitView, HonestyLine}; use crate::money::money; use crate::shell::{Shell, Surface}; use crate::theme; +use crate::widgets::{origin_header, Origin}; /// The Swap surface descriptor. Unlike Send/Shield it does NOT feed `render_commit` (Swap's /// compose + review are bespoke — see [`Shell::render_swap`]); it carries the copy, ids, glyph tone, @@ -66,8 +68,6 @@ pub static SWAP_VIEW: CommitView = CommitView { compose_hint_dynamic: None, // --- review (read by `commit_heading` on the review arm) --- - review_title: "Review swap", - review_subtitle: "Confirm what you sell, the minimum you receive, and where it goes, then swap with ⌘↵.", // The bespoke review card builds its own token-denominated rows; the generic ETH money rows // don't apply. extra_rows: &[], @@ -457,10 +457,33 @@ impl Shell { let fg = theme.foreground; let muted = theme.muted_foreground; let border = theme.border; + let danger = theme.danger; let mark_fill = theme::identity_square(theme.is_dark()); let mono = theme.mono_font_family.clone(); let chain_id = self.chain_id(); + // The request-origin rail — a self-initiated swap is You (amber "You are swapping"). Built + // now, while `theme` is borrowed; the `self.*(cx)` calls below re-borrow it. This rail is + // the ONLY thing that differs across origins — the review body is the shared surface. + let wallet_name = self.wallet_name(); + let origin_rail = origin_header( + Origin::You { + account: &wallet_name, + verb: "swapping", + }, + None, + theme, + ); + // The **Allowed by** line for a swap: the Swap rule permitted it. A swap ALWAYS asks (it + // never auto-allows), so there is no numeric cap-after the daily line could truthfully claim + // — the label alone is the honest authority (never a claim the engine doesn't back). `None` + // when the policy is unavailable or carries no swap rule. + let swap_rule_label = self + .agent_policy + .as_ref() + .and_then(|p| p.rules.iter().find(|r| matches!(r, Rule::Swap { .. }))) + .map(|r| r.label()); + // Pull the bound figures off the last quote (the quote that produced this proposal). The // proposal's `intent` carries the swap's value, but the token-denominated sell/buy figures // come from the quote snapshot still held on the shell — cleared only on a compose edit, @@ -562,7 +585,17 @@ impl Shell { .into_any_element(), ); if let Some(valid) = valid_row { - rows.push(kv_text_row("Valid until", valid, mono, muted, muted).into_any_element()); + rows.push( + kv_text_row("Valid until", valid, mono.clone(), muted, muted).into_any_element(), + ); + } + // The Allowed-by authority line — the Swap rule that permitted it (a swap always asks, so no + // numeric cap-after to claim). Omitted when no policy / no swap rule. + if let Some(label) = swap_rule_label { + rows.push( + kv_text_row("Allowed by", label.to_string(), mono.clone(), fg, muted) + .into_any_element(), + ); } let mut kvlist = v_flex() @@ -584,12 +617,9 @@ impl Shell { v_flex() .w_full() .gap_4() - .child(self.commit_heading( - &SWAP_VIEW, - SWAP_VIEW.review_title, - SWAP_VIEW.review_subtitle, - cx, - )) + // The shared request-origin rail — the ONLY cross-origin difference. The swap keeps + // its native two-token body below it (per E5: same rail, honest native content). + .child(origin_rail) // A faint reminder of the human-readable summary the proposal snapshot carries // (e.g. "0.05 WETH → at least 92.1 COW"), so the list matches what was reviewed. .child( @@ -598,6 +628,8 @@ impl Shell { .text_color(muted) .child(proposal.recipient.clone()), ) + // The ONE danger line for every value move (DESIGN §Clear-signing). + .child(crate::widgets::error_line(danger, "This can't be undone.")) .child(kvlist) .child(self.commit_honesty_swap(cx)) .children(self.swap.error.as_ref().map(|e| error_line(e, cx))) diff --git a/crates/deckard-app/src/widgets.rs b/crates/deckard-app/src/widgets.rs index 3aef1b6..67b20be 100644 --- a/crates/deckard-app/src/widgets.rs +++ b/crates/deckard-app/src/widgets.rs @@ -284,7 +284,7 @@ pub(crate) enum KeyCap { /// The primary-modifier label for `os` (pass `std::env::consts::OS`): `⌘` on macOS, `Ctrl` /// everywhere else. A pure fn so the platform mapping is unit-testable without a window. -#[allow(dead_code)] // reason: the platform half of `key_cap`; consumed by E5/E6 via `key_cap`. +/// The platform half of `key_cap`, now consumed by the shared Review (E5) via `key_cap`. fn primary_mod_label(os: &str) -> &'static str { if os == "macos" { "⌘" @@ -294,7 +294,7 @@ fn primary_mod_label(os: &str) -> &'static str { } /// The glyphs a [`KeyCap`] renders for `os`. Pure (no rendering) so it is unit-testable. -#[allow(dead_code)] // reason: the label half of `key_cap`; consumed by E5/E6 via `key_cap`. +/// The label half of `key_cap`, now consumed by the shared Review (E5) via `key_cap`. fn key_cap_label(cap: KeyCap, os: &str) -> String { match cap { KeyCap::CmdEnter => format!("{}↵", primary_mod_label(os)), @@ -308,9 +308,8 @@ fn key_cap_label(cap: KeyCap, os: &str) -> String { /// `std::env::consts::OS`), the `⌘↵` chord as ONE cap. `armed` renders the amber border + amber /// text (no fill) of the live confirm; at rest it is `border.strong` + `text.muted`. The one /// key-cap so no view hardcodes a `⌘`/`Ctrl` glyph. -// reason: the v4 confirm button (E5, #185) + activity / needs-you key hints (E6, #186) consume -// this; E1 lands the shared, platform-aware glyph so no later view re-rolls it. -#[allow(dead_code)] +// Consumed by the v4 confirm button (E5, #185 — the shared Review's ⌘↵) and, later, activity / +// needs-you key hints (E6, #186); the one platform-aware glyph so no view re-rolls a ⌘/Ctrl. pub(crate) fn key_cap( cap: KeyCap, armed: bool, @@ -454,8 +453,8 @@ pub(crate) enum KvValue<'a> { /// The ONE key/value row (DESIGN §Widget vocabulary): label-left `text.muted`, value-right (mono /// `text.primary`, or sans, or `success`), the row clamped so a long value truncates rather than /// overflowing. Shared by clear-signing quiet-facts, the policy ledger, and the metadata rail. -// reason: consumed by the v4 metadata rail (E3, #183) + the Review quiet facts (E5, #185). -#[allow(dead_code)] +// Consumed by the Review quiet facts (E5, #185 — From / Network / Allowed by) and, later, the v4 +// metadata rail (E3, #183). pub(crate) fn kv_row( label: &str, value: KvValue, @@ -526,8 +525,7 @@ pub(crate) fn page_header( .when_some(subtitle, |d, s| { d.child( div() - .min_w_0() - .truncate() + .flex_shrink_0() .text_sm() .text_color(muted) .when_some(subtitle_mono, |d, mono| d.font_family(mono)) @@ -540,7 +538,7 @@ pub(crate) fn page_header( /// Who a shared-Review request came FROM (DESIGN §The request-origin model): the human, an agent, /// or a dapp. The verb is the human's action (`You are sending`); agents `propose`, dapps `request`. -#[allow(dead_code)] // reason: constructed per request by the shared Review (E5, #185) + rail (E3). +/// Constructed per request by the shared Review (E5, #185): You / Agent / Dapp. pub(crate) enum Origin<'a> { /// The human principal — a round identity mark (`account` seeds it) + amber `You are {verb}`. You { account: &'a str, verb: &'a str }, @@ -561,7 +559,7 @@ pub(crate) enum Trust { } /// The small state-color trust badge for the [`origin_header`] rail. -#[allow(dead_code)] // reason: the badge half of `origin_header`; consumed via `origin_header`. +/// The badge half of `origin_header`, consumed via `origin_header` (E5, #185). fn trust_badge(trust: Trust, theme: &Theme) -> AnyElement { let is_dark = theme.is_dark(); // The tint is hue-keyed (DESIGN §Opacity): the amber caution reads weaker at equal alpha, so it @@ -605,8 +603,8 @@ fn trust_badge(trust: Trust, theme: &Theme) -> AnyElement { /// fooled by. A dapp/external origin is a neutral identity + a state-color badge, NEVER a third /// signal color; the agent mark is the bordered cyan squircle ([`agent_mark`], handle-aware). /// `You` is amber, an agent is cyan, a dapp is neutral. -// reason: consumed by the ONE shared Review (E5, #185) + the rail's compact clear-signing (E3). -#[allow(dead_code)] +// Consumed by the ONE shared Review (E5, #185) for every origin (self Send/Shield/Swap, an agent +// proposal, a dapp request) and, later, the rail's compact clear-signing (E3). pub(crate) fn origin_header(origin: Origin, trust: Option, theme: &Theme) -> AnyElement { let is_dark = theme.is_dark(); let amber = crate::theme::amber(is_dark); From 66ff03ac365e3c479ba950dc378871fab9cf35cc Mon Sep 17 00:00:00 2001 From: hellno Date: Mon, 6 Jul 2026 11:34:09 +0200 Subject: [PATCH 3/3] fix(errors): action-neutral deny copy so a Send never reads as a "deposit" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `humanize_deny` is shared by Send, Shield, and swap-fallthrough, but was worded for the shield flow — every line said "the deposit…". So a failed Send showed "the deposit couldn't be broadcast", which reads wrong (a send isn't a deposit; the copy for image-2's network error). Reword the multi-flow tags (broadcast/session/process: broadcast_failed, chain_mismatch, undecodable, not_approved, unknown_request, broadcast_timeout, already_executed) to "the transaction…". The genuinely shield-only tags (shield_to_mismatch → Railgun target; erc20_unsupported_v1 → native-ETH shields) keep shield wording — a send/swap can't reach them. Swaps already route swap-specific tags through humanize_swap_deny; this makes its fallthrough correct too. Adds a guard test (shared_deny_tags_are_action_neutral_not_deposit_worded) so a shared tag can't regress to deposit wording. Pre-existing nit surfaced while driving E5 (#185); no logic change. --- crates/deckard-app/src/errors.rs | 76 +++++++++++++++++++++++--------- 1 file changed, 55 insertions(+), 21 deletions(-) diff --git a/crates/deckard-app/src/errors.rs b/crates/deckard-app/src/errors.rs index bacd2f1..b125de6 100644 --- a/crates/deckard-app/src/errors.rs +++ b/crates/deckard-app/src/errors.rs @@ -34,44 +34,51 @@ pub fn humanize_read_error(raw: &str) -> String { /// Map a daemon deny/`reason` tag to a calm, user-facing line (the wire tags are terse + /// machine-readable; the UI shouldn't show `chain_mismatch` raw). +/// +/// The **shared** tags (broadcast/session/process — reachable by Send, Shield, AND swap-fallthrough) +/// are worded action-neutrally ("the transaction…"), so a plain Send never reads as a "deposit". +/// Only the genuinely shield-only tags (`shield_to_mismatch`, `erc20_unsupported_v1`) keep shield +/// wording, since a send/swap can't reach them. Swaps still route swap-specific tags through +/// [`humanize_swap_deny`] first. pub fn humanize_deny(reason: &str) -> String { // The broadcast error carries a variable RPC suffix, so match it by prefix. if reason.starts_with("broadcast_failed") { - return "the deposit couldn't be broadcast — check your network, then review again".into(); + return "the transaction couldn't be broadcast — check your network, then review again" + .into(); } match reason { "locked" => "unlock your wallet first".into(), "revoked" => "the signer is paused (STOP is active)".into(), "chain_mismatch" => { - "the signer is on a different chain than this deposit — reconcile the chain first" + "the signer is on a different chain than this transaction — reconcile the chain first" .into() } "over_cap" | "cap_exceeded" => "it exceeds the agent's spending cap".into(), "off_allowlist" => "the recipient isn't on the allowlist".into(), - "undecodable" => "the deposit calldata didn't validate".into(), + "undecodable" => "the transaction calldata didn't validate".into(), + // Shield-only: a send/swap never targets the Railgun contract, so keep the deposit wording. "shield_to_mismatch" => { "the deposit doesn't target the Railgun contract for this chain".into() } - "not_approved" => "this deposit hasn't been approved yet — review it again".into(), - "unknown_request" => { - "the signer session was reset — review the deposit again".into() - } + "not_approved" => "this transaction hasn't been approved yet — review it again".into(), + "unknown_request" => "the signer session was reset — review the transaction again".into(), + // Shield-only: v1 only shields native ETH. "erc20_unsupported_v1" => "only native-ETH shields are supported in v1".into(), "unsupported_v1" => "that action isn't supported in v1".into(), "broadcast_timeout" => { - "the network didn't confirm in time — your deposit may already be in flight, so check your activity before retrying" + "the network didn't confirm in time — your transaction may already be in flight, so check your activity before retrying" .into() } - "already_executed" => "this deposit was already submitted".into(), + "already_executed" => "this transaction was already submitted".into(), other => other.to_string(), } } -/// Map a daemon deny `reason` to calm, **swap-specific** copy (#25). [`humanize_deny`] is -/// deposit/shield-worded ("the deposit…"), which reads wrong on a swap, so the swap path routes -/// its denies here. The swap-only policy/admission tags get distinct copy; anything else falls -/// through to [`humanize_deny`] so the shared session/process tags (`locked`, `chain_mismatch`, -/// `broadcast_*`, …) keep their single source of truth. +/// Map a daemon deny `reason` to calm, **swap-specific** copy (#25). The swap-only policy/admission +/// tags (order/approve) read wrong under [`humanize_deny`], so the swap path routes its denies here +/// for distinct copy; anything else falls through to [`humanize_deny`] so the shared session/process +/// tags (`locked`, `chain_mismatch`, `broadcast_*`, …) keep their single source of truth (now +/// action-neutral, so the fallthrough reads correctly for a swap too). pub fn humanize_swap_deny(reason: &str) -> String { match reason { // --- order admission (deckard-contract::evaluate_order) --- @@ -145,7 +152,7 @@ mod tests { ); assert_eq!( humanize_deny("chain_mismatch"), - "the signer is on a different chain than this deposit — reconcile the chain first" + "the signer is on a different chain than this transaction — reconcile the chain first" ); // The two-tag arm collapses to one line. assert_eq!( @@ -162,19 +169,20 @@ mod tests { ); assert_eq!( humanize_deny("undecodable"), - "the deposit calldata didn't validate" + "the transaction calldata didn't validate" ); + // Shield-only tag: legitimately keeps deposit wording. assert_eq!( humanize_deny("shield_to_mismatch"), "the deposit doesn't target the Railgun contract for this chain" ); assert_eq!( humanize_deny("not_approved"), - "this deposit hasn't been approved yet — review it again" + "this transaction hasn't been approved yet — review it again" ); assert_eq!( humanize_deny("unknown_request"), - "the signer session was reset — review the deposit again" + "the signer session was reset — review the transaction again" ); assert_eq!( humanize_deny("erc20_unsupported_v1"), @@ -186,19 +194,45 @@ mod tests { ); assert_eq!( humanize_deny("broadcast_timeout"), - "the network didn't confirm in time — your deposit may already be in flight, so check your activity before retrying" + "the network didn't confirm in time — your transaction may already be in flight, so check your activity before retrying" ); assert_eq!( humanize_deny("already_executed"), - "this deposit was already submitted" + "this transaction was already submitted" ); } + #[test] + fn shared_deny_tags_are_action_neutral_not_deposit_worded() { + // A plain Send reuses `humanize_deny`, so the multi-flow tags (broadcast/session/process) + // must NOT read "deposit" — a send isn't a deposit. Only the genuinely shield-only tags + // keep shield wording (a send/swap can't reach them). + for tag in [ + "broadcast_failed", + "broadcast_failed: connection refused", + "chain_mismatch", + "undecodable", + "not_approved", + "unknown_request", + "broadcast_timeout", + "already_executed", + ] { + let line = humanize_deny(tag).to_lowercase(); + assert!( + !line.contains("deposit"), + "{tag} must be action-neutral, not deposit-worded: {line}" + ); + } + // The shield-only tags legitimately keep shield/deposit wording. + assert!(humanize_deny("shield_to_mismatch").contains("deposit")); + assert!(humanize_deny("erc20_unsupported_v1").contains("shields")); + } + #[test] fn humanize_deny_matches_broadcast_failed_by_prefix() { // The broadcast error carries a variable RPC suffix, so any `broadcast_failed*` maps to // the same calm line. - let line = "the deposit couldn't be broadcast — check your network, then review again"; + let line = "the transaction couldn't be broadcast — check your network, then review again"; assert_eq!(humanize_deny("broadcast_failed"), line); assert_eq!( humanize_deny("broadcast_failed: connection refused (http://localhost:8545)"),