Dev - #116
Conversation
…based, and submitIntent enforces non-malicious intent validation
- Atomic validation in buy/sell operations - Atomic validation in buy/sell operations - ERC4626 interface validation - Token decimals validation - updateTokenDecimals admin function - Zero totalAssets check - Slippage tolerance = 50% of targetBufferRatio - buyApprovalMultiplier removed - MAX_BUY_APPROVAL_MULTIPLIER removed - emergencyUpdateExecutionAdapter - Slippage propagation to adapters closes #99
… of LO; avoids splitting redeem/deposit into two updates due to minibatch-size constraints
… and deprecate old exchange-ratio logic
Reviewer's GuideRefactors epoch processing and slippage handling across InternalStatesOrchestrator, LiquidityOrchestrator, execution adapters, vaults and strategies, while removing curator whitelisting and updating tests to the new APIs and event semantics. Sequence diagram for updated epoch lifecycle and vault operationssequenceDiagram
actor Keeper as ChainlinkAutomation
participant ISO as InternalStatesOrchestrator
participant LO as LiquidityOrchestrator
participant Config as OrionConfig
participant TV as OrionTransparentVault
Keeper->>ISO: performUpkeep(performData)
ISO->>ISO: decode performData -> processLP, excludedAssets
ISO->>ISO: set processLP
ISO->>ISO: if config.isSystemIdle() && _shouldTriggerUpkeep()
ISO-->>ISO: _handleStart()
ISO->>Config: getAllOrionVaults(Transparent)
ISO->>ISO: build transparentVaultsEpoch
ISO->>ISO: currentPhase = PreprocessingTransparentVaults
loop Preprocessing minibatches
ISO->>ISO: _preprocessTransparentMinibatch()
end
ISO->>ISO: currentPhase = Buffering
ISO->>ISO: _buffer()
ISO->>ISO: currentPhase = PostprocessingTransparentVaults
loop Postprocessing minibatches
ISO->>ISO: _postprocessTransparentMinibatch(excludedAssets)
ISO->>ISO: update vaultsTotalAssets, vaultsTotalAssetsForFulfillRedeem, vaultsTotalAssetsForFulfillDeposit, vaultPortfolioTokens, vaultPortfolioShares
end
ISO->>ISO: currentPhase = BuildingOrders
ISO->>ISO: _buildOrders()
ISO->>LO: advanceIdlePhase()
LO->>LO: if currentPhase == Idle
LO->>LO: currentPhase = SellingLeg
Note over LO,ISO: LiquidityOrchestrator trading phases
loop SellingLeg
Keeper->>LO: checkUpkeep/performUpkeep
LO->>LO: _processSellLeg()
end
loop BuyingLeg
Keeper->>LO: checkUpkeep/performUpkeep
LO->>LO: _processBuyLeg()
end
LO->>ISO: getOrders(false)
LO->>LO: currentPhase = ProcessVaultOperations
loop ProcessVaultOperations minibatches
Keeper->>LO: performUpkeep
LO->>Config: getAllOrionVaults(Transparent)
LO->>ISO: getVaultTotalAssetsAll(vault)
LO->>ISO: getVaultPortfolio(vault)
LO->>TV: _processSingleVaultOperations(vault, totalAssetsForDeposit, totalAssetsForRedeem, finalTotalAssets)
end
LO->>LO: currentPhase = Idle
LO->>LO: currentMinibatchIndex = 0
LO->>LO: emit EpochProcessed(epochCounter)
LO->>LO: ++epochCounter
ISO->>ISO: currentPhase = Idle
Sequence diagram for updated buy/sell execution with slippage checkssequenceDiagram
participant LO as LiquidityOrchestrator
participant Exec as OrionAssetERC4626ExecutionAdapter
participant VA as ERC4626VaultAsset
participant UA as UnderlyingAssetToken
rect rgb(230,230,250)
Note over LO,Exec: Sell flow
LO->>Exec: sell(asset, sharesAmount, estimatedUnderlying)
Exec->>Exec: _validateExecutionAdapter(asset)
Exec->>VA: redeem(sharesAmount, msg.sender, msg.sender)
VA-->>Exec: receivedUnderlying
Exec->>Exec: if receivedUnderlying < estimatedUnderlying
Exec->>LO: read slippageTolerance()
Exec->>Exec: maxUnderlying = estimatedUnderlying * (BASIS_POINTS_FACTOR - slippageTolerance) / BASIS_POINTS_FACTOR
Exec->>Exec: if receivedUnderlying < maxUnderlying
Exec-->>LO: revert SlippageExceeded
Exec-->>LO: else return receivedUnderlying
end
rect rgb(230,250,230)
Note over LO,Exec: Buy flow
LO->>Exec: buy(asset, sharesAmount, estimatedUnderlying)
Exec->>Exec: _validateExecutionAdapter(asset)
Exec->>VA: previewMint(sharesAmount)
VA-->>Exec: previewedUnderlying
Exec->>Exec: if previewedUnderlying > estimatedUnderlying
Exec->>LO: read slippageTolerance()
Exec->>Exec: maxUnderlying = estimatedUnderlying * (BASIS_POINTS_FACTOR + slippageTolerance) / BASIS_POINTS_FACTOR
Exec->>Exec: if previewedUnderlying > maxUnderlying
Exec-->>LO: revert SlippageExceeded
end
Exec->>UA: safeTransferFrom(LO, Exec, previewedUnderlying)
Exec->>UA: forceApprove(VA, previewedUnderlying)
Exec->>VA: mint(sharesAmount, Exec)
VA-->>Exec: spentUnderlying
Exec->>UA: forceApprove(VA, 0)
Exec->>VA: safeTransfer(LO, sharesAmount)
Exec-->>LO: return spentUnderlying
Class diagram for updated orchestrators, vaults, adapters, and interfacesclassDiagram
class IInternalStateOrchestrator {
<<interface>>
+InternalUpkeepPhase currentPhase()
+void updateAutomationRegistry(address newAutomationRegistry)
+void updateProtocolFees(uint16 vFeeCoefficient, uint16 rsFeeCoefficient)
+void resetPhase(InternalUpkeepPhase targetPhase)
+uint256 pendingProtocolFees()
+uint256 bufferAmount()
+bool processLP()
+void subtractPendingProtocolFees(uint256 amount)
+void updateBufferAmount(int256 deltaAmount)
+tuple getVaultTotalAssetsAll(address vault)
+address[] getTokens()
+address[] getTransparentVaultsEpoch()
+tuple getVaultPortfolio(address vault)
+void pause()
+void unpause()
}
class InternalStatesOrchestrator {
+uint32 MAX_EPOCH_DURATION
+uint8 transparentMinibatchSize
+uint32 epochDuration
+uint256 bufferAmount
+bool processLP
+void performUpkeep(bytes performData)
+void resetPhase(InternalUpkeepPhase targetPhase)
+tuple getVaultTotalAssetsAll(address vault)
+address[] getTransparentVaultsEpoch()
+tuple getVaultPortfolio(address vault)
+bool processLP()
-EpochState _currentEpoch
-void _handleStart()
-void _preprocessTransparentMinibatch()
-void _buffer()
-void _postprocessTransparentMinibatch(address[] excludedAssets)
-void _buildOrders()
}
class EpochState {
+address[] tokens
+mapping(address => bool) tokenExists
+mapping(address => uint256) priceArray
+mapping(address => uint256) vaultsTotalAssets
+mapping(address => uint256) vaultsTotalAssetsForFulfillRedeem
+mapping(address => uint256) vaultsTotalAssetsForFulfillDeposit
+mapping(address => address[]) vaultPortfolioTokens
+mapping(address => uint256[]) vaultPortfolioShares
+mapping(address => uint256) initialBatchPortfolio
+mapping(address => uint256) finalBatchPortfolio
+mapping(address => uint256) sellingOrders
+mapping(address => uint256) buyingOrders
}
IInternalStateOrchestrator <|.. InternalStatesOrchestrator
InternalStatesOrchestrator o-- EpochState
class ILiquidityOrchestrator {
<<interface>>
+uint16 epochCounter()
+LiquidityUpkeepPhase currentPhase()
+uint256 targetBufferRatio()
+uint256 slippageTolerance()
+void updateMinibatchSize(uint8 minibatchSize)
+void setTargetBufferRatio(uint256 targetBufferRatio)
+void advanceIdlePhase()
+void transferRedemptionFunds(address to, uint256 amount)
}
class LiquidityOrchestrator {
+uint16 BASIS_POINTS_FACTOR
+uint16 epochCounter
+uint8 minibatchSize
+uint256 targetBufferRatio
+uint256 slippageTolerance
+void advanceIdlePhase()
+bool checkUpkeep(bytes checkData) returns (bool, bytes)
+void performUpkeep(bytes performData)
+void setTargetBufferRatio(uint256 targetBufferRatio)
-LiquidityUpkeepPhase currentPhase
-uint16 currentMinibatchIndex
-void _processSellLeg()
-void _processBuyLeg()
-void _processVaultOperations()
-void _processSingleVaultOperations(address vault, uint256 totalAssetsForDeposit, uint256 totalAssetsForRedeem, uint256 finalTotalAssets)
}
ILiquidityOrchestrator <|.. LiquidityOrchestrator
InternalStatesOrchestrator --> ILiquidityOrchestrator : liquidityOrchestrator
class IExecutionAdapter {
<<interface>>
+void validateExecutionAdapter(address asset)
+uint256 sell(address asset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount)
+uint256 buy(address asset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount)
}
class OrionAssetERC4626ExecutionAdapter {
+uint16 BASIS_POINTS_FACTOR
+IOrionConfig config
+IERC20 underlyingAssetToken
+ILiquidityOrchestrator liquidityOrchestrator
+constructor(address configAddress)
+void validateExecutionAdapter(address asset)
+uint256 sell(address vaultAsset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount)
+uint256 buy(address vaultAsset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount)
-void _validateExecutionAdapter(address asset)
}
class MockExecutionAdapter {
+uint256 buy(address asset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount)
+uint256 sell(address asset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount)
}
IExecutionAdapter <|.. OrionAssetERC4626ExecutionAdapter
IExecutionAdapter <|.. MockExecutionAdapter
OrionAssetERC4626ExecutionAdapter --> ILiquidityOrchestrator
class IOrionVault {
<<interface>>
+event Redeem(address vault, address user, uint256 redeemAmount, uint256 sharesBurned)
+event CuratorFeesAccrued(uint256 feeAmount, uint256 pendingCuratorFees)
+void updateCurator(address newCurator)
+void fulfillDeposit(uint256 depositTotalAssets)
+void fulfillRedeem(uint256 redeemTotalAssets)
+void accrueCuratorFees(uint256 feeAmount)
}
class OrionVault {
+IInternalStateOrchestrator internalStatesOrchestrator
+IOrionConfig config
+ILiquidityOrchestrator liquidityOrchestrator
+void cancelDeposit(uint256 amount)
+void cancelRedeem(uint256 shares)
+void fulfillDeposit(uint256 depositTotalAssets)
+void fulfillRedeem(uint256 redeemTotalAssets)
+void accrueCuratorFees(uint256 feeAmount)
-mapping(address => uint256) _depositRequests
-mapping(address => uint256) _redeemRequests
}
IOrionVault <|.. OrionVault
class IOrionTransparentVault {
<<interface>>
+void updateVaultState(address[] tokens, uint256[] shares, uint256 newTotalAssets)
}
class OrionTransparentVault {
-mapping(address => uint256) _portfolio
-uint256 _totalAssets
+void updateVaultState(address[] tokens, uint256[] shares, uint256 newTotalAssets)
+void updateCurator(address newCurator)
}
IOrionTransparentVault <|.. OrionTransparentVault
OrionTransparentVault --|> OrionVault
class OrionConfig {
+mapping(address => uint8) tokenDecimals
+void addWhitelistedAsset(address asset, address priceAdapter, address executionAdapter)
+bool isWhitelistedVaultOwner(address vaultOwner) returns bool
+bool isDecommissioningVault(address vault) returns bool
+void completeVaultDecommissioning(address vault)
}
class TransparentVaultFactory {
+void createTransparentVault(
address asset,
string name,
string symbol,
address curator,
address strategy,
uint8 feeType,
uint16 performanceFee,
uint16 managementFee,
address depositAccessControl
)
}
OrionConfig <.. TransparentVaultFactory
OrionConfig <.. InternalStatesOrchestrator
OrionConfig <.. LiquidityOrchestrator
OrionConfig <.. OrionAssetERC4626ExecutionAdapter
LiquidityOrchestrator --> OrionTransparentVault : process vault ops
LiquidityOrchestrator --> IExecutionAdapter : buy/sell
InternalStatesOrchestrator --> OrionTransparentVault : read intents
TransparentVaultFactory --> OrionTransparentVault : creates
class EventsLib {
<<library>>
+event EpochProcessed(uint16 epochCounter)
+event OrionVaultCreated(address vault, address asset, address vaultOwner, address curator, address strategy, uint8 feeType, uint16 performanceFee, uint16 managementFee, address depositAccessControl, VaultType vaultType)
}
class ErrorsLib {
<<library>>
+error SlippageExceeded(address asset, uint256 actual, uint256 expected)
}
class LiquidityUpkeepPhase {
<<enumeration>>
Idle
SellingLeg
BuyingLeg
ProcessVaultOperations
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughRemoved many generated artifact JSON files; removed curator-whitelist state and APIs; introduced slippage-tolerant buy/sell signatures and validation; replaced PortfolioPosition arrays with parallel tokens/shares arrays across orchestrators and vaults; renamed orchestrator phase and added epoch signaling; updated tests and bumped package version. Changes
Sequence Diagram(s)sequenceDiagram
participant LO as LiquidityOrchestrator
participant ISO as InternalStatesOrchestrator
participant VAULT as OrionTransparentVault / ERC4626 Vault
participant ADAPTER as ExecutionAdapter
participant CONFIG as OrionConfig
Note over LO,ISO: Epoch processing and per-vault portfolio flow
LO->>ISO: checkUpkeep()
ISO-->>LO: (upkeepNeeded, phase)
LO->>ISO: performUpkeep(phase) / preprocess
ISO-->>LO: vault list & store vaultPortfolioTokens/vaultPortfolioShares
loop ProcessVaultOperations per vault
LO->>ISO: getVaultTotalAssetsAll(vault)
ISO-->>LO: (forRedeem, forDeposit, total)
LO->>VAULT: updateVaultState(tokens[], shares[], finalTotalAssets)
VAULT-->>LO: ack
end
LO->>LO: epochCounter++
LO->>CONFIG: emit EpochProcessed(epochCounter)
sequenceDiagram
participant LO as LiquidityOrchestrator
participant ADAPTER as ExecutionAdapter
participant VAULT as ERC4626 Vault
participant CONFIG as OrionConfig
Note over LO,ADAPTER: Slippage-aware buy flow
LO->>CONFIG: slippageTolerance()
CONFIG-->>LO: tolerance
LO->>VAULT: approve(ADAPTER, approvalAmount) calculated with BASIS_POINTS_FACTOR + tolerance
LO->>ADAPTER: buy(vaultAsset, shares, estimatedUnderlying)
ADAPTER->>ADAPTER: _validateExecutionAdapter(vaultAsset)
ADAPTER->>VAULT: transferFrom(underlying, estimatedUnderlying)
ADAPTER->>VAULT: deposit(estimatedUnderlying, adapter)
VAULT-->>ADAPTER: mintedShares (may include dust)
ADAPTER-->>LO: actualUnderlyingUsed
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 |
🛡️ Immunefi PR ReviewsWe noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below: Once submitted, we'll take care of assigning a reviewer and follow up here. |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
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 (2)
contracts/OrionConfig.sol (1)
238-251: Silent duplicate handling may mask configuration errors.The function now silently skips adding duplicates to
whitelistedAssetsbut still:
- Overwrites
tokenDecimals[asset](line 244)- Re-registers adapters (lines 247-248)
- Emits
WhitelistedAssetAddedevent (line 250)This allows atomic adapter updates for existing assets, but the event emission for already-whitelisted assets could be misleading. Consider emitting a different event (e.g.,
WhitelistedAssetUpdated) when the asset was already present.function addWhitelistedAsset(address asset, address priceAdapter, address executionAdapter) external onlyOwner { if (!isSystemIdle()) revert ErrorsLib.SystemNotIdle(); + bool isNew = !this.isWhitelisted(asset); - if (!this.isWhitelisted(asset)) { + if (isNew) { // slither-disable-next-line unused-return whitelistedAssets.add(asset); } // Store token decimals tokenDecimals[asset] = IERC20Metadata(asset).decimals(); // Register the adapters IPriceAdapterRegistry(priceAdapterRegistry).setPriceAdapter(asset, IPriceAdapter(priceAdapter)); ILiquidityOrchestrator(liquidityOrchestrator).setExecutionAdapter(asset, IExecutionAdapter(executionAdapter)); - emit EventsLib.WhitelistedAssetAdded(asset); + if (isNew) { + emit EventsLib.WhitelistedAssetAdded(asset); + } else { + emit EventsLib.WhitelistedAssetUpdated(asset); + } }contracts/interfaces/IOrionTransparentVault.sol (1)
18-24: Remove unusedPortfolioPositionstruct.This struct is not referenced anywhere in the codebase and has been superseded by the parallel arrays approach in
updateVaultState. Remove it to reduce interface clutter.
🧹 Nitpick comments (10)
test/orchestrator/EpochSimulation.test.ts (4)
46-51: Box-Muller implementation is correct.The Gaussian random number generator using Box-Muller transform is mathematically sound. Note that
Math.random()is not seedable, so test results are non-deterministic. For reproducible tests, consider using a seeded PRNG.
192-205: Potential weight rounding issue with equal distribution.With
NUM_ASSETS = 100andintentFactor = 10^9,weightPerAsset = Math.floor(10^9 / 100) = 10000000. Total weight = 100 * 10000000 = 1000000000 = 10^9, so this case works perfectly.However, if
NUM_ASSETSdoesn't evenly divideintentFactor, the total weights won't sum tointentScale, potentially causingInvalidTotalWeightrevert. Consider adding remainder distribution logic similar toKBestTvlWeightedAverage._calculatePositions.
280-309: Consider using phase enum names instead of magic numbers.Hardcoded phase numbers (
1n,2n,3n,4n) are brittle. If the phase enum ordering changes inInternalStatesOrchestrator, this test will silently misbehave. Consider importing or referencing the enum values directly.Example approach:
// At the top of the file or in a helper const InternalPhase = { Idle: 0n, PreprocessingTransparentVaults: 1n, Buffering: 2n, PostprocessingTransparentVaults: 3n, BuildingOrders: 4n, }; // Then use: while ((await internalStatesOrchestrator.currentPhase()) === InternalPhase.PreprocessingTransparentVaults) { // ... }
351-365: Consider adding basic invariant assertions.The test only logs state without validating invariants. While this works as a smoke/stress test, adding assertions would catch regressions:
// Example invariants to check each epoch expect(sharePrice).to.be.gt(0, "Share price should be positive"); expect(totalAssets).to.be.gte(0, "Total assets should not be negative"); expect(pendingCuratorFees).to.be.gte(0, "Fees should not be negative");test/ExecutionAdapterValidation.test.ts (1)
159-172: Consider using a more specific error assertion.The assertion
.to.be.revertedis generic and may pass for unexpected failure reasons. Consider asserting the specific error or at least the contract that reverts.- ).to.be.reverted; + ).to.be.revertedWithoutReason(); // or revertedWithCustomError if specific error expectedcontracts/execution/OrionAssetERC4626ExecutionAdapter.sol (2)
91-99: Misleading variable name:maxUnderlyingAmountshould beminUnderlyingAmount.In the sell context, you're computing the minimum acceptable amount (applying negative slippage tolerance), yet the variable is named
maxUnderlyingAmount. This is semantically incorrect and confusing for maintainers.if (receivedUnderlyingAmount < estimatedUnderlyingAmount) { - uint256 maxUnderlyingAmount = estimatedUnderlyingAmount.mulDiv( + uint256 minUnderlyingAmount = estimatedUnderlyingAmount.mulDiv( BASIS_POINTS_FACTOR - liquidityOrchestrator.slippageTolerance(), BASIS_POINTS_FACTOR ); - if (receivedUnderlyingAmount < maxUnderlyingAmount) { + if (receivedUnderlyingAmount < minUnderlyingAmount) { revert ErrorsLib.SlippageExceeded(vaultAsset, receivedUnderlyingAmount, estimatedUnderlyingAmount); } }
136-137: Dust acceptance is reasonable but consider documenting threshold.The comment notes that some ERC4626 implementations may leave dust. Consider whether a maximum acceptable dust threshold should be enforced to prevent unexpected accumulation over many operations.
contracts/orchestrators/LiquidityOrchestrator.sol (1)
170-172: Slippage tolerance is coupled to buffer ratio.Setting
slippageTolerance = targetBufferRatio / 2creates an implicit relationship. While this may be intentional (higher buffer → can tolerate more slippage), consider whether these should be independently configurable for more fine-grained control.test/orchestrator/Orchestrators.test.ts (2)
718-722: Redundant duplicate calls togetVaultTotalAssetsAll.Two separate calls are made to retrieve the same tuple, destructuring different parts. This is inefficient and can be consolidated into a single call.
- const [, totalAssetsForDeposit] = await internalStatesOrchestrator.getVaultTotalAssetsAll(await v.getAddress()); - const [totalAssetsForRedeem, ,] = await internalStatesOrchestrator.getVaultTotalAssetsAll(await v.getAddress()); - expect(totalAssetsForDeposit).to.equal(0); - expect(totalAssetsForRedeem).to.equal(0); + const [totalAssetsForRedeem, totalAssetsForDeposit] = await internalStatesOrchestrator.getVaultTotalAssetsAll(await v.getAddress()); + expect(totalAssetsForRedeem).to.equal(0); + expect(totalAssetsForDeposit).to.equal(0);
762-795: Address TODO: Commented-out assertions should be implemented or removed.These commented-out expected total assets calculations represent incomplete test coverage. Either implement the assertions or remove the dead code with an issue tracking the work.
Would you like me to open an issue to track implementing these assertions, or should they be removed if no longer relevant?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (72)
.gitignore(2 hunks)artifacts/contracts/OrionConfig.sol/OrionConfig.json(0 hunks)artifacts/contracts/access_controllers/WhitelistAccessControl.sol/WhitelistAccessControl.json(0 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(0 hunks)artifacts/contracts/factories/TransparentVaultFactory.sol/TransparentVaultFactory.json(0 hunks)artifacts/contracts/interfaces/IExecutionAdapter.sol/IExecutionAdapter.json(0 hunks)artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json(0 hunks)artifacts/contracts/interfaces/ILiquidityOrchestrator.sol/ILiquidityOrchestrator.json(0 hunks)artifacts/contracts/interfaces/IOrionAccessControl.sol/IOrionAccessControl.json(0 hunks)artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json(0 hunks)artifacts/contracts/interfaces/IOrionStrategy.sol/IOrionStrategy.json(0 hunks)artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json(0 hunks)artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json(0 hunks)artifacts/contracts/interfaces/IPriceAdapter.sol/IPriceAdapter.json(0 hunks)artifacts/contracts/interfaces/IPriceAdapterRegistry.sol/IPriceAdapterRegistry.json(0 hunks)artifacts/contracts/libraries/ErrorsLib.sol/ErrorsLib.json(0 hunks)artifacts/contracts/libraries/EventsLib.sol/EventsLib.json(0 hunks)artifacts/contracts/libraries/UtilitiesLib.sol/UtilitiesLib.json(0 hunks)artifacts/contracts/mocks/MockERC4626Asset.sol/MockERC4626Asset.json(0 hunks)artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json(0 hunks)artifacts/contracts/mocks/MockPriceAdapter.sol/MockPriceAdapter.json(0 hunks)artifacts/contracts/mocks/MockUnderlyingAsset.sol/MockUnderlyingAsset.json(0 hunks)artifacts/contracts/orchestrators/InternalStatesOrchestrator.sol/InternalStatesOrchestrator.json(0 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(0 hunks)artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json(0 hunks)artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json(0 hunks)artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json(0 hunks)artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json(0 hunks)artifacts/contracts/test/UtilitiesLibTest.sol/UtilitiesLibTest.json(0 hunks)artifacts/contracts/vaults/OrionVault.sol/OrionVault.json(0 hunks)contracts/OrionConfig.sol(2 hunks)contracts/access_controllers/WhitelistAccessControl.sol(1 hunks)contracts/execution/OrionAssetERC4626ExecutionAdapter.sol(4 hunks)contracts/factories/TransparentVaultFactory.sol(1 hunks)contracts/interfaces/IExecutionAdapter.sol(1 hunks)contracts/interfaces/IInternalStateOrchestrator.sol(2 hunks)contracts/interfaces/ILiquidityOrchestrator.sol(3 hunks)contracts/interfaces/IOrionAccessControl.sol(1 hunks)contracts/interfaces/IOrionConfig.sol(0 hunks)contracts/interfaces/IOrionTransparentVault.sol(1 hunks)contracts/interfaces/IOrionVault.sol(4 hunks)contracts/libraries/ErrorsLib.sol(1 hunks)contracts/libraries/EventsLib.sol(3 hunks)contracts/mocks/MockExecutionAdapter.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(7 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(12 hunks)contracts/price/OrionAssetERC4626PriceAdapter.sol(2 hunks)contracts/strategies/KBestTvlWeightedAverage.sol(1 hunks)contracts/test/KBestTvlWeightedAverageInvalid.sol(1 hunks)contracts/vaults/OrionTransparentVault.sol(1 hunks)contracts/vaults/OrionVault.sol(6 hunks)package.json(1 hunks)test/AccessControl.test.ts(0 hunks)test/Adapters.test.ts(8 hunks)test/BatchLimitAccounting.test.ts(0 hunks)test/ExecutionAdapterValidation.test.ts(1 hunks)test/FeeCooldown.test.ts(0 hunks)test/MinimumAmountDOS.test.ts(0 hunks)test/OrionConfigVault.test.ts(0 hunks)test/OrionVaultExchangeRate.test.ts(16 hunks)test/PassiveCuratorStrategy.test.ts(0 hunks)test/ProtocolPause.test.ts(0 hunks)test/RedeemBeforeDepositOrder.test.ts(3 hunks)test/Removal.test.ts(0 hunks)test/TransparentVault.test.ts(0 hunks)test/VaultOwnerRemoval.test.ts(0 hunks)test/mainnet-fork/multiAssetRobustness.test.ts(1 hunks)test/orchestrator/EpochSimulation.test.ts(1 hunks)test/orchestrator/OrchestratorConfiguration.test.ts(0 hunks)test/orchestrator/OrchestratorSecurity.test.ts(2 hunks)test/orchestrator/Orchestrators.test.ts(10 hunks)test/orchestrator/OrchestratorsZeroState.test.ts(0 hunks)
💤 Files with no reviewable changes (42)
- test/VaultOwnerRemoval.test.ts
- test/BatchLimitAccounting.test.ts
- test/orchestrator/OrchestratorsZeroState.test.ts
- artifacts/contracts/OrionConfig.sol/OrionConfig.json
- artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json
- artifacts/contracts/test/UtilitiesLibTest.sol/UtilitiesLibTest.json
- artifacts/contracts/libraries/EventsLib.sol/EventsLib.json
- test/orchestrator/OrchestratorConfiguration.test.ts
- test/MinimumAmountDOS.test.ts
- artifacts/contracts/factories/TransparentVaultFactory.sol/TransparentVaultFactory.json
- artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json
- artifacts/contracts/interfaces/IPriceAdapterRegistry.sol/IPriceAdapterRegistry.json
- artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json
- artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json
- contracts/interfaces/IOrionConfig.sol
- test/Removal.test.ts
- test/ProtocolPause.test.ts
- artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json
- artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json
- test/AccessControl.test.ts
- artifacts/contracts/mocks/MockERC4626Asset.sol/MockERC4626Asset.json
- artifacts/contracts/orchestrators/InternalStatesOrchestrator.sol/InternalStatesOrchestrator.json
- artifacts/contracts/interfaces/IOrionAccessControl.sol/IOrionAccessControl.json
- artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json
- test/TransparentVault.test.ts
- test/FeeCooldown.test.ts
- artifacts/contracts/interfaces/IPriceAdapter.sol/IPriceAdapter.json
- artifacts/contracts/libraries/ErrorsLib.sol/ErrorsLib.json
- artifacts/contracts/libraries/UtilitiesLib.sol/UtilitiesLib.json
- artifacts/contracts/mocks/MockPriceAdapter.sol/MockPriceAdapter.json
- artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json
- test/OrionConfigVault.test.ts
- artifacts/contracts/interfaces/IExecutionAdapter.sol/IExecutionAdapter.json
- artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json
- artifacts/contracts/interfaces/ILiquidityOrchestrator.sol/ILiquidityOrchestrator.json
- artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json
- artifacts/contracts/mocks/MockUnderlyingAsset.sol/MockUnderlyingAsset.json
- artifacts/contracts/access_controllers/WhitelistAccessControl.sol/WhitelistAccessControl.json
- test/PassiveCuratorStrategy.test.ts
- artifacts/contracts/vaults/OrionVault.sol/OrionVault.json
- artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json
- artifacts/contracts/interfaces/IOrionStrategy.sol/IOrionStrategy.json
🧰 Additional context used
🧬 Code graph analysis (7)
test/orchestrator/OrchestratorSecurity.test.ts (1)
test/orchestrator/OrchestratorPerformUpkeep.test.ts (1)
it(748-2469)
test/RedeemBeforeDepositOrder.test.ts (1)
test/OrionConfigVault.test.ts (1)
describe(338-593)
test/ExecutionAdapterValidation.test.ts (1)
test/OrionConfigVault.test.ts (1)
describe(338-593)
contracts/test/KBestTvlWeightedAverageInvalid.sol (1)
test/PassiveCuratorStrategy.test.ts (2)
it(259-275)KBestTvlWeightedAverageInvalidFactory(509-568)
contracts/libraries/EventsLib.sol (1)
test/AccessControl.test.ts (1)
vault(270-280)
contracts/interfaces/IOrionVault.sol (1)
test/AccessControl.test.ts (2)
vault(270-280)vault(138-237)
contracts/strategies/KBestTvlWeightedAverage.sol (1)
test/PassiveCuratorStrategy.test.ts (1)
it(259-275)
⏰ 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). (3)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
- GitHub Check: Build, Lint and Test
🔇 Additional comments (58)
contracts/interfaces/IOrionAccessControl.sol (1)
8-8: Doc wording update is clear and accurateThe new dev comment better specifies compliance intent (KYC/AML/other requirements) without changing behavior. No further changes needed.
.gitignore (1)
25-25: Consolidation of artifact ignores aligns with PR cleanup goals.The two changes—adding
.cursor/(Cursor IDE cache) and consolidating artifact patterns into a blanketartifacts/ignore—are appropriate housekeeping. The shift from preservingartifacts/contracts/**/*.jsonto ignoring the entireartifacts/directory aligns with the PR's artifact cleanup and regeneration-on-build model.Please verify that the removal of the selective negation patterns (
!artifacts/contracts/**/*.json) doesn't inadvertently exclude files that should be version-controlled. Given the PR's scope around artifact cleanup, this appears intentional, but confirming that generated ABIs/artifacts are no longer expected to be committed would be helpful.Also applies to: 46-46
package.json (1)
4-4: Version bump aligns with architectural refactoring.The protocol version bump to 0.7.0 is appropriate given the significant architectural changes throughout this PR (orchestrator refactors, slippage handling, epoch tracking, vault state management).
contracts/access_controllers/WhitelistAccessControl.sol (4)
4-6: Import cleanup and base class usage look goodUsing
Ownable2Stepfor ownership and removing the unused error library keeps this access controller lean; no functional concerns here.
7-11: Contract-level NatSpec/author tag is clearThe added
@noticeand@authormetadata improve discoverability without altering behavior.
16-22: Event parameter documentation is now completeAdding
@param accountdescriptions for both whitelist events makes their semantics explicit and aligns with NatSpec best practices.
24-26: Constructor NatSpec matches the implementationThe constructor docs correctly describe
initialOwner_and the ownership pattern; no issues with the current wording or implementation.test/RedeemBeforeDepositOrder.test.ts (3)
252-253: Correct migration to newgetVaultTotalAssetsAllAPI.The destructuring properly extracts both
totalAssetsForRedeemandtotalAssetsForDepositfrom the tuple return value. The subsequent assertions correctly verify that redemptions use higher totalAssets (before deposit impact).
348-348: LGTM!Correct use of destructuring to extract only
totalAssetsForDepositwhile ignoring the first element.
383-384: LGTM!Edge case correctly verifies that without redemptions, both
totalAssetsForRedeemandtotalAssetsForDepositare equal.contracts/OrionConfig.sol (1)
397-398: LGTM!Using
this.isDecommissioningVault(vault)instead of direct set access improves consistency and maintainability, even with slight gas overhead. This is appropriate for an admin function called by the liquidity orchestrator.contracts/strategies/KBestTvlWeightedAverage.sol (1)
17-17: ERC165 inheritance removed as intended.The contract correctly removes ERC165 from the inheritance chain. The concern about failing tests related to
supportsInterfaceis not applicable—no such tests exist in the test suite, and no ERC165 references are present in any test files. The code change is clean with no test impact.Likely an incorrect or invalid review comment.
contracts/test/KBestTvlWeightedAverageInvalid.sol (1)
18-18: Contract signature change is correct; ERC165 tests do not exist and do not require updates.The contract correctly inherits only from
IOrionStrategyandOwnable. NosupportsInterfacetests exist for this contract or any strategy in the codebase. Strategy contracts do not inherit from ERC165.Likely an incorrect or invalid review comment.
contracts/interfaces/IInternalStateOrchestrator.sol (2)
83-90: Consolidated getter improves gas efficiency for callers.Replacing two separate getters (
getVaultTotalAssetsForFulfillRedeemandgetVaultTotalAssetsForFulfillDeposit) with a singlegetVaultTotalAssetsAllthat returns all three values reduces external call overhead when callers need multiple values.
101-106: New per-vault portfolio getter aligns with parallel arrays pattern.The
getVaultPortfoliofunction correctly returns parallel arrays (tokens,shares) consistent with the updatedupdateVaultStatesignature inIOrionTransparentVault. Documentation is clear.contracts/libraries/ErrorsLib.sol (1)
91-96: Well-structured slippage error with actionable parameters.The
SlippageExceedederror includesasset,actual, andexpectedvalues, enabling precise debugging and off-chain monitoring of slippage events. This follows the library's existing pattern for parameterized errors.test/mainnet-fork/multiAssetRobustness.test.ts (1)
402-411: Test updated for newdepositAccessControlparameter.Passing
ethers.ZeroAddressas the deposit access control parameter aligns with the factory interface change. This correctly configures the vault with no deposit restrictions for testing purposes.contracts/price/OrionAssetERC4626PriceAdapter.sol (2)
37-43: LGTM!Variable rename to
underlyingimproves readability in the local scope. The validation logic remains correct.
52-54: Helpful rounding behavior documentation.The comment clarifies the intentional use of floor rounding here versus ceiling rounding in
previewMintduring execution, with the buffer handling any rounding discrepancies. This aids future maintainers.contracts/factories/TransparentVaultFactory.sol (1)
64-75: LGTM!The
depositAccessControlparameter is correctly propagated through the vault constructor and included in theOrionVaultCreatedevent emission, aligning with the updated event signature inEventsLib.sol.test/orchestrator/OrchestratorSecurity.test.ts (2)
41-42: LGTM!Phase sequence documentation correctly updated to reflect the renamed
ProcessVaultOperationsphase, consistent with theILiquidityOrchestratorenum change.
56-58: LGTM!Cross-phase test documentation accurately reflects the new phase naming, maintaining consistency with the orchestrator interface changes.
contracts/mocks/MockExecutionAdapter.sol (1)
12-20: LGTM!The mock correctly implements the updated
IExecutionAdapterinterface with the new three-parameter signatures forbuy()andsell(). The unused third parameter is appropriate for a test mock that doesn't need to implement actual slippage logic.contracts/interfaces/ILiquidityOrchestrator.sol (3)
16-16: LGTM!The phase rename from
FulfillDepositAndRedeemtoProcessVaultOperationsaccurately reflects the broader scope of this phase, which now handles epoch accounting and completion events in addition to fulfill operations.
19-33: LGTM!The new
epochCounter()andslippageTolerance()getters are well-documented and provide the necessary state visibility for execution adapters and external consumers to implement slippage-aware operations.
48-52: LGTM!The dev comment clearly explains the slippage tolerance calculation (50% of targetBufferRatio) and the rationale behind it—ensuring all trades pass even with maximum price impact during full NAV rebalancing.
contracts/vaults/OrionTransparentVault.sol (1)
163-166: Verify removal of curator whitelist check is intentional.The
updateCuratorfunction no longer validates that the new curator is whitelisted. Per the PR objectives, this shifts responsibility to vault owners. However, this allows setting any address (includingaddress(0)) as curator.Consider whether a zero-address check should remain:
function updateCurator(address newCurator) external onlyVaultOwner { + if (newCurator == address(0)) revert ErrorsLib.ZeroAddress(); curator = newCurator; emit CuratorUpdated(newCurator); }contracts/libraries/EventsLib.sol (2)
106-108: LGTM!The event rename from
InternalStateProcessedtoEpochProcessedbetter reflects the epoch-based lifecycle signaling, aligning with the broader orchestration model changes.
125-138: LGTM!The
OrionVaultCreatedevent correctly includes the newdepositAccessControlparameter with proper documentation. The parameter placement beforeVaultTypemaintains logical grouping of vault configuration parameters.test/ExecutionAdapterValidation.test.ts (2)
31-113: LGTM!The test setup is comprehensive and correctly wires up the full deployment environment including underlying assets, ERC4626 vault seeding, config, price/execution adapters, and orchestrators. The seeding of the vault with initial deposits ensures
totalAssets > 0for validation tests.
452-544: LGTM!The integration tests provide excellent end-to-end coverage of the buy-sell cycle with validation and slippage checks. The tests properly verify share balances, underlying token returns, and slippage propagation across configuration changes.
contracts/interfaces/IExecutionAdapter.sol (1)
14-38: Interface updates for slippage-aware execution look good.The addition of
validateExecutionAdapter(address asset)and the newestimatedUnderlyingAmountparameter tobuy()andsell()aligns with the PR objectives for slippage tolerance handling. The NatSpec documentation is clear and accurate.test/Adapters.test.ts (2)
227-248: Proper test setup for the new execution adapter interface.The beforeEach block correctly:
- Seeds the vault with initial assets to enable validation
- Deploys and whitelists the mock price adapter
- Sets slippage tolerance via
setTargetBufferRatio(400)This ensures tests exercise the new slippage-aware execution paths.
397-403: Correct use ofpreviewRedeemfor sell operations.Using
previewRedeem(sharesAmount)to compute the expected underlying amount before callingsell()follows ERC4626 best practices and ensures the slippage check in the adapter has accurate data.test/OrionVaultExchangeRate.test.ts (2)
129-144: Tests correctly updated for newupdateVaultStatesignature.The tests now use
updateVaultState([], [], depositAmount)via the liquidity orchestrator, aligning with the refactored vault state update pipeline where the liquidity orchestrator applies portfolio/total-assets updates.
81-97: Fixture structure is appropriate.The fixture returns both orchestrator references, providing flexibility for tests while the actual state update calls correctly use the liquidity orchestrator path. This is a reasonable approach.
contracts/interfaces/IOrionVault.sol (3)
54-63: Simplified event signatures by removing epoch parameter.The
RedeemandCuratorFeesAccruedevents no longer include epoch information, aligning with the PR's goal to decouple epoch accounting from vault operations. This simplification is appropriate.
139-143: Good documentation enhancement for curator responsibility.The added clarification that "Curator can be a smart contract or an address. It is the FULL responsibility of the vault owner to ensure the curator is capable of performing its duties" is an important security reminder given the removal of curator whitelisting.
202-204:accrueCuratorFeessignature correctly simplified.Removing the epoch parameter aligns with the updated event signature and the decoupled epoch model.
contracts/vaults/OrionVault.sol (5)
375-386: Good dust prevention in deposit cancellation.Enforcing
minDepositon the remaining amount after partial cancellation prevents dust-level deposits that would be inefficient to process. The logic correctly handles the zero case (full cancellation) separately.
431-436: Consistent dust prevention for redeem cancellation.Mirrors the deposit cancellation pattern by enforcing
minRedeemon remaining shares. This prevents dust redemption requests.
637-642: SimplifiedaccrueCuratorFeesimplementation.The epoch parameter removal is consistent with the interface change. The function correctly handles zero fee amounts with an early return.
657-682: Batch processing refactored to pre-collect request data.Pre-collecting users and amounts into arrays before iterating and removing entries avoids the swap-and-pop reordering issues inherent to
EnumerableMap.remove()during iteration. This is a correct and important fix.The memory allocation is bounded by
config.maxFulfillBatchSize(), ensuring gas costs remain predictable.
697-726: Redemption fulfillment also correctly pre-collects request data.The same pre-collection pattern is applied consistently to
fulfillRedeem, avoiding iteration issues withEnumerableMap. The comment on line 697 clearly documents the rationale.contracts/execution/OrionAssetERC4626ExecutionAdapter.sol (2)
52-73: LGTM: Validation logic is well-structured.The
_validateExecutionAdapterfunction properly checks both the underlying asset compatibility and decimal consistency with the config. The fallback reverts on any failure are appropriate.
117-125: Slippage check for buy is correctly implemented.The pre-execution slippage check using
previewMintbefore pulling funds is the correct approach, preventing overspending within tolerance bounds.contracts/orchestrators/LiquidityOrchestrator.sol (4)
456-465: Potential logic issue: comparing shares to finalTotalAssets for decommissioning check.The condition
shares[i] == finalTotalAssetscompares the share amount of the underlying asset in the portfolio tofinalTotalAssets. However, for the underlying asset (which is not an ERC4626 vault), the "shares" are actually the underlying asset amount itself (1:1), so this comparison should be valid.However, this logic assumes the portfolio only contains the underlying asset when decommissioning is ready. If there are any residual non-underlying tokens with shares > 0, the vault won't be decommissioned even if the underlying portion equals finalTotalAssets.
Please verify that the decommissioning flow ensures all non-underlying assets are fully sold before this check runs. Consider adding a comment clarifying this expectation, or add an explicit check that verifies only the underlying asset remains in the portfolio.
132-132: Initial slippageTolerance of 0 may cause issues beforesetTargetBufferRatiois called.With
slippageTolerance = 0, any price movement between estimation and execution will cause slippage reverts. The owner must callsetTargetBufferRatiobefore the first epoch can successfully execute trades.Verify that the deployment/initialization sequence ensures
setTargetBufferRatiois called before any epoch processing. Consider documenting this requirement or setting a sensible default.
385-389: Approval includes slippage buffer for buy operations.Good approach - approving
estimatedUnderlyingAmount * (1 + slippageTolerance/BASIS_POINTS_FACTOR)ensures the adapter has sufficient allowance even with adverse price movement, while the adapter's internal slippage check prevents overspending.
409-415: Epoch counter increment and event emission are correctly placed.The
EpochProcessedevent is emitted andepochCounterincremented only after all vault operations in the final minibatch complete, ensuring consistent epoch signaling.test/orchestrator/Orchestrators.test.ts (2)
1187-1191: Good pattern: Using getVaultTotalAssetsAll to validate vault state.The test correctly validates that
totalAssets()from the vault matches the expected value from the internal state orchestrator after LiquidityOrchestrator completes.
1788-1793: Consistent use of consolidated getter pattern.The test properly destructures the tuple from
getVaultTotalAssetsAllto access both redeem and deposit totals in a single call.contracts/orchestrators/InternalStatesOrchestrator.sol (6)
84-109: Well-structured epoch state with proper token tracking.The addition of
tokensarray andtokenExistsmapping provides efficient O(1) duplicate checking while maintaining iteration order. The parallel arrays for vault portfolios (vaultPortfolioTokensandvaultPortfolioShares) are a reasonable pattern for Solidity.
310-348: Comprehensive epoch state cleanup in_handleStart.The cleanup logic properly handles:
- Token-specific mappings (price, portfolio, orders)
- Vault-specific mappings (total assets, portfolio tokens/shares)
- The tokens array itself
This prevents stale data from affecting subsequent epochs.
676-684: Clean consolidation of asset getters into single function.
getVaultTotalAssetsAllreturns all three values in one call, reducing gas costs for callers that need multiple values. This is a good refactoring pattern.
692-695: NewgetVaultPortfolioprovides clean access to per-vault portfolio data.This getter enables the LiquidityOrchestrator to retrieve computed portfolio data for each vault during
_processSingleVaultOperations.
527-528: Portfolio population uses push to dynamic arrays in mapping.Each call to
_postprocessTransparentMinibatchpushes tovaultPortfolioTokens[vaultAddress]andvaultPortfolioShares[vaultAddress]. Since these are cleared in_handleStart, this is safe. However, ensure that postprocessing is only called once per vault per epoch to avoid duplicate entries.The minibatch logic should guarantee each vault is processed exactly once. Verify this assumption holds with the minibatch index progression.
419-419: Interface and implementation signatures are already aligned.The
accrueCuratorFeesmethod is consistently defined with a singleuint256 feeAmountparameter acrossIOrionVault(interface),OrionVault(implementation), and all call sites. No action required.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
contracts/interfaces/IOrionTransparentVault.sol (1)
37-40: Signature and docs look good; consider minor naming alignment.The updated
updateVaultStatesignature and NatSpec fortokens/sharesandnewTotalAssetsare consistent and match the new portfolio handling model. As a small clarity tweak, consider aligning thesharesparameter name with thesharesPerAssetterminology used ingetPortfolio(either rename the param or adjust one of the docs) to keep the API surface fully self-consistent.test/orchestrator/EpochSimulation.test.ts (5)
45-51: Consider using a seeded random number generator for test determinism.The current implementation uses
Math.random(), which produces non-deterministic results. This makes test failures difficult to reproduce and debug, especially in a 10-epoch simulation with 100 assets (1000 random values).Consider using a seeded PRNG library like
seedrandomfor reproducible test runs:import seedrandom from 'seedrandom'; function generateGaussian(mean: number, stdDev: number, rng: () => number): number { const u1 = rng(); const u2 = rng(); const z0 = Math.sqrt(-2.0 * Math.log(u1)) * Math.cos(2.0 * Math.PI * u2); return z0 * stdDev + mean; } // In beforeEach: const rng = seedrandom('test-seed'); // In test: const gainFactor = generateGaussian(GAIN_MEAN, GAIN_STD_DEV, rng);
272-334: Consider extracting hardcoded phase constants.The phase numbers (1n, 2n, 3n, 4n) are hardcoded throughout the test. If phase enumeration changes in the contracts, this test will break silently or behave unexpectedly.
Consider defining phase constants at the top of the test or querying them from the contracts if exposed:
// At the top of the test suite const PHASE_IDLE = 0n; const PHASE_PREPROCESSING = 1n; const PHASE_BUFFERING = 2n; const PHASE_POSTPROCESSING = 3n; const PHASE_BUILDING_ORDERS = 4n; const LIQUIDITY_PHASE_SELLING = 1n; const LIQUIDITY_PHASE_BUYING = 2n; const LIQUIDITY_PHASE_VAULT_OPERATIONS = 3n; // Then use in conditions: while ((await internalStatesOrchestrator.currentPhase()) === PHASE_PREPROCESSING) { // ... }This makes the test more maintainable and self-documenting.
235-248: Extract duplicate idle-wait logic into a helper function.The idle-wait pattern is duplicated at lines 235-248 and 337-349, increasing maintenance burden and test length.
Extract the pattern into a reusable helper:
async function waitForSystemIdle() { while (!(await orionConfig.isSystemIdle())) { let [upkeepNeeded, performData] = await internalStatesOrchestrator.checkUpkeep("0x"); if (upkeepNeeded) { await internalStatesOrchestrator.connect(automationRegistry).performUpkeep(performData); continue; } [upkeepNeeded, performData] = await liquidityOrchestrator.checkUpkeep("0x"); if (upkeepNeeded) { await liquidityOrchestrator.connect(automationRegistry).performUpkeep(performData); continue; } } } // Usage: await waitForSystemIdle();Also applies to: 337-349
18-21: Consider test performance and organization.This test deploys 100 mock assets and simulates 10 epochs with extensive phase processing. While comprehensive, it may be slow to execute (potentially several minutes), which could impact CI/CD pipeline performance.
Consider one of the following approaches:
Reduce asset count for faster feedback:
const NUM_ASSETS = 10; // Reduced for faster test executionMark as integration test with longer timeout:
it("should simulate 10 epochs with Gaussian-distributed gains/losses", async function () { this.timeout(300000); // 5 minutes // ... });Split into multiple focused tests:
- One test for basic epoch progression with few assets
- Separate stress test for 100-asset scenario (run less frequently)
This maintains test value while improving developer experience during local testing.
Also applies to: 53-212
192-205: Distributing weight remainder improves robustness for varying asset counts.The vault enforces strict weight sum validation:
if (totalWeight != 10 ** config.curatorIntentDecimals()) revert InvalidTotalWeight();. WithNUM_ASSETS = 100andcuratorIntentDecimals = 9, the current calculation works correctly since 10^9 divides evenly by 100. However, if asset count changes to any non-divisor of 10^9, the test will fail.Apply this refactor to handle remainders:
const intentFactor = 10 ** curatorIntentDecimals; const weightPerAsset = Math.floor(intentFactor / NUM_ASSETS); + const remainder = intentFactor - (weightPerAsset * NUM_ASSETS); const intent = []; for (let i = 0; i < NUM_ASSETS; i++) { intent.push({ token: await mockAssets[i].getAddress(), - weight: weightPerAsset, + weight: weightPerAsset + (i < remainder ? 1 : 0), }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/interfaces/IOrionTransparentVault.sol(1 hunks)test/orchestrator/EpochSimulation.test.ts(1 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
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
Summary by Sourcery
Refactor orchestrators, vaults, and adapters to introduce slippage‑aware execution and explicit vault portfolio/state handling, simplify curator and access control configuration, and align tests and configuration with the new epoch and orchestration model.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.