Dev - #73
Conversation
Reviewer's GuideThis PR overhauls orchestrator and vault interactions by integrating a dedicated liquidity orchestrator in tests and contracts, strengthening authorization via onlyAuthorizedTrigger, unifying deposit and redeem processing in the LiquidityOrchestrator, extending the InternalStatesOrchestrator to track and expose deposit assets and epoch tokens, refining OrionVault’s fulfill logic and event emissions with epoch context, introducing vault owner whitelisting in configuration and enforcing it in factories, and updating interfaces and README badges to reflect these enhancements. Class diagram for updated OrionVault fulfill logic and event emissionsclassDiagram
class OrionVault {
+fulfillDeposit(uint256 depositTotalAssets)
+fulfillRedeem(uint256 redeemTotalAssets)
+event Deposit(address vault, address user, uint256 epoch, uint256 depositAmount, uint256 sharesMinted)
+event Redeem(address vault, address user, uint256 epoch, uint256 redeemAmount, uint256 sharesBurned)
}
File-Level Changes
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-owner whitelisting and enforces it in factories; removes slippage parameters and related error; extends orchestrator APIs, phases, and access gating (onlyAuthorizedTrigger); shifts fulfillDeposit/fulfillRedeem authorization to LiquidityOrchestrator; standardizes Deposit/Redeem events; updates tests and many compiled artifacts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Owner as Owner
participant AR as AutomationRegistry
participant LO as LiquidityOrchestrator
participant ISO as InternalStatesOrchestrator
participant V as OrionVault
rect rgba(220,235,255,0.35)
note over Owner,AR: Authorized triggers (owner or automation registry)
Owner->>LO: performUpkeep()
AR-->>LO: performUpkeep()
LO->>LO: onlyAuthorizedTrigger check
end
LO->>ISO: getEpochTokens()
ISO-->>LO: tokens[]
loop per vault
LO->>ISO: getVaultTotalAssetsForFulfillDeposit(vault)
ISO-->>LO: depositTotalAssets
alt pending deposit
LO->>V: fulfillDeposit(depositTotalAssets)
V-->>LO: ok
V-->>V: emit Deposit(vault,user,epoch,depositAmount,sharesMinted)
end
alt pending redeem
LO->>V: fulfillRedeem(redeemTotalAssets)
V-->>LO: ok
V-->>V: emit Redeem(vault,user,epoch,redeemAmount,sharesBurned)
end
end
LO-->>Owner: upkeep complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `contracts/orchestrators/LiquidityOrchestrator.sol:456-105` </location>
<code_context>
+ /// @param vault The vault address
+ /// @param totalAssetsForDeposit The total assets for deposit operations
+ /// @param totalAssetsForRedeem The total assets for redeem operations
+ function _processVaultDepositAndRedeem(
+ address vault,
+ uint256 totalAssetsForDeposit,
+ uint256 totalAssetsForRedeem
+ ) internal {
+ IOrionVault vaultContract = IOrionVault(vault);
+
+ // Check if vault has pending deposits and redemptions
+ uint256 pendingDeposit = vaultContract.pendingDeposit();
+ uint256 pendingRedeem = vaultContract.pendingRedeem();
+
+ if (pendingDeposit > 0) {
+ // Only deposits exist
+ vaultContract.fulfillDeposit(totalAssetsForDeposit);
+ } else if (pendingRedeem > 0) {
+ // Only redemptions exist
+ vaultContract.fulfillRedeem(totalAssetsForRedeem);
}
+ // If neither exists, do nothing
}
</code_context>
<issue_to_address>
**question:** Vault deposit and redeem logic only processes one or the other per call.
If both pendingDeposit and pendingRedeem are nonzero, only the deposit is processed. Should both be handled in the same call? If not intentional, consider updating the logic to process both sequentially.
</issue_to_address>
### Comment 2
<location> `contracts/vaults/OrionVault.sol:570-574` </location>
<code_context>
}
_pendingDeposit = 0;
+ uint16 currentEpoch = internalStatesOrchestrator.epochCounter();
// Process all requests
</code_context>
<issue_to_address>
**suggestion:** Epoch counter is used for event emission, but may not be consistent if called mid-epoch.
If these functions are called before the epoch is updated, emitted events may show the previous epoch, potentially confusing event consumers.
```suggestion
_pendingDeposit = 0;
// Fetch the latest epoch value right before event emission to ensure consistency
uint16 currentEpoch = internalStatesOrchestrator.epochCounter();
// Process all requests
for (uint32 i = 0; i < length; ++i) {
uint256 shares = convertToSharesWithPITTotalAssets(amount, depositTotalAssets, Math.Rounding.Floor);
_mint(user, shares);
// Fetch epoch again in case it changes during processing (if relevant)
uint16 epochForEvent = internalStatesOrchestrator.epochCounter();
emit Deposit(address(this), user, epochForEvent, amount, shares);
}
```
</issue_to_address>
### Comment 3
<location> `contracts/OrionConfig.sol:54` </location>
<code_context>
// Vault-specific configuration
using EnumerableSet for EnumerableSet.AddressSet;
EnumerableSet.AddressSet private whitelistedAssets;
+ EnumerableSet.AddressSet private whitelistedVaultOwners;
/// @notice Mapping of token address to its decimals
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Whitelisted vault owners are tracked, but not removed on asset removal.
Ensure whitelistedVaultOwners is updated when a vault owner is removed or an asset is de-whitelisted to avoid stale data.
Suggested implementation:
```
EnumerableSet.AddressSet private whitelistedAssets;
EnumerableSet.AddressSet private whitelistedVaultOwners;
```
```
// slither-disable-next-line unused-return
whitelistedAssets.add(underlyingAsset_);
// slither-disable-next-line unused-return
whitelistedVaultOwners.add(initialOwner);
}
/// @notice Remove an asset from the whitelist and its associated vault owner
function removeWhitelistedAsset(address asset, address vaultOwner) external onlyOwner {
// slither-disable-next-line unused-return
whitelistedAssets.remove(asset);
// Remove the vault owner from the whitelist if present
if (whitelistedVaultOwners.contains(vaultOwner)) {
// slither-disable-next-line unused-return
whitelistedVaultOwners.remove(vaultOwner);
}
}
/// @notice Remove a vault owner from the whitelist
function removeWhitelistedVaultOwner(address vaultOwner) external onlyOwner {
// slither-disable-next-line unused-return
whitelistedVaultOwners.remove(vaultOwner);
}
```
You may need to:
1. Update any places in your codebase that remove assets or vault owners to use these new functions.
2. Ensure that you have access to the vault owner address when removing an asset, or maintain a mapping from asset to owner if needed.
3. Add events for removals if your contract emits events for such changes.
</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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json (1)
108-140: Add missing Deposit event emission in OrionTransparentVault.sol
The TransparentVault implementation doesn’t emit the updated Deposit(vault, user, epoch, depositAmount, sharesMinted) event. In contracts/vaults/OrionTransparentVault.sol, emitDeposit(address(this), user, currentEpoch, amount, shares)(or renamed parameters) at the end of its deposit function.
artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json (1)
108-241: Emit Deposit and Redeem events in OrionEncryptedVault implementation.The interface defines
DepositandRedeemevents, butcontracts/vaults/OrionEncryptedVault.soldoes not emit them. Addemit Deposit(…)in your deposit logic andemit Redeem(…)in your redemption logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
README.md(1 hunks)artifacts/contracts/OrionConfig.sol/OrionConfig.json(3 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(1 hunks)artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json(2 hunks)artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json(2 hunks)artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json(3 hunks)artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json(3 hunks)artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json(3 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(1 hunks)artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json(1 hunks)artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json(1 hunks)artifacts/contracts/vaults/OrionVault.sol/OrionVault.json(3 hunks)contracts/OrionConfig.sol(3 hunks)contracts/factories/EncryptedVaultFactory.sol(1 hunks)contracts/factories/TransparentVaultFactory.sol(1 hunks)contracts/interfaces/IInternalStateOrchestrator.sol(1 hunks)contracts/interfaces/ILiquidityOrchestrator.sol(1 hunks)contracts/interfaces/IOrionConfig.sol(1 hunks)contracts/interfaces/IOrionVault.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(10 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(8 hunks)contracts/vaults/OrionVault.sol(5 hunks)test/Orchestrators.test.ts(2 hunks)test/OrionVaultExchangeRate.test.ts(17 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/OrionConfig.sol (1)
test/OrionConfigVault.test.ts (4)
owner(37-156)it(169-243)it(245-265)it(615-637)
⏰ 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 (24)
artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json (1)
175-176: Bytecode refresh looks consistent.No issues spotted with the regenerated bytecode/deployedBytecode; update aligns with the broader contract changes.
contracts/interfaces/IOrionConfig.sol (2)
96-99: LGTM!The function signature and documentation are clear and consistent with the existing whitelist pattern for assets.
101-104: LGTM!The function signature and documentation are clear and follow the established whitelist query pattern.
artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json (2)
47-59: LGTM!The ABI entry correctly represents the
addWhitelistedVaultOwnerfunction from the interface.
188-206: LGTM!The ABI entry correctly represents the
isWhitelistedVaultOwnerview function from the interface.artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json (1)
91-92: Bytecode-only artifact update.The bytecode has been updated without ABI changes, likely due to recompilation. No review required for bytecode-only changes.
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
654-655: Bytecode-only artifact update.The bytecode has been updated without ABI changes, likely due to recompilation. No review required for bytecode-only changes.
artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json (1)
205-241: Redeem event emission and Withdraw removal verified
Redeem is emitted in contracts/vaults/OrionVault.sol:615 with the correct parameters; no remaining Withdraw emissions found.contracts/factories/EncryptedVaultFactory.sol (1)
45-45: Whitelist initialization and unauthorized access tests confirmed
- OrionConfig constructor auto-whitelists
initialOwner.- UnauthorizedAccess revert cases are covered in existing tests.
- Confirm or add tests for successful vault creation by whitelisted owners (e.g., calling
addWhitelistedVaultOwnerin setup).contracts/interfaces/ILiquidityOrchestrator.sol (1)
16-16: Enum variant rename verified: AllLiquidityUpkeepPhase.FulfillRedeemreferences have been replaced withLiquidityUpkeepPhase.FulfillDepositAndRedeemin contracts and tests; no remaining old enum usage.contracts/factories/TransparentVaultFactory.sol (1)
45-45: LGTM! Whitelist-based access control is enforced.The change replaces the zero-address check with whitelist validation, aligning with the new vault-owner whitelisting model introduced in OrionConfig. The error type
UnauthorizedAccessis semantically appropriate for this access control scenario.artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json (2)
108-141: LGTM! Enhanced Deposit event structure.The updated
Depositevent now includesvault,user,epoch,depositAmount, andsharesMintedfields with appropriate indexing. This standardization improves deposit fulfillment tracking and enables efficient event filtering.
205-241: LGTM! New Redeem event standardizes redemption tracking.The new
Redeemevent provides symmetry with the updatedDepositevent, includingvault,user,epoch,redeemAmount, andsharesBurnedfields with appropriate indexing. This standardization improves redemption tracking and event filtering.artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json (2)
92-104: LGTM! New getter enhances epoch token observability.The
getEpochTokens()function exposes the list of tokens for the current epoch, improving transparency and enabling external systems to query epoch-specific token information.
162-180: LGTM! New getter enhances per-vault deposit tracking.The
getVaultTotalAssetsForFulfillDeposit(address vault)function exposes per-vault deposit fulfillment assets, improving transparency and enabling external systems to query vault-specific deposit information.artifacts/contracts/OrionConfig.sol/OrionConfig.json (2)
200-212: LGTM! New function enables vault owner whitelisting.The
addWhitelistedVaultOwner(address vaultOwner)function allows the contract owner to add vault owners to the whitelist, implementing the new access control model introduced in this PR.
354-372: LGTM! New function supports vault owner whitelist checks.The
isWhitelistedVaultOwner(address vaultOwner)function enables external contracts (e.g., vault factories) to verify vault owner whitelist membership, supporting the new access control model.contracts/interfaces/IOrionVault.sol (1)
49-74: LGTM! Standardized event signatures improve tracking.The updated
Depositevent and newRedeemevent includevault,user,epoch, and amount/shares fields with appropriate indexing. These standardized signatures improve deposit/redemption tracking and enable efficient event filtering across the vault lifecycle.contracts/OrionConfig.sol (3)
54-54: LGTM! New storage tracks whitelisted vault owners.The
whitelistedVaultOwnersEnumerableSet follows the same pattern aswhitelistedAssets, providing efficient membership checks and iteration capabilities for the new vault-owner whitelisting feature.
89-90: LGTM! Initial owner seeded in whitelist.The constructor adds the initial owner to the
whitelistedVaultOwnersset, ensuring the owner can create vaults immediately after deployment without requiring an additional transaction to whitelist themselves.
192-201: LGTM! New functions implement vault owner whitelisting.The
addWhitelistedVaultOwnerfunction (owner-only) andisWhitelistedVaultOwnerfunction (public view) implement the new vault-owner whitelisting feature. TheAlreadyRegisteredcheck prevents duplicate entries, and the access control is appropriate.contracts/interfaces/IInternalStateOrchestrator.sol (1)
105-113: LGTM! New functions expand orchestrator state exposure.The
getVaultTotalAssetsForFulfillDeposit(address vault)andgetEpochTokens()functions enhance observability by exposing per-vault deposit assets and epoch tokens. Note the dev comment indicatinggetEpochTokens()blocks if the orchestrator is not idle—ensure callers handle this appropriately.test/Orchestrators.test.ts (2)
739-774: LGTM! Comprehensive access control testing for performUpkeep.The new tests validate that both the owner and automation registry can call
performUpkeepon the internal states orchestrator, while unauthorized addresses are correctly blocked with theNotAuthorizederror. This aligns with the newonlyAuthorizedTriggerpattern introduced in this PR.
788-817: LGTM! Liquidity orchestrator access control tests.The new tests validate that both the owner and automation registry can call
performUpkeepon the liquidity orchestrator when upkeep is needed, while unauthorized addresses are correctly blocked with theNotAuthorizederror. The conditional guards based onupkeepNeededprevent false negatives and improve test reliability.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
contracts/orchestrators/LiquidityOrchestrator.sol (1)
493-499: Critical: if/else if prevents concurrent deposit and redeem processing.The
if/else ifstructure on lines 493-499 means when a vault has bothpendingDeposit > 0ANDpendingRedeem > 0, only the deposit is fulfilled. The redeem remains pending until the next epoch. This creates several problems:
- User experience: Redeemers wait an extra epoch unnecessarily
- Share price distortion: The deposit mint increases
totalSupplybefore redemptions are processed, slightly diluting existing shares- Inconsistent state: The vault tracks both pending amounts but only processes one
Based on past review comments, this issue was already flagged. Apply this fix to process both operations in the same cycle, executing redemptions before deposits to maintain accurate pricing:
- if (pendingDeposit > 0) { - // Only deposits exist - vaultContract.fulfillDeposit(totalAssetsForDeposit); - } else if (pendingRedeem > 0) { - // Only redemptions exist + if (pendingRedeem > 0) { vaultContract.fulfillRedeem(totalAssetsForRedeem); } + + if (pendingDeposit > 0) { + vaultContract.fulfillDeposit(totalAssetsForDeposit); + }
🧹 Nitpick comments (2)
contracts/orchestrators/InternalStatesOrchestrator.sol (1)
495-499: FHEVM mulDiv optimization tracked as technical debt.The TODO comment appropriately flags the need for a proper FHEVM
mulDivimplementation with type upcasting. This optimization should be tracked and prioritized based on gas profiling results.Do you want me to open a new issue to track this FHEVM optimization work?
contracts/orchestrators/LiquidityOrchestrator.sol (1)
436-444: Verify the 2x approval buffer is sufficient.Line 441 approves
estimatedUnderlyingAmount * 2for the buy operation, providing a generous buffer for price movement. This assumes prices won't move more than 100% during execution.Consider whether this 2x buffer is appropriate for your use case. If assets are highly volatile or if there's significant time between estimation and execution, this might be insufficient. You may want to:
- Make the multiplier configurable via a parameter
- Add explicit bounds checking after the buy to ensure the actual amount doesn't exceed reasonable limits
- Document the rationale for the 2x buffer
Do you want me to propose a more flexible approach?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.gitignore(1 hunks).prettierignore(1 hunks)artifacts/contracts/OrionConfig.sol/OrionConfig.json(3 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(1 hunks)artifacts/contracts/interfaces/IExecutionAdapter.sol/IExecutionAdapter.json(0 hunks)artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json(3 hunks)artifacts/contracts/interfaces/ILiquidityOrchestrator.sol/ILiquidityOrchestrator.json(3 hunks)artifacts/contracts/libraries/ErrorsLib.sol/ErrorsLib.json(1 hunks)artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json(1 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(4 hunks)artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json(1 hunks)artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json(1 hunks)contracts/OrionConfig.sol(3 hunks)contracts/execution/OrionAssetERC4626ExecutionAdapter.sol(2 hunks)contracts/interfaces/IExecutionAdapter.sol(1 hunks)contracts/interfaces/IInternalStateOrchestrator.sol(2 hunks)contracts/interfaces/ILiquidityOrchestrator.sol(3 hunks)contracts/libraries/ErrorsLib.sol(0 hunks)contracts/mocks/MockExecutionAdapter.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(11 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(10 hunks)eslint.config.mjs(1 hunks)test/Orchestrators.test.ts(4 hunks)
💤 Files with no reviewable changes (2)
- contracts/libraries/ErrorsLib.sol
- artifacts/contracts/interfaces/IExecutionAdapter.sol/IExecutionAdapter.json
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json
- artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json
🧰 Additional context used
🧬 Code graph analysis (2)
contracts/orchestrators/InternalStatesOrchestrator.sol (1)
test/OrionVaultExchangeRate.test.ts (3)
it(126-184)it(386-431)loadFixture(241-286)
contracts/execution/OrionAssetERC4626ExecutionAdapter.sol (1)
test/OrionConfigVault.test.ts (3)
it(537-554)it(651-676)it(428-471)
⏰ 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: Build, Lint and Test
- GitHub Check: Sourcery review
🔇 Additional comments (46)
.prettierignore (1)
14-14: LGTM!The addition of
orion-strategiesto the Prettier ignore list is correct and consistent with the file's existing format.eslint.config.mjs (1)
35-35: LGTM!The addition of
"**/orion-strategies"to the ESLint global ignores is correct and follows the established pattern for directory exclusions.artifacts/contracts/OrionConfig.sol/OrionConfig.json (1)
200-212: LGTM: Artifact correctly reflects contract changes.The ABI additions for
addWhitelistedVaultOwnerandisWhitelistedVaultOwner, along with the updated bytecode, accurately represent the new whitelist functionality in the Solidity source. As these are auto-generated artifacts, the primary review focuses on the source contract.Also applies to: 354-372, 618-619
contracts/OrionConfig.sol (3)
54-54: LGTM: Storage for vault owner whitelist.The addition of
whitelistedVaultOwnersusingEnumerableSetis consistent with the existingwhitelistedAssetspattern and provides efficient set operations for access control.
88-89: LGTM: Initial owner whitelisted in constructor.Adding the
initialOwnerto the whitelist is appropriate, as they should have permission to create vaults from the start. The zero address validation is already handled by OpenZeppelin'sOwnableconstructor.
197-200: LGTM: Whitelist membership check.The view function correctly checks if an address is in the
whitelistedVaultOwnersset.contracts/interfaces/ILiquidityOrchestrator.sol (3)
16-16: Enum member renamed to reflect broader scope.The rename from
FulfillRedeemtoFulfillDepositAndRedeemaccurately reflects the expanded scope where both deposit and redeem operations are now processed together in this phase, aligning with the per-vault fulfillment flow introduced across the PR.
40-42: Buffer-ratio-based liquidity management replaces slippage bounds.The function rename from
setSlippageBoundtosetTargetBufferRatioreflects a strategic shift in liquidity management. This change is consistent with the removal ofSlippageExceedederror handling and introduction of explicit buffer management throughdepositLiquidity/withdrawLiquidityfunctions.
77-86: Explicit buffer management functions added.The new
depositLiquidityandwithdrawLiquidityfunctions provide explicit, owner-controlled buffer management. The safety checks mentioned forwithdrawLiquidityare appropriate to prevent predatory withdrawals that could disrupt protocol operations.contracts/orchestrators/InternalStatesOrchestrator.sol (9)
100-101: Per-vault deposit fulfillment tracking added.The
vaultsTotalAssetsForFulfillDepositmapping appropriately tracks vault state for deposit fulfillment, complementing the existing redeem tracking and supporting the new per-vault processing flow.
153-159: Dual authorization for upkeep triggers.The new
onlyAuthorizedTriggermodifier allows both the owner and the Chainlink Automation Registry to triggerperformUpkeep, replacing the previous registry-only restriction. This improves operational flexibility by enabling manual intervention when needed.Verify that dual authorization is intentional and consider documenting any additional security considerations for owner-triggered upkeep in production scenarios.
236-238: Maximum volume fee tightened from 1% to 0.5%.The protocol fee cap reduction from 1% to 0.5% is a more conservative limit that may improve protocol competitiveness. Ensure this tightened constraint aligns with economic modeling and won't disrupt existing deployments expecting the previous 1% maximum.
283-283: performUpkeep authorization updated.The modifier change to
onlyAuthorizedTriggerapplies the dual authorization (owner and registry) toperformUpkeep, consistent with the new access control pattern.
332-332: Epoch state cleanup includes new deposit fulfillment tracking.Properly clears
vaultsTotalAssetsForFulfillDepositat epoch start, preventing stale data carryover and maintaining consistent state management.
432-432: Deposit fulfillment state captured at correct point in preprocessing.Both transparent (Line 432) and encrypted (Line 610) vault preprocessing correctly set
vaultsTotalAssetsForFulfillDepositafter subtracting pending redeems but before adding pending deposits, capturing the appropriate totalAssets for deposit fulfillment calculations.Also applies to: 610-610
823-823: Idle-state guards added to public getters.The
SystemNotIdlereverts added togetOrders(Line 823),getEpochTokens(Line 915), andgetPriceOf(Line 921) prevent reading inconsistent state during epoch processing. This is a breaking change for callers.Verify that external integrations and off-chain systems can handle or avoid calling these functions during non-idle phases. Consider whether a view function that returns both the phase and the data might be useful for callers who need to check availability.
Also applies to: 915-915, 921-921
913-917: Epoch tokens list exposed via public getter.The new
getEpochTokens()function appropriately exposes the current epoch's token list with an idle-state guard to prevent reading inconsistent data.
947-952: Consider idle-state guard for deposit fulfillment getter.The new
getVaultTotalAssetsForFulfillDepositfunction exposes per-vault deposit fulfillment state but lacks an idle-state guard, unlike other similar getters.Verify whether this getter should also have a
SystemNotIdleguard likegetEpochTokensandgetOrders, or if reading this value during processing is intentionally allowed. The similar gettergetVaultTotalAssetsForFulfillRedeem(Line 943) also lacks a guard, but consistency across the API would be clearer with explicit design intent documented.artifacts/contracts/libraries/ErrorsLib.sol/ErrorsLib.json (1)
115-116: Artifact updated consistently with error removal.The bytecode hashes reflect the removal of the
SlippageExceedederror, consistent with the broader shift away from slippage-based constraints across the codebase.artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (5)
48-52: InsufficientAmount error added to support buffer validation.The
InsufficientAmounterror addition supports validation logic in the new liquidity management functions.
326-338: depositLiquidity function added to ABI.Correctly reflects the new buffer deposit functionality in the public interface.
540-540: Buffer ratio setter replaces slippage bound setter.The rename from
setSlippageBoundtosetTargetBufferRatiois correctly reflected in the ABI, consistent with the strategic shift in liquidity management.Also applies to: 544-544
658-670: withdrawLiquidity function added to ABI.Correctly reflects the new buffer withdrawal functionality in the public interface.
672-673: Bytecode updated with new functionality.The artifact bytecode reflects all the ABI changes including new liquidity management functions and renamed setters.
artifacts/contracts/interfaces/ILiquidityOrchestrator.sol/ILiquidityOrchestrator.json (1)
56-68: Interface artifact consistent with source changes.The ABI correctly reflects the interface changes:
depositLiquidityandwithdrawLiquidityadditions, and thesetSlippageBound→setTargetBufferRatiorename.Also applies to: 135-135, 139-139, 227-239
contracts/mocks/MockExecutionAdapter.sol (1)
13-15: Mock adapter signatures simplified.The removal of
maxUnderlyingAmountandminUnderlyingAmountparameters frombuyandsellaligns the mock with the simplifiedIExecutionAdapterinterface and the broader removal of slippage-based constraints.Also applies to: 18-20
artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json (1)
160-161: Execution adapter artifact updated consistently.The bytecode reflects the removal of slippage parameters from
buyandsellfunctions, consistent with the interface simplification across the codebase.artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json (1)
6-18: New state exposure functions added to interface.The three new getters (
bufferAmount,getEpochTokens, andgetVaultTotalAssetsForFulfillDeposit) appropriately expose internal orchestrator state to support the new per-vault fulfillment and buffer management flows.Also applies to: 105-117, 175-193
contracts/interfaces/IExecutionAdapter.sol (1)
18-24: LGTM! Simplified interface aligns with centralized slippage handling.The removal of slippage parameters (
minUnderlyingAmount,maxUnderlyingAmount) from the adapter interface is a valid architectural choice that centralizes slippage protection at the orchestrator level. This simplification makes the adapter interface cleaner and easier to implement.contracts/interfaces/IInternalStateOrchestrator.sol (2)
57-59: LGTM! Buffer amount exposure supports external monitoring.The new
bufferAmount()getter enables external visibility into the protocol's liquidity buffer state.
108-117: LGTM! Per-vault asset queries support new fulfillment flow.The addition of
getVaultTotalAssetsForFulfillDepositandgetEpochTokensproperly extends the interface to support the refactored deposit and redeem fulfillment logic. The dev note on Line 116 appropriately warns about the blocking behavior when the orchestrator is not idle.artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json (1)
60-61: Artifact update aligns with interface changes.The bytecode and ABI updates correctly reflect the simplified
buyandsellsignatures from the interface changes.contracts/execution/OrionAssetERC4626ExecutionAdapter.sol (2)
50-66: LGTM! Sell logic correctly simplified.The removal of
minUnderlyingAmountshifts slippage protection to the orchestrator level. The vault asset validation via try-catch (lines 54-58) is a good security practice. The zero-amount check (line 59) prevents wasteful transactions.
69-100: LGTM! Buy logic correctly simplified with proper cleanup.The implementation correctly:
- Validates the vault asset (lines 73-77)
- Prevents zero-amount buys (line 78)
- Uses
previewMintto calculate costs (line 82)- Cleans up approvals after execution (line 95)
- Transfers shares to caller (line 98)
test/Orchestrators.test.ts (4)
168-184: LGTM! Target buffer ratio validation is thorough.The test properly validates:
- Zero ratio is rejected (line 172)
- Ratio > 500 (5%) is rejected (line 177)
- Valid ratios (1 and 400) are accepted (lines 183-184)
738-773: LGTM! Authorization model properly tested.The three tests comprehensively verify that:
- Owner can call
performUpkeep(lines 738-747)- Automation registry can call
performUpkeep(lines 749-759)- Unauthorized addresses are rejected with
NotAuthorizederror (lines 761-773)
787-816: LGTM! Liquidity orchestrator authorization properly tested.The tests correctly handle the conditional nature of liquidity orchestrator upkeep (checking
liquidityUpkeepNeededbefore attempting calls) and verify the authorization model.
818-824: LGTM! Edge case coverage for buffer ratio.The test verifies typical buffer ratio (100 = 1%) works correctly.
contracts/orchestrators/LiquidityOrchestrator.sol (8)
104-110: LGTM! Authorization expansion enables manual triggering.The new
onlyAuthorizedTriggermodifier properly allows both the contract owner and the Chainlink Automation Registry to callperformUpkeep, enabling manual intervention when needed. This is a sensible operational improvement.
163-169: LGTM! Target buffer ratio validation is appropriate.The validation correctly:
- Rejects zero ratio (line 164)
- Enforces maximum of 5% (500 basis points, line 166)
- Requires system to be idle (line 167)
172-201: LGTM! Liquidity management functions are well-implemented.Both
depositLiquidityandwithdrawLiquidityproperly:
- Validate non-zero amounts
- Check system is idle
- Update the internal states orchestrator buffer
- Handle token transfers with error checking
The withdrawal function includes an appropriate safety check (line 193) to prevent negative buffer amounts.
72-73: LGTM! Constant renaming reflects unified fulfillment flow.The rename from
ACTION_PROCESS_FULFILL_REDEEMtoACTION_PROCESS_FULFILL_DEPOSIT_AND_REDEEMaccurately describes the combined processing of both deposits and redemptions in a single phase.
287-293: LGTM! Phase transition logic updated correctly.The phase name change from
FulfillRedeemtoFulfillDepositAndRedeem(line 287) aligns with the unified fulfillment approach.
298-298: LGTM! Authorization enforced at entry point.The
performUpkeepfunction correctly uses the newonlyAuthorizedTriggermodifier to enforce access control.
423-423: LGTM! Adapter call simplified.The
sellcall now passes only(asset, sharesAmount), consistent with the simplified interface.
449-476: LGTM! Unified fulfillment processing is well-structured.The
_processFulfillDepositAndRedeemfunction correctly:
- Validates the current phase (line 451)
- Transitions to Idle (line 455)
- Processes both transparent and encrypted vaults (lines 457-475)
- Retrieves per-vault totals before processing
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
contracts/orchestrators/LiquidityOrchestrator.sol (1)
494-501: Process redemptions before deposits to preserve correct share pricing.The current implementation processes deposits (lines 494-496) before redemptions (lines 499-501). This order can inflate the vault's
totalSupplybefore redemptions are calculated, causing redeemers to receive fewer underlying assets than they should. Previous reviews explicitly flagged this issue.Apply this diff to fix the order:
- // Process deposits if pending - if (pendingDeposit > 0) { - vaultContract.fulfillDeposit(totalAssetsForDeposit); - } - // Process redemptions if pending if (pendingRedeem > 0) { vaultContract.fulfillRedeem(totalAssetsForRedeem); } + + // Process deposits if pending + if (pendingDeposit > 0) { + vaultContract.fulfillDeposit(totalAssetsForDeposit); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/orchestrators/LiquidityOrchestrator.sol(10 hunks)
⏰ 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 (6)
contracts/orchestrators/LiquidityOrchestrator.sol (6)
104-110: LGTM! Flexible access control for upkeep triggering.The new modifier appropriately allows both the contract owner and Chainlink Automation Registry to trigger upkeep operations, providing operational flexibility while maintaining access control.
287-290: LGTM! Phase check updated correctly.The condition correctly checks for the renamed
FulfillDepositAndRedeemphase and encodes the appropriate action constant.
298-298: LGTM! Access control updated for flexible triggering.The modifier change correctly implements the new access control model, allowing both owner and Chainlink Automation Registry to perform upkeep.
309-310: LGTM! Action handler updated correctly.The handler correctly processes the renamed action and delegates to the new fulfillment function.
392-392: LGTM! Phase transition updated correctly.The phase transition correctly moves to
FulfillDepositAndRedeemafter the buying leg completes.
163-169: No action required:targetBufferRatiois used in InternalStatesOrchestrator.sol (line 662) for buffer calculations.Likely an incorrect or invalid review comment.
1 participant
Summary by Sourcery
Update orchestrator and vault contracts to enhance access control, unify deposit/redeem logic, and enrich event data, while adjusting tests and configuration to support the new workflows and whitelisting features.
New Features:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Changes
Access Control
Documentation / Chores
Tests