Skip to content

ENG-162 add Kani invariant coverage#477

Merged
carrion256 merged 11 commits into
devfrom
feature/eng-162-investigate-kani-for-formal-verification-testing
Jun 24, 2026
Merged

ENG-162 add Kani invariant coverage#477
carrion256 merged 11 commits into
devfrom
feature/eng-162-investigate-kani-for-formal-verification-testing

Conversation

@carrion256

@carrion256 carrion256 commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds targeted Kani harnesses and cheaper regression/property coverage for high-risk Universal Account and Vault invariants, plus a dedicated GitHub Actions workflow to run those proofs nightly, manually, and on PRs that touch the relevant UA/Vault verification surfaces.

Universal Account coverage:

  • Kani proofs for migration scalar safety across V0/V1/unbrick migrations.
  • Kani proof for post-signature execution parameter/payload plumbing.
  • Negative tests proving signed payload/domain mutations are rejected across supported signing formats.
  • near-workspaces integration tests for nonce replay/rollback behavior and removed-key execution rejection.
  • Ignored breaking spec test documenting the future immutable domain_id replay-isolation requirement.

Vault coverage:

  • Kani proofs for bounded total-asset accounting, restore-to-idle accounting, withdrawal queue cache/FIFO/claimability, bounded sync/rebalance conservation, and allocation lifecycle accounting.
  • Kani proofs for emergency recovery from recoverable non-idle states, external sync immutability, refresh lifecycle accounting, refresh-fee accounting, and deposit/request-withdraw escrow consistency.
  • Refresh-fee Kani coverage includes an active nonzero mint path and asserts that fee refresh leaves asset accounting unchanged while updating the fee anchor and minting fee shares.
  • The deposit/request-withdraw Kani proof covers the post-deposit-equivalent state plus withdrawal request escrow/accounting consistency; the full dispatcher Deposit -> RequestWithdraw sequence remains too expensive for Kani.
  • Property tests cover deposit preview parity, atomic redeem preview/idle-cap behavior, request-withdraw escrow conservation, and mutation overflow/underflow behavior.
  • Fuzzer assertions cover floor/ceil conversion bounds.

Workflow coverage:

  • Adds .github/workflows/kani.yml with nightly, manual, and path-filtered PR triggers.
  • Splits Kani into focused jobs for Universal Account, broad Vault Kernel, Recovery, Sync External, Refresh Lifecycle, Refresh Fees, Allocation Lifecycle, and Withdraw Escrow proofs.
  • Adds a README badge for the Kani workflow.

Soroban vault CLI docs:

  • Documents that fresh vault markets need an accepted nonzero submit-set-cap --market-id ... --cap ... proposal before governance accepts them in the supply queue.
  • Documents that curator allocate-withdraw does not refresh route NAV; operators should run curator refresh-markets after adapter NAV changes before relying on share-rate/accounting views.
  • Aligns the new adapter allocation docs from dev with the cap and NAV semantics above.

Note: exact Kani proof of Vault deposit/redeem preview parity over the current production Number/U256 math path is intentionally not included. The real path currently stalls Kani, so this PR keeps that coverage as property tests rather than adding a simplified formal model that would not prove production behavior.

Validation

  • cargo fmt --all --check
  • git diff --check
  • cargo check -p templar-vault-kernel --all-features
  • cargo test -p templar-universal-account state::migration --lib -- --nocapture
  • cargo test -p templar-universal-account verify_rejects_signed_payload_mutation_for_each_format -- --nocapture
  • TEST_CONTRACTS_PREBUILT=1 cargo test -p templar-universal-account-contract replayed_nonce_fails_without_reexecuting_payload --test universal_account -- --nocapture
  • TEST_CONTRACTS_PREBUILT=1 cargo test -p templar-universal-account-contract removed_key_cannot_execute_transaction --test universal_account -- --nocapture
  • TEST_CONTRACTS_PREBUILT=1 cargo test -p templar-universal-account-contract failed_execute_does_not_consume_nonce_and_success_consumes_once --test universal_account -- --nocapture
  • cargo test -p templar-vault-kernel --test property_tests -- --nocapture
  • timeout 300s cargo kani -p templar-universal-account
  • timeout 300s cargo kani -p templar-vault-kernel
  • timeout 360s cargo kani -p templar-vault-kernel --features action-sync-external
  • cargo kani -p templar-vault-kernel --features action-refresh-fees --harness refresh_fees_zero_fee_rates_only_update_anchor --harness refresh_fees_active_rates_only_mint_fee_shares_and_update_anchor
  • GitHub Kani workflow passed on the pre-rebase head, including Vault Kernel / Refresh Fees, Vault Kernel / Withdraw Escrow, and the broad Vault Kernel job.
  • Rebased onto origin/dev at 50918bfe; local post-rebase checks passed: cargo fmt --all --check, git diff --check.
  • post-commit Soroban size-budget-check passed after rebase: 129255 bytes.

