feat(vault): migrate governance to compact Soroban bridge#414
feat(vault): migrate governance to compact Soroban bridge#414carrion256 wants to merge 1 commit into
Conversation
📝 WalkthroughGovernance Bridge RefactoringSummaryMigrates Soroban vault governance from nested Key ChangesCommand Separation
Abdication Migration
Governance Contract Refactoring
Test Coverage
Critical Security Findings
Recommended Manual Review
WalkthroughThe PR refactors governance control flow from multiple discrete setter endpoints to a unified bridge entrypoint. A new Changes
Sequence Diagram(s)sequenceDiagram
participant GovContract as Governance Contract
participant VaultContract as Vault Contract
participant Storage as Contract Storage
GovContract->>GovContract: Proposal timelock matures
GovContract->>GovContract: derive GovernanceCommand<br/>(SetGovernanceConfig,<br/>SetGovernancePolicy, or Skim)
GovContract->>VaultContract: execute_governance(self, payload)
VaultContract->>VaultContract: Decode GovernanceCommand
VaultContract->>VaultContract: Validate caller == governance
alt SetGovernanceConfig
VaultContract->>Storage: Update governance config<br/>(curator, governance addr, etc.)
else SetGovernancePolicy
VaultContract->>Storage: Update governance policy<br/>(fees, supply queue, restrictions, caps)
else Skim
VaultContract->>Storage: Execute skim operation
end
VaultContract-->>GovContract: Result<(), ContractError>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
595c43a to
09b12ca
Compare
0a549e4 to
6b6ccc2
Compare
09b12ca to
ec781b5
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/vault/soroban/justfile (1)
1127-1130:⚠️ Potential issue | 🟡 MinorRemove stale
set-curatorhelp entry.Line 1129 advertises
just set-curator <new_curator_address>, but that recipe no longer exists in this file. This leaves operators with a broken command.💡 Suggested doc cleanup
@@ `@echo` "ADMIN:" `@echo` " just pause / unpause" - `@echo` " just set-curator <new_curator_address>" `@echo` " just set-metadata [name] [symbol] [decimals]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/soroban/justfile` around lines 1127 - 1130, The help section in the justfile lists a non-existent recipe "set-curator <new_curator_address>" causing a broken operator command; update the ADMIN help block by removing the stale "just set-curator <new_curator_address>" line (or replace it with the correct recipe name if one exists) so the displayed commands match actual recipes (look for the ADMIN help echo lines that include "ADMIN:" and the following three echo statements).
🤖 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/governance/src/types.rs`:
- Line 21: DataKey::Abdicated changed from carrying a Symbol to
Abdicated(GovernanceActionKind), which breaks reads of existing on-chain keys;
add a migration or fallback: implement a one-time migration function (e.g.,
migrate_legacy_abdicated_keys) that scans storage for DataKey::Abdicated(Symbol)
entries, converts them to DataKey::Abdicated(GovernanceActionKind) and rewrites
storage, or alter accessors is_abdicated() and require_not_abdicated() to try
both key formats (first check Abdicated(GovernanceActionKind) then fall back to
the legacy Abdicated(Symbol) lookup and treat a legacy hit as abdicated) and
mark/upgrade the key when found; reference DataKey::Abdicated,
GovernanceActionKind, is_abdicated(), require_not_abdicated() when locating
where to add the migration or fallback logic.
In `@contract/vault/soroban/justfile`:
- Around line 136-143: The new recipe check-4626-proxy is not listed in the help
output; update the justfile's help recipe (the target that prints available
commands) to include an entry for check-4626-proxy with a short description like
"Check the Soroban ERC-4626 proxy crate" so operators see it; locate the
help-related recipe in the justfile and add a line referencing check-4626-proxy
alongside the other documented recipes (use the exact recipe name
check-4626-proxy to ensure it matches).
In `@contract/vault/soroban/README.md`:
- Around line 15-20: The README incorrectly states that retained-config keys
ALLOCATORS, ALLOWED_ADAPTERS, and VIRTUAL_OFFSETS are reachable via
execute(payload); update the text to remove that claim and instead point
integrators to the governance entrypoint execute_governance(env, caller,
payload) (and note that SetTimelock/Other remain local to the governance
contract) so all references to execute(payload) as a path for config/governance
mutations are deleted or replaced accordingly (also update the repeated text at
lines 76-78).
In `@contract/vault/soroban/src/tests.rs`:
- Around line 383-390: Two governance tests call
SorobanVaultContract::execute_governance directly under
mock_all_auths_allowing_non_root_auth(), which bypasses the contract-to-contract
bridge and only exercises the post-auth handler path; update those tests
(test_governance_config_updates_allowed_adapters_without_supply_queue_constraint
and test_governance_policy_group_membership_empty_string_clears_membership) to
perform the governance call via env.invoke_contract using the same storage_tests
helper pattern used by test_execute_governance_bridge_happy_path so the full
authorization/bridge semantics are executed (replace direct
SorobanVaultContract::execute_governance calls with env.invoke_contract
invocations that target the SorobanVault contract and use the same payload
construction and caller setup as the other bridge tests).
In `@contract/vault/soroban/STRIDE.md`:
- Around line 157-159: The table row containing "Spoof.3" (the multi-line cell
describing execute_governance) is missing leading/trailing pipe characters on
continuation lines; update the Markdown table so multi-line cell content is
either placed on a single line or each wrapped line includes the table cell
delimiters (pipes) to satisfy markdown table linting — locate the row
referencing "Spoof.3" and "execute_governance" and add the missing
leading/trailing pipes for each continuation line or collapse the description
into one line to restore valid table formatting.
---
Outside diff comments:
In `@contract/vault/soroban/justfile`:
- Around line 1127-1130: The help section in the justfile lists a non-existent
recipe "set-curator <new_curator_address>" causing a broken operator command;
update the ADMIN help block by removing the stale "just set-curator
<new_curator_address>" line (or replace it with the correct recipe name if one
exists) so the displayed commands match actual recipes (look for the ADMIN help
echo lines that include "ADMIN:" and the following three echo statements).
🪄 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: 32523839-7d7c-4edf-b1de-fe3171242b22
📒 Files selected for processing (11)
contract/vault/soroban/README.mdcontract/vault/soroban/STRIDE.mdcontract/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/src/lib.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rs
| CurrentRestrictionMode, | ||
| CurrentRestrictionAccounts, | ||
| Abdicated(Symbol), | ||
| Abdicated(GovernanceActionKind), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect abdication key usage and any migration/fallback logic"
rg -n -C 3 'Abdicated|abdicate|is_abdicated|migrat|legacy|Symbol' contract/vault/soroban/governance/srcRepository: Templar-Protocol/contracts
Length of output: 12301
🏁 Script executed:
cd contract/vault/soroban/governance/src && git log --oneline -20 types.rsRepository: Templar-Protocol/contracts
Length of output: 127
🏁 Script executed:
cd contract/vault/soroban/governance/src && git show HEAD:types.rs | head -30Repository: Templar-Protocol/contracts
Length of output: 253
🏁 Script executed:
cd contract/vault/soroban/governance/src && git diff HEAD~1 types.rs | grep -A 5 -B 5 Abdicated || echo "No diff found in last commit"Repository: Templar-Protocol/contracts
Length of output: 302
🏁 Script executed:
git show HEAD:contract/vault/soroban/governance/src/types.rs | head -40Repository: Templar-Protocol/contracts
Length of output: 843
🏁 Script executed:
git log --all --oneline contract/vault/soroban/governance/src/types.rs | head -5Repository: Templar-Protocol/contracts
Length of output: 211
🏁 Script executed:
git log --format="%H %s" | head -1Repository: Templar-Protocol/contracts
Length of output: 160
🏁 Script executed:
git show bb6f46e:contract/vault/soroban/governance/src/types.rs 2>/dev/null | grep -A 10 "enum DataKey" || echo "File did not exist in prior commit"Repository: Templar-Protocol/contracts
Length of output: 272
🏁 Script executed:
git diff bb6f46e ec781b5 -- contract/vault/soroban/governance/src/types.rs | grep -A 5 -B 5 "Abdicated"Repository: Templar-Protocol/contracts
Length of output: 333
🏁 Script executed:
git diff bb6f46e ec781b5 -- contract/vault/soroban/governance/src/lib.rs | grep -A 10 -B 10 "migrat\|legacy\|fallback" || echo "No migration logic found in diff"Repository: Templar-Protocol/contracts
Length of output: 102
🏁 Script executed:
grep -r "Abdicated(Symbol)" contract/vault/soroban/governance/src 2>/dev/null || echo "No legacy Symbol usage found"Repository: Templar-Protocol/contracts
Length of output: 98
🏁 Script executed:
cat contract/vault/soroban/governance/src/lib.rs | sed -n '1362,1373p'Repository: Templar-Protocol/contracts
Length of output: 423
Add migration or fallback logic for the Abdicated storage key type change.
DataKey::Abdicated(Symbol) changed to Abdicated(GovernanceActionKind) in the current commit. If existing on-chain abdication state uses the old Symbol-based key format, the new code will not retrieve those entries, causing is_abdicated() and require_not_abdicated() to return false. Previously abdicated actions would become executable again.
Add either:
- A migration function that rewrites legacy
Symbolkeys to the new enum format, or - A fallback in
is_abdicated()/require_not_abdicated()that checks both key formats
This must be in place before any deployment with existing state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/vault/soroban/governance/src/types.rs` at line 21,
DataKey::Abdicated changed from carrying a Symbol to
Abdicated(GovernanceActionKind), which breaks reads of existing on-chain keys;
add a migration or fallback: implement a one-time migration function (e.g.,
migrate_legacy_abdicated_keys) that scans storage for DataKey::Abdicated(Symbol)
entries, converts them to DataKey::Abdicated(GovernanceActionKind) and rewrites
storage, or alter accessors is_abdicated() and require_not_abdicated() to try
both key formats (first check Abdicated(GovernanceActionKind) then fall back to
the legacy Abdicated(Symbol) lookup and treat a legacy hit as abdicated) and
mark/upgrade the key when found; reference DataKey::Abdicated,
GovernanceActionKind, is_abdicated(), require_not_abdicated() when locating
where to add the migration or fallback logic.
| # Check the Soroban ERC-4626 proxy crate. | ||
| check-4626-proxy: | ||
| @echo "Checking Soroban ERC-4626 proxy crate..." | ||
| @RUSTFLAGS="${RUSTFLAGS:+$RUSTFLAGS }{{remap_rustflags}}" cargo check \ | ||
| --manifest-path "{{root}}/Cargo.toml" \ | ||
| -p templar-4626-proxy-soroban | ||
| @echo "✓ Checked: templar-4626-proxy-soroban" | ||
|
|
There was a problem hiding this comment.
Document check-4626-proxy in help output.
Line 137 adds a new operator-facing recipe, but it is not exposed in the help command output, so it’s easy to miss.
💡 Suggested doc update
@@
`@echo` "BUILD:"
`@echo` " just build Build vault WASM"
+ `@echo` " just check-4626-proxy Cargo-check ERC-4626 proxy crate"
`@echo` " just build-blend-adapter Build Blend adapter WASM"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/vault/soroban/justfile` around lines 136 - 143, The new recipe
check-4626-proxy is not listed in the help output; update the justfile's help
recipe (the target that prints available commands) to include an entry for
check-4626-proxy with a short description like "Check the Soroban ERC-4626 proxy
crate" so operators see it; locate the help-related recipe in the justfile and
add a line referencing check-4626-proxy alongside the other documented recipes
(use the exact recipe name check-4626-proxy to ensure it matches).
| Governance timelock/orchestration lives in the dedicated `contract/vault/soroban/governance` | ||
| contract. The runtime still applies canonical governance state changes. Vault-bound governance | ||
| actions cross the contract boundary via `execute_governance(env, caller, payload)`, where the | ||
| payload carries a `GovernanceCommand`. `SetTimelock` and `Other` actions stay local to the | ||
| governance contract. The generic `execute(payload)` path remains for user flows and for the | ||
| retained execute-path config subset (`ALLOCATORS`, `ALLOWED_ADAPTERS`, `VIRTUAL_OFFSETS`). |
There was a problem hiding this comment.
Remove the retained-execute config claim.
The runtime no longer accepts governance/config mutations through execute(payload). Documenting ALLOCATORS, ALLOWED_ADAPTERS, and VIRTUAL_OFFSETS as still reachable on that path will send integrators to a dead entrypoint; these bullets should point only to execute_governance(...).
Also applies to: 76-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/vault/soroban/README.md` around lines 15 - 20, The README
incorrectly states that retained-config keys ALLOCATORS, ALLOWED_ADAPTERS, and
VIRTUAL_OFFSETS are reachable via execute(payload); update the text to remove
that claim and instead point integrators to the governance entrypoint
execute_governance(env, caller, payload) (and note that SetTimelock/Other remain
local to the governance contract) so all references to execute(payload) as a
path for config/governance mutations are deleted or replaced accordingly (also
update the repeated text at lines 76-78).
| fn execute_governance_command( | ||
| env: &Env, | ||
| caller: &SdkAddress, | ||
| command: &GovernanceCommand, | ||
| ) -> Result<(), crate::error::ContractError> { | ||
| let payload = Bytes::from_slice(env, &command.encode()); | ||
| SorobanVaultContract::execute_governance(env.clone(), caller.clone(), payload) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Soroban SDK tests, does directly calling a contract function under mock_all_auths*exercise contract-to-contract authorization for a contractAddress, or is env.invoke_contract from a registered contract required to test that path?
💡 Result:
Directly calling a contract function via the generated ContractClient under env.mock_all_auths does not fully exercise contract-to-contract authorization for a contract Address. It primarily tests require_auth for account Addresses by mocking the check_auth calls on those contract accounts. To properly test the contract-to-contract authorization path (where one contract authorizes sub-invocations to another contract on its own behalf using env.authorize_as_current_contract and Address::require_auth for contract invoker auth), you need to use env.invoke_contract (or equivalent) from within a registered "caller" contract context, typically set up with env.as_contract(&caller_contract_id, || { ... }). Key evidence: - Standard client calls with mock_all_auths record auths for the top-level invocation and mock account auths (sources 3,17,22). - Contract account check_auth testing explicitly uses env.try_invoke_contract_check_auth to emulate host calls during require_auth, separate from normal function invocation (source 15). - Contract-to-contract auth uses InvokerContractAuthEntry for sub-invocations, authorized via authorize_as_current_contract before env.invoke_contract (sources 2,3,13,21). - No sources show client calls exercising invoker contract auth; all examples use direct client for account auth testing, and special utils or contract context for deeper paths. mock_all_auths skips check_auth entirely (source 2), so it mocks the account auth path but doesn't test the contract's custom auth logic. For full coverage, combine with try_invoke_contract_check_auth or actual C2C calls.
Citations:
- 1: https://developers.stellar.org/docs/build/guides/auth/contract-authorization
- 2: https://www.mintlify.com/stellar/rs-soroban-sdk/core/authentication
- 3: https://mintlify.com/stellar/rs-soroban-sdk/core/environment
- 4: https://github.com/stellar/soroban-examples/blob/v23.0.0/account/src/test.rs
- 5: https://github.com/stellar/soroban-examples/blob/9f46378a/token/src/test.rs
- 6: https://docs.rs/soroban-sdk/latest/soroban_sdk/auth/enum.InvokerContractAuthEntry.html
- 7: https://www.mintlify.com/stellar/rs-soroban-sdk/guides/testing
🏁 Script executed:
# First, check the file structure and size
wc -l contract/vault/soroban/src/tests.rsRepository: Templar-Protocol/contracts
Length of output: 110
🏁 Script executed:
# Read the context around lines 383-390 to see the full helper and surrounding tests
sed -n '350,420p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 2676
🏁 Script executed:
# Search for invoke_contract usage to check if contract-to-contract tests exist
rg "invoke_contract" contract/vault/soroban/src/tests.rsRepository: Templar-Protocol/contracts
Length of output: 512
🏁 Script executed:
# Search for governance contract registration or setup patterns
rg "governance" contract/vault/soroban/src/tests.rs -A 2 -B 2 | head -100Repository: Templar-Protocol/contracts
Length of output: 3132
🏁 Script executed:
# Find both execute_governance_command definitions with context
rg "fn execute_governance_command" contract/vault/soroban/src/tests.rs -A 5 -B 2Repository: Templar-Protocol/contracts
Length of output: 484
🏁 Script executed:
# Find line numbers for both helpers
rg -n "fn execute_governance_command" contract/vault/soroban/src/tests.rsRepository: Templar-Protocol/contracts
Length of output: 148
🏁 Script executed:
# Check which tests call which helper - search for calls to execute_governance_command
rg "execute_governance_command\(" contract/vault/soroban/src/tests.rs -B 5 | head -80Repository: Templar-Protocol/contracts
Length of output: 1287
🏁 Script executed:
# Also check for direct calls to execute_governance to see the pattern
rg "SorobanVaultContract::execute_governance" contract/vault/soroban/src/tests.rs -B 3 -A 2Repository: Templar-Protocol/contracts
Length of output: 688
🏁 Script executed:
# Get full context for both helpers to understand their usage
sed -n '383,395p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 668
🏁 Script executed:
# Get the second helper
sed -n '1939,1955p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 649
🏁 Script executed:
# Now check which tests call the first helper (lines 383-390) and which tests are affected
# Get all test functions that call execute_governance_command
rg "#\[.*test" contract/vault/soroban/src/tests.rs -A 50 | rg -B 50 "execute_governance_command" | head -150Repository: Templar-Protocol/contracts
Length of output: 5787
🏁 Script executed:
# Check which helper the newer tests (around governance) use - search tests after line 1900
sed -n '1900,2100p' contract/vault/soroban/src/tests.rs | grep -n "execute_governance_command\|invoke_contract" | head -20Repository: Templar-Protocol/contracts
Length of output: 145
🏁 Script executed:
# Check the structure - which module is the first helper in and which module is the second?
sed -n '350,395p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 1914
🏁 Script executed:
# Check what module contains line 1939
sed -n '1900,1950p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 2532
🏁 Script executed:
# Now let's see which tests actually USE the first helper (line 383)
# by checking test functions that call it before line 1900
sed -n '380,1900p' contract/vault/soroban/src/tests.rs | rg "#\[.*test" -A 30 | rg -B 30 "execute_governance_command" | head -100Repository: Templar-Protocol/contracts
Length of output: 1040
🏁 Script executed:
# Check if there are tests using invoke_contract directly for governance (C2C pattern)
rg "test.*governance" contract/vault/soroban/src/tests.rs -i | head -30Repository: Templar-Protocol/contracts
Length of output: 417
🏁 Script executed:
# Let's identify exactly which tests are NEW by looking for tests that call the first helper (before line 1900)
rg -n "fn test_" contract/vault/soroban/src/tests.rs | awk -F: '$1 < 1900 {print}' | tail -20Repository: Templar-Protocol/contracts
Length of output: 1004
🏁 Script executed:
# And check which tests call the first helper (direct call)
sed -n '380,1900p' contract/vault/soroban/src/tests.rs | grep -n "fn test_" | tail -10Repository: Templar-Protocol/contracts
Length of output: 564
🏁 Script executed:
# Let's be more specific - check the exact context of test_execute_governance_bridge_happy_path
rg -n "test_execute_governance_bridge_happy_path" contract/vault/soroban/src/tests.rsRepository: Templar-Protocol/contracts
Length of output: 127
🏁 Script executed:
# Get the test to see which helper it uses
sed -n '2020,2100p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 3428
🏁 Script executed:
# Look at the actual new tests - check test_execute_governance_rejects_unauthorized_callers
sed -n '2630,2680p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 1951
🏁 Script executed:
# Check test_execute_governance_rejects_malformed_payload as well
sed -n '2680,2730p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 2296
🏁 Script executed:
# Determine which module each test belongs to by checking mod declarations
rg -n "^mod " contract/vault/soroban/src/tests.rsRepository: Templar-Protocol/contracts
Length of output: 212
🏁 Script executed:
# Now let's understand which tests in contract_tests use the first helper
sed -n '286,1397p' contract/vault/soroban/src/tests.rs | grep -n "fn test_\|execute_governance_command"Repository: Templar-Protocol/contracts
Length of output: 1576
🏁 Script executed:
# Check what specific governance tests exist in contract_tests (the module containing line 383)
sed -n '286,1397p' contract/vault/soroban/src/tests.rs | grep -n "test.*governance\|execute_governance_command" -iRepository: Templar-Protocol/contracts
Length of output: 151
🏁 Script executed:
# Let's look at tests that DO use the first helper directly
sed -n '700,1100p' contract/vault/soroban/src/tests.rs | grep -B 10 "execute_governance_command"Repository: Templar-Protocol/contracts
Length of output: 321
🏁 Script executed:
# Let me see the full test_execute_governance_bridge_happy_path to confirm it uses invoke_contract properly
sed -n '2608,2680p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 2995
🏁 Script executed:
# And check test_execute_governance_rejects_unauthorized_callers fully
sed -n '2650,2730p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 3414
🏁 Script executed:
# Check if test_set_virtual_offsets_updates_contract_storage (which uses first helper) properly sets up governance
sed -n '675,730p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 2665
🏁 Script executed:
# Let me check which tests in contract_tests (before line 1398) actually use execute_governance_command
sed -n '286,1397p' contract/vault/soroban/src/tests.rs | grep -B 20 "execute_governance_command" | head -50Repository: Templar-Protocol/contracts
Length of output: 1521
🏁 Script executed:
# Check test_set_virtual_offsets_updates_contract_storage more carefully
sed -n '675,750p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 3507
🏁 Script executed:
# Key question: which module does test_execute_governance_bridge_happy_path belong to?
# It's at line 2608, which is after line 1910 (storage_tests), so it's in storage_tests
# Confirm by checking the module boundaries
sed -n '1908,1915p' contract/vault/soroban/src/tests.rsRepository: Templar-Protocol/contracts
Length of output: 353
🏁 Script executed:
# Now verify that test_execute_governance_bridge_happy_path calls the storage_tests version
# (4 parameters) not the contract_tests version (3 parameters)
sed -n '2608,2650p' contract/vault/soroban/src/tests.rs | grep -n "execute_governance_command"Repository: Templar-Protocol/contracts
Length of output: 108
🏁 Script executed:
# Count parameters in the call at line 2630 area to confirm
sed -n '2625,2640p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 619
🏁 Script executed:
# Find the line numbers of ALL the governance-related test functions
rg -n "fn test.*governance\|fn test_execute_governance" contract/vault/soroban/src/tests.rsRepository: Templar-Protocol/contracts
Length of output: 52
🏁 Script executed:
# Check where test_governance_config_updates_allowed_adapters is located
sed -n '1200,1300p' contract/vault/soroban/src/tests.rs | grep -n "test_governance\|fn test_"Repository: Templar-Protocol/contracts
Length of output: 127
🏁 Script executed:
# Search for all governance tests mentioned in the review
rg "test_governance_config_updates_allowed_adapters\|test_governance_policy_group_membership\|test_execute_governance" contract/vault/soroban/src/tests.rs -nRepository: Templar-Protocol/contracts
Length of output: 52
🏁 Script executed:
# Let's verify where test_set_virtual_offsets_updates_contract_storage is and whether it's in contract_tests
sed -n '670,710p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 1963
🏁 Script executed:
# Let me search for test_set_virtual_offsets by line number
grep -n "test_set_virtual_offsets_updates_contract_storage" contract/vault/soroban/src/tests.rsRepository: Templar-Protocol/contracts
Length of output: 134
🏁 Script executed:
# And check if the new tests mentioned in the original review exist
grep -n "test_governance_config_updates\|test_governance_policy_group\|test_execute_governance_bridge\|test_execute_governance_rejects" contract/vault/soroban/src/tests.rsRepository: Templar-Protocol/contracts
Length of output: 442
🏁 Script executed:
# Let me just search more broadly for all test functions that contain "governance"
grep -n "fn test" contract/vault/soroban/src/tests.rs | grep -i "govern"Repository: Templar-Protocol/contracts
Length of output: 442
🏁 Script executed:
# Let me check test_set_virtual_offsets_updates_contract_storage - how does it use execute_governance_command?
sed -n '960,1000p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 1727
🏁 Script executed:
# Check whether test_governance_config_updates_allowed_adapters uses invoke_contract or direct call
sed -n '2490,2510p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 1087
🏁 Script executed:
# Verify test_execute_governance_rejects_unauthorized_callers uses invoke_contract
sed -n '2721,2760p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 1805
🏁 Script executed:
# Let me verify test_governance_config_updates_allowed_adapters continues to see how it invokes governance
sed -n '2490,2540p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 2306
🏁 Script executed:
# And check test_execute_governance_rejects_malformed_payload to see its full pattern
sed -n '2811,2860p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 1396
🏁 Script executed:
# Now I see the pattern. Let me verify: test_governance_config_updates_allowed_adapters
# calls SorobanVaultContract::execute_governance directly (line 38), not through invoke_contract
# This is the issue! Let me confirm the exact call
sed -n '2525,2540p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 674
🏁 Script executed:
# And test_governance_policy_group_membership - check its implementation
sed -n '2540,2610p' contract/vault/soroban/src/tests.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 3275
Some governance tests lack contract-to-contract bridge coverage.
Tests test_governance_config_updates_allowed_adapters_without_supply_queue_constraint and test_governance_policy_group_membership_empty_string_clears_membership call SorobanVaultContract::execute_governance directly under mock_all_auths_allowing_non_root_auth(), which only exercises the post-auth handler path, not the real contract-to-contract invocation. However, test_execute_governance_bridge_happy_path and the rejection tests do use the proper env.invoke_contract pattern via the storage_tests helper, so bridge coverage exists. Consider whether the earlier policy tests should also use the invoke_contract helper to ensure authorization semantics are properly exercised.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/vault/soroban/src/tests.rs` around lines 383 - 390, Two governance
tests call SorobanVaultContract::execute_governance directly under
mock_all_auths_allowing_non_root_auth(), which bypasses the contract-to-contract
bridge and only exercises the post-auth handler path; update those tests
(test_governance_config_updates_allowed_adapters_without_supply_queue_constraint
and test_governance_policy_group_membership_empty_string_clears_membership) to
perform the governance call via env.invoke_contract using the same storage_tests
helper pattern used by test_execute_governance_bridge_happy_path so the full
authorization/bridge semantics are executed (replace direct
SorobanVaultContract::execute_governance calls with env.invoke_contract
invocations that target the SorobanVault contract and use the same payload
construction and caller setup as the other bridge tests).
| | | **Spoof.3** — `execute_governance` verifies the outer caller is the governance contract | ||
| before decoding or dispatching the payload, preventing direct invocation that would bypass | ||
| governance timelocks. Interactions: I8, I19–I22. | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fix table formatting for consistent Markdown linting.
Static analysis flagged missing table pipes on multi-line cells. While the content renders correctly in most viewers, proper table formatting improves maintainability.
📝 Example fix for multi-line table cells
Consider either:
- Keeping table cell content on a single line (may be long)
- Using proper continuation with trailing pipes:
-| | **Spoof.3** — `execute_governance` verifies the outer caller is the governance contract
-before decoding or dispatching the payload, preventing direct invocation that would bypass
-governance timelocks. Interactions: I8, I19–I22. |
+| | **Spoof.3** — `execute_governance` verifies the outer caller is the governance contract before decoding or dispatching the payload, preventing direct invocation that would bypass governance timelocks. Interactions: I8, I19–I22. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | | **Spoof.3** — `execute_governance` verifies the outer caller is the governance contract | |
| before decoding or dispatching the payload, preventing direct invocation that would bypass | |
| governance timelocks. Interactions: I8, I19–I22. | | |
| | | **Spoof.3** — `execute_governance` verifies the outer caller is the governance contract before decoding or dispatching the payload, preventing direct invocation that would bypass governance timelocks. Interactions: I8, I19–I22. | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 157-157: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
[warning] 158-158: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing leading pipe
(MD055, table-pipe-style)
[warning] 158-158: Table pipe style
Expected: leading_and_trailing; Actual: no_leading_or_trailing; Missing trailing pipe
(MD055, table-pipe-style)
[warning] 158-158: Table column count
Expected: 2; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
[warning] 159-159: Table pipe style
Expected: leading_and_trailing; Actual: trailing_only; Missing leading pipe
(MD055, table-pipe-style)
[warning] 159-159: Table column count
Expected: 2; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 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 157 - 159, The table row
containing "Spoof.3" (the multi-line cell describing execute_governance) is
missing leading/trailing pipe characters on continuation lines; update the
Markdown table so multi-line cell content is either placed on a single line or
each wrapped line includes the table cell delimiters (pipes) to satisfy markdown
table linting — locate the row referencing "Spoof.3" and "execute_governance"
and add the missing leading/trailing pipes for each continuation line or
collapse the description into one line to restore valid table formatting.
1 participant
Summary
Verification
This change is