Skip to content

feat(recovery): account recovery — schema, controller, compiler + client support - #83

Open
willemneal wants to merge 7 commits into
mainfrom
fm/perch-recovery-stage4-p6
Open

willemneal wants to merge 7 commits into
mainfrom
fm/perch-recovery-stage4-p6

Conversation

@willemneal

@willemneal willemneal commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Perch gains opt-in account recovery: guardian, ZK, and combined recovery
modes, configured directly in an account's policy document and enforced by
a shared, immutable controller contract. Generalizes the overall shape of a
companion smart-account implementation's validated experiment
(nidohq/nido#206) into perch
itself.

  • Doc schema (perch-ir): recovery: Option<RecoveryConfig> on
    PolicyDoc — guardian-only / ZK-only / combined modes (guardian-only
    carries no ZK-shaped field at all, enforced by the type, not a runtime
    check), Loss/Protected profiles, a non-circular baseline commitment,
    and a pending_activity field with no default (see "Open decision"
    below). Canonicalized per CANONICAL.md's existing omit-when-None
    convention; documents without recovery hash byte-for-byte unchanged
    (regression tested against the pre-existing pinned fixtures, not
    regenerated). New ci-publish-recovery{,-combined} conformance vectors
    extend testdata/ in lockstep Rust+TS.
  • Compiler (perch-doc-compiler): lowers recovery into a new
    CompiledRecoveryConfig wire type on CompiledDoc, resolving
    replaceable signer ids to sha256 credential fingerprints (of the
    decoded key bytes, casing-insensitive) so revocation survives an id being
    reused for a different physical key in a later document.
  • Shared controller (new perch-recovery crate): an OZ Policy
    implementing initiation/delay/expiry/cancel/completion, guardian quorum,
    and a generic proof-system-agnostic ZK adapter (no circuit shipped — see
    docs/recovery/controller-governance.md's "ZK adapter scope"). Variant A
    completion (recovery authorizes apply_doc itself, via a zero-signer
    "recovery" context rule) and a guard_apply_doc reconfigure gate: a
    Loss-profile account's ordinary admin can change or remove recovery
    freely; a Protected account additionally needs the currently enrolled
    recovery condition's evidence, with no restriction to additive changes.
    contract-feature-gated like perch-doc-compiler so consumers (i.e.
    perch-smart-account) link only the client, not controller logic.
  • perch-smart-account: apply_doc gains a recovery_evidence
    parameter, installs/removes the recovery rule, and gates every call
    through guard_apply_doc whenever a controller is enrolled (zero
    overhead otherwise).
  • perch-js: schema/builder support for recovery; parity tests prove
    the builder reproduces the Rust-pinned hashes.
  • docs/recovery/: schema design, controller governance, VK/controller
    immutability review, an account-mutation-path inventory, a migration doc
    for existing non-upgradeable accounts (a new account plus explicit
    ownership transfer is always required — there is no in-place upgrade
    path, since the doc-compiler address is baked into account wasm at build
    time), and a Lean/perch-conformance impact assessment (scoped out with
    rationale where a change wasn't undertaken).

Open decision: activity during a pending recovery attempt (release-blocking)

docs/recovery/pending-activity-policy.md records this as a deliberately
unresolved, release-blocking configuration decision — not something this PR
decides on anyone's behalf. While a recovery attempt is pending, should the
account's ordinary, admin-authorized activity continue, or freeze?

  • Freeze everything limits what a compromised admin can do during the
    delay window, at the cost of availability for that window even if the
    attempt is spurious.
  • Continue everything preserves availability, at the cost that a
    genuinely compromised admin can keep acting until recovery actually takes
    effect.
  • Restrict selectively (not modeled here) could preserve more of both,
    but needs its own capability-boundary design this PR doesn't attempt.

The schema forces every enrollment to name Freeze or Continue explicitly
— no default, no silent third option. One property is already unconditional
regardless of that choice: the controller refuses any policy-document change
not authorized by the recovery evidence itself while an attempt is pending.
What remains genuinely open is ordinary activity (moving funds, calling
other contracts) — recorded and queryable, but not yet mechanically enforced
end-to-end (enforcing Freeze against arbitrary non-recovery context rules
needs a cross-cutting interpreter change this PR doesn't make). No
recovery-enabled account should be treated as production-ready for resisting
a compromised admin specifically during the pending window until whoever
operates the account/protocol makes this decision explicitly.

Security fixes from review

An automated review pass surfaced a set of genuine authentication and
timing gaps in the initial perch-recovery implementation, all fixed here:

  • install, enforce, and guard_apply_doc are exported contract
    functions callable directly by anyone, not only via the real OZ/apply_doc
    flow. Each now starts with require_auth() on the relevant account,
    which succeeds for free on the genuine path (Soroban invoker-contract
    authorization) and rejects a direct, forged call — verified directly
    against the pinned soroban-env-host's own auth-tracker source, and
    explained in contract.rs's doc comments and
    controller-governance.md's new "Caller authentication" section.
  • submit_guardian_approval/submit_guardian_cancel now authenticate a
    digest bound to the specific attempt (id, action, target, config) via
    require_auth_for_args, instead of the bare (account, guardian) call
    arguments, so a signature can't be redirected to authorize a different
    attempt than the one a guardian actually approved.
  • submit_zk_proof reserves its nullifier immediately after verification
    instead of deferring to completion, closing a window where the same
    nullifier could authorize two different statements before either
    finished.
  • begin_compromise_attempt now requires its target to equal the enrolled
    baseline exactly, instead of only checking that a baseline exists.
  • Attempt gains an evidence_deadline, closing a permissionless,
    no-cost-to-grief path where a CollectingEvidence attempt with no
    evidence ever arriving would block ordinary apply_doc calls forever.
  • perch-recovery's soroban-sdk-tools dependency is no longer gated
    behind the contract feature, fixing a client-only build (as
    perch-smart-account uses) that referenced #[scerr] unconditionally.
  • TTL extension across the controller now uses the network's live
    max_ttl() instead of a fixed ~180-day constant, extends on the
    highest-traffic read paths (not just writes), and a new permissionless
    renew entry point lets a keeper extend an otherwise-idle account's
    state indefinitely — documented as a genuine, inherited platform
    constraint rather than solved outright (controller-governance.md's
    "Keeping permanent state alive").
  • Checked arithmetic in the timelock/expiry computation, and the
    RecoveryAuthorized event now names the actual account instead of the
    controller's own address.
  • A "What the commitment does, and does not, verify" section makes
    explicit that a target document's content correctness relative to
    replaceable/the permanent revoked set is an evidence-provider review
    responsibility, not something the hash-only commitment verifies
    on-chain — the same trust boundary the companion implementation above
    also accepts.

Wire-shape note: 0-or-1 Vec fields converted to Option where possible

Audited every compiled wire-type field documented as holding "zero or one"
entries in a Vec (a pattern used to emulate Option around a Soroban SDK
derive-macro limitation). Empirically confirmed the exact limitation first:
#[contracttype]'s derive macro has no ScVal conversion for Option<T>
where T is itself a custom #[contracttype] struct or enum, but
Option<T> for a host-builtin T (BytesN<32>, Address, Bytes, ...)
works fine end to end through a generated client.

Converted (builtin element type, so the invariant is now type-enforced):
CompiledRecoveryConfig::baseline, CompiledZkVerifierConfig::pool (and
ZkVerifierInterface::verify_proof's pool parameter to match),
ReconfigureEvidence::zk_nullifier/zk_proof, Attempt::nullifier — all
Vec<T>Option<T>.

Left as Vec, with the exact reason now stated at each field (element type
is itself a #[contracttype] struct, so Option doesn't compile):
CompiledRule::install, CompiledRule::cap, CompiledDoc::recovery.

This is wire-shape-only: perch-ir's canonical-JSON layer and
packages/perch-js/ are untouched (confirmed by empty diffs), because
doc_hash is computed from the canonical JSON document before compilation —
it has no dependency on how the compiled wire types are laid out. No
already-deployed instance is affected either way (none of this has shipped
on-chain yet).

Testing

  • cargo test --workspace, cargo fmt --all --check,
    cargo clippy --workspace --all-targets -- -D warnings: all clean.
  • npm test in packages/perch-js: 48 tests passing.
  • just drt (Lean differential replay): green, unaffected by this change
    (see docs/recovery/formal-verification-impact.md for why — recovery
    never lowers to a perch-program op).
  • crates/integration-tests/tests/recovery.rs: the full guardian-only
    lifecycle (enroll → initiate → guardian quorum → timelock → complete;
    replay/expiry/wrong-target refusal; cancellation as
    guardian-only/ZK-only/Combined requiring both factors; a pending
    attempt unconditionally blocking ordinary apply_doc; Protected
    reconfigure requiring guardian evidence, admin alone refused; baseline
    mismatch refusal; evidence-deadline timeout; nullifier-reservation
    immediacy; credential-fingerprint hex-casing) proven against the real OZ
    do_check_auth/Policy::enforce path (not host-level auth mocking,
    which — confirmed against soroban-env-host's own source — never
    invokes a custom account's __check_auth at all).
  • crates/perch-recovery/src/zk.rs's own unit test pins down that the
    statement guardian signatures and ZK proofs are bound to actually varies
    with every distinguishing field, independent of any authorization
    mocking.

Test plan for reviewers

  • Read docs/recovery/README.md first — it indexes the rest and names
    the pending-activity-policy gate up front.
  • Confirm the pre-existing ci-publish{,-delegated,-threshold}
    fixtures' pinned hashes are untouched (only new ci-publish-recovery*
    files were added to testdata/).
  • Review crates/perch-recovery/src/contract.rs's guard_apply_doc,
    the require_auth() gates on install/enforce/guard_apply_doc,
    and the digest-bound guardian authentication in
    submit_guardian_approval/submit_guardian_cancel.
  • Confirm .github/workflows/release.yml's perch-recovery
    registration (tagging/version-pin tracking only — deliberately
    excluded from the constructorless publish ALLOW list pending a
    separate on-chain-publish decision).

…r + client support

Finalizes Stage 4 of the perch recovery plan (follow-up.md), generalizing
the validated Nido Stage 3 experiment (fm/nido-recovery-stage3-n8, PR
nidohq/nido#206) into perch itself:

- perch-ir: `recovery: Option<RecoveryConfig>` on PolicyDoc (guardian-only /
  zk-only / combined modes, Loss/Protected profiles, non-circular baseline
  commitment, no-default pending-activity policy per §7), canonicalized per
  CANONICAL.md's existing omit-when-None convention, and validated. New
  ci-publish-recovery{,-combined} conformance fixtures extend testdata/ in
  lockstep Rust+TS; existing fixtures are byte-for-byte unchanged (regression
  tested).
- perch-doc-compiler: lowers the doc's recovery section into
  `CompiledRecoveryConfig` (a new field on `CompiledDoc`), resolving
  replaceable signer ids to credential fingerprints so revocation survives
  id reuse across documents.
- perch-recovery (new crate): the shared recovery controller as an OZ
  `Policy` — initiation/delay/expiry/cancel/completion, guardian quorum and
  a generic ZK verifier adapter (no circuit shipped — see
  docs/recovery/controller-governance.md), config_hash, Variant A completion
  (recovery authorizes apply_doc via a zero-signer `"recovery"` context
  rule), and a general Protected-reconfigure gate (`guard_apply_doc`)
  generalizing Nido's additive-only reconfigure to the full requirement.
  `contract`-feature-gated like perch-doc-compiler so consumers link only
  the client.
- perch-smart-account: `apply_doc` grows a `recovery_evidence` parameter,
  installs/removes the recovery rule, and gates every call through
  `guard_apply_doc` whenever a controller is enrolled.
- perch-js: schema/builder support for `recovery`, parity tests proving the
  builder reproduces the Rust-pinned hashes.
- docs/recovery/: schema design, controller governance, VK/controller
  immutability review, account-mutation-path inventory, migration doc for
  existing non-upgradeable accounts, Lean/conformance impact assessment, and
  the §7 pending-activity gate recorded as an explicit, release-blocking,
  unresolved parameter (not decided by this change).
- .github/workflows/release.yml: registers perch-recovery for
  tagging/version-pin tracking, deliberately excluded from the
  constructorless publish allow-list until on-chain publish is a separate,
  deliberate decision.

End-to-end guardian-only recovery (enroll → initiate → guardian quorum →
timelock → complete, replay/expiry/wrong-target refusal, cancellation as a
separate evidence domain, Protected-reconfigure gating) is proven against
the real OZ `do_check_auth`/`Policy::enforce` path in
crates/integration-tests/tests/recovery.rs.
Mechanical only: clippy::len_zero, clippy::cloned_ref_to_slice_refs in the
new cancellation tests, and rustfmt on the AND-gate fix in contract.rs. No
behavior change; the pipeline's test step (which passed all 15 recovery
integration tests plus the broader targeted suite) never reached the lint
step because a known no-mistakes test-analyzer bug
("scenario N result 'pass' requires live validation") failed the run first.
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://stellar-registry.github.io/perch/pr-preview/pr-83/

Built to branch gh-pages at 2026-09-16 18:24 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical recovery-controller security issues and dependency-release pin problems block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds opt-in account recovery across the policy schema, compiler, smart account, shared controller, JavaScript client, documentation, and tests.

Changes:

  • Adds guardian, ZK, and combined recovery schemas with canonical fixtures.
  • Integrates recovery authorization into apply_doc.
  • Adds controller lifecycle support, client builders, migration guidance, tests, and release tracking.
File summaries
File Reviewed change
testdata/README.md Documents recovery fixtures.
testdata/ci-publish-recovery.json Guardian-only recovery fixture.
testdata/ci-publish-recovery.doc-hash Guardian-only document hash.
testdata/ci-publish-recovery.canonical.json Guardian-only canonical fixture.
testdata/ci-publish-recovery-combined.json Combined recovery fixture.
testdata/ci-publish-recovery-combined.doc-hash Combined document hash.
testdata/ci-publish-recovery-combined.canonical.json Combined canonical fixture.
README.md Documents recovery support.
packages/perch-js/test/schema.test.ts Tests recovery schema validation.
packages/perch-js/test/parity.test.ts Tests Rust and TypeScript parity.
packages/perch-js/test/builder.test.ts Tests recovery builder behavior.
packages/perch-js/src/schema.ts Defines recovery schema types.
packages/perch-js/src/builder.ts Adds recovery builder support.
docs/recovery/vk-and-controller-immutability.md Documents immutability requirements.
docs/recovery/section-7-gate.md Records the pending-activity release gate.
docs/recovery/schema.md Documents recovery schema semantics.
docs/recovery/README.md Indexes recovery documentation.
docs/recovery/migration.md Documents migration constraints.
docs/recovery/formal-verification-impact.md Describes verification impact.
docs/recovery/controller-governance.md Documents controller governance.
docs/recovery/account-mutation-paths.md Inventories account mutation paths.
crates/perch-testkit/src/lib.rs Exposes recovery test helpers.
crates/perch-testkit/src/fixture.rs Adds recovery evidence fixtures.
crates/perch-testkit/Cargo.toml Adds recovery test dependencies.
crates/perch-smart-account/src/lib.rs Integrates recovery into apply_doc.
crates/perch-smart-account/Cargo.toml Adds the recovery client dependency.
crates/perch-recovery/src/zk.rs Defines ZK adapter support.
crates/perch-recovery/src/types.rs Defines recovery attempt types.
crates/perch-recovery/src/storage.rs Defines recovery controller storage.
crates/perch-recovery/src/lib.rs Exposes recovery API and client types.
crates/perch-recovery/src/contract.rs Implements recovery lifecycle and policy enforcement.
crates/perch-recovery/Cargo.toml Defines the recovery controller crate.
crates/perch-ir/tests/recovery.rs Tests recovery parsing and validation.
crates/perch-ir/tests/common/mod.rs Updates recovery test document helpers.
crates/perch-ir/src/validate.rs Validates recovery configuration.
crates/perch-ir/src/parse.rs Parses recovery configuration.
crates/perch-ir/src/lib.rs Exposes recovery schema support.
crates/perch-ir/src/doc.rs Defines recovery document models.
crates/perch-ir/src/canon.rs Canonicalizes recovery documents.
crates/perch-doc-compiler/src/lib.rs Lowers recovery configuration to wire types.
crates/perch-conformance/tests/differential.rs Updates conformance document generation.
crates/integration-tests/tests/cap_matrix.rs Updates capability test documents.
crates/integration-tests/tests/apply_doc.rs Updates account mutation tests.
crates/integration-tests/tests/apply_doc_cap.rs Updates capped mutation tests.
crates/integration-tests/test_snapshots/reapply_replaces_the_whole_rule_set.1.json Refreshes reapply snapshot.
crates/integration-tests/test_snapshots/protected_reconfigure_requires_guardian_evidence_admin_alone_is_refused.1.json Adds protected reconfiguration snapshot.
crates/integration-tests/test_snapshots/piecemeal_mutation_entry_points_do_not_exist.1.json Refreshes mutation snapshot.
crates/integration-tests/test_snapshots/garbage_and_unknown_fields_are_rejected.1.json Refreshes rejection snapshot.
crates/integration-tests/test_snapshots/doc_without_admin_rule_is_rejected_anti_brick.1.json Refreshes anti-brick snapshot.
crates/integration-tests/test_snapshots/doc_for_another_network_is_rejected.1.json Refreshes network rejection snapshot.
crates/integration-tests/test_snapshots/apply_doc_rejects_a_cap_token_that_is_not_the_scope.1.json Refreshes capability rejection snapshot.
crates/integration-tests/test_snapshots/apply_doc_installs_the_cap_beside_the_interpreter.1.json Refreshes capability installation snapshot.
crates/integration-tests/test_snapshots/apply_doc_installs_rules_and_stores_canonical_hash.1.json Refreshes apply-doc snapshot.
crates/integration-tests/Cargo.toml Adds recovery test dependencies.
Cargo.toml Adds the recovery workspace member.
Cargo.lock Locks recovery dependencies.
AGENTS.md Records recovery and testing guidance.
.github/workflows/release.yml Registers recovery release tracking.
Review details

Suppressed comments (5)

.github/workflows/release.yml:247

  • The new perch-recovery contract embeds perch-doc-compiler, but the perch-doc-compiler case above does not include perch-recovery. A compiler-only release will therefore leave this crate's 0.2.1 path pin stale; with Cargo's 0.x caret rules, the next compiler minor can make whole-workspace metadata fail. Include the reverse consumer in the compiler release scope or update this pin as part of the release process.
    crates/integration-tests/Cargo.toml:25
  • The integration-test crate also pins the new controller at 0.1.0, but it is not in the controller's release scope. Once perch-recovery crosses a 0.x minor, this dev-only pin will make workspace-wide Cargo metadata fail until manually corrected. Include it in the dependency-pin update process.
    crates/perch-recovery/src/contract.rs:613
  • next_attempt_id is documented as monotonic and cancels_used as a lifetime counter, but these persistent entries are written without any TTL extension. They can expire at the storage default, resetting attempt ids and the cancellation cap; this also weakens the attempt/nullifier replay assumptions. Extend both counters whenever they are updated, or store them under a durable retention policy.
    crates/perch-recovery/src/contract.rs:574
  • maybe_promote does not receive the account and uses e.current_contract_address(). In both callers the current contract is the recovery controller, so every RecoveryAuthorized.account event records the controller address instead of the enrolled smart-account address. Pass the account through and publish it in the event so off-chain recovery tracking is not misattributed.
    crates/perch-recovery/src/contract.rs:135
  • Attempt::target_doc_hash is documented as the SHA-256 of canonical document bytes, and PerchDocCompiler::compile_doc returns that canonical doc_hash, but this hashes the raw apply_doc argument. A pretty-printed or minified equivalent therefore gets a different hash and a recovery initiated with the reviewed doc_hash cannot complete unless the exact serialization is guessed. Compare against the compiler's canonical hash (or consistently redefine the commitment as raw bytes).
  • Files reviewed: 46/73 changed files
  • Comments generated: 22
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/perch-doc-compiler/src/lib.rs Outdated
Comment thread crates/perch-recovery/Cargo.toml Outdated
Comment thread crates/perch-recovery/src/contract.rs Outdated
Comment thread crates/perch-recovery/src/contract.rs Outdated
Comment thread crates/perch-recovery/src/contract.rs
Comment thread crates/perch-recovery/src/storage.rs
Comment thread crates/perch-recovery/src/contract.rs Outdated
Comment thread crates/perch-recovery/src/contract.rs
Comment thread crates/perch-recovery/src/contract.rs
Comment thread crates/perch-recovery/src/types.rs Outdated
…s standalone

Fold every finding from PR #83's review into the account-recovery feature
itself, and drop the internal staged-plan framing from its docs and code
comments so the feature is documented and reviewable on its own terms.

Security/correctness fixes in crates/perch-recovery and perch-doc-compiler:
- `install`, `enforce`, and `guard_apply_doc` are exported contract
  functions callable directly by anyone, not only via the real OZ/apply_doc
  flow. Each now starts with `require_auth()` on the relevant account,
  which succeeds for free on the real path (Soroban invoker-contract
  authorization) and rejects a direct, forged call — verified against the
  pinned soroban-env-host's own auth-tracker source.
- `submit_guardian_approval`/`submit_guardian_cancel` now authenticate a
  digest bound to the specific attempt (id, action, target, config) via
  `require_auth_for_args`, instead of the bare `(account, guardian)` call
  arguments, so a signature can't be redirected to a different attempt.
- `submit_zk_proof` reserves its nullifier immediately after verification
  instead of deferring to completion, closing a window where the same
  nullifier could authorize two different statements before either
  finished.
- `begin_compromise_attempt` now requires its target to equal the enrolled
  baseline exactly, instead of only checking that a baseline exists.
- `Attempt` gains `evidence_deadline`, closing a permissionless,
  no-cost-to-grief path where a `CollectingEvidence` attempt with no
  evidence ever arriving would block ordinary `apply_doc` calls forever.
- `credential_fingerprint` decodes a signer's hex key to its physical bytes
  before hashing, matching `perch-ir` validation's own casing-insensitive
  treatment, so re-declaring a revoked credential under different hex
  casing no longer evades revocation.
- `perch-recovery`'s `soroban-sdk-tools` dependency is no longer gated
  behind the `contract` feature, fixing a client-only build (as
  `perch-smart-account` uses) that referenced `#[scerr]` unconditionally.
- TTL extension across the controller now uses the network's live
  `max_ttl()` instead of a fixed ~180-day constant, extends on the
  highest-traffic read paths (not just writes), and a new permissionless
  `renew` entry point lets a keeper extend an otherwise-idle account's
  state indefinitely — documented in controller-governance.md as a real,
  inherited platform constraint rather than solved outright.
- Checked arithmetic in the timelock/expiry computation, and the
  `RecoveryAuthorized` event now names the actual account instead of the
  controller's own address.

Regression tests added in crates/perch-recovery (a `zk::statement`
unit test proving the guardian-approval digest actually varies per
attempt) and crates/integration-tests/tests/recovery.rs (baseline
mismatch, evidence-deadline timeout, nullifier-reservation immediacy,
fingerprint hex-casing).

Docs: reworded docs/recovery/* and the crate's own doc comments to explain
every design decision on its own terms (renamed section-7-gate.md to
pending-activity-policy.md, dropped "Stage 4"/"§N" references throughout),
added a "What the commitment does, and does not, verify" section making
explicit that target-document content correctness relative to
`replaceable`/`revoked` is an evidence-provider review responsibility, not
on-chain-verified — the same trust boundary a companion smart-account
implementation this design generalizes from also accepts.

Behavior for existing (non-recovery, and previously-passing recovery)
flows is unchanged; full workspace test/fmt/clippy suite and perch-js
tests pass.
@willemneal willemneal changed the title feat(recovery): Stage 4 — recovery schema, shared controller, compiler + client support feat(recovery): account recovery — schema, controller, compiler + client support Sep 14, 2026
@willemneal

Copy link
Copy Markdown
Contributor Author

Addressing the review's 5 "suppressed" findings, which GitHub didn't give their own inline comment threads to reply on directly:

  • .github/workflows/release.yml:247 (perch-doc-compiler's paths_for() doesn't list perch-recovery as a reverse consumer) and crates/integration-tests/Cargo.toml:25 (dev-only pin on perch-recovery not in its release scope) — no code change. Both already follow the established pattern in this file: a crate that takes a real (non-dev) path dependency lists its upstream dependency in its own paths_for() entry, so a change to the upstream crate is picked up when computing the downstream crate's own version bump — see the existing perch-account case (line 246), which already does exactly this two levels deep (perch-accountperch-smart-accountperch-recoveryperch-doc-compiler), and perch-recovery's own entry (line 247) already lists perch-doc-compiler. The dev-only case (crates/integration-tests, a test-only consumer never itself released) is the deliberately-excluded exception documented in AGENTS.md's "Release pipeline sharp edge" entry, added after a real incident (PR fix(doc-compiler): unblock 0.2.0 cap-capable release, republish as 0.2.1 #79) — the mitigation there is a manual grep-and-bump step at release time, not automatic scoping, since a dev-only crate never gets its own release PR to carry that update.
  • crates/perch-recovery/src/contract.rs:613 (next_attempt_id/cancels_used TTL) — fixed; duplicates the two inline comments on those lines, replied to individually.
  • crates/perch-recovery/src/contract.rs:574 (maybe_promote uses e.current_contract_address() instead of the account in RecoveryAuthorized) — fixed: maybe_promote now takes account: &Address and publishes it in the event. This one had no inline duplicate, so noting it here since there's no thread to reply on.
  • crates/perch-recovery/src/types.rs:135 (target_doc_hash doc comment) — fixed; duplicates the inline comment on that line, replied to individually.

… allows it

Audited every field across the compiler and recovery wire types documented
as holding "zero or one" entries in a `Vec` (a pattern used to emulate
`Option` around a Soroban SDK limitation). Empirically confirmed the exact
limitation first (a throwaway probe test, not committed): `#[contracttype]`'s
derive macro has no `ScVal` conversion for `Option<T>` where `T` is itself a
custom `#[contracttype]` struct or enum ("the trait bound `ScVal:
TryFrom<&Option<T>>` is not satisfied") — but `Option<T>` for a host-builtin
T (`BytesN<32>`, `Address`, `Bytes`, `u32`, ...) works fine, verified via a
full round-trip through a generated `#[contractclient]`.

Converted (element type is a host builtin, so `Option<T>` compiles and the
invariant is now type-enforced instead of documented-and-hoped):
- `CompiledRecoveryConfig::baseline`: `Vec<BytesN<32>>` -> `Option<BytesN<32>>`
- `CompiledZkVerifierConfig::pool`: `Vec<Address>` -> `Option<Address>`
  (and `ZkVerifierInterface::verify_proof`'s `pool` parameter to match)
- `ReconfigureEvidence::zk_nullifier`/`zk_proof`: `Vec<BytesN<32>>`/`Vec<Bytes>`
  -> `Option<BytesN<32>>`/`Option<Bytes>` (kept as two parallel `Option`s,
  not bundled into one `Option<(nullifier, proof)>` — a tuple or wrapper
  struct would hit the same custom-type limitation)
- `Attempt::nullifier`: `Vec<BytesN<32>>` -> `Option<BytesN<32>>`

Left as `Vec` with the exact reason now stated at each field (element type
is itself a `#[contracttype]` struct, so `Option` doesn't compile):
- `CompiledRule::install: Vec<InstallParams>`
- `CompiledRule::cap: Vec<CompiledCap>`
- `CompiledDoc::recovery: Vec<CompiledRecoveryConfig>`

`CompiledRecoveryConfig::replaceable` was checked and is genuinely
multi-valued (a set of many credential fingerprints), not a 0-or-1 field —
left as `Vec` with no change.

This is a wire-shape-only change: `perch-ir`'s canonical-JSON layer
(`crates/perch-ir/`, `testdata/*.canonical.json`, `*.doc-hash`) and
`packages/perch-js/` are untouched (confirmed by empty diffs) and continue
to pass unmodified, because `doc_hash` is computed from the canonical JSON
document before compilation — it has no dependency on how the *compiled*
wire types are laid out. Updated call sites in `perch-recovery::contract`
and the `no_recovery_evidence` test helper accordingly; test snapshots
regenerated (Vec `{vec: []}`/`{vec: [x]}` XDR shapes become `void`/`x`
directly). Full workspace test/fmt/clippy -D warnings clean, wasm builds
verified via `stellar contract build` for perch-recovery/perch-doc-compiler/
perch-smart-account/perch-account, perch-js suite clean (48 tests).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical and moderate findings remain in recovery authorization, release synchronization, and account/deploy integration.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 31/78 changed files
  • Comments generated: 2
  • Review effort level: Lite

# client apply_doc cross-calls), so a perch-recovery change re-releases
# the account too — see AGENTS.md's version-pin sharp edge.
perch-account) echo "perch-account perch-smart-account perch-doc-compiler perch-compile perch-ir perch-program perch-registry-resolve perch-registry-resolve-macro perch-recovery";;
perch-recovery) echo "perch-recovery perch-doc-compiler perch-compile perch-ir perch-program";;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed: added a sync pass to release-pr right after the per-contract bump loop. For every crate actually bumped this run, it now greps every crates/*/Cargo.toml — tracked contracts and dev-only consumers alike, not just the ones in that contract's own paths_for() scope — for a { version = "...", path = "..." } pin naming it, and rewrites that pin's version in place. Verified against a real Ubuntu/GNU-sed container (matching runs-on: ubuntu-latest) with a stubbed git cliff forcing a 0.x minor bump on two contracts — confirmed every dependent pin (perch-smart-account, perch-testkit, integration-tests, and perch-recovery's own pin on perch-doc-compiler) updates correctly. Also updated AGENTS.md's sharp-edge entry, which described the old manual grep-and-bump step this automates.

Comment thread .github/workflows/release.yml Outdated
# perch-recovery is deliberately NOT in this allow-list yet: it is
# tagged/versioned (CONTRACTS above) so its intra-workspace pins stay
# tracked, but publishing a recovery controller on-chain is a
# separate, deliberate decision (see docs/recovery/section-7-gate.md)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed: corrected the path to docs/recovery/pending-activity-policy.md (the file was renamed from section-7-gate.md during an earlier framing pass on this PR and this one reference was missed).

…ix stale doc path

New review comments on PR 83:

- release.yml's release-pr job bumped a contract's own Cargo.toml version
  but never touched any *dependent* crate's `{ version = "...", path = "..." }`
  pin on it — including tracked contracts (perch-recovery pins
  perch-doc-compiler) and dev-only consumers (perch-testkit,
  integration-tests pin perch-recovery). A 0.x minor bump is breaking under
  Cargo's caret rules, so a stale pin fails `cargo metadata` workspace-wide
  the next time anything touches the dependent crate — exactly what happened
  once already (PR #79, fixed by hand at the time). Added a sync pass after
  the per-contract bump loop: for every crate actually bumped this run, grep
  every crates/*/Cargo.toml for a pin naming it and rewrite that pin's
  version in place. Verified the exact sed/grep logic against a real
  Ubuntu container (matching the runs-on: ubuntu-latest / GNU sed the job
  actually executes under — a first pass tested against this machine's BSD
  sed and produced a false negative, since BSD sed's `-i` argument handling
  differs from GNU's) with a stubbed git-cliff forcing a 0.x minor bump on
  two contracts; confirmed every dependent pin (tracked and dev-only alike)
  updates correctly while preserving surrounding TOML formatting.
- A `publish-plan` comment still pointed at the pre-rename
  `docs/recovery/section-7-gate.md`; corrected to
  `docs/recovery/pending-activity-policy.md`.

Updated AGENTS.md's release-pipeline sharp-edge entry to describe the new
automated behavior instead of the old "grep and bump by hand" instruction,
which this change makes obsolete for anything going through `release-pr`.

actionlint clean; full workspace cargo test/fmt/clippy -D warnings clean
(unaffected — workflow/doc-only change).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants