Conversation
📝 WalkthroughWalkthroughReplaces epoch-fee boolean with incremental accrual and buffer-history state, adds cached asset/vault hashes for incremental commitments, exposes epoch buffer history, adapts minibatch sell/buy flows to use pending protocol fees and buffer history, and batches vault redeem burns. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/LiquidityOrchestrator.sol`:
- Around line 469-474: The commitment refresh can drift because the code reads
states.bufferAmount into bufferAmount but later hashes _epochBufferHistory (not
the live buffer) and can append duplicate checkpoints when currentMinibatchIndex
== 0; update the logic in LiquidityOrchestrator.sol (the block that uses
currentMinibatchIndex, bufferAmount, _epochBufferHistory and
_pendingEpochProtocolFees) to first checkpoint the live bufferAmount into
_epochBufferHistory before any early return (use the live states.bufferAmount
value), and dedupe checkpoint pushes by only pushing when the new buffer value
differs from the last entry; also ensure _pendingEpochProtocolFees is set from
states.epochProtocolFees immediately when currentMinibatchIndex == 0 so the
refreshed commitment binds to the live buffer and fees.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aaa60c0f-2e68-4442-b6d1-e22559be3ec0
📒 Files selected for processing (5)
contracts/LiquidityOrchestrator.solcontracts/interfaces/ILiquidityOrchestrator.solcontracts/interfaces/IOrionVault.solcontracts/vaults/OrionVault.solpackage.json
💤 Files with no reviewable changes (1)
- package.json
| if (currentMinibatchIndex == 0) { | ||
| // Update buffer amount | ||
| bufferAmount = states.bufferAmount; | ||
| // Accrue protocol fees once per epoch | ||
| pendingProtocolFees += states.epochProtocolFees; | ||
| emit EventsLib.ProtocolFeesAccrued(states.epochProtocolFees); | ||
| epochFeesAccrued = true; | ||
| _epochBufferHistory.push(bufferAmount); | ||
| _pendingEpochProtocolFees = states.epochProtocolFees; | ||
| } |
There was a problem hiding this comment.
Commitment refresh can drift from real buffer state after partial minibatch failures.
Line 724/Line 760 can return after partial execution updates bufferAmount, but the refreshed commitment (Line 725/Line 761) hashes _epochBufferHistory (Line 583), not the live bufferAmount, and the failure path does not checkpoint the latest buffer first. Also, Line 472 can append duplicate checkpoints on retries while currentMinibatchIndex stays 0.
🔧 Proposed fix (bind live buffer + dedupe checkpoints)
diff --git a/contracts/LiquidityOrchestrator.sol b/contracts/LiquidityOrchestrator.sol
@@
- config.decommissioningAssets(),
- _failedEpochTokens,
- _epochBufferHistory
+ config.decommissioningAssets(),
+ _failedEpochTokens,
+ _epochBufferHistory,
+ bufferAmount
)
);
@@
- _epochBufferHistory.push(bufferAmount);
+ _recordBufferCheckpoint();
@@
- _epochBufferHistory.push(bufferAmount);
+ _recordBufferCheckpoint();
@@
- _currentEpoch.epochStateCommitment = keccak256(
+ _recordBufferCheckpoint();
+ _currentEpoch.epochStateCommitment = keccak256(
abi.encode(_buildProtocolStateHash(), _cachedAssetsHash, _cachedVaultsHash)
);
@@
- _epochBufferHistory.push(bufferAmount);
+ _recordBufferCheckpoint();
@@
- _currentEpoch.epochStateCommitment = keccak256(
+ _recordBufferCheckpoint();
+ _currentEpoch.epochStateCommitment = keccak256(
abi.encode(_buildProtocolStateHash(), _cachedAssetsHash, _cachedVaultsHash)
);
@@
- _epochBufferHistory.push(bufferAmount);
+ _recordBufferCheckpoint();
+
+ function _recordBufferCheckpoint() internal {
+ uint256 n = _epochBufferHistory.length;
+ if (n == 0 || _epochBufferHistory[n - 1] != bufferAmount) {
+ _epochBufferHistory.push(bufferAmount);
+ }
+ }Also applies to: 511-512, 581-584, 724-727, 733-733, 760-763, 769-769
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/LiquidityOrchestrator.sol` around lines 469 - 474, The commitment
refresh can drift because the code reads states.bufferAmount into bufferAmount
but later hashes _epochBufferHistory (not the live buffer) and can append
duplicate checkpoints when currentMinibatchIndex == 0; update the logic in
LiquidityOrchestrator.sol (the block that uses currentMinibatchIndex,
bufferAmount, _epochBufferHistory and _pendingEpochProtocolFees) to first
checkpoint the live bufferAmount into _epochBufferHistory before any early
return (use the live states.bufferAmount value), and dedupe checkpoint pushes by
only pushing when the new buffer value differs from the last entry; also ensure
_pendingEpochProtocolFees is set from states.epochProtocolFees immediately when
currentMinibatchIndex == 0 so the refreshed commitment binds to the live buffer
and fees.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
contracts/LiquidityOrchestrator.sol (3)
732-732: 🧹 Nitpick | 🔵 TrivialConsider deduplicating buffer history entries.
If a minibatch fails after partial execution and is retried, buffer checkpoints could accumulate duplicate or near-duplicate values. While not functionally incorrect (zkVM will process whatever is committed), this could lead to unnecessary storage growth and potentially confusing market impact data.
♻️ Optional: Add deduplication helper
+ /// `@notice` Records buffer checkpoint, skipping if value unchanged + function _recordBufferCheckpoint() internal { + uint256 n = _epochBufferHistory.length; + if (n == 0 || _epochBufferHistory[n - 1] != bufferAmount) { + _epochBufferHistory.push(bufferAmount); + } + }Then replace
_epochBufferHistory.push(bufferAmount)calls with_recordBufferCheckpoint().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/LiquidityOrchestrator.sol` at line 732, Duplicate buffer checkpoints are being pushed into _epochBufferHistory; implement a helper function _recordBufferCheckpoint(uint256 bufferAmount) that checks the last entry of _epochBufferHistory and only pushes when the new bufferAmount differs (or differs by a small threshold) to avoid near-duplicates, then replace direct `_epochBufferHistory.push(bufferAmount)` calls with `_recordBufferCheckpoint(bufferAmount)` across the contract (keep function visibility/internal and gas-efficient).
723-732:⚠️ Potential issue | 🟠 MajorBuffer history not checkpointed before commitment refresh on partial failure.
When a minibatch partially succeeds (some sells execute, updating
bufferAmountvia_updateBufferAmountat line 826), but a later sell in the same minibatch fails, the code refreshes the commitment at lines 724-727 using_buildProtocolStateHash()which hashes_epochBufferHistory. However, the updatedbufferAmountfrom successful sells within this minibatch hasn't been pushed to_epochBufferHistoryyet (line 732 is skipped due to early return).This causes the commitment to bind to stale buffer history while the actual
bufferAmountstate has advanced.🔧 Proposed fix - checkpoint buffer before commitment refresh
} catch { _failedEpochTokens.push(token); + _epochBufferHistory.push(bufferAmount); // Refresh commitment from current state so next zkVM run matches on-chain input. _currentEpoch.epochStateCommitment = keccak256( abi.encode(_buildProtocolStateHash(), _cachedAssetsHash, _cachedVaultsHash) ); emit EventsLib.EpochStateCommitted(epochCounter, _currentEpoch.epochStateCommitment); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/LiquidityOrchestrator.sol` around lines 723 - 732, The early-return path refreshes _currentEpoch.epochStateCommitment using _buildProtocolStateHash() but can do so before the minibatch's bufferAmount (modified via _updateBufferAmount) is checkpointed into _epochBufferHistory, causing the commitment to reflect stale buffer history; modify the early-return branch to push the current bufferAmount into _epochBufferHistory (same action as the existing _epochBufferHistory.push(bufferAmount) at the normal exit) before computing and emitting the new epochStateCommitment so the commitment includes the latest buffer updates.
759-768:⚠️ Potential issue | 🟠 MajorSame buffer checkpoint issue in buy leg failure path.
Identical to the sell leg: partial minibatch success followed by failure doesn't checkpoint the updated
bufferAmountbefore refreshing the commitment.🔧 Proposed fix - checkpoint buffer before commitment refresh
} catch { _failedEpochTokens.push(token); + _epochBufferHistory.push(bufferAmount); // Refresh commitment from current state so next zkVM run matches on-chain input. _currentEpoch.epochStateCommitment = keccak256( abi.encode(_buildProtocolStateHash(), _cachedAssetsHash, _cachedVaultsHash) ); emit EventsLib.EpochStateCommitted(epochCounter, _currentEpoch.epochStateCommitment); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/LiquidityOrchestrator.sol` around lines 759 - 768, The buy-leg failure path fails to checkpoint the updated bufferAmount into _epochBufferHistory before recomputing and emitting the epoch commitment; fix by pushing bufferAmount into _epochBufferHistory immediately when a partial minibatch success is followed by a failure (i.e., before updating _currentEpoch.epochStateCommitment and calling emit EventsLib.EpochStateCommitted), ensuring the commitment and event reflect the buffered state; reference the buffer update site (_epochBufferHistory.push(bufferAmount)), the commitment calculation (_currentEpoch.epochStateCommitment = keccak256(abi.encode(_buildProtocolStateHash(), _cachedAssetsHash, _cachedVaultsHash))) and the event (EventsLib.EpochStateCommitted) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@contracts/LiquidityOrchestrator.sol`:
- Line 732: Duplicate buffer checkpoints are being pushed into
_epochBufferHistory; implement a helper function _recordBufferCheckpoint(uint256
bufferAmount) that checks the last entry of _epochBufferHistory and only pushes
when the new bufferAmount differs (or differs by a small threshold) to avoid
near-duplicates, then replace direct `_epochBufferHistory.push(bufferAmount)`
calls with `_recordBufferCheckpoint(bufferAmount)` across the contract (keep
function visibility/internal and gas-efficient).
- Around line 723-732: The early-return path refreshes
_currentEpoch.epochStateCommitment using _buildProtocolStateHash() but can do so
before the minibatch's bufferAmount (modified via _updateBufferAmount) is
checkpointed into _epochBufferHistory, causing the commitment to reflect stale
buffer history; modify the early-return branch to push the current bufferAmount
into _epochBufferHistory (same action as the existing
_epochBufferHistory.push(bufferAmount) at the normal exit) before computing and
emitting the new epochStateCommitment so the commitment includes the latest
buffer updates.
- Around line 759-768: The buy-leg failure path fails to checkpoint the updated
bufferAmount into _epochBufferHistory before recomputing and emitting the epoch
commitment; fix by pushing bufferAmount into _epochBufferHistory immediately
when a partial minibatch success is followed by a failure (i.e., before updating
_currentEpoch.epochStateCommitment and calling emit
EventsLib.EpochStateCommitted), ensuring the commitment and event reflect the
buffered state; reference the buffer update site
(_epochBufferHistory.push(bufferAmount)), the commitment calculation
(_currentEpoch.epochStateCommitment =
keccak256(abi.encode(_buildProtocolStateHash(), _cachedAssetsHash,
_cachedVaultsHash))) and the event (EventsLib.EpochStateCommitted) when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb89aedc-63a1-41cc-a8df-719abba01c24
📒 Files selected for processing (1)
contracts/LiquidityOrchestrator.sol
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores