diff --git a/Cargo.lock b/Cargo.lock index db8f240..4b2f4df 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -292,7 +292,7 @@ dependencies = [ [[package]] name = "comebackhere-multisig" -version = "0.2.0" +version = "0.3.0" dependencies = [ "soroban-sdk", ] diff --git a/contracts/compliance/src/lib.rs b/contracts/compliance/src/lib.rs index 5e911b8..c5f9fca 100644 --- a/contracts/compliance/src/lib.rs +++ b/contracts/compliance/src/lib.rs @@ -105,7 +105,7 @@ pub struct AddressStatus { /// Primary error type for the compliance contract. /// /// Variants must only be appended at the end (highest numeric value) to preserve -/// on-chain backwards compatibility. Range: 1..=5 (see `ARCHITECTURE.md`). +/// on-chain backwards compatibility. Range: 1..=6 (see `ARCHITECTURE.md`). #[contracterror] #[derive(Copy, Clone, Debug, PartialEq)] #[repr(u32)] @@ -115,6 +115,9 @@ pub enum ContractError { AlreadyInitialized = 3, BatchTooLarge = 4, AddressIndexFull = 5, + /// A bulk allow/block call was made before [`BULK_OP_COOLDOWN_SECS`] elapsed since the + /// caller's previous bulk call (see #454). + BulkOperationCooldown = 6, } /// Upper bound on the number of distinct addresses tracked in `DataKey::AddressIndex`. @@ -128,6 +131,15 @@ const MAX_TRACKED_ADDRESSES: u32 = 50_000; /// the batch caps used elsewhere in the workspace (see #8/#21/#29). pub const MAX_BATCH_SIZE: u32 = 50; +/// Minimum time (seconds) a caller must wait between successive calls to the *same* bulk +/// entrypoint (`bulk_allow_addresses` or `bulk_block_addresses`). `MAX_BATCH_SIZE` bounds how +/// many addresses a single call can affect, but without a time dimension a compromised admin +/// key could still call one bulk entrypoint repeatedly in quick succession and affect an +/// unbounded number of addresses in aggregate (see #454). Tracked separately per entrypoint +/// (rather than shared) so that legitimate admin flows — e.g. allowing a batch and then +/// immediately blocking a different batch — are not penalized for using both in succession. +pub const BULK_OP_COOLDOWN_SECS: u64 = 60; + #[contract] pub struct ComplianceContract; @@ -161,6 +173,7 @@ impl ComplianceContract { ) -> Result<(), ContractError> { Self::require_admin(&env, &admin)?; Self::require_not_paused(&env)?; + Self::check_bulk_op_cooldown(&env, DataKey::LastBulkAllow(admin.clone()))?; if addresses.len() > MAX_BATCH_SIZE { return Err(ContractError::BatchTooLarge); } @@ -351,6 +364,7 @@ impl ComplianceContract { addresses: Vec
, ) -> Result<(), ContractError> { Self::require_admin(&env, &admin)?; + Self::check_bulk_op_cooldown(&env, DataKey::LastBulkBlock(admin.clone()))?; if addresses.len() > MAX_BATCH_SIZE { return Err(ContractError::BatchTooLarge); } @@ -858,6 +872,20 @@ impl ComplianceContract { Err(ContractError::Unauthorized) } + /// Enforces [`BULK_OP_COOLDOWN_SECS`] between successive calls keyed by `key` + /// (a `LastBulkAllow`/`LastBulkBlock` variant), and records `now` as the new + /// last-call timestamp on success. + fn check_bulk_op_cooldown(env: &Env, key: DataKey) -> Result<(), ContractError> { + let now = env.ledger().timestamp(); + if let Some(last) = env.storage().instance().get::<_, u64>(&key) { + if now < last.saturating_add(BULK_OP_COOLDOWN_SECS) { + return Err(ContractError::BulkOperationCooldown); + } + } + env.storage().instance().set(&key, &now); + Ok(()) + } + fn require_not_paused(env: &Env) -> Result<(), ContractError> { let paused: bool = env .storage() diff --git a/contracts/treasury/README.md b/contracts/treasury/README.md index d189c73..3af8073 100644 --- a/contracts/treasury/README.md +++ b/contracts/treasury/README.md @@ -24,9 +24,13 @@ The Treasury contract manages funds and settlements using a multi-signature appr | `unpause` | `admin` | `admin: Address` | `()` | `Unauthorized` | | `raise_dispute` | `claimant` | `claimant: Address, settlement_id: u64, counterparty: Address, amount: i128` | `u64` | `ContractPaused`, `Unauthorized`, `InvalidAmount` | | `resolve_dispute` | `admin` | `admin: Address, dispute_id: u64, in_favor_of_claimant: bool` | `()` | `Unauthorized`, `ContractPaused`, `DisputeNotFound`, `DisputeAlreadyResolved` | +| `resolve_dispute_split` | `admin` | `admin: Address, dispute_id: u64, claimant_bps: u32, token_contract: Address` | `()` | `Unauthorized`, `ContractPaused`, `DisputeNotFound`, `DisputeAlreadyResolved`, `InvalidSplitRatio` | | `vote_dispute_resolution` | `signer` | `signer: Address, dispute_id: u64, in_favor_of_claimant: bool` | `()` | `ContractPaused`, `UnauthorizedSigner`, `DisputeNotFound`, `DisputeAlreadyResolved`, `ResolutionDirectionMismatch` | | `deposit` | `from` | `from: Address, token_contract: Address, amount: i128` | `()` | `ContractPaused`, `Unauthorized`, `InvalidAmount` | -| `withdraw` | `to` | `to: Address, token_contract: Address, amount: i128` | `()` | `ContractPaused`, `Unauthorized`, `InvalidAmount`, `InsufficientBalance` | +| `withdraw` | `to` | `to: Address, token_contract: Address, amount: i128` | `()` | `ContractPaused`, `Unauthorized`, `InvalidAmount`, `InsufficientBalance`, `DestinationNotAllowed`, `WithdrawalLimitExceeded` | +| `withdraw_all` | `admin` | `admin: Address, token_contract: Address, recipient: Address` | `()` | `Unauthorized`, `NotPaused`, `WithdrawalLimitExceeded` | +| `set_withdrawal_limit` | `admin` | `admin: Address, limit: i128, window_secs: u64` | `()` | `Unauthorized` | +| `get_withdrawal_limit` | None | None | `(i128, u64)` | None | | `add_allowed_token` | `admin` | `admin: Address, token: Address` | `()` | `Unauthorized` | | `remove_allowed_token` | `admin` | `admin: Address, token: Address` | `()` | `Unauthorized` | | `get_balance` | None | `address: Address, token_contract: Address` | `i128` | None | diff --git a/contracts/treasury/src/deposits.rs b/contracts/treasury/src/deposits.rs index a1bd3ee..9d7f458 100644 --- a/contracts/treasury/src/deposits.rs +++ b/contracts/treasury/src/deposits.rs @@ -114,6 +114,7 @@ impl TreasuryContract { let token_client = token::Client::new(&env, &token_contract); let balance = token_client.balance(&treasury); if balance > 0 { + enforce_withdrawal_limit(&env, &recipient, balance); token_client.transfer(&treasury, &recipient, &balance); } env.events() diff --git a/contracts/treasury/src/disputes.rs b/contracts/treasury/src/disputes.rs index e5d99aa..29df38d 100644 --- a/contracts/treasury/src/disputes.rs +++ b/contracts/treasury/src/disputes.rs @@ -4,7 +4,10 @@ use crate::{ TreasuryContractClient, TreasuryError, }; use multisig::{meets_threshold, record_approval, require_authorized_signer}; -use soroban_sdk::{contractimpl, Address, Env, Symbol, Vec}; +use soroban_sdk::{contractimpl, token, Address, Env, Symbol, Vec}; + +/// Basis-points denominator for `resolve_dispute_split`'s ratio (10_000 = 100.00%). +pub const BPS_DENOMINATOR: u32 = 10_000; #[contractimpl] impl TreasuryContract { @@ -57,6 +60,7 @@ impl TreasuryContract { resolution_weight: 0, resolution_for_claimant: false, dispute_expires_at: expires_at, + claimant_share_bps: 0, }; env.storage() .persistent() @@ -152,40 +156,57 @@ impl TreasuryContract { .set(&DataKey::Dispute(dispute_id), &dispute); env.events() .publish((Symbol::new(&env, "dispute_resolved"), dispute_id), dispute); - if let Some(mut settlement) = env + release_settlement_hold_if_no_open_disputes(&env, settlement_id); + } + + /// Resolves an open dispute by splitting `dispute.amount` between claimant and + /// counterparty according to `claimant_bps` (out of [`BPS_DENOMINATOR`]) instead of the + /// binary claimant/counterparty outcome `resolve_dispute` supports (admin-only). + /// + /// Design note: like `resolve_dispute`, this only ever pays out `dispute.amount` (the + /// amount raised with the dispute, not necessarily equal to the associated settlement's + /// own `amount`) directly from the treasury's token balance to the two parties; it does + /// not itself execute the settlement. Releasing the settlement hold below only means the + /// settlement becomes eligible for its own `execute_settlement`/`partially_execute_settlement` + /// payout again — callers must ensure the disputed amount and the settlement's payout are + /// not double-counted when both eventually execute, exactly as with a binary resolution. + /// Panics: `DisputeNotFound`, `DisputeAlreadyResolved`, `ContractPaused`, `InvalidSplitRatio`. + /// Emits: `dispute_resolved_split`. + pub fn resolve_dispute_split( + env: Env, + admin: Address, + dispute_id: u64, + claimant_bps: u32, + token_contract: Address, + ) { + require_admin(&env, &admin); + require_not_paused(&env); + if claimant_bps > BPS_DENOMINATOR { + soroban_sdk::panic_with_error!(env, TreasuryError::InvalidSplitRatio); + } + let mut dispute: Dispute = env .storage() .persistent() - .get::(&DataKey::Settlement(settlement_id)) - { - if settlement.status == SettlementStatus::OnHold { - let dispute_count: u64 = env - .storage() - .instance() - .get(&DataKey::DisputeCount) - .unwrap_or(0); - let mut has_open = false; - let mut i = 1u64; - while i <= dispute_count { - if let Some(d) = env - .storage() - .persistent() - .get::(&DataKey::Dispute(i)) - { - if d.settlement_id == settlement_id && d.status == DisputeStatus::Raised { - has_open = true; - break; - } - } - i += 1; - } - if !has_open { - settlement.status = SettlementStatus::Pending; - settlement.hold_reason = SettlementHoldReason::None; - env.storage() - .persistent() - .set(&DataKey::Settlement(settlement_id), &settlement); - } - } + .get(&DataKey::Dispute(dispute_id)) + .unwrap_or_else(|| soroban_sdk::panic_with_error!(env, TreasuryError::DisputeNotFound)); + if dispute.status != DisputeStatus::Raised { + soroban_sdk::panic_with_error!(env, TreasuryError::DisputeAlreadyResolved); + } + let claimant_amount = dispute + .amount + .checked_mul(claimant_bps as i128) + .unwrap_or_else(|| { + soroban_sdk::panic_with_error!(env, TreasuryError::ArithmeticOverflow) + }) + / BPS_DENOMINATOR as i128; + let counterparty_amount = dispute.amount - claimant_amount; + let treasury = env.current_contract_address(); + let token_client = token::Client::new(&env, &token_contract); + if claimant_amount > 0 { + token_client.transfer(&treasury, &dispute.claimant, &claimant_amount); + } + if counterparty_amount > 0 { + token_client.transfer(&treasury, &dispute.counterparty, &counterparty_amount); } Ok(()) } @@ -243,3 +264,43 @@ impl TreasuryContract { Ok(()) } } + +/// Shared by `resolve_dispute` and `resolve_dispute_split`: releases `settlement_id` from +/// `OnHold` back to `Pending` once no `Raised` dispute references it anymore. +fn release_settlement_hold_if_no_open_disputes(env: &Env, settlement_id: u64) { + if let Some(mut settlement) = env + .storage() + .persistent() + .get::(&DataKey::Settlement(settlement_id)) + { + if settlement.status == SettlementStatus::OnHold { + let dispute_count: u64 = env + .storage() + .instance() + .get(&DataKey::DisputeCount) + .unwrap_or(0); + let mut has_open = false; + let mut i = 1u64; + while i <= dispute_count { + if let Some(d) = env + .storage() + .persistent() + .get::(&DataKey::Dispute(i)) + { + if d.settlement_id == settlement_id && d.status == DisputeStatus::Raised { + has_open = true; + break; + } + } + i += 1; + } + if !has_open { + settlement.status = SettlementStatus::Pending; + settlement.hold_reason = SettlementHoldReason::None; + env.storage() + .persistent() + .set(&DataKey::Settlement(settlement_id), &settlement); + } + } + } +} diff --git a/contracts/treasury/src/lib.rs b/contracts/treasury/src/lib.rs index 1d64c07..d8a7a4b 100644 --- a/contracts/treasury/src/lib.rs +++ b/contracts/treasury/src/lib.rs @@ -110,6 +110,43 @@ impl TreasuryContract { env.events() .publish((Symbol::new(&env, "treasury_unpaused"),), admin); } + + /// Configures the maximum amount withdrawable per rolling time window (admin-only). + /// Applies to both `withdraw` (tracked per recipient `to`) and `withdraw_all` (tracked + /// per `recipient`) — see `deposits.rs`. Passing `limit <= 0` disables the cap + /// (the default at initialization is uncapped), trading off protection against a + /// compromised-but-authorized withdrawer for the ability to move arbitrarily large + /// legitimate withdrawals in a single call; admins needing large one-off withdrawals + /// should raise the limit first rather than relying on an uncapped default long-term. + /// Emits: `withdrawal_limit_set`. + pub fn set_withdrawal_limit(env: Env, admin: Address, limit: i128, window_secs: u64) { + require_admin(&env, &admin); + env.storage() + .instance() + .set(&DataKey::WithdrawalLimitPerWindow, &limit); + env.storage() + .instance() + .set(&DataKey::WithdrawalWindowSecs, &window_secs); + env.events().publish( + (Symbol::new(&env, "withdrawal_limit_set"),), + (limit, window_secs), + ); + } + + /// Returns the currently configured `(limit, window_secs)`. `limit <= 0` means uncapped. + pub fn get_withdrawal_limit(env: Env) -> (i128, u64) { + let limit: i128 = env + .storage() + .instance() + .get(&DataKey::WithdrawalLimitPerWindow) + .unwrap_or(0); + let window_secs: u64 = env + .storage() + .instance() + .get(&DataKey::WithdrawalWindowSecs) + .unwrap_or(0); + (limit, window_secs) + } } /// Maximum number of tokens allowed in the allowlist to prevent unbounded storage growth. diff --git a/contracts/treasury/tests/multisig_version_lock_test.rs b/contracts/treasury/tests/multisig_version_lock_test.rs index f70a86d..6a12367 100644 --- a/contracts/treasury/tests/multisig_version_lock_test.rs +++ b/contracts/treasury/tests/multisig_version_lock_test.rs @@ -98,6 +98,8 @@ fn treasury_error_shape_is_unchanged() { assert_eq!(TreasuryError::InsufficientBalance as u32, 31); assert_eq!(TreasuryError::NotPaused as u32, 32); assert_eq!(TreasuryError::RotationProposalCooldown as u32, 33); + assert_eq!(TreasuryError::WithdrawalLimitExceeded as u32, 34); + assert_eq!(TreasuryError::InvalidSplitRatio as u32, 35); // No wildcard arm: adding, removing, or renaming a variant fails this compile. fn assert_exhaustive(err: TreasuryError) { @@ -134,7 +136,9 @@ fn treasury_error_shape_is_unchanged() { | TreasuryError::DestinationNotAllowed | TreasuryError::InsufficientBalance | TreasuryError::NotPaused - | TreasuryError::RotationProposalCooldown => {} + | TreasuryError::RotationProposalCooldown + | TreasuryError::WithdrawalLimitExceeded + | TreasuryError::InvalidSplitRatio => {} } } assert_exhaustive(TreasuryError::AlreadyOnHold); @@ -168,6 +172,7 @@ fn assert_dispute_status_exhaustive(status: DisputeStatus) { DisputeStatus::ResolvedClaimant => {} DisputeStatus::ResolvedCounterparty => {} DisputeStatus::Expired => {} + DisputeStatus::ResolvedSplit => {} } } @@ -212,6 +217,7 @@ fn dispute_enum_variants_are_unchanged() { DisputeStatus::ResolvedClaimant, DisputeStatus::ResolvedCounterparty, DisputeStatus::Expired, + DisputeStatus::ResolvedSplit, ] { assert_dispute_status_exhaustive(status); } @@ -286,6 +292,7 @@ fn dispute_struct_shape_is_unchanged() { resolution_weight, resolution_for_claimant, dispute_expires_at, + claimant_share_bps, } = dispute; assert_eq!(id, did); @@ -298,6 +305,7 @@ fn dispute_struct_shape_is_unchanged() { assert_eq!(resolution_weight, 0); assert!(!resolution_for_claimant); assert_eq!(dispute_expires_at, 1_000); + assert_eq!(claimant_share_bps, 0); } /// Builds a real `SignerRotationProposal` through the deployed contract and diff --git a/crates/multisig/src/lib.rs b/crates/multisig/src/lib.rs index 3a74d61..d26aeaf 100644 --- a/crates/multisig/src/lib.rs +++ b/crates/multisig/src/lib.rs @@ -78,6 +78,9 @@ pub enum DisputeStatus { ResolvedClaimant, ResolvedCounterparty, Expired, + /// The disputed amount was split between claimant and counterparty; see + /// `Dispute::claimant_share_bps` for the ratio and `resolve_dispute_split` (#456). + ResolvedSplit, } #[contracttype] @@ -106,6 +109,9 @@ pub struct Dispute { pub resolution_weight: u32, pub resolution_for_claimant: bool, pub dispute_expires_at: u64, + /// Claimant's share of `amount` in basis points (0..=10_000), set when `status` is + /// `ResolvedSplit`; meaningless (always 0) for every other status. See #456. + pub claimant_share_bps: u32, } #[contracttype] @@ -161,6 +167,14 @@ pub enum DataKey { WithdrawalAllowlist, LastRotationProposal(Address), PartialApprovedTotal(u64), + /// Admin-configured max amount withdrawable per rolling window; `0` means uncapped (#455). + WithdrawalLimitPerWindow, + /// Window length (seconds) paired with `WithdrawalLimitPerWindow`. + WithdrawalWindowSecs, + /// Start timestamp of the current withdrawal window for a given tracked address. + WithdrawalWindowStart(Address), + /// Amount withdrawn so far within the current window for a given tracked address. + WithdrawnInWindow(Address), } /// Returns the approval weight assigned to `signer`, or `0` if not registered. diff --git a/docs/compliance-sanctions-design.md b/docs/compliance-sanctions-design.md new file mode 100644 index 0000000..0f74d32 --- /dev/null +++ b/docs/compliance-sanctions-design.md @@ -0,0 +1,114 @@ +# Compliance: External Sanctions-List Integration — Design + +> **Status:** Design proposal · No behavioral contract changes accompany this PR. +> **Issue:** #453 +> **Branch:** `docs/compliance-sanctions-oracle-design` + +## 1. Problem + +`compliance`'s allow/block state is entirely admin-managed: every address-level +decision requires an explicit `allow_address` / `block_address` (or their +batch/timed variants) call. There is no mechanism to react to an *external* +sanctions-list update (e.g. a new OFAC addition) without an admin manually +noticing it and manually blocking each newly-sanctioned address — which does +not scale and introduces unbounded delay between a real-world sanctions event +and this protocol reflecting it. + +This document designs an oracle/attestation pattern for feeding external +sanctions-list updates into the existing `block_address` mechanism. It +deliberately does **not** implement real off-chain data-source integration — +that depends on data-source and legal decisions out of scope for a contract +design doc (which feed, which jurisdiction's list, licensing, legal liability +for false positives/negatives). + +## 2. Trust model options considered + +| Option | Description | Trade-off | +|---|---|---| +| **A. Reuse existing `Operator` role** | The already-defined but unused `DataKey::Operator` (see `address_status`'s `require_admin_or_operator`) is granted a new capability: submitting sanctions updates. | No new storage/role plumbing. But `Operator` is a single address today — a single sanctions feed key becomes a single point of compromise, same class of risk as the admin key it's meant to reduce reliance on. | +| **B. Dedicated multisig "attestor set"** | A new weighted-signer set (mirroring `treasury`'s `multisig` crate: `signer_weight`, `record_approval`, `meets_threshold`) requires N-of-M attestors to agree before a sanctions batch applies. | Matches the trust bar of an irreversible, high-consequence action (blocking funds). Adds a second signer registry to a contract that otherwise has none — real complexity cost. | +| **C. External oracle contract** | A separate on-chain contract (or existing bridge/oracle infra) is trusted as a single cross-contract caller, verified via `require_auth()` on the oracle's own contract address. | Cleanest separation of concerns; sanctions-feed logic lives outside `compliance`. Requires that oracle contract to exist and be independently trustworthy/audited — nothing in this repo provides one today. | + +## 3. Recommendation + +**Start with Option A (reuse `Operator`), with an explicit upgrade path to +Option B.** Rationale: + +- `Operator` already exists in storage (`DataKey::Operator`) and already has a + defined trust tier below `Admin` (see `address_status`'s + `require_admin_or_operator`) — extending its authority to sanctions + submission is additive, not a new primitive. +- The realistic near-term operator of a sanctions feed is a single automated + service (e.g. an indexer polling OFAC/SDN and relaying diffs), not a + committee — a single authorized submitter matches that shape today. +- Option B's multisig attestor set is the right answer once more than one + external feed/party needs to co-sign updates, but building it now would be + speculative: `compliance` has no other multisig surface, and introducing one + purely for this feature before a second real attestor exists is scope creep + the issue itself explicitly cautions against ("rather than fully + implementing actual off-chain sanctions-list integration end to end"). +- Option C is the eventual target if/when a shared cross-protocol oracle + contract exists, but nothing in this workspace provides one, and building a + bespoke oracle contract is a separate, larger design effort. + +## 4. Proposed shape (for the follow-up implementation issue) + +Entrypoints, all gated on `require_admin_or_operator` (i.e. callable by +`Admin` or the designated sanctions-feed `Operator`): + +```rust +/// Applies a batch of sanctions-list updates in one call. Each entry blocks an +/// address with a structured `reason` tag (distinct from the free-text +/// `BlockReason` used by manual `block_address`) so blocks originating from an +/// external feed are distinguishable from admin-initiated blocks in +/// `export_snapshot` / indexers. +pub fn apply_sanctions_update( + env: Env, + caller: Address, // admin or operator + addresses: Vec
, + list_id: Symbol, // e.g. "OFAC_SDN"; identifies the source feed + effective_at: u64, // ledger timestamp the update should be attributed to +) -> Result<(), ContractError>; +``` + +Design notes carried over from the existing bulk entrypoints: + +- Reuse `MAX_BATCH_SIZE` and `MAX_TRACKED_ADDRESSES` guards (#8/#21/#29, #48) — + a sanctions batch is not exempt from the storage-growth bound. +- Reuse the `BULK_OP_COOLDOWN_SECS` velocity cap added for `bulk_block_addresses` + (#454) — an external feed is not a more trusted caller than the admin key, + and a compromised or malfunctioning feed should be bounded the same way. +- Emit a distinct event (`sanctions_block_applied`) rather than reusing + `address_blocked`, so downstream indexers can separate "admin decided" from + "external list said so" without parsing `BlockReason` free text. +- `list_id` is stored (new `DataKey::SanctionsSource(Address)`, analogous to + `BlockReason(Address)`) so a later `clear_address` / dispute-style appeal + process can show provenance. +- No automatic *unblock* path is proposed: removing an address from an + external list should still require an explicit admin `clear_address` call — + auto-unblocking on "the feed stopped listing it" is a materially different + trust decision (a missing/failed feed poll must never silently unblock) and + is out of scope here. + +## 5. Threat model + +- **Compromised operator key**: bounded by the existing `MAX_BATCH_SIZE` cap + plus the reused `BULK_OP_COOLDOWN_SECS` velocity cap (#454) — same blast + radius as a compromised admin key today, not worse. +- **Malicious/compromised feed pushing false positives**: the design + intentionally keeps `block_address`'s existing semantics (an address can + always be `clear_address`'d back by the real admin), and does not grant the + operator any new *irreversible* capability — only `Admin` can permanently + clear a block. +- **Stale/absent feed (false negatives)**: out of scope for this contract — + detecting that a feed has stopped publishing is an off-chain monitoring + concern, not something `compliance` can observe on its own. + +## 6. Disposition + +Per this issue's scope, this PR is **design-only**. The `apply_sanctions_update` +entrypoint and `DataKey::SanctionsSource` described in §4 are deferred to a +dedicated follow-up implementation issue, to be reviewed against this document +before any contract code is written — consistent with the issue's own +instruction to "propose the approach in the PR description and get it +reviewed before writing significant contract code."