test: vault scenarios - #80
Merged
Merged
Conversation
Reviewer's GuideThis PR enhances the Orchestrators and Adapters test suites by introducing comprehensive vault scenarios covering multiple fee models, refactoring repeated upkeep logic into loops, cleaning up commented noise, and adding a new invalid adapter test. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. 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. WalkthroughTests updated: orchestrator deployments now pass an added automationRegistry address; Adapters tests add an InvalidAdapter revert case; Orchestrators tests expand a single vault scenario into five distinct vault instances with per-vault flows and phase-progression loops. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Factory
participant TestHarness
participant Orchestrator
participant Vault1 as AbsoluteVault
participant Vault2 as HighWaterMarkVault
participant Vault3 as SoftHurdleVault
participant Vault4 as HardHurdleVault
participant Vault5 as HurdleHWMVault
Note right of TestHarness #d5f5e3: Vault creation flow (per-vault)
TestHarness->>Factory: createVault(configX)
Factory-->>TestHarness: OrionVaultCreated(event)
TestHarness->>Vault1: instantiate(contractAddress)
Note right of TestHarness #fef3d9: Repeat for Vault2..Vault5
TestHarness->>Vault2: instantiate(contractAddress)
TestHarness->>Vault3: instantiate(contractAddress)
TestHarness->>Vault4: instantiate(contractAddress)
TestHarness->>Vault5: instantiate(contractAddress)
Note over TestHarness,Orchestrator #e8f0ff: Phase progression / upkeep loops
loop per-phase across vaults
TestHarness->>Orchestrator: performUpkeep()
Orchestrator-->>Vault1: progressPhase()
Orchestrator-->>Vault2: progressPhase()
Orchestrator-->>Vault3: progressPhase()
Orchestrator-->>Vault4: progressPhase()
Orchestrator-->>Vault5: progressPhase()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Rationale: Two test files updated with heterogeneous changes—one adding parameterized constructor usage and a new validation test, the other converting a single-vault flow into five coordinated vault flows with per-vault logic and phase-loop control — requiring moderate-to-deep review of test logic and orchestration correctness. Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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:
- Extract the repetitive vault creation and event‐parsing logic into a helper function to DRY up the test setup and improve readability.
- Parameterize the different fee models and intent scenarios so you can loop through vault types instead of duplicating nearly identical blocks of code.
- Consider splitting this monolithic end‐to‐end test into smaller, focused tests per vault behavior to make failures easier to diagnose and maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the repetitive vault creation and event‐parsing logic into a helper function to DRY up the test setup and improve readability.
- Parameterize the different fee models and intent scenarios so you can loop through vault types instead of duplicating nearly identical blocks of code.
- Consider splitting this monolithic end‐to‐end test into smaller, focused tests per vault behavior to make failures easier to diagnose and maintain.
## Individual Comments
### Comment 1
<location> `test/Orchestrators.test.ts:594-599` </location>
<code_context>
- await transparentVault.connect(user).cancelRedeemRequest(redeemAmount / 2n);
+ await hurdleHwmVault.connect(user).approve(await hurdleHwmVault.getAddress(), redeemAmount);
+ await hurdleHwmVault.connect(user).requestRedeem(redeemAmount);
+ await hurdleHwmVault.connect(user).cancelRedeemRequest(redeemAmount / 2n);
- await transparentVault.connect(user).approve(await transparentVault.getAddress(), redeemAmount);
- await transparentVault.connect(user).requestRedeem(redeemAmount);
+ // Get the updated balance after cancellation
+ const updatedRedeemAmount = await hurdleHwmVault.balanceOf(user.address);
+ await hurdleHwmVault.connect(user).approve(await hurdleHwmVault.getAddress(), updatedRedeemAmount);
+ await hurdleHwmVault.connect(user).requestRedeem(updatedRedeemAmount);
// Inject a lot of capital in asset tokens to increase their share price so in the next epoch there is a non-zero performance fee
</code_context>
<issue_to_address>
**suggestion (testing):** Missing assertions for redeem request cancellation effects.
Add assertions to confirm that cancelling the redeem request updates both the user's pending redeem amount and the vault's state as expected.
Suggested implementation:
```typescript
await hurdleHwmVault.connect(user).cancelRedeemRequest(redeemAmount / 2n);
// Assert that the user's pending redeem amount is updated correctly after cancellation
const pendingRedeemAfterCancel = await hurdleHwmVault.pendingRedeem(user.address);
expect(pendingRedeemAfterCancel).to.equal(redeemAmount - redeemAmount / 2n);
// Optionally, assert the vault's total pending redeem amount is updated
if (typeof hurdleHwmVault.totalPendingRedeem === "function") {
const totalPendingRedeem = await hurdleHwmVault.totalPendingRedeem();
// You may want to check the expected value here, e.g. with a snapshot or calculation
expect(totalPendingRedeem).to.be.gte(0);
}
// Get the updated balance after cancellation
const updatedRedeemAmount = await hurdleHwmVault.balanceOf(user.address);
```
- If `pendingRedeem` or `totalPendingRedeem` do not exist, replace with the correct method/property for your vault contract.
- Adjust the expected values in the assertions if your contract logic differs (e.g. if cancellation affects other state).
- If you want to assert more details about the vault's state, add further checks after cancellation.
</issue_to_address>
### Comment 2
<location> `test/Orchestrators.test.ts:601` </location>
<code_context>
+ await hurdleHwmVault.connect(user).approve(await hurdleHwmVault.getAddress(), updatedRedeemAmount);
+ await hurdleHwmVault.connect(user).requestRedeem(updatedRedeemAmount);
// Inject a lot of capital in asset tokens to increase their share price so in the next epoch there is a non-zero performance fee
const largeDepositAmount = ethers.parseUnits("1000000", 12); // 1M tokens
</code_context>
<issue_to_address>
**suggestion (testing):** No explicit assertion for performance fee accrual after share price increase.
Please add assertions to confirm that the performance fee is accrued and claimable after the share price increases.
Suggested implementation:
```typescript
// Inject a lot of capital in asset tokens to increase their share price so in the next epoch there is a non-zero performance fee
const largeDepositAmount = ethers.parseUnits("1000000", 12); // 1M tokens
await internalStatesOrchestrator.connect(automationRegistry).performUpkeep(performData);
expect(await internalStatesOrchestrator.currentPhase()).to.equal(1); // PreprocessingTransparentVaults
// Process all vaults in preprocessing phase - continue until we reach buffering phase
while ((await internalStatesOrchestrator.currentPhase()) === 1n) {
[_upkeepNeeded, performData] = await internalStatesOrchestrator.checkUpkeep("0x");
await internalStatesOrchestrator.connect(automationRegistry).performUpkeep(performData);
}
// ====== Assert performance fee accrual and claimability ======
// Get the accrued performance fee for the vault
const accruedPerformanceFee = await hurdleHwmVault.accruedPerformanceFee();
expect(accruedPerformanceFee).to.be.gt(0);
// Check that the performance fee is claimable (assuming claimablePerformanceFee() exists)
const claimablePerformanceFee = await hurdleHwmVault.claimablePerformanceFee();
expect(claimablePerformanceFee).to.be.gt(0);
```
- If your vault contract uses a different method name for accrued or claimable performance fees, replace `accruedPerformanceFee()` and `claimablePerformanceFee()` with the correct function names.
- If the performance fee is claimable by a specific address (e.g., feeRecipient), you may need to pass that address as a parameter.
- If you want to assert the actual claiming, you can add a call to the claim function and check the recipient's balance.
</issue_to_address>
### Comment 3
<location> `test/Orchestrators.test.ts:876-569` </location>
<code_context>
+ await mockAsset3.connect(owner).simulateLosses(lossAmount3, owner.address);
</code_context>
<issue_to_address>
**suggestion (testing):** No assertions for vault state after simulated asset losses.
Add assertions to confirm that vault share price, user balances, and fee calculations are updated correctly after asset losses.
Suggested implementation:
```typescript
await mockAsset3.connect(owner).simulateLosses(lossAmount3, owner.address);
// Assert vault share price is updated after asset losses
const updatedSharePrice = await vault3.sharePrice();
// Replace expectedSharePriceAfterLoss with the correct calculation for your vault
expect(updatedSharePrice).to.be.closeTo(expectedSharePriceAfterLoss, tolerance);
// Assert user balances are updated after asset losses
const userBalanceAfterLoss = await vault3.balanceOf(owner.address);
// Replace expectedUserBalanceAfterLoss with the correct calculation for your vault
expect(userBalanceAfterLoss).to.be.closeTo(expectedUserBalanceAfterLoss, tolerance);
// Assert fee calculations are correct after asset losses
const feesAfterLoss = await vault3.accruedFees();
// Replace expectedFeesAfterLoss with the correct calculation for your vault
expect(feesAfterLoss).to.be.closeTo(expectedFeesAfterLoss, tolerance);
// Continue liquidity orchestrator execution phases
```
You will need to:
- Define `expectedSharePriceAfterLoss`, `expectedUserBalanceAfterLoss`, `expectedFeesAfterLoss`, and `tolerance` based on your vault logic and test scenario.
- If your vault uses a different method for share price, user balance, or fees, adjust the function calls accordingly.
</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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
test/Orchestrators.test.ts (4)
31-35: Reduce per‑vault globals; prefer table‑driven testsDefining five separate OrionTransparentVault variables scales poorly. Use a data table (fee model, params, label) and iterate to deploy/verify each, reducing duplication and easing future additions.
154-156: Avoid assertions inside beforeEach; set a single initial targetbeforeEach currently both mutates state and asserts reverts (at Lines 144‑152) and then flips the ratio twice here. Move boundary assertions into a dedicated test and set a single canonical value in setup to keep tests isolated and predictable.
- await liquidityOrchestrator.setTargetBufferRatio(1); - await liquidityOrchestrator.setTargetBufferRatio(400); + // Keep setup deterministic; choose one default + await liquidityOrchestrator.setTargetBufferRatio(100);
181-200: DRY up vault creation and event parsingThe five blocks duplicate createVault + log parsing. Extract a helper to deploy and return the vault contract; call it with model/fees to keep tests concise.
+ async function createVault( + factory: TransparentVaultFactory, + curator: string, + name: string, + symbol: string, + model: number, + perfBps: number, + mgmtBps: number + ): Promise<OrionTransparentVault> { + const tx = await factory.createVault(curator, name, symbol, model, perfBps, mgmtBps); + const receipt = await tx.wait(); + const ev = receipt!.logs.find((log) => { + try { return factory.interface.parseLog(log)?.name === "OrionVaultCreated"; } catch { return false; } + })!; + const addr = factory.interface.parseLog(ev)!.args[0]; + return (await ethers.getContractAt("OrionTransparentVault", addr)) as unknown as OrionTransparentVault; + }Usage example:
- // Vault 1... - const absoluteVaultTx = await transparentVaultFactory.connect(owner).createVault(...); - ... - absoluteVault = (await ethers.getContractAt(...)) as OrionTransparentVault; + absoluteVault = await createVault(transparentVaultFactory.connect(owner), curator.address, "Absolute Fee Vault", "AFV", 0, 500, 50);Also applies to: 201-220, 221-240, 241-260, 261-280
281-380: Parametrize intents and depositsIntent construction + approve + requestDeposit repeats per vault. Create a small helper to submit intent and deposit, then iterate over a config array of {vault, intent, deposit}.
+ async function setupIntentAndDeposit(vault: OrionTransparentVault, intent: Array<{token:string; value: bigint}>, depositor: SignerWithAddress, amount: bigint) { + await vault.connect(curator).submitIntent(intent); + await underlyingAsset.connect(depositor).approve(await vault.getAddress(), amount); + await vault.connect(depositor).requestDeposit(amount); + }test/Adapters.test.ts (1)
95-115: Good negative test; add a positive control and tighten typesNice addition validating ERC20 × ERC4626 execution adapter mismatch. Consider:
- Add a positive test for an actual ERC4626 asset + ERC4626 execution adapter to ensure the happy path works alongside the revert.
- Optionally import the OrionAssetERC4626ExecutionAdapter type for stronger typings of erc4626ExecutionAdapter in assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/Adapters.test.ts(1 hunks)test/Orchestrators.test.ts(22 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/Orchestrators.test.ts (1)
test/TransparentVault.test.ts (3)
it(295-341)owner(34-126)it(129-161)
⏰ 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 (3)
test/Orchestrators.test.ts (3)
460-485: Idle‑only reverts on vault ops look goodTargeting a single vault for request/cancel deposit/redeem under non‑idle state tightens coverage without noise. LGTM.
584-587: Confirm rightful caller for curator fee claimsClaims are executed via owner. If the contract restricts claims to the curator or a specific role, this could mask an auth bug. Please confirm the intended caller and adjust to curator if required.
Also applies to: 688-691
893-896: Ignore the original review comment—it references non-existent code.The review comment claims there's a comment saying losses "lead to decreasing buffer amount," but this text does not appear in the file. The actual comment at lines 894-895 is generic: "The buffer amount should have changed due to market impact." The assertion correctly expects an increase (
finalBufferAmount > initialBufferAmount), which is consistent with the generic comment. No mismatch exists.Likely an incorrect or invalid review comment.
…tly after cancellation
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Extend orchestrator tests to cover multiple vault fee model scenarios and streamline phase progression, and add invalid adapter whitelisting test in the price adapter suite.
New Features:
Enhancements:
Tests:
Chores:
Summary by CodeRabbit
New Features
Improvements
Chores