Develop - #68
Conversation
|
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. Caution Review failedThe pull request is closed. WalkthroughAdds ignore entries; updates LiquidityOrchestrator to use ReentrancyGuard, minibatch-driven upkeep via encoded performData, and expose epoch order arrays; refactors InternalStatesOrchestrator loop handling and callback signature; modifies encrypted/transparent vault intent validation and decryption callback signatures; updates ABIs/artifacts and test to exercise the new upkeep flow; removes gas report. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Keeper as Chainlink Automation
participant LO as LiquidityOrchestrator
participant ISO as InternalStatesOrchestrator
participant Adapter as DEX Adapter(s)
Note over LO: New public state: selling/buying tokens & amounts,\nexecutionMinibatchSize, currentMinibatchIndex
Keeper->>LO: checkUpkeep(checkData)
activate LO
LO-->>Keeper: (upkeepNeeded, performData: action + minibatchIndex)
deactivate LO
alt upkeepNeeded
Keeper->>+LO: performUpkeep(performData) [nonReentrant]
alt action == start
LO->>ISO: getSellingOrders() / getBuyingOrders()
ISO-->>LO: orders
LO->>LO: _handleStart() -> load epoch orders, advance phase
else action == processSell
LO->>LO: _processMinibatchSell(minibatchIndex)
LO->>Adapter: _executeSell(...)
Adapter-->>LO: result
else action == processBuy
LO->>LO: _processMinibatchBuy(minibatchIndex)
LO->>Adapter: _executeBuy(...)
Adapter-->>LO: result
end
LO-->>-Keeper: done
else no upkeep
Note over Keeper,LO: no operation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Reviewer's GuideThis PR refactors LiquidityOrchestrator into an action-driven, reentrancy-protected Chainlink Automation executor with phased minibatch processing and new epoch state tracking; tightens loop performance and lint directives in InternalStatesOrchestrator; simplifies FHE intent validation in vault contracts; and updates ESLint ignore patterns. Sequence diagram for phased action-driven upkeep in LiquidityOrchestratorsequenceDiagram
participant AutomationRegistry
participant LiquidityOrchestrator
participant InternalStatesOrchestrator
AutomationRegistry->>LiquidityOrchestrator: checkUpkeep()
LiquidityOrchestrator->>InternalStatesOrchestrator: epochCounter()
InternalStatesOrchestrator-->>LiquidityOrchestrator: epoch value
AutomationRegistry->>LiquidityOrchestrator: performUpkeep(performData)
LiquidityOrchestrator->>LiquidityOrchestrator: decode performData
alt ACTION_START
LiquidityOrchestrator->>LiquidityOrchestrator: _handleStart()
LiquidityOrchestrator->>InternalStatesOrchestrator: getSellingOrders()
LiquidityOrchestrator->>InternalStatesOrchestrator: getBuyingOrders()
else ACTION_PROCESS_SELL
LiquidityOrchestrator->>LiquidityOrchestrator: _processMinibatchSell(minibatchIndex)
else ACTION_PROCESS_BUY
LiquidityOrchestrator->>LiquidityOrchestrator: _processMinibatchBuy(minibatchIndex)
end
ER diagram for new epoch state tracking in LiquidityOrchestratorerDiagram
LIQUIDITY_ORCHESTRATOR {
uint16 lastProcessedEpoch
uint8 executionMinibatchSize
uint8 currentMinibatchIndex
address[] sellingTokens
uint256[] sellingAmounts
address[] buyingTokens
uint256[] buyingAmounts
}
INTERNAL_STATES_ORCHESTRATOR {
getSellingOrders()
getBuyingOrders()
}
LIQUIDITY_ORCHESTRATOR ||--o| INTERNAL_STATES_ORCHESTRATOR : fetches orders
Class diagram for updated LiquidityOrchestrator structureclassDiagram
class LiquidityOrchestrator {
+Ownable
+ReentrancyGuard
+ILiquidityOrchestrator
uint16 lastProcessedEpoch
uint8 executionMinibatchSize
uint8 currentMinibatchIndex
LiquidityUpkeepPhase currentPhase
uint256 slippageBound
uint256 targetBufferRatio
address[] sellingTokens
uint256[] sellingAmounts
address[] buyingTokens
uint256[] buyingAmounts
transferRedemptionFunds(user, amount)
checkUpkeep(bytes)
performUpkeep(bytes)
_handleStart()
_processMinibatchSell(minibatchIndex)
_processMinibatchBuy(minibatchIndex)
}
LiquidityOrchestrator --|> Ownable
LiquidityOrchestrator --|> ReentrancyGuard
LiquidityOrchestrator --|> ILiquidityOrchestrator
Class diagram for updated OrionEncryptedVault intent validationclassDiagram
class OrionEncryptedVault {
+_validateIntent(assets, totalWeight)
}
Class diagram for updated OrionTransparentVault intent validationclassDiagram
class OrionTransparentVault {
+set(token, weight)
// Removed: revert if weight == 0
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.github.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `contracts/orchestrators/LiquidityOrchestrator.sol:255` </location>
<code_context>
- // solhint-disable-next-line code-complexity
+ /// @param performData Encoded data containing the action type and minibatch index
function performUpkeep(bytes calldata performData) external override onlyAutomationRegistry nonReentrant {
if (performData.length < 4) revert ErrorsLib.InvalidArguments();
</code_context>
<issue_to_address>
Input validation for performData length is added; consider stricter checks.
Since the expected encoding is (bytes4, uint8), update the check to performData.length < 5 to ensure proper validation and prevent decoding errors.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (performData.length < 4) revert ErrorsLib.InvalidArguments();
=======
if (performData.length < 5) revert ErrorsLib.InvalidArguments();
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/vaults/OrionEncryptedVault.sol (1)
172-189: Bug: new portfolio loop uses old keys length.Second loop indexes portfolio[] using portfolioLength taken from _portfolioKeys.length, leading to out-of-bounds reads or missed entries when lengths differ.
- // Update portfolio - for (uint16 i = 0; i < portfolioLength; ++i) { + // Update portfolio + uint16 newPortfolioLength = uint16(portfolio.length); + for (uint16 i = 0; i < newPortfolioLength; ++i) { _portfolio[portfolio[i].token] = portfolio[i].value; _portfolioKeys.push(portfolio[i].token); }contracts/orchestrators/LiquidityOrchestrator.sol (2)
364-378: Use SafeERC20 and reset allowance to zero first.Avoid approve race; some ERC20s require zeroing before increasing.
-import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; ... contract LiquidityOrchestrator is Ownable, ReentrancyGuard, ILiquidityOrchestrator { + using SafeERC20 for IERC20; ... - bool success = IERC20(asset).approve(address(adapter), amount); - if (!success) revert ErrorsLib.TransferFailed(); + IERC20 token = IERC20(asset); + token.safeApprove(address(adapter), 0); + token.safeApprove(address(adapter), amount);
384-398: Mirror SafeERC20 pattern for buys.- bool success = IERC20(underlyingAsset).approve(address(adapter), amount); - if (!success) revert ErrorsLib.TransferFailed(); + IERC20 ua = IERC20(underlyingAsset); + ua.safeApprove(address(adapter), 0); + ua.safeApprove(address(adapter), amount);
🧹 Nitpick comments (6)
eslint.config.mjs (1)
34-34: Consider ignoring reports as well.Optional: add "**/reports" to globalIgnores to avoid linting generated artifacts.
"**/fhevmTemp", "**/protocol-costs", + "**/reports",contracts/vaults/OrionEncryptedVault.sol (1)
125-138: Nit: typo in variable naming.Prefer “cipherTexts” over “cypherTexts” for consistency with crypto nomenclature.
- bytes32[] memory cypherTexts = new bytes32[](1); - cypherTexts[0] = FHE.toBytes32(isIntentEValid); + bytes32[] memory cipherTexts = new bytes32[](1); + cipherTexts[0] = FHE.toBytes32(isIntentEValid); ... - FHE.requestDecryption(cypherTexts, this.callbackDecryptSingleEbool.selector); + FHE.requestDecryption(cipherTexts, this.callbackDecryptSingleEbool.selector);contracts/orchestrators/LiquidityOrchestrator.sol (4)
58-66: Use uint16 for minibatch index if >255 items are possible.currentMinibatchIndex as uint8 risks wrap in large token sets. If not guaranteed <256, widen.
- uint8 public executionMinibatchSize; + uint8 public executionMinibatchSize; ... - uint8 public currentMinibatchIndex; + uint16 public currentMinibatchIndex;
79-90: Storing epoch arrays on-chain: consider sparse compaction.Optional: filter out zero-amount entries before storing to reduce gas in subsequent loops.
252-266: Decode guard is weak; tighten length check.abi.decode(bytes4,uint8) needs at least 68 bytes for dynamic encoding. Enforce a stricter bound or assert action matches known constants.
- if (performData.length < 4) revert ErrorsLib.InvalidArguments(); + if (performData.length < 68) revert ErrorsLib.InvalidArguments();
310-330: Start handler: filter zeros to shrink work.Drop entries with amount == 0 when populating arrays to avoid no-op execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks).prettierignore(1 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(6 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(9 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(6 hunks)contracts/vaults/OrionEncryptedVault.sol(2 hunks)contracts/vaults/OrionTransparentVault.sol(0 hunks)eslint.config.mjs(1 hunks)reports/gas-report.txt(0 hunks)
💤 Files with no reviewable changes (2)
- contracts/vaults/OrionTransparentVault.sol
- reports/gas-report.txt
🧰 Additional context used
🪛 GitHub Check: Build, Lint and Test
contracts/orchestrators/LiquidityOrchestrator.sol
[warning] 350-350:
Code contains empty blocks
[warning] 334-334:
Code contains empty blocks
⏰ 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 (17)
.gitignore (1)
22-23: Additive ignore entries look good.protocol-costs/ and reports/ are safely ignored. No conflicts with existing negation rules.
.prettierignore (1)
13-13: Prettier ignore is fine.Adding protocol-costs reduces noise in formatting runs.
contracts/vaults/OrionEncryptedVault.sol (2)
104-104: Intent validation change: confirm risk acceptance.You’ve dropped per-weight checks and now validate only totalWeight == target. Ensure downstream logic tolerates zero weights and does not rely on individual-weight constraints.
194-198: Callback access control relies on signature check — OK.FHE.checkSignatures gate is sufficient; no extra modifier needed.
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
75-79: Reentrancy error added — consistent with nonReentrant.ABI includes ReentrancyGuardReentrantCall; matches new ReentrancyGuard usage.
contracts/orchestrators/InternalStatesOrchestrator.sol (6)
241-241: OK to suppress code-complexity for orchestrator.Given the action dispatcher, the pragma is reasonable.
278-279: Doc param addition improves clarity.performData now documented; keep.
432-435: Static-analysis guards are appropriate.Solhint/slither suppressions scoped and justified.
Also applies to: 535-535
569-569: Comment tweak LGTM.“Skip if intent is invalid” clarifies flow.
579-579: Fee labeling clarified.“CURATOR + PROTOCOL REVENUE SHARE FEES” reads better.
631-633: Loop-bound locals reduce repeated SLOADs.Using nTransparentVaults/nEncryptedVaults is a small gas win; nice.
Also applies to: 636-642, 653-664
contracts/orchestrators/LiquidityOrchestrator.sol (6)
5-5: Reentrancy guard import is correct.
30-30: Inheritance update is appropriate.nonReentrant on performUpkeep matches ABI change.
73-78: Action selectors approach LGTM.
235-246: Upkeep gating logic looks sound.Starts only when internal epoch advanced; phases return actionable performData.
333-347: Empty body triggers automation churn; gate or revert for now.Until implemented, either advance the phase/index or revert to avoid repeated no-op upkeeps and static-analysis “empty block” warnings.
- function _processMinibatchSell(uint8 minibatchIndex) internal { - // TODO: implement. + function _processMinibatchSell(uint8 minibatchIndex) internal { + // TODO: implement. + revert ErrorsLib.InvalidState(); }
349-358: Same as sell path — avoid silent no-ops.- function _processMinibatchBuy(uint8 minibatchIndex) internal { - // TODO: implement. + function _processMinibatchBuy(uint8 minibatchIndex) internal { + // TODO: implement. + revert ErrorsLib.InvalidState(); }
|
Caution No docstrings were generated. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/orchestrators/InternalStatesOrchestrator.sol (3)
5-5: Fix import path for ReentrancyGuard (build will fail).OpenZeppelin’s ReentrancyGuard is under security/, not utils/.
-import "@openzeppelin/contracts/utils/ReentrancyGuard.sol"; +import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
329-349: Vault mappings cleared with token keys — leaves stale/incorrect state.In _handleStart(), vaultsTotalAssets and encryptedVaultsTotalAssets are erased using token addresses, not vault addresses. This can leave stale per‑vault values around and corrupt state.
for (uint16 i = 0; i < _currentEpoch.tokens.length; ++i) { address token = _currentEpoch.tokens[i]; delete _currentEpoch.priceArray[token]; delete _currentEpoch.initialBatchPortfolio[token]; - delete _currentEpoch.vaultsTotalAssets[token]; delete _currentEpoch.finalBatchPortfolio[token]; delete _currentEpoch.sellingOrders[token]; delete _currentEpoch.buyingOrders[token]; delete _currentEpoch.tokenExists[token]; _currentEpoch.encryptedInitialBatchPortfolio[token] = _ezero; - _currentEpoch.encryptedVaultsTotalAssets[token] = _ezero; _currentEpoch.encryptedFinalBatchPortfolio[token] = _ezero; } delete _currentEpoch.tokens; delete _decryptedValues; + +// Clear previous epoch per-vault mappings before overwriting epoch vault arrays +for (uint16 i = 0; i < transparentVaultsEpoch.length; ++i) { + delete _currentEpoch.vaultsTotalAssets[transparentVaultsEpoch[i]]; +} +for (uint16 i = 0; i < encryptedVaultsEpoch.length; ++i) { + _currentEpoch.encryptedVaultsTotalAssets[encryptedVaultsEpoch[i]] = _ezero; +} transparentVaultsEpoch = config.getAllOrionVaults(EventsLib.VaultType.Transparent); encryptedVaultsEpoch = config.getAllOrionVaults(EventsLib.VaultType.Encrypted); validEncryptedVaultsCount = 0;
650-671: mulDiv can revert on zero denominator in _buffer().If protocolTotalAssets == 0 (e.g., no vaults or all zero), vaultBufferCost = delta.mulDiv(..., protocolTotalAssets) will revert even when delta is 0. Guard before the per‑vault loops.
// Only increase buffer if current buffer is below target (conservative approach) if (bufferAmount > targetBufferAmount) return; -uint256 deltaBufferAmount = targetBufferAmount - bufferAmount; +if (protocolTotalAssets == 0) return; +uint256 deltaBufferAmount = targetBufferAmount - bufferAmount; +if (deltaBufferAmount == 0) return;
♻️ Duplicate comments (1)
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
75-79: ABI–source mismatch (AdapterNotSet) — re-verify artifacts.Earlier we flagged that LiquidityOrchestrator.sol reverts with ErrorsLib.AdapterNotSet but the ABI lacked that error. Reconfirm the source/artifact sync for this PR build and update artifacts if needed.
#!/bin/bash # Check whether source still references AdapterNotSet and whether ABI exposes it rg -n "AdapterNotSet" contracts/orchestrators/LiquidityOrchestrator.sol contracts/libraries/ErrorsLib.sol || true rg -n '"name":\s*"AdapterNotSet"' artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json || true
🧹 Nitpick comments (10)
package.json (1)
4-4: Version bump to 0.4.3 — add CHANGELOG/tag.Patch bump looks fine; remember to update release notes and tag on merge.
contracts/interfaces/IOrionEncryptedVault.sol (1)
53-59: Clarify encoding and enforce caller constraints in implementations.Switching to bytes for cleartexts/proof is fine, but the interface docs should spell out the expected encoding and security requirements (authorized caller, request binding).
Apply doc tweak:
- /// @param cleartexts The cleartexts - /// @param decryptionProof The decryption proof + /// @param cleartexts The oracle-delivered cleartexts payload (encoding MUST be documented: e.g., FHE plaintext serialization or ABI-encoded values). + /// @param decryptionProof The proof/attestation binding requestID to cleartexts. + /// @dev Implementations MUST restrict msg.sender to the authorized oracle/relayer and validate requestID binding before state changes.Please confirm the concrete implementation gates this with an onlyOracle/onlyRelayer check and validates requestID via FHE.checkSignatures or equivalent.
contracts/interfaces/IInternalStateOrchestrator.sol (1)
63-69: Bytes-based decrypt callback — document payload layout + auth.Good to move to bytes; please extend docs to define cleartexts/proof format and require authorized-caller validation in implementations.
- /// @param cleartexts The cleartexts - /// @param decryptionProof The decryption proof + /// @param cleartexts Oracle-delivered cleartexts payload (document encoding/layout). + /// @param decryptionProof Proof binding requestID↔cleartexts. + /// @dev Implementations MUST restrict caller to the oracle/relayer and validate requestID and payload lengths before use.Confirm InternalStatesOrchestrator enforces caller auth and checks requestID inside callbackPreProcessDecrypt.
artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json (1)
14-22: ABI-breaking change acknowledged — update clients and regenerate types.callbackPreProcessDecrypt now accepts raw bytes. Ensure off-chain producer/relayer encodes payloads accordingly and that tests cover malformed length/encoding.
I can generate a minimal test that feeds bad-length cleartexts/proof to assert reverts if you want.
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
165-183: Array element getters only — consider batched reads to cut RPC round-trips.buyingTokens/buyingAmounts and sellingTokens/sellingAmounts expose per-index getters. Off-chain reads will require N calls. Add view helpers that return the full arrays for a given epoch/minibatch to reduce I/O.
Also applies to: 185-202, 388-425
test/Orchestrators.test.ts (1)
304-354: End-to-end upkeep flow LGTM; add one assertion and drop superfluous void casts.
- After returning to Idle, also assert liquidityOrchestrator.checkUpkeep returns true to link both orchestrators in this “with decryption” path (you already do it in the “without decryption” test).
- Replace void expect(...) with plain expect(...) — no awaits here.
contracts/orchestrators/InternalStatesOrchestrator.sol (4)
280-283: Tighten performData length validation to avoid ambiguity.performData is always (bytes4,uint8) → 5 bytes. Enforce exact length.
-if (performData.length < 5) revert ErrorsLib.InvalidArguments(); +if (performData.length != 5) revert ErrorsLib.InvalidArguments();
361-367: Use >= instead of (a > b || a == b).Equivalent and clearer; apply in all three places.
-if (i1 > transparentVaultsEpoch.length || i1 == transparentVaultsEpoch.length) { +if (i1 >= transparentVaultsEpoch.length) {-if (i1 > nVaults || i1 == nVaults) { +if (i1 >= nVaults) {Also applies to: 686-691, 738-747
445-451: Prefer uint256 loop/index types to avoid silent truncation.Indices and counters are uint16/uint8. If vault/token counts ever exceed 65,535, arithmetic overflows. Using uint256 avoids this class of bugs with negligible gas impact on modern Solidity.
Also applies to: 637-639, 641-648, 659-670, 771-780
505-531: Build cipherTexts only for valid intents to cut costs.You allocate for all nVaults but skip invalid ones in processing. Consider packing only validEncryptedVaultsCount to reduce calldata to the FHE backend and decryption costs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(1 hunks)artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json(1 hunks)artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json(1 hunks)artifacts/contracts/libraries/UtilitiesLib.sol/UtilitiesLib.json(1 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(6 hunks)artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json(1 hunks)contracts/interfaces/IInternalStateOrchestrator.sol(1 hunks)contracts/interfaces/IOrionEncryptedVault.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(9 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(8 hunks)contracts/vaults/OrionEncryptedVault.sol(4 hunks)package.json(4 hunks)test/Orchestrators.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/orchestrators/LiquidityOrchestrator.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build, Lint and Test
🔇 Additional comments (10)
artifacts/contracts/libraries/UtilitiesLib.sol/UtilitiesLib.json (1)
6-7: Bytecode-only delta — artifacts missing compiler/metadata; confirm deterministic builds & pin solcArtifact artifacts/contracts/libraries/UtilitiesLib.sol/UtilitiesLib.json shows "compiler" and "metadata" === null, so the bytecode/deployedBytecode delta cannot be verified as a metadata/hash-only change. hardhat.config.ts pins solidity v0.8.28 with optimizer enabled (runs:10) and evmVersion: "cancun".
- Regenerate artifacts (npx hardhat compile) and provide jq '.compiler, .metadata' output for the artifact to confirm the delta is only metadata/hash.
- If it is metadata-only, accept; otherwise align compiler/settings or lock the toolchain in CI (pin solc 0.8.28) to prevent artifact churn.
package.json (2)
35-37: FHE toolchain upgrades — verify plugin/config breaking changes
- hardhat.config.ts imports "@fhevm/hardhat-plugin" (line 1); hardhat.slither.config.ts has the import commented out.
- No occurrences of @fhevm/mock-utils or @zama-fhe/relayer-sdk found in tests; "scripts" directory was missing so that path wasn't searched.
- Public release notes show no documented breaking changes between the 0.0.x series and 0.1.0.
Action: run the repo's test/CI locally on a branch with the upgraded deps and compare the plugin changelog/commits; if tests fail inspect Hardhat config and any hre.fhevm usages for API changes.
75-75: @fhevm/solidity → ^0.8.0 — confirm API compatibility.package.json (line 75) pins @fhevm/solidity ^0.8.0. Repo consistently imports/uses euint128/externalEuint128 and FHE helpers (fromExternal, allowThis, asEuint128, add/div/mul); FHE.checkSignatures(requestID, cleartexts, decryptionProof) is invoked at contracts/vaults/OrionEncryptedVault.sol:200 and contracts/orchestrators/InternalStatesOrchestrator.sol:542. Compile the project and run tests against @fhevm/solidity ^0.8.0 to confirm there are no breaking API changes or renamed helpers.
artifacts/contracts/OrionConfig.sol/OrionConfig.json (1)
605-606: Bytecode-only change — ABI NOT VERIFIEDVerification script failed with: "/bin/bash: -c: line 4: conditional binary operator expected" — unable to confirm ABI equality. Run:
git show HEAD~1:artifacts/contracts/OrionConfig.sol/OrionConfig.json | jq -c '.abi' > /tmp/prev_abi.json && jq -c '.abi' artifacts/contracts/OrionConfig.sol/OrionConfig.json > /tmp/curr_abi.json && diff -u /tmp/prev_abi.json /tmp/curr_abi.json
If ABI is unchanged, treat this as a non-breaking deployment; if proxies are used, validate storage/layout compatibility.artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json (1)
226-227: Runtime bytecode updated — ABI/interface intact.
Function ABI unchanged: adapterOf, configAddress, getPrice, owner, priceAdapterDecimals, renounceOwnership, setPriceAdapter, transferOwnership, unsetPriceAdapter.
If contracts are already deployed, plan a migration path — no storage-layout change expected but verify storage layout and upgrade steps separately.artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (2)
75-79: ReentrancyGuard error surfaced in ABI — good.Presence of ReentrancyGuardReentrantCall confirms artifacts include the guard added to the contract.
351-355: Regenerate TypeChain/ethers typings for performUpkeep param rename (performData).ABI shows performUpkeep param renamed to "performData" (artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json:351-355). Rebuild typings and update call sites/tests to use performData: BytesLike.
Current local attempt failed with: "Error: Cannot find module '@fhevm/hardhat-plugin'". Fix/install deps (e.g., pnpm install or npm ci / add the missing plugin) and then run:
rm -rf typechain-types && npx hardhat compile && rg -nP 'performUpkeep\s*(\sperformData:\sBytesLike' typechain-types
contracts/vaults/OrionEncryptedVault.sol (2)
104-104: Good: validate before mutating on-chain state.Calling _validateIntent before overwriting intent storage avoids partial writes if validation fails.
180-182: LGTM: cache length before loop.Minor gas/readability win by using newPortfolioLength.
contracts/orchestrators/InternalStatesOrchestrator.sol (1)
354-367: Validate minibatchIndex against state to prevent reordering.Optionally require minibatchIndex == currentMinibatchIndex to avoid accidental reprocessing/reordering by the automation.
Do you want a patch enforcing this check in both _preprocessTransparentMinibatch and _preprocessEncryptedMinibatch?
Also applies to: 438-451
| "internalType": "bytes", | ||
| "name": "cleartexts", | ||
| "type": "bytes" | ||
| }, | ||
| { | ||
| "internalType": "bytes[]", | ||
| "name": "signatures", | ||
| "type": "bytes[]" | ||
| "internalType": "bytes", | ||
| "name": "decryptionProof", | ||
| "type": "bytes" | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ABI-breaking change: callbackDecryptSingleEbool now (uint256, bytes cleartexts, bytes decryptionProof).
Update all callers (TS, relayer/oracle) and regenerate TypeChain types. Ensure the wire format of cleartexts/proof is specified and validated on-chain.
Consider emitting an event on successful callback with requestID to aid observability.
🏁 Script executed:
#!/bin/bash
# Locate old parameter names/usages to update
rg -nP 'callbackDecryptSingleEbool\s*\(' --type=sol --type=ts -C2
rg -n 'decryptedInput|signatures' -g '!artifacts/**'Length of output: 176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'PWD: %s\n' "$(pwd)"
echo '--- searching for occurrences of "callbackDecryptSingleEbool" ---'
if command -v rg >/dev/null 2>&1; then
rg -n -uu 'callbackDecryptSingleEbool' -S -C3 || true
else
grep -nR --exclude-dir=.git 'callbackDecryptSingleEbool' . || true
fi
echo '--- searching for "cleartexts" or "decryptionProof" occurrences ---'
if command -v rg >/dev/null 2>&1; then
rg -n -uu 'cleartexts|decryptionProof' -S -C3 || true
else
grep -nR --exclude-dir=.git -E 'cleartexts|decryptionProof' . || true
fi
ART="artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json"
echo "--- checking artifact file: $ART ---"
if [ -f "$ART" ]; then
echo "FOUND $ART"
if command -v jq >/dev/null 2>&1; then
jq '.abi[] | select(.name=="callbackDecryptSingleEbool")' "$ART" || true
else
python3 - <<'PY'
import json,sys
p="artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json"
try:
with open(p) as f:
j=json.load(f)
for a in j.get("abi",[]):
if a.get("name")=="callbackDecryptSingleEbool":
import pprint
pprint.pprint(a)
break
else:
print("ABI entry not found")
except Exception as e:
print("ERROR", e, file=sys.stderr)
PY
fi
else
echo "$ART not found"
fi
echo '--- searching generated types / TypeChain for interface or function ---'
if command -v rg >/dev/null 2>&1; then
rg -n -uu 'callbackDecryptSingleEbool|IOrionEncryptedVault' -S -C3 || true
else
grep -nR --exclude-dir=.git -E 'callbackDecryptSingleEbool|IOrionEncryptedVault' . || true
fiLength of output: 29410
ABI-breaking change: callbackDecryptSingleEbool signature changed — update callers & regenerate TypeChain
- Confirmed: signature is now (uint256 requestID, bytes cleartexts, bytes decryptionProof) in contracts/interfaces/IOrionEncryptedVault.sol and contracts/vaults/OrionEncryptedVault.sol (implementation uses abi.decode(cleartexts,(bool))); artifacts updated (artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json).
- Action: Update all off-chain callers (TypeScript client, relayer/oracle) to the new ABI, regenerate TypeChain types, and ensure the relayer wire-format matches the on-chain decoding (add explicit on-chain validation of cleartexts/decryptionProof and test coverage).
- Optional: emit an event on successful callback with requestID for observability.
🤖 Prompt for AI Agents
In
artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json
around lines 424 to 432, the callbackDecryptSingleEbool ABI changed to (uint256
requestID, bytes cleartexts, bytes decryptionProof); update all off-chain
callers (TypeScript client, relayer/oracle) to pass requestID, and encode
cleartexts as the on-chain-expected ABI (bool via abi.encode/abi.decode) and
decryptionProof as bytes, then regenerate TypeChain types so callers’ signatures
match the new ABI; also update relayer wire-format to send cleartexts as an
ABI-encoded bool, add on-chain validation in OrionEncryptedVault to assert
cleartexts length/decoded type and validate decryptionProof, add tests covering
successful and malformed payloads, and consider emitting an event with requestID
on successful callback for observability.
| @@ -563,12 +570,9 @@ contract InternalStatesOrchestrator is SepoliaConfig, Ownable, ReentrancyGuard, | |||
| address vault = encryptedVaultsEpoch[i]; | |||
| IOrionEncryptedVault vaultContract = IOrionEncryptedVault(vault); | |||
|
|
|||
There was a problem hiding this comment.
Callback sentinel check is incorrect; decode as uint256[] and handle empty.
Comparing cleartexts to abi.encode(uint256(0)) is incompatible with decoding as uint256[] and risks a revert. Decode first, then branch on length.
FHE.checkSignatures(requestID, cleartexts, decryptionProof);
-
-if (keccak256(cleartexts) == keccak256(abi.encode(uint256(0)))) {
- currentPhase = InternalUpkeepPhase.Buffering;
- currentMinibatchIndex = 0;
- return;
-}
-
-// Store decrypted values for processing in the next phase.
-// TODO(fhevm): avoid breaking down this into two phases, consider letting Zama callback do all the work.
-_decryptedValues = abi.decode(cleartexts, (uint256[]));
+uint256[] memory values = abi.decode(cleartexts, (uint256[]));
+if (values.length == 0) {
+ currentPhase = InternalUpkeepPhase.Buffering;
+ currentMinibatchIndex = 0;
+ return;
+}
+// Store decrypted values for processing in the next phase.
+_decryptedValues = values;📝 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.
| FHE.checkSignatures(requestID, cleartexts, decryptionProof); | |
| if (keccak256(cleartexts) == keccak256(abi.encode(uint256(0)))) { | |
| currentPhase = InternalUpkeepPhase.Buffering; | |
| currentMinibatchIndex = 0; | |
| return; | |
| } | |
| // Store decrypted values for processing in the next phase. | |
| // TODO(fhevm): avoid breaking down this into two phases, consider letting Zama callback do all the work. | |
| _decryptedValues = decryptedValues; | |
| _decryptedValues = abi.decode(cleartexts, (uint256[])); | |
| FHE.checkSignatures(requestID, cleartexts, decryptionProof); | |
| uint256[] memory values = abi.decode(cleartexts, (uint256[])); | |
| if (values.length == 0) { | |
| currentPhase = InternalUpkeepPhase.Buffering; | |
| currentMinibatchIndex = 0; | |
| return; | |
| } | |
| // Store decrypted values for processing in the next phase. | |
| // TODO(fhevm): avoid breaking down this into two phases, consider letting Zama callback do all the work. | |
| _decryptedValues = values; |
🤖 Prompt for AI Agents
In contracts/orchestrators/InternalStatesOrchestrator.sol around lines 542 to
553, the code currently compares raw cleartexts to abi.encode(uint256(0)) before
decoding, which is incompatible with the subsequent abi.decode to uint256[] and
can revert; instead, first abi.decode(cleartexts, (uint256[])) into a local
uint256[] variable, then check if the decoded array is empty (length == 0) and
if so set currentPhase = InternalUpkeepPhase.Buffering, currentMinibatchIndex =
0 and return; remove the keccak256 comparison and use the decoded array for
subsequent storage by assigning _decryptedValues = decodedArray.
| function _validateIntent(address[] memory assets, euint128 totalWeight) internal { | ||
| _validateIntentAssets(assets); | ||
|
|
||
| ebool isIntentEValid = FHE.and(areWeightsValid, FHE.eq(totalWeight, _encryptedTotalWeight)); | ||
| ebool isIntentEValid = FHE.eq(totalWeight, _encryptedTotalWeight); | ||
|
|
||
| // slither-disable-next-line unused-return | ||
| FHE.allowThis(isIntentEValid); | ||
|
|
||
| bytes32[] memory cypherTexts = new bytes32[](1); | ||
| cypherTexts[0] = FHE.toBytes32(isIntentEValid); | ||
| bytes32[] memory cipherTexts = new bytes32[](1); | ||
| cipherTexts[0] = FHE.toBytes32(isIntentEValid); | ||
|
|
||
| // slither-disable-next-line unused-return | ||
| FHE.requestDecryption(cypherTexts, this.callbackDecryptSingleEbool.selector); | ||
| FHE.requestDecryption(cipherTexts, this.callbackDecryptSingleEbool.selector); | ||
| } |
There was a problem hiding this comment.
Race on decryption callbacks — stale requests can flip isIntentValid.
Multiple submitIntent calls can overlap; callbackDecryptSingleEbool accepts any valid signed callback and will update isIntentValid for the latest or a stale request indistinguishably. Store the latest request ID and ignore stale callbacks.
Apply:
function _validateIntent(address[] memory assets, euint128 totalWeight) internal {
_validateIntentAssets(assets);
ebool isIntentEValid = FHE.eq(totalWeight, _encryptedTotalWeight);
// slither-disable-next-line unused-return
FHE.allowThis(isIntentEValid);
bytes32[] memory cipherTexts = new bytes32[](1);
cipherTexts[0] = FHE.toBytes32(isIntentEValid);
- // slither-disable-next-line unused-return
- FHE.requestDecryption(cipherTexts, this.callbackDecryptSingleEbool.selector);
+ // Track the latest request to guard against stale callbacks
+ uint256 reqId = FHE.requestDecryption(cipherTexts, this.callbackDecryptSingleEbool.selector);
+ _latestDecryptRequestId = reqId;
}Add state (outside this hunk):
// Track the most recent decryption request for intent validation
uint256 private _latestDecryptRequestId;| function callbackDecryptSingleEbool( | ||
| uint256 requestID, | ||
| bytes calldata cleartexts, | ||
| bytes calldata decryptionProof | ||
| ) external { | ||
| FHE.checkSignatures(requestID, cleartexts, decryptionProof); | ||
|
|
||
| isIntentValid = abi.decode(cleartexts, (bool)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate callback to the latest request; optionally clear the latch.
Guard against out-of-order deliveries and stray callbacks.
Apply:
function callbackDecryptSingleEbool(
uint256 requestID,
bytes calldata cleartexts,
bytes calldata decryptionProof
) external {
FHE.checkSignatures(requestID, cleartexts, decryptionProof);
- isIntentValid = abi.decode(cleartexts, (bool));
+ if (requestID != _latestDecryptRequestId) {
+ return; // drop stale callback
+ }
+ isIntentValid = abi.decode(cleartexts, (bool));
+ _latestDecryptRequestId = 0; // optional: reset
}📝 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.
| function callbackDecryptSingleEbool( | |
| uint256 requestID, | |
| bytes calldata cleartexts, | |
| bytes calldata decryptionProof | |
| ) external { | |
| FHE.checkSignatures(requestID, cleartexts, decryptionProof); | |
| isIntentValid = abi.decode(cleartexts, (bool)); | |
| } | |
| function callbackDecryptSingleEbool( | |
| uint256 requestID, | |
| bytes calldata cleartexts, | |
| bytes calldata decryptionProof | |
| ) external { | |
| FHE.checkSignatures(requestID, cleartexts, decryptionProof); | |
| if (requestID != _latestDecryptRequestId) { | |
| return; // drop stale callback | |
| } | |
| isIntentValid = abi.decode(cleartexts, (bool)); | |
| _latestDecryptRequestId = 0; // optional: reset | |
| } |
1 participant
Summary by Sourcery
Integrate modular Chainlink Automation flows and reentrancy protection in LiquidityOrchestrator, streamline InternalStatesOrchestrator complexity, and simplify vault intent validation.
New Features:
Enhancements:
Chores:
Summary by CodeRabbit
New Features
Refactor
Chores
Tests