Dev - #78
Conversation
Reviewer's GuideThis PR bolsters adapter and strategy validation across the protocol, refines orchestrator behavior, enriches error handling, and dramatically expands test coverage for orchestrators, vaults, adapters, and utilities. Sequence diagram for adapter validation during setExecutionAdapter in LiquidityOrchestratorsequenceDiagram
participant Owner
participant LiquidityOrchestrator
participant IExecutionAdapter
Owner->>LiquidityOrchestrator: setExecutionAdapter(asset, adapter)
LiquidityOrchestrator->>IExecutionAdapter: validateExecutionAdapter(asset)
IExecutionAdapter-->>LiquidityOrchestrator: returns true or reverts
LiquidityOrchestrator-->>Owner: ExecutionAdapterSet event or error
Sequence diagram for price adapter validation during setPriceAdapter in PriceAdapterRegistrysequenceDiagram
participant Owner
participant PriceAdapterRegistry
participant IPriceAdapter
Owner->>PriceAdapterRegistry: setPriceAdapter(asset, adapter)
PriceAdapterRegistry->>IPriceAdapter: validatePriceAdapter(asset)
IPriceAdapter-->>PriceAdapterRegistry: returns true or reverts
PriceAdapterRegistry-->>Owner: PriceAdapterSet event or error
Sequence diagram for strategy validation in OrionTransparentVaultsequenceDiagram
participant VaultOwner
participant OrionTransparentVault
participant IOrionStrategy
VaultOwner->>OrionTransparentVault: updateVaultWhitelist(assets)
OrionTransparentVault->>IOrionStrategy: validateStrategy(assets)
IOrionStrategy-->>OrionTransparentVault: returns or reverts
OrionTransparentVault-->>VaultOwner: update complete or error
Class diagram for new and updated adapter and strategy validation interfacesclassDiagram
class IExecutionAdapter {
+buy(asset, sharesAmount)
+sell(asset, sharesAmount)
+validateExecutionAdapter(asset)
}
class IPriceAdapter {
+getPriceData(asset)
+validatePriceAdapter(asset)
}
class IOrionStrategy {
+computeIntent(vaultWhitelistedAssets)
+validateStrategy(vaultWhitelistedAssets)
}
IExecutionAdapter <|.. OrionAssetERC4626ExecutionAdapter
IPriceAdapter <|.. OrionAssetERC4626PriceAdapter
IOrionStrategy <|.. KBestTvlWeightedAverage
OrionAssetERC4626ExecutionAdapter : +validateExecutionAdapter(asset)
OrionAssetERC4626PriceAdapter : +validatePriceAdapter(asset)
KBestTvlWeightedAverage : +validateStrategy(vaultWhitelistedAssets)
MockExecutionAdapter <|.. IExecutionAdapter
MockExecutionAdapter : +validateExecutionAdapter(asset)
MockPriceAdapter <|.. IPriceAdapter
MockPriceAdapter : +validatePriceAdapter(asset)
Class diagram for updated ErrorsLib error definitionsclassDiagram
class ErrorsLib {
<<library>>
+InvalidState()
+InvalidAdapter()
+SystemNotIdle()
+TransferFailed()
+InvalidCuratorContract()
+InvalidStrategy()
}
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 adds runtime validation functions for adapters and strategies, introduces three new error types, refactors vault whitelist handling and curator detection, updates orchestrator filtering and adapter/price registry validation, and extends tests and artifacts to reflect these interface and behavior changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant LO as LiquidityOrchestrator
participant Adapter as ExecutionAdapter
participant Registry as Registry
rect #e8f0ff
Note over LO,Adapter: setExecutionAdapter flow (NEW)
Caller->>LO: setExecutionAdapter(asset, adapter)
LO->>Adapter: validateExecutionAdapter(asset)
Adapter-->>LO: true / revert
alt validation true
LO->>Registry: register adapter
Registry-->>LO: success
LO-->>Caller: success
else validation fails
LO-->>Caller: revert InvalidAdapter
end
end
sequenceDiagram
participant User
participant Vault as OrionTransparentVault
participant Strategy as IOrionStrategy
User->>Vault: updateVaultWhitelist(assets)
Vault->>Vault: validate assets via config
Vault->>Vault: _updateCuratorType(assets)
alt curator supports IOrionStrategy
Vault->>Strategy: validateStrategy(assets)
Strategy-->>Vault: success / revert InvalidStrategy
alt success
Vault-->>User: VaultWhitelistUpdated event + success
else
Vault-->>User: revert InvalidStrategy
end
else
Vault-->>User: VaultWhitelistUpdated event + success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.github.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The orchestrator tests have a lot of duplicated phase-advancement and upkeep calls—consider extracting that logic into a helper function to reduce repetition and improve readability.
- In OrionTransparentVault.updateVaultWhitelist you clear and repopulate the set but don’t emit the VaultWhitelistUpdated event or call the base implementation—add the event emit (or call super) to keep event semantics consistent.
- The new UtilitiesLib.convertDecimals test only covers a basic scenario—add edge-case tests (e.g. equal decimals, max values, down-conversions) to ensure the converter handles all boundary conditions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The orchestrator tests have a lot of duplicated phase-advancement and upkeep calls—consider extracting that logic into a helper function to reduce repetition and improve readability.
- In OrionTransparentVault.updateVaultWhitelist you clear and repopulate the set but don’t emit the VaultWhitelistUpdated event or call the base implementation—add the event emit (or call super) to keep event semantics consistent.
- The new UtilitiesLib.convertDecimals test only covers a basic scenario—add edge-case tests (e.g. equal decimals, max values, down-conversions) to ensure the converter handles all boundary conditions.
## Individual Comments
### Comment 1
<location> `contracts/vaults/OrionTransparentVault.sol:165-174` </location>
<code_context>
+ /// @notice Override updateVaultWhitelist to validate strategy compatibility
+ /// @param assets The new whitelisted assets for the vault
+ function updateVaultWhitelist(address[] calldata assets) external override(OrionVault, IOrionVault) onlyVaultOwner {
+ // Clear existing whitelist
+ _vaultWhitelistedAssets.clear();
+
+ for (uint256 i = 0; i < assets.length; ++i) {
+ address token = assets[i];
+
+ if (!config.isWhitelisted(token)) revert ErrorsLib.TokenNotWhitelisted(token);
+
+ bool inserted = _vaultWhitelistedAssets.add(token);
+ if (!inserted) revert ErrorsLib.AlreadyRegistered();
+ }
+
+ if (_isPassiveCurator) {
+ IOrionStrategy(curator).validateStrategy(assets);
+ }
</code_context>
<issue_to_address>
**suggestion:** Consider emitting an event after updating the vault whitelist.
This will help track whitelist changes and make it easier to audit state transitions, given the function's potential to revert and its impact on vault state.
Suggested implementation:
```
/// @notice Emitted when the vault whitelist is updated
/// @param assets The new whitelisted assets for the vault
event VaultWhitelistUpdated(address[] assets);
/// @notice Override updateVaultWhitelist to validate strategy compatibility
/// @param assets The new whitelisted assets for the vault
```
```
function updateVaultWhitelist(address[] calldata assets) external override(OrionVault, IOrionVault) onlyVaultOwner {
// Clear existing whitelist
_vaultWhitelistedAssets.clear();
for (uint256 i = 0; i < assets.length; ++i) {
address token = assets[i];
if (!config.isWhitelisted(token)) revert ErrorsLib.TokenNotWhitelisted(token);
bool inserted = _vaultWhitelistedAssets.add(token);
if (!inserted) revert ErrorsLib.AlreadyRegistered();
}
if (_isPassiveCurator) {
IOrionStrategy(curator).validateStrategy(assets);
}
emit VaultWhitelistUpdated(assets);
```
</issue_to_address>
### Comment 2
<location> `contracts/strategies/KBestTvlWeightedAverage.sol:56-65` </location>
<code_context>
}
+ /// @inheritdoc IOrionStrategy
+ function validateStrategy(address[] calldata vaultWhitelistedAssets) external view {
+ uint8 n = uint8(vaultWhitelistedAssets.length);
+ address referenceUnderlyingAsset = address(0);
+
+ for (uint8 i = 0; i < n; ++i) {
+ address asset = vaultWhitelistedAssets[i];
+
+ // slither-disable-next-line unused-return
+ try IERC4626(asset).totalAssets() returns (uint256) {
+ // Asset is ERC4626 compliant, good.
+ } catch {
+ revert ErrorsLib.InvalidStrategy();
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using uint8 for asset count may limit vault size.
If the vault may contain more than 255 assets, use uint256 for the counter to avoid overflow issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/execution/OrionAssetERC4626ExecutionAdapter.sol (1)
76-103: Buy path can mint wrong share amount; usemintfor exact shares and refund dust
previewMint(shares) + deposit(assets)does not guaranteesharesAmountminted (rounding/fees). Transfer at Line 101 can fail or leave residual shares in the adapter. Usemint(shares)to guarantee exact shares, and optionally refund any over-collected assets.Apply this refactor:
@@ IERC4626 vault = IERC4626(vaultAsset); - - spentUnderlyingAmount = vault.previewMint(sharesAmount); - // Pull underlying assets from the caller - underlyingAssetToken.safeTransferFrom(msg.sender, address(this), spentUnderlyingAmount); - // Approve vault to spend underlying assets - underlyingAssetToken.forceApprove(vaultAsset, spentUnderlyingAmount); - // Deposit underlying assets to get vault shares - // slither-disable-next-line unused-return - vault.deposit(spentUnderlyingAmount, address(this)); - // Clean up approval - underlyingAssetToken.forceApprove(vaultAsset, 0); - // Push the received shares to the caller - bool success = vault.transfer(msg.sender, sharesAmount); - if (!success) revert ErrorsLib.TransferFailed(); + uint256 maxSpend = vault.previewMint(sharesAmount); + // Pull underlying from caller into adapter + underlyingAssetToken.safeTransferFrom(msg.sender, address(this), maxSpend); + // Approve and mint exact shares directly to caller + underlyingAssetToken.forceApprove(vaultAsset, maxSpend); + spentUnderlyingAmount = vault.mint(sharesAmount, msg.sender); + underlyingAssetToken.forceApprove(vaultAsset, 0); + // Refund dust if any (defensive; previewMint should round up) + if (spentUnderlyingAmount < maxSpend) { + unchecked { + underlyingAssetToken.safeTransfer(msg.sender, maxSpend - spentUnderlyingAmount); + } + }
🧹 Nitpick comments (7)
contracts/interfaces/IPriceAdapter.sol (1)
16-19: Clarify validation function return semantics.The documentation states "returns true if compatible, reverts otherwise", implying the function never returns false. However, typical validation patterns return boolean (true/false) rather than returning true or reverting. Consider either:
- Updating documentation to allow
returns bool(true/false) if implementations may return false- Changing the signature to not return anything if implementations should always revert on failure
Consistency with IExecutionAdapter.validateExecutionAdapter (lines 26-28) and IOrionStrategy.validateStrategy would improve clarity.
contracts/interfaces/IExecutionAdapter.sol (1)
26-29: Standardize validation function signatures across interfaces.The documentation states "returns true if compatible, reverts otherwise", which matches IPriceAdapter but creates inconsistency:
- IExecutionAdapter.validateExecutionAdapter: returns bool
- IPriceAdapter.validatePriceAdapter: returns bool
- IOrionStrategy.validateStrategy: returns void (no return value)
Consider standardizing the validation pattern across all three interfaces. If the intent is "return true or revert (never return false)", the bool return type may be misleading. If the intent is to return true/false, update the documentation.
contracts/interfaces/IOrionStrategy.sol (1)
24-28: Consider aligning validation pattern with adapter interfaces.The
validateStrategyfunction has no return value and just reverts on failure, whilevalidateExecutionAdapterandvalidatePriceAdapterreturn bool. This creates inconsistency in the validation framework:
- IOrionStrategy.validateStrategy: void (reverts on failure)
- IExecutionAdapter.validateExecutionAdapter: returns bool
- IPriceAdapter.validatePriceAdapter: returns bool
Consider either:
- Adding a bool return to validateStrategy for consistency
- Removing bool returns from adapter validations and using void (revert-only pattern)
A consistent pattern across all validation functions improves maintainability.
test/PassiveCuratorStrategy.test.ts (2)
257-266: Interface ID calc works; consider an explicit constant to avoid BigInt XOR noiseYour XOR approach is correct. For readability and to prevent accidental selector drift, consider asserting against a constant
IOrionStrategy.interfaceIdexported by the contract (or a hardcoded constant in test computed once).
493-577: Nice overflow regression test; add a rounding-edge case forbuy/mintConsider adding a test that forces
previewMintrounding to differ fromdeposit/mintsemantics to catch the buy-path issue fixed above (exact-shares vs deposit). Also add a test thatvalidateExecutionAdapter/validatePriceAdapterrevert when vault underlying mismatchesunderlyingAsset. I can draft these.contracts/mocks/MockExecutionAdapter.sol (1)
13-20: Mock buy/sell: OK for tests; tiny readability nitReturning a fixed amount and using unnamed params keeps the mock minimal. If you want slightly clearer intent, consider a named constant.
- function buy(address, uint256) external pure returns (uint256 executionUnderlyingAmount) { - executionUnderlyingAmount = 1e12; - } + uint256 private constant MOCK_EXECUTION_UNDERLYING = 1e12; + function buy(address, uint256) external pure returns (uint256 executionUnderlyingAmount) { + executionUnderlyingAmount = MOCK_EXECUTION_UNDERLYING; + } - function sell(address, uint256) external pure returns (uint256 executionUnderlyingAmount) { - executionUnderlyingAmount = 1e12; - } + function sell(address, uint256) external pure returns (uint256 executionUnderlyingAmount) { + executionUnderlyingAmount = MOCK_EXECUTION_UNDERLYING; + }contracts/vaults/OrionTransparentVault.sol (1)
159-161: Avoid external self‑calls; add internal accessor for whitelistCalling this.vaultWhitelist() performs an external call to self and slightly widens reentrancy surface. Prefer an internal view helper (in the base) that materializes the array from storage.
If adding an internal function in OrionVault isn’t feasible now, leave as is; it’s safe with your modifiers, just a minor gas/complexity nit.
Also applies to: 211-214
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(1 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(2 hunks)artifacts/contracts/interfaces/IExecutionAdapter.sol/IExecutionAdapter.json(1 hunks)artifacts/contracts/interfaces/IOrionStrategy.sol/IOrionStrategy.json(1 hunks)artifacts/contracts/interfaces/IPriceAdapter.sol/IPriceAdapter.json(1 hunks)artifacts/contracts/libraries/ErrorsLib.sol/ErrorsLib.json(3 hunks)artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json(3 hunks)artifacts/contracts/mocks/MockPriceAdapter.sol/MockPriceAdapter.json(1 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(2 hunks)artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json(2 hunks)artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json(2 hunks)artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json(2 hunks)artifacts/contracts/test/UtilitiesLibTest.sol/UtilitiesLibTest.json(1 hunks)contracts/execution/OrionAssetERC4626ExecutionAdapter.sol(1 hunks)contracts/interfaces/IExecutionAdapter.sol(1 hunks)contracts/interfaces/IOrionStrategy.sol(1 hunks)contracts/interfaces/IPriceAdapter.sol(1 hunks)contracts/libraries/ErrorsLib.sol(1 hunks)contracts/mocks/MockExecutionAdapter.sol(1 hunks)contracts/mocks/MockPriceAdapter.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(1 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(1 hunks)contracts/price/OrionAssetERC4626PriceAdapter.sol(1 hunks)contracts/price/PriceAdapterRegistry.sol(1 hunks)contracts/strategies/KBestTvlWeightedAverage.sol(2 hunks)contracts/test/UtilitiesLibTest.sol(1 hunks)contracts/vaults/OrionTransparentVault.sol(5 hunks)contracts/vaults/OrionVault.sol(2 hunks)test/Adapters.test.ts(2 hunks)test/Orchestrators.test.ts(6 hunks)test/OrchestratorsZeroState.test.ts(3 hunks)test/OrionConfigVault.test.ts(1 hunks)test/PassiveCuratorStrategy.test.ts(6 hunks)test/TransparentVault.test.ts(0 hunks)test/UtilitiesLib.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/TransparentVault.test.ts
🧰 Additional context used
🧬 Code graph analysis (6)
contracts/vaults/OrionVault.sol (1)
test/TransparentVault.test.ts (3)
newWhitelist(184-188)whitelist(282-297)owner(34-126)
test/OrionConfigVault.test.ts (1)
test/TransparentVault.test.ts (1)
owner(34-126)
test/Orchestrators.test.ts (1)
test/OrionVaultExchangeRate.test.ts (4)
loadFixture(408-454)it(407-455)it(248-351)it(126-188)
test/OrchestratorsZeroState.test.ts (2)
test/TransparentVault.test.ts (1)
it(300-346)test/OrionVaultExchangeRate.test.ts (1)
it(407-455)
test/PassiveCuratorStrategy.test.ts (1)
test/TransparentVault.test.ts (4)
tx(301-345)describe(128-347)it(300-346)whitelist(282-297)
contracts/vaults/OrionTransparentVault.sol (1)
test/TransparentVault.test.ts (4)
newWhitelist(184-188)whitelist(282-297)it(300-346)whitelist(237-257)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Build, Lint and Test
🔇 Additional comments (28)
contracts/libraries/ErrorsLib.sol (1)
59-72: LGTM! Well-documented error additions.The three new errors (
InvalidAdapter,InvalidCuratorContract,InvalidStrategy) are clearly documented and align with the validation framework introduced in this PR. The NatSpec comments provide sufficient context for each error's use case.contracts/orchestrators/InternalStatesOrchestrator.sol (1)
281-281: Verify the vault filtering logic change.The filtering condition has changed from checking
pendingDeposit() == 0 && pendingRedeem() == 0(per AI summary) topendingDeposit() + totalAssets() == 0. This significantly alters which vaults are included in each epoch:
- Old behavior: Skipped vaults with no pending deposits AND no pending redeems (no pending activity)
- New behavior: Skips vaults with no pending deposits AND no total assets (completely empty vaults)
Impact: A vault with existing assets but no pending activity (e.g.,
totalAssets = 1000,pendingDeposit = 0,pendingRedeem = 0) would previously be skipped but is now included in epoch processing.Please verify:
- This change is intentional and aligns with the desired vault selection behavior
- The performance impact of potentially processing more vaults per epoch is acceptable
- Edge cases are handled correctly (e.g., vaults with assets but no intent defined are already filtered at line 284)
contracts/price/PriceAdapterRegistry.sol (1)
47-48: LGTM! Good validation addition.The adapter validation check correctly enforces compatibility before assignment. Calling
adapter.validatePriceAdapter(asset)and reverting withInvalidAdapter()ensures that only compatible adapters can be registered for an asset.contracts/mocks/MockPriceAdapter.sol (1)
20-23: LGTM!Mock implementation appropriately returns true for all assets, enabling test scenarios without validation constraints. The
puremodifier is correct since no state is accessed.artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
53-57: LGTM!Artifact update correctly reflects the new
InvalidAdaptererror introduced in the contract. The ABI entry is properly formatted.artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json (1)
27-31: LGTM!Artifact update correctly reflects the new
InvalidAdaptererror introduced in PriceAdapterRegistry. The ABI entry is properly formatted.test/OrionConfigVault.test.ts (1)
257-283: LGTM!Comprehensive test coverage for
addWhitelistedVaultOwner:
- ✓ Success case with proper state validation
- ✓ Duplicate prevention with appropriate error
- ✓ Access control enforcement
Test structure is consistent with the existing test patterns in the file.
artifacts/contracts/interfaces/IOrionStrategy.sol/IOrionStrategy.json (1)
37-49: LGTM! Validation function added to strategy interface.The new
validateStrategyfunction extends the IOrionStrategy interface to support validation of vault whitelisted assets. This aligns with the broader validation framework introduced in this PR.contracts/vaults/OrionVault.sol (2)
59-59: LGTM! Visibility change enables derived contract access.Changing
_vaultWhitelistedAssetsfromprivatetointernalallows derived contracts likeOrionTransparentVaultto access the whitelist for validation purposes while maintaining appropriate encapsulation.
387-398: LGTM! Vault whitelist management with protocol validation.The
updateVaultWhitelistfunction correctly:
- Validates each asset against the protocol whitelist
- Prevents duplicates in the vault whitelist
- Allows derived contracts to override via
virtualmodifierThe clear-and-rebuild pattern is acceptable for this owner-only operation.
test/Adapters.test.ts (1)
5-86: LGTM! Enhanced test setup with orchestrators and registry.The expanded test setup properly deploys and wires the orchestrator components and price adapter registry, creating a more realistic test environment that mirrors production deployment patterns.
contracts/strategies/KBestTvlWeightedAverage.sol (1)
55-81: LGTM! Robust strategy validation with ERC4626 compatibility checks.The
validateStrategyfunction properly validates that:
- All assets implement the ERC4626 interface (
totalAssets()callable)- All assets share the same underlying asset (required for TVL-weighted allocation)
The defensive try-catch pattern correctly handles non-compliant tokens and reverts with
InvalidStrategy.test/UtilitiesLib.test.ts (1)
1-193: LGTM! Comprehensive test suite for decimal conversion.The test suite thoroughly validates
UtilitiesLib.convertDecimalsacross multiple scenarios:
- Scaling up/down between different decimal precisions
- Zero and large values
- Edge cases with single-decimal differences
- No-op conversions
The test structure is clear and well-organized.
artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (1)
27-31: LGTM! Artifact reflects strategy validation additions.The artifact correctly includes the new
InvalidStrategyerror andvalidateStrategyfunction in the ABI, consistent with the Solidity implementation.Also applies to: 199-212
contracts/test/UtilitiesLibTest.sol (1)
1-10: LGTM! Clean test wrapper for library functions.The
UtilitiesLibTestcontract provides a straightforward wrapper to exposeUtilitiesLib.convertDecimalsfor testing. This is a standard pattern for testing library functions.test/OrchestratorsZeroState.test.ts (3)
26-29: LGTM! Setup expanded to support deposit test scenarios.The addition of a
usersigner and minting of underlying assets enables testing of deposit-related edge cases in the orchestrator upkeep flow.Also applies to: 93-94
112-143: LGTM! Test validates orchestrator behavior with intent but no assets.This test correctly validates that the orchestrator completes upkeep but remains in Idle phase when a vault has a valid intent but no actual assets to process. This prevents unnecessary state transitions for inactive vaults.
145-172: LGTM! Test validates orchestrator behavior with deposits but no intent.This test correctly validates that the orchestrator completes upkeep but remains in Idle phase when a vault has pending deposits but no curator intent. This ensures deposits aren't processed without curator guidance on allocation.
contracts/execution/OrionAssetERC4626ExecutionAdapter.sol (2)
49-61: Validator looks good and defensiveCovers non-ERC4626 and asset mismatch via try/catch and consistent InvalidAdapter revert. No changes requested.
62-74: Review comment is incorrect; pre-approval is already implemented in the orchestratorThe review assumes
sell()lacks pre-approval on the vault shares, but the orchestrator explicitly pre-approves the adapter to spend shares before callingadapter.sell()(lines 410-414 in LiquidityOrchestrator._executeSell()). With this approval in place,vault.redeem(sharesAmount, msg.sender, msg.sender)succeeds as designed. The current implementation is correct and follows the same pre-approval pattern used forbuy().Likely an incorrect or invalid review comment.
test/PassiveCuratorStrategy.test.ts (1)
441-472: Great coverage on whitelist validation pathsValidates happy path, rejects non-ERC4626 asset, and non-strategy curator bypass. LGTM.
contracts/mocks/MockExecutionAdapter.sol (1)
22-25: validateExecutionAdapter: OK; confirm mutability compatibilityPure is fine for a mock; ensure the interface allows overriding with equal/more restrictive mutability (pure vs view/nonpayable).
Would you confirm IExecutionAdapter.validateExecutionAdapter is declared view (or pure), not payable? If needed, I can scan and report all declarations/overrides.
artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json (1)
32-58: ABI changes reflect code; ensure artifacts are compiler‑generatedABI shows buy/sell becoming pure and new validateExecutionAdapter(bool). Looks consistent with the Solidity changes. Just make sure these artifacts are generated by Hardhat (not hand‑edited) and aligned with the interface ABIs to avoid runtime mismatches.
If helpful I can script‑check all ABI signatures against interfaces and implementations.
Also applies to: 59-76, 79-81
contracts/vaults/OrionTransparentVault.sol (2)
163-181: Event parity on whitelist updatesIf the base implementation emitted an event on whitelist change, the override should do the same to keep off‑chain indexers in sync. Please confirm event emission parity.
If an event exists (e.g., VaultWhitelistUpdated), mirror it after successful updates.
178-181: The review comment is factually incorrect and should be dismissed.The
validateStrategyfunction signature inIOrionStrategy.sol(line 28) is:function validateStrategy(address[] calldata vaultWhitelistedAssets) external view;This returns void (no return type), not a boolean. The interface documentation explicitly states: "Should revert with appropriate error if validation fails." The implementation in
KBestTvlWeightedAverage.solfollows this contract—it usestry-catchwithrevert ErrorsLib.InvalidStrategy()on validation failure, not a false return value.The current code at lines 179 and 195 in
OrionTransparentVault.solis correct. It calls the function and allows reverting errors to propagate. The suggested diff would not compile since you cannot assignvoidtobool.Likely an incorrect or invalid review comment.
test/Orchestrators.test.ts (3)
467-470: Good coverage for partial redeem request flowApproving/requesting/cancelling half the amount exercises edge cases before full redeem. LGTM.
568-593: Epoch tokens and price asserts: good sanity checksEnsuring tokens are present and underlying price is 1 (scaled) strengthens invariants. LGTM.
895-900: Cross‑orchestrator automation registry update: solid verificationAsserting both event emission and state update on LiquidityOrchestrator in addition to InternalStatesOrchestrator is great. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json (1)
305-317: Removeindexedmodifier from array parameter.The
indexedmodifier on theassetsparameter (typeaddress[]) is ignored by Solidity—arrays cannot be indexed in events.Apply the same fix as the IOrionVault interface:
{ "anonymous": false, "inputs": [ { - "indexed": true, + "indexed": false, "internalType": "address[]", "name": "assets", "type": "address[]" } ], "name": "VaultWhitelistUpdated", "type": "event" },artifacts/contracts/vaults/OrionVault.sol/OrionVault.json (1)
531-543: Removeindexedmodifier from array parameter.Same issue: the
indexedmodifier onaddress[]type is ignored by Solidity.{ "anonymous": false, "inputs": [ { - "indexed": true, + "indexed": false, "internalType": "address[]", "name": "assets", "type": "address[]" } ], "name": "VaultWhitelistUpdated", "type": "event" },contracts/interfaces/IOrionVault.sol (1)
76-78: Removeindexedmodifier from array parameter.Arrays cannot be indexed in Solidity events. The
indexedmodifier onaddress[]is ignored by the compiler./// @notice The vault whitelist has been updated. /// @param assets The new whitelist of assets. - event VaultWhitelistUpdated(address[] indexed assets); + event VaultWhitelistUpdated(address[] assets);contracts/vaults/OrionTransparentVault.sol (1)
60-60: Validate strategy against vault whitelist, not global config.Passing
config.getAllWhitelistedAssets()during construction may trigger strategy validation against the wrong asset set before the vault's own whitelist is initialized via_initializeVaultWhitelist().Defer validation to after whitelist initialization:
- _updateCuratorType(config.getAllWhitelistedAssets()); + _updateCuratorType(new address[](0));Then ensure
updateVaultWhitelistor a post-initialization hook validates the strategy with the actual vault whitelist.test/Adapters.test.ts (1)
88-101: Test correctly validates adapter compatibility.The test properly verifies that attempting to whitelist an incompatible asset with an ERC4626 price adapter reverts with the expected InvalidAdapter error. The test description now correctly matches the expected error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(1 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(2 hunks)artifacts/contracts/interfaces/IExecutionAdapter.sol/IExecutionAdapter.json(1 hunks)artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json(1 hunks)artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json(1 hunks)artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json(3 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(1 hunks)artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json(3 hunks)artifacts/contracts/vaults/OrionVault.sol/OrionVault.json(1 hunks)contracts/execution/OrionAssetERC4626ExecutionAdapter.sol(1 hunks)contracts/interfaces/IExecutionAdapter.sol(1 hunks)contracts/interfaces/IOrionVault.sol(1 hunks)contracts/mocks/MockExecutionAdapter.sol(1 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(1 hunks)contracts/strategies/KBestTvlWeightedAverage.sol(7 hunks)contracts/vaults/OrionTransparentVault.sol(5 hunks)contracts/vaults/OrionVault.sol(1 hunks)test/Adapters.test.ts(2 hunks)test/Orchestrators.test.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json
- artifacts/contracts/OrionConfig.sol/OrionConfig.json
- contracts/orchestrators/LiquidityOrchestrator.sol
- artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json
- artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json
- artifacts/contracts/interfaces/IExecutionAdapter.sol/IExecutionAdapter.json
🧰 Additional context used
🧬 Code graph analysis (12)
contracts/interfaces/IOrionVault.sol (1)
test/TransparentVault.test.ts (2)
newWhitelist(184-188)whitelist(282-297)
contracts/interfaces/IExecutionAdapter.sol (1)
test/OrionConfigVault.test.ts (2)
assetAddress(208-216)assetAddress(197-206)
artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json (1)
test/TransparentVault.test.ts (3)
newWhitelist(184-188)whitelist(282-297)it(129-161)
artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json (1)
test/TransparentVault.test.ts (2)
newWhitelist(184-188)whitelist(282-297)
artifacts/contracts/vaults/OrionVault.sol/OrionVault.json (1)
test/TransparentVault.test.ts (2)
newWhitelist(184-188)whitelist(282-297)
test/Orchestrators.test.ts (2)
test/OrionConfigVault.test.ts (1)
it(472-527)test/OrionVaultExchangeRate.test.ts (3)
loadFixture(408-454)it(407-455)it(248-351)
contracts/mocks/MockExecutionAdapter.sol (2)
test/OrionConfigVault.test.ts (2)
assetAddress(197-206)assetAddress(208-216)test/TransparentVault.test.ts (1)
newWhitelist(184-188)
contracts/vaults/OrionVault.sol (2)
test/TransparentVault.test.ts (3)
newWhitelist(184-188)whitelist(282-297)owner(34-126)test/OrionConfigVault.test.ts (6)
assetAddress(160-166)it(159-233)assetAddress(185-195)assetAddress(226-232)assetAddress(208-216)initialCount(176-183)
contracts/vaults/OrionTransparentVault.sol (3)
test/TransparentVault.test.ts (4)
newWhitelist(184-188)whitelist(282-297)it(300-346)whitelist(237-257)test/OrionConfigVault.test.ts (4)
newCurator(549-557)newCurator(575-581)it(548-582)it(607-629)test/PassiveCuratorStrategy.test.ts (1)
vaultWhitelist(285-304)
test/Adapters.test.ts (1)
test/OrchestratorsZeroState.test.ts (1)
owner(27-95)
contracts/strategies/KBestTvlWeightedAverage.sol (1)
test/PassiveCuratorStrategy.test.ts (7)
strategy(412-427)it(284-357)strategy(340-356)strategy(322-338)strategy(271-274)it(411-428)_tokens(306-320)
artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (1)
test/PassiveCuratorStrategy.test.ts (4)
strategy(340-356)strategy(412-427)it(284-357)strategy(322-338)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Build, Lint and Test
🔇 Additional comments (16)
artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (2)
19-21: LGTM: k parameter type widened to support larger vaults.Changing
kfromuint8touint16allows the strategy to support vaults with up to 65,535 assets instead of 255, addressing the previous limitation noted in past reviews.Also applies to: 127-129, 190-192
200-212: LGTM: validateStrategy function added for compatibility checks.The new
validateStrategyfunction provides runtime validation to ensure assets are ERC4626-compliant with a common underlying asset, strengthening the strategy's robustness.contracts/vaults/OrionVault.sol (1)
59-59: LGTM: Visibility change enables derived contracts to manage whitelist.Changing
_vaultWhitelistedAssetsfromprivatetointernalallows derived contracts likeOrionTransparentVaultto access and manage the vault-specific whitelist while keeping it encapsulated from external callers.contracts/vaults/OrionTransparentVault.sol (2)
163-183: LGTM: Whitelist update logic is clear and well-structured.The function correctly clears the existing whitelist, validates each asset against the protocol config, and re-validates the strategy for passive curators. Event emission at Line 182 provides good observability.
179-179: Verify validateStrategy return handling.Lines 179 and 197 call
validateStrategy, which per the IOrionStrategy interface is declared asviewwith no return value (it reverts on failure). Confirm that the interface signature matches and that revert-based validation is the intended design.If
validateStrategyis supposed to return a boolean, update the calls to capture and check the result:bool valid = IOrionStrategy(curator).validateStrategy(assets); if (!valid) revert ErrorsLib.InvalidStrategy();Otherwise, document that validation failures are signaled via reverts.
Also applies to: 197-197
contracts/strategies/KBestTvlWeightedAverage.sol (2)
24-24: LGTM: Consistent type widening from uint8 to uint16.All occurrences of
kand related counters (n,kActual) have been consistently updated touint16, supporting vaults with up to 65,535 assets and addressing the previous limitation.Also applies to: 30-30, 41-41, 44-44, 89-89, 108-109, 138-138, 164-164
56-81: LGTM: validateStrategy ensures ERC4626 compliance and common underlying asset.The validation logic correctly:
- Checks each asset implements
totalAssets()(ERC4626 compliance)- Verifies all assets share the same underlying asset via
asset()- Reverts with
InvalidStrategyon any failureThis prevents misconfiguration and aligns with the strategy's TVL-based allocation logic.
contracts/interfaces/IExecutionAdapter.sol (1)
26-28: LGTM! Well-documented validation interface.The new validation method is properly documented and follows the revert-on-failure pattern, which is appropriate for compatibility checks.
contracts/execution/OrionAssetERC4626ExecutionAdapter.sol (1)
49-57: LGTM! Robust validation implementation.The validation properly handles both incompatible vault underlying assets and non-ERC4626 contracts via the try/catch pattern, with appropriate error handling.
contracts/mocks/MockExecutionAdapter.sol (1)
13-25: LGTM! Mock implementation appropriate for testing.The no-op validation and unnamed parameters are suitable for a mock adapter used in tests.
test/Adapters.test.ts (1)
5-86: LGTM! Test setup properly extended.The orchestrator deployments and configuration are correctly wired for integration testing of the adapter validation flow.
test/Orchestrators.test.ts (5)
467-472: LGTM! Partial redemption test flow is correct.The test properly validates the cancel redemption functionality by requesting, canceling, and then requesting again with appropriate approvals.
568-593: LGTM! Comprehensive token and price validation.The test thoroughly validates that epoch tokens are correctly tracked and prices are properly calculated, with special handling for the underlying asset's fixed price.
894-899: LGTM! Properly tests automation registry update.The test correctly validates that the LiquidityOrchestrator's automation registry can be updated and emits the expected event.
903-1010: LGTM! Comprehensive InvalidState protection tests.The security test suite thoroughly validates that the InternalStatesOrchestrator properly rejects out-of-order phase transitions, replay attacks, and invalid state manipulations.
1012-1288: LGTM! Thorough security test coverage.The LiquidityOrchestrator security test suite comprehensively validates state machine integrity, preventing unauthorized phase transitions, replay attacks, and cross-phase function calls.
| { | ||
| "anonymous": false, | ||
| "inputs": [ | ||
| { | ||
| "indexed": true, | ||
| "internalType": "address[]", | ||
| "name": "assets", | ||
| "type": "address[]" | ||
| } | ||
| ], | ||
| "name": "VaultWhitelistUpdated", | ||
| "type": "event" | ||
| }, |
There was a problem hiding this comment.
Remove indexed modifier from array parameter.
The indexed modifier on the assets parameter (type address[]) is ignored by Solidity—arrays cannot be indexed in events. This creates misleading ABI metadata and inconsistency across similar event definitions in the PR.
Remove the indexed modifier:
{
"anonymous": false,
"inputs": [
{
- "indexed": true,
+ "indexed": false,
"internalType": "address[]",
"name": "assets",
"type": "address[]"
}
],
"name": "VaultWhitelistUpdated",
"type": "event"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "anonymous": false, | |
| "inputs": [ | |
| { | |
| "indexed": true, | |
| "internalType": "address[]", | |
| "name": "assets", | |
| "type": "address[]" | |
| } | |
| ], | |
| "name": "VaultWhitelistUpdated", | |
| "type": "event" | |
| }, | |
| { | |
| "anonymous": false, | |
| "inputs": [ | |
| { | |
| "indexed": false, | |
| "internalType": "address[]", | |
| "name": "assets", | |
| "type": "address[]" | |
| } | |
| ], | |
| "name": "VaultWhitelistUpdated", | |
| "type": "event" | |
| }, |
🤖 Prompt for AI Agents
In artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json around lines
305 to 317, the event input for "assets" is incorrectly marked with "indexed":
true even though array types cannot be indexed in Solidity; remove the "indexed"
property (or set it to false/remove the key) from that input so the ABI
accurately reflects a non-indexed address[] parameter and matches other event
definitions.
| // Create price mismatch by simulating losses AFTER the oracle price call but BEFORE liquidity orchestrator execution | ||
| // This will cause the execution price to be lower than the oracle price, leading to decreasing buffer amount. | ||
|
|
||
| let [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | ||
| void expect(liquidityUpkeepNeeded).to.be.true; | ||
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | ||
| expect(await liquidityOrchestrator.currentPhase()).to.equal(2); // From Idle to BuyingLeg (no selling in this scenario) | ||
|
|
||
| // Simulate losses in mock assets to decrease their share prices | ||
| const lossAmount1 = ethers.parseUnits("5", 12); | ||
| await mockAsset1.connect(owner).simulateLosses(lossAmount1, owner.address); | ||
|
|
||
| const lossAmount2 = ethers.parseUnits("7", 12); | ||
| await mockAsset2.connect(owner).simulateLosses(lossAmount2, owner.address); | ||
|
|
||
| const lossAmount3 = ethers.parseUnits("10", 12); | ||
| await mockAsset3.connect(owner).simulateLosses(lossAmount3, owner.address); | ||
|
|
||
| // Continue liquidity orchestrator execution phases | ||
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | ||
| void expect(liquidityUpkeepNeeded).to.be.true; | ||
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | ||
|
|
||
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | ||
| void expect(liquidityUpkeepNeeded).to.be.true; | ||
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | ||
|
|
||
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | ||
| void expect(liquidityUpkeepNeeded).to.be.true; | ||
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | ||
|
|
||
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | ||
| void expect(liquidityUpkeepNeeded).to.be.true; | ||
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | ||
|
|
||
| expect(await liquidityOrchestrator.currentPhase()).to.equal(3); // FulfillDepositAndRedeem | ||
|
|
||
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | ||
| void expect(liquidityUpkeepNeeded).to.be.true; | ||
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | ||
| expect(await liquidityOrchestrator.currentPhase()).to.equal(0); // Idle | ||
|
|
||
| // Check that buffer amount has changed due to market impact | ||
| const finalBufferAmount = await internalStatesOrchestrator.bufferAmount(); | ||
| // The buffer amount should have changed due to market impact. | ||
| expect(finalBufferAmount).to.be.gt(initialBufferAmount); |
There was a problem hiding this comment.
Misleading comment about buffer behavior.
The comment at lines 729-731 states that losses will cause "execution price to be lower than the oracle price, leading to decreasing buffer amount," but line 774 expects finalBufferAmount > initialBufferAmount (an increase).
The test logic is actually correct: when losses occur after oracle pricing but before execution, the execution price is lower, so the protocol spends less underlying than anticipated, causing the buffer to increase. The comment should be updated to reflect this.
Apply this diff to fix the comment:
- // Create price mismatch by simulating losses AFTER the oracle price call but BEFORE liquidity orchestrator execution
- // This will cause the execution price to be lower than the oracle price, leading to decreasing buffer amount.
+ // Create price mismatch by simulating losses AFTER the oracle price call but BEFORE liquidity orchestrator execution
+ // This will cause the execution price to be lower than the oracle price, so the protocol spends less than anticipated, leading to increasing buffer amount.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create price mismatch by simulating losses AFTER the oracle price call but BEFORE liquidity orchestrator execution | |
| // This will cause the execution price to be lower than the oracle price, leading to decreasing buffer amount. | |
| let [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| expect(await liquidityOrchestrator.currentPhase()).to.equal(2); // From Idle to BuyingLeg (no selling in this scenario) | |
| // Simulate losses in mock assets to decrease their share prices | |
| const lossAmount1 = ethers.parseUnits("5", 12); | |
| await mockAsset1.connect(owner).simulateLosses(lossAmount1, owner.address); | |
| const lossAmount2 = ethers.parseUnits("7", 12); | |
| await mockAsset2.connect(owner).simulateLosses(lossAmount2, owner.address); | |
| const lossAmount3 = ethers.parseUnits("10", 12); | |
| await mockAsset3.connect(owner).simulateLosses(lossAmount3, owner.address); | |
| // Continue liquidity orchestrator execution phases | |
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| expect(await liquidityOrchestrator.currentPhase()).to.equal(3); // FulfillDepositAndRedeem | |
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| expect(await liquidityOrchestrator.currentPhase()).to.equal(0); // Idle | |
| // Check that buffer amount has changed due to market impact | |
| const finalBufferAmount = await internalStatesOrchestrator.bufferAmount(); | |
| // The buffer amount should have changed due to market impact. | |
| expect(finalBufferAmount).to.be.gt(initialBufferAmount); | |
| // Create price mismatch by simulating losses AFTER the oracle price call but BEFORE liquidity orchestrator execution | |
| // This will cause the execution price to be lower than the oracle price, so the protocol spends less than anticipated, leading to increasing buffer amount. | |
| let [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| expect(await liquidityOrchestrator.currentPhase()).to.equal(2); // From Idle to BuyingLeg (no selling in this scenario) | |
| // Simulate losses in mock assets to decrease their share prices | |
| const lossAmount1 = ethers.parseUnits("5", 12); | |
| await mockAsset1.connect(owner).simulateLosses(lossAmount1, owner.address); | |
| const lossAmount2 = ethers.parseUnits("7", 12); | |
| await mockAsset2.connect(owner).simulateLosses(lossAmount2, owner.address); | |
| const lossAmount3 = ethers.parseUnits("10", 12); | |
| await mockAsset3.connect(owner).simulateLosses(lossAmount3, owner.address); | |
| // Continue liquidity orchestrator execution phases | |
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| expect(await liquidityOrchestrator.currentPhase()).to.equal(3); // FulfillDepositAndRedeem | |
| [liquidityUpkeepNeeded, liquidityPerformData] = await liquidityOrchestrator.checkUpkeep("0x"); | |
| void expect(liquidityUpkeepNeeded).to.be.true; | |
| await liquidityOrchestrator.connect(automationRegistry).performUpkeep(liquidityPerformData); | |
| expect(await liquidityOrchestrator.currentPhase()).to.equal(0); // Idle | |
| // Check that buffer amount has changed due to market impact | |
| const finalBufferAmount = await internalStatesOrchestrator.bufferAmount(); | |
| // The buffer amount should have changed due to market impact. | |
| expect(finalBufferAmount).to.be.gt(initialBufferAmount); |
🤖 Prompt for AI Agents
In test/Orchestrators.test.ts around lines 729 to 774, update the misleading
comment that says losses will lead to a decreasing buffer amount — the test
actually expects the buffer to increase. Change the comment to explain that
simulating losses after the oracle price but before execution makes the
execution price lower than the oracle, so the protocol spends less underlying
than anticipated and the buffer amount increases; keep the test assertions
as-is.
1 participant
Summary by Sourcery
Enforce strategy and adapter compatibility, strengthen vault whitelist and orchestrator filtering logic, refactor core vault and orchestrator contracts for better validation, and greatly expand test coverage across orchestrators, strategies, adapters, and utilities.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests