Dev - #254
Merged
Merged
Conversation
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 5 to 7. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v5...v7) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe protocol adds upgradeable encrypted vaults, HPKE configuration, ciphertext-based epoch processing, and test utilities. The orchestrator includes encrypted vaults in epochs, hashes ciphertext state, processes structured vault states, and applies encrypted decommissioning rules. The CI workflow updates the Codecov action. ChangesEncrypted vault support
CI coverage action update
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Owner
participant OrionConfig
participant EncryptedVaultFactory
participant OrionEncryptedVault
participant LiquidityOrchestrator
Owner->>OrionConfig: configure encrypted factory and HPKE key
EncryptedVaultFactory->>OrionEncryptedVault: deploy and initialize vault proxy
EncryptedVaultFactory->>OrionConfig: register encrypted vault
OrionEncryptedVault->>OrionEncryptedVault: store strategist intent ciphertext
LiquidityOrchestrator->>OrionEncryptedVault: read portfolio and intent ciphertext
LiquidityOrchestrator->>LiquidityOrchestrator: build epoch commitment
LiquidityOrchestrator->>OrionEncryptedVault: write portfolio ciphertext and total assets
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…/dev/codecov/codecov-action-7 chore(deps): bump codecov/codecov-action from 5 to 7
matteoettam09
marked this pull request as ready for review
August 10, 2026 17:36
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/LiquidityOrchestrator.sol (1)
575-608: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCommit the vault type inside the leaf.
The leaf preimage mixes two different hashing schemes into the same
portfolioHashandintentHashpositions. For an encrypted vault the preimage is a raw ciphertext. For a transparent vault it isabi.encodeof parallel arrays. The leaf carries no tag that records which scheme applied.The prover must therefore learn the vault type from state that sits outside the commitment. Add the type to the leaf so the commitment binds the interpretation of both hashes.
♻️ Proposed change
bytes32 portfolioHash; bytes32 intentHash; - if (config.isEncryptedVault(vaultAddress)) { + bool isEncrypted = config.isEncryptedVault(vaultAddress); + if (isEncrypted) { IOrionEncryptedVault encryptedVault = IOrionEncryptedVault(vaultAddress); portfolioHash = keccak256(encryptedVault.getPortfolio()); intentHash = keccak256(encryptedVault.getIntent()); } else { IOrionTransparentVault transparentVault = IOrionTransparentVault(vaultAddress); (address[] memory portfolioTokens, uint256[] memory portfolioShares) = transparentVault.getPortfolio(); (address[] memory intentTokens, uint32[] memory intentWeights) = transparentVault.getIntent(); portfolioHash = keccak256(abi.encode(portfolioTokens, portfolioShares)); intentHash = keccak256(abi.encode(intentTokens, intentWeights)); } bytes32 vaultLeaf = keccak256( abi.encode( vaultAddress, + isEncrypted, uint8(feeModel.feeType),This changes the commitment preimage. Update the off-chain prover to match.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/LiquidityOrchestrator.sol` around lines 575 - 608, Update the vault leaf preimage in the loop building vaultLeaf to include an explicit vault-type discriminator alongside portfolioHash and intentHash, using distinct values for encrypted and transparent vaults. Ensure the discriminator is derived from config.isEncryptedVault(vaultAddress) and included in the committed abi.encode data, then update the off-chain prover’s leaf construction to use the same field and encoding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 58: Update the Codecov step’s codecov/codecov-action reference to an
immutable commit SHA instead of the mutable v7 tag, preserving the existing
action configuration.
In `@contracts/factories/EncryptedVaultFactory.sol`:
- Around line 61-90: Update the shared __OrionVault_init initializer to reject
strategist_ == address(0) with the existing invalid-arguments error, alongside
its feeType_ validation. Ensure all vault creation paths using this initializer
enforce a nonzero strategist before assigning it.
In `@contracts/LiquidityOrchestrator.sol`:
- Around line 982-994: Update the encrypted branch of the decommissioning check
in the vault finalization flow to require both an empty portfolioCiphertext and
vaultState.finalTotalAssets == 0 before setting portfolioLiquidated true. Leave
the transparent-branch conditions and surrounding request checks unchanged.
In `@contracts/OrionConfig.sol`:
- Around line 280-286: Update setHpkePublicKey in OrionConfig to require
isSystemIdle() alongside onlyOwner before accepting the key, preserving the
existing nonzero validation. Add the proposed HpkePublicKeyUpdated event to
EventsLib and emit it after successfully storing the new key, following the
setGuardian event pattern.
In `@contracts/test/LiquidityOrchestratorEpochEndHarness.sol`:
- Around line 44-71: Align h_advancePvoIndexLikeProcessMinibatch with production
by reusing internal helpers for PVO completion and epoch-end handling. Extract
the shared i1 clamping and _nextUpdateTime update into a production helper used
by both performUpkeep and the harness, and extract the inline epoch-end sequence
into another helper used by both paths, replacing _maybeEpochEndAfterPvo while
preserving existing state resets, failed-token cleanup, event emission, and
epoch increment.
In `@contracts/test/MaliciousManagerRemovalVault.sol`:
- Around line 7-10: Rename the local IOwnable2Step interface to an
accept-ownership-specific name containing only acceptOwnership. Update
acceptOwnership() to cast config through the renamed interface, and change
triggerRemoveManager() and overrideIntentForDecommissioning() to call
config.removeWhitelistedManager(manager) directly through IOrionConfig.
In `@contracts/vaults/OrionEncryptedVault.sol`:
- Around line 70-80: Mark the base OrionVault.overrideIntentForDecommissioning
hook as virtual, then override it in OrionEncryptedVault with external override
onlyConfig; set isDecommissioning to true and delete _intentCiphertext so
getIntent no longer exposes stale data during decommissioning.
In `@test/EncryptedVaultFactoryOrchestratorBranches.test.ts`:
- Around line 49-53: Ensure all impersonated accounts are released after each
test. In test/EncryptedVaultFactoryOrchestratorBranches.test.ts lines 49-53,
track addresses enabled by impersonate and release them in an afterEach hook. In
test/LiquidityOrchestratorEncrypted.test.ts lines 243-248 and 309-313, stop
impersonating loAddr after each updateVaultState call or use shared afterEach
cleanup; preserve the existing test behavior.
In `@test/LiquidityOrchestratorBufferAccrual.test.ts`:
- Around line 17-18: Remove the duplicated payload ABI definitions and reuse the
canonical helpers. In test/LiquidityOrchestratorBufferAccrual.test.ts:17-18,
delete STATES_STRUCT_TYPE and the local encodePerformPayload, importing both
from ./helpers/loPerformPayload; in
test/LiquidityOrchestratorEpochEnd.test.ts:55-67, delete local emptyVaultState
and import it from that helper; in
test/EncryptedVaultFactoryOrchestratorBranches.test.ts:30, delete local
PUBLIC_VALUES_TYPE and add it to the existing helper import.
In `@test/OrionEncryptedVault.test.ts`:
- Around line 226-232: Extend the updateVaultState test around
vault.connect(loSigner).updateVaultState to assert the
ConfidentialVaultStateUpdated event arguments, including the expected
currentSharePrice. Add a separate scenario that deposits shares before updating
the vault so totalSupply() is non-zero and the high-water-mark branch executes,
then verify both feeModel.highWaterMark and oldFeeModel.highWaterMark advance.
- Around line 150-170: Add an assertion in the `rejects stranger, empty,
too-short, and too-long blobs` test that
`strategist.submitIntent(ciphertextOfLength(maxLen))` succeeds, pairing the
existing `maxLen + 1` rejection with the inclusive upper-boundary acceptance.
---
Outside diff comments:
In `@contracts/LiquidityOrchestrator.sol`:
- Around line 575-608: Update the vault leaf preimage in the loop building
vaultLeaf to include an explicit vault-type discriminator alongside
portfolioHash and intentHash, using distinct values for encrypted and
transparent vaults. Ensure the discriminator is derived from
config.isEncryptedVault(vaultAddress) and included in the committed abi.encode
data, then update the off-chain prover’s leaf construction to use the same field
and encoding.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87e2a85c-5149-4725-a36a-329aed7bef42
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.github/workflows/ci.ymlcontracts/LiquidityOrchestrator.solcontracts/OrionConfig.solcontracts/factories/EncryptedVaultFactory.solcontracts/interfaces/ILiquidityOrchestrator.solcontracts/interfaces/IOrionConfig.solcontracts/interfaces/IOrionEncryptedVault.solcontracts/libraries/EventsLib.solcontracts/test/LiquidityOrchestratorEpochEndHarness.solcontracts/test/LiquidityOrchestratorHarness.solcontracts/test/LiquidityOrchestratorSlippageHarness.solcontracts/test/MaliciousManagerRemovalVault.solcontracts/vaults/OrionEncryptedVault.solpackage.jsontest/EncryptedVaultFactoryOrchestratorBranches.test.tstest/LiquidityOrchestratorBufferAccrual.test.tstest/LiquidityOrchestratorEncrypted.test.tstest/LiquidityOrchestratorEpochEnd.test.tstest/LiquidityOrchestratorSlippage.test.tstest/OrionEncryptedVault.test.tstest/OrionEncryptedVaultHpke.test.tstest/OrionHpke.test.tstest/OrionVaultConfigOrchestratorBranches.test.tstest/VaultDecommissioning.test.tstest/helpers/loPerformPayload.tstest/helpers/orionHpke.tstest/vectors/hpke-orion-v1.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contracts/LiquidityOrchestrator.sol (1)
992-994:⚠️ Potential issue | 🟠 MajorRequire zero assets before encrypted decommissioning.
portfolioCiphertext.length == 0does not prove that the vault has no assets. Lines 975-978 store the prover-suppliedvaultState.finalTotalAssetsin the encrypted vault. An empty ciphertext with a non-zero total can therefore satisfyportfolioLiquidatedand callconfig.completeVaultDecommissioningat Line 1002.Require both conditions before finalizing decommissioning.
Proposed fix
if (encrypted) { - // Empty encrypted portfolio => liquidation complete. - portfolioLiquidated = vaultState.portfolioCiphertext.length == 0; + // Empty encrypted portfolio and zero assets => liquidation complete. + portfolioLiquidated = + vaultState.portfolioCiphertext.length == 0 && vaultState.finalTotalAssets == 0; } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/LiquidityOrchestrator.sol` around lines 992 - 994, Update the encrypted branch in the liquidation logic around portfolioLiquidated so decommissioning is considered complete only when portfolioCiphertext is empty and the stored finalTotalAssets represents zero assets. Preserve the existing ciphertext check and require both conditions before config.completeVaultDecommissioning can be reached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@contracts/LiquidityOrchestrator.sol`:
- Around line 992-994: Update the encrypted branch in the liquidation logic
around portfolioLiquidated so decommissioning is considered complete only when
portfolioCiphertext is empty and the stored finalTotalAssets represents zero
assets. Preserve the existing ciphertext check and require both conditions
before config.completeVaultDecommissioning can be reached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e185bf40-fdfe-4c77-b362-a308af648d89
📒 Files selected for processing (1)
contracts/LiquidityOrchestrator.sol
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 CodeRabbit
New Features
Tests
Chores