feat: implement admin-driven vault removal and internal redemption ac… - #84
Conversation
…counting, closes #81
Reviewer's GuideThis PR implements an admin-driven vault removal mechanism by tracking decommissioned vaults, enables synchronous redemption for decommissioned vaults with proper asset accounting and share burning, extends the liquidity orchestrator and config interfaces to support these flows, and adjusts tests and minor code cleanups throughout. Sequence diagram for synchronous redemption of decommissioned vaultssequenceDiagram
participant User
participant OrionVault
participant OrionConfig
participant LiquidityOrchestrator
User->>OrionVault: redeem(shares, receiver, owner)
OrionVault->>OrionConfig: isDecommissionedVault(address(this))
OrionConfig-->>OrionVault: true
OrionVault->>LiquidityOrchestrator: withdraw(assets, receiver)
LiquidityOrchestrator->>OrionConfig: isDecommissionedVault(vault)
OrionConfig-->>LiquidityOrchestrator: true
LiquidityOrchestrator-->>OrionVault: transfer assets
OrionVault-->>User: return assets
ER diagram for vault status tracking in OrionConfigerDiagram
ORIONCONFIG {
address id
decommissionedVaults address[]
encryptedVaults address[]
transparentVaults address[]
}
VAULT {
address id
}
ORIONCONFIG ||--o| VAULT : manages
ORIONCONFIG ||--|{ VAULT : decommissionedVaults
ORIONCONFIG ||--|{ VAULT : encryptedVaults
ORIONCONFIG ||--|{ VAULT : transparentVaults
Class diagram for decommissioned vault tracking and synchronous redemptionclassDiagram
class OrionConfig {
+removeOrionVault(address vault, VaultType vaultType)
+isDecommissionedVault(address vault) bool
-decommissionedVaults: EnumerableSet.AddressSet
}
class OrionVault {
+redeem(uint256 shares, address receiver, address owner) uint256
-config: OrionConfig
-_totalAssets: uint256
}
class LiquidityOrchestrator {
+withdraw(uint256 assets, address receiver)
}
OrionVault --> OrionConfig : uses
OrionVault --> LiquidityOrchestrator : calls withdraw
OrionConfig --> OrionVault : manages vault status
LiquidityOrchestrator --> OrionConfig : checks vault status
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds vault decommissioning lifecycle: OrionConfig tracks decommissioning and decommissioned vaults and exposes query/complete functions; OrionVault implements operational synchronous redeem for decommissioned vaults; LiquidityOrchestrator gains a withdraw path using SafeERC20; events, interfaces, artifacts, tests, and ignore files updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant OrionConfig
participant OrionVault
participant LiquidityOrchestrator
participant SafeERC20
participant Token
Admin->>OrionConfig: removeOrionVault(vault)
OrionConfig->>OrionConfig: mark vault decommissioningInProgress
OrionConfig->>OrionVault: call overrideIntentForDecommissioning()
OrionVault->>OrionVault: set isDecommissioning = true
Note over LP,OrionVault: During decommissioning LP attempts sync redeem
LP->>OrionVault: redeem(shares, receiver, owner)
OrionVault->>OrionConfig: isDecommissionedVault(vault)?
OrionConfig-->>OrionVault: false (initially)
OrionVault->>OrionVault: revert SynchronousCallDisabled
Note over Orchestrator,OrionConfig: After orchestrator finalizes decommissioning
OrionConfig->>OrionConfig: completeVaultDecommissioning(vault)
OrionConfig->>OrionConfig: move vault to decommissionedVaults
LP->>OrionVault: redeem(shares, receiver, owner)
OrionVault->>OrionConfig: isDecommissionedVault(vault)?
OrionConfig-->>OrionVault: true
OrionVault->>LiquidityOrchestrator: withdraw(assets, receiver)
LiquidityOrchestrator->>OrionConfig: isDecommissionedVault(caller)?
OrionConfig-->>LiquidityOrchestrator: true
LiquidityOrchestrator->>SafeERC20: safeTransfer(token, receiver, assets)
SafeERC20->>Token: transfer(receiver, assets)
Token-->>SafeERC20: success
SafeERC20-->>LiquidityOrchestrator: success
LiquidityOrchestrator-->>OrionVault: success
OrionVault-->>LP: assets transferred
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.github.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new VaultRemovalProcessed event is never emitted – either remove it or emit it in removeOrionVault with the vault’s totalAssets and curatorFee for proper off‐chain tracking.
- Add integration tests covering the synchronous redemption path (OrionVault.redeem + orchestrator.withdraw) to ensure the decommissioned‐vault flow behaves as expected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new VaultRemovalProcessed event is never emitted – either remove it or emit it in removeOrionVault with the vault’s totalAssets and curatorFee for proper off‐chain tracking.
- Add integration tests covering the synchronous redemption path (OrionVault.redeem + orchestrator.withdraw) to ensure the decommissioned‐vault flow behaves as expected.
## Individual Comments
### Comment 1
<location> `contracts/orchestrators/LiquidityOrchestrator.sol:258` </location>
<code_context>
}
+ /// @inheritdoc ILiquidityOrchestrator
+ function withdraw(uint256 assets, address receiver) external {
+ address vault = msg.sender;
+
</code_context>
<issue_to_address>
**suggestion:** Consider adding a check for zero assets in withdraw to prevent unnecessary transfers.
Currently, withdraw allows zero-asset transfers, which can trigger SafeERC20 operations unnecessarily and may confuse receivers. Adding a zero-check would avoid this.
Suggested implementation:
```
/// @inheritdoc ILiquidityOrchestrator
function withdraw(uint256 assets, address receiver) external {
address vault = msg.sender;
if (assets == 0) revert ErrorsLib.AmountMustBeGreaterThanZero(underlyingAsset);
```
You may need to ensure that `underlyingAsset` is available in the scope of the `withdraw` function, as it is used in the error revert. If not, you should pass or retrieve it as appropriate for your contract's logic.
</issue_to_address>
### Comment 2
<location> `contracts/OrionConfig.sol:224-232` </location>
<code_context>
- bool removed;
if (vaultType == EventsLib.VaultType.Encrypted) {
- removed = encryptedVaults.remove(vault);
+ // slither-disable-next-line unused-return
+ encryptedVaults.remove(vault);
} else {
- removed = transparentVaults.remove(vault);
+ // slither-disable-next-line unused-return
+ transparentVaults.remove(vault);
}
- if (!removed) revert ErrorsLib.UnauthorizedAccess();
+ // slither-disable-next-line unused-return
+ decommissionedVaults.add(vault);
+
emit EventsLib.OrionVaultRemoved(vault);
</code_context>
<issue_to_address>
**suggestion:** Consider emitting an event when a vault is added to decommissionedVaults for better traceability.
Adding a specific event for vaults added to decommissionedVaults will enhance auditability and clarify state changes beyond the current OrionVaultRemoved event.
Suggested implementation:
```
EnumerableSet.AddressSet private decommissionedVaults;
/// @notice Emitted when a vault is added to decommissionedVaults
event DecommissionedVaultAdded(address indexed vault);
```
```
// slither-disable-next-line unused-return
decommissionedVaults.add(vault);
emit DecommissionedVaultAdded(vault);
emit EventsLib.OrionVaultRemoved(vault);
```
</issue_to_address>
### Comment 3
<location> `contracts/libraries/EventsLib.sol:77` </location>
<code_context>
+ /// @param vault The address of the removed vault.
+ /// @param totalAssets The total assets of the vault at removal.
+ /// @param curatorFee The curator fee calculated for the final state.
+ event VaultRemovalProcessed(address indexed vault, uint256 indexed totalAssets, uint256 indexed curatorFee);
+
/// @notice Enumeration of available vault types.
</code_context>
<issue_to_address>
**suggestion (performance):** Consider whether all indexed parameters are necessary for VaultRemovalProcessed event.
Indexing all parameters increases gas costs. If totalAssets or curatorFee are rarely used for filtering, consider removing their indexing to reduce costs.
```suggestion
event VaultRemovalProcessed(address indexed vault, uint256 totalAssets, uint256 curatorFee);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/OrionConfigVault.test.ts (1)
383-391: Test gap: Missing validation prevents detection of inconsistent vault state.This test verifies the vault remains registered after wrong-type removal, but doesn't check the decommissioned state. The current implementation creates inconsistent state where
isOrionVaultreturns true ANDisDecommissionedVaultreturns true.Add assertion to catch the state inconsistency:
it("Should handle removal of vault with wrong vault type", async function () { const vaultAddress = await vault.getAddress(); // Remove transparent vault as encrypted vault (wrong type) await orionConfig.removeOrionVault(vaultAddress, 1); // 1 = EventsLib.VaultType.Encrypted // Verify vault is still registered expect(await orionConfig.isOrionVault(vaultAddress)).to.equal(true); + + // Verify vault is not marked as decommissioned when removal fails + expect(await orionConfig.isDecommissionedVault(vaultAddress)).to.equal(false); });
🧹 Nitpick comments (5)
contracts/test/UtilitiesLibTest.sol (1)
11-12: Named return variable is redundant.The named return variable
resultis declared but immediately overwritten by the return statement. Consider either using the named return implicitly or removing the name.Option 1: Remove the named return variable
- ) external pure returns (uint256 result) { + ) external pure returns (uint256) { return UtilitiesLib.convertDecimals(value, fromDecimals, toDecimals);Option 2: Use implicit return (if you want to keep the named return)
- ) external pure returns (uint256 result) { - return UtilitiesLib.convertDecimals(value, fromDecimals, toDecimals); + ) external pure returns (uint256 result) { + result = UtilitiesLib.convertDecimals(value, fromDecimals, toDecimals); }contracts/orchestrators/InternalStatesOrchestrator.sol (1)
442-442: Early return logic is correct; consider minor optimization.The early return correctly implements the conservative buffer management strategy described in the comments. When the buffer is already above target, skipping the redistribution prevents unwanted buffer reduction.
However, consider using
>=instead of>for a minor optimization:-if (bufferAmount > targetBufferAmount) return; +if (bufferAmount >= targetBufferAmount) return;When
bufferAmount == targetBufferAmount,deltaBufferAmountwould be zero (line 444), making the loop at lines 445-450 a no-op. The>=condition would skip these unnecessary iterations.Note: This buffer management change appears unrelated to the PR's stated objective of implementing vault decommissioning functionality.
contracts/libraries/EventsLib.sol (1)
73-77: Consider indexing strategy for numeric event parameters.All three parameters are indexed, which reaches Solidity's limit of 3 indexed parameters per event. While indexing
vaultis beneficial for filtering removal events by vault address, indexing theuint256valuestotalAssetsandcuratorFeemay be less useful since:
- Indexed
uint256parameters are stored as keccak256 hashes in logs, making them harder to read off-chain- They're typically used for exact-match filtering rather than range queries
- Moving them to non-indexed data fields would preserve the actual values in logs while still allowing queries by vault address
If exact filtering by these numeric values isn't required, consider this structure:
- event VaultRemovalProcessed(address indexed vault, uint256 indexed totalAssets, uint256 indexed curatorFee); + event VaultRemovalProcessed(address indexed vault, uint256 totalAssets, uint256 curatorFee);contracts/orchestrators/LiquidityOrchestrator.sol (2)
17-17: Use SafeERC20 consistently for all token transfersYou’ve imported SafeERC20 but still use raw transfer/transferFrom elsewhere. To harden against non‑standard ERC20s, migrate calls in returnDepositFunds, withdrawLiquidity, claimProtocolFees, transferCuratorFees, transferRedemptionFunds to SafeERC20.
237-237: Auth widening for curator fees looks correctAllowing decommissioned vaults to call is aligned with removal flow; msg.sender remains the vault. Consider also switching the underlying transfer to SafeERC20 as noted above for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.prettierignore(1 hunks).solhintignore(1 hunks)artifacts/contracts/OrionConfig.sol/OrionConfig.json(2 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(1 hunks)artifacts/contracts/interfaces/ILiquidityOrchestrator.sol/ILiquidityOrchestrator.json(1 hunks)artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json(1 hunks)artifacts/contracts/libraries/EventsLib.sol/EventsLib.json(2 hunks)artifacts/contracts/mocks/MockERC4626Asset.sol/MockERC4626Asset.json(1 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(3 hunks)artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json(1 hunks)artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json(1 hunks)artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json(1 hunks)artifacts/contracts/test/UtilitiesLibTest.sol/UtilitiesLibTest.json(1 hunks)artifacts/contracts/vaults/OrionVault.sol/OrionVault.json(6 hunks)contracts/OrionConfig.sol(3 hunks)contracts/interfaces/ILiquidityOrchestrator.sol(1 hunks)contracts/interfaces/IOrionConfig.sol(1 hunks)contracts/libraries/EventsLib.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(1 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(3 hunks)contracts/strategies/KBestTvlWeightedAverage.sol(0 hunks)contracts/test/UtilitiesLibTest.sol(1 hunks)contracts/vaults/OrionTransparentVault.sol(1 hunks)contracts/vaults/OrionVault.sol(1 hunks)test/OrionConfigVault.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- contracts/strategies/KBestTvlWeightedAverage.sol
🧰 Additional context used
🧬 Code graph analysis (5)
artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json (1)
test/Adapters.test.ts (1)
orionConfig(14-117)
artifacts/contracts/test/UtilitiesLibTest.sol/UtilitiesLibTest.json (1)
test/UtilitiesLib.test.ts (10)
utilitiesLib(6-193)describe(16-192)value(144-150)it(143-191)it(59-107)it(17-57)value(76-82)value(168-174)value(60-66)value(100-106)
contracts/test/UtilitiesLibTest.sol (1)
test/UtilitiesLib.test.ts (10)
utilitiesLib(6-193)describe(16-192)it(143-191)it(17-57)it(59-107)value(144-150)value(76-82)value(168-174)value(184-190)value(100-106)
contracts/vaults/OrionTransparentVault.sol (2)
test/PassiveCuratorStrategy.test.ts (4)
transparentVault(442-451)it(441-472)transparentVault(463-471)vaultWhitelist(296-315)test/TransparentVault.test.ts (3)
newWhitelist(184-188)whitelist(232-252)whitelist(208-230)
artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json (1)
test/Adapters.test.ts (2)
orionConfig(14-117)MockPriceAdapterFactory(95-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Build, Lint and Test
🔇 Additional comments (14)
artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json (1)
173-174: Expected artifact rebuild with ABI compatibility maintained.This is a routine bytecode update reflecting the ecosystem-wide contract changes in this PR. The ABI (lines 5-172) and link references (lines 175-176) remain unchanged, confirming that the adapter's public interface is stable and no breaking changes have been introduced.
artifacts/contracts/test/UtilitiesLibTest.sol/UtilitiesLibTest.json (1)
1-40: Generated artifact - no review needed.This is a compiled artifact automatically generated by Hardhat. The changes reflect the source code modifications in
contracts/test/UtilitiesLibTest.sol.contracts/test/UtilitiesLibTest.sol (1)
7-12: Changes appear unrelated to PR objectives.The PR objectives focus on implementing vault removal and redemption accounting, but these changes only add a named return value to a utility test function. Ensure this change was intentionally included or consider moving it to a separate housekeeping PR.
artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json (1)
1-222: Now let me search for the source file changes to understand what happened:Based on my verification, the inclusion of this artifact file is intentional and correct. Here's what I found:
Source file was modified: The
PriceAdapterRegistry.solsource file was genuinely updated as part of this PR (confirmed in commit 521eee5).ABI remains stable: The contract interface is unchanged—no breaking changes to the public API or function signatures, which is what matters for compatibility.
No blocking dependencies: The PriceAdapterRegistry has no cross-references with vault removal contracts (OrionVault, LiquidityOrchestrator, OrionConfig), confirming this is a safe, unrelated change.
Bytecode changes are expected: Bytecode changes can result from compiler metadata hash updates (even whitespace changes), compiler version differences, or changes to the compilation context. This is normal behavior.
Artifact versioning is intentional: It is safe to commit Hardhat's artifacts directory as it only contains the result of compiling contracts. Many projects commit artifacts for reproducibility and deployment purposes.
No concerns identified. The contract's stable ABI ensures backward compatibility with any existing deployments or integrations. The artifact inclusion is appropriate and supports the PR's goal.
contracts/orchestrators/InternalStatesOrchestrator.sol (1)
465-465: LGTM! Comment improves code clarity.The added comment clarifies that this branch handles the last minibatch before transitioning to the next phase, improving code readability.
artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json (1)
110-111: Artifact rebuild with no ABI changes.The bytecode and deployedBytecode fields have been updated while the ABI remains unchanged, indicating this is a rebuild triggered by changes in dependencies elsewhere in the codebase.
.prettierignore (1)
15-15: LGTM!Adding
contracts/testto the Prettier ignore list is appropriate for excluding test utilities and contracts from formatting checks, and aligns with the similar update to.solhintignore.artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (1)
214-215: Artifact rebuild with no ABI changes.The bytecode update without ABI changes indicates this is a recompilation triggered by dependency updates in the broader PR.
artifacts/contracts/mocks/MockERC4626Asset.sol/MockERC4626Asset.json (1)
852-853: Artifact rebuild with no ABI changes.The bytecode fields have been updated while the ABI remains unchanged, indicating a recompilation of this mock contract.
.solhintignore (1)
5-5: LGTM!Adding
contracts/testto the Solhint ignore list appropriately excludes test contracts from linting, consistent with the.prettierignoreupdate.contracts/vaults/OrionTransparentVault.sol (1)
217-217: LGTM!Adding parameter documentation for
whitelistedAssetsimproves code clarity without any functional changes.artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json (1)
137-155: Verified: Implementation and usage confirmed.The
isDecommissionedVaultfunction is properly implemented inOrionConfig.sol(lines 256-258), with corresponding storage declared at line 61 and actively populated. Usage inLiquidityOrchestratorincludes:
- Line 237: Authorization check combining
isOrionVaultandisDecommissionedVaultfor curator fee transfers- Line 261: Exclusive authorization for decommissioned vaults in withdrawals
Additional usage in
OrionVault.sol(line 222) for synchronous redemption control. All integration points follow proper authorization patterns.contracts/OrionConfig.sol (1)
255-258: LGTM!The
isDecommissionedVaultquery function correctly checks the decommissioned vaults set. However, its correctness depends on fixing the critical issue inremoveOrionVaultto ensure only properly removed vaults are added to this set.artifacts/contracts/vaults/OrionVault.sol/OrionVault.json (1)
1208-1223: ABI updates are consistent with the implementationParam naming and mutability (preview* as view, redeem nonpayable) align with the current OrionVault.sol behavior and ERC‑4626 inheritance. No issues.
Also applies to: 1226-1242, 1245-1261, 1264-1280, 1283-1309
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
contracts/orchestrators/LiquidityOrchestrator.sol (1)
258-262: Add a zero-amount guard inwithdraw.Every other payout path in this contract rejects zero-value transfers to avoid pointless external calls and keep accounting consistent.
withdrawshould do the same before reachingsafeTransfer.if (!config.isDecommissionedVault(msg.sender)) revert ErrorsLib.NotAuthorized(); + if (assets == 0) revert ErrorsLib.AmountMustBeGreaterThanZero(underlyingAsset); SafeERC20.safeTransfer(IERC20(underlyingAsset), receiver, assets); }contracts/OrionConfig.sol (1)
61-63: Consider emitting “decommissioning started” for auditability.When adding to decommissioningInProgressVaults, emit an event (e.g., DecommissioningStarted(vault)) to complement the finalization event. This eases off‑chain tracking.
🧹 Nitpick comments (3)
contracts/interfaces/IOrionConfig.sol (1)
111-117: Update NatSpec to reflect two‑phase removal.removeOrionVault now initiates decommissioning (vault remains registered until completion). Please update the comment to avoid implying immediate deregistration.
test/Removal.test.ts (1)
436-601: Great end‑to‑end coverage for decommissioning + sync redeem.Flow and assertions look correct and resilient. Consider extracting “advanceInternalPhasesUntilIdle” and “advanceLiquidityPhasesUntilIdle” helpers to cut duplication and reduce flake risk across tests.
contracts/OrionConfig.sol (1)
221-232: Avoid external self‑call; add state guards to prevent duplicate initiation.Replace
this.isOrionVault(vault)with direct set checks, and guard against re‑initiating decommissioning or acting on already decommissioned vaults.- if (!this.isOrionVault(vault)) { + bool exists = encryptedVaults.contains(vault) || transparentVaults.contains(vault); + if (!exists) { revert ErrorsLib.InvalidAddress(); } - - // slither-disable-next-line unused-return - decommissioningInProgressVaults.add(vault); + if (decommissionedVaults.contains(vault) || decommissioningInProgressVaults.contains(vault)) { + revert ErrorsLib.AlreadyRegistered(); + } + bool inserted = decommissioningInProgressVaults.add(vault); + if (!inserted) revert ErrorsLib.AlreadyRegistered(); IOrionVault(vault).overrideIntentForDecommissioning();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(4 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(1 hunks)artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json(2 hunks)artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json(1 hunks)artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json(1 hunks)artifacts/contracts/libraries/EventsLib.sol/EventsLib.json(2 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(3 hunks)artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json(1 hunks)artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json(1 hunks)artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json(1 hunks)artifacts/contracts/vaults/OrionVault.sol/OrionVault.json(9 hunks)contracts/OrionConfig.sol(3 hunks)contracts/interfaces/IOrionConfig.sol(2 hunks)contracts/interfaces/IOrionVault.sol(1 hunks)contracts/libraries/EventsLib.sol(1 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(4 hunks)contracts/vaults/OrionTransparentVault.sol(2 hunks)contracts/vaults/OrionVault.sol(3 hunks)test/Orchestrators.test.ts(2 hunks)test/OrionConfigVault.test.ts(0 hunks)test/Removal.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- test/OrionConfigVault.test.ts
✅ Files skipped from review due to trivial changes (1)
- artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/libraries/EventsLib.sol
- artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json
- contracts/vaults/OrionTransparentVault.sol
🧰 Additional context used
🧬 Code graph analysis (7)
artifacts/contracts/libraries/EventsLib.sol/EventsLib.json (1)
test/OrionConfigVault.test.ts (5)
it(555-589)tx(393-422)it(264-423)vaultAddress(305-311)vaultAddress(337-352)
artifacts/contracts/OrionConfig.sol/OrionConfig.json (1)
test/OrionConfigVault.test.ts (9)
describe(426-676)describe(148-424)it(264-423)vaultAddress(337-352)it(555-589)vaultAddress(354-367)tx(393-422)vaultAddress(313-319)vaultAddress(305-311)
test/Orchestrators.test.ts (1)
test/OrionConfigVault.test.ts (8)
vaultAddress(337-352)vaultAddress(354-367)it(264-423)describe(426-676)vaultAddress(305-311)vaultAddress(265-276)describe(148-424)vaultAddress(313-319)
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
test/OrionConfigVault.test.ts (2)
withdrawAmount(448-459)it(427-470)
contracts/interfaces/IOrionConfig.sol (1)
test/OrionConfigVault.test.ts (6)
it(264-423)vaultAddress(337-352)vaultAddress(354-367)vaultAddress(305-311)it(555-589)vaultAddress(313-319)
contracts/OrionConfig.sol (2)
test/OrionConfigVault.test.ts (8)
describe(148-424)describe(426-676)it(264-423)vaultAddress(337-352)vaultAddress(354-367)tx(393-422)it(555-589)maliciousTransparentVault(150-157)test/OrchestratorsZeroState.test.ts (1)
orionConfig(15-173)
test/Removal.test.ts (2)
test/OrionConfigVault.test.ts (4)
describe(148-424)describe(426-676)it(264-423)redeemAmount(461-469)test/TransparentVault.test.ts (3)
describe(128-342)it(295-341)tx(296-340)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (7)
artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (1)
214-215: Artifact bytecode update is expected and contains no breaking changes.The bytecode fields have been regenerated as a side effect of the broader PR changes affecting dependencies or compilation context. No ABI, constructor, or public function signatures have changed, so the contract interface remains stable.
artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json (1)
60-72: ABI surface matches planned lifecycle changes.The new functions and the 1‑parameter removeOrionVault are correctly reflected in the artifact. No issues.
Also applies to: 150-187, 298-309
artifacts/contracts/OrionConfig.sol/OrionConfig.json (1)
27-41: No stale tests or signature mismatches found; code is correct.Verification confirms all
removeOrionVaultcalls in tests (test/Removal.test.ts:507,test/Orchestrators.test.ts:394,test/Orchestrators.test.ts:421) use the correct single-parameter signature. NoOrionVaultRemovedevent references exist in the codebase, and function signatures are consistent across contract, interface, and tests. The ABI changes are properly reflected throughout the codebase.contracts/interfaces/IOrionConfig.sol (1)
129-144: No downstream caller updates required—implementation correctly maintains vault state during decommissioning.The design uses a two-phase decommissioning workflow:
removeOrionVaulttransitions a vault to decommissioning state while keeping it in active vault lists, andcompleteVaultDecommissioninglater removes it. Callers checkingisOrionVault(returnDepositFunds, transferRedemptionFunds) continue to work correctly during the decommissioning phase, and callers gating onisDecommissionedVault(withdraw) only activate after completion. No changes needed.contracts/vaults/OrionVault.sol (3)
297-303: LGTM! Access control and one-way decommissioning flag are correct.The function properly restricts access to the config contract and implements the decommissioning intent override as a one-way operation, which aligns with the terminal nature of vault decommissioning.
220-249: Previous review concerns have been addressed. Implementation looks correct.The past review comment correctly identified missing
nonReentrantmodifier and ERC-4626Withdrawevent emission. Both have been added in this implementation:
- ✓
nonReentrantmodifier added to function signature (line 224)- ✓
Withdrawevent emitted with correct parameters (line 244)Additionally, the implementation follows the CEI pattern correctly:
- Checks: decommissioned status, maxRedeem validation, zero-amount check
- Effects: allowance spending, _totalAssets update, share burning, event emission
- Interactions: external call to liquidityOrchestrator.withdraw
The synchronous redemption for decommissioned vaults with fixed exchange rate (via
previewRedeem) aligns with the PR objectives.
120-122: No issues found. Flag is properly used in derived contracts.The
isDecommissioningflag is correctly implemented:
- Set via
overrideIntentForDecommissioning()in OrionVault.sol:302- Checked in OrionTransparentVault.sol:110 within
getIntent(), where it returns 100% underlying asset weight when trueThe implementation matches the documented behavior.
| function completeVaultDecommissioning(address vault) external { | ||
| if (msg.sender != liquidityOrchestrator) revert ErrorsLib.NotAuthorized(); | ||
| if (!decommissioningInProgressVaults.contains(vault)) revert ErrorsLib.InvalidAddress(); | ||
|
|
||
| // Determine vault type and remove from appropriate vault list | ||
| EventsLib.VaultType vaultType; | ||
| if (encryptedVaults.contains(vault)) { | ||
| vaultType = EventsLib.VaultType.Encrypted; | ||
| // slither-disable-next-line unused-return | ||
| encryptedVaults.remove(vault); | ||
| } else if (transparentVaults.contains(vault)) { | ||
| vaultType = EventsLib.VaultType.Transparent; | ||
| // slither-disable-next-line unused-return | ||
| transparentVaults.remove(vault); | ||
| } else { | ||
| revert ErrorsLib.InvalidAddress(); | ||
| } | ||
|
|
||
| // Remove from decommissioning in progress list | ||
| // slither-disable-next-line unused-return | ||
| decommissioningInProgressVaults.remove(vault); | ||
|
|
||
| // Add to decommissioned vaults list | ||
| // slither-disable-next-line unused-return | ||
| decommissionedVaults.add(vault); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Emit completion event (or remove unused local); improves traceability.
completeVaultDecommissioning computes vaultType but doesn’t use it. Emit the finalization event, or drop the variable.
// Add to decommissioned vaults list
// slither-disable-next-line unused-return
decommissionedVaults.add(vault);
+ // If EventsLib exposes it, emit the finalization event for indexers/auditors.
+ // emit EventsLib.VaultRemovalProcessed(vault, vaultType);If you prefer not to emit here, remove vaultType:
- EventsLib.VaultType vaultType;
- if (encryptedVaults.contains(vault)) {
- vaultType = EventsLib.VaultType.Encrypted;
+ if (encryptedVaults.contains(vault)) {
// slither-disable-next-line unused-return
encryptedVaults.remove(vault);
} else if (transparentVaults.contains(vault)) {
- vaultType = EventsLib.VaultType.Transparent;
// slither-disable-next-line unused-return
transparentVaults.remove(vault);
} else {
revert ErrorsLib.InvalidAddress();
}🏁 Script executed:
#!/bin/bash
# Confirm the event exists and its signature before wiring it
rg -nP 'event\s+VaultRemovalProcessed\s*\(' contracts/libraries/EventsLib.sol -nLength of output: 115
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full context of completeVaultDecommissioning function to see all parameters
rg -nP -A 20 'function\s+completeVaultDecommissioning\s*\(' contracts/OrionConfig.sol
Length of output: 1092
🏁 Script executed:
#!/bin/bash
# Check vault interface/structure to see if totalAssets and curatorFee are stored
rg -nP '(totalAssets|curatorFee)' contracts/OrionConfig.sol -C 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check EventsLib to understand all vault-related events
rg -nP 'event.*Vault' contracts/libraries/EventsLib.sol -nLength of output: 264
🏁 Script executed:
#!/bin/bash
# Verify that vaultType is only computed and never used elsewhere in the function
rg -nP 'vaultType' contracts/OrionConfig.sol -C 2Length of output: 1275
Remove unused vaultType variable; emitting the event is not feasible.
vaultType is computed but never used in completeVaultDecommissioning. Emitting VaultRemovalProcessed is not viable—the event signature requires totalAssets and curatorFee parameters, which the function does not have access to. Simply remove the unused variable:
- EventsLib.VaultType vaultType;
if (encryptedVaults.contains(vault)) {
- vaultType = EventsLib.VaultType.Encrypted;
// slither-disable-next-line unused-return
encryptedVaults.remove(vault);
} else if (transparentVaults.contains(vault)) {
- vaultType = EventsLib.VaultType.Transparent;
// slither-disable-next-line unused-return
transparentVaults.remove(vault);
} else {
revert ErrorsLib.InvalidAddress();
}📝 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.
| function completeVaultDecommissioning(address vault) external { | |
| if (msg.sender != liquidityOrchestrator) revert ErrorsLib.NotAuthorized(); | |
| if (!decommissioningInProgressVaults.contains(vault)) revert ErrorsLib.InvalidAddress(); | |
| // Determine vault type and remove from appropriate vault list | |
| EventsLib.VaultType vaultType; | |
| if (encryptedVaults.contains(vault)) { | |
| vaultType = EventsLib.VaultType.Encrypted; | |
| // slither-disable-next-line unused-return | |
| encryptedVaults.remove(vault); | |
| } else if (transparentVaults.contains(vault)) { | |
| vaultType = EventsLib.VaultType.Transparent; | |
| // slither-disable-next-line unused-return | |
| transparentVaults.remove(vault); | |
| } else { | |
| revert ErrorsLib.InvalidAddress(); | |
| } | |
| // Remove from decommissioning in progress list | |
| // slither-disable-next-line unused-return | |
| decommissioningInProgressVaults.remove(vault); | |
| // Add to decommissioned vaults list | |
| // slither-disable-next-line unused-return | |
| decommissionedVaults.add(vault); | |
| } | |
| function completeVaultDecommissioning(address vault) external { | |
| if (msg.sender != liquidityOrchestrator) revert ErrorsLib.NotAuthorized(); | |
| if (!decommissioningInProgressVaults.contains(vault)) revert ErrorsLib.InvalidAddress(); | |
| // Determine vault type and remove from appropriate vault list | |
| if (encryptedVaults.contains(vault)) { | |
| // slither-disable-next-line unused-return | |
| encryptedVaults.remove(vault); | |
| } else if (transparentVaults.contains(vault)) { | |
| // slither-disable-next-line unused-return | |
| transparentVaults.remove(vault); | |
| } else { | |
| revert ErrorsLib.InvalidAddress(); | |
| } | |
| // Remove from decommissioning in progress list | |
| // slither-disable-next-line unused-return | |
| decommissioningInProgressVaults.remove(vault); | |
| // Add to decommissioned vaults list | |
| // slither-disable-next-line unused-return | |
| decommissionedVaults.add(vault); | |
| } |
🤖 Prompt for AI Agents
In contracts/OrionConfig.sol around lines 263 to 288, the local variable
`vaultType` is declared and assigned but never used; remove its declaration and
all assignments (the if/else branches should only determine which set to
remove), leaving the authorization check, membership checks, the appropriate
removal from `encryptedVaults` or `transparentVaults`, removal from
`decommissioningInProgressVaults`, and addition to `decommissionedVaults`; do
not add any event emission since required event parameters are unavailable.
1 participant
…counting, closes #81
Summary by Sourcery
Implement admin-driven vault removal and enable synchronous redemption accounting for decommissioned vaults
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Refactor
Tests