fix: no more optimistic and double counting across epochs - #106
Conversation
…correct share price calculation and no protocol halts Test Coverage BatchLimitAccounting.test.ts validates: Returns full amount when < 150 requests Returns only first 150 when > 150 requests Handles varying amounts correctly Works with redeem requests Multi-epoch scenarios (no double-counting) Integration with fulfillDeposit()/fulfillRedeem() Edge cases (0, 1, 150, 151 requests)
🛡️ 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 GuideRefactored pendingDeposit and pendingRedeem to compute processable amounts/shares limited by MAX_FULFILL_BATCH_SIZE, added tests to validate batch limit behavior, and updated corresponding artifacts. Class diagram for updated OrionVault pendingDeposit and pendingRedeem methodsclassDiagram
class OrionVault {
+pendingDeposit() uint256
+pendingRedeem() uint256
-_depositRequests
-_redeemRequests
-MAX_FULFILL_BATCH_SIZE
}
OrionVault : pendingDeposit() now sums only first MAX_FULFILL_BATCH_SIZE deposit requests
OrionVault : pendingRedeem() now sums only first MAX_FULFILL_BATCH_SIZE redeem requests
OrionVault --> "*" _depositRequests
OrionVault --> "*" _redeemRequests
Flow diagram for batch-limited pendingDeposit and pendingRedeem calculationflowchart TD
A["pendingDeposit() called"] --> B["Get _depositRequests length"]
B --> C{"length == 0?"}
C -- Yes --> D["Return 0"]
C -- No --> E["Set batchSize = min(length, MAX_FULFILL_BATCH_SIZE)"]
E --> F["Sum amounts of first batchSize deposit requests"]
F --> G["Return processableAmount"]
H["pendingRedeem() called"] --> I["Get _redeemRequests length"]
I --> J{"length == 0?"}
J -- Yes --> K["Return 0"]
J -- No --> L["Set batchSize = min(length, MAX_FULFILL_BATCH_SIZE)"]
L --> M["Sum shares of first batchSize redeem requests"]
M --> N["Return processableShares"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughRemoved cached pending totals and per-vault constant; pendingDeposit/pendingRedeem now accept a fulfillBatchSize and compute sums from request queues up to that size. Fulfill functions use config.maxFulfillBatchSize() for batching. Tests and config/orchestrator call sites updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Orchestrator as Orchestrator
participant Config as OrionConfig
participant Vault as OrionVault
Note over Orchestrator,Config: Preprocessing / batch-size retrieval
Orchestrator->>Config: maxFulfillBatchSize()
Config-->>Orchestrator: batchSize
Note over Orchestrator,Vault: Query processable amounts (batch-aware)
Orchestrator->>Vault: pendingDeposit(batchSize)
Vault-->>Orchestrator: depositProcessableAmount
Orchestrator->>Vault: pendingRedeem(batchSize)
Vault-->>Orchestrator: redeemProcessableAmount
alt depositProcessableAmount > 0
Orchestrator->>Vault: fulfillDeposit(batchSize) %% uses config internally for loop limit
Vault-->>Orchestrator: sharesMinted / assetsMoved
end
alt redeemProcessableAmount > 0
Orchestrator->>Vault: fulfillRedeem(batchSize)
Vault-->>Orchestrator: assetsReturned / sharesBurned
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
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 - here's some feedback:
- Consider extracting the batching sum logic in pendingDeposit and pendingRedeem into a reusable internal helper to reduce code duplication.
- Review the gas impact of looping up to MAX_FULFILL_BATCH_SIZE in view functions, as on-chain iterations could become expensive at higher batch sizes.
- Ensure tests include scenarios where the request count is below, equal to, and exceeds MAX_FULFILL_BATCH_SIZE to validate correct boundary behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the batching sum logic in pendingDeposit and pendingRedeem into a reusable internal helper to reduce code duplication.
- Review the gas impact of looping up to MAX_FULFILL_BATCH_SIZE in view functions, as on-chain iterations could become expensive at higher batch sizes.
- Ensure tests include scenarios where the request count is below, equal to, and exceeds MAX_FULFILL_BATCH_SIZE to validate correct boundary behavior.
## Individual Comments
### Comment 1
<location> `test/BatchLimitAccounting.test.ts:28-130` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</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 (1)
test/BatchLimitAccounting.test.ts (1)
223-238: Clarify request aggregation comments and consider adding a >MAX_FULFILL_BATCH_SIZE coverage test
In the “Simulated batch limit behavior” block, comments conflict:
- Line 224 claims
requestDepositoverwrites the previous request per user,- Line 232 correctly notes it actually increments the amount for that address.
This is a bit confusing for readers; it would help to align these comments with the contract behavior (one aggregated request per address).The documentation section explains the 200‑request scenario and the 150‑request batch limit, but the suite never actually exercises
pendingDeposit/pendingRedeemwith more than 150 unique requesters. If practical, adding a targeted test that constructs >MAX_FULFILL_BATCH_SIZEdistinct LP addresses (e.g., via impersonation of pre‑funded accounts) would validate the fix in the exact edge case described in the docs.Neither of these are blockers, but they would make the tests more self‑consistent and strengthen confidence in the batch‑limit behavior.
Also applies to: 301-319
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/vaults/OrionVault.sol(1 hunks)test/BatchLimitAccounting.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/BatchLimitAccounting.test.ts (2)
test/orchestrator/Orchestrators.test.ts (3)
MIN_DEPOSIT(2542-2716)orionConfig(2642-2660)orionConfig(2662-2678)test/OrionVaultExchangeRate.test.ts (3)
loadFixture(408-454)loadFixture(458-483)it(353-405)
⏰ 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 (1)
test/BatchLimitAccounting.test.ts (1)
28-129: Fixture wiring and environment setup look solidThe deployment fixture cleanly wires the mock underlying asset, config, both orchestrators, vault factory, registry, and vault instance, then pre‑funds and approves all test users. This gives a realistic environment for the batch‑limit tests without unnecessary complexity. No issues from a correctness perspective.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/MinimumAmountDOS.test.ts (1)
445-445: Use the MAX_FULFILL_BATCH_SIZE variable instead of hardcoding.Line 445 hardcodes
150nin the calculation, but the test already retrieves the batch size dynamically on line 322. This creates inconsistency and the test will fail if the default config value changes.Apply this diff to use the variable:
- const capitalRequired = (MIN_DEPOSIT * 150n) / 10n ** 6n; // 150 = MAX_FULFILL_BATCH_SIZE + const capitalRequired = (MIN_DEPOSIT * MAX_FULFILL_BATCH_SIZE) / 10n ** 6n;Note: You'll need to ensure
MAX_FULFILL_BATCH_SIZEis accessible in this test's scope, or retrieve it again within this test case.
♻️ Duplicate comments (1)
contracts/vaults/OrionVault.sol (1)
576-591: pendingDeposit/pendingRedeem still don’t match the batch actually processed in fulfillDeposit/fulfillRedeem (and uint16 loop index is unsafe with configurable batch size)
pendingDeposit/pendingRedeemnow correctly cap their sums tobatchSize = min(length, fulfillBatchSize)and iterate viaat(i)over indices0 .. batchSize-1. However,fulfillDeposit/fulfillRedeemprocess batches usingat(0)on each iteration combined withremove(user), which internally uses swap‑and‑pop inEnumerableMap. Whenlength > batchSize, the actual set of processed entries in a fulfill call is{index 0, last, second‑last, ...}rather than the fixed prefix0 .. batchSize-1used inpending*. This means:
- The
depositTotalAssets/redeemTotalAssetsvalues computed off‑chain or by orchestrators viapending*can diverge from the amounts actually fulfilled in that epoch under high load (more thanfulfillBatchSizeunique requesters with heterogeneous sizes).- Share/asset accounting for the epoch can be incorrect, since
fulfill*usesdepositTotalAssets/redeemTotalAssetsto derive per-user share/asset conversions but then applies them to a different subset of requests than whatpending*summed.- This is the same structural issue that was flagged in the previous review and still needs to be addressed.
On top of this, all four loops use
uint16 iwhilebatchSizeisuint256. If eitherfulfillBatchSize(forpending*) orconfig.maxFulfillBatchSize()/ queue length (forfulfill*) ever exceedtype(uint16).max(65,535), the++iincrement will overflow and revert due to Solidity 0.8 checked arithmetic, effectively DoSing these paths for misconfigured batch sizes or very large queues.To make
pending*andfulfill*strictly consistent, and to remove the implicituint16limit, you can:
- Keep
pending*summingat(i)fori in [0, batchSize).- Change
fulfillDeposit/fulfillRedeemto:
- First snapshot the first
batchSizeentries viaat(i)into memory arrays of(user, amount/shares), and- Then iterate over that snapshot to process and
remove(user)by key.- Use
uint256as the loop index type everywhere.Illustrative diff (adapt as needed):
@@ - function pendingDeposit(uint256 fulfillBatchSize) external view returns (uint256) { - uint256 length = _depositRequests.length(); - if (length == 0) { - return 0; - } - - uint256 batchSize = Math.min(length, fulfillBatchSize); - uint256 processableAmount = 0; - - for (uint16 i = 0; i < batchSize; ++i) { - // slither-disable-next-line unused-return - (, uint256 amount) = _depositRequests.at(i); - processableAmount += amount; - } - - return processableAmount; - } + function pendingDeposit(uint256 fulfillBatchSize) external view returns (uint256) { + uint256 length = _depositRequests.length(); + if (length == 0) { + return 0; + } + + uint256 batchSize = Math.min(length, fulfillBatchSize); + uint256 processableAmount = 0; + + for (uint256 i = 0; i < batchSize; ++i) { + // slither-disable-next-line unused-return + (, uint256 amount) = _depositRequests.at(i); + processableAmount += amount; + } + + return processableAmount; + } @@ - function pendingRedeem(uint256 fulfillBatchSize) external view returns (uint256) { - uint256 length = _redeemRequests.length(); - if (length == 0) { - return 0; - } - - uint256 batchSize = Math.min(length, fulfillBatchSize); - uint256 processableShares = 0; - - for (uint16 i = 0; i < batchSize; ++i) { - // slither-disable-next-line unused-return - (, uint256 shares) = _redeemRequests.at(i); - processableShares += shares; - } - - return processableShares; - } + function pendingRedeem(uint256 fulfillBatchSize) external view returns (uint256) { + uint256 length = _redeemRequests.length(); + if (length == 0) { + return 0; + } + + uint256 batchSize = Math.min(length, fulfillBatchSize); + uint256 processableShares = 0; + + for (uint256 i = 0; i < batchSize; ++i) { + // slither-disable-next-line unused-return + (, uint256 shares) = _redeemRequests.at(i); + processableShares += shares; + } + + return processableShares; + } @@ function fulfillDeposit(uint256 depositTotalAssets) external onlyLiquidityOrchestrator nonReentrant { uint256 length = _depositRequests.length(); if (length == 0) { return; } uint256 batchSize = Math.min(length, config.maxFulfillBatchSize()); uint16 currentEpoch = internalStatesOrchestrator.epochCounter(); @@ - // Process requests in batch - uint256 processedAmount = 0; - for (uint16 i = 0; i < batchSize; ++i) { - // Get request by index (index 0 since we remove as we go) - (address user, uint256 amount) = _depositRequests.at(0); - - // slither-disable-next-line unused-return - _depositRequests.remove(user); + // Snapshot first `batchSize` requests so selection matches `pendingDeposit` + address[] memory users = new address[](batchSize); + uint256[] memory amounts = new uint256[](batchSize); + for (uint256 i = 0; i < batchSize; ++i) { + (users[i], amounts[i]) = _depositRequests.at(i); + } + + // Process requests in batch + uint256 processedAmount = 0; + for (uint256 i = 0; i < batchSize; ++i) { + address user = users[i]; + uint256 amount = amounts[i]; + + // slither-disable-next-line unused-return + _depositRequests.remove(user); @@ function fulfillRedeem(uint256 redeemTotalAssets) external onlyLiquidityOrchestrator nonReentrant { uint256 length = _redeemRequests.length(); if (length == 0) { return; } uint256 batchSize = Math.min(length, config.maxFulfillBatchSize()); uint16 currentEpoch = internalStatesOrchestrator.epochCounter(); @@ - // Process requests in batch - uint256 processedShares = 0; - for (uint16 i = 0; i < batchSize; ++i) { - // Get request by index (index 0 since we remove as we go) - (address user, uint256 shares) = _redeemRequests.at(0); - - // slither-disable-next-line unused-return - _redeemRequests.remove(user); + // Snapshot first `batchSize` requests so selection matches `pendingRedeem` + address[] memory users = new address[](batchSize); + uint256[] memory sharesList = new uint256[](batchSize); + for (uint256 i = 0; i < batchSize; ++i) { + (users[i], sharesList[i]) = _redeemRequests.at(i); + } + + // Process requests in batch + uint256 processedShares = 0; + for (uint256 i = 0; i < batchSize; ++i) { + address user = users[i]; + uint256 shares = sharesList[i]; + + // slither-disable-next-line unused-return + _redeemRequests.remove(user);This keeps:
- The set of requests selected by
pendingDeposit/pendingRedeemandfulfillDeposit/fulfillRedeemexactly aligned (the same indices0..batchSize-1).- The batch bounded by the same
fulfillBatchSize/config.maxFulfillBatchSize()parameter.- Loop indices as
uint256, removing the hidden assumption thatbatchSize <= 65,535.Also applies to: 595-610, 623-655, 658-693
🧹 Nitpick comments (3)
test/orchestrator/Orchestrators.test.ts (1)
1263-1264: Tests now correctly use config-driven fulfill batch sizeSwitching all
pendingDeposit/pendingRedeemcalls to passawait orionConfig.maxFulfillBatchSize()keeps the orchestrator tests aligned with the new batched accounting semantics, and the DOS test derivingMAX_FULFILL_BATCH_SIZEfrom config avoids hard-coding protocol constants.One minor coupling: the DOS scenario still asserts
capitalInUnits === 15000n, so changing the defaultmaxFulfillBatchSizewill intentionally break this test; if you expect that parameter to change often, consider expressing the expectation symbolically in terms ofMAX_FULFILL_BATCH_SIZEinstead of the numeric 15000.Also applies to: 1466-1473, 1519-1523, 1710-1715, 2647-2660, 2676-2678
contracts/OrionConfig.sol (1)
63-65: Batch-size config and pause controls are sound; consider a couple of guardsThe new
maxFulfillBatchSizeparameter (default 150) plussetMaxFulfillBatchSize()gating onisSystemIdle()andsize != 0fits the intended batching model, and the guardian-basedpauseAll/unpauseAllwiring into both orchestrators looks correct.Two optional hardening tweaks you might consider:
- In
pauseAll/unpauseAll, early-revert ifinternalStatesOrchestratororliquidityOrchestratoris still unset to avoid no-op calls against zero addresses.- If you expect very large
maxFulfillBatchSizevalues to be dangerous for gas, add a conservative upper bound (e.g., revert ifsizeexceeds a protocol constant), so misconfiguration can’t accidentally create an unfulfillable epoch.Also applies to: 112-113, 196-204, 206-235
test/BatchLimitAccounting.test.ts (1)
281-301: Strengthen the >maxFulfillBatchSize assertion for pendingDepositThe “documentation” test correctly demonstrates that with
maxFulfillBatchSizeset below the number of deposits,pendingDeposit(batchSize)should be less than the total queued deposits. To catch more subtle regressions, you could assert the exact expected amount:- const pendingDeposit = await vault.pendingDeposit(await config.maxFulfillBatchSize()); - - void expect(pendingDeposit).to.be.lessThan(DEPOSIT_AMOUNT * BigInt(numUsers)); + const batchSize = await config.maxFulfillBatchSize(); + const pendingDeposit = await vault.pendingDeposit(batchSize); + + const expectedProcessable = DEPOSIT_AMOUNT * batchSize; + void expect(pendingDeposit).to.equal(expectedProcessable);(This assumes each of the first
batchSizerequests is forDEPOSIT_AMOUNT, which matches how this test is constructed.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(4 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(1 hunks)artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json(2 hunks)artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json(2 hunks)artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json(2 hunks)artifacts/contracts/libraries/EventsLib.sol/EventsLib.json(2 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(1 hunks)artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json(1 hunks)artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json(1 hunks)artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json(1 hunks)artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json(1 hunks)artifacts/contracts/vaults/OrionVault.sol/OrionVault.json(2 hunks)contracts/OrionConfig.sol(3 hunks)contracts/interfaces/IOrionConfig.sol(1 hunks)contracts/interfaces/IOrionVault.sol(1 hunks)contracts/libraries/EventsLib.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(3 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(1 hunks)contracts/vaults/OrionVault.sol(3 hunks)test/BatchLimitAccounting.test.ts(1 hunks)test/MinimumAmountDOS.test.ts(1 hunks)test/OrionConfigVault.test.ts(4 hunks)test/ProtocolPause.test.ts(3 hunks)test/RedeemBeforeDepositOrder.test.ts(6 hunks)test/orchestrator/OrchestratorPerformUpkeep.test.ts(4 hunks)test/orchestrator/Orchestrators.test.ts(6 hunks)test/orchestrator/OrchestratorsZeroState.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json
- artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json
🧰 Additional context used
🧬 Code graph analysis (2)
artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json (1)
test/Adapters.test.ts (2)
OrionAssetERC4626ExecutionAdapterFactory(209-225)orionConfig(16-381)
test/orchestrator/Orchestrators.test.ts (1)
test/orchestrator/OrchestratorConfiguration.test.ts (1)
ABSOLUTE_VAULT_DEPOSIT(85-705)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build, Lint and Test
- GitHub Check: Sourcery review
🔇 Additional comments (23)
artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (1)
222-223: Verify bytecode changes align with source contract modifications.This is a compiled artifact file with updated bytecode and deployedBytecode. While bytecode changes are expected when the underlying contract changes, I cannot verify the correctness of raw bytecode hex strings. The ABI remains unchanged, which is good.
To ensure the new bytecode is correct, I need to review the actual source contract (
contracts/strategies/KBestTvlWeightedAverage.sol) to confirm it aligns with the PR's batching logic changes.Can you provide or point to the source contract file that was modified? This will help me verify that:
- The contract logic correctly implements the MaxFulfillBatchSize batching changes mentioned in the PR
- The compiled bytecode reflects those source changes accurately
artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json (1)
172-173: Build artifact—bytecode regeneration is expected; verify source code instead.This is a compiler-generated artifact file. The bytecode and deployedBytecode changes on lines 172–173 are expected outcomes when the underlying Solidity source changes. These hex strings cannot be meaningfully reviewed by humans.
Since the ABI (lines 5–171) remains unchanged, the public interface is stable. Verify the logic changes by reviewing the source Solidity file (
contracts/execution/OrionAssetERC4626ExecutionAdapter.sol) rather than this artifact. Ensure build artifacts are regenerated automatically during CI/CD rather than manually versioned in the repository if not already doing so.artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json (1)
110-111: Artifact bytecode change is justified and necessary.The
IOrionConfiginterface was modified to include themaxFulfillBatchSize()function as part of this PR. SinceOrionAssetERC4626PriceAdapterimports and depends onIOrionConfig, recompilation occurs automatically when the dependency changes, resulting in the updated bytecode. This is expected behavior—the artifact should remain in the PR.artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json (1)
329-341: LGTM! Configuration interface properly extended.The maxFulfillBatchSize getter and setMaxFulfillBatchSize setter follow standard patterns for configuration management in the ABI.
Also applies to: 498-510
artifacts/contracts/vaults/OrionVault.sol/OrionVault.json (1)
1246-1262: LGTM! ABI correctly reflects interface changes.The OrionVault ABI properly includes the new
fulfillBatchSizeparameter for bothpendingDepositandpendingRedeemfunctions.Also applies to: 1265-1281
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
889-890: LGTM! Bytecode updated as expected.The bytecode changes reflect the implementation updates to pass
maxFulfillBatchSize()when calling vault'spendingDepositandpendingRedeemmethods.contracts/libraries/EventsLib.sol (1)
40-42: LGTM! Event properly declared.The
MaxFulfillBatchSizeUpdatedevent follows the established pattern for configuration update events in EventsLib. The indexed parameter enables efficient event filtering.test/MinimumAmountDOS.test.ts (1)
322-323: LGTM! Correctly retrieves batch size from config.The test now dynamically retrieves the batch size from
config.maxFulfillBatchSize()instead of using a hardcoded constant, which properly aligns with the configurable batch size changes.test/RedeemBeforeDepositOrder.test.ts (1)
91-92: LGTM! All test calls properly updated.All calls to
vault.pendingDeposit()andvault.pendingRedeem()have been consistently updated to pass the batch size parameter retrieved fromorionConfig.maxFulfillBatchSize().Also applies to: 237-238, 368-369, 388-388, 401-402, 417-417
artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json (1)
801-817: LGTM! Interface ABI correctly updated.The IOrionVault interface artifact properly reflects the addition of the
fulfillBatchSizeparameter topendingDepositandpendingRedeemmethods.Also applies to: 820-836
test/ProtocolPause.test.ts (1)
324-442: Batch-size-aware pendingDeposit checks match the new vault APIUsing
await config.maxFulfillBatchSize()in allpendingDepositassertions keeps these pause/unpause integration tests aligned with the updated interface, and the single-request scenarios ensure the batch cap cannot hide pending work.test/OrionConfigVault.test.ts (2)
384-436: Deposit cancellation tests correctly use batch-limited pendingDepositThe updates to call
vault.pendingDeposit(await orionConfig.maxFulfillBatchSize())before and after cancellations are consistent with the new ABI and still accurately assert full/partial cancellation behavior for single-request cases.
507-526: Pending amounts tests aligned with maxFulfillBatchSize configurationSwitching
pendingDeposit()/pendingRedeem()to acceptawait orionConfig.maxFulfillBatchSize()keeps the “pending amounts” expectations correct under the batch-limited model and matches how orchestrators now query vaults.artifacts/contracts/libraries/EventsLib.sol/EventsLib.json (1)
77-89: MaxFulfillBatchSizeUpdated event ABI looks consistentThe added
MaxFulfillBatchSizeUpdated(uint256 maxFulfillBatchSize)event (indexed param) matches the new configuration knob and follows the pattern of other EventsLib config events; the corresponding bytecode updates are expected.Also applies to: 339-340
test/orchestrator/OrchestratorPerformUpkeep.test.ts (1)
1320-1802: performUpkeep flow tests correctly adopt batch-limited pending queuesUsing
await orionConfig.maxFulfillBatchSize()forpendingRedeem/pendingDepositin the long upkeep scenario keeps the expectations in sync with the new batch-limited accounting in both orchestrators, without changing the economic meaning of the tests (which never exceed one batch of requests).contracts/interfaces/IOrionConfig.sol (1)
210-217: IOrionConfig exposes maxFulfillBatchSize used across orchestratorsAdding
maxFulfillBatchSize()andsetMaxFulfillBatchSize(uint256 size)to the interface matches how InternalStatesOrchestrator, LiquidityOrchestrator, and tests are now consuming this config value.One thing to double-check in
OrionConfig.solis thatsetMaxFulfillBatchSize:
- rejects
size == 0, and- is gated by
isSystemIdle()like other critical config setters,so that all components see a stable, non-degenerate batch size during an epoch.
contracts/orchestrators/LiquidityOrchestrator.sol (1)
493-511: Vault fulfillment now respects config-driven maxFulfillBatchSizeFetching
uint256 maxFulfillBatchSize = config.maxFulfillBatchSize();and passing it intovaultContract.pendingRedeem/pendingDepositaligns fulfillment with the batch-limited accounting done in InternalStatesOrchestrator and removes the old “unbounded pending” assumption, without changing the fulfill logic itself.test/orchestrator/OrchestratorsZeroState.test.ts (1)
100-131: Zero-state orchestrator tests correctly use batch-size-aware pending queriesUsing
await orionConfig.maxFulfillBatchSize()in the zero-deposit/zero-intent checks keeps these guardrail tests compatible with the new vault API while still asserting that no spurious work is scheduled when there are no deposits/redeems.contracts/orchestrators/InternalStatesOrchestrator.sol (2)
298-307: Epoch vault filtering now uses batch-limited pendingDepositCaching
uint256 maxFulfillBatchSize = config.maxFulfillBatchSize();and using it inIOrionVault(v).pendingDeposit(maxFulfillBatchSize)keeps the epoch vault selection logic consistent with the new batch-cap semantics, while still correctly including any vault with nonzero TVL or in-batch pending deposits.
364-435: Preprocessing minibatch uses maxFulfillBatchSize consistently for accountingReading
maxFulfillBatchSizeonce per minibatch and passing it into bothvault.pendingRedeem(maxFulfillBatchSize)(beforeconvertToAssetsWithPITTotalAssets) andvault.pendingDeposit(maxFulfillBatchSize)ensures:
- Fee and buffer calculations only consider up to the configured number of requests per epoch.
- The same capped amounts underpin
vaultsTotalAssetsForFulfillRedeem/vaultsTotalAssetsForFulfillDeposit, keeping InternalStatesOrchestrator in lockstep with LiquidityOrchestrator and avoiding optimistic double-counting across epochs.The changes are local and don’t alter existing fee math or ordering logic.
artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json (1)
837-873: ABI update for batch-sized pending calls looks consistent*
pendingDeposit(uint256 fulfillBatchSize)andpendingRedeem(uint256 fulfillBatchSize)signatures (singleuint256input,uint256output,view) align with the new batch-size–aware logic used elsewhere; no issues from the artifact side as long as the Solidity interface and implementation match this ABI.artifacts/contracts/OrionConfig.sol/OrionConfig.json (1)
121-133: OrionConfig ABI correctly exposes maxFulfillBatchSize and its eventThe artifact cleanly reflects the new contract surface:
MaxFulfillBatchSizeUpdated(uint256 indexed maxFulfillBatchSize), themaxFulfillBatchSize()view, andsetMaxFulfillBatchSize(uint256 size)mutator. This ABI matches the Solidity implementation and supports the new batch-size–aware callers.Also applies to: 632-644, 854-866
test/BatchLimitAccounting.test.ts (1)
28-130: Fixture and core batch-limit tests look correct and aligned with protocol changesThe fixture wires USDC, OrionConfig, both orchestrators, vault factory, vault, and curator whitelist in a minimal yet realistic way, and the core tests for
pendingDeposit/pendingRedeemwith< maxFulfillBatchSizeand0-request scenarios correctly exercise the newfulfillBatchSizeparameter viaconfig.maxFulfillBatchSize(). ThependingRedeemtest that mints funding, routes it through the liquidity orchestrator, impersonates its address, and callsfulfillDepositis a good end-to-end sanity check that pending shares and total supply stay in sync with whatpendingRedeem(batchSize)reports.Also applies to: 132-209
| /// @param fulfillBatchSize The maximum number of requests to process per fulfill call | ||
| /// @return Total pending deposits denominated in underlying asset units (e.g., USDC, ETH) | ||
| /// @dev This returns asset amounts, not share amounts | ||
| function pendingDeposit() external view returns (uint256); | ||
| function pendingDeposit(uint256 fulfillBatchSize) external view returns (uint256); | ||
|
|
||
| /// @notice Get total pending redemption shares across all users | ||
| /// @param fulfillBatchSize The maximum number of requests to process per fulfill call | ||
| /// @return Total pending redemptions denominated in vault share units | ||
| /// @dev This returns share amounts, not underlying asset amounts | ||
| function pendingRedeem() external view returns (uint256); | ||
| function pendingRedeem(uint256 fulfillBatchSize) external view returns (uint256); |
There was a problem hiding this comment.
🧩 Analysis chain
Breaking API change - verify all callers updated.
The addition of the fulfillBatchSize parameter to pendingDeposit and pendingRedeem is a breaking change. The tests in this PR have been updated, but ensure all external integrations and off-chain components that call these functions are also updated.
🏁 Script executed:
#!/bin/bash
# Verify all calls to pendingDeposit and pendingRedeem pass the batch size parameter
# Search for calls to pendingDeposit without a parameter (potential missed updates)
echo "=== Checking for pendingDeposit calls ==="
rg -n 'pendingDeposit\s*\(\s*\)' --type solidity --type typescript
# Search for calls to pendingRedeem without a parameter (potential missed updates)
echo "=== Checking for pendingRedeem calls ==="
rg -n 'pendingRedeem\s*\(\s*\)' --type solidity --type typescriptLength of output: 148
🏁 Script executed:
#!/bin/bash
# Verify all calls to pendingDeposit and pendingRedeem pass the batch size parameter
# Using proper file patterns instead of invalid --type solidity
echo "=== Checking for pendingDeposit/pendingRedeem calls in .sol files ==="
rg -n 'pendingDeposit|pendingRedeem' -g '*.sol' -C 2
echo ""
echo "=== Checking for pendingDeposit/pendingRedeem calls in TypeScript files ==="
rg -n 'pendingDeposit|pendingRedeem' -g '*.ts' -g '*.tsx' -C 2Length of output: 33967
Update all callers of pendingDeposit and pendingRedeem to include the fulfillBatchSize parameter.
The breaking API change was not fully applied. Three calls in test/mainnet-fork/multiAssetRobustness.test.ts are missing the required parameter and will fail:
- Line 517:
vault.pendingDeposit()→ add parameter - Line 528:
vault.pendingDeposit()→ add parameter - Line 552:
vault.pendingRedeem()→ add parameter
Pass await config.maxFulfillBatchSize() (or equivalent) to each call to match the pattern used elsewhere in the codebase.
🤖 Prompt for AI Agents
In test/mainnet-fork/multiAssetRobustness.test.ts around lines 517, 528 and 552,
three calls to the updated vault API omit the required fulfillBatchSize
parameter; update each call to pass the configured batch size (e.g., replace
vault.pendingDeposit() at lines 517 and 528 with vault.pendingDeposit(await
config.maxFulfillBatchSize()) and replace vault.pendingRedeem() at line 552 with
vault.pendingRedeem(await config.maxFulfillBatchSize()), matching the pattern
used elsewhere in the codebase.
2 participants
Summary by Sourcery
Fix optimistic double counting by limiting pending deposit and redeem calculations to the first MAX_FULFILL_BATCH_SIZE requests per epoch and add tests for batch limit accounting
Bug Fixes:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
API
Tests