feat(vault): migrate governance to compact bridge#417
Closed
carrion256 wants to merge 77 commits into
Closed
Conversation
This was referenced Apr 23, 2026
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds fixed-point financial primitives ( Changes
Sequence Diagram(s)sequenceDiagram
participant Gov as Governance Contract
participant Vault as Vault Runtime
participant Storage as Config/Policy Storage
participant Lock as Market Execution Lock
participant Callback as Market Callback
Gov->>Vault: execute_governance(caller, payload)
Note over Vault: decode GovernanceCommand, validate caller
Vault->>Storage: apply governance action (fees/caps/curator/...)
Storage-->>Vault: ack
Vault-->>Gov: return ()
Callback->>Vault: withdrawal/rebalance callback(op_id, token)
Vault->>Lock: has_current_lease(market, op_id, token)?
Lock-->>Vault: true / false
alt current lease
Vault->>Storage: perform settlement / state update
Storage-->>Vault: updated
else stale callback
Vault-->>Callback: early return (no-op)
end
Vault->>Lock: unlock(market, op_id, FencingToken(...))
Vault-->>Callback: complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
This was referenced Apr 23, 2026
peer2f00l
approved these changes
Apr 24, 2026
carrion256
force-pushed
the
refactor/vault-ergonomics
branch
from
April 24, 2026 13:42
6b6ccc2 to
a951db4
Compare
This reverts commit 9bc4876.
carrion256
force-pushed
the
spr/refactor/vault-ergonomics/4f330057
branch
from
April 24, 2026 15:03
4f964ca to
71ecf80
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
contract/vault/README.md (1)
158-173:⚠️ Potential issue | 🟠 MajorAuthorization diagram incomplete: Curator can execute Sentinel actions but diagram omits this edge
The code in
contract/vault/soroban/src/auth/mod.rs(lines 61-67, 48-57) shows thatCuratorimplicitly fulfills all delegated roles via theis_curator_or()fallback—when aSentinelorAllocatorrole is required, theCuratoraddress passes the check. This means theCuratorcan executePauseandSetRestrictionsactions.The README diagram (lines 158-173) does not document this transitive capability. Add a
Curator → Sentineledge to match the implemented closure, or add a note explaining thatCuratoris the implicit fallback for all delegated roles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/README.md` around lines 158 - 173, The README mermaid authorization diagram is missing the transitive edge showing Curator's implicit authority over Sentinel actions; update the diagram block to include a "Curator --> Sentinel" edge (or add a short caption stating that is_curator_or() lets Curator act as any delegated role), referencing the Curator and Sentinel roles and the is_curator_or() fallback in auth/mod.rs so the docs match the implementation.contract/vault/soroban/governance/src/lib.rs (1)
582-589:⚠️ Potential issue | 🟠 MajorKind collisions make
accept_kindand abdication ambiguous.
GovernanceActionKind::CapGroupnow covers three different mutations, andGovernanceActionKind::Skimcovers both recipient rotation and token skims. Because this enum is now used byaccept_kind,revoke_kind, and irreversible abdication, callers can execute/revoke the wrong pending proposal and abdication becomes broader than the old per-action behavior. These externally addressable actions need distinct kinds, or those APIs need to stay keyed at the finer-grained action level.Based on learnings: "Governance abdication is irreversible. Any change to governance action kind mapping or timelock policy needs a high-suspicion review."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/soroban/governance/src/lib.rs` around lines 582 - 589, The GovernanceActionKind enum conflates multiple different GovernanceAction variants (e.g., SetGroupCap, SetGroupRelCap, SetGroupMember into CapGroup and SetSkimRecipient/Skim into Skim), which makes accept_kind, revoke_kind and irreversible abdication ambiguous; modify GovernanceActionKind to introduce distinct variants for each concrete action (for example CapGroupSet, CapGroupRelSet, CapGroupMemberSet, SkimSetRecipient, SkimExecute or similarly named per-action kinds), update the match mapping in lib.rs that maps GovernanceAction -> GovernanceActionKind to return these new fine-grained kinds, and then update all callers of accept_kind, revoke_kind and any abdication logic to use the new per-action kinds so that execution/revocation/abdication remain precise and unambiguous (reference symbols: GovernanceAction, GovernanceActionKind, accept_kind, revoke_kind, abdication).contract/vault/near/src/tests.rs (1)
358-367:⚠️ Potential issue | 🟠 MajorUse a non-equal
request_idin at least one regression case.These fixtures all mirror
request_idtoop_id, so they still pass if the implementation accidentally keeps reconciling or dequeuing byop_id. Please add one end-to-end case where the two differ and assert cleanup follows the queued request id. Based on learnings: Check invariants around refunds, withdrawals, account deletion, authorization, and replay or double-execution risks.Also applies to: 625-633, 1068-1077, 1155-1164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/near/src/tests.rs` around lines 358 - 367, The test fixtures set OpState::Withdrawing with request_id equal to op_id which can hide bugs; add at least one end-to-end regression case where you call c.set_op_state(OpState::Withdrawing(WithdrawingState { op_id: <A>, request_id: <B>, ... })) with A != B (use the same fields shown: remaining, collected, receiver, owner, escrow_shares) and then run the full flow and assertions to ensure cleanup, dequeuing, and refunds are performed against request_id B (not op_id A); update the test assertions to explicitly check that the queued request with id B is removed, refunds/withdrawals and account deletion/authorization behave correctly, and that no double-execution or replay occurs for op_id A.contract/vault/soroban/src/storage/mod.rs (1)
793-845:⚠️ Potential issue | 🔴 CriticalKeep reading the old storage format until migration rewrites it.
load_state()andload_policy_state()now assume the new raw encodings, but the Sorobanmigrate()path still only handles the legacy paused flag. Existing deployments will keep their oldStateBlob/ policy bytes and start failing reads unless this release either decodes both formats or rewrites the stored blobs during migration.As per coding guidelines:
contract/vault/soroban/src/storage/**/*.rs: Changes to postcard serialization, versioning, migration, or storage keys are security-relevant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/soroban/src/storage/mod.rs` around lines 793 - 845, load_state() and load_policy_state() currently assume only the new raw encodings and will break for existing deployments using legacy blobs; update them to attempt legacy decoding when the new decode fails and ensure migrate() rewrites legacy blobs to the new format. Specifically, in load_state() use the existing load_state_blob() result and first try decode_state_blob(&stored) and on Err attempt the legacy_state_decode function/path (or the older decode_state_blob_v0) and return that result, and in load_policy_state() for each loader (load_policy_locks, load_policy_supply_queue, load_policy_markets, load_policy_principals, load_policy_cap_groups) try the new decode_* first and if it errors fall back to the legacy decode_* variant; additionally update the migrate() implementation to detect legacy paused/state/policy blobs and re-encode and save them using encode_state_blob / encode_* functions so stored blobs are rewritten to the new format during migration.contract/vault/soroban/STRIDE.md (2)
299-302:⚠️ Potential issue | 🟡 MinorUpdate this checklist bullet to
GovernanceActionKindterminology.Line 301 still says abdication uses “canonical method name mapping”, but the rest of the document has already moved to kind-based abdication. Leaving the old term here is misleading for operators and reviewers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/soroban/STRIDE.md` around lines 299 - 302, Update the checklist bullet that currently describes abdication as using “canonical method name mapping” to use the kind-based terminology used elsewhere: replace that phrase with “GovernanceActionKind-based mapping” (or similar explicit reference to GovernanceActionKind) so the sentence reads that abdicate() permanently disables actions via GovernanceActionKind mapping; ensure the term GovernanceActionKind and the method name abdicate() are used for consistency with the rest of the document.
153-246:⚠️ Potential issue | 🟡 MinorRepair the broken Markdown table rows.
Several threat/remediation cells now span raw newlines inside the table, which breaks rendering and matches the MD055/MD056 warnings in static analysis. Keep each row on one line, or use
<br>/ HTML blocks inside a single cell.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/soroban/STRIDE.md` around lines 153 - 246, The table rows in the STRIDE.md threats/remediations table contain raw newlines inside cells (e.g., under Spoofing/Tampering sections and remediation entries like Spoof.1.R.1, Tamper.7.R.1, DoS.2.R.3, etc.), which breaks Markdown rendering; fix by collapsing each table row to a single line or replacing internal newlines with HTML line breaks (<br>) or an HTML block so each cell remains a single continuous cell value, ensuring entries like Spoof.1–Spoof.3, Tamper.1–Tamper.9, Repudiate.*, Info.*, DoS.*, and the Remediation rows (e.g., Spoof.1.R.1, Tamper.7.R.1, DoS.2.R.3) render correctly.contract/vault/soroban/src/contract/curator_vault.rs (1)
800-839:⚠️ Potential issue | 🟠 MajorReject partial or duplicate refresh reports before syncing assets.
This validation only checks that each reported market appears somewhere in the refresh plan. A callback can omit one planned market or report the same market twice, and
complete_refresh_with_positions()will still recomputeexternal_assetsand finish the op using stale principals for the missing entry. That corrupts the runtime’s canonical external-asset accounting for the completed refresh.Suggested fix
fn validate_refreshed_positions_against_plan( &self, refreshed_positions: &[(TargetId, u128)], ) -> Result<(), RuntimeError> { let refreshing = self .state()? .op_state .as_refreshing() .ok_or_else(|| invalid_state_error(""))?; + + if refreshed_positions.len() != refreshing.plan.len() { + return Err(RuntimeError::invalid_input("")); + } + + let mut seen = Vec::with_capacity(refreshed_positions.len()); for (market, _) in refreshed_positions { - if !refreshing.plan.contains(market) { + if !refreshing.plan.contains(market) || seen.contains(market) { return Err(RuntimeError::invalid_input("")); } + seen.push(*market); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/soroban/src/contract/curator_vault.rs` around lines 800 - 839, The current validation in validate_refreshed_positions_against_plan only checks membership and allows missing or duplicate reports; change it to reject partial or duplicate reports by comparing the set of reported markets to the expected plan set: in validate_refreshed_positions_against_plan (used by complete_refresh_with_positions) build a deduplicated set of markets from the classified refreshed_positions (ensure you call or accept the output of classify_refreshed_positions), ensure its size equals the plan size and that every plan market is present (and no extras), and return Err(RuntimeError::invalid_input("")) when they differ so apply_refreshed_positions and sync_external_assets only run for complete, non-duplicated reports.contract/vault/kernel/src/actions/mod.rs (1)
1743-1772:⚠️ Potential issue | 🔴 CriticalValidate the
request_idbefore planning an idle payout.This path still only matches the queue head by owner/receiver/escrow. If the head changes to a different request with the same tuple, the kernel will price and settle the wrong withdrawal. That defeats the new queue-head identity tracking added elsewhere in this PR.
Suggested fix
- let (request_owner, request_receiver, request_escrow, request_expected) = state + let (request_id, request_owner, request_receiver, request_escrow, request_expected) = state .withdraw_queue .head() - .map(|(_, request)| { + .map(|(id, request)| { ( + id, request.owner, request.receiver, request.escrow_shares, request.expected_assets, ) @@ - if request_owner != withdrawing.owner + if request_id != withdrawing.request_id + || request_owner != withdrawing.owner || request_receiver != withdrawing.receiver || request_escrow != withdrawing.escrow_shares { return Err(KernelError::from( InvalidStateCode::WithdrawalQueueHeadMismatch,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/kernel/src/actions/mod.rs` around lines 1743 - 1772, The code currently verifies the queue head by owner/receiver/escrow but must also validate the queue entry's unique request_id to prevent mismatching identical tuples; update the head extraction to capture request.request_id (e.g., let request_id = request.request_id) and include a comparison against the in-flight OpState::Withdrawing record (e.g., withdrawing.request_id) alongside owner/receiver/escrow, returning KernelError::from(InvalidStateCode::WithdrawalQueueHeadMismatch) if the request_ids differ so the idle payout only proceeds for the exact queued request.contract/vault/curator-primitives/src/governance/mod.rs (1)
261-317:⚠️ Potential issue | 🟠 MajorCanonicalize restriction member order before storing them in
Vec.
normalize_members()only removes duplicates; it does not impose a stable order. That means[A, B]and[B, A]are still differentRestrictionsvalues and serialize differently even though the policy is identical. In practice this makes no-change detection, pending replacement, and bridged governance payloads order-sensitive.As per coding guidelines:
**/*.rs: Keep serialization explicit for contract-facing and event-facing structs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/curator-primitives/src/governance/mod.rs` around lines 261 - 317, normalize_members currently only deduplicates but leaves member order unstable; change it to produce a canonical, deterministic order by sorting members before deduplication (e.g., sort then dedup). To do this, update the generic bounds where needed (e.g., normalize_members and the Restrictions impls that call it such as Restrictions::blacklist, ::whitelist, and ::normalized) to require T: Ord in addition to PartialEq (or otherwise provide a consistent comparator), then implement normalize_members to sort the input Vec<T> and remove adjacent duplicates so [A,B] and [B,A] serialize identically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/src/vault/mod.rs`:
- Line 22: The remaining imports still reference templar_vault_kernel instead of
the extracted wad module; update all consumers to import the exported symbols
(Wad, Number, WIDE, fee constants and helper functions) from the new module path
(crate::vault::wad for internal crates or templar_common::vault::wad for
external crates) instead of templar_vault_kernel::wad; locate usages of Wad,
Number, WIDE, any fee constants or helpers and change their use/import
statements to the new module path and adjust any path-qualified references
accordingly.
In `@contract/vault/curator-primitives/src/policy/cooldown/mod.rs`:
- Around line 17-23: The new field types in Cooldown (last_event_ns:
Option<u64>, interval_ns: Option<NonZeroU64>) change the on-wire encoding and
will break decoding of previously persisted Cooldown/RefreshThrottle/lease
values; revert the external wire format by implementing explicit serialization
for Cooldown that preserves the old layout (serialize last_event_ns as u64 with
0 meaning None and serialize interval_ns as u64 with 0 meaning unlimited,
otherwise the nonzero value), and apply the same custom (de)serialization for
any wrapper types like RefreshThrottle/lease state that embed Cooldown so
existing stored blobs decode correctly; locate the Cooldown struct and add
custom borsh/serde/postcard impls (or manual encode/decode helpers) to
marshal/unmarshal using the legacy u64-0 conventions.
In `@contract/vault/curator-primitives/src/policy/market_lock/mod.rs`:
- Around line 133-140: from_parts currently trusts the caller-supplied
next_fencing_token which can be lower than existing lease tokens and break
monotonicity; change from_parts to scan leases_by_target (e.g. iterate
MarketLease entries and read each lease.fencing_token) to compute the maximum
fencing token present and then set next_fencing_token =
max(supplied_next_fencing_token, max_present + 1) (or return an error if you
prefer validation) before constructing Self so try_acquire and
assert_token_current cannot regress; update the logic in from_parts and any
tests that construct registry state via from_parts accordingly.
In `@contract/vault/curator-primitives/src/policy/state/mod.rs`:
- Around line 534-550: The current From<PolicyStateError> for CapGroupError
conflates many non-cap-group errors into InconsistentRecord with
CapGroupId::policy_state_sentinel(), losing context; instead add a dedicated
CapGroupError variant (e.g., PolicyState { source: PolicyStateError } or
PolicyStateError(Box<PolicyStateError>)) and change the From implementation to
map the unrelated PolicyStateError arms (InvalidSupplyQueue,
SupplyQueueUnknownMarket, SupplyQueueDisabledMarket,
SupplyQueueUnauthorizedMarket, PrincipalOverflow, UnknownMarket) to that new
variant; update any match sites or error conversions that expect the old
sentinel behavior to handle the new variant (or keep the
UnknownCapGroup/CapGroupInUse mappings as-is).
In `@contract/vault/curator-primitives/src/policy/supply_queue/mod.rs`:
- Around line 191-201: The dequeue method can underflow when doing self.len -=
1; update SupplyQueue::dequeue to defensively decrement using checked/saturating
arithmetic (e.g., self.len = self.len.saturating_sub(1)) or perform a guard that
returns SupplyQueueError on inconsistency before subtracting; locate the logic
in the dequeue function in mod.rs where buckets.iter_mut().rev() is used, adjust
the decrement to use saturating_sub (or an explicit check) so corrupted state
cannot cause an underflow.
- Around line 245-253: In excluding_leased, avoid calling unwrap() on
push_validated_entry; instead replace it with a defensive expect that documents
the invariant (e.g., push_validated_entry(...).expect("push_validated_entry
cannot overflow: filtering from an existing queue with same max_length")), so
the failure message explains why overflow is impossible for future debugging;
reference: excluding_leased and push_validated_entry.
In `@contract/vault/curator-primitives/src/recovery/mod.rs`:
- Around line 416-428: The two no-op bindings in abort_allocating_action and
abort_withdrawing_action (let _ = state.remaining; and let _ =
state.escrow_shares;) are unclear; update those functions to include a one-line
comment clarifying intent (e.g., "// ensure field exists / reserved for future
logging or metrics" or "// keep field referenced to avoid unused-field warning")
next to the bindings so readers know these are deliberate compile-time
references to state.remaining and state.escrow_shares; leave
abort_refreshing_action as-is or add a similar comment if consistency is
desired.
In `@contract/vault/curator-primitives/tests/integration_tests.rs`:
- Around line 32-35: The test calls compute_payout_failure_outcome with
duplicate restore_idle arguments; change the second argument to use the intended
payout amount variable refund_shares and remove the unused placeholder
assignment. Specifically, in the test replace the call
compute_payout_failure_outcome(escrow_shares, restore_idle, restore_idle) with
compute_payout_failure_outcome(escrow_shares, refund_shares, restore_idle) and
delete the line let _ = refund_shares; so refund_shares is used directly.
In `@contract/vault/kernel/src/actions/mod.rs`:
- Around line 1731-1735: The centralized pause helper is ignoring the runtime
restriction payload; update is_globally_paused to return true when either
config.paused is true or the provided Restrictions indicates a global pause
(i.e., honor the Restrictions::Paused variant) instead of discarding the
restrictions argument (remove the placeholder let _ = restrictions). Concretely,
in is_globally_paused(&VaultConfig, Option<&Restrictions>) check the Option and
return true if matches!(restrictions, Some(Restrictions::Paused)) ||
config.paused so handle_execute_withdraw and handle_finish_allocating respect
governance pauses.
In `@contract/vault/kernel/src/fee/mod.rs`:
- Around line 281-333: The custom Serialize/Deserialize impls in module
postcard_serde_impl (for FeeSlot and FeesSpec) currently force tuple/compact
encoding for all serializers; update the implementations for FeeSlot and
FeesSpec (the Serialize and Deserialize impl blocks) to branch on
serializer.is_human_readable() / Deserializer::is_human_readable() so that
human-readable formats use the explicit struct-like representation ({fee_wad,
recipient} and {performance, management, max_total_assets_growth_rate}) while
only non-human-readable serializers (postcard) use the compact tuple encoding,
or alternatively remove these impls and provide a separate wrapper type that
enforces postcard-only compact encoding; ensure both Serialize and Deserialize
use the same branching logic so the ABI for contract storage (VaultConfig ->
FeesSpec) remains stable.
In `@contract/vault/kernel/src/state/escrow/mod.rs`:
- Around line 107-125: Add a guard for expected_assets == 0 before the
proportional/burn branches: when expected_assets is zero, return
EscrowSettlement::refund_all(shares) (the same treatment as actual_assets == 0)
so we don't trigger the actual_assets >= expected_assets burn path; modify the
logic around actual_assets, expected_assets, and shares in the escrow settlement
code (the block that currently computes to_burn using Number::mul_div_ceil and
then refund = shares.saturating_sub(to_burn)) to first check expected_assets ==
0 and refund all.
In `@contract/vault/kernel/src/state/queue/mod.rs`:
- Around line 531-540: The from_sorted_entries function (and any callers like
with_state) currently accepts arbitrary order and can produce an invalid
PendingWithdrawals that breaks PendingWithdrawals::locate; restore the invariant
by either (a) reverting to the original FromIterator construction path that
enforces ordering/uniqueness, or (b) add a defensive check in
from_sorted_entries that iterates the incoming Vec<(u64, PendingWithdrawal)>,
verifies ids are strictly increasing and unique, and only then constructs
entries as PendingWithdrawalEntry { id, withdrawal } (otherwise return/handle an
error or panic); reference from_sorted_entries, with_state,
PendingWithdrawals::locate, PendingWithdrawalEntry, and FromIterator when making
the change.
In `@contract/vault/near/src/governance.rs`:
- Around line 1232-1235: The event currently serializes a cleared relative cap
(new_relative_cap == None) as U128(Wad::one()), which incorrectly signals a 100%
cap; update Event::CapGroupRelativeCapSet (and the analogous case at lines
~1339-1342) to carry the relative cap as an Option<U128> and emit None when
new_relative_cap is None instead of mapping to Wad::one(), or otherwise preserve
the Option semantics (i.e., map Some(cap) -> Some(U128(...)) and None -> None)
so consumers can distinguish “cleared” from an explicit 1.0 value; adjust the
Event enum variant and any code constructing or serializing that variant (e.g.,
where cap_group and new_relative_cap are used) accordingly.
In `@contract/vault/near/src/impl_callbacks.rs`:
- Around line 552-556: The rebalance callbacks currently call
or_stop::<AllocatingSpec>(self, op_id) and run
market_execution_lock.assert_current_token(market_id, FencingToken(...)) after
checking op state, which allows stale callbacks to stop or panic; change the
ordering in the affected functions (those using or_stop::<AllocatingSpec>,
AllocatingSpec, market_execution_lock.assert_current_token and FencingToken) to
first perform the lease/token lookup and call
market_execution_lock.assert_current_token(market_id, FencingToken(...)) (or
validate presence of the lease/token) and if the lease/token is missing or stale
return early as a no-op, only then call or_stop::<AllocatingSpec>(self, op_id)
and proceed, and apply the same fix to the other affected blocks around the
indicated code regions (the similar logic at the later callbacks).
- Around line 234-238: The current use of env::block_timestamp() as the lease
token in market_execution_lock.lock(market, op_id,
u64::MAX.saturating_sub(env::block_timestamp())) is unsafe because receipts in
the same NEAR block share the same timestamp; replace this with a strictly
monotonic lease counter or make token validation include op_id so callbacks must
match both op_id and token. Concretely: add a monotonic counter field (e.g.,
market_execution_lock.next_lease_token or similar) and increment it when
acquiring a lock and pass that counter as the token to lock(...), and/or update
is_current_token(...) (and any callback validation code) to require the tuple
(op_id, token) to match the currently stored (op_id, lease_token), ensuring
stale callbacks from the same block cannot be mistaken for current leases.
- Around line 282-289: The withdraw callback must also treat missing leases as
stale: update the guard in impl_callbacks.rs around the calls to
market_execution_lock.has_active_lease and
market_execution_lock.is_current_token so that if
!market_execution_lock.has_active_lease(market) you return early
(PromiseOrValue::Value(())) instead of falling through to
withdraw_ctx_and_market_or_exit; do the same pattern for the other callback
sites referenced (the blocks around lines 338-345 and 418-425) so stale
callbacks never call withdraw_ctx_and_market_or_exit (which can trigger
stop_and_exit on the current op_state).
In `@contract/vault/near/src/lib.rs`:
- Around line 704-721: The branch that handles plan.is_empty should not advance
cooldown/operation counters for a no-op refresh: remove updating
idle.last_refresh_ns and removing incrementing idle.next_op_id in the
plan.is_empty branch (while still emitting RefreshStarted/RefreshCompleted and
returning the report). Locate the block around plan.is_empty,
idle.build_real_assets_report, idle.last_refresh_ns, and idle.next_op_id and
change it so a no-op (empty plan) emits events and returns the report but does
NOT modify last_refresh_ns or call saturating_add on next_op_id.
- Around line 867-870: The call to determine_recovery_action(&kernel_state,
&context, &progress, payout_evidence).unwrap_or(None) swallows errors by turning
any Err into None; replace this swallowing with proper error propagation so
unbrick() surfaces planner failures: remove unwrap_or(None) and handle the
Result from determine_recovery_action directly (e.g. use ? to return the Err
from unbrick or match Result to log/return the error), then continue to match
the inner Option to decide “no action” vs a concrete action; reference
determine_recovery_action, unbrick, kernel_state, context, progress, and
payout_evidence when making the change.
In `@contract/vault/near/src/policy.rs`:
- Around line 167-180: from_markets currently fabricates op_id/token ownership
(via MarketLeaseRegistry::default and acquire_lease_or_panic) which creates
synthetic leases that resumed ops cannot release; change from_markets to refuse
migration when only legacy locks are present by making it return Result<Self,
MigrationError> (or panic earlier if that pattern is preferred), detect
legacy-only entries before calling acquire_lease_or_panic, and return an
explicit error describing that legacy locks must carry op_id/token metadata;
alternatively, if you must migrate, preserve real ownership by accepting and
propagating the original op_id and fencing token into MarketLeaseRegistry (i.e.,
change the function signature to accept ownership tuples and use those to
construct registry instead of inventing values in from_markets and avoid using
acquire_lease_or_panic to fabricate owners).
In `@contract/vault/near/src/tests.rs`:
- Around line 5164-5221: The test currently only verifies that unbrick() returns
a Promise but never executes the payout callback; update the test to drive the
async callback to completion by simulating a resolved promise and invoking the
contract's reconciliation callback (stop_and_exit_payout_01_reconcile or the
internal method that handles Promise results) with a successful PromiseResult,
using the same op_id/request_id and context set up earlier (OpState::Payout,
PendingWithdrawal, escrow values), then assert final invariants: balances (vault
and owner), total_supply, pending_withdrawals_len,
withdraw_queue.next_withdraw_to_execute, and that op_state is cleared/updated as
expected; ensure you use the existing symbols unbrick,
stop_and_exit_payout_01_reconcile, PendingWithdrawal, OpState::Payout, and
withdraw_queue.next_withdraw_to_execute to locate the code to modify.
In `@contract/vault/soroban/AGENTS.md`:
- Around line 288-289: The PR updated the agent guide but not the operator docs;
update contract/vault/soroban/README.md and contract/vault/soroban/STRIDE.md to
reflect the new governance bridge flow (matching the changes made in AGENTS.md),
and ensure README.md, STRIDE.md, SIZE_BUDGET.md and the Soroban Vault Agent
Guide remain consistent; specifically, propagate the same wording and behavioral
details about governance bridging, withdrawal lifecycle, and share-token auth
into the README and STRIDE files so all operator docs and the threat model are
aligned with the agent guide changes.
In `@contract/vault/soroban/governance/src/lib.rs`:
- Around line 781-788: The SetGroupMember branch incorrectly passes None as the
current membership when building the TimelockDecision, which makes any clear
request appear as None->None; update the GovernanceAction::SetGroupMember
handling to read the real current membership for the market (instead of using
None) and pass that as the first argument to
TimelockDecision::from_membership_assignment_change::<SdkString>, or add an
explicit clear-path that loads current membership and compares current
Some(value) -> proposed None so removals are detected; reference
GovernanceAction::SetGroupMember, cap_group_id, and
TimelockDecision::from_membership_assignment_change::<SdkString> when locating
and changing the code.
In `@contract/vault/soroban/governance/src/types.rs`:
- Around line 160-174: The restriction-mode value “1” must not be recycled;
update the enum handling so that from_u32 and any consumers (notably
submit_set_restrictions) explicitly reject the legacy value instead of remapping
it: add a reserved/deprecated branch or return a clear error (e.g.,
GovernanceError::DeprecatedValue or InvalidInput) when value == 1 in from_u32,
update as_u32 only for active variants, and add validation in
submit_set_restrictions to fail fast on incoming raw u32 == 1 so legacy messages
are rejected during the migration window.
- Line 21: DataKey::Abdicated was changed from storing a legacy Symbol key to
storing a GovernanceActionKind, which alters on-chain key encoding and can make
previously-written abdications unreadable; update the contract to preserve
legacy abdication keys by implementing a dual-read path or a migration: when
reading abdication state (lookup functions that reference DataKey::Abdicated,
e.g., any get_abdicated()/is_abdicated()/load_abdication() functions), first
attempt to read using the new GovernanceActionKind-typed key and if not found
fall back to the legacy Symbol-encoded key, then if fallback succeeds convert
and persist the value under the new key (or perform a one-time migration during
upgrade that enumerates legacy Symbol keys and writes equivalent
GovernanceActionKind keys); ensure all write paths still write the new
GovernanceActionKind key, add a clear comment referencing the legacy Symbol
format, and add unit/integration tests that simulate pre-upgrade Symbol entries
to validate the dual-read/migration behavior.
In `@contract/vault/soroban/justfile`:
- Line 44: The remap_rustflags value is not being applied to the optimizer
rebuilds and wasm analysis steps: update the invocations in the _optimize-wasm
and wasm-analyze tasks so the same remap_rustflags variable is exported/passed
into the build step that runs "stellar contract build --optimize" (i.e., ensure
the optimizer rebuild uses the remap path prefixes when invoking the Cargo
builds underlying stellar), and change the check-4626-proxy step to run "cargo
check --target wasm32-unknown-unknown" (with remap_rustflags applied) so the
proxy crate is validated for the wasm32 target; reference the remap_rustflags
variable and the tasks/functions named _optimize-wasm, wasm-analyze, and
check-4626-proxy when making these updates.
In `@contract/vault/soroban/README.md`:
- Around line 15-20: The README describes a new governance bridge/control-plane
split that must be mirrored across the other Soroban docs; update STRIDE.md and
SIZE_BUDGET.md (and the Soroban vault agent guide) to reflect the governance
timelock/orchestration moving to contract/vault/soroban/governance and the
runtime applying canonical governance state changes via execute_governance(env,
caller, payload) carrying GovernanceCommand, noting that SetTimelock and Other
remain local to the governance contract and that the generic execute(payload)
path is retained for user flows and the
ALLOCATORS/ALLOWED_ADAPTERS/VIRTUAL_OFFSETS subset; ensure threat model,
operational guidance, and any references to control-plane vs.
governance-boundary are consistent with README wording.
In `@contract/vault/soroban/src/contract/entrypoints.rs`:
- Around line 806-836: The initialize() entrypoint is unauthenticated and can be
front-run; fix it by enforcing a caller check at the start: add a
deployer/initializer Address parameter (or otherwise require the intended
authority) and assert env.invoker() (or the appropriate Soroban caller identity)
equals that address before checking VaultDataKey::Initialized or writing any
config; update the initialize signature and add a guard (e.g.
require_invoker_equals_deployer) before calling set_config_address,
store_virtual_offsets, runtime_to_contract(store_fees_spec), or any storage
writes so only the intended deployer/authority can perform initialization.
In `@contract/vault/soroban/src/contract/helpers.rs`:
- Around line 120-130: The current decoder for max_total_assets_growth_rate
accepts a presence flag of 0 while ignoring trailing bytes, allowing malformed
FeesSpec blobs; update the decoding logic in the function that reads from
bytes/cursor (the block handling max_total_assets_growth_rate) to validate the
expected total length: when the flag is 0, assert that no extra bytes remain
(cursor must equal bytes.len()) and when the flag is 1, advance the cursor by
reading the u128 via read_u128 and then assert the cursor equals bytes.len(); on
any mismatch return a RuntimeError::storage_error so inconsistent or oversized
encodings are rejected.
- Around line 22-27: address_from_alloc_string currently calls the panicking
Address::from_str and wraps it in Ok, which hides panics; replace that with a
fallible parse and map parse failures to ContractError::InvalidInput (i.e., call
the SDK's fallible parser for SdkAddress—for example
SdkAddress::from_str_checked / try_from_str / try_into if available—or otherwise
use a provided validation function) and return Err(ContractError::InvalidInput)
on bad input; update address_from_alloc_string to propagate that Result rather
than wrapping a panic-prone call.
In `@contract/vault/soroban/src/effects/mod.rs`:
- Around line 46-48: The kernel event payload format changed from postcard bytes
but encode_kernel_event still emits the same unversioned `kernel` topic and its
docstring claims postcard; update encode_kernel_event (and any similar emitter
such as publish_kernel_event / the code around the other kernel-event encoding
at the other location) to prepend a small version header (e.g., a single byte or
fixed header) to the payload before returning so downstream indexers can detect
the format, and update the docstrings to document the new versioned payload
format; ensure the version value is a constant symbol (e.g.,
KERNEL_EVENT_VERSION) so future changes are easy to detect and maintain backward
compatibility.
In `@contract/vault/soroban/src/storage/mod.rs`:
- Around line 154-167: decode_restrictions currently parses the tag, length and
addresses but ignores any trailing bytes, allowing mixed-version/corrupted blobs
to be accepted; modify decode_restrictions (and the other similar decoders
noted) to validate that the read cursor exactly equals bytes.len() after all
expected fields are consumed and return a storage error (e.g.,
RuntimeError::storage_error with a descriptive message) if there are leftover
bytes so the function fails closed on trailing data.
---
Outside diff comments:
In `@contract/vault/curator-primitives/src/governance/mod.rs`:
- Around line 261-317: normalize_members currently only deduplicates but leaves
member order unstable; change it to produce a canonical, deterministic order by
sorting members before deduplication (e.g., sort then dedup). To do this, update
the generic bounds where needed (e.g., normalize_members and the Restrictions
impls that call it such as Restrictions::blacklist, ::whitelist, and
::normalized) to require T: Ord in addition to PartialEq (or otherwise provide a
consistent comparator), then implement normalize_members to sort the input
Vec<T> and remove adjacent duplicates so [A,B] and [B,A] serialize identically.
In `@contract/vault/kernel/src/actions/mod.rs`:
- Around line 1743-1772: The code currently verifies the queue head by
owner/receiver/escrow but must also validate the queue entry's unique request_id
to prevent mismatching identical tuples; update the head extraction to capture
request.request_id (e.g., let request_id = request.request_id) and include a
comparison against the in-flight OpState::Withdrawing record (e.g.,
withdrawing.request_id) alongside owner/receiver/escrow, returning
KernelError::from(InvalidStateCode::WithdrawalQueueHeadMismatch) if the
request_ids differ so the idle payout only proceeds for the exact queued
request.
In `@contract/vault/near/src/tests.rs`:
- Around line 358-367: The test fixtures set OpState::Withdrawing with
request_id equal to op_id which can hide bugs; add at least one end-to-end
regression case where you call
c.set_op_state(OpState::Withdrawing(WithdrawingState { op_id: <A>, request_id:
<B>, ... })) with A != B (use the same fields shown: remaining, collected,
receiver, owner, escrow_shares) and then run the full flow and assertions to
ensure cleanup, dequeuing, and refunds are performed against request_id B (not
op_id A); update the test assertions to explicitly check that the queued request
with id B is removed, refunds/withdrawals and account deletion/authorization
behave correctly, and that no double-execution or replay occurs for op_id A.
In `@contract/vault/README.md`:
- Around line 158-173: The README mermaid authorization diagram is missing the
transitive edge showing Curator's implicit authority over Sentinel actions;
update the diagram block to include a "Curator --> Sentinel" edge (or add a
short caption stating that is_curator_or() lets Curator act as any delegated
role), referencing the Curator and Sentinel roles and the is_curator_or()
fallback in auth/mod.rs so the docs match the implementation.
In `@contract/vault/soroban/governance/src/lib.rs`:
- Around line 582-589: The GovernanceActionKind enum conflates multiple
different GovernanceAction variants (e.g., SetGroupCap, SetGroupRelCap,
SetGroupMember into CapGroup and SetSkimRecipient/Skim into Skim), which makes
accept_kind, revoke_kind and irreversible abdication ambiguous; modify
GovernanceActionKind to introduce distinct variants for each concrete action
(for example CapGroupSet, CapGroupRelSet, CapGroupMemberSet, SkimSetRecipient,
SkimExecute or similarly named per-action kinds), update the match mapping in
lib.rs that maps GovernanceAction -> GovernanceActionKind to return these new
fine-grained kinds, and then update all callers of accept_kind, revoke_kind and
any abdication logic to use the new per-action kinds so that
execution/revocation/abdication remain precise and unambiguous (reference
symbols: GovernanceAction, GovernanceActionKind, accept_kind, revoke_kind,
abdication).
In `@contract/vault/soroban/src/contract/curator_vault.rs`:
- Around line 800-839: The current validation in
validate_refreshed_positions_against_plan only checks membership and allows
missing or duplicate reports; change it to reject partial or duplicate reports
by comparing the set of reported markets to the expected plan set: in
validate_refreshed_positions_against_plan (used by
complete_refresh_with_positions) build a deduplicated set of markets from the
classified refreshed_positions (ensure you call or accept the output of
classify_refreshed_positions), ensure its size equals the plan size and that
every plan market is present (and no extras), and return
Err(RuntimeError::invalid_input("")) when they differ so
apply_refreshed_positions and sync_external_assets only run for complete,
non-duplicated reports.
In `@contract/vault/soroban/src/storage/mod.rs`:
- Around line 793-845: load_state() and load_policy_state() currently assume
only the new raw encodings and will break for existing deployments using legacy
blobs; update them to attempt legacy decoding when the new decode fails and
ensure migrate() rewrites legacy blobs to the new format. Specifically, in
load_state() use the existing load_state_blob() result and first try
decode_state_blob(&stored) and on Err attempt the legacy_state_decode
function/path (or the older decode_state_blob_v0) and return that result, and in
load_policy_state() for each loader (load_policy_locks,
load_policy_supply_queue, load_policy_markets, load_policy_principals,
load_policy_cap_groups) try the new decode_* first and if it errors fall back to
the legacy decode_* variant; additionally update the migrate() implementation to
detect legacy paused/state/policy blobs and re-encode and save them using
encode_state_blob / encode_* functions so stored blobs are rewritten to the new
format during migration.
In `@contract/vault/soroban/STRIDE.md`:
- Around line 299-302: Update the checklist bullet that currently describes
abdication as using “canonical method name mapping” to use the kind-based
terminology used elsewhere: replace that phrase with “GovernanceActionKind-based
mapping” (or similar explicit reference to GovernanceActionKind) so the sentence
reads that abdicate() permanently disables actions via GovernanceActionKind
mapping; ensure the term GovernanceActionKind and the method name abdicate() are
used for consistency with the rest of the document.
- Around line 153-246: The table rows in the STRIDE.md threats/remediations
table contain raw newlines inside cells (e.g., under Spoofing/Tampering sections
and remediation entries like Spoof.1.R.1, Tamper.7.R.1, DoS.2.R.3, etc.), which
breaks Markdown rendering; fix by collapsing each table row to a single line or
replacing internal newlines with HTML line breaks (<br>) or an HTML block so
each cell remains a single continuous cell value, ensuring entries like
Spoof.1–Spoof.3, Tamper.1–Tamper.9, Repudiate.*, Info.*, DoS.*, and the
Remediation rows (e.g., Spoof.1.R.1, Tamper.7.R.1, DoS.2.R.3) render correctly.
🪄 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: cbf3b0be-48ea-47f7-be37-42c1d306bba6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (87)
AGENTS.mdCargo.tomlclient/vault/src/lib.rsclient/vault/src/methods.rscommon/src/vault/mod.rscommon/src/vault/restrictions.rscommon/src/vault/wad.rscontract/vault/README.mdcontract/vault/curator-primitives/src/auth/mod.rscontract/vault/curator-primitives/src/governance/mod.rscontract/vault/curator-primitives/src/lib.rscontract/vault/curator-primitives/src/policy/cap_group/mod.rscontract/vault/curator-primitives/src/policy/cap_group_adapter.rscontract/vault/curator-primitives/src/policy/cooldown/mod.rscontract/vault/curator-primitives/src/policy/lock_filter/mod.rscontract/vault/curator-primitives/src/policy/market_lock/mod.rscontract/vault/curator-primitives/src/policy/mod.rscontract/vault/curator-primitives/src/policy/refresh_plan/mod.rscontract/vault/curator-primitives/src/policy/state/mod.rscontract/vault/curator-primitives/src/policy/supply_queue/mod.rscontract/vault/curator-primitives/src/policy/target_set/mod.rscontract/vault/curator-primitives/src/policy/withdraw_route/mod.rscontract/vault/curator-primitives/src/rbac/mod.rscontract/vault/curator-primitives/src/recovery/mod.rscontract/vault/curator-primitives/src/tests.rscontract/vault/curator-primitives/tests/integration_tests.rscontract/vault/curator-primitives/tests/property_tests.rscontract/vault/curator-primitives/tests/regression_proof_equivalents.rscontract/vault/kernel/src/abort.rscontract/vault/kernel/src/actions/mod.rscontract/vault/kernel/src/actions/tests.rscontract/vault/kernel/src/fee/mod.rscontract/vault/kernel/src/fee/tests.rscontract/vault/kernel/src/lib.rscontract/vault/kernel/src/math/number.rscontract/vault/kernel/src/math/number/tests.rscontract/vault/kernel/src/math/wad.rscontract/vault/kernel/src/restrictions/mod.rscontract/vault/kernel/src/restrictions/tests.rscontract/vault/kernel/src/state/escrow/mod.rscontract/vault/kernel/src/state/escrow/tests.rscontract/vault/kernel/src/state/op_state/mod.rscontract/vault/kernel/src/state/op_state/tests.rscontract/vault/kernel/src/state/queue/mod.rscontract/vault/kernel/src/state/queue/tests.rscontract/vault/kernel/src/state/vault/mod.rscontract/vault/kernel/src/transitions/mod.rscontract/vault/kernel/src/transitions/tests.rscontract/vault/kernel/src/types/mod.rscontract/vault/kernel/src/types/tests.rscontract/vault/kernel/src/utils/mod.rscontract/vault/kernel/tests/property_tests.rscontract/vault/near/src/governance.rscontract/vault/near/src/impl_callbacks.rscontract/vault/near/src/kernel_mirror.rscontract/vault/near/src/lib.rscontract/vault/near/src/policy.rscontract/vault/near/src/tests.rscontract/vault/near/tests/governance.rscontract/vault/soroban/AGENTS.mdcontract/vault/soroban/README.mdcontract/vault/soroban/STRIDE.mdcontract/vault/soroban/governance/Cargo.tomlcontract/vault/soroban/governance/src/lib.rscontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/governance/src/types.rscontract/vault/soroban/justfilecontract/vault/soroban/shared-types/Cargo.tomlcontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/auth/mod.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/helpers.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/effects/mod.rscontract/vault/soroban/src/error/mod.rscontract/vault/soroban/src/fungible_vault.rscontract/vault/soroban/src/lib.rscontract/vault/soroban/src/storage/mod.rscontract/vault/soroban/src/test_utils.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/e2e_serialization.rscontract/vault/soroban/tests/integration_tests.rscontract/vault/soroban/tests/property_tests.rsfuzz/Cargo.tomlfuzz/fuzz_targets/fuzz_soroban_storage_codecs.rs
# Conflicts: # common/src/vault/mod.rs # common/src/vault/wad.rs # contract/vault/soroban/STRIDE.md # contract/vault/soroban/governance/src/lib.rs # contract/vault/soroban/governance/src/tests.rs # contract/vault/soroban/shared-types/src/lib.rs # contract/vault/soroban/src/contract/entrypoints.rs # contract/vault/soroban/src/tests.rs # contract/vault/soroban/tests/blend_e2e.rs # contract/vault/soroban/tests/integration_tests.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/src/vault/event.rs`:
- Around line 217-220: The CapGroupRelativeCapRaiseSubmitted event variant
changed its payload type for new_relative_cap to Option<U128> (nullable), so
bump the event schema/version identifier used for emitted events to a new
version so off-chain consumers can differentiate payload shapes; locate the
event definition and its associated version constant/serialization tag
(referencing CapGroupRelativeCapRaiseSubmitted and the other affected variants
around the same block) and increment the version string/number for this variant
(and the similar variants mentioned) so the serialized event version reflects
the schema change.
In `@contract/vault/near/src/tests.rs`:
- Around line 1548-1573: The test
stale_rebalance_callback_checks_lease_before_op_state currently passes because
op_id in the callback (20) differs from the live AllocatingState op_id (21), so
an implementation that returns early on op-state mismatch would still pass; to
fix, make the test actually exercise lease validation-before-op-state by
aligning the live AllocatingState op_id with the callback op_id (set
AllocatingState { op_id: 20, ... }) and then ensure the lease/token passed to
rebalance_withdraw_01_execute_withdraw_fetch_position is invalidated (or
expired) so the call should be rejected due to lease validation while leaving
op_state unchanged, or alternatively rename the test to indicate it only
verifies op-state preservation when the callback returns a value.
🪄 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: 05e1d315-22c5-4e8d-b5c0-ebdbcaaf52b9
📒 Files selected for processing (6)
common/src/vault/event.rscontract/vault/near/src/governance.rscontract/vault/near/src/impl_callbacks.rscontract/vault/near/src/lib.rscontract/vault/near/src/policy.rscontract/vault/near/src/tests.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contract/vault/curator-primitives/src/policy/supply_queue/mod.rs (1)
249-250:⚠️ Potential issue | 🟡 MinorUse a release-time invariant check instead of
debug_assert!inexcluding_leased.
debug_assert!is removed in release builds, so a failed insertion path is silent. Keep this fail-fast in all profiles.Suggested fix
- let inserted = filtered.push_validated_entry(entry.clone()); - debug_assert!(inserted.is_some()); + filtered.push_validated_entry(entry.clone()).expect( + "excluding_leased constructs a subset of an existing queue; length overflow is unreachable", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/curator-primitives/src/policy/supply_queue/mod.rs` around lines 249 - 250, The code in excluding_leased uses debug_assert! on the result of push_validated_entry, which vanishes in release builds; change this to a release-time invariant (e.g., assert! or an explicit panic) so a failed insertion is always fatal: in the excluding_leased logic, check the value returned by push_validated_entry(entry.clone()) (the inserted variable) and if it is None, panic with a clear message referencing the entry and the failure to insert, rather than using debug_assert!.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contract/vault/kernel/src/fee/tests.rs`:
- Around line 104-108: The test currently checks nested fields for "performance"
but only presence for "management"; update the assertions in
contract/vault/kernel/src/fee/tests.rs to mirror the coverage for "performance"
by asserting the expected nested keys on the json variable (e.g.,
assert!(json["management"].get("fee_wad").is_some());
assert!(json["management"].get("recipient").is_some()); or other expected
management subkeys), so the "management" object shape is validated as strictly
as "performance".
---
Duplicate comments:
In `@contract/vault/curator-primitives/src/policy/supply_queue/mod.rs`:
- Around line 249-250: The code in excluding_leased uses debug_assert! on the
result of push_validated_entry, which vanishes in release builds; change this to
a release-time invariant (e.g., assert! or an explicit panic) so a failed
insertion is always fatal: in the excluding_leased logic, check the value
returned by push_validated_entry(entry.clone()) (the inserted variable) and if
it is None, panic with a clear message referencing the entry and the failure to
insert, rather than using debug_assert!.
🪄 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: 4cfa2a85-f62d-4dda-a165-c0115429e3c0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
common/src/vault/event.rscontract/vault/curator-primitives/src/policy/market_lock/mod.rscontract/vault/curator-primitives/src/policy/supply_queue/mod.rscontract/vault/curator-primitives/src/recovery/mod.rscontract/vault/curator-primitives/src/tests.rscontract/vault/kernel/Cargo.tomlcontract/vault/kernel/src/fee/mod.rscontract/vault/kernel/src/fee/tests.rscontract/vault/kernel/src/state/escrow/mod.rscontract/vault/kernel/src/state/escrow/tests.rscontract/vault/kernel/src/state/queue/mod.rscontract/vault/kernel/src/state/queue/tests.rscontract/vault/near/src/tests.rs
💤 Files with no reviewable changes (1)
- contract/vault/curator-primitives/src/recovery/mod.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contract/vault/soroban/README.md`:
- Around line 180-194: Update the README to correct the artifact mappings: state
that wasm-path echoes {{default_wasm}} which is set to optimized_wasm (so
wasm-path returns the optimized artifact, i.e.,
templar_soroban_runtime.optimized.wasm), and that deploy-wasm-path returns the
contractspec-stripped deploy artifact (templar_soroban_runtime.deploy.wasm)
which is the one used by size-budget-check; mention default_wasm :=
optimized_wasm and explicitly advise operators to use deploy-wasm-path for
deployment/size verification and wasm-path for the optimized runtime artifact.
In `@contract/vault/soroban/STRIDE.md`:
- Around line 43-46: The STRIDE.md remediation is inaccurate: update it to state
that execute_governance decodes and dispatches the payload without an auth check
and that authorization is enforced inside each handler
(execute_governance_command dispatches to set_governance_config_impl,
set_governance_policy_impl, skim_impl, each of which calls require_governance as
their first action), note there is no require_auth wrapping execute_governance,
and add a caution that this deferred-auth pattern is fragile—document that
future GovernanceCommand variants must include their own require_governance
checks and be reviewed carefully to avoid missing authorization at the
entrypoint level.
🪄 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: 641e8e4a-df92-413a-82db-d6f107e22590
📒 Files selected for processing (4)
contract/vault/soroban/AGENTS.mdcontract/vault/soroban/README.mdcontract/vault/soroban/SIZE_BUDGET.mdcontract/vault/soroban/STRIDE.md
carrion256
force-pushed
the
spr/refactor/vault-ergonomics/4f330057
branch
2 times, most recently
from
April 28, 2026 13:45
d9c179e to
43a9e56
Compare
carrion256
force-pushed
the
spr/refactor/vault-ergonomics/4f330057
branch
from
April 28, 2026 13:48
43a9e56 to
559c13f
Compare
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack:
Auditor Review Map
This PR is large because it includes the Soroban vault/governance bridge plus review-followup fixes. To keep review focused, please prioritize these files/areas:
contract/vault/soroban/src/contract/entrypoints.rs,contract/vault/soroban/src/contract/helpers.rs,contract/vault/soroban/governance/src/lib.rs,contract/vault/soroban/governance/src/types.rsexecute_governance, timelock/no-change handling, current-state mirrors, abdication/kind mapping.contract/vault/soroban/src/storage/mod.rs,contract/vault/soroban/src/tests.rscontract/vault/soroban/src/effects/mod.rs,contract/vault/soroban/src/contract/helpers.rscontract/vault/kernel/src/state/escrow/mod.rs,contract/vault/kernel/src/state/queue/mod.rs,contract/vault/kernel/src/fee/mod.rscontract/vault/curator-primitives/src/policy/market_lock/mod.rs,contract/vault/curator-primitives/src/policy/supply_queue/mod.rs,contract/vault/curator-primitives/src/recovery/mod.rscontract/vault/near/src/impl_callbacks.rs,contract/vault/near/src/kernel_effects.rs,contract/vault/near/src/policy.rs,contract/vault/near/src/governance.rs.github/workflows/test.yml,contract/vault/soroban/justfile,contract/vault/soroban/SIZE_BUDGET.md,contract/vault/soroban/scripts/install-stellar-cli.shOut of scope for this audit pass: unrelated market math/accounting re-review. We intentionally removed lint-only common-market churn from this PR to avoid invalidating previous market audit work.
Tracker-backed stale findings on PR 417 base
The audit tracker pins this PR branch (
spr/refactor/vault-ergonomics/4f330057ata657f923e1095b247d6077a97c71b6e581630863) as the current remediation base/trunk. Against this base, two governance-control-plane findings are recorded as stale rather than requiring separate behavior changes:6ccf320a-505c-4b1b-8948-afbae91d726c— No production code path adds new marketsstalesubmit_set_cap; accepted cap proposals route throughexecute_governance; runtime cap policy creates missing market config on accepted governance cap.test_execute_governance_cap_creates_new_runtime_market_after_timelockb95a8642-0094-4121-92d1-0a491a6e541a— Vault rejects every matured cap increasestalesubmit_set_cap; immature accepts fail; matured accepts route to the vault; the governance mirror updates accepted cap state so later increases are not rejected as if no current cap exists.cap_action_is_timelocked_and_accepts_after_maturity;accepted_cap_updates_mirror_for_future_decisionsThese are stale proofs against the PR 417 remediation base, not claims that PR 417 is the per-finding remediation PR for every audit item. The remaining accepted findings are tracked in follow-up PRs stacked on this base.
This change is