This change is Reviewable

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: db01a83c-87e6-47f7-9c5a-307890ec91d5

📥 Commits

Reviewing files that changed from the base of the PR and between 919c814 and a39c949.

📒 Files selected for processing (2)
  • .github/workflows/kani.yml
  • tools/soroban-vault-cli/README.md

📝 Walkthrough

Summary

  • Added targeted Kani harnesses for Universal Account and Vault kernel invariants, plus regression/property/fuzz coverage for high-risk UA/Vault behaviors: migration scalar safety (v0/v1/unbrick), verify_execution execution-parameter/payload plumbing, nonce replay/rollback and removed-key execution rejection, signed-payload/domain/chain mismatches (including an ignored future domain_id replay-isolation spec), and Vault accounting/lifecycle correctness (queue/escrow/FIFO/claimability, conservation across allocation/sync/rebalance/withdrawal, refresh/recovery/emergency reset, and conversion rounding bounds).
  • Extended Universal Account tests to cover:
    • skipped/incorrect nonce failures without nonce consumption or side-effects, then success with correct nonce;
    • replay rejection (no re-execution/effects, nonce not advanced);
    • key index monotonicity across removal/re-addition and nonce reset semantics on re-add;
    • inability to remove the last remaining key;
    • removed keys cannot execute (verifies no changes to nonce/state and no unintended mint/balance changes).
  • Refactored Vault kernel withdrawal/payout/emergency-reset logic to separate planning vs applying:
    • promoted queue/head and request/settlement planning helpers to pub(crate) and extracted apply-stage helpers (apply_withdrawal_request_plan, plan/apply_payout_settlement, plan_emergency_reset + wrapper handle_emergency_reset), preserving effect ordering and refund/op metadata propagation.
  • Added Vault property tests for ERC-4626-style preview/deposit/redeem/atomic withdraw/request-withdraw invariants, including escrow conservation and a deposit-preview parity property (explicitly not proven by Kani over the production Number/U256 path).
  • Added/updated supporting coverage:
    • fuzz differential check that bounded floor/ceil conversion outputs satisfy floor <= ceil <= floor+1;
    • cfg(kani) lint allowances via workspace + crate lints.rust (unexpected_cfgs warn; check-cfg whitelisting cfg(kani));
    • test-utils adds add_key/remove_key contract-call helpers.
  • Added CI to run proofs:
    • .github/workflows/kani.yml runs Kani daily, manually, and on PRs to main/dev when relevant files/surfaces change, with concurrency kani-${ref} and pinned actions/checkout, and jobs covering UA + multiple Vault lifecycle feature-sets (sync-external, withdraw-escrow, allocation lifecycle, refresh lifecycle, recovery, refresh-fees).
    • README badge for the new workflow.
  • Updated Soroban vault CLI docs:
    • clarifies curator adapter setup requirements, including nonzero market cap submission (submit-set-cap) and governance explain/accept flows;
    • tightens NAV refresh expectations after adapter/NAV changes and clarifies adapter allocation semantics (allocate-withdraw uses realized token delta, not adapter route NAV; route NAV must be monotonic/non-stale).

Notable review areas

  • Kani harness correctness & adequacy: verify asserted invariants precisely match on-chain semantics (especially UA verify_execution return value/error fields; Vault cached escrow/claimability and conservation equations across lifecycle transitions).
  • Refactor safety (Vault plan/apply split): confirm extracted/apply helpers preserve exact state mutation/effect emission ordering, especially around emergency reset refunds and the propagation of op_id/from_code.
  • Replay/domain/signature rejection logic: review mismatch-field mapping (nonce, domain_id, etc.) and the rationale/coverage for the currently ignored future domain_id replay-isolation requirement.
  • Preview vs execution accounting: ensure property expectations for preview conversions, idle-cap constraints, and atomic redeem/withdraw behaviors align with production rounding and effect calculations.
  • CI/workflow coverage gaps: confirm Kani job package selection + harness feature flags match the intended verification surfaces, and that PR path filters reliably trigger the relevant jobs.

Walkthrough

Adds Kani workflow wiring and proof harnesses for vault kernel and universal-account code, expands related tests, and updates Soroban vault CLI documentation with new governance and NAV prerequisites.

Changes

Kani Proofs, Vault Kernel Refactoring, and Test Coverage

Layer / File(s) Summary
CI workflow, workspace lint config, and README badge
.github/workflows/kani.yml, Cargo.toml, contract/vault/kernel/Cargo.toml, README.md
Adds the Kani workflow with scheduled, manual, and PR triggers; sets concurrency, permissions, and shared env vars; adds cfg(kani) lint configuration; and adds the Kani README badge.
Vault kernel action internals exposed as pub(crate)
contract/vault/kernel/src/actions/mod.rs, contract/vault/kernel/src/math/number.rs
Promotes withdrawal, payout, and emergency-reset planning types and helpers to crate visibility, extracts withdrawal request application and emergency-reset planning/apply steps, and changes the number deserializer visitor lifetime annotation.
Vault kernel Kani proof harnesses
contract/vault/kernel/src/lib.rs
Adds the cfg(kani) proof module with bounded generators and assertions, plus proof harnesses for vault state invariants, queue behavior, withdrawal planning, accounting transitions, lifecycle operations, emergency reset, sync-external behavior, and fee refresh behavior.
Vault kernel property tests and fuzz extensions
contract/vault/kernel/tests/property_tests.rs, fuzz/fuzz_targets/fuzz_fee_math.rs
Adds property tests for deposit accounting, atomic withdraw and redeem behavior, withdraw-request escrow conservation, and previewed-share parity, plus bounded floor-versus-ceil conversion assertions in the fuzz target.
Universal account Kani proofs
universal-account/src/authentication/mod.rs, universal-account/src/state/migration.rs
Adds cfg(kani) proof modules that check verify_execution matching and nonce mismatch handling, and verify legacy state transforms preserve next_key_index and chain_id.
Universal account execute_args and integration tests
universal-account/src/execute_args.rs, test-utils/src/controller/universal_account.rs, contract/universal-account/tests/universal_account.rs
Expands ExecuteArgs::verify coverage with a domain-id mismatch case, execution-domain mutation tests, signed payload mutation tests, adds add_key and remove_key controller methods, and adds integration tests for nonce rollback, replay rejection, key re-add indexing, last-key removal rejection, and removed-key execution denial.

Soroban Vault CLI Documentation Updates

Layer / File(s) Summary
CLI README governance and NAV refresh guidance
tools/soroban-vault-cli/README.md
Clarifies adapter and market-cap prerequisites, adds submit-set-cap steps to setup examples, refines NAV and allocate-withdraw descriptions, and adds refresh-markets and fresh-market guidance.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • Templar-Protocol/contracts#386: Adds Kani proof harnesses for universal-account migration logic, which is the same verification area extended here.
  • Templar-Protocol/contracts#401: Covers vault-kernel action refactoring around withdrawal and emergency-reset planning/application, which overlaps with the refactor in this PR.

Suggested reviewers

  • peer2f00l
  • SmolinPavel
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/eng-162-investigate-kani-for-formal-verification-testing

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • LINEAR integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.

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

}

#[test]
#[ignore = "ENG-162: enable after PayloadExecutionParameters stores and verifies immutable domain_id"]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@peer2f00l this is your failing test

