Upgradability - #118
Conversation
…egistry - Converted OrionConfig to OrionConfigUpgradeable using UUPS pattern - Converted PriceAdapterRegistry to PriceAdapterRegistryUpgradeable - Changed immutable ADMIN to regular storage variable - Replaced constructors with initialize() functions - Added _authorizeUpgrade() for owner-controlled upgrades - Added 50-slot storage gaps for future upgrades - Used standard EnumerableSet (no upgradeable version in OZ v5) and Added @openzeppelin/contracts-upgradeable@^5.4.0 and @openzeppelin/hardhat-upgrades for UUPS and Beacon proxy patterns
- Converted InternalStatesOrchestrator to upgradeable version - Converted LiquidityOrchestrator to upgradeable version - Changed all base contracts to upgradeable versions: - Ownable2Step → Ownable2StepUpgradeable - ReentrancyGuard → ReentrancyGuardUpgradeable - Pausable → PausableUpgradeable - Added UUPS pattern with _authorizeUpgrade() - Replaced constructors with initialize() functions - Added 50-slot storage gaps
- Created OrionVaultUpgradeable abstract base contract - Created OrionTransparentVaultUpgradeable concrete implementation - Converted from ERC4626 to ERC4626Upgradeable - Changed ReentrancyGuard to ReentrancyGuardUpgradeable - Replaced constructors with initialize() functions - Used __OrionVault_init() for internal initialization - NO UUPS logic - upgrades handled by Beacon pattern - Added 50-slot storage gaps This enables all vault instances to share a single upgradeable implementation via UpgradeableBeacon, allowing protocol-wide vault upgrades with a single transaction.
- Converted TransparentVaultFactory to UUPS upgradeable pattern - Added vaultBeacon state variable for UpgradeableBeacon reference - Modified createVault() to deploy BeaconProxy instances instead of direct contracts - Encodes initialization data and passes to BeaconProxy constructor - Added setVaultBeacon() function for updating beacon address - Storage gap: 49 slots (50 - 1 for vaultBeacon) Factory now deploys all vaults as BeaconProxy instances pointing to a shared UpgradeableBeacon, enabling single-transaction upgrades for all vaults.
Demonstrates complete upgrade lifecycle: - Deploy all UUPS contracts (Config, Registry, Orchestrators, Factory) - Deploy Beacon + vault implementation - Create vault via factory (BeaconProxy) - Test V1 behavior - Upgrade vault implementation via Beacon - Verify state preservation after vault upgrade - Upgrade OrionConfig via UUPS - Verify state preservation after UUPS upgrade
Mock V2 implementations: - OrionConfigUpgradeableV2: adds newV2Variable state and setV2Variable() - OrionTransparentVaultUpgradeableV2: adds vaultDescription and setVaultDescription() - Both include version() function returning "v2" Test scripts: 1. testUpgradeability.ts - Full upgrade lifecycle test - Deploy all UUPS contracts (Config, Registry, Orchestrators, Factory) - Deploy Beacon + vault implementation - Create vault via factory (BeaconProxy) - Upgrade vault via beacon (V1 → V2) - Verify implementation address changes - Test V2 new features work - Upgrade OrionConfig via UUPS (V1 → V2) - Verify state preservation and V2 features 2. verifyUpgradeRequiresNewAddress.ts - Verification test - Proves implementation address MUST change for real upgrades - Tests both UUPS and Beacon patterns - Demonstrates same-address "upgrades" are no-ops
now uses a Deploy complete upgradeable protocol infrastructure
What the Tests Cover: pendingDeposit Batch Limit (3 tests) Returns exact amount when requests < maxFulfillBatchSize Returns ONLY first maxFulfillBatchSize when requests exceed limit Handles edge case when requests = maxFulfillBatchSize exactly pendingRedeem Batch Limit (3 tests) Returns exact shares when requests < maxFulfillBatchSize Returns ONLY first maxFulfillBatchSize when requests exceed limit Handles edge case when requests = maxFulfillBatchSize exactly Verify Fix Prevents Double-Counting (2 tests) Verifies pendingDeposit limiting prevents totalAssets overcounting Verifies pendingRedeem limiting prevents double-subtraction from totalAssets
This commit addresses 4 critical code review action items:
1. **cancelRedeemRequest Edge-Case Tests** (OrionConfigVault.test.ts)
- Add test for zero amount (should revert with AmountMustBeGreaterThanZero)
- Add test for excessive amount (should revert with InsufficientAmount)
- Add test for successful full cancellation
- Add test for partial cancellation
- Mirrors existing cancelDepositRequest test pattern
- Uses LiquidityOrchestrator impersonation to mint shares for testing
2. **USDC Decimals On-Chain Fetch** (erc4626VaultCompatibility.test.ts)
- Replace hardcoded USDC_DECIMALS = 6 with on-chain contract call
- Prevents configuration drift detection issues
- Fetches decimals via USDC contract interface in before() hook
3. **Real Immutability Test** (erc4626VaultCompatibility.test.ts)
- Replace basic "call 3 times" test with comprehensive checks:
* Bytecode immutability verification across blocks
* EIP-1967 implementation slot immutability (proxy detection)
* Cross-block property validation for asset() and decimals()
* Deterministic read verification within same block
- Detects upgradeable contracts and validates implementation slots
4. **Adapter Decimals Assertion** (erc4626VaultCompatibility.test.ts)
- Add assertion that adapter decimals match underlying asset decimals
- Critical for ensuring consistent price calculations
- Prevents potential price calculation bugs
**Bonus Fix:**
- Execution adapter compatibility test now whitelists vaults before validation
- Ensures OrionConfig has vault decimals registered before adapter validates
- Prevents InvalidAdapter errors during validation
🛡️ 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. |
Reviewer's GuideIntroduces a fully upgradeable version of the Orion protocol (config, orchestrators, vaults, price adapter registry, and factory) using UUPS + Beacon patterns, centralizes deployment/ wiring in a shared helper, and extends the test suite to cover upgradeability behaviors and new redeem-request cancellation semantics while updating all existing tests to target the new upgradeable contracts. Sequence diagram for upgrade flow of config and transparent vaultssequenceDiagram
actor Owner
participant Admin
participant OrionConfigProxy
participant ConfigImplV1 as OrionConfigImplV1
participant ConfigImplV2 as OrionConfigImplV2
participant PriceRegistryProxy
participant ISOProxy as InternalStatesOrchestratorProxy
participant LOProxy as LiquidityOrchestratorProxy
participant FactoryProxy as TransparentVaultFactoryProxy
participant VaultBeacon
participant VaultImplV1 as OrionTransparentVaultImplV1
participant VaultImplV2 as OrionTransparentVaultImplV2
participant VaultProxy as OrionTransparentVaultBeaconProxy
Note over Owner,Admin: Initial deployment of upgradeable core contracts
Owner->>OrionConfigProxy: deployProxy(OrionConfigUpgradeable, initialize(owner, admin, underlying))
OrionConfigProxy-->>ConfigImplV1: delegatecall initialize
Owner->>PriceRegistryProxy: deployProxy(PriceAdapterRegistryUpgradeable, initialize(owner, config))
OrionConfigProxy->>PriceRegistryProxy: setPriceAdapterRegistry(address)
Owner->>ISOProxy: deployProxy(InternalStatesOrchestratorUpgradeable, initialize(owner, config, automationRegistry))
Owner->>LOProxy: deployProxy(LiquidityOrchestratorUpgradeable, initialize(owner, config, automationRegistry))
OrionConfigProxy->>ISOProxy: setInternalStatesOrchestrator(address)
OrionConfigProxy->>LOProxy: setLiquidityOrchestrator(address)
Note over Owner,VaultBeacon: Deploy vault implementation and beacon
Owner->>VaultImplV1: deploy OrionTransparentVaultUpgradeable
Owner->>VaultBeacon: deploy UpgradeableBeacon(VaultImplV1, owner)
Owner->>FactoryProxy: deployProxy(TransparentVaultFactoryUpgradeable, initialize(owner, config, VaultBeacon))
OrionConfigProxy->>FactoryProxy: setVaultFactory(address)
Note over Owner,VaultProxy: Create a transparent vault via factory
Owner->>FactoryProxy: createVault(curator, name, symbol, fees, accessControl)
FactoryProxy->>VaultBeacon: new BeaconProxy(beacon, initData)
VaultBeacon-->>VaultProxy: deploy proxy pointing to VaultImplV1
VaultProxy-->>VaultImplV1: delegatecall initialize(vaultOwner, curator, config,...)
OrionConfigProxy->>OrionConfigProxy: addOrionVault(vault, Transparent)
Note over Owner,VaultBeacon: Upgrade vault implementation via beacon
Owner->>VaultImplV2: deploy OrionTransparentVaultUpgradeableV2
Owner->>VaultBeacon: upgradeTo(VaultImplV2)
VaultBeacon-->>VaultImplV2: set new implementation
VaultProxy-->>VaultImplV2: future calls delegate to V2
Note over Owner,OrionConfigProxy: Upgrade OrionConfig via UUPS
Owner->>ConfigImplV2: deploy OrionConfigUpgradeableV2
Owner->>OrionConfigProxy: upgradeTo(address ConfigImplV2)
OrionConfigProxy-->>ConfigImplV2: future calls delegate to V2
OrionConfigProxy-->>ConfigImplV2: preserve storage (ADMIN, underlying, vault sets)
Note over Owner,VaultProxy: After upgrades, vaults and orchestrators
Note over Owner,VaultProxy: continue operating with same proxy addresses but new logic
Sequence diagram for epoch processing across internal and liquidity orchestratorssequenceDiagram
actor Automation as ChainlinkAutomation
participant ISO as InternalStatesOrchestratorProxy
participant LO as LiquidityOrchestratorProxy
participant Config as OrionConfigProxy
participant Vault as OrionTransparentVaultBeaconProxy
Note over Automation,ISO: Epoch start when system is idle and time passed
Automation->>ISO: checkUpkeep()
ISO-->>Automation: upkeepNeeded = true (Idle && time > nextUpdateTime)
Automation->>ISO: performUpkeep(processLP, excludedAssets)
ISO->>ISO: _handleStart()
ISO->>Config: getAllOrionVaults(Transparent)
ISO->>ISO: _buildTransparentVaultsEpoch()
ISO->>ISO: currentPhase = PreprocessingTransparentVaults
loop Preprocess transparent minibatches
Automation->>ISO: performUpkeep(...)
ISO->>Vault: getPortfolio()
ISO->>Vault: curatorFee(totalAssets)
ISO->>Vault: accrueCuratorFees(curatorFee)
ISO->>Vault: pendingRedeem(maxFulfillBatchSize)
ISO->>Vault: pendingDeposit(maxFulfillBatchSize)
ISO->>ISO: update vaultsTotalAssets mappings
ISO->>ISO: if last minibatch set currentPhase = Buffering
end
Note over ISO: Buffering step adjusts protocol buffer
Automation->>ISO: performUpkeep(...)
ISO->>ISO: _buffer()
ISO->>LO: targetBufferRatio()
ISO->>ISO: bufferAmount updated
ISO->>ISO: currentPhase = PostprocessingTransparentVaults
loop Postprocess transparent minibatches
Automation->>ISO: performUpkeep(excludedAssets)
ISO->>Vault: getIntent()
ISO->>ISO: build finalBatchPortfolio per vault
ISO->>ISO: if last minibatch set currentPhase = BuildingOrders
end
Note over ISO: Build aggregated buy/sell orders
Automation->>ISO: performUpkeep(...)
ISO->>ISO: _buildOrders()
ISO->>ISO: currentPhase = Idle
ISO->>LO: advanceIdlePhase()
LO->>LO: currentPhase = SellingLeg
Note over Automation,LO: LiquidityOrchestrator executes orders and vault operations
loop Sell leg
Automation->>LO: performUpkeep()
LO->>ISO: getOrders(true)
LO->>LO: _processSellLeg()
LO->>ExecutionAdapter: sell(token, amount, estimatedUnderlying)
LO->>LO: update deltaBufferAmount
LO->>LO: currentPhase = BuyingLeg
end
loop Buy leg
Automation->>LO: performUpkeep()
LO->>ISO: getOrders(false)
LO->>ExecutionAdapter: buy(token, amount, estimatedUnderlying)
LO->>LO: update deltaBufferAmount
LO->>ISO: updateBufferAmount(deltaBufferAmount)
LO->>LO: deltaBufferAmount = 0
LO->>LO: currentPhase = ProcessVaultOperations
end
loop Process vault operations minibatches
Automation->>LO: performUpkeep()
LO->>Config: getAllOrionVaults(Transparent)
LO->>ISO: getVaultTotalAssetsAll(vault)
alt processLP == true
LO->>Vault: pendingRedeem(maxFulfillBatchSize)
LO->>Vault: fulfillRedeem(totalAssetsForRedeem)
LO->>Vault: pendingDeposit(maxFulfillBatchSize)
LO->>Vault: fulfillDeposit(totalAssetsForDeposit)
end
LO->>ISO: getVaultPortfolio(vault)
LO->>Vault: updateVaultState(tokens, shares, finalTotalAssets)
LO->>Config: completeVaultDecommissioning(vault) (if fully in underlying)
LO->>LO: if last minibatch currentPhase = Idle, increment epochCounter
end
LO->>ISO: updateNextUpdateTime()
ISO->>ISO: _nextUpdateTime = block.timestamp + epochDuration
Class diagram for upgradeable core Orion contractsclassDiagram
direction LR
%% OpenZeppelin upgradeable bases
class Initializable
class UUPSUpgradeable
class Ownable2StepUpgradeable
class OwnableUpgradeable
class ReentrancyGuardUpgradeable
class PausableUpgradeable
class ERC20Upgradeable
class ERC4626Upgradeable
%% Interfaces
class IOrionConfig {
+address underlyingAsset()
+address internalStatesOrchestrator()
+address liquidityOrchestrator()
+address transparentVaultFactory()
+address priceAdapterRegistry()
+uint8 curatorIntentDecimals
+uint8 priceAdapterDecimals()
+uint16 riskFreeRate()
+uint256 minDepositAmount()
+uint256 minRedeemAmount()
+uint256 feeChangeCooldownDuration()
+uint256 maxFulfillBatchSize()
+bool isWhitelisted(address)
+bool isWhitelistedVaultOwner(address)
+bool isOrionVault(address)
+bool isDecommissioningVault(address)
+bool isDecommissionedVault(address)
+bool isSystemIdle()
+uint8 getTokenDecimals(address)
+address admin()
+addOrionVault(address, VaultType)
+completeVaultDecommissioning(address)
}
class IInternalStateOrchestrator {
<<interface>>
+enum InternalUpkeepPhase
+uint32 epochDuration()
+InternalUpkeepPhase currentPhase()
+uint256 bufferAmount()
+bool processLP()
+updateNextUpdateTime()
+getOrders(bool)
+getEpochTokens()
+getPriceOf(address)
+subtractPendingProtocolFees(uint256)
+updateBufferAmount(int256)
+getVaultTotalAssetsAll(address)
+getTransparentVaultsEpoch()
+getVaultPortfolio(address)
+pause()
+unpause()
}
class ILiquidityOrchestrator {
<<interface>>
+enum LiquidityUpkeepPhase
+LiquidityUpkeepPhase currentPhase()
+uint8 minibatchSize()
+uint256 targetBufferRatio()
+updateMinibatchSize(uint8)
+updateAutomationRegistry(address)
+setInternalStatesOrchestrator(address)
+setTargetBufferRatio(uint256)
+depositLiquidity(uint256)
+withdrawLiquidity(uint256)
+claimProtocolFees(uint256)
+setExecutionAdapter(address, IExecutionAdapter)
+advanceIdlePhase()
+returnDepositFunds(address, uint256)
+transferCuratorFees(uint256)
+transferRedemptionFunds(address, uint256)
+withdraw(uint256, address)
+pause()
+unpause()
}
class IOrionVault {
<<interface>>
+address vaultOwner()
+address curator()
+overrideIntentForDecommissioning()
+requestDeposit(uint256)
+cancelDepositRequest(uint256)
+requestRedeem(uint256)
+cancelRedeemRequest(uint256)
+vaultWhitelist() address[]
+curatorFee(uint256) uint256
+claimCuratorFees(uint256)
+setDepositAccessControl(address)
+pendingDeposit(uint256) uint256
+pendingRedeem(uint256) uint256
+accrueCuratorFees(uint256)
+fulfillDeposit(uint256)
+fulfillRedeem(uint256)
}
class IOrionTransparentVault {
<<interface>>
+struct IntentPosition
+submitIntent(IntentPosition[])
+getPortfolio() address[] uint256[]
+getIntent() address[] uint32[]
+updateVaultState(address[], uint256[], uint256)
+removeFromVaultWhitelist(address)
}
class IPriceAdapterRegistry {
<<interface>>
+setPriceAdapter(address, IPriceAdapter)
+getPrice(address) uint256
}
class IPriceAdapter {
<<interface>>
+validatePriceAdapter(address)
+getPriceData(address) uint256 uint8
}
class IExecutionAdapter {
<<interface>>
+validateExecutionAdapter(address)
+sell(address, uint256, uint256) uint256
+buy(address, uint256, uint256) uint256
}
%% Concrete upgradeable config
class OrionConfigUpgradeable {
+address ADMIN
+address guardian
+IERC20 underlyingAsset
+address internalStatesOrchestrator
+address liquidityOrchestrator
+address transparentVaultFactory
+address priceAdapterRegistry
+uint8 curatorIntentDecimals
+uint8 priceAdapterDecimals
+uint16 riskFreeRate
+uint256 minDepositAmount
+uint256 minRedeemAmount
+uint256 feeChangeCooldownDuration
+uint256 maxFulfillBatchSize
+addWhitelistedAsset(address, address, address)
+removeWhitelistedAsset(address)
+addWhitelistedVaultOwner(address)
+removeWhitelistedVaultOwner(address)
+setInternalStatesOrchestrator(address)
+setLiquidityOrchestrator(address)
+setVaultFactory(address)
+setPriceAdapterRegistry(address)
+setProtocolRiskFreeRate(uint16)
+setMinDepositAmount(uint256)
+setMinRedeemAmount(uint256)
+setFeeChangeCooldownDuration(uint256)
+setMaxFulfillBatchSize(uint256)
+setGuardian(address)
+pauseAll()
+unpauseAll()
}
OrionConfigUpgradeable ..|> IOrionConfig
OrionConfigUpgradeable --|> Initializable
OrionConfigUpgradeable --|> Ownable2StepUpgradeable
OrionConfigUpgradeable --|> UUPSUpgradeable
%% Price adapter registry
class PriceAdapterRegistryUpgradeable {
+address configAddress
+uint8 priceAdapterDecimals
+mapping adapterOf
+setPriceAdapter(address, IPriceAdapter)
+getPrice(address) uint256
}
PriceAdapterRegistryUpgradeable ..|> IPriceAdapterRegistry
PriceAdapterRegistryUpgradeable --|> Initializable
PriceAdapterRegistryUpgradeable --|> Ownable2StepUpgradeable
PriceAdapterRegistryUpgradeable --|> UUPSUpgradeable
%% Internal state orchestrator
class InternalStatesOrchestratorUpgradeable {
+address automationRegistry
+IOrionConfig config
+ILiquidityOrchestrator liquidityOrchestrator
+IPriceAdapterRegistry registry
+uint256 priceAdapterPrecision
+uint256 intentFactor
+address underlyingAsset
+uint8 underlyingDecimals
+uint16 vFeeCoefficient
+uint16 rsFeeCoefficient
+uint16 oldVFeeCoefficient
+uint16 oldRsFeeCoefficient
+uint256 newProtocolFeeRatesTimestamp
+uint256 pendingProtocolFees
+uint32 epochDuration
+uint8 transparentMinibatchSize
+InternalUpkeepPhase currentPhase
+uint8 currentMinibatchIndex
+address[] transparentVaultsEpoch
+uint256 bufferAmount
+bool processLP
+updateAutomationRegistry(address)
+updateEpochDuration(uint32)
+updateMinibatchSize(uint8)
+updateProtocolFees(uint16, uint16)
+activeProtocolFees() uint16 uint16
+resetPhase(InternalUpkeepPhase)
+checkUpkeep(bytes) bool bytes
+performUpkeep(bytes)
+updateNextUpdateTime()
+getOrders(bool)
+getEpochTokens() address[]
+getPriceOf(address) uint256
+subtractPendingProtocolFees(uint256)
+updateBufferAmount(int256)
+getVaultTotalAssetsAll(address)
+getTransparentVaultsEpoch() address[]
+getVaultPortfolio(address) address[] uint256[]
+pause()
+unpause()
}
InternalStatesOrchestratorUpgradeable ..|> IInternalStateOrchestrator
InternalStatesOrchestratorUpgradeable --|> Initializable
InternalStatesOrchestratorUpgradeable --|> Ownable2StepUpgradeable
InternalStatesOrchestratorUpgradeable --|> ReentrancyGuardUpgradeable
InternalStatesOrchestratorUpgradeable --|> PausableUpgradeable
InternalStatesOrchestratorUpgradeable --|> UUPSUpgradeable
%% Liquidity orchestrator
class LiquidityOrchestratorUpgradeable {
+address automationRegistry
+IOrionConfig config
+IInternalStateOrchestrator internalStatesOrchestrator
+address underlyingAsset
+address admin
+mapping executionAdapterOf
+uint16 epochCounter
+uint8 minibatchSize
+LiquidityUpkeepPhase currentPhase
+uint8 currentMinibatchIndex
+uint256 targetBufferRatio
+uint256 slippageTolerance
+int256 deltaBufferAmount
+updateMinibatchSize(uint8)
+updateAutomationRegistry(address)
+setInternalStatesOrchestrator(address)
+setTargetBufferRatio(uint256)
+depositLiquidity(uint256)
+withdrawLiquidity(uint256)
+claimProtocolFees(uint256)
+setExecutionAdapter(address, IExecutionAdapter)
+advanceIdlePhase()
+returnDepositFunds(address, uint256)
+transferCuratorFees(uint256)
+transferRedemptionFunds(address, uint256)
+withdraw(uint256, address)
+checkUpkeep(bytes) bool bytes
+performUpkeep(bytes)
+pause()
+unpause()
}
LiquidityOrchestratorUpgradeable ..|> ILiquidityOrchestrator
LiquidityOrchestratorUpgradeable --|> Initializable
LiquidityOrchestratorUpgradeable --|> Ownable2StepUpgradeable
LiquidityOrchestratorUpgradeable --|> ReentrancyGuardUpgradeable
LiquidityOrchestratorUpgradeable --|> PausableUpgradeable
LiquidityOrchestratorUpgradeable --|> UUPSUpgradeable
%% Base vault
class OrionVaultUpgradeable {
<<abstract>>
+address vaultOwner
+address curator
+IOrionConfig config
+IInternalStateOrchestrator internalStatesOrchestrator
+ILiquidityOrchestrator liquidityOrchestrator
+address depositAccessControl
+uint256 _totalAssets
+uint256 pendingCuratorFees
+bool isDecommissioning
+struct FeeModel
+FeeModel feeModel
+FeeModel oldFeeModel
+uint256 newFeeRatesTimestamp
+overrideIntentForDecommissioning()
+requestDeposit(uint256)
+cancelDepositRequest(uint256)
+requestRedeem(uint256)
+cancelRedeemRequest(uint256)
+vaultWhitelist() address[]
+updateFeeModel(uint8, uint16, uint16)
+curatorFee(uint256) uint256
+claimCuratorFees(uint256)
+setDepositAccessControl(address)
+pendingDeposit(uint256) uint256
+pendingRedeem(uint256) uint256
+accrueCuratorFees(uint256)
+fulfillDeposit(uint256)
+fulfillRedeem(uint256)
}
OrionVaultUpgradeable ..|> IOrionVault
OrionVaultUpgradeable --|> Initializable
OrionVaultUpgradeable --|> ERC4626Upgradeable
OrionVaultUpgradeable --|> ReentrancyGuardUpgradeable
%% Transparent vault
class OrionTransparentVaultUpgradeable {
+mapping _portfolio
+mapping _portfolioIntent
+initialize(address, address, IOrionConfig, string, string, uint8, uint16, uint16, address)
+submitIntent(IntentPosition[])
+getPortfolio() address[] uint256[]
+getIntent() address[] uint32[]
+updateVaultState(address[], uint256[], uint256)
+updateCurator(address)
+updateVaultWhitelist(address[])
+removeFromVaultWhitelist(address)
}
OrionTransparentVaultUpgradeable ..|> IOrionTransparentVault
OrionTransparentVaultUpgradeable --|> OrionVaultUpgradeable
%% Transparent vault factory
class TransparentVaultFactoryUpgradeable {
+IOrionConfig config
+UpgradeableBeacon vaultBeacon
+initialize(address, address, address)
+createVault(address, string, string, uint8, uint16, uint16, address) address
+setVaultBeacon(address)
}
TransparentVaultFactoryUpgradeable --|> Initializable
TransparentVaultFactoryUpgradeable --|> OwnableUpgradeable
TransparentVaultFactoryUpgradeable --|> UUPSUpgradeable
%% Relationships between core components
OrionConfigUpgradeable o-- PriceAdapterRegistryUpgradeable : configAddress
OrionConfigUpgradeable o-- InternalStatesOrchestratorUpgradeable : internalStatesOrchestrator
OrionConfigUpgradeable o-- LiquidityOrchestratorUpgradeable : liquidityOrchestrator
OrionConfigUpgradeable o-- TransparentVaultFactoryUpgradeable : transparentVaultFactory
PriceAdapterRegistryUpgradeable o-- IPriceAdapter : adapterOf
InternalStatesOrchestratorUpgradeable o-- IOrionConfig : config
InternalStatesOrchestratorUpgradeable o-- ILiquidityOrchestrator : liquidityOrchestrator
InternalStatesOrchestratorUpgradeable o-- IPriceAdapterRegistry : registry
LiquidityOrchestratorUpgradeable o-- IOrionConfig : config
LiquidityOrchestratorUpgradeable o-- IInternalStateOrchestrator : internalStatesOrchestrator
LiquidityOrchestratorUpgradeable o-- IExecutionAdapter : executionAdapterOf
OrionVaultUpgradeable o-- IOrionConfig : config
OrionVaultUpgradeable o-- IInternalStateOrchestrator : internalStatesOrchestrator
OrionVaultUpgradeable o-- ILiquidityOrchestrator : liquidityOrchestrator
TransparentVaultFactoryUpgradeable o-- IOrionConfig : config
TransparentVaultFactoryUpgradeable o-- OrionTransparentVaultUpgradeable : creates
OrionTransparentVaultUpgradeable <.. OrionTransparentVaultUpgradeableV2 : upgraded_version
OrionConfigUpgradeable <.. OrionConfigUpgradeableV2 : upgraded_version
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. WalkthroughThis PR migrates core Orion protocol contracts from non-upgradeable implementations to OpenZeppelin UUPS upgradeable proxies. It introduces upgradeable variants of OrionConfig, vault implementations, factories, orchestrators, and price adapters with Initializable/Ownable2Step/UUPS patterns, includes a new deployable protocol helper for tests, and updates 20+ test files to use the new upgradeable architecture. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 found some issues that need to be addressed.
- In
TransparentVaultFactoryUpgradeable.setVaultBeaconyou allow the owner to change the beacon at any time without checkingconfig.isSystemIdle(), which is stricter elsewhere (e.g.OrionConfigUpgradeable.setVaultFactory/update*); consider adding the same idle-phase guard so upgrades can't happen mid-epoch. - Both
LiquidityOrchestratorUpgradeableandInternalStatesOrchestratorUpgradeableexposeupdateAutomationRegistrybut theautomationRegistryaddress is not validated against any known registry interface or sanity-checked beyond non-zero; if this is security-sensitive, consider constraining or emitting additional diagnostic information to help detect misconfiguration. - The upgradeability test scripts under
scripts/(e.g.testUpgradeability.ts,verifyUpgradeRequiresNewAddress.ts) embed fairly heavy, test-like logic; if they are meant purely for local experiments it may be worth clearly separating or guarding them so they are not confused with production tooling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TransparentVaultFactoryUpgradeable.setVaultBeacon` you allow the owner to change the beacon at any time without checking `config.isSystemIdle()`, which is stricter elsewhere (e.g. `OrionConfigUpgradeable.setVaultFactory/update*`); consider adding the same idle-phase guard so upgrades can't happen mid-epoch.
- Both `LiquidityOrchestratorUpgradeable` and `InternalStatesOrchestratorUpgradeable` expose `updateAutomationRegistry` but the `automationRegistry` address is not validated against any known registry interface or sanity-checked beyond non-zero; if this is security-sensitive, consider constraining or emitting additional diagnostic information to help detect misconfiguration.
- The upgradeability test scripts under `scripts/` (e.g. `testUpgradeability.ts`, `verifyUpgradeRequiresNewAddress.ts`) embed fairly heavy, test-like logic; if they are meant purely for local experiments it may be worth clearly separating or guarding them so they are not confused with production tooling.
## Individual Comments
### Comment 1
<location> `contracts/orchestrators/InternalStatesOrchestratorUpgradeable.sol:540-541` </location>
<code_context>
+ for (uint16 i = 0; i < nTransparentVaults; ++i) {
+ address vault = transparentVaultsEpoch[i];
+ uint256 vaultAssets = _currentEpoch.vaultsTotalAssets[address(vault)];
+ uint256 vaultBufferCost = deltaBufferAmount.mulDiv(vaultAssets, protocolTotalAssets);
+ _currentEpoch.vaultsTotalAssets[address(vault)] -= vaultBufferCost;
+ actualBufferAllocated += vaultBufferCost;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle protocolTotalAssets == 0 in _buffer to avoid division-by-zero.
In `_buffer`, when all vaults have zero assets, `protocolTotalAssets` remains 0 and `deltaBufferAmount.mulDiv(vaultAssets, protocolTotalAssets)` will revert due to division by zero. Add a guard (e.g., `if (protocolTotalAssets == 0) return;` before computing per-vault costs) to avoid reverting upkeep when TVL is zero.
</issue_to_address>
### Comment 2
<location> `contracts/orchestrators/InternalStatesOrchestratorUpgradeable.sol:739-743` </location>
<code_context>
+ }
+
+ /// @inheritdoc IInternalStateOrchestrator
+ function updateBufferAmount(int256 deltaAmount) external onlyLiquidityOrchestrator {
+ if (deltaAmount > 0) {
+ bufferAmount += uint256(deltaAmount);
+ } else if (deltaAmount < 0) {
+ bufferAmount -= uint256(-deltaAmount);
+ }
+ }
</code_context>
<issue_to_address>
**issue:** Add safety checks to prevent bufferAmount underflow when applying negative deltas.
When `deltaAmount < 0`, this subtracts `uint256(-deltaAmount)` from `bufferAmount` without ensuring `bufferAmount` is sufficient. Unlike the admin withdrawal path, the P&L `deltaBufferAmount` path has no guard. A large negative delta (e.g., from slippage) could underflow, revert, and wedge the protocol in a non-idle phase. Please either cap the negative delta at `bufferAmount` or explicitly revert with a domain-specific error when `uint256(-deltaAmount) > bufferAmount` so the failure mode is controlled and clear.
</issue_to_address>
### Comment 3
<location> `contracts/factories/TransparentVaultFactoryUpgradeable.sol:106-108` </location>
<code_context>
+
+ /// @notice Updates the vault beacon address
+ /// @param newVaultBeacon The new UpgradeableBeacon address
+ function setVaultBeacon(address newVaultBeacon) external onlyOwner {
+ if (newVaultBeacon == address(0)) revert ErrorsLib.ZeroAddress();
+ vaultBeacon = UpgradeableBeacon(newVaultBeacon);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider restricting beacon changes to idle system state to avoid mid-epoch implementation flips.
Elsewhere (e.g. `OrionConfigUpgradeable`) config setters require `isSystemIdle()`, but `setVaultBeacon` does not. Changing the beacon mid-epoch could cause some vault calls in the same cycle to hit different implementations. Please add a `config.isSystemIdle()` check here to match the rest of the config surface and avoid inconsistent behaviour during upgrades.
Suggested implementation:
```
/// @notice Updates the vault beacon address
/// @param newVaultBeacon The new UpgradeableBeacon address
function setVaultBeacon(address newVaultBeacon) external onlyOwner {
if (!config.isSystemIdle()) revert ErrorsLib.SystemNotIdle();
if (newVaultBeacon == address(0)) revert ErrorsLib.ZeroAddress();
vaultBeacon = UpgradeableBeacon(newVaultBeacon);
}
```
I assumed the existence of a `config` state variable of type `IOrionConfig` and a custom error `ErrorsLib.SystemNotIdle()`, based on the rest of the codebase pattern you referenced. If the error name differs, adjust `ErrorsLib.SystemNotIdle()` to the correct one used in other config setters that gate on `isSystemIdle()`. Also ensure `config` is already initialized in the contract’s initializer/constructor as it is for other config-dependent functions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/FeeCooldown.test.ts (1)
55-85: Fix return type to match upgradeable contract.The function returns
OrionTransparentVaultUpgradeablebut declares return type asOrionTransparentVault. This type mismatch should be corrected.async function createVault( fixture: Awaited<ReturnType<typeof deployFixture>>, feeType: number, performanceFee: number, managementFee: number, - ): Promise<OrionTransparentVault> { + ): Promise<OrionTransparentVaultUpgradeable> {test/mainnet-fork/erc4626VaultCompatibility.test.ts (1)
34-55: Missing type imports for orchestrator variables.
InternalStatesOrchestratorandLiquidityOrchestratortypes are used at lines 50-51 but are not imported fromtypechain-types. This will cause TypeScript compilation errors.import { OrionConfigUpgradeable, TransparentVaultFactoryUpgradeable, PriceAdapterRegistryUpgradeable, OrionAssetERC4626PriceAdapter, OrionAssetERC4626ExecutionAdapter, + InternalStatesOrchestrator, + LiquidityOrchestrator, } from "../../typechain-types";test/mainnet-fork/removeWhitelistedAsset.test.ts (2)
106-118: Type mismatch: deploying non-upgradeable contracts but casting to upgradeable types.The test deploys
OrionConfigandTransparentVaultFactory(non-upgradeable versions) but casts them toOrionConfigUpgradeableandTransparentVaultFactoryUpgradeable. These are fundamentally different contracts with different ABIs and storage layouts. Theas unknown ascast masks compile-time type errors but will cause runtime issues if upgradeable-specific methods are called.Either:
- Use the
deployUpgradeableProtocolhelper like other test files, or- Keep non-upgradeable types if testing with non-upgradeable contracts
- orionConfig = orionConfigDeployed as unknown as OrionConfigUpgradeable; + // Keep using non-upgradeable types since we're deploying non-upgradeable contracts + // Or switch to deployUpgradeableProtocol helper for consistency
143-149: Same type mismatch issue for PriceAdapterRegistry.
PriceAdapterRegistryis deployed but cast toPriceAdapterRegistryUpgradeable. This is inconsistent with the upgradeable architecture.
🧹 Nitpick comments (23)
tsconfig.json (1)
11-11: Track the TODO for cleanup after migration testing.The inline comment indicates this inclusion is temporary. Consider creating a tracking issue to ensure this is revisited once upgradeable contracts are validated in production.
contracts/orchestrators/LiquidityOrchestratorUpgradeable.sol (1)
384-422: Consider overflow safety forint256casts.Lines 397 and 421 cast
uint256values toint256for delta calculations. While unlikely in practice, extremely large amounts could overflow. Consider usingSafeCastfrom OpenZeppelin for explicit overflow protection.+import "@openzeppelin/contracts/utils/math/SafeCast.sol"; ... + using SafeCast for uint256; ... - deltaBufferAmount += int256(executionUnderlyingAmount) - int256(estimatedUnderlyingAmount); + deltaBufferAmount += executionUnderlyingAmount.toInt256() - estimatedUnderlyingAmount.toInt256();test/OrionVaultExchangeRate.test.ts (1)
42-55: Add explicit check for missing event.If
eventisundefined(e.g., event not emitted or transaction failed),parseLog(event!)will throw a confusing error. Consider adding an explicit check.const event = receipt?.logs.find((log) => { try { const parsed = factory.interface.parseLog(log); return parsed?.name === "OrionVaultCreated"; } catch { return false; } }); + if (!event) { + throw new Error("OrionVaultCreated event not found in transaction receipt"); + } + const parsedEvent = factory.interface.parseLog(event!); const vaultAddress = parsedEvent?.args[0];scripts/verifyUpgradeRequiresNewAddress.ts (1)
20-20: Unused variableadmin.The
adminsigner is destructured but never used in this script. Consider removing it to avoid confusion.- const [owner, admin] = await ethers.getSigners(); + const [owner] = await ethers.getSigners();test/BatchLimitConsistency.test.ts (1)
113-124: Add null check before parsing event.If no matching log is found,
eventwill beundefinedand the non-null assertion on line 123 could cause a runtime error. Consider adding explicit validation.const event = receipt?.logs.find((log) => { try { const parsed = transparentVaultFactory.interface.parseLog(log); return parsed?.name === "OrionVaultCreated"; } catch { return false; } }); + if (!event) { + throw new Error("OrionVaultCreated event not found"); + } const parsedEvent = transparentVaultFactory.interface.parseLog(event!); const vaultAddress = parsedEvent?.args[0];scripts/testUpgradeability.ts (1)
239-248: Consider using interface.parseLog for event extraction.The current event parsing approach with manual type coercion is fragile. Using the factory's interface for parsing (as done in test files) is more reliable.
// Get vault address from event - const vaultCreatedEvent = receipt.logs.find((log: unknown) => { - const parsed = log as { fragment?: { name?: string }; args?: string[] }; - return parsed.fragment && parsed.fragment.name === "OrionVaultCreated"; - }) as { args: string[] } | undefined; - - if (!vaultCreatedEvent || !vaultCreatedEvent.args[0]) { + const vaultCreatedEvent = receipt.logs.find((log) => { + try { + const parsed = factoryProxy.interface.parseLog(log); + return parsed?.name === "OrionVaultCreated"; + } catch { + return false; + } + }); + + if (!vaultCreatedEvent) { throw new Error("OrionVaultCreated event not found or invalid"); } - const vaultAddress: string = vaultCreatedEvent.args[0]; + const parsedEvent = factoryProxy.interface.parseLog(vaultCreatedEvent); + const vaultAddress: string = parsedEvent?.args[0];contracts/price/PriceAdapterRegistryUpgradeable.sol (1)
85-89: Storage gap calculation is incorrect.The comment states "Total storage slots reserved: 50", but the contract has 3 state variables (
configAddress,priceAdapterDecimals,adapterOf) consuming slots before the gap. With a 50-slot gap, the total is 53 slots. If the intention is 50 total slots for future upgrades, the gap should be 47. Alternatively, update the comment to clarify the gap reserves 50 additional slots./** * @dev Storage gap to allow for future upgrades - * Total storage slots reserved: 50 + * Storage gap for future state variables (50 additional slots) */ uint256[50] private __gap;test/mainnet-fork/multiAssetRobustness.test.ts (1)
324-363: Test deploys non-upgradeable contracts but uses upgradeable types.This test deploys the non-upgradeable versions (
OrionConfig,PriceAdapterRegistry,TransparentVaultFactory) but casts them to upgradeable types. This is inconsistent with the PR's upgradeable architecture and doesn't actually test the upgradeable contracts.Consider using
deployUpgradeableProtocolhelper (as done inOrchestratorsZeroState.test.ts) or deploying the upgradeable versions directly for consistency across the test suite.contracts/factories/TransparentVaultFactoryUpgradeable.sol (2)
104-109: Consider emitting an event when the vault beacon is updated.Changing the vault beacon affects all future vault deployments. Emitting an event would help with off-chain monitoring and auditing of beacon changes.
+ // Add to EventsLib: + // event VaultBeaconUpdated(address indexed oldBeacon, address indexed newBeacon); function setVaultBeacon(address newVaultBeacon) external onlyOwner { if (newVaultBeacon == address(0)) revert ErrorsLib.ZeroAddress(); + address oldBeacon = address(vaultBeacon); vaultBeacon = UpgradeableBeacon(newVaultBeacon); + // emit EventsLib.VaultBeaconUpdated(oldBeacon, newVaultBeacon); }
117-121: Storage gap calculation in comment is incorrect.The comment states "50 - 1 for vaultBeacon" but there are 2 state variables (
configat line 22 andvaultBeaconat line 25). The gap should be 48 (50 - 2), or the comment should be updated to reflect the actual calculation./** * @dev Storage gap to allow for future upgrades - * Total storage slots reserved: 49 (50 - 1 for vaultBeacon) + * Storage gap: 48 slots (50 - 2 for config and vaultBeacon) */ - uint256[49] private __gap; + uint256[48] private __gap;test/helpers/deployUpgradeable.ts (2)
23-23: Consider using proper type forvaultBeaconinstead ofunknown.The
unknowntype loses type safety. Consider importing and using the proper type from the typechain-generated types or at minimum use a more descriptive type.- vaultBeacon: unknown; // UpgradeableBeacon instance + vaultBeacon: Awaited<ReturnType<typeof ethers.deployContract>>; // UpgradeableBeacon instanceAlternatively, if you have generated types for OpenZeppelin contracts:
import { UpgradeableBeacon } from "../../typechain-types/@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon";
91-109: Duplicate step numbering: two "step 5" comments.Steps 5 (lines 91-98) and 5 (lines 100-109) have the same number. This creates confusion in the deployment sequence documentation.
- // 5. Deploy InternalStatesOrchestratorUpgradeable (UUPS) - reads liquidityOrchestrator from config + // 5. Deploy InternalStatesOrchestratorUpgradeable (UUPS) - reads liquidityOrchestrator from config const InternalStatesOrchestratorFactory = await ethers.getContractFactory("InternalStatesOrchestratorUpgradeable"); ... await internalStatesOrchestrator.waitForDeployment(); - // 5. Deploy UpgradeableBeacon for vaults + // 6. Deploy UpgradeableBeacon for vaults const VaultImplFactory = await ethers.getContractFactory("OrionTransparentVaultUpgradeable"); ... - // 6. Deploy TransparentVaultFactoryUpgradeable (UUPS) + // 7. Deploy TransparentVaultFactoryUpgradeable (UUPS) ... - // 7. Configure OrionConfig with remaining deployed contracts + // 8. Configure OrionConfig with remaining deployed contracts ... - // 8. Link orchestrators (LiquidityOrchestrator needs InternalStatesOrchestrator reference) + // 9. Link orchestrators (LiquidityOrchestrator needs InternalStatesOrchestrator reference)test/TransparentVault.test.ts (1)
29-31: Unused variables declared with underscore prefix.The variables
_priceAdapterRegistry,_internalStatesOrchestrator, and_liquidityOrchestratorare assigned from the deployment result but appear unused in this test file. If they're intentionally unused, consider not destructuring them at all to avoid confusion.-let _priceAdapterRegistry: PriceAdapterRegistryUpgradeable; -let _internalStatesOrchestrator: InternalStatesOrchestratorUpgradeable; -let _liquidityOrchestrator: LiquidityOrchestratorUpgradeable;And at lines 65-67:
- _priceAdapterRegistry = deployed.priceAdapterRegistry; - _internalStatesOrchestrator = deployed.internalStatesOrchestrator; - _liquidityOrchestrator = deployed.liquidityOrchestrator; + // priceAdapterRegistry, internalStatesOrchestrator, liquidityOrchestrator available from deployed if neededcontracts/vaults/OrionTransparentVaultUpgradeable.sol (1)
111-120: Consider using uint256 for loop indices to avoid potential truncation.Multiple functions cast
.length()touint16, which could truncate for large maps:
- Line 112:
uint16(_portfolio.length())- Line 131:
uint16(_portfolioIntent.length())- Line 150:
uint16(tokens.length)While unlikely to exceed 65,535 in practice, using
uint256is safer and consistent with Solidity conventions.- uint16 length = uint16(_portfolio.length()); + uint256 length = _portfolio.length(); tokens = new address[](length); sharesPerAsset = new uint256[](length); - for (uint16 i = 0; i < length; ++i) { + for (uint256 i = 0; i < length; ++i) {Also applies to: 131-139, 150-154
test/BatchLimitAccounting.test.ts (2)
27-42: UnusedautomationRegistrysigner while deployment uses default.The fixture declares
automationRegistryat line 31 but doesn't pass it todeployUpgradeableProtocol. The deployment helper will default to usingadmin(which isownerhere) as the automation registry. This inconsistency could cause confusion if tests expectautomationRegistryto have special privileges.If
automationRegistryshould be used:- const deployed = await deployUpgradeableProtocol(owner, owner); + const deployed = await deployUpgradeableProtocol(owner, owner, undefined, automationRegistry);If
automationRegistryis intentionally unused, remove it:const owner = allSigners[0]; const curator = allSigners[1]; - const automationRegistry = allSigners[2]; - const users = allSigners.slice(3); // Remaining signers for testing + const users = allSigners.slice(2); // Remaining signers for testing
72-83: Fixture return object includes unusedautomationRegistry.Given the earlier observation that
automationRegistryisn't passed to deployment and may not have expected privileges, including it in the returned object could mislead test authors. Either pass it to deployment or remove from returns.test/OrionConfigVault.test.ts (1)
414-421: Impersonated account not stopped after use.The
impersonateAccountis called forloAddressbutstopImpersonatingAccountis not called after thefulfillDepositoperation. While this may work in isolated tests, it's good practice to clean up impersonation to avoid state leakage across tests in the samebeforeEachblock.await vault.connect(loSigner).fulfillDeposit(depositAmount); + + // Stop impersonation + await ethers.provider.send("hardhat_stopImpersonatingAccount", [loAddress]); });test/Adapters.test.ts (1)
44-48: Incorrect type cast: MockUnderlyingAsset cast to MockERC4626Asset.The code deploys
MockUnderlyingAsset(a simple ERC20 token) but casts it toMockERC4626Asset(an ERC4626 vault). These are fundamentally different contracts:
MockUnderlyingAssetis a basic ERC20 withmintfunctionMockERC4626Assetis an ERC4626 vault withdeposit,withdraw,asset(), etc.The test at line 59 calls
orionConfig.addWhitelistedAssetwith this miscast asset, which should correctly revert withInvalidAdaptersince a plain ERC20 is not ERC4626-compliant. However, the variable name and type are misleading.Consider renaming for clarity:
- // Deploy additional mock asset for testing (different from underlying) + // Deploy a regular ERC20 (not ERC4626) for adapter validation tests const MockERC20AssetFactory = await ethers.getContractFactory("MockUnderlyingAsset"); const mockAsset1Deployed = await MockERC20AssetFactory.deploy(10); await mockAsset1Deployed.waitForDeployment(); - mockAsset1 = mockAsset1Deployed as unknown as MockERC4626Asset; + // Note: This is intentionally a plain ERC20 to test adapter rejection + mockAsset1 = mockAsset1Deployed as unknown as MockERC4626Asset; // Used to test InvalidAdapter errorscontracts/OrionConfigUpgradeable.sol (2)
37-84: Storage gap should be placed before state variables or storage layout documented.In upgradeable contracts, the storage gap at line 463 comes after all state variables. However, the
EnumerableSetvariables (lines 73-83) are complex types that internally manage their own storage slots. When adding new state variables in future upgrades, developers must be careful to add them before the gap, not after the existing variables.Consider documenting the storage layout explicitly or moving the gap declaration closer to a comment that clarifies where new variables should be inserted in V2.
251-267: External call tothis.isWhitelistedis unnecessary gas overhead.Using
this.isWhitelisted(asset)on line 254 makes an external call to the same contract. SinceisWhitelistedjust reads fromwhitelistedAssets, you can call the internal storage directly.function addWhitelistedAsset(address asset, address priceAdapter, address executionAdapter) external onlyOwner { if (!isSystemIdle()) revert ErrorsLib.SystemNotIdle(); - if (!this.isWhitelisted(asset)) { + if (!whitelistedAssets.contains(asset)) { // slither-disable-next-line unused-return whitelistedAssets.add(asset); }contracts/orchestrators/InternalStatesOrchestratorUpgradeable.sol (2)
568-577: TODO comment indicates incomplete implementation for excluded assets.Lines 568-577 contain a TODO describing complex logic for handling excluded assets during rebalancing. This appears to be unimplemented functionality that could affect accounting correctness if
excludedAssetsis non-empty.The TODO describes reinterpreting intents when assets are excluded. Would you like help implementing this logic, or should a follow-up issue be created to track this?
93-118: Clarify the EpochState mapping invariant with explicit documentation.The EpochState struct with nested mappings relies on a subtle invariant: all mappings (
priceArray,initialBatchPortfolio,finalBatchPortfolio,sellingOrders,buyingOrders) are only updated for tokens tracked in thetokensarray, ensuring epoch reset in_handleStartclears all stale data. Solidity cannot delete mappings because it does not know the keys, so this array-based tracking is essential.The code currently maintains this invariant correctly:
priceArrayupdates are gated bytokenExistschecks, and all other mapping assignments are followed by or derive from_addTokenIfNotExistscalls. However, this pattern is implicit and fragile. Add a comment above the EpochState struct definition clearly stating this invariant, or consider adding runtime checks (e.g., assertions in_handleStart) to detect violations during testing.contracts/vaults/OrionVaultUpgradeable.sol (1)
224-230:_initializeVaultWhitelistreverts on duplicate which should be impossible.The check
if (!inserted) revert ErrorsLib.AlreadyRegistered()on line 228 would only trigger if the protocol whitelist contains duplicates, which should be prevented byOrionConfig.addWhitelistedAsset. This is defensive but the revert could be removed or changed to acontinue.function _initializeVaultWhitelist() internal { address[] memory protocolAssets = config.getAllWhitelistedAssets(); for (uint256 i = 0; i < protocolAssets.length; ++i) { - bool inserted = _vaultWhitelistedAssets.add(protocolAssets[i]); - if (!inserted) revert ErrorsLib.AlreadyRegistered(); + // Protocol whitelist should not contain duplicates + _vaultWhitelistedAssets.add(protocolAssets[i]); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
contracts/OrionConfigUpgradeable.sol(1 hunks)contracts/factories/TransparentVaultFactoryUpgradeable.sol(1 hunks)contracts/mocks/OrionConfigUpgradeableV2.sol(1 hunks)contracts/mocks/OrionTransparentVaultUpgradeableV2.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestratorUpgradeable.sol(1 hunks)contracts/orchestrators/LiquidityOrchestratorUpgradeable.sol(1 hunks)contracts/price/PriceAdapterRegistryUpgradeable.sol(1 hunks)contracts/vaults/OrionTransparentVaultUpgradeable.sol(1 hunks)contracts/vaults/OrionVaultUpgradeable.sol(1 hunks)package.json(1 hunks)scripts/testUpgradeability.ts(1 hunks)scripts/verifyUpgradeRequiresNewAddress.ts(1 hunks)test/AccessControl.test.ts(7 hunks)test/Adapters.test.ts(2 hunks)test/BatchLimitAccounting.test.ts(3 hunks)test/BatchLimitConsistency.test.ts(1 hunks)test/ExecutionAdapterValidation.test.ts(1 hunks)test/FeeCooldown.test.ts(3 hunks)test/MinimumAmountDOS.test.ts(3 hunks)test/OrionConfigVault.test.ts(5 hunks)test/OrionVaultExchangeRate.test.ts(2 hunks)test/PassiveCuratorStrategy.test.ts(6 hunks)test/ProtocolPause.test.ts(6 hunks)test/Removal.test.ts(5 hunks)test/TransparentVault.test.ts(8 hunks)test/VaultOwnerRemoval.test.ts(4 hunks)test/helpers/deployUpgradeable.ts(1 hunks)test/mainnet-fork/erc4626VaultCompatibility.test.ts(11 hunks)test/mainnet-fork/multiAssetRobustness.test.ts(6 hunks)test/mainnet-fork/removeWhitelistedAsset.test.ts(6 hunks)test/orchestrator/OrchestratorConfiguration.test.ts(12 hunks)test/orchestrator/OrchestratorSecurity.test.ts(12 hunks)test/orchestrator/OrchestratorsZeroState.test.ts(3 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
test/BatchLimitConsistency.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/orchestrator/OrchestratorsZeroState.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/Adapters.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/BatchLimitAccounting.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/VaultOwnerRemoval.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/MinimumAmountDOS.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/PassiveCuratorStrategy.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/OrionConfigVault.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/ExecutionAdapterValidation.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
contracts/orchestrators/LiquidityOrchestratorUpgradeable.sol (2)
test/orchestrator/Orchestrators.test.ts (2)
expect(677-2173)it(676-2608)test/RedeemBeforeDepositOrder.test.ts (1)
processLiquidityOrchestrator(73-79)
test/ProtocolPause.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/TransparentVault.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/Removal.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
test/AccessControl.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(44-138)
🪛 GitHub Actions: CI
contracts/orchestrators/LiquidityOrchestratorUpgradeable.sol
[error] 465-465: Hardhat compilation failed: TypeError: Member "processLP" not found or not visible after argument-dependent lookup in contract IInternalStateOrchestrator.
⏰ 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 (78)
package.json (1)
80-80: Verify package version validity and security advisories.The addition of
@openzeppelin/contracts-upgradeableis appropriate for the PR's upgradeable architecture objectives. Ensure that version 5.4.0 exists, is secure, and is compatible with the base contracts version.Run the following script to verify the package version and check for security advisories:
#!/bin/bash # Description: Verify @openzeppelin/contracts-upgradeable version 5.4.0 exists and check for vulnerabilities # Check NPM registry for version existence echo "Checking NPM registry for version 5.4.0..." curl -s https://registry.npmjs.org/@openzeppelin/contracts-upgradeable/5.4.0 | jq -r '.version // "Version not found"' # Check latest available version echo -e "\nLatest version available:" curl -s https://registry.npmjs.org/@openzeppelin/contracts-upgradeable/latest | jq -r '.version' # Check for security advisories echo -e "\nChecking for security advisories..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "@openzeppelin/contracts-upgradeable") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'contracts/orchestrators/LiquidityOrchestratorUpgradeable.sol (6)
510-514: Storage gap placement looks correct.The 50-slot storage gap at the end of the contract follows OpenZeppelin's upgradeable contract pattern and provides reasonable room for future state additions.
504-508: LGTM: Standard UUPS authorization.The
_authorizeUpgradeimplementation correctly restricts upgrades to the owner.
306-334: Upkeep functions have proper guards.The
performUpkeepcorrectly usesonlyAuthorizedTrigger,nonReentrant, andwhenNotPausedmodifiers. Phase transitions are deterministic.
424-451: Batch processing logic is sound.The minibatch approach correctly handles vault iteration with proper boundary checks and state resets. This prevents gas limit issues for large vault counts.
178-183: One-time setter forinternalStatesOrchestrator.The function intentionally prevents re-registration. Ensure the correct address is set during deployment as there's no recovery path without redeployment.
465-477: Remove this comment —processLP()is correctly defined inIInternalStateOrchestratorinterface.The method
processLP()is properly declared in the interface at line 63 asfunction processLP() external view returns (bool);. The code at line 465 is valid and will compile without errors.Likely an incorrect or invalid review comment.
test/OrionVaultExchangeRate.test.ts (2)
63-80: Fixture structure looks consistent with test patterns.The return structure correctly provides both contract instances and addresses. The naming convention (
liquidityOrchestratorfor signer,liquidityOrchestratorContractfor contract) aligns with impersonation patterns used in tests.
1-6: Clean migration to upgradeable protocol deployment.The test file correctly adopts the
deployUpgradeableProtocolhelper and upgrades all contract interactions to use upgradeable variants. Test semantics remain unchanged.contracts/mocks/OrionConfigUpgradeableV2.sol (1)
12-31: Well-structured V2 mock for UUPS upgrade testing.The mock correctly extends
OrionConfigUpgradeable, adds a new state variable after inherited storage, and provides upgrade verification viaversion(). TheonlyOwnermodifier is correctly inherited.contracts/mocks/OrionTransparentVaultUpgradeableV2.sol (1)
12-31: Well-structured V2 mock for beacon upgrade testing.The mock correctly extends the V1 contract, adds new state after inherited storage, and provides a
version()function for upgrade verification. TheonlyVaultOwnermodifier is properly inherited fromOrionVaultUpgradeableand ensures access control consistency.scripts/verifyUpgradeRequiresNewAddress.ts (3)
1-3: LGTM on imports.The imports are correctly structured for a Hardhat upgrades verification script.
84-147: LGTM on Beacon pattern verification.The Beacon upgrade test correctly demonstrates the no-op behavior when upgrading to the same implementation and validates successful upgrade to V2.
190-195: LGTM on script execution pattern.Standard Hardhat script execution pattern with proper error handling and exit codes.
test/BatchLimitConsistency.test.ts (4)
1-13: LGTM on imports and helper function.The imports are correctly structured for the upgradeable protocol testing, and the
impersonateLiquidityOrchestratorhelper correctly funds the impersonated account with ETH for gas.
135-193: LGTM on pendingDeposit batch limit tests.The tests correctly handle Hardhat's signer limitations and validate the batch limiting logic with appropriate assertions.
195-281: LGTM on pendingRedeem batch limit tests.The nested
beforeEachcorrectly sets up users with shares, and the tests properly validate redeem batch limiting behavior.
283-356: LGTM on double-counting prevention tests.The tests effectively validate the critical accounting fix, ensuring
pendingDepositandpendingRedeemrespectmaxFulfillBatchSizeto prevent overcounting intotalAssets().scripts/testUpgradeability.ts (3)
1-3: LGTM on imports.Correctly imports ethers, upgrades, and chai expect for the upgrade demonstration script.
267-339: LGTM on Beacon upgrade testing.The script properly demonstrates Beacon upgrade lifecycle: V2 deployment, beacon upgrade, state preservation verification, and V2 feature testing.
341-420: LGTM on UUPS upgrade testing and summary.The UUPS upgrade demonstration is thorough, with proper state preservation verification and V2 feature testing. The summary provides excellent documentation of upgrade semantics.
test/MinimumAmountDOS.test.ts (3)
1-6: LGTM on imports update.Imports correctly updated to include the OpenZeppelin upgrades plugin and the
deployUpgradeableProtocolhelper.
20-54: LGTM on fixture migration to upgradeable protocol.The fixture correctly uses
deployUpgradeableProtocoland attaches the vault using the upgradeable factory pattern.
75-422: LGTM on DOS prevention test coverage.The tests comprehensively cover minimum amount enforcement, attack prevention scenarios, edge cases, and integration with existing validations. The migration to upgradeable contracts is seamless.
test/FeeCooldown.test.ts (3)
1-6: LGTM on imports update.Imports correctly updated to include the OpenZeppelin upgrades plugin and the
deployUpgradeableProtocolhelper.
31-53: LGTM on fixture migration.The fixture correctly uses
deployUpgradeableProtocoland properly types theInternalStatesOrchestratorUpgradeable.
87-619: LGTM on fee cooldown test coverage.The tests comprehensively cover cooldown duration management, timing transitions, authorization, edge cases, and attack prevention scenarios. The migration to upgradeable contracts is properly integrated.
test/orchestrator/OrchestratorsZeroState.test.ts (2)
30-40: LGTM - correctly uses upgradeable deployment helper.The test properly uses
deployUpgradeableProtocoland correctly assigns all returned components. The pattern aligns with the upgradeable architecture introduced in this PR.Note:
useris passed as theadminparameter (second argument), which appears intentional for this test's access control setup.
60-63: LGTM - vault resolution uses correct upgradeable type.The vault is correctly retrieved using
OrionTransparentVaultUpgradeablecontract name, consistent with the beacon proxy pattern used in the upgradeable factory.test/mainnet-fork/erc4626VaultCompatibility.test.ts (1)
316-381: Good enhancement: Comprehensive immutability checks for proxy contracts.The updated immutability tests now properly:
- Check bytecode stability across blocks
- Detect proxy contracts via EIP-1967 implementation slot
- Verify implementation slot immutability for proxies
- Validate
asset()anddecimals()consistency across block boundariesThis is a solid approach for validating that external ERC4626 vaults meet Orion's immutability requirements.
test/helpers/deployUpgradeable.ts (1)
44-138: LGTM on the deployment flow logic.The deployment sequence correctly handles the dependency ordering:
- OrionConfig first (central registry)
- PriceAdapterRegistry (depends on config)
- LiquidityOrchestrator before InternalStatesOrchestrator (due to config dependency)
- Setting LiquidityOrchestrator in config before InternalStatesOrchestrator deployment
- Beacon + Factory for vaults
- Final wiring of cross-references
The UUPS proxy pattern is correctly applied with
kind: "uups"and proper initializer functions.test/TransparentVault.test.ts (2)
36-68: LGTM on the migration to upgradeable protocol deployment.The test setup correctly:
- Deploys mock underlying asset with explicit decimals
- Uses the new
deployUpgradeableProtocolhelper with the pre-deployed underlying asset- Extracts required components from the deployment result
- Properly types contract instances with upgradeable variants
124-127: Consistent usage of upgradeable contract artifacts.All vault retrievals correctly use
"OrionTransparentVaultUpgradeable"artifact name and cast toOrionTransparentVaultUpgradeabletype. This is consistent with the migration pattern.Also applies to: 160-163, 201-204, 235-238, 416-419
contracts/vaults/OrionTransparentVaultUpgradeable.sol (4)
36-38: LGTM on the constructor pattern for upgradeable contracts.The constructor correctly calls
_disableInitializers()to prevent the implementation contract from being initialized directly, following OpenZeppelin's UUPS best practices.
223-228: LGTM on storage gap for upgrade safety.The
uint256[50] private __gapprovides 50 storage slots for future upgrades, following OpenZeppelin's upgrade-safe storage pattern.
199-221: LGTM onremoveFromVaultWhitelistweight redistribution.The function correctly:
- Removes the asset from the whitelist
- Retrieves the blacklisted asset's weight from intent
- Redistributes the weight to the underlying asset
- Handles both cases where underlying already exists in intent or not
This ensures intent always sums to 100% after blacklist removal.
143-166: No reentrancy guard needed forupdateVaultState– the called function is view-only.The
convertToAssetsfunction is a public view function, meaning it cannot make external calls or modify state. The function only performs arithmetic operations and reads internal state. While the function modifies critical state variables (_portfolio,_totalAssets,feeModel.highWaterMark), the access control modifieronlyLiquidityOrchestratorprovides sufficient protection. Note that similar privileged functions likefulfillDepositandfulfillRedeemdo include thenonReentrantmodifier for consistency—consider adding it here if you prefer uniform defensive coverage across all orchestrator-called functions.test/ProtocolPause.test.ts (2)
86-97: LGTM on migration to upgradeable protocol deployment.The test correctly uses
deployUpgradeableProtocolwith proper parameters:
adminas both owner and adminundefinedfor underlying asset (creates mock internally)automationRegistryfor automation registryComponents are correctly extracted from the deployment result.
155-158: Consistent usage of upgradeable contract artifacts for vault retrieval.The vault is correctly retrieved using
"OrionTransparentVaultUpgradeable"artifact name.test/BatchLimitAccounting.test.ts (1)
63-64: LGTM on vault retrieval with upgradeable contract.The vault is correctly obtained using
getContractAt("OrionTransparentVaultUpgradeable", vaultAddress)and properly typed.test/OrionConfigVault.test.ts (3)
40-48: Good migration to upgradeable protocol deployment.The test correctly uses
deployUpgradeableProtocolhelper and properly destructures the returned components. Type assignments are consistent with the upgradeable contract variants.
112-115: Vault instantiation correctly uses upgradeable type.The vault is correctly retrieved using
OrionTransparentVaultUpgradeablecontract name, consistent with the upgradeable factory deployment.
397-506: Comprehensive redeem request cancellation tests added.The new test section properly sets up the user with shares via deposit fulfillment, then tests various cancellation scenarios including zero amount, over-cancellation, full cancellation, and partial cancellation. The test logic correctly validates share balances before and after operations.
test/ExecutionAdapterValidation.test.ts (3)
35-42: Correct upgradeable protocol deployment.The test properly uses
deployUpgradeableProtocolwith appropriate signers and correctly destructures the returned components.
24-26: Underscore prefix indicates intentionally unused variables.The variables
_priceAdapterRegistryand_internalStatesOrchestratorare prefixed with underscore following the convention for variables that need to be declared for destructuring but aren't used in tests. This is consistent with other test files.
183-201: Impersonation pattern is correct with proper cleanup.The test correctly impersonates
LiquidityOrchestrator, sets balance for gas, executes operations, and stops impersonation in the finally block. This pattern is consistent throughout the file.test/Adapters.test.ts (2)
35-42: Correct upgradeable protocol deployment.The test properly uses
deployUpgradeableProtocolwith the appropriate signers includingautomationRegistry.
176-218: ERC4626 Execution Adapter tests are well-structured.The nested
beforeEachproperly sets up an actual ERC4626 vault with the correct underlying asset, seeds it with initial deposits, and whitelists it. The slippage tolerance configuration ensures realistic testing conditions.test/VaultOwnerRemoval.test.ts (4)
31-38: Correct upgradeable protocol deployment in fixture.The fixture properly uses
deployUpgradeableProtocoland correctly types all returned components with their upgradeable variants.
59-89: Well-typed createVault helper function.The helper function correctly:
- Accepts
TransparentVaultFactoryUpgradeableandOrionConfigUpgradeableparameters- Returns
Promise<OrionTransparentVaultUpgradeable>- Uses
getContractAt("OrionTransparentVaultUpgradeable", ...)to retrieve the vaultThis is consistent with the upgradeable architecture.
91-128: Comprehensive vault owner removal tests.Tests properly validate:
- Decommissioning of all vaults owned by removed vault owner
- Event emission for
VaultOwnerRemoved- State consistency (
isOrionVault,isDecommissioningVault,isWhitelistedVaultOwner)
243-273: Good integration test for vault lifecycle.The test properly demonstrates the complete decommissioning flow:
- Creates vault
- Removes vault owner (triggers decommissioning)
- Impersonates LiquidityOrchestrator to complete decommissioning
- Validates final state (
isDecommissionedVault)The impersonation pattern with balance setting is correctly implemented.
test/orchestrator/OrchestratorConfiguration.test.ts (2)
71-85: LGTM!The imports are correctly updated to use the upgradeable contract type variants, and the import path for
deployUpgradeableProtocolis properly set.
186-193: LGTM!The upgradeable protocol deployment is correctly wired. The helper is called with the appropriate parameters (
owner,user,underlyingAsset,automationRegistry), and the returned components are properly destructured and assigned to the test variables.test/PassiveCuratorStrategy.test.ts (3)
1-6: LGTM!The imports are correctly placed and updated to use upgradeable contract variants.
110-117: LGTM!The upgradeable protocol deployment is properly integrated. The mock underlying asset is deployed first and then passed to the helper, which is the correct pattern for tests requiring a specific underlying asset configuration.
190-193: LGTM!Vault lookup correctly uses the upgradeable contract name
OrionTransparentVaultUpgradeable.test/AccessControl.test.ts (3)
1-13: LGTM!Imports are correctly updated for the upgradeable migration. The
@openzeppelin/hardhat-upgradesimport is properly placed.
23-27: LGTM!The underscore prefix convention for unused variables (
_orionConfig,_priceAdapterRegistry,_internalStatesOrchestrator,_liquidityOrchestrator) is appropriate and signals that these are intentionally unused in this test file.
35-43: LGTM!The deployment correctly uses the upgradeable helper and destructures all needed components. Passing
owneras both theownerandadminparameters is appropriate for these access control tests.test/Removal.test.ts (3)
1-6: LGTM!Imports are correctly updated for the upgradeable migration, and the import placement is proper.
77-84: LGTM!The upgradeable protocol deployment is correctly integrated with the mock underlying asset deployed beforehand. The destructuring properly assigns all required components.
130-133: LGTM!Vault lookup correctly uses the upgradeable contract name
OrionTransparentVaultUpgradeable.test/orchestrator/OrchestratorSecurity.test.ts (2)
94-108: LGTM!The imports after the JSDoc block are correctly set up. Once the misplaced import on line 3 is fixed, this file will have proper import structure.
209-216: LGTM!The upgradeable protocol deployment is correctly wired with proper parameter passing and destructuring.
contracts/OrionConfigUpgradeable.sol (2)
455-456: LGTM on upgrade authorization.The
_authorizeUpgradefunction correctly restricts upgrades to only the owner, following UUPS best practices.
434-440: I need the review comment to verify and rewrite. Please provide:
- The original review comment that needs to be rewritten
- The relevant code context or file contents
- Any repository or project information needed for verification
[cannot_proceed_missing_input]
contracts/orchestrators/InternalStatesOrchestratorUpgradeable.sol (3)
410-497: LGTM on preprocessing minibatch logic.The preprocessing logic correctly:
- Handles minibatch pagination with proper bounds checking
- Caches prices to avoid redundant oracle calls
- Calculates fees in the correct order (protocol volume fee, then curator fees)
- Uses proper decimal conversions via
UtilitiesLib.convertDecimals
516-546: Buffer calculation could leave dust due to rounding.In
_buffer(), thedeltaBufferAmountis distributed proportionally across vaults usingmulDiv. Due to integer division rounding, the sum ofvaultBufferCostacross all vaults may be slightly less thandeltaBufferAmount. The code correctly tracksactualBufferAllocatedto handle this, which is good.
785-789: Storage gap is correctly placed and sized.The 50-slot storage gap follows OpenZeppelin's recommended pattern for upgradeable contracts, allowing future versions to add state variables without breaking storage layout.
contracts/vaults/OrionVaultUpgradeable.sol (8)
158-162: Constructor correctly disables initializers for implementation contract.This follows the OpenZeppelin pattern for UUPS proxies, preventing the implementation contract from being initialized directly.
351-375: Deposit request flow correctly validates and transfers to liquidity orchestrator.The
requestDepositfunction:
- Checks access control, system idle state, and decommissioning status
- Validates minimum deposit amount
- Safely transfers tokens using
safeTransferFrom- Correctly accumulates requests for the same user
517-555: Fee calculation order matters - management fee reduces base before performance fee.The
curatorFeefunction calculates management fee first, then uses the reducedintermediateTotalAssetsfor performance fee calculation. This is intentional and correctly prevents double-counting. The_managementFeeAmountand_performanceFeeAmountfunctions properly prorate fees based on epoch duration.
561-581: Potential division by zero in_getBenchmarkwhendivisoris 0.In
_getBenchmark, thedivisorvariable can be 0 ifcurrentSharePriceis 0 (forABSOLUTEtype) or iffeeModel.highWaterMarkis 0 (forHIGH_WATER_MARKtype). The caller_performanceFeeAmountchecksif (activeSharePrice < benchmark || divisor == 0) return 0which guards against division by zero, but only after calling_getBenchmark.The current code is safe because the check happens before division, but documenting this invariant would help maintainability.
742-746: Storage gap correctly reserves 50 slots for future upgrades.The storage gap follows best practices and is placed at the end of the contract's state variables.
42-46: Abstract contract lacks UUPSUpgradeable - verify derived contracts include it.
OrionVaultUpgradeableis abstract and inherits fromInitializable,ERC4626Upgradeable, andReentrancyGuardUpgradeable, but notUUPSUpgradeable. For the UUPS pattern to work, derived contracts (e.g.,OrionTransparentVaultUpgradeable) must inheritUUPSUpgradeableand implement_authorizeUpgrade.Verify derived vault contracts include UUPS:
#!/bin/bash # Check if derived vault contracts inherit UUPSUpgradeable rg -n "contract.*Vault.*Upgradeable.*is" --type sol -A 5 | rg -A 5 "OrionVaultUpgradeable"
698-740:fulfillRedeemdoes not update_totalAssetsafter burning shares.Similar to
fulfillDeposit, thefulfillRedeemfunction burns shares and transfers assets without updating_totalAssets. The function accepts aredeemTotalAssetsparameter but never uses it to modify the state variable. This leaves the vault's total assets tracking stale after redemptions are processed. Verify whether the orchestration layer properly updates_totalAssetsthrough a separate mechanism or if this represents an accounting gap.
659-696:fulfillDepositdoes not update_totalAssetsdespite minting shares.When deposits are fulfilled, shares are minted to users but
_totalAssetsis not increased. While share pricing correctly uses thedepositTotalAssetsparameter passed from InternalStatesOrchestrator (not the vault's internal_totalAssets), this design creates an asymmetry where the vault's share count increases while its reportedtotalAssets()remains unchanged. Verify that this async architecture—where assets remain in LiquidityOrchestrator rather than transferred to the vault—is intentional and clarify whethertotalAssets()should reflect assets under management or assets physically held.
|
@matteoettam09, we need to delete the original contracts as well as they are all replaced by upgradable versions of the same |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/mainnet-fork/erc4626VaultCompatibility.test.ts (1)
181-225: Inconsistent deployment: Deploys non-upgradeable contracts but uses upgradeable type annotations.Lines 188-191 deploy
OrionConfig(non-upgradeable) but cast toOrionConfigUpgradeable. Similarly for other contracts. This creates a mismatch between runtime behavior and compile-time types.🔎 Suggested fix: Use consistent types
Either use the original non-upgradeable imports and types:
-import { - OrionConfigUpgradeable, - TransparentVaultFactoryUpgradeable, - PriceAdapterRegistryUpgradeable, +import { + OrionConfig, + TransparentVaultFactory, + PriceAdapterRegistry, ... -let orionConfig: OrionConfigUpgradeable; +let orionConfig: OrionConfig;Or deploy actual upgradeable contracts using the helper:
import { deployUpgradeableProtocol } from "../helpers/deployUpgradeable"; // In before(): const deployed = await deployUpgradeableProtocol(owner, admin); orionConfig = deployed.orionConfig;
♻️ Duplicate comments (11)
contracts/price/PriceAdapterRegistryUpgradeable.sol (1)
49-59: LGTM! Zero address validation and initialization are correct.The past review concern about missing
initialOwner_validation has been addressed. The initialization properly chains the upgradeable base contracts and readspriceAdapterDecimalsfrom config.contracts/factories/TransparentVaultFactoryUpgradeable.sol (2)
38-47: LGTM! Zero address validation now included for all initialization parameters.The past review concern has been addressed -
initialOwneris now validated before initialization proceeds.
105-111: MissingisSystemIdle()check insetVaultBeacon.As previously noted, changing the vault beacon while the system is active could cause vaults to behave inconsistently within the same epoch (some calls hitting old implementation, others hitting new). Other config setters in the protocol require
isSystemIdle().🔎 Recommended fix
function setVaultBeacon(address newVaultBeacon) external onlyOwner { + if (!config.isSystemIdle()) revert ErrorsLib.SystemNotIdle(); if (newVaultBeacon == address(0)) revert ErrorsLib.ZeroAddress(); vaultBeacon = UpgradeableBeacon(newVaultBeacon); emit EventsLib.VaultBeaconUpdated(newVaultBeacon); }contracts/vaults/OrionTransparentVaultUpgradeable.sol (1)
89-98: uint16 cast issue persists from previous review.The cast
uint16(intent.length)at line 90 can silently truncate if the array exceeds 65,535 elements. While practically unlikely, this pattern is repeated ingetPortfolio(line 112),getIntent(line 131), andupdateVaultState(line 150). Usinguint256for loop variables and length is safer and has negligible gas impact.🔎 Proposed fix for all affected functions
uint256 totalWeight = 0; - uint16 intentLength = uint16(intent.length); - for (uint16 i = 0; i < intentLength; ++i) { + uint256 intentLength = intent.length; + for (uint256 i = 0; i < intentLength; ++i) {Apply similar changes to
getPortfolio,getIntent, andupdateVaultState.contracts/OrionConfigUpgradeable.sol (2)
290-292: Potential truncation castinglength()touint16.If the number of whitelisted assets exceeds 65,535, the cast will silently truncate. While unlikely, consider adding a bounds check or using
uint256.
230-237: Potential revert if orchestrators not yet set.If
pauseAllis called beforeinternalStatesOrchestratororliquidityOrchestratorare set, the calls will revert with an unhelpful error. Consider adding explicit zero-address checks.contracts/orchestrators/InternalStatesOrchestratorUpgradeable.sol (4)
362-367: Loop usesuint16for index but array length isuint256.If
allTransparent.lengthexceeds 65,535, the loop index would overflow. Consider usinguint256for the loop index.
192-201: Dependencies from config are not validated before use.Lines 193 and 201 read
priceAdapterRegistry()andliquidityOrchestrator()from config. If these haven't been set on the config contract yet,registryorliquidityOrchestratorcould beaddress(0), causing issues later. Consider adding validation.🔎 Proposed fix
config = IOrionConfig(config_); -registry = IPriceAdapterRegistry(config.priceAdapterRegistry()); +address registryAddr = config.priceAdapterRegistry(); +if (registryAddr == address(0)) revert ErrorsLib.ZeroAddress(); +registry = IPriceAdapterRegistry(registryAddr); // ... -liquidityOrchestrator = ILiquidityOrchestrator(config.liquidityOrchestrator()); +address loAddr = config.liquidityOrchestrator(); +if (loAddr == address(0)) revert ErrorsLib.ZeroAddress(); +liquidityOrchestrator = ILiquidityOrchestrator(loAddr);
516-546: Division by zero whenprotocolTotalAssetsis zero.If all vaults have zero assets,
protocolTotalAssetsremains 0, anddeltaBufferAmount.mulDiv(vaultAssets, protocolTotalAssets)at line 540 will revert. Add a guard to return early when TVL is zero.🔎 Proposed fix
function _buffer() internal { currentPhase = InternalUpkeepPhase.PostprocessingTransparentVaults; uint16 nTransparentVaults = uint16(transparentVaultsEpoch.length); uint256 protocolTotalAssets = 0; for (uint16 i = 0; i < nTransparentVaults; ++i) { address vault = transparentVaultsEpoch[i]; protocolTotalAssets += _currentEpoch.vaultsTotalAssets[address(vault)]; } + // Guard against division by zero when TVL is zero + if (protocolTotalAssets == 0) return; + uint256 targetBufferAmount = protocolTotalAssets.mulDiv(
739-745: Potential underflow inupdateBufferAmountwhen delta exceeds buffer.If
deltaAmountis negative and its absolute value exceedsbufferAmount, line 743 will underflow. While caller logic inLiquidityOrchestratormay protect against this, adding a defensive check provides safety.🔎 Proposed fix
function updateBufferAmount(int256 deltaAmount) external onlyLiquidityOrchestrator { if (deltaAmount > 0) { bufferAmount += uint256(deltaAmount); } else if (deltaAmount < 0) { + uint256 absAmount = uint256(-deltaAmount); + if (absAmount > bufferAmount) revert ErrorsLib.InsufficientAmount(); - bufferAmount -= uint256(-deltaAmount); + bufferAmount -= absAmount; } }contracts/vaults/OrionVaultUpgradeable.sol (1)
261-264: Potential underflow when updating_totalAssetsin redeem.Line 263 subtracts
assetsfrom_totalAssetswithout verifyingassets <= _totalAssets. WhilepreviewRedeemshould return a consistent value, adding a defensive check prevents potential accounting issues.🔎 Proposed fix
uint256 assets = previewRedeem(shares); // Update total assets accounting +if (assets > _totalAssets) revert ErrorsLib.InsufficientAmount(); _totalAssets -= assets;
🧹 Nitpick comments (9)
test/TransparentVault.test.ts (1)
30-88:beforeEachpositioned outsidedescribeblock affects test isolation.The
beforeEachhook (lines 30-88) is defined at module scope, outside anydescribeblock. This means it runs before every test in the file, including those in nesteddescribeblocks.While this works, it's unconventional and could cause confusion. Consider wrapping all tests in a top-level
describeblock to make the scope explicit:+describe("TransparentVault Tests", function () { beforeEach(async function () { // ... setup code }); describe("TransparentVault - Curator Pipeline", function () { // ... tests }); +});This is a minor structural concern and doesn't affect test correctness.
test/Adapters.test.ts (1)
24-26: Underscore-prefixed variables appear unused.The variables
_priceAdapterRegistryand_internalStatesOrchestratorare declared and assigned but never referenced in the test file. If they're intentionally kept for debugging or future use, this is fine. Otherwise, consider removing them to reduce noise.- let _priceAdapterRegistry: PriceAdapterRegistryUpgradeable; let liquidityOrchestrator: LiquidityOrchestratorUpgradeable; - let _internalStatesOrchestrator: InternalStatesOrchestratorUpgradeable;And update the assignment:
- _priceAdapterRegistry = deployed.priceAdapterRegistry; - _internalStatesOrchestrator = deployed.internalStatesOrchestrator;contracts/price/PriceAdapterRegistryUpgradeable.sol (1)
57-58: Consider:priceAdapterDecimalsis cached at initialization and could become stale.If
priceAdapterDecimalsis ever updated inOrionConfigafter this registry is initialized, this contract's cached value won't reflect the change. This may be intentional for consistency, but worth documenting or adding a setter if runtime updates are expected.test/orchestrator/OrchestratorConfiguration.test.ts (1)
66-84: Missing@openzeppelin/hardhat-upgradesimport.Unlike other test files (e.g.,
BatchLimitAccounting.test.ts,OrionConfigVault.test.ts), this file does not have theimport "@openzeppelin/hardhat-upgrades";side-effect import. While the tests may still run if the plugin is globally registered viahardhat.config.ts, adding the explicit import maintains consistency across test files and ensures the upgrades plugin is properly loaded.🔎 Proposed fix
import { SignerWithAddress } from "@nomicfoundation/hardhat-ethers/signers"; import { expect } from "chai"; import { ethers } from "hardhat"; +import "@openzeppelin/hardhat-upgrades"; import { time } from "@nomicfoundation/hardhat-network-helpers"; import { deployUpgradeableProtocol } from "../helpers/deployUpgradeable";test/orchestrator/OrchestratorSecurity.test.ts (1)
89-107: Missing@openzeppelin/hardhat-upgradesimport for consistency.Same as
OrchestratorConfiguration.test.ts, this file lacks the explicitimport "@openzeppelin/hardhat-upgrades";side-effect import that other test files include.🔎 Proposed fix
import { SignerWithAddress } from "@nomicfoundation/hardhat-ethers/signers"; import { expect } from "chai"; import { ethers } from "hardhat"; +import "@openzeppelin/hardhat-upgrades"; import { time } from "@nomicfoundation/hardhat-network-helpers"; import { deployUpgradeableProtocol } from "../helpers/deployUpgradeable";contracts/orchestrators/LiquidityOrchestratorUpgradeable.sol (3)
514-515: Storage gap placement is acceptable but verify slot count.The 50-slot gap is reasonable. However, verify that the total state variable slots plus the gap will remain consistent across upgrades. Consider documenting the current slot usage to prevent storage collisions in future upgrades.
327-338:PortfolioRebalancedevent emitted prematurely on each phase.The event is emitted after every
performUpkeepcall (selling, buying, or vault operations), but the portfolio is only fully rebalanced after the final phase. Consider emitting this event only in_processVaultOperationswhen the epoch completes (near line 441-442).🔎 Proposed fix
function performUpkeep(bytes calldata) external override onlyAuthorizedTrigger nonReentrant whenNotPaused { if (currentPhase == LiquidityUpkeepPhase.SellingLeg) { _processSellLeg(); } else if (currentPhase == LiquidityUpkeepPhase.BuyingLeg) { _processBuyLeg(); } else if (currentPhase == LiquidityUpkeepPhase.ProcessVaultOperations) { _processVaultOperations(); internalStatesOrchestrator.updateNextUpdateTime(); } - - emit EventsLib.PortfolioRebalanced(); }Then in
_processVaultOperations, after the epoch completes:if (i1 > transparentVaults.length || i1 == transparentVaults.length) { i1 = uint16(transparentVaults.length); currentPhase = LiquidityUpkeepPhase.Idle; currentMinibatchIndex = 0; emit EventsLib.EpochProcessed(epochCounter); + emit EventsLib.PortfolioRebalanced(); ++epochCounter; }
354-359: Loop index usesuint16but array length isuint256.While unlikely in practice, if
sellingTokens.lengthexceeds 65,535, the loop indexiwould overflow. Consider usinguint256for consistency and safety.-for (uint16 i = 0; i < sellingTokens.length; ++i) { +for (uint256 i = 0; i < sellingTokens.length; ++i) {contracts/vaults/OrionVaultUpgradeable.sol (1)
621-625: Loop index usesuint16butbatchSizeisuint256.If
batchSizeexceeds 65,535 (theoretically possible ifmaxFulfillBatchSizeis set very high), the loop index would overflow. Consider usinguint256for consistency.-for (uint16 i = 0; i < batchSize; ++i) { +for (uint256 i = 0; i < batchSize; ++i) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
contracts/OrionConfigUpgradeable.sol(1 hunks)contracts/factories/TransparentVaultFactoryUpgradeable.sol(1 hunks)contracts/libraries/EventsLib.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestratorUpgradeable.sol(1 hunks)contracts/orchestrators/LiquidityOrchestratorUpgradeable.sol(1 hunks)contracts/price/PriceAdapterRegistryUpgradeable.sol(1 hunks)contracts/vaults/OrionTransparentVaultUpgradeable.sol(1 hunks)contracts/vaults/OrionVaultUpgradeable.sol(1 hunks)test/Adapters.test.ts(2 hunks)test/BatchLimitAccounting.test.ts(3 hunks)test/BatchLimitConsistency.test.ts(1 hunks)test/FeeCooldown.test.ts(4 hunks)test/OrionConfigVault.test.ts(5 hunks)test/OrionVaultExchangeRate.test.ts(2 hunks)test/ProtocolPause.test.ts(5 hunks)test/TransparentVault.test.ts(8 hunks)test/helpers/deployUpgradeable.ts(1 hunks)test/mainnet-fork/erc4626VaultCompatibility.test.ts(10 hunks)test/mainnet-fork/multiAssetRobustness.test.ts(1 hunks)test/mainnet-fork/removeWhitelistedAsset.test.ts(5 hunks)test/orchestrator/OrchestratorConfiguration.test.ts(11 hunks)test/orchestrator/OrchestratorSecurity.test.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/mainnet-fork/multiAssetRobustness.test.ts
- test/mainnet-fork/removeWhitelistedAsset.test.ts
🧰 Additional context used
🧬 Code graph analysis (8)
test/OrionVaultExchangeRate.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-139)
test/TransparentVault.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-139)
test/BatchLimitConsistency.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-139)
test/BatchLimitAccounting.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-139)
test/orchestrator/OrchestratorConfiguration.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-139)
test/FeeCooldown.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-139)
test/OrionConfigVault.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-139)
test/ProtocolPause.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-139)
🪛 GitHub Actions: CI
contracts/orchestrators/LiquidityOrchestratorUpgradeable.sol
[error] 469-469: TypeError: Member "processLP" not found or not visible after argument-dependent lookup in contract IInternalStateOrchestrator. HH600: Compilation failed.
⏰ 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 (35)
test/OrionVaultExchangeRate.test.ts (2)
19-26: Verifyattackeras admin parameter is intentional.The
deployUpgradeableProtocolhelper receivesattackeras the second argument (admin), which grants admin privileges to this signer. While this may be intentional for test isolation, it differs from other test files that typically use a dedicated admin orothersigner.If the intent is to test scenarios where the attacker has no special privileges, consider using a separate signer for the admin role:
- const deployed = await deployUpgradeableProtocol(owner, attacker); + const [owner, curator, lp1, lp2, lp3, internalStatesOrchestratorSigner, liquidityOrchestratorSigner, attacker, admin] = + await ethers.getSigners(); + const deployed = await deployUpgradeableProtocol(owner, admin);
31-59: LGTM!The vault creation via factory with event parsing is correctly implemented. The pattern is consistent with other test files in this PR, and the error handling for missing events is appropriate.
test/BatchLimitConsistency.test.ts (4)
15-73: Well-documented test file with clear explanation of the critical bug fix.The documentation block thoroughly explains the accounting bug, its impact, the fix, and the test approach. The limitation section honestly addresses Hardhat signer constraints.
156-182: LGTM!The
effectiveBatchSizecalculation correctly handles the limited signer constraint while still validating the batch limiting logic. The test properly verifies thatpendingDepositreturns only the firsteffectiveBatchSizerequests and not all requests.
200-221: Consider potential test isolation issue with shared beforeEach.The
describe("2. pendingRedeem Batch Limit")block has its ownbeforeEachthat runs in addition to the outerbeforeEach. This deposits and fulfills for up to 30 users, but individual tests may expect different user counts.For test
"Should return exact shares when requests < maxFulfillBatchSize"(line 223), only 10 users are processed, but 30 users may have received shares from the innerbeforeEach. This could cause unexpected behavior if tests rely on specific user counts.Consider moving the share provisioning into individual tests or ensuring test expectations account for the full user set.
287-360: LGTM!The double-counting prevention tests correctly verify that
pendingDepositandpendingRedeemrespect batch limits, preventing the critical accounting bug wheretotalAssetswould be overcounted or double-subtracted across epochs.contracts/libraries/EventsLib.sol (1)
139-143: LGTM!The new
VaultBeaconUpdatedevent is correctly defined with appropriate NatSpec documentation and an indexed parameter for efficient filtering. This enables observability for beacon updates in the upgradeable vault factory.test/TransparentVault.test.ts (1)
115-118: LGTM!The contract retrieval is correctly updated to use the upgradeable vault type. The pattern is consistent across all test cases in this file.
test/Adapters.test.ts (2)
35-42: LGTM!The migration to
deployUpgradeableProtocolis correct. The helper receives the automation registry as the 4th parameter, and sinceunderlyingAssetisundefined, the helper will deploy a mock underlying asset internally.
44-56: LGTM!The comment on lines 48-49 clearly explains why a plain ERC20 is cast to
MockERC4626Asset— it's intentional for testing adapter rejection when non-ERC4626 tokens are used. This is good documentation for non-obvious test setup.test/FeeCooldown.test.ts (2)
31-53: LGTM! Clean migration to upgradeable protocol deployment.The fixture correctly uses
deployUpgradeableProtocolhelper and properly destructures the deployed components. The type annotations are consistent with the upgradeable contract types.
55-85: LGTM! Vault creation helper properly updated for upgradeable contracts.The
createVaultfunction correctly returnsOrionTransparentVaultUpgradeableand usesgetContractAtwith the upgradeable contract name.contracts/price/PriceAdapterRegistryUpgradeable.sol (1)
80-87: LGTM! Upgrade authorization and storage gap are correctly implemented.The
_authorizeUpgradefunction withonlyOwnermodifier follows the UUPS pattern correctly. The 50-slot storage gap is standard practice for upgradeable contracts.test/mainnet-fork/erc4626VaultCompatibility.test.ts (3)
317-382: LGTM! Comprehensive immutability verification for proxy-aware testing.The enhanced immutability checks properly verify:
- Bytecode consistency across blocks
- EIP-1967 implementation slot stability for proxies
- Cross-block determinism for
asset()anddecimals()This is a thorough approach for validating external ERC4626 vaults.
566-573: LGTM! Critical validation that adapter decimals match underlying asset.This assertion ensures price calculations remain consistent across the protocol by verifying the adapter returns decimals matching the underlying USDC decimals.
607-631: LGTM! Proper whitelisting workflow for execution adapter validation.The test now correctly whitelists vaults before validation (required for
OrionConfig.getTokenDecimalslookup) and cleans up afterward, preventing test pollution.contracts/factories/TransparentVaultFactoryUpgradeable.sol (2)
58-103: LGTM! Vault creation properly enforces authorization and system state checks.The
createVaultfunction correctly:
- Validates caller is whitelisted vault owner
- Ensures system is idle before deployment
- Encodes initialization data and deploys via BeaconProxy
- Registers vault in config and emits event
113-121: LGTM! Standard UUPS upgrade authorization and storage gap.The
_authorizeUpgradewithonlyOwnerand the 50-slot storage gap follow best practices for upgradeable contracts.test/ProtocolPause.test.ts (3)
45-58: LGTM! Clean imports for upgradeable protocol testing.The imports are properly organized with the deployment helper and upgradeable types. The past issue with imports inside the JSDoc block has been resolved.
85-96: LGTM! Proper migration to upgradeable protocol deployment.The test correctly uses
deployUpgradeableProtocolhelper and destructures the deployed components. This aligns with the pattern used across other test files in this PR.
136-168: LGTM! Vault creation and retrieval correctly use upgradeable types.The vault factory is obtained from the deployed protocol, and the vault is correctly attached using
OrionTransparentVaultUpgradeablecontract name.test/helpers/deployUpgradeable.ts (2)
45-139: Well-structured upgradeable deployment helper.The deployment sequence is correctly ordered (LiquidityOrchestrator before InternalStatesOrchestrator, config wiring before dependent deployments). The helper centralizes protocol deployment for tests, reducing duplication across test files.
Minor observation: the
as unknown as Typecasts are necessary due to ethers v6 and typechain typing quirks, but consider adding a brief inline comment explaining this pattern for future maintainers.
147-150: LGTM!Simple attach helper that correctly uses the contract factory pattern to connect to an existing vault proxy.
test/BatchLimitAccounting.test.ts (1)
27-81: Clean migration to upgradeable protocol deployment.The fixture correctly uses
deployUpgradeableProtocoland extracts components. The vault creation and retrieval pattern usingethers.getContractAt("OrionTransparentVaultUpgradeable", ...)is appropriate.One note: the impersonation pattern for
loSigner(lines 141-145) usinghardhat_setBalanceandgetImpersonatedSigneris correct for testing LiquidityOrchestrator calls.test/orchestrator/OrchestratorConfiguration.test.ts (1)
185-192: Correct upgradeable protocol deployment integration.The test correctly deploys the mock underlying asset first, then passes it to
deployUpgradeableProtocolalong with theautomationRegistrysigner. The destructured components are properly assigned to module-level variables.test/orchestrator/OrchestratorSecurity.test.ts (1)
208-215: Correct integration with upgradeable deployment helper.The test properly deploys the upgradeable protocol and extracts the required components for security testing.
test/OrionConfigVault.test.ts (2)
37-120: Clean migration to upgradeable protocol with proper test setup.The
beforeEachcorrectly usesdeployUpgradeableProtocoland sets up additional mock assets and adapters needed for testing. The vault creation and retrieval pattern is consistent with other migrated tests.
397-509: Comprehensive redeem request cancellation test coverage.Excellent addition of the
cancelRedeemRequesttest suite covering:
- Zero amount rejection
- Over-cancellation rejection
- Full cancellation with share return verification
- Partial cancellation with correct remaining balance
The test setup in
beforeEach(lines 398-424) properly creates shares via deposit fulfillment using LO impersonation. The assertions verify both the user's balance changes and pending redeem state.contracts/vaults/OrionTransparentVaultUpgradeable.sol (3)
36-38: Correct UUPS implementation pattern.The constructor properly disables initializers on the implementation contract, preventing direct initialization of the implementation and ensuring only proxies can be initialized.
199-221: Weight rebalancing on asset removal is well-designed.When a blacklisted asset is removed from the intent, its weight is correctly transferred to the underlying asset, maintaining the 100% total weight invariant. The logic handles both cases: when the underlying asset already exists in the intent (add to existing weight) and when it doesn't (set the blacklisted weight).
223-224: Storage gap correctly sized.The
__gap[50]provides adequate space for future storage variables while following OpenZeppelin's recommended upgrade-safe pattern.contracts/orchestrators/LiquidityOrchestratorUpgradeable.sol (1)
140-159: LGTM - Initialization is properly implemented.All three parameters (
initialOwner,config_,automationRegistry_) are now validated for zero addresses, and parent initializers are correctly chained. This addresses the previous review concern.contracts/OrionConfigUpgradeable.sol (1)
113-137: LGTM - Initialization properly validates all parameters.The
initialOwnerzero-address check has been added (line 114), addressing the previous review concern. The initialization correctly sets up defaults and whitelists the underlying asset.contracts/vaults/OrionVaultUpgradeable.sol (2)
659-696: LGTM - Batch processing correctly uses snapshot pattern.The
fulfillDepositfunction properly capturessnapshotTotalSupplybefore processing and collects all users/amounts first to avoid swap-and-pop reordering issues during EnumerableMap removal. This ensures consistent pricing for all users in the batch.
527-555: LGTM - Fee calculations properly handle edge cases.The fee calculation logic correctly handles zero fees (lines 529, 541), guards against division by zero (line 551), and applies epoch-based proration. The benchmark/divisor pattern properly supports all fee types.
2 participants
Summary by Sourcery
Introduce upgradeable (UUPS and beacon-based) versions of core Orion protocol contracts and refactor tests to use the new upgradeable deployment flow, while adding regression tests around batch limits and redeem cancellation.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.