feat(soroban): split idle and async withdrawals#423
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughSummaryThis PR separates Soroban vault withdrawal semantics into two explicit paths: atomic idle-only exits ( Critical Issues Requiring Review1. Authorization Bypass: Atomic Operations Missing RBAC Check
|
| Layer / File(s) | Summary |
|---|---|
Kernel queued-withdraw liquidity & finish-allocation contract/vault/kernel/src/actions/mod.rs, contract/vault/kernel/src/actions/tests.rs |
Actionable queued withdrawals now require available_assets >= expected_assets; plan_idle_payout uses state.idle_assets; handle_finish_allocating no longer chains into withdrawals; tests updated to assert refusal/no-partial-settlement and restricted-head retention. |
VaultCommand schema, codec, and CLI encoder contract/vault/soroban/shared-types/src/lib.rs, contract/vault/soroban/scripts/vault_payload.py, tests |
Adds AtomicWithdraw/AtomicRedeem variants with tags 6/7, extends encode/decode and roundtrip/trailing-byte tests, and adds CLI subcommands/encoders for the new commands. |
Entrypoints and proxy_view wiring contract/vault/soroban/src/contract/entrypoints.rs, contract/vault/soroban/src/contract/mod.rs, contract/vault/soroban/src/fungible_vault.rs |
Implements atomic_withdraw_impl/atomic_redeem_impl, dispatches new commands and returns i128 results, updates proxy_view to compute per-owner atomic limits when idle/unpaused, and adds share_balance helper plus related docs. |
Soroban tests and fixture wiring contract/vault/soroban/src/tests.rs, contract/vault/soroban/tests/integration_tests.rs |
Adjusts low-liquidity test setups and expectations to refuse partial settlements when idle is insufficient, adds atomic command success/failure tests (idle vs external liquidity), and updates integration fixture to pass/store share token and assert non-zero per-owner proxy previews. |
Docs, diagrams, and recipes contract/vault/soroban/STRIDE.md, contract/vault/soroban/README.md, contract/vault/soroban/justfile |
Clarifies atomic vs queued withdrawal flows, introduces Keeper/Allocator role, updates sequence diagrams and STRIDE interaction mapping, and adds an atomic withdraw recipe plus demo/help updates. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
- Templar-Protocol/contracts#457: Both PRs modify the vault kernel’s withdrawal-liquidity gating in
contract/vault/kernel/src/actions/mod.rs.
Suggested reviewers
- peer2f00l
- royalf00l
- SmolinPavel
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/soroban-idle-withdraw-async-requests
Warning
Review ran into problems
🔥 Problems
Errors were encountered while retrieving linked issues.
Errors (1)
- LINEAR integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.
Comment @coderabbitai help to get the list of available commands and usage tips.
|
@coderabbitai review |
|
@coderabbitai for the benefit of the human reviewers - please output a description of this PR and any critique on the usability of the design when you are reviewing. |
✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me examine the actual code changes in this PR to give the human reviewers a thorough description and design critique. 🏁 Script executed: Length of output: 421 🏁 Script executed: Length of output: 1996 🏁 Script executed: Length of output: 124994 🏁 Script executed: Length of output: 57489
PR DescriptionThis PR makes the withdrawal semantics of the Soroban vault explicit by splitting previously ambiguous code paths into distinct, clearly-bounded operations. What changedThree withdrawal verbs, three clear contracts:
Before this PR: New error code added: New kernel actions:
Design Critique1. Slippage locked at request time, not execution time
This is a known ERC-4626 async withdrawal design trade-off. It should be explicitly acknowledged in the STRIDE threat model or a comment explaining the intentional trade-off (protect the withdrawer's slippage expectation vs. protect remaining depositors). 2. No user self-escape from the queueOnce a user calls
A 3.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contract/vault/soroban/justfile (1)
737-745:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
demo-withdrawtext says conditional execute, but the recipe still hard-fails unconditionally.Line 737 says execution happens only if idle-funded, but Line 744 always runs
just execute-withdraw. When cooldown/idle conditions are unmet, the demo aborts instead of demonstrating the async path cleanly.Suggested fix
demo-withdraw: `@echo` "=== WITHDRAWAL DEMO ===" `@echo` "Step 1: Request withdrawal" `@just` request-withdraw-self 500000 `@echo` "" `@echo` "Step 2: Execute withdrawal" - `@just` execute-withdraw + `@if` just execute-withdraw; then \ + echo "execute_withdraw completed"; \ + else \ + echo "execute_withdraw pending (cooldown or insufficient idle); retry later."; \ + fi `@echo` "" `@just` vault-state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/soroban/justfile` around lines 737 - 745, The demo-withdraw recipe claims to only execute when idle-funded but unconditionally runs just execute-withdraw, causing hard failures; update the demo-withdraw recipe so after request-withdraw-self 500000 it checks the vault idle/funded condition (using the existing idle-check helper or CLI/status command used elsewhere) and only invokes just execute-withdraw when that check succeeds, otherwise print a message like "idle not funded, withdrawal pending" and exit successfully to demonstrate the async path; reference the demo-withdraw recipe and the execute-withdraw and request-withdraw-self targets when making the change.contract/vault/soroban/src/tests.rs (1)
702-706:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep these queue tests solvent while starving idle liquidity.
Both setups lower
idle_assetsbut leaveexternal_assetsat zero, sototal_assetsalso drops below the queued payout. That means the test can still pass because the vault is insolvent, not because the request is fully funded overall but not idle-funded. Move the shortfall intoexternal_assetsso this specifically covers the new "no partial settlement from idle" invariant.Based on learnings: "Watch for `OpState` transitions, escrow accounting, keeper-routed withdrawals, callback ordering, idle-balance resync, fee accrual, and reconciliation after partial failures in the vault contract".🧪 Suggested test fix
{ let state = vault.state_mut().unwrap(); state.idle_assets = MIN_WITHDRAWAL_ASSETS.saturating_sub(1); + state.external_assets = deposit_amount.saturating_sub(state.idle_assets); state.total_assets = state.idle_assets.saturating_add(state.external_assets); } @@ { let state = vault.state_mut().unwrap(); state.idle_assets = MIN_WITHDRAWAL_ASSETS.saturating_add(1); + state.external_assets = deposit_amount.saturating_sub(state.idle_assets); state.total_assets = state.idle_assets.saturating_add(state.external_assets); }Also applies to: 763-767
🤖 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 702 - 706, The test currently reduces state.idle_assets and recomputes state.total_assets so total_assets falls below the queued payout, making the vault insolvent rather than specifically starving idle liquidity; instead leave total_assets intact and move the shortfall into state.external_assets so idle_assets < required but total_assets >= required; update the two test blocks that set state.idle_assets and state.total_assets (references: state.idle_assets, state.external_assets, state.total_assets, MIN_WITHDRAWAL_ASSETS) to set state.idle_assets = MIN_WITHDRAWAL_ASSETS.saturating_sub(1) and then set state.external_assets = state.external_assets.saturating_add(1 + (original_total_assets - state.total_assets)) (i.e. adjust external_assets so total_assets remains sufficient) and apply the same change to the other occurrence mentioned (lines around the second block at 763-767).
🤖 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 58-59: The diagram incorrectly implies entrypoint-level
require_auth() for atomic withdraws; update the README so it either
removes/narrows the Entry->>Entry: require_auth() arrow for the atomic withdraw
path or add a brief note stating that atomic_withdraw_impl and
atomic_redeem_impl do not call require_signed and that authorization is enforced
deeper in the vault/share-token path (e.g., via the share-token or vault-level
checks), referencing the atomic_withdraw_impl and atomic_redeem_impl symbols so
readers know where auth actually occurs.
---
Outside diff comments:
In `@contract/vault/soroban/justfile`:
- Around line 737-745: The demo-withdraw recipe claims to only execute when
idle-funded but unconditionally runs just execute-withdraw, causing hard
failures; update the demo-withdraw recipe so after request-withdraw-self 500000
it checks the vault idle/funded condition (using the existing idle-check helper
or CLI/status command used elsewhere) and only invokes just execute-withdraw
when that check succeeds, otherwise print a message like "idle not funded,
withdrawal pending" and exit successfully to demonstrate the async path;
reference the demo-withdraw recipe and the execute-withdraw and
request-withdraw-self targets when making the change.
In `@contract/vault/soroban/src/tests.rs`:
- Around line 702-706: The test currently reduces state.idle_assets and
recomputes state.total_assets so total_assets falls below the queued payout,
making the vault insolvent rather than specifically starving idle liquidity;
instead leave total_assets intact and move the shortfall into
state.external_assets so idle_assets < required but total_assets >= required;
update the two test blocks that set state.idle_assets and state.total_assets
(references: state.idle_assets, state.external_assets, state.total_assets,
MIN_WITHDRAWAL_ASSETS) to set state.idle_assets =
MIN_WITHDRAWAL_ASSETS.saturating_sub(1) and then set state.external_assets =
state.external_assets.saturating_add(1 + (original_total_assets -
state.total_assets)) (i.e. adjust external_assets so total_assets remains
sufficient) and apply the same change to the other occurrence mentioned (lines
around the second block at 763-767).
🪄 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: 81393740-b140-4dca-b6f1-71bc15a58721
📒 Files selected for processing (10)
contract/vault/kernel/src/actions/mod.rscontract/vault/kernel/src/actions/tests.rscontract/vault/kernel/src/error.rscontract/vault/soroban/README.mdcontract/vault/soroban/STRIDE.mdcontract/vault/soroban/justfilecontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/fungible_vault.rscontract/vault/soroban/src/tests.rs
…6066-2ebb-4b87-9e22-844ce7913a9c) Make Soroban withdrawal semantics explicit: withdraw/redeem stay atomic and idle-liquidity-only, request_withdraw remains the async queued exit path, and execute_withdraw only progresses fully backed queued withdrawals. Preserves PR #423 tree; amends the commit subject for Halborn remediation tracking.
71c36bd to
e3d167f
Compare
…6066-2ebb-4b87-9e22-844ce7913a9c) Make Soroban withdrawal semantics explicit: withdraw/redeem stay atomic and idle-liquidity-only, request_withdraw remains the async queued exit path, and execute_withdraw only progresses fully backed queued withdrawals. Preserves PR #423 tree; amends the commit subject for Halborn remediation tracking.
e3d167f to
a0b40f3
Compare
…6066-2ebb-4b87-9e22-844ce7913a9c) Make Soroban withdrawal semantics explicit: withdraw/redeem stay atomic and idle-liquidity-only, request_withdraw remains the async queued exit path, and execute_withdraw only progresses fully backed queued withdrawals. Preserves PR #423 tree; amends the commit subject for Halborn remediation tracking.
a0b40f3 to
1a1dc1b
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a1dc1b115
ℹ️ 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".
| let Some(settlement) = | ||
| compute_idle_settlement(request_escrow, request_expected, available_assets) | ||
| compute_idle_settlement(request_escrow, request_expected, state.idle_assets) |
There was a problem hiding this comment.
When the cooled-down queue head expects more than the current idle balance but the idle balance is still above min_withdrawal_assets (for example expected 600, idle 500), classify_withdrawal_head still marks it Ready, and this call to compute_idle_settlement returns a partial settlement rather than None. execute_withdraw then burns/refunds shares and transfers the partial idle amount, so queued withdrawals still partially settle and mutate the queue instead of failing atomically and leaving it unchanged under the new full-idle-liquidity semantics.
Useful? React with 👍 / 👎.
…6066-2ebb-4b87-9e22-844ce7913a9c) Make Soroban withdrawal semantics explicit: withdraw/redeem stay atomic and idle-liquidity-only, request_withdraw remains the async queued exit path, and execute_withdraw only progresses fully backed queued withdrawals. Preserves PR #423 tree; amends the commit subject for Halborn remediation tracking.
1a1dc1b to
455e582
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/README.md`:
- Around line 114-115: Update the phrasing around "primary user exit path" in
the README where it describes "atomic withdraw/redeem" — replace the ambiguous
"primary user exit path whenever sufficient idle liquidity is already available"
with clearer wording such as "This is the immediate user exit path when
sufficient idle liquidity is available" or "This provides instant withdrawal
from idle assets, while `request_withdraw` handles positions requiring allocator
work" to clarify that atomic withdraw/redeem only applies when idle liquidity
exists.
🪄 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: de92ca76-0e87-467d-8155-c80124690d56
📒 Files selected for processing (2)
contract/vault/soroban/README.mdcontract/vault/soroban/STRIDE.md
…6066-2ebb-4b87-9e22-844ce7913a9c) Make Soroban withdrawal semantics explicit: withdraw/redeem stay atomic and idle-liquidity-only, request_withdraw remains the async queued exit path, and execute_withdraw only progresses fully backed queued withdrawals. Preserves PR #423 tree; amends the commit subject for Halborn remediation tracking.
… 39906066-2ebb-4b87-9e22-844ce7913a9c)
2df853c to
2f08907
Compare
There was a problem hiding this comment.
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/justfile`:
- Around line 775-787: The withdraw recipe should fail fast when the current
identity (addr) does not match the user-supplied owner because the contract
requires operator == owner; inside the withdraw recipe (function name: withdraw)
after computing `@addr` and before calling just vault-execute atomic-withdraw,
compare addr to the "{{owner}}" value and if they differ print a clear error
(including both operator and owner) and exit non‑zero so the shell recipe aborts
early rather than invoking vault-execute with mismatched operator/owner.
In `@contract/vault/soroban/scripts/vault_payload.py`:
- Around line 117-130: The atomic CLI branches for "atomic-withdraw" and
"atomic-redeem" allow an arbitrary --operator which will be rejected on-chain;
update the payload construction in the atomic-withdraw and atomic-redeem
branches so that args.operator is validated or replaced with args.owner (i.e.,
if args.operator is present and != args.owner, either raise/exit with an error
or set operator = owner) before calling push_string(out, args.operator) (and do
the same for the other identical block around lines 215-227) to ensure operator
== owner in the encoded payload.
In `@contract/vault/soroban/shared-types/src/lib.rs`:
- Around line 1080-1095: The test for ProxyViewFields currently uses the same
sdk_address() for all four tuple slots, so swaps between curator, governance,
asset_token, and share_token won't be caught; update the tuple in the
ProxyViewFields::from(...) construction to supply four distinct addresses (e.g.,
curator_addr, governance_addr, asset_token_addr, share_token_addr created via
sdk_address or Env::from_seed variants) and then assert each named field on the
resulting fields variable explicitly (check ProxyViewFields::curator,
::governance, ::asset_token, ::share_token) to ensure the test fails on
wire-position regressions; apply the same change pattern to the other similar
block around lines 1104-1125.
- Around line 30-33: Replace the public tuple alias ProxyCapGroupView with a
named struct (e.g., ProxyCapGroupFields) containing named fields for the three
components, add a From<ProxyCapGroupView> (or implement
From<(String,i128,i128)>) so existing tuple-based conversions still work, and
update ProxyPolicyView and any other type aliases (e.g., ProxyPolicyView,
ProxyViewResponse, places referencing ProxyCapGroupView) to use
Vec<ProxyCapGroupFields> instead of Vec<ProxyCapGroupView>; ensure the new
struct is exported from the crate and that any conversions or tests that
previously used the tuple are adapted to use the named fields or the From
implementation.
In `@contract/vault/soroban/src/contract/entrypoints.rs`:
- Around line 1623-1632: proxy_view currently computes
max_withdraw_value/max_redeem_value from state.idle_assets and owner_shares but
omits the same atomic-path dust floor check used by
handle_atomic_withdraw/handle_atomic_redeem; update the proxy_view logic (the
block that calculates max_withdraw_u128 and max_redeem_u128 in entrypoints.rs)
to apply the min_withdrawal guard (compare against config.min_withdrawal_assets
or MIN_WITHDRAWAL_ASSETS) so that if the computed max_withdraw_u128 is below
that dust floor it is treated as zero (and similarly ensure redeem follows the
same floor rule), using the existing helpers convert_to_assets/convert_to_shares
and state.idle_assets to decide whether to return zero or the to_i128(...)
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: d9d91dac-7179-4877-a922-33bce6056535
📒 Files selected for processing (12)
contract/vault/kernel/src/actions/mod.rscontract/vault/kernel/src/actions/tests.rscontract/vault/soroban/README.mdcontract/vault/soroban/STRIDE.mdcontract/vault/soroban/justfilecontract/vault/soroban/scripts/vault_payload.pycontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/fungible_vault.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/integration_tests.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/justfile`:
- Around line 775-787: The withdraw recipe should fail fast when the current
identity (addr) does not match the user-supplied owner because the contract
requires operator == owner; inside the withdraw recipe (function name: withdraw)
after computing `@addr` and before calling just vault-execute atomic-withdraw,
compare addr to the "{{owner}}" value and if they differ print a clear error
(including both operator and owner) and exit non‑zero so the shell recipe aborts
early rather than invoking vault-execute with mismatched operator/owner.
In `@contract/vault/soroban/scripts/vault_payload.py`:
- Around line 117-130: The atomic CLI branches for "atomic-withdraw" and
"atomic-redeem" allow an arbitrary --operator which will be rejected on-chain;
update the payload construction in the atomic-withdraw and atomic-redeem
branches so that args.operator is validated or replaced with args.owner (i.e.,
if args.operator is present and != args.owner, either raise/exit with an error
or set operator = owner) before calling push_string(out, args.operator) (and do
the same for the other identical block around lines 215-227) to ensure operator
== owner in the encoded payload.
In `@contract/vault/soroban/shared-types/src/lib.rs`:
- Around line 1080-1095: The test for ProxyViewFields currently uses the same
sdk_address() for all four tuple slots, so swaps between curator, governance,
asset_token, and share_token won't be caught; update the tuple in the
ProxyViewFields::from(...) construction to supply four distinct addresses (e.g.,
curator_addr, governance_addr, asset_token_addr, share_token_addr created via
sdk_address or Env::from_seed variants) and then assert each named field on the
resulting fields variable explicitly (check ProxyViewFields::curator,
::governance, ::asset_token, ::share_token) to ensure the test fails on
wire-position regressions; apply the same change pattern to the other similar
block around lines 1104-1125.
- Around line 30-33: Replace the public tuple alias ProxyCapGroupView with a
named struct (e.g., ProxyCapGroupFields) containing named fields for the three
components, add a From<ProxyCapGroupView> (or implement
From<(String,i128,i128)>) so existing tuple-based conversions still work, and
update ProxyPolicyView and any other type aliases (e.g., ProxyPolicyView,
ProxyViewResponse, places referencing ProxyCapGroupView) to use
Vec<ProxyCapGroupFields> instead of Vec<ProxyCapGroupView>; ensure the new
struct is exported from the crate and that any conversions or tests that
previously used the tuple are adapted to use the named fields or the From
implementation.
In `@contract/vault/soroban/src/contract/entrypoints.rs`:
- Around line 1623-1632: proxy_view currently computes
max_withdraw_value/max_redeem_value from state.idle_assets and owner_shares but
omits the same atomic-path dust floor check used by
handle_atomic_withdraw/handle_atomic_redeem; update the proxy_view logic (the
block that calculates max_withdraw_u128 and max_redeem_u128 in entrypoints.rs)
to apply the min_withdrawal guard (compare against config.min_withdrawal_assets
or MIN_WITHDRAWAL_ASSETS) so that if the computed max_withdraw_u128 is below
that dust floor it is treated as zero (and similarly ensure redeem follows the
same floor rule), using the existing helpers convert_to_assets/convert_to_shares
and state.idle_assets to decide whether to return zero or the to_i128(...)
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: d9d91dac-7179-4877-a922-33bce6056535
📒 Files selected for processing (12)
contract/vault/kernel/src/actions/mod.rscontract/vault/kernel/src/actions/tests.rscontract/vault/soroban/README.mdcontract/vault/soroban/STRIDE.mdcontract/vault/soroban/justfilecontract/vault/soroban/scripts/vault_payload.pycontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/fungible_vault.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/integration_tests.rs
🛑 Comments failed to post (5)
contract/vault/soroban/justfile (1)
775-787:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an explicit owner/operator guard in
withdrawrecipe.At Line 779 and Line 784,
operatoris always the current identity whileowneris user-supplied. This recipe will fail when they differ, but only after invoking the contract. Fail fast with a clear check.Suggested patch
withdraw assets receiver owner max_shares_burned="": `@echo` "Atomic withdrawal of {{assets}} assets..." `@addr`="$(stellar keys address ${SOROBAN_IDENTITY:-{{default_identity}}})"; \ + if [ "$addr" != "{{owner}}" ]; then \ + echo "Error: delegated withdraw is not supported; owner must equal current identity ($addr)."; \ + exit 1; \ + fi; \ max_shares="{{max_shares_burned}}"; \ if [ -z "$max_shares" ]; then max_shares="{{assets}}"; fi; \ just vault-execute atomic-withdraw \ --owner "{{owner}}" \ --operator "$addr" \ --assets "{{assets}}" \ --receiver "{{receiver}}" \ --max-shares-burned "$max_shares"Based on learnings: delegated withdrawal operators are intentionally rejected in
contract/proxy-4626-soroban/src/contract.rs(operator == owner).📝 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.# Atomic withdraw (ERC-4626-style immediate exit; idle assets only, never queues). # Usage: just withdraw <assets> <receiver> <owner> [max_shares_burned] withdraw assets receiver owner max_shares_burned="": `@echo` "Atomic withdrawal of {{assets}} assets..." `@addr`="$(stellar keys address ${SOROBAN_IDENTITY:-{{default_identity}}})"; \ if [ "$addr" != "{{owner}}" ]; then \ echo "Error: delegated withdraw is not supported; owner must equal current identity ($addr)."; \ exit 1; \ fi; \ max_shares="{{max_shares_burned}}"; \ if [ -z "$max_shares" ]; then max_shares="{{assets}}"; fi; \ just vault-execute atomic-withdraw \ --owner "{{owner}}" \ --operator "$addr" \ --assets "{{assets}}" \ --receiver "{{receiver}}" \ --max-shares-burned "$max_shares"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/soroban/justfile` around lines 775 - 787, The withdraw recipe should fail fast when the current identity (addr) does not match the user-supplied owner because the contract requires operator == owner; inside the withdraw recipe (function name: withdraw) after computing `@addr` and before calling just vault-execute atomic-withdraw, compare addr to the "{{owner}}" value and if they differ print a clear error (including both operator and owner) and exit non‑zero so the shell recipe aborts early rather than invoking vault-execute with mismatched operator/owner.contract/vault/soroban/scripts/vault_payload.py (1)
117-130:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
operator != ownerin the atomic CLI paths.These subcommands currently let users encode arbitrary
--operatorvalues, but the proxy intentionally rejects delegated withdrawal operators. That means this tool can generate payloads that are guaranteed to fail on-chain. Either validateoperator == ownerbefore encoding or defaultoperatortoowner.Based on learnings, delegated withdrawal operators are explicitly rejected and the Soroban proxy
withdraw/redeempaths enforceoperator == owner.Also applies to: 215-227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/soroban/scripts/vault_payload.py` around lines 117 - 130, The atomic CLI branches for "atomic-withdraw" and "atomic-redeem" allow an arbitrary --operator which will be rejected on-chain; update the payload construction in the atomic-withdraw and atomic-redeem branches so that args.operator is validated or replaced with args.owner (i.e., if args.operator is present and != args.owner, either raise/exit with an error or set operator = owner) before calling push_string(out, args.operator) (and do the same for the other identical block around lines 215-227) to ensure operator == owner in the encoded payload.contract/vault/soroban/shared-types/src/lib.rs (2)
30-33: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Replace the remaining cap-group triple with a named type.
ProxyViewFieldsis clearly trying to eliminate tuple-position footguns, butcap_groupsstill exposes raw(String, i128, i128)entries in the public API. That keeps the most error-prone part of the payload positional for downstream consumers. A dedicatedProxyCapGroupFieldswrapper plusFrom<ProxyCapGroupView>would finish the job.As per coding guidelines, "Prefer stronger types over loosely constrained values in Rust" and "make invalid states unrepresentable."
Also applies to: 76-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/soroban/shared-types/src/lib.rs` around lines 30 - 33, Replace the public tuple alias ProxyCapGroupView with a named struct (e.g., ProxyCapGroupFields) containing named fields for the three components, add a From<ProxyCapGroupView> (or implement From<(String,i128,i128)>) so existing tuple-based conversions still work, and update ProxyPolicyView and any other type aliases (e.g., ProxyPolicyView, ProxyViewResponse, places referencing ProxyCapGroupView) to use Vec<ProxyCapGroupFields> instead of Vec<ProxyCapGroupView>; ensure the new struct is exported from the crate and that any conversions or tests that previously used the tuple are adapted to use the named fields or the From implementation.
1080-1095:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse distinct addresses in the tuple-order test.
This test is meant to catch wire-position regressions, but Lines 1091-1094 feed the same address into every slot. A swap between
curator,governance,asset_token, andshare_tokenwould still pass here. Give each position a distinct address and assert the named fields explicitly.Also applies to: 1104-1125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/soroban/shared-types/src/lib.rs` around lines 1080 - 1095, The test for ProxyViewFields currently uses the same sdk_address() for all four tuple slots, so swaps between curator, governance, asset_token, and share_token won't be caught; update the tuple in the ProxyViewFields::from(...) construction to supply four distinct addresses (e.g., curator_addr, governance_addr, asset_token_addr, share_token_addr created via sdk_address or Env::from_seed variants) and then assert each named field on the resulting fields variable explicitly (check ProxyViewFields::curator, ::governance, ::asset_token, ::share_token) to ensure the test fails on wire-position regressions; apply the same change pattern to the other similar block around lines 1104-1125.contract/vault/soroban/src/contract/entrypoints.rs (1)
1623-1632:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Preview limits vs atomic withdrawal floor" rg -n -C5 '\b(max_withdraw|max_redeem|MIN_WITHDRAWAL_ASSETS|min_withdrawal_assets|atomic_withdraw|atomic_redeem)\b' \ contract/vault/soroban/src \ contract/vault/kernel/srcRepository: Templar-Protocol/contracts
Length of output: 50382
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Search for atomic withdraw/redeem + MIN_WITHDRAWAL_ASSETS" rg -n --hidden -S '\batomic_(withdraw|redeem)\b' contract/vault/soroban/src/contract contract/vault/kernel/src echo rg -n '\bMIN_WITHDRAWAL_ASSETS\b|min_withdrawal_assets' contract/vault/kernel/src contract/vault/soroban/src/contract | head -n 200 echo echo "Inspect handle_atomic_withdraw/redeem in kernel actions" rg -n 'handle_atomic_(withdraw|redeem)' contract/vault/kernel/src/actions/mod.rs echo # Print a focused window around the functions (best effort) awk 'NR>=1&&NR<=220{print}' contract/vault/kernel/src/actions/mod.rs > /tmp/kernel_actions_head.txt # locate line numbers for handle_atomic_withdraw and then print around for fn in handle_atomic_withdraw handle_atomic_redeem; do ln=$(rg -n "$fn" contract/vault/kernel/src/actions/mod.rs | head -n1 | cut -d: -f1) echo "---- $fn at line $ln ----" if [ -n "${ln:-}" ]; then start=$((ln-40)); end=$((ln+80)); sed -n "${start},${end}p" contract/vault/kernel/src/actions/mod.rs fi done echo echo "Inspect curator_vault.rs atomic_withdraw/atomic_redeem" curator="contract/vault/soroban/src/contract/curator_vault.rs" if [ -f "$curator" ]; then for fn in atomic_withdraw atomic_redeem; do ln=$(rg -n "$fn" "$curator" | head -n1 | cut -d: -f1) echo "---- $fn at line $ln ----" if [ -n "${ln:-}" ]; then start=$((ln-40)); end=$((ln+120)); sed -n "${start},${end}p" "$curator" fi done else echo "Missing $curator (file not present in repo)" fi echo echo "Inspect proxy_view max_withdraw/max_redeem computation in entrypoints" entry="contract/vault/soroban/src/contract/entrypoints.rs" if [ -f "$entry" ]; then rg -n 'proxy_view' "$entry" # show around the snippet area ~1623-1632 sed -n '1590,1670p' "$entry" fiRepository: Templar-Protocol/contracts
Length of output: 27682
🏁 Script executed:
# (end)Repository: Templar-Protocol/contracts
Length of output: 52
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Search for atomic withdraw/redeem + MIN_WITHDRAWAL_ASSETS" rg -n --hidden -S '\batomic_(withdraw|redeem)\b' contract/vault/soroban/src/contract contract/vault/kernel/src echo echo "Search for MIN_WITHDRAWAL_ASSETS / min_withdrawal_assets usages (limited)" rg -n '\bMIN_WITHDRAWAL_ASSETS\b|min_withdrawal_assets' contract/vault/kernel/src contract/vault/soroban/src/contract | head -n 200 echo echo "Locate handle_atomic_withdraw/handle_atomic_redeem in kernel actions" rg -n 'handle_atomic_(withdraw|redeem)' contract/vault/kernel/src/actions/mod.rs echo echo "Focused code windows for kernel atomic handlers" for fn in handle_atomic_withdraw handle_atomic_redeem; do ln=$(rg -n "$fn" contract/vault/kernel/src/actions/mod.rs | head -n1 | cut -d: -f1 || true) echo "---- $fn at line ${ln:-?} ----" if [ -n "${ln:-}" ]; then start=$((ln-40)); end=$((ln+120)); sed -n "${start},${end}p" contract/vault/kernel/src/actions/mod.rs fi done echo echo "Inspect curator_vault.rs atomic_withdraw/atomic_redeem" curator="contract/vault/soroban/src/contract/curator_vault.rs" if [ -f "$curator" ]; then for fn in atomic_withdraw atomic_redeem; do ln=$(rg -n "$fn" "$curator" | head -n1 | cut -d: -f1 || true) echo "---- $fn at line ${ln:-?} ----" if [ -n "${ln:-}" ]; then start=$((ln-40)); end=$((ln+160)); sed -n "${start},${end}p" "$curator" fi done else echo "Missing $curator (file not present in repo)" fi echo echo "Inspect proxy_view max_withdraw/max_redeem computation in entrypoints" entry="contract/vault/soroban/src/contract/entrypoints.rs" if [ -f "$entry" ]; then rg -n 'proxy_view' "$entry" | head -n 20 sed -n '1590,1670p' "$entry" fiRepository: Templar-Protocol/contracts
Length of output: 33102
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Files existence:" ls -1 contract/vault/soroban/src/contract | rg -n 'curator_vault\.rs|entrypoints\.rs' || true ls -1 contract/vault/kernel/src/actions/mod.rs || true echo echo "MIN_WITHDRAWAL_ASSETS usage sites in kernel + soroban contract:" rg -n '\bMIN_WITHDRAWAL_ASSETS\b|min_withdrawal_assets' contract/vault/kernel/src contract/vault/soroban/src/contract | head -n 80 echo echo "kernel atomic handler blocks (small):" for fn in handle_atomic_withdraw handle_atomic_redeem; do ln=$(rg -n "$fn" contract/vault/kernel/src/actions/mod.rs | head -n1 | cut -d: -f1 || true) echo "$fn at $ln" if [ -n "${ln:-}" ]; then sed -n "$((ln-20)),$((ln+60))p" contract/vault/kernel/src/actions/mod.rs fi done echo echo "curator_vault.rs atomic blocks (small):" curator="contract/vault/soroban/src/contract/curator_vault.rs" if [ -f "$curator" ]; then for fn in atomic_withdraw atomic_redeem; do ln=$(rg -n "$fn" "$curator" | head -n1 | cut -d: -f1 || true) echo "$fn at $ln" if [ -n "${ln:-}" ]; then sed -n "$((ln-20)),$((ln+80))p" "$curator" fi done else echo "Missing $curator" fi echo echo "entrypoints.rs proxy_view max limits section:" entry="contract/vault/soroban/src/contract/entrypoints.rs" sed -n '1585,1665p' "$entry" | rg -n 'max_withdraw|max_redeem|min_withdrawal|idle_assets|convert_to_shares|convert_to_assets|paused|op_state' -n || trueRepository: Templar-Protocol/contracts
Length of output: 17219
proxy_viewpreviews ignore the atomic-path dust floor (MIN_WITHDRAWAL_ASSETS).Kernel atomic handlers (
handle_atomic_withdraw/handle_atomic_redeem) only enforce “idle + non-zero” plus idle-liquidity/silippage bounds, and there is noconfig.min_withdrawal_assets/MIN_WITHDRAWAL_ASSETScheck in those atomic branches; themin_withdrawal_assetsguard shows up in the queued/idle-payout planning paths instead. Sinceproxy_view’smax_withdraw_value/max_redeem_valuebecomes positive based onstate.idle_assetsalone, previews can overstate whatwithdraw()/redeem()will accept via the atomic idle path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/soroban/src/contract/entrypoints.rs` around lines 1623 - 1632, proxy_view currently computes max_withdraw_value/max_redeem_value from state.idle_assets and owner_shares but omits the same atomic-path dust floor check used by handle_atomic_withdraw/handle_atomic_redeem; update the proxy_view logic (the block that calculates max_withdraw_u128 and max_redeem_u128 in entrypoints.rs) to apply the min_withdrawal guard (compare against config.min_withdrawal_assets or MIN_WITHDRAWAL_ASSETS) so that if the computed max_withdraw_u128 is below that dust floor it is treated as zero (and similarly ensure redeem follows the same floor rule), using the existing helpers convert_to_assets/convert_to_shares and state.idle_assets to decide whether to return zero or the to_i128(...) value.
|
@coderabbitai ur comments failed to post |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check review state and inline review comments on PR `#423`
gh pr view 423 --repo Templar-Protocol/contracts --json reviews,reviewDecision,state
echo ""
echo "=== REVIEW COMMENTS (inline) ==="
gh api repos/Templar-Protocol/contracts/pulls/423/comments --jq '[.[] | {id: .id, path: .path, line: .line, body: (.body | .[0:120])}]'
echo ""
echo "=== REVIEW THREADS ==="
gh api repos/Templar-Protocol/contracts/pulls/423/reviews --jq '[.[] | {id: .id, user: .user.login, state: .state, submitted_at: .submitted_at, body: (.body | .[0:80])}]'Length of output: 44322
Here are the 5 comments that failed to post inline, reproduced in full: 1 ·
|
@coderabbitai
The current CodeRabbit comments look like they’re based on an older branch state or a mistaken model of atomic exits. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
@CodeRabbit ai u need to approve the PR then since i resolved all ur comments :/ |
|
🧠 Learnings used✅ Action performedComments resolved and changes approved. |
…6066-2ebb-4b87-9e22-844ce7913a9c) Make Soroban withdrawal semantics explicit: withdraw/redeem stay atomic and idle-liquidity-only, request_withdraw remains the async queued exit path, and execute_withdraw only progresses fully backed queued withdrawals. Preserves PR #423 tree; amends the commit subject for Halborn remediation tracking.
1 participant
Summary
This PR makes Soroban vault withdrawal semantics explicit:
withdraw/redeemcommand flow is atomic and idle-liquidity-only.request_withdrawremains the async user path for queued exits.execute_withdrawis keeper/allocator progression and now only advances a cooled-down queue head when it is fully backed by idle assets.Validation
cargo test -p templar-soroban-shared-types -- --nocapturecargo test -p templar-vault-kernel --features all-actions -- --nocapturecargo test -p templar-soroban-runtime -- --nocapturejust -f contract/vault/soroban/justfile buildjust -f contract/vault/soroban/justfile size-budget-checkSize gate: runtime deploy WASM is
98522bytes, under the131072byte budget.This change is