@carrion256
carrion256 marked this pull request as ready for review June 24, 2026 08:57
@carrion256
carrion256 force-pushed the feature/eng-162-investigate-kani-for-formal-verification-testing branch from 38ee973 to 2e257b9 Compare June 24, 2026 09:01
@carrion256

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tools/soroban-vault-cli/README.md (1)

161-179: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add acceptance steps to make the setup sequence executable in timelocked governance.

At Line 161–Line 179, the flow submits proposals but never accepts them, so the “active supply route” setup can remain pending and non-operative. Add governance accept-ready steps (or explicitly scope this block to zero-timelock environments).

Suggested doc patch
 tmplr-soroban-vault governance submit-set-allowed-adapters \
   --admin GCURATOR_OR_MULTISIG... \
   --adapters CBLENDADAPTER...,CCUSTODIALADAPTER...
+tmplr-soroban-vault governance accept-ready --admin GCURATOR_OR_MULTISIG... --kind allowed-adapters
 
 tmplr-soroban-vault governance submit-set-cap \
   --admin GCURATOR_OR_MULTISIG... \
   --market-id 0 \
   --cap 1000000000
 tmplr-soroban-vault governance submit-set-cap \
   --admin GCURATOR_OR_MULTISIG... \
   --market-id 1 \
   --cap 1000000000
+tmplr-soroban-vault governance accept-ready --admin GCURATOR_OR_MULTISIG... --kind cap
 
 tmplr-soroban-vault governance submit-set-supply-queue \
   --admin GCURATOR_OR_MULTISIG... \
   --entry 0:CBLENDADAPTER... \
   --entry 1:CCUSTODIALADAPTER...
