feat(soroban): return typed execute receipts#422
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace the older VaultCommandResult model with tagged receipt types; vault execute returns ABI-encoded receipt bytes; proxy and consumers decode receipts for events/returns; shared-types, kernel, tests, CLI, README, and workspace lint policy updated. ChangesReceipt migration and downstream alignment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
carrion256
force-pushed
the
spr/refactor/vault-ergonomics/4641dafc
branch
from
April 29, 2026 13:23
b4f1402 to
411d3fd
Compare
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
from
April 29, 2026 13:31
2c89d70 to
1695e3c
Compare
carrion256
force-pushed
the
spr/refactor/vault-ergonomics/4641dafc
branch
from
April 30, 2026 15:11
91fcfc6 to
6db2fc4
Compare
Collaborator
Author
|
@coderabbitai review |
Collaborator
Author
|
@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. |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather the key files to give human reviewers a thorough description and design critique. 🏁 Script executed: Length of output: 82845 🏁 Script executed: Length of output: 3584 🏁 Script executed: Length of output: 7330 🏁 Script executed: Length of output: 59 🏁 Script executed: Length of output: 4043 🏁 Script executed: Length of output: 1206 🏁 Script executed: Length of output: 3 🏁 Script executed: Length of output: 7746
PR
|
| Type | Tag | Fields |
|---|---|---|
DepositReceipt |
0 |
shares_out: i128 |
RequestWithdrawReceipt |
1 |
request_id: u64, shares_escrowed: i128 |
ExecuteWithdrawReceipt |
2 |
Enum: NoPayout (inner 0) or Completed (inner 1) with request_id, owner, receiver, assets_out, shares_burned |
I128Receipt |
3 |
value: i128 |
EmptyReceipt |
4 |
(none) |
Every decoder enforces trailing-byte rejection via ensure_finished(), preventing corrupt/padded blobs from silently succeeding.
Layer 2 — entrypoints.rs: Receipt plumbing
execute_public_command() now dispatches each VaultCommand variant and returns an appropriate encoded receipt as Bytes. The execute() public entrypoint thus returns Bytes (previously a generic result). execute_withdraw_impl() maps the kernel-internal CompletedWithdrawal struct into ExecuteWithdrawReceipt::Completed, performing a reverse address lookup (kernel Address → SdkAddress → strkey string) before encoding.
Layer 3 — curator_vault.rs: Propagating completion data
execute_withdraw() now fills ExecuteWithdrawResult.completed: Option<CompletedWithdrawal>, carrying the payout owner, receiver, amounts, and request ID up from the kernel into the entrypoint layer.
Layer 4 — proxy-4626-soroban/src/contract.rs: Receipt decoding and event correction
The proxy now decodes each receipt type and:
- Emits
Depositevents with realshares_outfromDepositReceipt. - Emits
RedeemRequestevents with realrequest_id/shares_escrowedfromRequestWithdrawReceipt. - Only emits
Withdrawevents whenExecuteWithdrawReceipt::Completedis returned — the key correctness fix; no-op executions (NoPayout) are now silent.
Design Critique
✅ What's well done
- Trailing-byte rejection is correct and critical. Without
ensure_finished(), a malformed or re-used byte blob could decode without error. - Leading type-tag bytes on receipts prevent cross-type decoding (e.g., feeding a
DepositReceipttoRequestWithdrawReceipt::decode()fails fast). Thecommand_receipt_decoders_reject_wrong_tagstest covers this. Withdrawevent is now gated onCompleted— the pre-existing bug where a no-opexecute_withdraw()call could emit a phantom event is fixed.address_from_wire()validates StrKeys in the proxy via a hand-rolled base32 + CRC16-XMODEM check before callingAddress::from_str, preventing malformed strings from silently reaching Soroban.
⚠️ Design concerns for auditors
1. Overlapping numeric tag namespaces between commands and receipts
VaultCommand tag bytes (0=DepositWithMin, 1=RequestWithdraw, 2=ExecuteWithdraw, …) overlap numerically with receipt tag bytes (0=DepositReceipt, 1=RequestWithdrawReceipt, 2=ExecuteWithdrawReceipt). In the current design, commands and receipts flow in opposite directions so mixing is not possible today. But if a future consumer stores an encoded receipt and later tries to decode it as a command (or vice versa), the codec will happily accept it as long as the first byte matches — it will not reject it as a wrong message type. A simple MessageKind outer envelope (0x00 = command, 0x01 = receipt) or non-overlapping tag ranges would seal this.
2. Double-tag byte on ExecuteWithdrawReceipt
The structure is [0x02][0x00|0x01][payload] — outer type tag then variant tag. This is technically correct but unusual. Auditors may want to confirm that nothing in the decode path attempts to interpret the inner 0x00/0x01 as a length or option-presence flag by mistake.
3. Late storage lookup failure after effects are committed
In execute_withdraw_impl():
let owner = storage
.load_address(&completed.owner)
.ok_or(ContractError::StorageError)?;This lookup happens after vault.execute_withdraw() has run — meaning the asset transfer and share burn effects are already committed to the Soroban host (via execute_effects). If the address mapping is missing (e.g., due to a corrupt or unpopulated address store), the execute() call returns ContractError::StorageError and the transaction reverts. Because Soroban ledger writes are transactional, the revert would roll back the effects — but reviewers should confirm that this revert boundary holds cleanly, since the effects include token contract cross-calls.
4. u128 → i128 conversion on the hot path
assets_out and shares_burned are u128 in the kernel and are cast via to_i128() before being placed into ExecuteWithdrawReceipt::Completed. If the vault ever accumulates more than i128::MAX (≈ 1.7 × 10³⁸) base units in a single withdrawal, this conversion returns an error and the execute() call fails after effects are applied. While astronomically unlikely for any realistic token denomination, the asymmetry between kernel u128 accounting and the wire's i128 type is a latent risk that is not called out in the PR description.
5. No codec versioning; ensure_finished() blocks forward-compatibility
The codec has no version field. Adding a new field to any receipt type in a future PR would require a new tag value (breaking), a new inner variant, or changing the schema. Appending fields is blocked because ensure_finished() rejects trailing bytes. This is a reasonable tradeoff for simplicity now, but it should be explicitly documented as a design constraint so future contributors don't assume they can extend receipts in a backward-compatible way.
6. I128Receipt is semantically ambiguous for future consumers
Both Allocate and RefreshMarkets return I128Receipt { value: new_external_assets }. A receipt recipient cannot determine which command produced it without out-of-band context. This is fine for the current proxy (which never calls those commands), but is a footgun for any future integrator consuming raw execute() bytes.
7. Proxy execute_withdraw() returns ()
pub fn execute_withdraw(env: Env, caller: Address) -> Result<(), ContractError>The payout data (request_id, assets_out, receiver) is available in the decoded receipt but is silently dropped. A keeper or on-chain integrator calling this cannot inspect the outcome without parsing emitted events — which is not possible within a contract call. Returning at least the request_id (or a small struct) would make the API more composable for future keeper contract use.
8. withdraw() / redeem() return u64, not i128
These return request_id rather than shares burned / assets transferred, making them ERC-7540-style async functions, not ERC-4626 synchronous ones. This is architecturally correct given the queued withdrawal lifecycle, but auditors comparing against ERC-4626 ABI expectations should note this divergence explicitly.
9. Hand-rolled StrKey validator in proxy
validate_address_strkey() is a 50-line base32 + CRC16-XMODEM implementation. It should be compared against the Stellar StrKey spec (SEP-0023) to ensure edge cases (odd padding bits, wrong version bytes) are handled identically to how Soroban's native Address::from_str would behave. The version-byte check allows both ACCOUNT_VERSION (6 << 3 = 48) and CONTRACT_VERSION (2 << 3 = 16), which is correct for this context.
🧠 Learnings used
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/src/contract/entrypoints.rs : withdraw() and redeem() include an idle-only atomic path that bypasses the queued withdrawal lifecycle. Do not change queue semantics without checking both paths
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/** : Read README.md, STRIDE.md, SIZE_BUDGET.md, src/contract/entrypoints.rs, src/effects/mod.rs, and src/storage/mod.rs before making non-trivial changes to the Soroban vault runtime
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/src/contract/entrypoints.rs : Review auth on every privileged Soroban entrypoint. Do not rely on outer routing alone
Learnt from: carrion256
Repo: Templar-Protocol/contracts PR: 417
File: contract/vault/soroban/STRIDE.md:0-0
Timestamp: 2026-04-28T08:36:25.374Z
Learning: In `contract/vault/soroban/src/contract/entrypoints.rs` (Templar-Protocol/contracts, Soroban vault), `execute_governance()` decodes the payload and dispatches to `execute_governance_command()` **without** an auth check at the entrypoint. Authorization is enforced inside each command handler: `set_governance_config_impl`, `set_governance_policy_impl`, and `skim_impl` each call `require_governance(env, &caller)?` as their first action. There is no `require_auth()` wrapping `execute_governance()` itself. Any future `GovernanceCommand` variant added must include its own `require_governance` call; do not assume entrypoint-level auth exists.
Learnt from: carrion256
Repo: Templar-Protocol/contracts PR: 417
File: contract/vault/curator-primitives/src/policy/cooldown/mod.rs:17-23
Timestamp: 2026-04-27T09:33:55.992Z
Learning: In Templar-Protocol/contracts, the Soroban vault (`contract/vault/soroban/`) and NEAR vault (`contract/vault/near/`) are greenfield deployments with no live persisted state. Do not flag internal serialization format changes (e.g., field type changes in structs like `Cooldown`, `RefreshThrottle`, or policy state) as breaking migration risks — there is no existing stored data to break.
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/** : If behavior changes around governance bridging, withdrawal lifecycle, or share-token auth, update the docs in contract/vault/soroban/ in the same change
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:16.597Z
Learning: 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
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/src/** : Run `cargo test -p templar-soroban-runtime -- --nocapture`, integration tests, and property tests before submitting changes to the runtime
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:16.597Z
Learning: Run `cargo test -p templar-vault-contract -- --nocapture` as minimum verification when changing the vault contract
Learnt from: carrion256
Repo: Templar-Protocol/contracts PR: 417
File: contract/vault/soroban/src/contract/entrypoints.rs:806-836
Timestamp: 2026-04-28T08:29:30.164Z
Learning: In `contract/vault/soroban/src/contract/entrypoints.rs` (Templar-Protocol/contracts, Soroban vault), the `SorobanVaultContract::initialize()` entrypoint intentionally has no caller-authorization check. Deployment is expected to go through a controlled/factory flow where `initialize` is called atomically with contract deployment, eliminating any front-running window. Do not flag the missing auth check on `initialize` in this contract as a security issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/proxy-4626-soroban/src/contract.rs`:
- Around line 225-246: execute_withdraw currently discards the request_id from
ExecuteWithdrawReceipt::Completed and the Withdraw event lacks that id, breaking
traceability; update the Withdraw emission and plumbing to include request_id:
modify emit_withdraw_event to accept a request_id parameter (and its emitted
event to include the id), extract request_id from
ExecuteWithdrawReceipt::Completed inside execute_withdraw, and pass that
request_id into emit_withdraw_event (or alternatively return the request_id from
execute_withdraw if you prefer callers to receive it) so indexers/relayers can
correlate the completion to the original RedeemRequest.
- Around line 129-193: The delegated withdraw/redeem flow (functions withdraw
and redeem) allows caller != owner via require_auth_or_allowance but then calls
invoke_vault_execute with VaultCommand::RequestWithdraw which requires the
owner's signature in the vault, causing failure; as a short-term guard, add a
check in both withdraw and redeem that returns an error if caller != owner
(e.g., require!(caller == owner, ContractError::DelegatedNotSupported) or a new
explicit ContractError variant) before calling invoke_vault_execute so delegated
callers are rejected until end-to-end delegated auth is implemented.
In `@contract/proxy-4626-soroban/src/tests.rs`:
- Around line 114-148: Add a unit test in the same tests.rs that constructs a
vault command payload (use WireVaultCommand::DepositWithMin or
WireVaultCommand::ExecuteWithdraw), then arrange the mocked vault response to be
a well-formed DepositReceipt.encode() or ExecuteWithdrawReceipt.encode() with
extra trailing garbage bytes (append some arbitrary bytes) by writing that byte
vector into the mock storage keys (MockVaultDataKey::RecordedPayloads or
MockVaultDataKey::CompletedWithdrawal as appropriate), call the execute(env,
payload) helper, and assert the proxy rejects the malformed receipt (i.e., fails
strict decode or returns the error/empty-receipt path you expect); this will
exercise execute, WireVaultCommand decoding,
DepositReceipt/ExecuteWithdrawReceipt handling and ensure malformed
trailing-bytes are rejected.
In `@contract/vault/soroban/tests/integration_tests.rs`:
- Around line 175-191: The tests never decode the new
ExecuteWithdrawReceipt::Completed payload, so update the helper(s) to add a
typed execute_withdraw (or similar) assertion path: add a method (e.g.
execute_withdraw) alongside execute and execute_unit that calls
self.execute(command), decodes the returned bytes into ExecuteWithdrawReceipt
(handling decoding errors to map into
templar_soroban_runtime::ContractError::InvalidInput), and returns the typed
fields (owner, receiver, assets_out, shares_burned) or asserts on them in tests;
reference the existing execute, execute_unit, EmptyReceipt, and the enum variant
ExecuteWithdrawReceipt::Completed when locating where to add the decoding and
return/assert logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51b95be4-6558-4392-9690-472c530c2dce
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.gitignoreCargo.tomlcontract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/error.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/proxy-4626-soroban/tests/integration_tests.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/justfilecontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rs
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
from
June 2, 2026 11:40
1695e3c to
98b1d01
Compare
carrion256
changed the base branch from
spr/refactor/vault-ergonomics/4641dafc
to
halborn-audit
June 2, 2026 11:41
An error occurred while trying to automatically change base from
halborn-audit
to
dev
June 4, 2026 16:06
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
from
June 5, 2026 08:48
98b1d01 to
bbf10d7
Compare
Contributor
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 `@Cargo.toml`:
- Line 141: Revert the workspace-level Clippy override for unwraps by changing
the unwrap_used setting in Cargo.toml from "allow" back to a stricter policy
(e.g., "deny" or remove the override) so accidental unwrap() calls in non-test
code are guarded; ensure test files still compile by relying on per-crate or
test-module allowances instead of a workspace-wide "allow" for unwrap_used and
update any workspace-level comment to reflect the stricter policy (look for the
unwrap_used entry in Cargo.toml).
🪄 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: 23f7f040-85a1-4660-a88f-025cc8d4e1d3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.gitignoreCargo.tomlcontract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/error.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/proxy-4626-soroban/tests/integration_tests.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/justfilecontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rs
💤 Files with no reviewable changes (11)
- contract/vault/soroban/justfile
- contract/vault/soroban/tests/integration_tests.rs
- contract/vault/soroban/src/contract/types.rs
- contract/vault/soroban/src/contract/mod.rs
- contract/vault/kernel/src/actions/mod.rs
- contract/vault/soroban/governance/src/tests.rs
- contract/vault/soroban/shared-types/src/lib.rs
- contract/vault/soroban/tests/blend_e2e.rs
- contract/vault/soroban/src/contract/entrypoints.rs
- contract/vault/soroban/src/tests.rs
- contract/vault/soroban/src/contract/curator_vault.rs
Contributor
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: 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 `@Cargo.toml`:
- Line 141: Revert the workspace-level Clippy override for unwraps by changing
the unwrap_used setting in Cargo.toml from "allow" back to a stricter policy
(e.g., "deny" or remove the override) so accidental unwrap() calls in non-test
code are guarded; ensure test files still compile by relying on per-crate or
test-module allowances instead of a workspace-wide "allow" for unwrap_used and
update any workspace-level comment to reflect the stricter policy (look for the
unwrap_used entry in Cargo.toml).
🪄 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: 23f7f040-85a1-4660-a88f-025cc8d4e1d3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.gitignoreCargo.tomlcontract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/error.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/proxy-4626-soroban/tests/integration_tests.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/justfilecontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rs
💤 Files with no reviewable changes (11)
- contract/vault/soroban/justfile
- contract/vault/soroban/tests/integration_tests.rs
- contract/vault/soroban/src/contract/types.rs
- contract/vault/soroban/src/contract/mod.rs
- contract/vault/kernel/src/actions/mod.rs
- contract/vault/soroban/governance/src/tests.rs
- contract/vault/soroban/shared-types/src/lib.rs
- contract/vault/soroban/tests/blend_e2e.rs
- contract/vault/soroban/src/contract/entrypoints.rs
- contract/vault/soroban/src/tests.rs
- contract/vault/soroban/src/contract/curator_vault.rs
🛑 Comments failed to post (1)
Cargo.toml (1)
141-141:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-enable
clippy::unwrap_usedat workspace scope.Setting this to
allowremoves an important guardrail and makes accidentalunwrap()in non-test contract code easier to merge unnoticed.Suggested change
-unwrap_used = "allow" +unwrap_used = "warn"As per coding guidelines,
!(*.test.rs|test_*.rs|**/tests/**/*.rs)/**/*.rs: “Avoid introducingunwrap()in non-test Rust code unless the surrounding file already relies on an invariant and documents it clearly”.📝 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.unwrap_used = "warn"🤖 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 `@Cargo.toml` at line 141, Revert the workspace-level Clippy override for unwraps by changing the unwrap_used setting in Cargo.toml from "allow" back to a stricter policy (e.g., "deny" or remove the override) so accidental unwrap() calls in non-test code are guarded; ensure test files still compile by relying on per-crate or test-module allowances instead of a workspace-wide "allow" for unwrap_used and update any workspace-level comment to reflect the stricter policy (look for the unwrap_used entry in Cargo.toml).
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
from
June 5, 2026 13:03
bbf10d7 to
ba61e97
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
contract/proxy-4626-soroban/src/contract.rs (1)
297-314:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
request_idomitted fromWithdrawevent breaks traceability.The
ExecuteWithdrawReceipt::Completedincludesrequest_id(line 302 elides it with..), butemit_withdraw_event(lines 669-686) does not accept or emit this field. Indexers/relayers cannot correlate theWithdrawevent to the originalRedeemRequestevent when multiple withdrawals have similar amounts.🔗 Proposed fix to include request_id in event
Update the event helper signature and emission:
#[allow(deprecated)] pub(crate) fn emit_withdraw_event( env: &Env, sender: &Address, receiver: &Address, owner: &Address, + request_id: u64, assets: i128, shares: i128, ) { env.events().publish( ( symbol_short!("Withdraw"), sender.clone(), receiver.clone(), owner.clone(), + request_id, ), (assets, shares), ); }Then pass
request_idat the call site:if let ExecuteWithdrawReceipt::Completed { owner, receiver, assets_out, shares_burned, - .. + request_id, } = receipt { let owner = address_from_wire(&env, &owner)?; let receiver = address_from_wire(&env, &receiver)?; emit_withdraw_event( &env, &operator, &receiver, &owner, + request_id, assets_out, shares_burned, ); }🤖 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/proxy-4626-soroban/src/contract.rs` around lines 297 - 314, The Withdraw event is missing request_id: when matching ExecuteWithdrawReceipt::Completed, capture the request_id instead of eliding it and pass it into emit_withdraw_event; update the emit_withdraw_event function signature and its internal event emission to include a request_id field (and update any other call sites to pass the new parameter) so the emitted Withdraw event contains request_id for traceability (symbols: ExecuteWithdrawReceipt::Completed, emit_withdraw_event, request_id, RedeemRequest).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/vault/kernel/src/actions/mod.rs`:
- Around line 952-962: The helper has_actionable_withdrawal_liquidity currently
requires full funding (available_assets >= expected_assets) which blocks partial
payouts; change its logic to return true when there is any available idle
liquidity to make progress (e.g., available_assets > 0) so queued withdrawals
can take partial payouts and allow compute_idle_settlement to run; update the
helper function has_actionable_withdrawal_liquidity and ensure callers in the
queue classifier and plan_idle_payout still call this helper (so they allow
progress when available_assets > 0 rather than requiring full coverage).
- Around line 1384-1395: The issue is that handle_finish_allocating calls
complete_allocation(..., None) which forces the operation to end in Idle and
drops any queued withdrawal; change the call to propagate the pending withdrawal
identifier instead of None so the allocation completion resumes the withdrawal
workflow. Specifically, update handle_finish_allocating to call
complete_allocation(mem::take(&mut state.op_state), op_id, Some(<appropriate
withdraw/op id from the taken OpState>)) (or otherwise extract and pass the
queued withdraw id contained in the OpState) so apply_transition_result(state,
transition) receives a transition that continues the withdrawal-rebalance
lifecycle rather than hard-ending it.
In `@contract/vault/soroban/README.md`:
- Around line 188-196: Update the README keeper guidance to reference the actual
ExecuteWithdrawReceipt enum variants (NoPayout and Completed) instead of
non-existent fields; instruct keepers to derive the head request's expected
assets from the indexed WithdrawalRequested event stream, compare that to
current idle assets from proxy_view, treat a NoPayout receipt as a signal to
free liquidity and retry, and treat a Completed receipt with zero assets_paid
(or equivalent within Completed) as an unexpected no-progress state to alert on;
also mention that the A-002 fix aims to reject zero-progress transitions before
persistence so automation should not rely on a bare Unit success.
- Around line 178-186: The README's description of ExecuteWithdrawReceipt is
incorrect; update the docs to reflect that ExecuteWithdrawReceipt is a
two-variant enum (ExecuteWithdrawReceipt::NoPayout with tag 0 and
ExecuteWithdrawReceipt::Completed with tag 1) and list the Completed fields as
request_id: u64, owner: Address, receiver: Address, assets_out: i128,
shares_burned: i128 instead of
op_state_before/op_state_after/assets_transferred/events_emitted; reference the
typed execute_withdraw entrypoint and the generic Execute(payload) path
(VaultCommand::ExecuteWithdraw) in the text so readers can match the documented
receipt shape to the receipt codec and keeper integration expectations.
In `@contract/vault/soroban/shared-types/src/lib.rs`:
- Around line 509-521: The public receipt ExecuteWithdrawReceipt::Completed
currently narrows assets_out and shares_burned to i128 which can overflow at the
ABI boundary; change those fields to u128 to match the runtime's unsigned domain
(and update any related pattern matches/consumers of ExecuteWithdrawReceipt and
the entrypoint that serializes this enum to avoid downcasting), or alternatively
add validation where withdrawals are queued (e.g., in the function that creates
queued withdrawals) to reject amounts > i128::MAX so the signed narrowing can
never occur; prefer the first approach (change ExecuteWithdrawReceipt::Completed
assets_out and shares_burned to u128 and remove any downcasts in the entrypoint)
to make the state representable and safe.
In `@contract/vault/soroban/src/tests.rs`:
- Around line 364-366: The tests are decoding the DepositWithMin variant using
I128Receipt; change those decodes to use DepositReceipt instead to match the
receipt ABI. Locate occurrences where DepositWithMin is passed to decode as
I128Receipt (e.g., the decode calls near the symbols DepositWithMin, I128Receipt
and the surrounding test helpers in tests.rs around the failing cases) and
replace the type parameter with DepositReceipt so the tag and ABI validation
align with the typed receipt contract.
---
Duplicate comments:
In `@contract/proxy-4626-soroban/src/contract.rs`:
- Around line 297-314: The Withdraw event is missing request_id: when matching
ExecuteWithdrawReceipt::Completed, capture the request_id instead of eliding it
and pass it into emit_withdraw_event; update the emit_withdraw_event function
signature and its internal event emission to include a request_id field (and
update any other call sites to pass the new parameter) so the emitted Withdraw
event contains request_id for traceability (symbols:
ExecuteWithdrawReceipt::Completed, emit_withdraw_event, request_id,
RedeemRequest).
🪄 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: af492c10-e39e-4b22-913e-9eae204533da
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
contract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/README.mdcontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/scripts/vault_payload.pycontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rs
Contributor
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: 7
♻️ Duplicate comments (1)
contract/proxy-4626-soroban/src/contract.rs (1)
297-314:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
request_idomitted fromWithdrawevent breaks traceability.The
ExecuteWithdrawReceipt::Completedincludesrequest_id(line 302 elides it with..), butemit_withdraw_event(lines 669-686) does not accept or emit this field. Indexers/relayers cannot correlate theWithdrawevent to the originalRedeemRequestevent when multiple withdrawals have similar amounts.🔗 Proposed fix to include request_id in event
Update the event helper signature and emission:
#[allow(deprecated)] pub(crate) fn emit_withdraw_event( env: &Env, sender: &Address, receiver: &Address, owner: &Address, + request_id: u64, assets: i128, shares: i128, ) { env.events().publish( ( symbol_short!("Withdraw"), sender.clone(), receiver.clone(), owner.clone(), + request_id, ), (assets, shares), ); }Then pass
request_idat the call site:if let ExecuteWithdrawReceipt::Completed { owner, receiver, assets_out, shares_burned, - .. + request_id, } = receipt { let owner = address_from_wire(&env, &owner)?; let receiver = address_from_wire(&env, &receiver)?; emit_withdraw_event( &env, &operator, &receiver, &owner, + request_id, assets_out, shares_burned, ); }🤖 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/proxy-4626-soroban/src/contract.rs` around lines 297 - 314, The Withdraw event is missing request_id: when matching ExecuteWithdrawReceipt::Completed, capture the request_id instead of eliding it and pass it into emit_withdraw_event; update the emit_withdraw_event function signature and its internal event emission to include a request_id field (and update any other call sites to pass the new parameter) so the emitted Withdraw event contains request_id for traceability (symbols: ExecuteWithdrawReceipt::Completed, emit_withdraw_event, request_id, RedeemRequest).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/vault/kernel/src/actions/mod.rs`:
- Around line 952-962: The helper has_actionable_withdrawal_liquidity currently
requires full funding (available_assets >= expected_assets) which blocks partial
payouts; change its logic to return true when there is any available idle
liquidity to make progress (e.g., available_assets > 0) so queued withdrawals
can take partial payouts and allow compute_idle_settlement to run; update the
helper function has_actionable_withdrawal_liquidity and ensure callers in the
queue classifier and plan_idle_payout still call this helper (so they allow
progress when available_assets > 0 rather than requiring full coverage).
- Around line 1384-1395: The issue is that handle_finish_allocating calls
complete_allocation(..., None) which forces the operation to end in Idle and
drops any queued withdrawal; change the call to propagate the pending withdrawal
identifier instead of None so the allocation completion resumes the withdrawal
workflow. Specifically, update handle_finish_allocating to call
complete_allocation(mem::take(&mut state.op_state), op_id, Some(<appropriate
withdraw/op id from the taken OpState>)) (or otherwise extract and pass the
queued withdraw id contained in the OpState) so apply_transition_result(state,
transition) receives a transition that continues the withdrawal-rebalance
lifecycle rather than hard-ending it.
In `@contract/vault/soroban/README.md`:
- Around line 188-196: Update the README keeper guidance to reference the actual
ExecuteWithdrawReceipt enum variants (NoPayout and Completed) instead of
non-existent fields; instruct keepers to derive the head request's expected
assets from the indexed WithdrawalRequested event stream, compare that to
current idle assets from proxy_view, treat a NoPayout receipt as a signal to
free liquidity and retry, and treat a Completed receipt with zero assets_paid
(or equivalent within Completed) as an unexpected no-progress state to alert on;
also mention that the A-002 fix aims to reject zero-progress transitions before
persistence so automation should not rely on a bare Unit success.
- Around line 178-186: The README's description of ExecuteWithdrawReceipt is
incorrect; update the docs to reflect that ExecuteWithdrawReceipt is a
two-variant enum (ExecuteWithdrawReceipt::NoPayout with tag 0 and
ExecuteWithdrawReceipt::Completed with tag 1) and list the Completed fields as
request_id: u64, owner: Address, receiver: Address, assets_out: i128,
shares_burned: i128 instead of
op_state_before/op_state_after/assets_transferred/events_emitted; reference the
typed execute_withdraw entrypoint and the generic Execute(payload) path
(VaultCommand::ExecuteWithdraw) in the text so readers can match the documented
receipt shape to the receipt codec and keeper integration expectations.
In `@contract/vault/soroban/shared-types/src/lib.rs`:
- Around line 509-521: The public receipt ExecuteWithdrawReceipt::Completed
currently narrows assets_out and shares_burned to i128 which can overflow at the
ABI boundary; change those fields to u128 to match the runtime's unsigned domain
(and update any related pattern matches/consumers of ExecuteWithdrawReceipt and
the entrypoint that serializes this enum to avoid downcasting), or alternatively
add validation where withdrawals are queued (e.g., in the function that creates
queued withdrawals) to reject amounts > i128::MAX so the signed narrowing can
never occur; prefer the first approach (change ExecuteWithdrawReceipt::Completed
assets_out and shares_burned to u128 and remove any downcasts in the entrypoint)
to make the state representable and safe.
In `@contract/vault/soroban/src/tests.rs`:
- Around line 364-366: The tests are decoding the DepositWithMin variant using
I128Receipt; change those decodes to use DepositReceipt instead to match the
receipt ABI. Locate occurrences where DepositWithMin is passed to decode as
I128Receipt (e.g., the decode calls near the symbols DepositWithMin, I128Receipt
and the surrounding test helpers in tests.rs around the failing cases) and
replace the type parameter with DepositReceipt so the tag and ABI validation
align with the typed receipt contract.
---
Duplicate comments:
In `@contract/proxy-4626-soroban/src/contract.rs`:
- Around line 297-314: The Withdraw event is missing request_id: when matching
ExecuteWithdrawReceipt::Completed, capture the request_id instead of eliding it
and pass it into emit_withdraw_event; update the emit_withdraw_event function
signature and its internal event emission to include a request_id field (and
update any other call sites to pass the new parameter) so the emitted Withdraw
event contains request_id for traceability (symbols:
ExecuteWithdrawReceipt::Completed, emit_withdraw_event, request_id,
RedeemRequest).
🪄 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: af492c10-e39e-4b22-913e-9eae204533da
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
contract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/README.mdcontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/scripts/vault_payload.pycontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rs
🛑 Comments failed to post (7)
contract/vault/kernel/src/actions/mod.rs (2)
952-962:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't require full liquidity before a queued withdrawal can make progress.
This helper now returns
trueonly whenavailable_assets >= expected_assets, and both the queue classifier andplan_idle_payout()consult it beforecompute_idle_settlement(). That removes the partial-payout path entirely: a withdrawal that could legally burn part of the escrow and transfer some idle assets now stays blocked until the vault can fund the full request.Also applies to: 1881-1891
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/kernel/src/actions/mod.rs` around lines 952 - 962, The helper has_actionable_withdrawal_liquidity currently requires full funding (available_assets >= expected_assets) which blocks partial payouts; change its logic to return true when there is any available idle liquidity to make progress (e.g., available_assets > 0) so queued withdrawals can take partial payouts and allow compute_idle_settlement to run; update the helper function has_actionable_withdrawal_liquidity and ensure callers in the queue classifier and plan_idle_payout still call this helper (so they allow progress when available_assets > 0 rather than requiring full coverage).
1384-1395:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
FinishAllocatingnow breaks the withdrawal-rebalance lifecycle.
complete_withdraw_allocation()still ends by dispatchingFinishAllocating, but this path now hardcodescomplete_allocation(..., None). After liquidity is rebalanced back from a market, the vault drops toIdleinstead of resuming the queued withdrawal, so the request now needs an extra explicitexecute_withdrawhop. Based on learnings: Watch forOpStatetransitions, escrow accounting, keeper-routed withdrawals, callback ordering, idle-balance resync, fee accrual, and reconciliation after partial failures in the vault contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/kernel/src/actions/mod.rs` around lines 1384 - 1395, The issue is that handle_finish_allocating calls complete_allocation(..., None) which forces the operation to end in Idle and drops any queued withdrawal; change the call to propagate the pending withdrawal identifier instead of None so the allocation completion resumes the withdrawal workflow. Specifically, update handle_finish_allocating to call complete_allocation(mem::take(&mut state.op_state), op_id, Some(<appropriate withdraw/op id from the taken OpState>)) (or otherwise extract and pass the queued withdraw id contained in the OpState) so apply_transition_result(state, transition) receives a transition that continues the withdrawal-rebalance lifecycle rather than hard-ending it.contract/vault/soroban/README.md (2)
178-186:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Documented receipt fields do not match actual ExecuteWithdrawReceipt structure.
The documentation describes
ExecuteWithdrawReceiptas containingop_state_before,op_state_after,assets_transferred, andevents_emittedfields. Per the PR objectives and receipt codec implementation,ExecuteWithdrawReceiptis actually a two-variant enum:
ExecuteWithdrawReceipt::NoPayout(inner tag 0): no fieldsExecuteWithdrawReceipt::Completed(inner tag 1):request_id: u64,owner: Address,receiver: Address,assets_out: i128,shares_burned: i128This mismatch will break keeper integrations that attempt to decode the receipt using the documented field names.
📝 Proposed documentation fix
-The typed `execute_withdraw` entrypoint keeps returning `Result<(), _>` for the stable contract -ABI. The generic `execute(payload)` command path returns -`ExecuteWithdrawReceipt` bytes for `VaultCommand::ExecuteWithdraw`, with: - -- `op_state_before` and `op_state_after`: kernel operation-state codes - (`0 = Idle`, `1 = Allocating`, `2 = Withdrawing`, `3 = Refreshing`, - `4 = Payout`). -- `assets_transferred`: assets paid to receivers during this command. -- `events_emitted`: kernel/runtime events emitted while processing the command. +The typed `execute_withdraw` entrypoint keeps returning `Result<(), _>` for the stable contract +ABI. The generic `execute(payload)` command path returns `ExecuteWithdrawReceipt` bytes for +`VaultCommand::ExecuteWithdraw`. `ExecuteWithdrawReceipt` is a two-variant enum: + +- `ExecuteWithdrawReceipt::NoPayout` (inner tag 0): No withdrawal was completed; the queue head + was not cooled down or insufficient idle liquidity was available. +- `ExecuteWithdrawReceipt::Completed` (inner tag 1): A withdrawal was completed with fields: + - `request_id: u64`: the queue request that was paid out + - `owner: Address`: the original withdrawal requester + - `receiver: Address`: the destination address that received assets + - `assets_out: i128`: underlying assets transferred to the receiver + - `shares_burned: i128`: escrowed shares burned during settlement🤖 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/README.md` around lines 178 - 186, The README's description of ExecuteWithdrawReceipt is incorrect; update the docs to reflect that ExecuteWithdrawReceipt is a two-variant enum (ExecuteWithdrawReceipt::NoPayout with tag 0 and ExecuteWithdrawReceipt::Completed with tag 1) and list the Completed fields as request_id: u64, owner: Address, receiver: Address, assets_out: i128, shares_burned: i128 instead of op_state_before/op_state_after/assets_transferred/events_emitted; reference the typed execute_withdraw entrypoint and the generic Execute(payload) path (VaultCommand::ExecuteWithdraw) in the text so readers can match the documented receipt shape to the receipt codec and keeper integration expectations.
188-196:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix keeper guidance to reference actual receipt structure.
This section references
assets_transferredandop_state_afterfields that do not exist inExecuteWithdrawReceipt. The guidance should be rewritten to work with the actual two-variant enum structure (NoPayout vs Completed).📝 Proposed guidance fix
-Keepers should treat a failed `ExecuteWithdraw` with the kernel low-liquidity error as a signal to -free market liquidity before retrying. The error is intentionally compact and does not carry -`needed` / `available` amounts; automation should derive the head request's `expected_assets` from -the indexed `WithdrawalRequested` event stream and compare it with the current idle assets exposed -by `proxy_view` before choosing how much liquidity to free. A successful command with -`assets_transferred == 0` and a non-idle `op_state_after` should be alerted as an unexpected -no-progress withdrawal state. The A-002 fix is intended to reject that zero-progress transition -before it is persisted, but the structured result keeps automation from relying on a bare `Unit` -success. +Keepers should treat a failed `ExecuteWithdraw` with the kernel low-liquidity error as a signal to +free market liquidity before retrying. The error is intentionally compact and does not carry +`needed` / `available` amounts; automation should derive the head request's `expected_assets` from +the indexed `WithdrawalRequested` event stream and compare it with the current idle assets exposed +by `proxy_view` before choosing how much liquidity to free. A successful `ExecuteWithdraw` that +returns `ExecuteWithdrawReceipt::NoPayout` indicates that no withdrawal was completed (queue head +not cooled down or insufficient idle liquidity); keepers should verify the reason and free +liquidity if needed before retrying. `ExecuteWithdrawReceipt::Completed` with `assets_out > 0` +confirms successful payout; the `Completed` variant carries the full settlement details for +reconciliation.🤖 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/README.md` around lines 188 - 196, Update the README keeper guidance to reference the actual ExecuteWithdrawReceipt enum variants (NoPayout and Completed) instead of non-existent fields; instruct keepers to derive the head request's expected assets from the indexed WithdrawalRequested event stream, compare that to current idle assets from proxy_view, treat a NoPayout receipt as a signal to free liquidity and retry, and treat a Completed receipt with zero assets_paid (or equivalent within Completed) as an unexpected no-progress state to alert on; also mention that the A-002 fix aims to reject zero-progress transitions before persistence so automation should not rely on a bare Unit success.contract/vault/soroban/shared-types/src/lib.rs (1)
509-521:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep completed-withdraw receipt amounts in the same unsigned domain as the runtime.
CompletedWithdrawalnow carriesassets_outandshares_burnedasu128, but this public receipt narrows both toi128. The new entrypoint has to downcast before replying, so a valid queued withdrawal abovei128::MAXwill fail at the ABI boundary instead of completing. Either keep these fields unsigned on the wire or reject oversized requests when they are created so this state cannot reach execution. As per coding guidelines: "Prefer stronger types over loosely constrained values in Rust" and "make invalid states unrepresentable."🤖 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 509 - 521, The public receipt ExecuteWithdrawReceipt::Completed currently narrows assets_out and shares_burned to i128 which can overflow at the ABI boundary; change those fields to u128 to match the runtime's unsigned domain (and update any related pattern matches/consumers of ExecuteWithdrawReceipt and the entrypoint that serializes this enum to avoid downcasting), or alternatively add validation where withdrawals are queued (e.g., in the function that creates queued withdrawals) to reject amounts > i128::MAX so the signed narrowing can never occur; prefer the first approach (change ExecuteWithdrawReceipt::Completed assets_out and shares_burned to u128 and remove any downcasts in the entrypoint) to make the state representable and safe.contract/vault/soroban/src/contract/entrypoints.rs (1)
1660-1666:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
max_redeemis rounded the wrong way.Using
convert_to_shares(idle_assets)under-reports redeemable shares because the redeem path floors on the asset side. For example, when one share would redeem to exactly one idle asset after flooring, this code can still return0here. Derivemax_redeemfrom the redeem predicate instead of the inverse floor conversion.contract/vault/soroban/src/tests.rs (1)
364-366:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDecode
DepositWithMinasDepositReceiptinstead ofI128Receipt.
DepositWithMinis being decoded withI128Receiptat Line 2675 and Line 2762. That conflicts with the typed receipt contract (deposit has its own receipt type), so these tests can fail on tag mismatch or validate the wrong ABI.Suggested fix
use templar_soroban_shared_types::{ - EmptyReceipt, ExecuteWithdrawReceipt, ExecuteWithdrawStatus, GovernanceCommand, + DepositReceipt, EmptyReceipt, ExecuteWithdrawReceipt, ExecuteWithdrawStatus, GovernanceCommand, I128Receipt, RequestWithdrawReceipt, VaultCommand, GOVERNANCE_CONFIG_KIND_IDLE_RESYNC_COOLDOWN, GOVERNANCE_CONFIG_KIND_VIRTUAL_OFFSETS, GOVERNANCE_CONFIG_KIND_WITHDRAWAL_COOLDOWN, }; @@ fn decode_i128_receipt(bytes: &Bytes) -> i128 { I128Receipt::decode(&bytes.to_alloc_vec()) .expect("decode i128 receipt") .value } + + fn decode_deposit_receipt(bytes: &Bytes) -> DepositReceipt { + DepositReceipt::decode(&bytes.to_alloc_vec()).expect("decode deposit receipt") + } @@ - let minted = decode_i128_receipt(&minted); + let minted = decode_deposit_receipt(&minted).shares_out; @@ - assert_eq!(decode_i128_receipt(&deposit_receipt), deposit_assets); + assert_eq!( + decode_deposit_receipt(&deposit_receipt).shares_out, + deposit_assets + );Also applies to: 468-472, 2675-2675, 2752-2763
🤖 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/tests.rs` around lines 364 - 366, The tests are decoding the DepositWithMin variant using I128Receipt; change those decodes to use DepositReceipt instead to match the receipt ABI. Locate occurrences where DepositWithMin is passed to decode as I128Receipt (e.g., the decode calls near the symbols DepositWithMin, I128Receipt and the surrounding test helpers in tests.rs around the failing cases) and replace the type parameter with DepositReceipt so the tag and ABI validation align with the typed receipt contract.
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
3 times, most recently
from
June 5, 2026 13:52
3c9f43e to
8194eb6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
contract/proxy-4626-soroban/src/tests.rs (1)
114-156: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMalformed-receipt regression test is still missing.
All mocked vault responses are currently well-formed, so the proxy tests never exercise the strict
ensure_finished()decode path that rejects trailing bytes. A test case returning a valid receipt (e.g.,DepositReceiptorExecuteWithdrawReceipt) with appended garbage bytes would catch regressions where the proxy accidentally stops rejecting malformed payloads.As per coding guidelines,
**/tests/**/*.rs: Write comprehensive unit tests for new, non-trivial Rust logic.🧪 Suggested test structure
Add a unit test that:
- Constructs a
WireVaultCommand::DepositWithMinorWireVaultCommand::ExecuteWithdrawpayload- Arranges the mock to return a well-formed
DepositReceipt.encode()orExecuteWithdrawReceipt.encode()with extra trailing garbage bytes appended by extendingMockVaultDataKey::RecordedPayloadsor adding a new mock storage key- Calls the proxy's
depositorexecute_withdrawvia the fixture- Asserts the proxy rejects the malformed receipt (returns an error or expected fallback behavior)
This exercises the
ensure_finished()validation and prevents future regressions.🤖 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/proxy-4626-soroban/src/tests.rs` around lines 114 - 156, Add a unit test that mocks a vault response containing a valid encoded receipt plus trailing garbage bytes so the proxy's ensure_finished() path is exercised; specifically, in tests for execute (which pushes to MockVaultDataKey::RecordedPayloads) construct a WireVaultCommand::DepositWithMin or ::ExecuteWithdraw payload, arrange the mock RecordedPayloads to return DepositReceipt::encode() or ExecuteWithdrawReceipt::encode() with extra bytes appended, invoke the proxy deposit or execute_withdraw fixture, and assert the call rejects the malformed receipt (error/fallback) to ensure ensure_finished() enforces no trailing data.
🤖 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/shared-types/src/lib.rs`:
- Around line 1265-1288: Add tests exercising malformed ExecuteWithdrawReceipt
decoding: create an encoded ExecuteWithdrawReceipt with a wrong inner
discriminant to assert ExecuteWithdrawReceipt::decode(...) returns
Err(CodecError::InvalidTag), and create a truncated encoding for the Completed
variant (e.g., drop bytes from a Completed payload) to assert
ExecuteWithdrawReceipt::decode(...) returns Err(CodecError::InvalidEncoding);
place these alongside the existing command_receipt_decoders_* tests and reuse
the same encode/decode helpers so the new tests reference
ExecuteWithdrawReceipt, Completed, decode, and the CodecError variants.
- Around line 514-520: The Completed receipt variant currently uses signed i128
for assets_out and shares_burned which can reject valid u128 kernel values;
change those fields in the Completed enum to unsigned (u128 or a dedicated
newtype like Amount) and update any related ABI/serialization code so the
receipt ABI expresses non-negative quantities. Then update
contract/vault/soroban/src/contract/entrypoints.rs to stop narrowing kernel u128
values into i128 and instead construct the receipt using the new unsigned types
(or validated newtype) with only necessary checks for overflow in serialization,
ensuring all conversions/serde traits align with the new types.
In `@contract/vault/soroban/src/tests.rs`:
- Around line 364-365: The tests decode DepositWithMin receipts using the wrong
codec (I128Receipt) causing mismatches; locate the decoding branches that handle
the DepositWithMin path/variant (where I128Receipt is used) and replace the
decoder/type usage with DepositReceipt so deposit receipts are decoded as their
typed DepositReceipt; apply the same change for the other occurrences mentioned
(the other DepositWithMin decoding sites in the file that reference I128Receipt)
to ensure all deposit receipts use DepositReceipt.
In `@contract/vault/soroban/tests/blend_e2e.rs`:
- Around line 36-43: The match in execute_command currently uses a wildcard
branch decoding everything not VaultCommand::DepositWithMin as I128Receipt,
which can hide invalid command/receipt pairings; update the match to enumerate
the exact VaultCommand variants used by this test file (e.g., list each command
like VaultCommand::Withdraw { .. }, VaultCommand::SomeOtherCommand { .. } that
expect I128Receipt) and decode each explicitly with I128Receipt::decode(...).
Keep DepositWithMin using DepositReceipt::decode(...) and preserve the map_err
to templar_soroban_runtime::ContractError::InvalidInput for all decode failures
so only valid command/receipt combinations are accepted.
---
Duplicate comments:
In `@contract/proxy-4626-soroban/src/tests.rs`:
- Around line 114-156: Add a unit test that mocks a vault response containing a
valid encoded receipt plus trailing garbage bytes so the proxy's
ensure_finished() path is exercised; specifically, in tests for execute (which
pushes to MockVaultDataKey::RecordedPayloads) construct a
WireVaultCommand::DepositWithMin or ::ExecuteWithdraw payload, arrange the mock
RecordedPayloads to return DepositReceipt::encode() or
ExecuteWithdrawReceipt::encode() with extra bytes appended, invoke the proxy
deposit or execute_withdraw fixture, and assert the call rejects the malformed
receipt (error/fallback) to ensure ensure_finished() enforces no trailing data.
🪄 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: 0f9505b5-2e34-4b62-acfd-8d96ca572449
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
contract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/README.mdcontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/scripts/vault_payload.pycontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rs
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
2 times, most recently
from
June 5, 2026 14:10
35919b1 to
f64d0f3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/src/contract/curator_vault.rs (1)
560-606:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve completed-withdraw address mappings before payout effects.
CompletedWithdrawalnow carries raw kernelAddressvalues, but the receipt ABI eventually serializesownerandreceiveras strings. This path starts the payout flow before proving both mappings exist, so a missing owner mapping can turn an otherwise valid withdrawal into a lateexecute_withdrawerror. Preflight both mappings here beforewithdrawal_settled/ transfer work, or store already-mapped wire values in the completion payload.♻️ Minimal hardening
fn complete_withdrawal_from_idle( &mut self, now_ns: u64, ) -> Result<ExecuteWithdrawResult, RuntimeError> { let min_withdrawal_assets = self.kernel_config().min_withdrawal_assets; let idle_payout = transition_to_runtime(plan_idle_payout(self.state()?, min_withdrawal_assets))?; + self.ensure_mapped(&idle_payout.owner)?; + self.ensure_mapped(&idle_payout.receiver)?; + let assets_out = idle_payout.assets_out; let burn_shares = idle_payout.burn_shares; let op_id = idle_payout.op_id; let completed = CompletedWithdrawal { request_id: idle_payout.request_id,Based on learnings, evaluate edge cases around asynchronous receipt execution and cross-contract call ordering in Rust smart contract code; watch for TOCTOU-style issues.
🤖 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/curator_vault.rs` around lines 560 - 606, The code constructs CompletedWithdrawal with raw kernel Address values (idle_payout.owner and idle_payout.receiver) but then begins the payout flow (calling withdrawal_settled, executing effects and TransferAssets) before ensuring those addresses are mapped, which can cause late execute_withdraw failures; fix by pre-flighting/mapping the owner and receiver addresses (call ensure_effect_addresses_mapped or otherwise convert/map idle_payout.owner and idle_payout.receiver into wire/mapped forms) immediately after obtaining idle_payout and before calling withdrawal_settled/execute_effects/creating transfer_effects, or alternatively populate CompletedWithdrawal with the already-mapped wire values so subsequent code uses mapped addresses; update uses of CompletedWithdrawal, withdrawal_settled, transfer_effects, and ensure_effect_addresses_mapped accordingly.
♻️ Duplicate comments (1)
contract/vault/soroban/tests/blend_e2e.rs (1)
36-42: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMake unsupported command/receipt pairs impossible in this helper.
The wildcard arm still routes any future
VaultCommandthroughI128Receipt, so an unsupported command becomes a generic decode failure instead of an explicit mismatch. Match only the variants this file actually uses and make the fallback unreachable.As per coding guidelines: "
**/*.rs: Prefer parsing over validation in Rust code; express invariants in types wherever practical, and make invalid states unrepresentable"🤖 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/tests/blend_e2e.rs` around lines 36 - 42, The match currently routes all non-DepositWithMin VaultCommand variants through I128Receipt, hiding unsupported command/receipt mismatches; update the match on VaultCommand to list only the concrete variants this test actually expects (e.g., VaultCommand::DepositWithMin => decode as DepositReceipt and each other used command => decode with the correct receipt type like I128Receipt) and replace the wildcard arm with an explicit unreachable!() or panic with a clear message (e.g., unreachable!("unsupported VaultCommand in helper")) so future unsupported commands fail loudly; keep the existing .map_err(|_| templar_soroban_runtime::ContractError::InvalidInput) behavior for decode failures.
🤖 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/proxy-4626-soroban/src/contract.rs`:
- Around line 702-764: Duplicate StrKey logic: move the shared
validate_address_strkey and crc16_xmodem implementations into a single shared
module and import it from both places (instead of copying). Create a small
common module (e.g., a new helpers/strkey module or crate) that exposes
validate_address_strkey and crc16_xmodem, update contract.rs to call that shared
functions (same names), and update
contract/vault/soroban/src/contract/helpers.rs to re-export or call the same
shared implementations; do not add the stellar-strkey crate (keep the inline
implementation to avoid WASM size regression).
In `@contract/vault/soroban/tests/integration_tests.rs`:
- Around line 1225-1292: Add a new #[rstest] test (e.g.,
soroban_contract_execute_withdraw_decodes_no_payout_receipt) that mirrors the
existing Completed test but drives the path that yields
ExecuteWithdrawReceipt::NoPayout: use the SorobanContractFixture, VaultProxy,
and StellarAssetClient to create a withdrawal request that will produce no
payout (for example by arranging deposits/shares or balances so assets_out would
be zero), advance the ledger timestamp as needed, call
proxy.execute_withdraw(&curator) inside env.as_contract, match the returned
receipt against ExecuteWithdrawReceipt::NoPayout and assert the expected fields
(request_id, owner/receiver wire values, and any zero/empty payout indicators);
reference and reuse symbols ExecuteWithdrawReceipt::NoPayout, execute_withdraw,
VaultCommand::RequestWithdraw, VaultProxy, and MIN_WITHDRAWAL_ASSETS to locate
and implement the test.
---
Outside diff comments:
In `@contract/vault/soroban/src/contract/curator_vault.rs`:
- Around line 560-606: The code constructs CompletedWithdrawal with raw kernel
Address values (idle_payout.owner and idle_payout.receiver) but then begins the
payout flow (calling withdrawal_settled, executing effects and TransferAssets)
before ensuring those addresses are mapped, which can cause late
execute_withdraw failures; fix by pre-flighting/mapping the owner and receiver
addresses (call ensure_effect_addresses_mapped or otherwise convert/map
idle_payout.owner and idle_payout.receiver into wire/mapped forms) immediately
after obtaining idle_payout and before calling
withdrawal_settled/execute_effects/creating transfer_effects, or alternatively
populate CompletedWithdrawal with the already-mapped wire values so subsequent
code uses mapped addresses; update uses of CompletedWithdrawal,
withdrawal_settled, transfer_effects, and ensure_effect_addresses_mapped
accordingly.
---
Duplicate comments:
In `@contract/vault/soroban/tests/blend_e2e.rs`:
- Around line 36-42: The match currently routes all non-DepositWithMin
VaultCommand variants through I128Receipt, hiding unsupported command/receipt
mismatches; update the match on VaultCommand to list only the concrete variants
this test actually expects (e.g., VaultCommand::DepositWithMin => decode as
DepositReceipt and each other used command => decode with the correct receipt
type like I128Receipt) and replace the wildcard arm with an explicit
unreachable!() or panic with a clear message (e.g., unreachable!("unsupported
VaultCommand in helper")) so future unsupported commands fail loudly; keep the
existing .map_err(|_| templar_soroban_runtime::ContractError::InvalidInput)
behavior for decode failures.
🪄 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: a16e5701-7c3e-46e2-8030-bdc6cd467d8e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.tomlcontract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/vault/curator-primitives/src/policy/cap_group/mod.rscontract/vault/curator-primitives/src/policy/cooldown/mod.rscontract/vault/curator-primitives/src/policy/state/mod.rscontract/vault/curator-primitives/src/policy/supply_queue/mod.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/README.mdcontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/scripts/vault_payload.pycontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rstest-utils/examples/registry_add_version_args.rstest-utils/src/lib.rs
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
3 times, most recently
from
June 5, 2026 22:32
9cd1e3c to
9e1f010
Compare
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
from
June 8, 2026 08:44
9e1f010 to
dd83754
Compare
Contributor
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
♻️ Duplicate comments (1)
contract/vault/soroban/tests/blend_e2e.rs (1)
36-42: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winEnumerate the supported receipt decoders here.
The wildcard
I128Receiptarm lets any futureVaultCommandcompile through this helper even when its receipt is a different type, so unsupported command/receipt pairings degrade into ambiguousInvalidInputfailures instead of failing obviously in the test. Match only the commands this file actually exercises.Suggested change
match command { VaultCommand::DepositWithMin { .. } => DepositReceipt::decode(&bytes) .map(|receipt| receipt.shares_out) .map_err(|_| templar_soroban_runtime::ContractError::InvalidInput), - _ => I128Receipt::decode(&bytes) + VaultCommand::Allocate { .. } | VaultCommand::RefreshMarkets { .. } => { + I128Receipt::decode(&bytes) .map(|receipt| receipt.value) - .map_err(|_| templar_soroban_runtime::ContractError::InvalidInput), + .map_err(|_| templar_soroban_runtime::ContractError::InvalidInput) + } + _ => panic!("unsupported command for execute_command helper"), }As per coding guidelines: "Prefer parsing over validation in Rust code; express invariants in types wherever practical, and make invalid states unrepresentable."
🤖 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/tests/blend_e2e.rs` around lines 36 - 42, The match currently defaults to decoding every non-DepositWithMin command with I128Receipt, which hides unsupported command/receipt mismatches; update the match in the helper so it enumerates only the VaultCommand variants exercised by these tests (e.g., VaultCommand::DepositWithMin -> DepositReceipt::decode(...).map(|r| r.shares_out) and explicitly match each other tested variant to its correct decoder (e.g., VaultCommand::SomeOtherCmd -> SomeOtherReceipt::decode(...).map(|r| r.field)), and add a final _ => return Err(templar_soroban_runtime::ContractError::InvalidInput) to fail fast for unknown commands rather than silently decoding with I128Receipt.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/proxy-4626-soroban/src/contract.rs`:
- Around line 697-700: The current address_from_wire uses
validate_address_strkey then calls Address::from_str which can panic; wrap the
conversion in a panic-safe guard and return ContractError::InvalidInput on
failure. Specifically, inside address_from_wire keep or run
validate_address_strkey(value.as_bytes()) and then call Address::from_str(env,
value) inside std::panic::catch_unwind (or use any non-panicking SDK helper if
available) and map any panic or invalid result to
Err(ContractError::InvalidInput) so the function never panics while still
returning Ok(Address) on success.
In `@contract/vault/soroban/shared-types/src/lib.rs`:
- Around line 583-589: The Completed receipt variant is exposing owner and
receiver as unchecked String; change those fields to a validated StrKey newtype
(e.g., ReceiptAddress or StrKeyAddress) so decoding enforces address format at
receipt parse time. Add a newtype that wraps the existing StrKey/Address
validator used elsewhere (implement Deserialize/Serialize, FromStr, Display, and
TryFrom<String>) and replace owner: String and receiver: String in the Completed
enum variant with owner: ReceiptAddress and receiver: ReceiptAddress; update any
receipt decode/construct paths (the receipt decode function and any
builders/constructors that create Completed) to validate/convert strings into
ReceiptAddress and update call-sites to use the new type (or call .to_string()
only where a raw string is absolutely needed). Ensure the newtype reuses the
repo's canonical strkey/address validation utilities to avoid duplicated logic.
In `@contract/vault/soroban/src/contract/entrypoints.rs`:
- Around line 1208-1224: The branch handlers (VaultCommand::AtomicWithdraw,
AtomicRedeem, Allocate, RefreshMarkets) all return an I128Receipt via
encode_receipt, but the meaning of its field `value` differs per command; change
to command-specific receipt types (e.g., AtomicWithdrawReceipt,
AtomicRedeemReceipt, AllocateReceipt, RefreshMarketsReceipt) or a single enum
Receipt that encodes the command tag plus semantic field names, update
encode_receipt to accept these new types (or the enum) and their encode
implementations, and replace the use of I128Receipt::encode in the
atomic_withdraw_impl/atomic_redeem_impl/allocate_impl/refresh_markets_impl
return paths so the ABI bytes are self-describing rather than reusing
I128Receipt.
In `@contract/vault/soroban/src/contract/types.rs`:
- Around line 45-50: The current ExecuteWithdrawResult struct allows invalid
combinations of summary and completed; replace it with an enum (e.g.,
ExecuteWithdrawResult::Payout { summary: EffectSummary, completed:
CompletedWithdrawal } and ExecuteWithdrawResult::NoPayout { summary:
EffectSummary }) so the two wire receipt variants are represented explicitly;
update all construction sites and pattern matches that currently access
ExecuteWithdrawResult.summary or .completed to use the new enum variants and
adjust any (de)serialization/derive attributes used on ExecuteWithdrawResult to
match the enum shape.
In `@contract/vault/soroban/tests/integration_tests.rs`:
- Around line 1354-1367: The test verifies that execute_withdraw returns a
NoPayout receipt and validates the status fields, but never checks whether the
queued restricted request was actually consumed from the queue. After the
existing assertions on the status object (op_state_before, op_state_after,
assets_transferred, events_emitted), add an assertion that verifies the queue
state to confirm that the restricted head request was dequeued or marked as
consumed by the NoPayout operation.
---
Duplicate comments:
In `@contract/vault/soroban/tests/blend_e2e.rs`:
- Around line 36-42: The match currently defaults to decoding every
non-DepositWithMin command with I128Receipt, which hides unsupported
command/receipt mismatches; update the match in the helper so it enumerates only
the VaultCommand variants exercised by these tests (e.g.,
VaultCommand::DepositWithMin -> DepositReceipt::decode(...).map(|r|
r.shares_out) and explicitly match each other tested variant to its correct
decoder (e.g., VaultCommand::SomeOtherCmd ->
SomeOtherReceipt::decode(...).map(|r| r.field)), and add a final _ => return
Err(templar_soroban_runtime::ContractError::InvalidInput) to fail fast for
unknown commands rather than silently decoding with I128Receipt.
🪄 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: 1c0ce67b-f7d9-4737-b4c9-3a3083136351
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Cargo.tomlcontract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/vault/curator-primitives/src/policy/cap_group/mod.rscontract/vault/curator-primitives/src/policy/cooldown/mod.rscontract/vault/curator-primitives/src/policy/state/mod.rscontract/vault/curator-primitives/src/policy/supply_queue/mod.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/README.mdcontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/scripts/vault_payload.pycontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/helpers.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rstest-utils/examples/registry_add_version_args.rstest-utils/src/lib.rs
Contributor
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
♻️ Duplicate comments (3)
contract/vault/soroban/src/contract/types.rs (1)
45-50: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftRepresent execute-withdraw outcomes as explicit variants.
ExecuteWithdrawResult { summary, completed: Option<_> }still admits impossible combinations and leaves the internal model looser than the wire receipt shape. An enum withNoPayout { summary }andCompleted { summary, completed }keeps the runtime and receipt contracts aligned.As per coding guidelines,
**/*.rsshould “prefer stronger types over loosely constrained values” and “make invalid states unrepresentable.”🤖 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/types.rs` around lines 45 - 50, Replace the loose struct ExecuteWithdrawResult with a closed enum to make impossible states unrepresentable: define enum ExecuteWithdrawResult { NoPayout { summary: EffectSummary }, Completed { summary: EffectSummary, completed: CompletedWithdrawal } } (keeping existing derives like Clone/PartialEq/Eq and cfg_attr for Debug) and update all usages that construct or pattern-match ExecuteWithdrawResult to use the NoPayout and Completed variants instead of checking completed: Option<_>; ensure any serialization/deserialization or receipt mapping mirrors the new enum shape so the runtime and wire receipt remain aligned.Source: Coding guidelines
contract/vault/soroban/src/contract/entrypoints.rs (1)
1208-1264: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMake scalar execute receipts command-specific.
These four branches all return
I128Receipt { value }, butvaluemeans shares burned, assets out, or new external assets depending on the command. The receipt bytes are therefore not self-describing, which undercuts the move to typed execute receipts.As per coding guidelines,
**/*.rsshould “prefer stronger types over loosely constrained values” and “make invalid states unrepresentable.”Source: Coding guidelines
contract/vault/soroban/shared-types/src/lib.rs (1)
583-589: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftDecode receipt addresses into a validated type.
Completedstill storesownerandreceiveras arbitraryStrings, so malformed receipt bytes can pass codec parsing and rely on every consumer to remember a second validation step. Wrap these fields in a validated StrKey newtype and enforcevalidate_address_strkeyduring decode.Based on learnings and coding guidelines, this receipt boundary is security-relevant, and
**/*.rsshould “express invariants in types wherever practical” and “make invalid states unrepresentable.”Also applies to: 943-979
🤖 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 583 - 589, The Completed receipt variant currently stores owner and receiver as raw String; change those fields to a validated StrKey newtype (e.g., AddressStrKey or reuse existing StrKey) and update the Decodable/codec implementation where Completed is parsed to call validate_address_strkey on the incoming strings, failing decode on invalid addresses so malformed receipt bytes cannot produce a Completed with arbitrary strings; update the Completed definition and its decode logic (and similarly adjust the other receipt variants/locations mentioned around the 943-979 region) to use the new type and validation function so the invariant is enforced at type level.Sources: Coding guidelines, Learnings
🤖 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/proxy-4626-soroban/src/tests.rs`:
- Around line 516-549: Add tests mirroring
test_deposit_rejects_receipt_with_trailing_bytes for the other two receipt
decoders: create malformed_receipt bytes from RequestWithdrawReceipt and
ExecuteWithdrawReceipt by encoding a valid struct and appending trailing bytes,
call the corresponding proxy entrypoints (use
Soroban4626ProxyContract::request_withdraw and
Soroban4626ProxyContract::execute_withdraw via fixture.env.as_contract with
Fixture::new, fixture.initialize, fixture.set_next_receipt), assert the call
returns Err(ContractError::InvalidInput), and verify fixture.recorded_payloads()
contains exactly one payload whose decode_command equals the expected
WireVaultCommand for that operation; to avoid duplication, parameterize these
cases with #[rstest] or a small helper that takes the receipt-constructor,
proxy-method-invocation closure, and expected WireVaultCommand.
In `@contract/vault/curator-primitives/src/policy/supply_queue/mod.rs`:
- Around line 178-181: In supply_queue::mod.rs where is_full() is used, replace
the unreachable None-branch that currently returns
Err(SupplyQueueError::LengthOverflow) with an explicit unreachable!() because
is_full() guarantees self.max_length is Some; specifically update the pattern
match in the code that does `let Some(max_length) = self.max_length else { ...
};` to call unreachable!("is_full() guarantees max_length is Some") in the else
branch and keep returning Err(SupplyQueueError::QueueFull { max_length }); so
the error semantics remain correct (refer to is_full(),
SupplyQueueError::QueueFull and SupplyQueueError::LengthOverflow).
In `@contract/vault/soroban/shared-types/src/lib.rs`:
- Around line 15-82: Add unit tests that call validate_address_strkey (and
indirectly crc16_xmodem) with explicit accept/reject vectors: include at least
one valid ACCOUNT_STRKEY and one valid CONTRACT_STRKEY to assert Ok(()), plus
separate failing inputs for bad checksum (corrupt trailing bytes so expected !=
actual), bad alphabet (introduce characters outside 'A'-'Z' and '2'-'7'), bad
length (shorter and longer than STRKEY_LEN), and bad version byte (valid base32
payload but with out[0] not equal to ACCOUNT_VERSION or CONTRACT_VERSION) to
assert Err(CodecError::InvalidEncoding); place tests near other strkey tests and
use the constants STRKEY_LEN, BINARY_LEN, ACCOUNT_VERSION, CONTRACT_VERSION to
construct/validate vectors so the parser behavior cannot silently drift.
---
Duplicate comments:
In `@contract/vault/soroban/shared-types/src/lib.rs`:
- Around line 583-589: The Completed receipt variant currently stores owner and
receiver as raw String; change those fields to a validated StrKey newtype (e.g.,
AddressStrKey or reuse existing StrKey) and update the Decodable/codec
implementation where Completed is parsed to call validate_address_strkey on the
incoming strings, failing decode on invalid addresses so malformed receipt bytes
cannot produce a Completed with arbitrary strings; update the Completed
definition and its decode logic (and similarly adjust the other receipt
variants/locations mentioned around the 943-979 region) to use the new type and
validation function so the invariant is enforced at type level.
In `@contract/vault/soroban/src/contract/types.rs`:
- Around line 45-50: Replace the loose struct ExecuteWithdrawResult with a
closed enum to make impossible states unrepresentable: define enum
ExecuteWithdrawResult { NoPayout { summary: EffectSummary }, Completed {
summary: EffectSummary, completed: CompletedWithdrawal } } (keeping existing
derives like Clone/PartialEq/Eq and cfg_attr for Debug) and update all usages
that construct or pattern-match ExecuteWithdrawResult to use the NoPayout and
Completed variants instead of checking completed: Option<_>; ensure any
serialization/deserialization or receipt mapping mirrors the new enum shape so
the runtime and wire receipt remain aligned.
🪄 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: 59a4f155-ad7e-4097-be22-14bf586717fd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Cargo.tomlcontract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/vault/curator-primitives/src/policy/cap_group/mod.rscontract/vault/curator-primitives/src/policy/cooldown/mod.rscontract/vault/curator-primitives/src/policy/state/mod.rscontract/vault/curator-primitives/src/policy/supply_queue/mod.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/README.mdcontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/scripts/vault_payload.pycontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/helpers.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rstest-utils/examples/registry_add_version_args.rstest-utils/src/lib.rs
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
from
June 8, 2026 09:05
dd83754 to
d704cbf
Compare
Contributor
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: 1
♻️ Duplicate comments (1)
contract/proxy-4626-soroban/src/tests.rs (1)
518-551: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMalformed receipt test coverage incomplete for RequestWithdraw and ExecuteWithdraw paths.
The test
test_deposit_rejects_receipt_with_trailing_bytesvalidates trailing-byte rejection forDepositReceipt, but parallel tests forRequestWithdrawReceipt(used bywithdraw,redeem,request_withdraw) andExecuteWithdrawReceipt(used byexecute_withdraw) are missing. All three decoders enforceensure_finished()per the PR objectives, so all three paths should be tested for robustness.Add parameterized malformed-receipt tests for the remaining decoders:
#[rstest] #[case::deposit(/* DepositReceipt + trailing */)] #[case::request_withdraw(/* RequestWithdrawReceipt + trailing */)] #[case::execute_withdraw(/* ExecuteWithdrawReceipt + trailing */)] fn test_receipt_rejects_trailing_bytes(#[case] malformed: Vec<u8>, #[case] entrypoint: ...) { // arrange: fixture.set_next_receipt(&malformed) // act: call entrypoint // assert: Err(ContractError::InvalidInput) }🤖 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/proxy-4626-soroban/src/tests.rs` around lines 518 - 551, Extend test coverage beyond test_deposit_rejects_receipt_with_trailing_bytes by adding parameterized tests that assert trailing-byte receipts are rejected for the other decoders: construct malformed vectors by encoding RequestWithdrawReceipt and ExecuteWithdrawReceipt (like how DepositReceipt was encoded then extended with extra bytes), call the matching entrypoints (withdraw, redeem, request_withdraw for RequestWithdrawReceipt and execute_withdraw for ExecuteWithdrawReceipt) via the fixture (use Fixture::set_next_receipt to inject the malformed receipt and call the contract through fixture.env.as_contract), and assert the call returns Err(ContractError::InvalidInput); use rstest #[case] variants to drive the three cases so the same assertion logic covers DepositReceipt (existing), RequestWithdrawReceipt, and ExecuteWithdrawReceipt.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/vault/soroban/shared-types/src/lib.rs`:
- Around line 1369-1375: Test uses identical addresses for
ExecuteWithdrawReceipt::Completed (owner and receiver), which hides layout
errors; update the test fixture so owner and receiver are distinct values (e.g.,
introduce a second fixture function or constant like receipt_address_alt() and
use that for receiver or owner) and prefer using two different accepted StrKey
variants for the pair to ensure the new owner field is actually pinned in the
wire layout and roundtrip serialization/deserialization.
---
Duplicate comments:
In `@contract/proxy-4626-soroban/src/tests.rs`:
- Around line 518-551: Extend test coverage beyond
test_deposit_rejects_receipt_with_trailing_bytes by adding parameterized tests
that assert trailing-byte receipts are rejected for the other decoders:
construct malformed vectors by encoding RequestWithdrawReceipt and
ExecuteWithdrawReceipt (like how DepositReceipt was encoded then extended with
extra bytes), call the matching entrypoints (withdraw, redeem, request_withdraw
for RequestWithdrawReceipt and execute_withdraw for ExecuteWithdrawReceipt) via
the fixture (use Fixture::set_next_receipt to inject the malformed receipt and
call the contract through fixture.env.as_contract), and assert the call returns
Err(ContractError::InvalidInput); use rstest #[case] variants to drive the three
cases so the same assertion logic covers DepositReceipt (existing),
RequestWithdrawReceipt, and ExecuteWithdrawReceipt.
🪄 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: a436c094-d0b0-421a-9303-560c68d573c8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Cargo.tomlcontract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/vault/curator-primitives/src/policy/cap_group/mod.rscontract/vault/curator-primitives/src/policy/cooldown/mod.rscontract/vault/curator-primitives/src/policy/state/mod.rscontract/vault/curator-primitives/src/policy/supply_queue/mod.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/README.mdcontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/scripts/vault_payload.pycontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/helpers.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rstest-utils/examples/registry_add_version_args.rstest-utils/src/lib.rs
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
from
June 8, 2026 09:31
c61d914 to
82841dc
Compare
carrion256
force-pushed
the
codex/soroban-execute-receipts
branch
from
June 8, 2026 10:48
82841dc to
df5f9aa
Compare
Contributor
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: 2
♻️ Duplicate comments (3)
contract/vault/soroban/shared-types/src/lib.rs (2)
1102-1105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a second receipt-address fixture for
Completedreceipt tests.Using the same fixture for both
ownerandreceivermeans swapped or duplicated fields in the wire layout would still pass. Make them distinct—ideally covering both accepted StrKey variants—to actually pin both fields. Based on learnings, this receipt wire contract is security-relevant.🤖 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 1102 - 1105, Create a second distinct fixture function (e.g., receipt_address_b or receipt_address_completed) that returns a different ReceiptAddress via ReceiptAddress::from_str(...).expect(...), using a different valid StrKey string than the existing receipt_address() so owner and receiver in Completed receipt tests are unique; update the Completed receipt tests to use receipt_address() and the new fixture respectively to ensure swapped/duplicated fields fail the wire-contract checks.Source: Learnings
22-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd direct accept/reject vectors for
validate_address_strkey.This parser is now a receipt-boundary security check, but the tests only exercise it indirectly through
ReceiptAddressand receipt decode. Please add explicit validG.../C...cases plus bad checksum, alphabet, length, and version-byte cases so validator drift fails fast. Based on learnings, receipt serialization changes incontract/vault/soroban/**/*.rsare security-relevant.🤖 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 22 - 89, The validate_address_strkey function is only exercised indirectly; add direct unit tests that call validate_address_strkey and cover explicit accept/reject vectors: include valid G... (ACCOUNT_VERSION) and C... (CONTRACT_VERSION) base32-encoded 56-byte STRKEY_LEN inputs that decode to BINARY_LEN bytes, and negative cases for bad checksum (mismatched crc16_xmodem), invalid alphabet characters, incorrect length, and wrong version byte values; reference the symbols validate_address_strkey, crc16_xmodem, ACCOUNT_VERSION, CONTRACT_VERSION, STRKEY_LEN and BINARY_LEN when creating tests so failures map directly to the parser and will catch receipt-serialization regressions.Source: Learnings
contract/vault/soroban/tests/blend_e2e.rs (1)
29-43: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winEnumerate the supported receipt pairings in
execute_command.The
_branch makes every non-deposit command implicitly “expectsI128Receipt”, so a future test addition falls back to a generic decode error instead of an obvious unsupported-command failure. List the actual commands used here (Allocate,RefreshMarkets, etc.) and reject the rest explicitly. As per coding guidelines, prefer parsing over validation and make invalid states unrepresentable.🤖 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/tests/blend_e2e.rs` around lines 29 - 43, The execute_command function currently treats every non-DepositWithMin as returning an I128Receipt; update execute_command to explicitly match each VaultCommand variant and decode to the correct receipt type (e.g., VaultCommand::Allocate => I128Receipt, VaultCommand::RefreshMarkets => I128Receipt, VaultCommand::SomeOtherVariant => SomeOtherReceipt) instead of using the wildcard arm, and for any unsupported/unknown VaultCommand variants return a clear templar_soroban_runtime::ContractError::UnsupportedCommand (or a new explicit UnsupportedCommand error) so invalid/unsupported commands fail with a descriptive error rather than a generic decode failure; locate the execute_command function and the VaultCommand enum to map each variant to its expected receipt decoder and add the explicit error branch for remaining cases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/proxy-4626-soroban/src/tests.rs`:
- Around line 115-118: The NextReceipt flag is treated as a permanent override
because set_next_receipt writes MockVaultDataKey::NextReceipt but
MockVaultContract::execute never clears it; update the contract to consume the
injected receipt instead of leaving it sticky by removing
MockVaultDataKey::NextReceipt when execute reads it (i.e., read-and-remove from
env.storage().instance() so the injected blob is one-shot), or alternatively
change set_next_receipt/execute semantics so execute explicitly clears the key
after using it to ensure later calls don't reuse the same receipt.
In `@contract/vault/soroban/tests/integration_tests.rs`:
- Around line 1270-1292: Add an assertion that the queued withdrawal was
actually removed after execute_withdraw completes: after calling
proxy.execute_withdraw (which yields ExecuteWithdrawReceipt::Completed) read the
contract state (via env.as_contract / the same contract proxy) and assert the
request queue no longer contains the request_id (or that the queue length is
decremented/empty and any queued entry for request_id is absent); update the
test around execute_withdraw/ExecuteWithdrawReceipt::Completed to fetch the
queue (e.g., call the contract method that returns queued requests or inspect
the same storage item used by withdraw/redeem) and assert the queued request is
drained to prevent a regression where the receipt is returned before state
mutation persists.
---
Duplicate comments:
In `@contract/vault/soroban/shared-types/src/lib.rs`:
- Around line 1102-1105: Create a second distinct fixture function (e.g.,
receipt_address_b or receipt_address_completed) that returns a different
ReceiptAddress via ReceiptAddress::from_str(...).expect(...), using a different
valid StrKey string than the existing receipt_address() so owner and receiver in
Completed receipt tests are unique; update the Completed receipt tests to use
receipt_address() and the new fixture respectively to ensure swapped/duplicated
fields fail the wire-contract checks.
- Around line 22-89: The validate_address_strkey function is only exercised
indirectly; add direct unit tests that call validate_address_strkey and cover
explicit accept/reject vectors: include valid G... (ACCOUNT_VERSION) and C...
(CONTRACT_VERSION) base32-encoded 56-byte STRKEY_LEN inputs that decode to
BINARY_LEN bytes, and negative cases for bad checksum (mismatched crc16_xmodem),
invalid alphabet characters, incorrect length, and wrong version byte values;
reference the symbols validate_address_strkey, crc16_xmodem, ACCOUNT_VERSION,
CONTRACT_VERSION, STRKEY_LEN and BINARY_LEN when creating tests so failures map
directly to the parser and will catch receipt-serialization regressions.
In `@contract/vault/soroban/tests/blend_e2e.rs`:
- Around line 29-43: The execute_command function currently treats every
non-DepositWithMin as returning an I128Receipt; update execute_command to
explicitly match each VaultCommand variant and decode to the correct receipt
type (e.g., VaultCommand::Allocate => I128Receipt, VaultCommand::RefreshMarkets
=> I128Receipt, VaultCommand::SomeOtherVariant => SomeOtherReceipt) instead of
using the wildcard arm, and for any unsupported/unknown VaultCommand variants
return a clear templar_soroban_runtime::ContractError::UnsupportedCommand (or a
new explicit UnsupportedCommand error) so invalid/unsupported commands fail with
a descriptive error rather than a generic decode failure; locate the
execute_command function and the VaultCommand enum to map each variant to its
expected receipt decoder and add the explicit error branch for remaining cases.
🪄 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: ac2a2557-9a49-4e3b-8528-f614745efec9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Cargo.tomlcontract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/vault/curator-primitives/src/policy/cap_group/mod.rscontract/vault/curator-primitives/src/policy/cooldown/mod.rscontract/vault/curator-primitives/src/policy/state/mod.rscontract/vault/curator-primitives/src/policy/supply_queue/mod.rscontract/vault/kernel/src/actions/mod.rscontract/vault/soroban/README.mdcontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/scripts/vault_payload.pycontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/helpers.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rscontract/vault/soroban/tests/integration_tests.rstest-utils/examples/registry_add_version_args.rstest-utils/src/lib.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Withdrawonly after a completed payout receiptAuditor notes
Please focus review on:
contract/vault/soroban/shared-types/src/lib.rsreceipt codecs and trailing-byte rejectioncontract/vault/soroban/src/contract/entrypoints.rsandcurator_vault.rsexecute receipt plumbingcontract/proxy-4626-soroban/src/contract.rsreceipt decoding andRedeemRequest/Withdrawevent semanticsNo storage migration, no command payload change, and no governance/share-token/adapter behavior change intended.
Verification
cargo test -p templar-soroban-shared-types -- --nocapturecargo test -p templar-vault-kernel --lib -- --nocapturecargo test -p templar-soroban-runtime -- --nocapturecargo test -p templar-4626-proxy-soroban -- --nocapturejust -f contract/vault/soroban/justfile size-budget-checkSize
94449bytes96199bytes+1750bytes96199 <= 131072bytesThis change is