Add Soroban custodial market adapter#470
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughSoroban Custodial Adapter for Offchain-Managed RoutesThis PR adds a complete custodial adapter implementing explicit NAV tracking, permission-tiered reporting, and exact balance-delta transfer verification. Core Security AspectsArithmetic Safety: All balance mutations use Transfer Integrity: The Authorization Model:
Replay & Staleness Protection: Initialization Hardening: Constructor rejects 8 role/address collision patterns (asset ≠ admin/vault/custodian; custodian ≠ vault; no self-references). Requires vault and asset to be contract addresses only; custodian can be account or contract. Withdrawal Bounds: Both paths correctly enforce Formal Verification: Kani proof verifies conservation invariant: Intentional Design Decisions
Test CoverageComprehensive unit + proptest coverage:
Fuzz target validates Critical Reviewer Focus
Validation Completeness
WalkthroughA new ChangesCustodial Adapter Contract, CLI Integration, and Tooling
Sequence Diagram(s)sequenceDiagram
participant Operator
participant soroban_vault_cli
participant StellarNetwork
participant CustodialAdapterContract
participant TokenContract
rect rgba(70, 130, 180, 0.5)
note over Operator,StellarNetwork: Deploy Phase
Operator->>soroban_vault_cli: deploy stack --custodian <addr>
soroban_vault_cli->>StellarNetwork: upload custodial adapter WASM
soroban_vault_cli->>StellarNetwork: deploy CustodialAdapterContract(admin, vault, custodian, asset)
StellarNetwork-->>soroban_vault_cli: contract ID
soroban_vault_cli->>soroban_vault_cli: checkpoint manifest (custodial_adapter_0)
end
rect rgba(60, 179, 113, 0.5)
note over Operator,TokenContract: Operational Phase
Operator->>CustodialAdapterContract: supply(vault, asset, amount)
CustodialAdapterContract->>TokenContract: transfer(adapter → custodian, amount)
CustodialAdapterContract->>CustodialAdapterContract: ReportedAssets += amount
Operator->>CustodialAdapterContract: set_reported_assets(reporter, asset, new_nav)
CustodialAdapterContract->>CustodialAdapterContract: validate nonce, set ReportedAssets
Operator->>CustodialAdapterContract: progress_withdrawal(vault, asset, requested)
CustodialAdapterContract->>CustodialAdapterContract: actual = min(idle, reported, requested)
CustodialAdapterContract->>TokenContract: transfer(adapter → vault, actual)
CustodialAdapterContract-->>Operator: actual released
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
000a8d6 to
2048e1d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2048e1dce8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/vault/soroban/custodial-adapter/src/lib.rs`:
- Around line 63-68: The role-collision validation in the constructor is
incomplete and allows dangerous state combinations. The condition starting at
line 63 checks several role overlaps but is missing validation for vault ==
asset and custodian == asset combinations. Add these two additional checks to
the existing if condition that validates role collisions, ensuring that vault
cannot equal asset and custodian cannot equal asset, to prevent misconfiguration
that could brick the adapter or route funds to the token contract address.
In `@contract/vault/soroban/README.md`:
- Around line 319-321: The deployment documentation in the README.md file
(around the governance configuration section) needs to be enhanced to explicitly
emphasize human-runbook verification requirements for the custodial trust
boundary. Add a new section or subsection before the production rollout phase
that outlines mandatory operator validation steps: custodian key controls
verification, NAV reporting cadence and approvers confirmation, and
delayed-liquidity incident handling procedures. This addition should clearly
state that these human-runbook verifications are essential prerequisites before
production deployment because the vault architecture intentionally depends on
offchain custodian operations.
In `@tools/soroban-vault-cli/src/commands.rs`:
- Around line 3702-3717: The unindexed CUSTODIAL_ADDRESS exported by the
custodial_adapter_statuses loop in commands.rs is being silently read as a
deployment input by deploy structs, causing implicit behavior. In commands.rs
(lines 3702-3717), remove the unindexed CUSTODIAL_ADDRESS export (keep only the
indexed CUSTODIAL_0_ADDRESS) or rename it to a status-only name that does not
match deploy inputs. In cli.rs (lines 273-275), update the deploy struct to read
from a different environment variable that is never emitted by export-env, or
ensure CUSTODIAL_ADDRESS is reserved only for explicit user input. Apply the
same fix in cli.rs (lines 304-306) for additive adapter deployment to maintain
consistency.
- Around line 1819-1824: The custodial adapter TTL extension calls in
`run_extend_ttl` bypass the governance authorization that was established in
`append_custodial_adapters`. The custodial contract's `extend_ttl` entrypoint
requires admin authorization, so direct calls with the resolved source/caller
will fail. In tools/soroban-vault-cli/src/commands.rs at lines 1819-1824 and
lines 3311-3323, modify the custodial adapter invocations to route through the
authorized governance admin path instead of directly calling with the resolved
caller. This ensures that the TTL extension requests have the proper
authorization context required by the custodial contract's admin-only
`extend_ttl` entrypoint.
- Around line 1486-1507: The loop iterating through args.custodians calls
next_custodial_adapter_key(manifest) repeatedly, generating duplicate keys like
custodial_adapter_0 for each custodian even though different custodians should
get unique keys (_0, _1, etc.). Before the loop, deduplicate the custodians
using a set instead of iterating the vector directly, and maintain a local index
counter starting at 0. Inside the loop in the else block, replace the call to
next_custodial_adapter_key(manifest) with a key generated from your local
counter, then increment the counter after processing each custodian. This same
fix must be applied to the second occurrence of this pattern around lines
1585-1607.
🪄 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: f125b606-bb28-496f-b9f7-db050d53702e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlcontract/vault/soroban/README.mdcontract/vault/soroban/custodial-adapter/Cargo.tomlcontract/vault/soroban/custodial-adapter/src/lib.rscontract/vault/soroban/justfilefuzz/Cargo.tomlfuzz/fuzz_targets/fuzz_soroban_custodial_adapter.rstools/soroban-vault-cli/README.mdtools/soroban-vault-cli/src/artifacts.rstools/soroban-vault-cli/src/cli.rstools/soroban-vault-cli/src/commands.rs
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/vault/soroban/custodial-adapter/src/lib.rs`:
- Around line 502-509: The require_report_update_authority function currently
allows admin to bypass the pause check and update NAV while paused, whereas
non-admin reporters are blocked. This creates an exception path for admin.
Either document this intentional admin recovery exception in the public pause
and report update documentation to clarify that pause blocks non-admin reports
only, or if admin should also be subject to pause restrictions, move the
is_paused check before the admin bypass check so the pause condition is
evaluated regardless of caller role.
- Around line 1465-1488: The test
fee_on_transfer_asset_is_rejected_without_accounting_change verifies that
total_assets remains unchanged but does not assert the token balances
themselves. Add assertions after the rejected CustodialAdapterContract::supply
call to check that both the adapter (contract_id) and custodian token balances
are unchanged from their initial state, demonstrating that the token transfer
operation was properly rolled back when the fee-on-transfer guard rejected the
supply.
- Around line 224-241: The current implementation accepts any report nonce
greater than the previously stored nonce, which creates unsafe state transitions
where stale reports can become valid again or a nonce jump to u64::MAX blocks
future reports. Change the nonce validation condition from accepting any future
nonce (report_nonce <= load_report_nonce) to requiring an exact match of the
current revision. Additionally, locate the supply, withdraw, and
progress_withdrawal functions and ensure they advance the report nonce by the
same increment mechanism used in the report update function whenever they
successfully update ReportedAssets. This makes invalid states unrepresentable by
coupling all NAV mutations to revision advancement, not just report submissions.
🪄 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: 01130bff-771b-480d-8171-8295e099e055
📒 Files selected for processing (4)
contract/vault/soroban/README.mdcontract/vault/soroban/custodial-adapter/src/lib.rscontract/vault/soroban/justfiletools/soroban-vault-cli/README.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/soroban-vault-cli/src/commands.rs (2)
1507-1514:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMirror deploy-time adapter validation before planning.
With both
blend_poolsandcustodiansempty, this plan path now emits no adapter targets and still returnsOk, whiledeploy_adaptersrejects the same input. Add the same guard at the start ofdeploy_adapters_plan.Proposed fix
fn deploy_adapters_plan( cli: &Cli, manifest: &Manifest, args: &crate::cli::DeployAdaptersArgs, ) -> anyhow::Result<PlanResponse> { + anyhow::ensure!( + !args.blend_pools.is_empty() || !args.custodians.is_empty(), + "deploy adapters requires at least one --blend-pool or --custodian" + ); let mut plan = PlanResponse::new("deploy adapters", &cli.network);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/soroban-vault-cli/src/commands.rs` around lines 1507 - 1514, The deploy_adapters_plan function does not validate that at least one of blend_pools or custodians is provided, which creates an inconsistency with deploy_adapters that rejects this condition. Add the same validation guard at the beginning of deploy_adapters_plan that deploy_adapters uses to check whether both blend_pools and custodians are empty, ensuring both functions enforce the same requirement and neither allows empty adapter plans to proceed.
3314-3324:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not treat missing custodial admin metadata as authorized.
For imported or legacy custodial adapter records without
constructor_args["admin"],custodial_ttl_skip_reasonreturnsNone, sorun_extend_ttlfalls through to a directextend_ttlcall. If the resolved caller is not the unknown admin, one adapter can fail the whole deployment-wide TTL run instead of being reported as skipped.Proposed fix
fn custodial_ttl_skip_reason(manifest: &Manifest, key: &str, caller: &str) -> Option<String> { - let admin = custodial_adapter_admin(manifest, key)?; + let Some(admin) = custodial_adapter_admin(manifest, key) else { + return Some(format!( + "{key}: custodial adapter admin is not recorded in manifest" + )); + }; if contract_id(manifest, "governance").is_some_and(|governance| governance == admin) { return Some(format!( "{key}: admin is governance; no direct custodial adapter TTL authorization path"Also applies to: 3338-3349
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/soroban-vault-cli/src/commands.rs` around lines 3314 - 3324, The issue is that when custodial_ttl_skip_reason returns None for imported or legacy custodial adapter records without constructor_args["admin"], the code falls through to invoke extend_ttl directly instead of treating it as a skip condition. This can cause a single adapter to fail the entire deployment-wide TTL run if the caller is not authorized. Fix this by ensuring custodial_ttl_skip_reason explicitly returns a skip reason when constructor_args["admin"] is missing, rather than returning None, so that all adapters lacking proper admin metadata are properly reported as skipped and do not attempt unauthorized extend_ttl invocations. This issue appears in the extend_ttl invocation block and applies to similar patterns elsewhere in the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tools/soroban-vault-cli/src/commands.rs`:
- Around line 1507-1514: The deploy_adapters_plan function does not validate
that at least one of blend_pools or custodians is provided, which creates an
inconsistency with deploy_adapters that rejects this condition. Add the same
validation guard at the beginning of deploy_adapters_plan that deploy_adapters
uses to check whether both blend_pools and custodians are empty, ensuring both
functions enforce the same requirement and neither allows empty adapter plans to
proceed.
- Around line 3314-3324: The issue is that when custodial_ttl_skip_reason
returns None for imported or legacy custodial adapter records without
constructor_args["admin"], the code falls through to invoke extend_ttl directly
instead of treating it as a skip condition. This can cause a single adapter to
fail the entire deployment-wide TTL run if the caller is not authorized. Fix
this by ensuring custodial_ttl_skip_reason explicitly returns a skip reason when
constructor_args["admin"] is missing, rather than returning None, so that all
adapters lacking proper admin metadata are properly reported as skipped and do
not attempt unauthorized extend_ttl invocations. This issue appears in the
extend_ttl invocation block and applies to similar patterns elsewhere in the
codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c08cfe18-7a59-4fee-8643-d55ccb2207da
📒 Files selected for processing (5)
contract/vault/soroban/README.mdcontract/vault/soroban/custodial-adapter/src/lib.rscontract/vault/soroban/justfiletools/soroban-vault-cli/README.mdtools/soroban-vault-cli/src/commands.rs
This reverts commit 445aa9c.
3e90d21 to
4df7b87
Compare
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/soroban-vault-cli/src/artifacts.rs (1)
24-30: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd regression assertions for the new custodial artifact mapping.
The new
CustodialAdaptermapping is deployment-critical, but current tests only lock Vault path behavior. Add assertions forArtifactSpec::from_name(ArtifactName::CustodialAdapter)andstack_artifacts(false, true)to prevent key/package/path drift.💡 Suggested test patch
#[test] fn artifact_paths_match_workspace_layout() { let root = Path::new("/workspace"); assert_eq!( ArtifactSpec::from_name(ArtifactName::Vault).wasm_path(root), @@ "/workspace/target/wasm32-unknown-unknown/release-soroban/templar_soroban_runtime.wasm" ) ); + assert_eq!( + ArtifactSpec::from_name(ArtifactName::CustodialAdapter).wasm_path(root), + PathBuf::from( + "/workspace/target/wasm32-unknown-unknown/release-soroban/templar_soroban_custodial_adapter.wasm" + ) + ); + let specs = ArtifactSpec::stack_artifacts(false, true); + assert!(specs.iter().any(|s| s.key == "custodial_adapter")); }Also applies to: 36-49, 93-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/soroban-vault-cli/src/artifacts.rs` around lines 24 - 30, Add regression test assertions to verify the new CustodialAdapter artifact mapping. Create test cases that assert ArtifactSpec::from_name(ArtifactName::CustodialAdapter) returns the correct specification with expected key, package, and path values. Additionally, add test assertions for stack_artifacts(false, true) to lock in the current artifact stack behavior and prevent drift in deployment-critical artifact paths. These tests should be added to your test module to match the pattern of existing Vault path assertions and ensure consistency across all artifact mappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/vault/soroban/justfile`:
- Around line 611-665: The custodial adapter section in the justfile is missing
recipe definitions for TTL extension and admin transfer operations that both the
blend adapter and the custodial adapter contract support. Add four new recipes
after the custodial-adapter-status recipe: custodial-adapter-extend-ttl (to
extend the contract TTL), custodial-adapter-set-admin (to initiate admin
transfer), custodial-adapter-accept-admin (to accept pending admin), and
custodial-adapter-pending-admin (to query pending admin). Follow the same
pattern as existing custodial adapter recipes like custodial-adapter-status and
custodial-adapter-total-assets, using the blend adapter recipes as a structural
reference for how to implement these operations.
In `@fuzz/fuzz_targets/fuzz_soroban_custodial_adapter.rs`:
- Around line 14-33: The `bounded_non_negative` function uses `saturating_abs()`
which converts all values to non-negative, preventing the fuzzer from testing
error paths in `simulate_progress_withdrawal` that require negative values for
`reported`, `idle_balance`, and `requested`. Replace the bounded_non_negative
function to preserve the sign of input values while still bounding them to a
reasonable range (for example, by modulating the absolute value but keeping the
original sign), so the fuzzer can exercise the InvalidInput and
InsufficientReturnedLiquidity error branches that depend on detecting negative
or zero values in these fields.
In `@tools/soroban-vault-cli/src/cli.rs`:
- Around line 1116-1117: Add a focused test case in the cli.rs file that
validates comma-delimited custodian parsing. The test should pass a single
--custodian argument with a comma-separated value like "A,B" and assert that
exactly two custodian values are parsed correctly. This test case should be
added alongside the existing repeated flags tests around lines 1116 and 1156 to
ensure the documented comma-delimited parsing path is covered and prevent future
delimiter parsing regressions.
In `@tools/soroban-vault-cli/src/commands.rs`:
- Around line 3276-3280: The code currently resolves a caller whenever any
custodial adapter exists, but some custodial adapters (governance-admin and
those with missing admin metadata) should not trigger caller resolution. Filter
the custodial_adapter_statuses(manifest) result to exclude non-callable adapters
(governance-admin and missing-admin statuses) before checking if a caller needs
to be resolved. At the anchor site (3276-3280), update the condition to only
call resolve_extend_ttl_caller if there are custodial adapters that can actually
be called directly. Apply the same filtering logic at the sibling sites
(3312-3316 and 3338-3348) to ensure consistent handling of non-callable
custodial adapters across all locations where custodial_adapter_statuses is
checked.
- Around line 3664-3697: The function `custodial_adapter_statuses` currently
only appends the legacy `custodial_adapter` when the adapters list is empty,
which causes it to be dropped when indexed adapters exist. Remove the `if
adapters.is_empty()` condition so that the legacy `custodial_adapter` is always
appended to the results (if it exists in the manifest) regardless of whether
indexed adapters have already been added, ensuring the legacy adapter remains
visible in the status output.
- Around line 1791-1823: Add preflight validation in the custodian deployment
loop to reject invalid role address combinations before attempting contract
deployment. Before the call to deploy_contract_if_needed, validate that the role
addresses (governance, vault, custodian, and asset_token) do not have
problematic equal-address combinations that the custodial contract constructor
would reject. This ensures invalid configurations fail immediately without
network writes, rather than discovering validation failures after calling
deploy_contract_if_needed.
---
Outside diff comments:
In `@tools/soroban-vault-cli/src/artifacts.rs`:
- Around line 24-30: Add regression test assertions to verify the new
CustodialAdapter artifact mapping. Create test cases that assert
ArtifactSpec::from_name(ArtifactName::CustodialAdapter) returns the correct
specification with expected key, package, and path values. Additionally, add
test assertions for stack_artifacts(false, true) to lock in the current artifact
stack behavior and prevent drift in deployment-critical artifact paths. These
tests should be added to your test module to match the pattern of existing Vault
path assertions and ensure consistency across all artifact mappings.
🪄 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: c3b04a32-6d18-43c5-bfcc-b2559b628b93
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlcontract/vault/soroban/README.mdcontract/vault/soroban/custodial-adapter/Cargo.tomlcontract/vault/soroban/custodial-adapter/src/lib.rscontract/vault/soroban/justfilefuzz/Cargo.tomlfuzz/fuzz_targets/fuzz_soroban_custodial_adapter.rstools/soroban-vault-cli/README.mdtools/soroban-vault-cli/src/artifacts.rstools/soroban-vault-cli/src/cli.rstools/soroban-vault-cli/src/commands.rs
Summary
Adds a Soroban custodial market adapter for the offchain-managed Option 1 route.
Trust model
This adapter intentionally does not initiate or prove an offchain market exit. The configured custodian and its operating process are part of the vault trust boundary. Offchain infra is responsible for bridging, depositing, unwinding, and returning Stellar-side liquidity before the vault can withdraw from the adapter.
Validation
RUSTUP_TOOLCHAIN=1.89.0 CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-soroban-custodial-adapter/target cargo test -p templar-soroban-custodial-adapter -- --nocaptureRUSTUP_TOOLCHAIN=1.89.0 CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-soroban-custodial-adapter/target cargo test -p templar-fuzz --no-run --bin fuzz_soroban_custodial_adapterCARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-soroban-custodial-adapter/target just -f contract/vault/soroban/justfile build-custodial-adapterRUSTUP_TOOLCHAIN=1.89.0 cargo fmt -- --checkRUSTUP_TOOLCHAIN=1.89.0 CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-soroban-custodial-adapter/target cargo clippy -p templar-soroban-custodial-adapter --all-targets -- -D warningsgit diff --checksize-budget-checkpassed at128894bytescargo-fuzzis not installed in this environment, so I compiled the new fuzz target but did not runcargo fuzz run.This change is