+tmplr-soroban-vault governance accept-ready --admin GCURATOR_OR_MULTISIG... --kind supply-queue
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/soroban-vault-cli/README.md` around lines 161 - 179, The governance
setup flow in the README submits proposals for allowed adapters, caps, and the
supply queue, but it never includes the acceptance step needed for timelocked
governance. Update the sequence around the governance commands to add the
corresponding governance accept-ready step after each submit, or clearly state
that this block only applies when timelock is zero. Make sure the instructions
reference the governance CLI flow shown in the README so the setup becomes
executable end-to-end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/kani.yml:
- Line 45: The Kani workflow still references the mutable
model-checking/kani-github-action@v1 tag in multiple jobs, so update every use
of that action to an immutable 40-character commit SHA. Replace the `@v1`
reference in the workflow steps that invoke the Kani action so each job is
pinned consistently, using the action identifier
model-checking/kani-github-action to locate all occurrences.

In `@contract/vault/kernel/src/actions/mod.rs`:
- Around line 112-128: `PayoutSettlement` and `EmergencyResetOutcome` currently
expose contradictory crate-visible state that `apply_payout_settlement` and
`handle_emergency_reset` must trust implicitly. Replace them with stronger
enums/variants so payout success vs failure are distinct and the refund data is
represented as a single optional payload, then update the consumers and any
Kani/test paths that inspect these results to match the new shape. Keep the
existing symbols easy to locate by refactoring around `PayoutSettlement` and
`EmergencyResetOutcome` in `actions::mod`.

In `@contract/vault/kernel/src/lib.rs`:
- Around line 211-216: The refund-owner proof check is only comparing the first
byte of the Address, so mutations in the remaining bytes can slip through.
Update assert_refund_owner_is_owner in kernel::lib to reuse assert_address_eq
for the Some(owner) case, matching the existing full-address proof pattern used
elsewhere. Also review the other affected proof assertions noted in the diff and
align them to full Address equality so the harness validates the entire
owner/receiver/refund address.

In `@contract/vault/kernel/tests/property_tests.rs`:
- Around line 1647-1712: The property test for AtomicRedeem only checks the
success path, so add an explicit reject-path assertion in
prop_atomic_redeem_respects_idle_cap. Use preview_withdraw_assets and the
existing apply_action call to branch on previewed_assets >
old_state.idle_assets, and assert result.is_err() in that case before validating
BurnShares and TransferAssets effects on the success branch. Mirror the
structure of prop_atomic_withdraw_respects_idle_cap so the test covers both the
failure and success behavior of KernelAction::AtomicRedeem.

---

Outside diff comments:
In `@tools/soroban-vault-cli/README.md`:
- Around line 161-179: The governance setup flow in the README submits proposals
for allowed adapters, caps, and the supply queue, but it never includes the
acceptance step needed for timelocked governance. Update the sequence around the
governance commands to add the corresponding governance accept-ready step after
each submit, or clearly state that this block only applies when timelock is
zero. Make sure the instructions reference the governance CLI flow shown in the
README so the setup becomes executable end-to-end.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: f490281c-7ed9-4afa-97aa-0160d0ae1d60

📥 Commits

Reviewing files that changed from the base of the PR and between 50918bf and 2e257b9.

📒 Files selected for processing (15)
  • .github/workflows/kani.yml
  • Cargo.toml
  • README.md
  • contract/universal-account/tests/universal_account.rs
  • contract/vault/kernel/Cargo.toml
  • contract/vault/kernel/src/actions/mod.rs
  • contract/vault/kernel/src/lib.rs
  • contract/vault/kernel/src/math/number.rs
  • contract/vault/kernel/tests/property_tests.rs
  • fuzz/fuzz_targets/fuzz_fee_math.rs
  • test-utils/src/controller/universal_account.rs
  • tools/soroban-vault-cli/README.md
  • universal-account/src/authentication/mod.rs
  • universal-account/src/execute_args.rs
  • universal-account/src/state/migration.rs

Comment thread .github/workflows/kani.yml Outdated
Comment thread contract/vault/kernel/src/actions/mod.rs
Comment thread contract/vault/kernel/src/lib.rs
Comment thread contract/vault/kernel/tests/property_tests.rs
@carrion256

Copy link
Copy Markdown
Collaborator Author

@coderabbitai stop jerking off and review my PR!

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@carrion256 Apologies for the frustration — I'm on it. Triggering a full review of the PR now.

✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@carrion256 Apologies for the frustration! I'll kick off a full review of your PR right away — this will re-examine all changes regardless of what was previously reviewed.

✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@carrion256 Understood — triggering a full review of your PR now. Please keep the conversation respectful going forward.

✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tools/soroban-vault-cli/README.md (1)

161-179: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add explicit proposal acceptance steps in this setup flow.

Line 161-179 shows governance submit-* commands only; in timelocked setups this sequence is incomplete without governance accept-ready, so readers can follow commands that won’t activate routes. Add matching acceptance commands (or a one-line note that these submits require acceptance before allocation works).

Suggested doc patch
 tmplr-soroban-vault governance submit-set-supply-queue \
   --admin GCURATOR_OR_MULTISIG... \
   --entry 0:CBLENDADAPTER... \
   --entry 1:CCUSTODIALADAPTER...
+tmplr-soroban-vault governance accept-ready --admin GCURATOR_OR_MULTISIG... --kind allowed-adapters
+tmplr-soroban-vault governance accept-ready --admin GCURATOR_OR_MULTISIG... --kind cap
+tmplr-soroban-vault governance accept-ready --admin GCURATOR_OR_MULTISIG... --kind supply-queue
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/soroban-vault-cli/README.md` around lines 161 - 179, The governance
setup flow in README is incomplete because the `submit-*` commands in the
`governance` section do not show the required acceptance step for timelocked
proposals. Update the documentation around the `governance
submit-set-allowed-adapters`, `governance submit-set-cap`, and `governance
submit-set-supply-queue` examples to include the matching `governance
accept-ready` command(s) or a clear note that these submissions must be accepted
before the routes become active.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@contract/vault/kernel/src/actions/mod.rs`:
- Around line 1811-1823: The emergency-reset path in plan_emergency_reset should
not blindly dequeue the withdraw_queue after refunding from OpState::Withdrawing
or OpState::Payout. First validate that the active queue head exists and matches
the current op-state/owner (and expected shares/amount) before calling
withdraw_queue.dequeue(); if the head is missing or mismatched, abort the reset
instead of reporting success. Use the existing refund_owner, refund_shares,
restore_to_idle, and withdraw_queue logic in plan_emergency_reset to enforce
this invariant.

---

Outside diff comments:
In `@tools/soroban-vault-cli/README.md`:
- Around line 161-179: The governance setup flow in README is incomplete because
the `submit-*` commands in the `governance` section do not show the required
acceptance step for timelocked proposals. Update the documentation around the
`governance submit-set-allowed-adapters`, `governance submit-set-cap`, and
`governance submit-set-supply-queue` examples to include the matching
`governance accept-ready` command(s) or a clear note that these submissions must
be accepted before the routes become active.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 97e66cb1-99db-492e-a969-2095571313d6

📥 Commits

Reviewing files that changed from the base of the PR and between 50918bf and 2e257b9.

📒 Files selected for processing (15)
  • .github/workflows/kani.yml
  • Cargo.toml
  • README.md
  • contract/universal-account/tests/universal_account.rs
  • contract/vault/kernel/Cargo.toml
  • contract/vault/kernel/src/actions/mod.rs
  • contract/vault/kernel/src/lib.rs
  • contract/vault/kernel/src/math/number.rs
  • contract/vault/kernel/tests/property_tests.rs
  • fuzz/fuzz_targets/fuzz_fee_math.rs
  • test-utils/src/controller/universal_account.rs
  • tools/soroban-vault-cli/README.md
  • universal-account/src/authentication/mod.rs
  • universal-account/src/execute_args.rs
  • universal-account/src/state/migration.rs

Comment thread contract/vault/kernel/src/actions/mod.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 4

♻️ Duplicate comments (3)
contract/vault/kernel/src/lib.rs (1)

211-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert full Address equality in these proofs.

These checks still only compare .0[0], so mutations in the other 31 bytes can slip through the harness. Reuse assert_address_eq anywhere the proof is meant to pin owner/receiver identity.

As per coding guidelines, “Write comprehensive unit tests for new, non-trivial logic.”

Also applies to: 1427-1435

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contract/vault/kernel/src/lib.rs` around lines 211 - 216, The proof helper in
assert_refund_owner_is_owner is only comparing owner.0[0], so it can miss
mutations in the rest of the Address bytes. Update this check to use
assert_address_eq (as used elsewhere for identity checks) so the full Address is
validated, and apply the same fix to the related owner/receiver proof assertions
referenced in the comment.

Source: Coding guidelines

contract/vault/kernel/tests/property_tests.rs (1)

1647-1712: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the atomic-redeem reject path explicitly.

This property still only validates successful redeems. When previewed_assets > old_state.idle_assets, a buggy implementation that succeeds or clamps would still pass the test.

As per coding guidelines, “Write comprehensive unit tests for new, non-trivial logic.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contract/vault/kernel/tests/property_tests.rs` around lines 1647 - 1712, The
property test for `prop_atomic_redeem_respects_idle_cap` only checks the success
case, so add an explicit reject-path assertion when `previewed_assets >
old_state.idle_assets` by verifying `apply_action` returns an error for
`KernelAction::AtomicRedeem`. Keep the existing success assertions for the `Ok`
branch, but make the test fail if the redeem incorrectly succeeds or clamps the
transfer amount, using the same `preview_withdraw_assets`, `apply_action`, and
`KernelEffect` checks to locate the logic.

Source: Coding guidelines

contract/vault/kernel/src/actions/mod.rs (1)

1811-1823: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate the queue head before emergency reset dequeues it.

plan_emergency_reset still refunds the owner from OpState::Withdrawing / OpState::Payout and then blindly dequeues whatever head is present. If op-state and withdraw_queue drift, reset can refund one request while removing another, or succeed with no matching queue entry at all.

As per coding guidelines, “Check invariants around refunds, withdrawals, account deletion, authorization, and replay or double-execution risks.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contract/vault/kernel/src/actions/mod.rs` around lines 1811 - 1823, The
emergency reset path in plan_emergency_reset should not blindly dequeue after
refunding from OpState::Withdrawing or OpState::Payout. Before calling
state.withdraw_queue.dequeue(), validate that the queue head matches the active
op-state being refunded (owner and escrow/amount should align), and fail or skip
the reset if the invariant is broken. Use the OpState variants and the
withdraw_queue access in this function to locate the logic and add the head
check right before the dequeue.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@contract/vault/kernel/src/lib.rs`:
- Around line 1176-1265: The emergency-reset Kani proofs are using an invalid
starting state by constructing OpState::Withdrawing and OpState::Payout while
the withdraw queue is empty, which masks queue-head mismatch issues. Update
these proofs so the initial VaultState includes a queue/op-state combination
that satisfies the real invariant for the active operation, using the existing
helpers around plan_emergency_reset, withdraw_queue, and the OpState variants to
build a valid withdrawal/payout in-progress state. Keep the assertions focused
on the reset outcome, but ensure the setup no longer depends on drift-tolerant
invalid state handling.

In `@tools/soroban-vault-cli/README.md`:
- Around line 159-174: The “active route” example in the README currently shows
only submit commands, but the setup is not effective until the relevant
proposals are accepted. Update the example around the governance
submit-set-allowed-adapters and submit-set-cap flow to either include the
matching accept-ready commands for those proposal kinds or clearly note that
acceptance is still required before the route becomes active.

In `@universal-account/src/authentication/mod.rs`:
- Around line 232-243: The `valid_raw_message` helper in `authentication::mod`
builds a `raw::Message` from `WithRawString` using an empty `raw` string paired
with a separate parsed `Payload`, which makes the signed/raw state inconsistent.
Update it to construct the message through the same serializer-backed path
production uses, such as `WithRawString::from_parsed(...)`, so the raw and
parsed forms are derived from one another before wrapping it in
`MessageWithValidSignature`. This keeps the proof harness aligned with
`verify_execution` and makes invalid states unrepresentable.

In `@universal-account/src/state/migration.rs`:
- Around line 211-260: The migration proofs in V0, V1, and UnbrickV1 only cover
empty key maps, so they miss regressions that break populated key storage.
Update the Kani proofs or add a conventional regression test around state::V0,
state::V1, V1.transform, and UnbrickV1.transform that initializes IterableMap
with at least one KeyId/KeyParameters entry and asserts the keys survive the
migration unchanged, alongside the existing scalar checks.

---

Duplicate comments:
In `@contract/vault/kernel/src/actions/mod.rs`:
- Around line 1811-1823: The emergency reset path in plan_emergency_reset should
not blindly dequeue after refunding from OpState::Withdrawing or
OpState::Payout. Before calling state.withdraw_queue.dequeue(), validate that
the queue head matches the active op-state being refunded (owner and
escrow/amount should align), and fail or skip the reset if the invariant is
broken. Use the OpState variants and the withdraw_queue access in this function
to locate the logic and add the head check right before the dequeue.

In `@contract/vault/kernel/src/lib.rs`:
- Around line 211-216: The proof helper in assert_refund_owner_is_owner is only
comparing owner.0[0], so it can miss mutations in the rest of the Address bytes.
Update this check to use assert_address_eq (as used elsewhere for identity
checks) so the full Address is validated, and apply the same fix to the related
owner/receiver proof assertions referenced in the comment.

In `@contract/vault/kernel/tests/property_tests.rs`:
- Around line 1647-1712: The property test for
`prop_atomic_redeem_respects_idle_cap` only checks the success case, so add an
explicit reject-path assertion when `previewed_assets > old_state.idle_assets`
by verifying `apply_action` returns an error for `KernelAction::AtomicRedeem`.
Keep the existing success assertions for the `Ok` branch, but make the test fail
if the redeem incorrectly succeeds or clamps the transfer amount, using the same
`preview_withdraw_assets`, `apply_action`, and `KernelEffect` checks to locate
the logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 38a9239c-b5bb-46fa-bb0a-f922d7a577e6

📥 Commits

Reviewing files that changed from the base of the PR and between 50918bf and 2e257b9.

📒 Files selected for processing (15)
  • .github/workflows/kani.yml
  • Cargo.toml
  • README.md
  • contract/universal-account/tests/universal_account.rs
  • contract/vault/kernel/Cargo.toml
  • contract/vault/kernel/src/actions/mod.rs
  • contract/vault/kernel/src/lib.rs
  • contract/vault/kernel/src/math/number.rs
  • contract/vault/kernel/tests/property_tests.rs
  • fuzz/fuzz_targets/fuzz_fee_math.rs
  • test-utils/src/controller/universal_account.rs
  • tools/soroban-vault-cli/README.md
  • universal-account/src/authentication/mod.rs
  • universal-account/src/execute_args.rs
  • universal-account/src/state/migration.rs

Comment thread contract/vault/kernel/src/lib.rs
Comment thread tools/soroban-vault-cli/README.md
Comment thread universal-account/src/authentication/mod.rs
Comment thread universal-account/src/state/migration.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tools/soroban-vault-cli/README.md (1)

162-187: 🎯 Functional Correctness | 🔵 Trivial

Human-check priority: verify CLI command/flag names remain exact (--kind allowed-adapters|cap|supply-queue, --entry, --market-id) against the current parser before release.

As per path instructions, “point out the critical aspects of the pull request that a human would be useful to inspect.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/soroban-vault-cli/README.md` around lines 162 - 187, The CLI usage
examples for governance commands should be double-checked against the current
parser before release, with special attention to the exact flag and kind names
used in the README snippets. Verify that `submit-set-allowed-adapters`,
`submit-set-cap`, `submit-set-supply-queue`, and `accept-ready` still accept the
documented `--kind allowed-adapters|cap|supply-queue`, `--entry`, and
`--market-id` values exactly as shown, and update the examples if any parser
names or accepted values have changed.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@tools/soroban-vault-cli/README.md`:
- Around line 162-187: The CLI usage examples for governance commands should be
double-checked against the current parser before release, with special attention
to the exact flag and kind names used in the README snippets. Verify that
`submit-set-allowed-adapters`, `submit-set-cap`, `submit-set-supply-queue`, and
`accept-ready` still accept the documented `--kind
allowed-adapters|cap|supply-queue`, `--entry`, and `--market-id` values exactly
as shown, and update the examples if any parser names or accepted values have
changed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b17897ad-a62c-4638-b979-f254161013ac

📥 Commits

Reviewing files that changed from the base of the PR and between 2e257b9 and bee807b.

📒 Files selected for processing (4)
  • .github/workflows/kani.yml
  • contract/vault/kernel/src/lib.rs
  • contract/vault/kernel/tests/property_tests.rs
  • tools/soroban-vault-cli/README.md

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 919c814d64

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/kani.yml
Comment thread .github/workflows/kani.yml
Comment thread tools/soroban-vault-cli/README.md Outdated
Comment on lines +177 to +179
tmplr-soroban-vault governance accept-ready \
--admin GCURATOR_OR_MULTISIG... \
--kind cap

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid the broad cap kind in setup docs

When a governance queue also has a ready cap-group proposal, this documented accept-ready --kind cap can accept more than the market-cap proposals submitted above: the CLI filters by checking whether the action/raw proposal text contains the lowercased kind string (tools/soroban-vault-cli/src/commands.rs:2998-3008), so CapGroup/cap_group_id also matches cap, and run_governance_accept_ready accepts every ready match. The setup instructions should direct operators to accept the specific proposal IDs or otherwise disambiguate the cap proposal, rather than using this broad filter.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a39c949659

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

- "Cargo.lock"
- "common/**"
- "contract/universal-account/**"
- "contract/vault/kernel/**"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Include vault macros in Kani path filter

When a PR changes only contract/vault/macros/**, this workflow is skipped because the pull_request.paths list matches contract/vault/kernel/** but not the proc-macro crate. I checked contract/vault/kernel/Cargo.toml: templar-vault-kernel depends on templar-vault-macros, so macro changes can alter generated kernel code or break Kani compilation while avoiding PR Kani until the nightly run. Please include contract/vault/macros/** or remove the filter so these Kani jobs run for that dependency.

Useful? React with 👍 / 👎.

@carrion256
carrion256 merged commit 847149f into dev Jun 24, 2026
23 checks passed
@carrion256
carrion256 deleted the feature/eng-162-investigate-kani-for-formal-verification-testing branch June 24, 2026 15:05
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.

1 participant