Passive curator - #101
Conversation
Reviewer's GuideThis PR transitions the passive curator architecture from a pull-based to a push-based model by replacing computeIntent/validateStrategy with a submitIntent API, removing on-vault passive detection and fallback, enforcing curator whitelisting in config and factory, refactoring the KBestTvlWeightedAverage strategy and vault contracts accordingly, updating orchestrator logic, and harmonizing all tests (including a new invalid strategy test case). Sequence diagram for push-based passive curator intent submissionsequenceDiagram
participant VaultFactory
participant OrionConfig
participant Curator (Smart Contract)
participant Vault
actor VaultOwner
VaultOwner->>VaultFactory: createVault(curator)
VaultFactory->>OrionConfig: isWhitelistedCurator(curator)
alt curator is whitelisted
VaultFactory->>Vault: deploy with curator
Curator->>Vault: submitIntent(vault)
Vault->>Vault: update _portfolioIntent
else curator not whitelisted
VaultFactory-->>VaultOwner: revert UnauthorizedAccess
end
Entity relationship diagram for curator whitelisting in OrionConfigerDiagram
ORIONCONFIG {
address id
address[] whitelistedAssets
address[] whitelistedVaultOwners
address[] whitelistedCurators
}
CURATOR {
address id
}
ORIONCONFIG ||--o{ CURATOR : "whitelistedCurators"
VAULTOWNER {
address id
}
ORIONCONFIG ||--o{ VAULTOWNER : "whitelistedVaultOwners"
Class diagram for updated passive curator architectureclassDiagram
class OrionTransparentVault {
- EnumerableMap.AddressToUintMap _portfolioIntent
+ updateCurator(address newCurator)
+ getIntent()
// Removed: _isPassiveCurator, _updateCuratorType(), isPassiveCurator(), _computePassiveIntent()
}
class IOrionStrategy {
+ submitIntent(IOrionTransparentVault vault)
// Removed: computeIntent(), validateStrategy(), getStatefulIntent()
}
class KBestTvlWeightedAverage {
+ k: uint16
+ config: IOrionConfig
+ submitIntent(IOrionTransparentVault vault)
- _getAssetTVLs(address[] vaultWhitelistedAssets, uint16 n)
- _selectTopKAssets(address[] vaultWhitelistedAssets, uint256[] tvls, uint16 n, uint16 kActual)
- _calculatePositions(address[] tokens, uint256[] topTvls, uint16 kActual)
// Removed: kMax, _statefulIntent, computeIntent(), validateStrategy(), getStatefulIntent()
}
class OrionConfig {
+ whitelistedCurators: EnumerableSet.AddressSet
+ addWhitelistedCurator(address curator)
+ removeWhitelistedCurator(address curator)
+ isWhitelistedCurator(address curator) : bool
}
OrionTransparentVault --> IOrionStrategy
KBestTvlWeightedAverage ..|> IOrionStrategy
OrionTransparentVault --> OrionConfig
KBestTvlWeightedAverage --> OrionConfig
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🛡️ 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. |
|
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. WalkthroughAdds curator whitelisting APIs to OrionConfig, replaces pull-based strategy interface with a push-based submitIntent flow, removes passive-curator logic from OrionTransparentVault, updates vault creation/validation and orchestrator epoch filtering, and updates artifacts and tests to match these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Owner as Owner
participant Config as OrionConfig
participant Vault as OrionTransparentVault
participant Strat as Strategy
Note over Config,Vault: Curator whitelisting and update flow
Owner->>Config: addWhitelistedCurator(curator)
Config-->>Owner: ack
Owner->>Vault: updateCurator(curator)
Vault->>Config: isWhitelistedCurator(curator)
Config-->>Vault: true
Vault-->>Owner: CuratorUpdated event
sequenceDiagram
participant Vault as OrionTransparentVault
participant Strat as KBestTvlWeightedAverage
participant Config as OrionConfig
Note over Vault,Strat: New push-based intent submission
Strat->>Config: isWhitelistedCurator(strat)
Config-->>Strat: true
Strat->>Vault: vault.vaultWhitelist()
Vault-->>Strat: address[] assets
Strat->>Strat: compute TVLs, select top-k, build Intent[]
Strat->>Vault: submitIntent(Intent[])
Vault-->>Vault: store intent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (5)artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (2)
contracts/vaults/OrionTransparentVault.sol (2)
contracts/strategies/KBestTvlWeightedAverage.sol (2)
test/PassiveCuratorStrategy.test.ts (1)
contracts/interfaces/IOrionStrategy.sol (2)
⏰ 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)
🔇 Additional comments (19)
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:
- Ensure the new
submitIntententry point onOrionTransparentVaultincludes an explicit access check (e.g. only the current curator contract may call it) to prevent unauthorized contracts from pushing arbitrary intents. - In
KBestTvlWeightedAverage, the residual weight correction is always applied to the first token; consider applying it to the asset with the largest raw allocation (or the last index) to avoid biasing your final distribution. updateCuratornow lumps zero‐address and non‐whitelisted failures underUnauthorizedAccess; introducing a distinctZeroAddresserror when passing the zero address would make error handling more precise.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Ensure the new `submitIntent` entry point on `OrionTransparentVault` includes an explicit access check (e.g. only the current curator contract may call it) to prevent unauthorized contracts from pushing arbitrary intents.
- In `KBestTvlWeightedAverage`, the residual weight correction is always applied to the first token; consider applying it to the asset with the largest raw allocation (or the last index) to avoid biasing your final distribution.
- `updateCurator` now lumps zero‐address and non‐whitelisted failures under `UnauthorizedAccess`; introducing a distinct `ZeroAddress` error when passing the zero address would make error handling more precise.
## Individual Comments
### Comment 1
<location> `test/PassiveCuratorStrategy.test.ts:336-345` </location>
<code_context>
+ it("should handle case when k > number of available assets", async function () {
</code_context>
<issue_to_address>
**suggestion (testing):** Edge case for k > assets is covered, but does not test for k = 0.
Please add a test for k = 0 to verify correct handling of this edge case.
</issue_to_address>
### Comment 2
<location> `test/PassiveCuratorStrategy.test.ts:465-462` </location>
<code_context>
- ).to.be.revertedWithCustomError(strategy, "InvalidStrategy");
- });
-
it("should allow whitelist updates when curator is not a strategy", async function () {
await transparentVault.connect(owner).updateCurator(owner.address);
</code_context>
<issue_to_address>
**suggestion (testing):** Test for whitelist update when curator is not a strategy may miss edge cases.
Add tests for updating the whitelist to an empty array and to a set with only non-ERC4626 assets to verify correct contract behavior in these scenarios.
Suggested implementation:
```typescript
it("should allow whitelist updates when curator is not a strategy", async function () {
await transparentVault.connect(owner).updateCurator(owner.address);
});
it("should handle whitelist update to an empty array", async function () {
await transparentVault.connect(owner).updateCurator(owner.address);
await expect(
transparentVault.connect(owner).updateWhitelist([])
).to.not.be.reverted;
const whitelist = await transparentVault.getWhitelist();
expect(whitelist).to.be.an("array").that.is.empty;
});
it("should handle whitelist update to only non-ERC4626 assets", async function () {
await transparentVault.connect(owner).updateCurator(owner.address);
// Assume nonErc4626Asset is a valid address but not an ERC4626 asset
const nonErc4626Asset = owner.address; // Replace with actual non-ERC4626 asset address if available
await expect(
transparentVault.connect(owner).updateWhitelist([nonErc4626Asset])
).to.not.be.reverted;
const whitelist = await transparentVault.getWhitelist();
expect(whitelist).to.deep.equal([nonErc4626Asset]);
// Optionally, add more assertions to check contract behavior with non-ERC4626 assets
});
```
- If you have a specific non-ERC4626 asset address, replace `owner.address` with that address in the test.
- Ensure that `updateWhitelist` and `getWhitelist` are the correct contract methods for updating and reading the whitelist.
- Add any additional assertions relevant to your contract's expected behavior when the whitelist contains only non-ERC4626 assets.
</issue_to_address>
### Comment 3
<location> `test/orchestrator/Orchestrators.test.ts:692-689` </location>
<code_context>
});
describe("performUpkeep", function () {
- it("should complete full upkeep cycles without intent decryption", async function () {
+ it("", async function () {
// Fast forward time to trigger upkeep
expect(await internalStatesOrchestrator.currentPhase()).to.equal(0); // Idle
</code_context>
<issue_to_address>
**nitpick:** Test name was removed, now empty string.
A meaningful test name will make it easier to understand and maintain the test suite.
</issue_to_address>
### Comment 4
<location> `test/MinimumAmountDOS.test.ts:59-66` </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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/orchestrator/OrchestratorsZeroState.test.ts (1)
431-432: Add missing access control to strategy's submitIntent() function.KBestTvlWeightedAverage.submitIntent() is called by test code with explicit owner/curator signers, indicating the design intent was to restrict access, but the function lacks the access control modifier. Currently, any external caller can invoke submitIntent() directly on the strategy contract, which then submits intents to the vault. This bypasses the authorization model.
Since the contract is Ownable and updateParameters() correctly uses
onlyOwner, submitIntent() should enforce the same access control:contracts/strategies/KBestTvlWeightedAverage.sol line 37: Add
onlyOwnermodifier to submitIntent() function declaration.contracts/vaults/OrionTransparentVault.sol (1)
14-25: Active-only intent model is consistent, but docs and whitelist signaling need a refresh
- Seeding
_portfolioIntentto 100% underlying in the constructor and reassigning removed-asset weight tothis.asset()both preserve the “weights sum to curatorIntentDecimals” invariant, which fits the new push‑only intent model.- Requiring
config.isWhitelistedCurator(newCurator)inupdateCuratoris a good hardening step around curator ACLs.- Ensuring the underlying asset is always present in
_vaultWhitelistedAssetskeeps intents and portfolio logic consistent even if callers omit it fromassets.Two follow‑ups worth addressing:
- The header comment still describes passive management via
IOrionStrategyand automatic curator‑type detection, but the implementation is now active‑only. That’s likely to confuse integrators and should be updated to reflect the current behavior.VaultWhitelistUpdated(assets)does not include the implicitly-added underlying asset when it wasn’t inassets. If off‑chain consumers rely on that event to reconstruct the whitelist, consider either appending the underlying to the emitted list or documenting that the on-chain whitelist may be a strict superset of the event payload.Also applies to: 56-58, 155-159, 176-179, 196-205
contracts/strategies/KBestTvlWeightedAverage.sol (2)
114-136: Guard againstkActual == 0andtotalTVL == 0edge cases in_calculatePositionsIn
_calculatePositions:uint16 kActual = ...; // passed in // ... for (uint16 i = 0; i < kActual; ++i) { totalTVL += topTvls[i]; } // ... for (uint16 i = 0; i < kActual; ++i) { uint32 weight = uint32((topTvls[i] * intentScale) / totalTVL); // ... } if (sumWeights < intentScale) { intent[0].weight += intentScale - sumWeights; }Two edge cases can cause hard-to-diagnose reverts:
kActual == 0(e.g.,k == 0viaupdateParametersor an empty whitelist):
intentis a zero-length array, but you still executeintent[0].weight += ..., which will underflow with an out-of-bounds read/write.
totalTVL == 0(e.g., all selected assets havetotalAssets() == 0and no fallback kicks in):
- The weight computation divides by
totalTVL, causing a division-by-zero revert.Recommend explicitly handling these cases, for example:
function _calculatePositions( address[] memory tokens, uint256[] memory topTvls, uint16 kActual ) internal view returns (IOrionTransparentVault.IntentPosition[] memory intent) { - uint256 totalTVL = 0; + uint256 totalTVL = 0; for (uint16 i = 0; i < kActual; ++i) { totalTVL += topTvls[i]; } + if (kActual == 0 || totalTVL == 0) { + revert ErrorsLib.InvalidTotalTVL(); + } + uint32 intentScale = uint32(10 ** config.curatorIntentDecimals()); intent = new IOrionTransparentVault.IntentPosition[](kActual); // ... if (sumWeights < intentScale) { intent[0].weight += intentScale - sumWeights; } }(or any equivalent policy you prefer).
This keeps misconfiguration or pathological-TVL states from surfacing as generic arithmetic/oob errors deeper in the call stack.
138-142: Prevent misconfiguration by rejectingkNew == 0inupdateParameters
updateParameterscurrently allows settingkto0:function updateParameters(uint16 kNew) external onlyOwner { k = kNew; }Given the logic in
submitIntent/_calculatePositions, a zerokleads tokActual == 0and the edge case noted above. Even if you add guards in_calculatePositions, it’s clearer to simply disallowk == 0at the config level:function updateParameters(uint16 kNew) external onlyOwner { - k = kNew; + if (kNew == 0) revert ErrorsLib.InvalidParameter(); + k = kNew; }This avoids a class of invalid configurations and makes the strategy’s expected domain explicit.
🧹 Nitpick comments (11)
test/orchestrator/Orchestrators.test.ts (1)
689-689: Give the longperformUpkeepscenario test a descriptive name
it("")makes this very large, end‑to‑end test hard to identify in output and when it fails. Consider renaming to something like"should run full multi-epoch upkeep with passive strategy and fee accounting"or similar to improve maintainability.contracts/test/KBestTvlWeightedAverageInvalid.sol (1)
1-151: Invalid strategy matches test goal; guard againsttotalTVL == 0This helper is well‑structured for negative‑path testing: it picks top‑K by TVL and deliberately avoids weight normalization so
submitIntentcan fail onInvalidTotalWeight. One edge case: if all selected assets havetotalAssets() == 0,totalTVLstays 0 and_calculatePositionswill divide by zero.Since this is test‑only, it’s not critical, but you may want to short‑circuit when
totalTVL == 0(e.g., revert with a clearer error or return an empty intent) to keep the failure mode aligned with the “invalid weights” semantics rather than a generic arithmetic fault.contracts/interfaces/IOrionStrategy.sol (1)
6-14: Align interface docs with new push-basedsubmitIntentmodelThe interface definition is now a single
submitIntent(IOrionTransparentVault vault)entrypoint, but the header comments still talk about “passive curators” and “compute portfolio intents on-demand”. Consider rewording@notice/@devto describe curator strategies that submit intents to vaults (push model) rather than on-demand computation, to avoid confusion for integrators.test/PassiveCuratorStrategy.test.ts (6)
17-19: Negative-path invalid-strategy test is well structured; minor naming / reuse nitsThe addition of
KBestTvlWeightedAverageInvalidand the dedicated"should fail when strategy does not adjust weights to sum to intentScale"test give good coverage of the vault-side invariant. You:
- Deploy an invalid strategy.
- Create a separate vault, configure whitelist, and set the invalid strategy as curator.
- Assert revert with
InvalidTotalWeightonsubmitIntent.This is solid. If you want to tighten things further, consider:
- Reusing the vault-creation helper logic from the main setup (to avoid duplicating event parsing).
- Explicitly asserting that
weightsreturned by the invalid strategy do not sum tointentScale(if you ever expose them) to make the test intent even clearer.Also applies to: 495-554
297-318: Explicitly document reliance on implicit inclusion ofunderlyingAssetinvaultWhitelistThe expectations:
const vaultWhitelist = await transparentVault.vaultWhitelist(); expect(vaultWhitelist.length).to.equal(5); // ... // mockAsset1: 3000, mockAsset2: 2000, mockAsset3: 1500, mockAsset4: 1000, underlyingAsset: 0 // ... expect(tokens).to.not.include(await underlyingAsset.getAddress());encode the assumption that
vaultWhitelist()returns the 4 ERC4626 assets plusunderlyingAsset, and that the strategy’s top‑K selection excludesunderlyingAssetfork = 3.That coupling is fine, but it’s fairly protocol-specific. To avoid future confusion if the vault implementation ever changes its internal whitelist representation, I’d either:
- Expand the comment to clearly state that the vault automatically includes
underlyingAssetin its whitelist, or- Add an assertion that
vaultWhitelistactually containsunderlyingAsset(in addition to the length check) to make the implicit behavior explicit.
336-354: Good coverage fork > nandk = 1; consider adding a weight-sum assertionThese tests correctly:
- Update
kto 6 when there are only 5 whitelisted assets (including the underlying), callsubmitIntent, and assert that all 5 assets are selected.- Update
kto 1, callsubmitIntent, and assert that only the highest‑TVL asset is selected with 100% allocation.To further lock in the invariant that weights always sum to
10^curatorIntentDecimalsacross these corner cases (not just in the separate weight-distribution test), you might also add an explicittotalWeightsum check here, similar to the “correct weight distribution” test.Also applies to: 359-373
428-447: Push-model semantics: ensure tests always callsubmitIntentwhen asserting behavior after parameter changesYou correctly added
submitIntent(transparentVault)calls afterupdateParametersin the"Strategy Parameter Updates"test, which is necessary under the push-based model.However, in
"should maintain valid intent weights after parameter changes"you still:for (let k = 1; k <= 4; k++) { await strategy.connect(curator).updateParameters(k); const [_tokens, weights] = await transparentVault.getIntent(); // ... }without re-submitting an intent. Under the new model, this loop is only checking whatever intent was last pushed, not the updated
kconfigurations the description refers to.Suggest either:
- Adding
await strategy.connect(curator).submitIntent(transparentVault);inside the loop, or- Renaming the test to reflect that it’s checking “stored intent always has valid weights” rather than change-after-parameter-updates.
Also applies to: 476-492
453-463: Whitelist length expectation reflects underlying presence; consider asserting the curator behavior tooChanging the expectation to
expect(whitelist.length).to.equal(3);aligns with the expanded whitelist (two assets plus the underlying). Since this test is meant to validate strategy behavior on whitelist updates when the curator is a strategy, you might also assert that the current curator is still the strategy (or that intent remains valid) to fully exercise the “validation” semantics implied by the describe block name.
263-267: Update comments and simplify interface ID computationThe comment on line 265 is stale—it mentions "XOR all three selectors," but the code only computes a single selector and performs no XOR operation. Additionally, the
BigIntconversion and manual hex formatting add unnecessary complexity.Since EIP-165 defines interface identifiers as the XOR of all function selectors, and for a single-function interface the ID equals that selector, the computation can be simplified:
const interfaceId = ethers.id("submitIntent(address)").slice(0, 10); expect(await strategy.supportsInterface(interfaceId)).to.be.true;This keeps the test correct while making it easier to maintain.
contracts/strategies/KBestTvlWeightedAverage.sol (2)
37-52:submitIntentflow is coherent with push-based curator designThe new
submitIntent(IOrionTransparentVault vault):
- Reads
vault.vaultWhitelist().- Computes TVLs (with graceful fallback) and selects top‑K.
- Builds
IntentPosition[]and forwards it viavault.submitIntent(intent).This is clean and keeps the strategy stateless with respect to intents. One small suggestion: consider validating that
address(vault) != address(0)(and optionally thatconfigmatches the vault’s config, if exposed) to fail fast on mis-wiring rather than propagating more obscure downstream errors.
59-72: TVL fallback ontotalAssets()failure is reasonable; consider making the “dust” value configurableThe try/catch around
IERC4626.totalAssets()that assignstvls[i] = 1on failure:
- Prevents a single misbehaving asset (or non‑ERC4626 like the underlying) from reverting the entire strategy.
- Ensures such assets are least favored in selection when
k < n.This matches the tests’ expectations. If you want more flexibility long term, you could expose the fallback “dust TVL” as a constant or config parameter instead of hard‑coding
1, but that’s a minor improvement, not a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(4 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(1 hunks)artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json(3 hunks)artifacts/contracts/interfaces/IOrionStrategy.sol/IOrionStrategy.json(1 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(2 hunks)artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json(1 hunks)contracts/OrionConfig.sol(3 hunks)contracts/factories/TransparentVaultFactory.sol(1 hunks)contracts/interfaces/IOrionConfig.sol(1 hunks)contracts/interfaces/IOrionStrategy.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(0 hunks)contracts/strategies/KBestTvlWeightedAverage.sol(4 hunks)contracts/test/KBestTvlWeightedAverageInvalid.sol(1 hunks)contracts/vaults/OrionTransparentVault.sol(4 hunks)test/MinimumAmountDOS.test.ts(1 hunks)test/OrionConfigVault.test.ts(4 hunks)test/PassiveCuratorStrategy.test.ts(9 hunks)test/RedeemBeforeDepositOrder.test.ts(1 hunks)test/Removal.test.ts(1 hunks)test/TransparentVault.test.ts(1 hunks)test/orchestrator/OrchestratorConfiguration.test.ts(3 hunks)test/orchestrator/OrchestratorPerformUpkeep.test.ts(2 hunks)test/orchestrator/OrchestratorSecurity.test.ts(2 hunks)test/orchestrator/Orchestrators.test.ts(4 hunks)test/orchestrator/OrchestratorsZeroState.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- contracts/orchestrators/InternalStatesOrchestrator.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). (2)
- GitHub Check: Sourcery review
- GitHub Check: Build, Lint and Test
🔇 Additional comments (28)
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
792-793: Bytecode changes are expected recompilation output.This artifact file reflects the recompilation of LiquidityOrchestrator after source code modifications in this PR. The bytecode and deployedBytecode have been updated (lines 792–793), while the ABI structure and public interface remain unchanged. The contract's function signatures, events, and error definitions are all preserved, confirming the recompilation is sound. No action needed.
artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json (1)
168-169: Bytecode update follows recompilation—verify source contract alignment.Since the AI summary states that only bytecode and deployedBytecode changed while the ABI and function signatures remain unchanged, this is a safe recompilation. However, compiled artifacts should be verified to ensure they correspond to the intended source changes introduced in this PR.
Please confirm that:
- The source contract
OrionAssetERC4626ExecutionAdapter.solwas recompiled with the correct compiler version and settings (as specified in the repo's build config).- The contract's public interface (ABI) is stable and backwards compatible, which the unchanged ABI confirms.
- Any changes to the underlying source were intentional and align with the PR's transition to the push-based active curator model.
If you'd like, I can generate a verification script to check that the artifact was produced by a clean recompilation of the source, or inspect related contracts to ensure consistency.
artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json (1)
213-214: Artifact recompilation - LGTM!Bytecode updated due to recompilation. No ABI changes, consistent with other contracts being updated in this PR.
artifacts/contracts/OrionConfig.sol/OrionConfig.json (1)
246-258: Curator whitelisting ABI additions - LGTM!The artifact correctly exposes the new curator whitelist management functions:
addWhitelistedCurator,isWhitelistedCurator, andremoveWhitelistedCurator. These align with the PR's objective to introduce curator whitelisting as part of the transition from passive to active curator model.Also applies to: 464-482, 606-618, 815-816
artifacts/contracts/interfaces/IOrionStrategy.sol/IOrionStrategy.json (1)
7-18: Strategy interface simplification - LGTM!The interface correctly reflects the architectural shift from passive to active curator model. The new
submitIntent(IOrionTransparentVault vault)function replaces the previous multi-function approach (computeIntent/getStatefulIntent/validateStrategy), streamlining strategy-vault interaction.test/MinimumAmountDOS.test.ts (1)
62-62: Curator whitelisting setup - LGTM!Properly whitelists the curator before vault creation, aligning with the new curator access control model introduced in this PR.
test/Removal.test.ts (1)
156-157: Curator whitelisting setup - LGTM!Correctly whitelists the curator in test setup, maintaining consistency with the new access control requirements across the test suite.
test/TransparentVault.test.ts (1)
130-131: Curator whitelisting setup - LGTM!Curator is properly whitelisted in the test setup before vault operations, ensuring tests align with the new curator access control model.
artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json (1)
104-105: Artifact recompilation - LGTM!Bytecode updated due to recompilation. No ABI changes detected.
test/RedeemBeforeDepositOrder.test.ts (1)
170-171: Curator whitelisting setup - LGTM!Curator whitelisting properly added to test setup, ensuring the test suite remains aligned with the new curator access control model.
test/orchestrator/OrchestratorsZeroState.test.ts (3)
77-78: LGTM: Curator whitelisting before vault creation.The addition of curator whitelisting aligns with the new access control model introduced in this PR. The curator is properly whitelisted before vault creation, which is required by the updated
TransparentVaultFactoryvalidation.
334-335: LGTM: Curator whitelisting in test setup.The curator is properly whitelisted before vault creation operations, ensuring compliance with the new access control requirements.
453-453: Ignore this review comment—the file reference and line numbers are incorrect.The test file
test/orchestrator/OrchestratorsZeroState.test.tscontains only 150 lines, making the reference to line 453 impossible. The code snippet shown is empty. No test for a "passive vault curator update flow" orupdateCuratoroperation exists in this file. The file contains only two test cases related to upkeep execution with zero TVL and intents, with curator used only in setup and intent submission.While the contract implementation (contracts/vaults/OrionTransparentVault.sol lines 155–159) does properly validate curator whitelist via
config.isWhitelistedCurator()before updating, the test location specified in this review comment does not correspond to any actual code changes in the current codebase.Likely an incorrect or invalid review comment.
artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json (1)
47-59: LGTM: Curator whitelist management ABI entries.The artifact correctly exposes three new curator whitelist management functions:
addWhitelistedCurator(address): nonpayable function to add curatorsisWhitelistedCurator(address): view function to check whitelist statusremoveWhitelistedCurator(address): nonpayable function to remove curatorsThese entries are consistent with the interface changes described in the PR summary.
Also applies to: 265-283, 394-406
test/orchestrator/OrchestratorPerformUpkeep.test.ts (1)
321-322: LGTM: Curator and strategy whitelisting in test setup.Both the human curator and the strategy contract are properly whitelisted before vault creation and configuration. This ensures the test setup complies with the new whitelist-based authorization model.
Also applies to: 418-419
test/OrionConfigVault.test.ts (3)
131-132: LGTM: Curator whitelisting in test setup.The curator is properly whitelisted in the
beforeEachsetup, ensuring all tests have a valid whitelisted curator available for vault operations.
429-448: Excellent test coverage for curator whitelist validation.The updated tests properly cover both positive and negative scenarios:
- Whitelisting curator before successful update (lines 432-438)
- Verifying rejection of non-whitelisted curator (lines 441-448)
This ensures the whitelist authorization is properly enforced.
459-464: Improved error handling: Zero address now UnauthorizedAccess.Changing the error from
InvalidAddresstoUnauthorizedAccessfor zero address is semantically correct, as the zero address is not whitelisted and therefore unauthorized. This provides consistent error handling across all unauthorized curator scenarios.test/orchestrator/OrchestratorConfiguration.test.ts (2)
311-312: LGTM: Curator and strategy whitelisting in test setup.Both the human curator and the strategy contract are properly whitelisted before vault creation, consistent with the new authorization model.
Also applies to: 408-409
431-431: All verification criteria confirmed - no issues found.The implementation correctly supports the push-based active curator model:
✓ Access control on submitIntent: The
onlyCuratormodifier at./contracts/vaults/OrionTransparentVault.sol:63enforces that only the vault's assigned curator can callsubmitIntent(). The modifier checksif (msg.sender != curator) revert ErrorsLib.UnauthorizedAccess().✓ Vault validates curator identity: The onlyCurator modifier performs a direct identity check (
msg.sender != curator), ensuring the calling strategy is confirmed as the vault's assigned curator.✓ Idempotent and safe to retry: The portfolio intent submission is idempotent by design—the function clears the previous intent (
_portfolioIntent.clear()at line 66) and replaces it atomically with the new one, making repeated submissions safe.The test suite validates this behavior, particularly at
test/TransparentVault.test.ts:363-378, which confirms non-curator calls are properly rejected.artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json (1)
1-182: LGTM: Test artifact for invalid strategy.This artifact provides a test-only strategy contract that generates invalid intents (weights not summing correctly). This is useful for negative testing to ensure the system properly validates and rejects invalid intents with
InvalidTotalWeighterror.The presence of this test artifact indicates good test coverage for edge cases and error handling.
contracts/interfaces/IOrionConfig.sol (1)
115-128: Curator whitelist interface matches existing whitelist patternsThe new curator whitelist methods mirror the existing vault‑owner whitelist API (ownership‑gated mutators and a simple view), so they slot cleanly into the config interface without changing existing call semantics.
test/orchestrator/Orchestrators.test.ts (1)
153-153: Whitelisting strategy/curator and pushing initial intent look correctWhitelisting both the KBest strategy and the human curator before vault creation/update, and then having the strategy push its intent to
passiveVault, is consistent with the new curator ACL and push‑based intent model. Callers used here (default signer /owner) align with Ownable/ACL expectations.Also applies to: 253-253, 371-371
contracts/OrionConfig.sol (1)
63-63: Curator whitelist storage and API integrate cleanly with configIntroducing
whitelistedCurators, seeding it withinitialOwner, and exposing add/remove/is functions that mirror the vault‑owner whitelist keeps ACLs consistent and straightforward to reason about. The error semantics (AlreadyRegistered/InvalidAddress) also match existing patterns.Also applies to: 113-115, 258-275
artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (1)
119-131: ABI/bytecode update forsubmitIntentlooks consistentThe added ABI entry for
submitIntent(IOrionTransparentVault vault)(nonpayable, no return) aligns with the new strategy interface, and the regenerated bytecode/deployedBytecode correspondingly reflect the implementation change. Nothing stands out as inconsistent here.Also applies to: 178-179
test/PassiveCuratorStrategy.test.ts (1)
198-199: Curator whitelisting and strategy setup verified as correctThe push-based model implementation with curator whitelisting, vault curator update, and initial
submitIntentcall is confirmed at lines 249-252 and aligns with the pattern throughout the test suite. Subsequent tests consistently callsubmitIntentafterupdateParameters(lines 338-339, 358-359, 431-432, 438-439, 445-446, etc.), confirming that the codebase properly handles refreshing stored intents when parameters change.test/orchestrator/OrchestratorSecurity.test.ts (2)
334-334: LGTM - Curator whitelisting properly integrated.The curator is correctly whitelisted after configuration and before vault creation, ensuring the curator can submit intents later in the test setup. This aligns with the new push-based curator model.
431-453: LGTM - Strategy-as-curator architecture properly implemented.The KBest TVL strategy is correctly whitelisted as a curator before being assigned to the passive vault. This change, combined with the removal of revert expectations (per the AI summary), properly implements the transition from a pull-based passive curator model to a push-based active curator model where strategies can submit intents directly to vaults.
The setup flow is correct:
- Whitelist strategy as curator (line 431)
- Create passive vault (lines 434-451)
- Update vault curator to strategy (line 453)
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 (1)
contracts/vaults/OrionTransparentVault.sol (1)
176-205: Add guard to prevent removal of the underlying asset from vault whitelist (contracts/vaults/OrionTransparentVault.sol, line 186)The review comment is accurate.
removeFromVaultWhitelistcurrently lacks any protection against removingthis.asset(), which breaks the invariant established byupdateVaultWhitelist(lines 176–178) that the underlying asset must always remain whitelisted. The function unconditionally removes any asset passed to it, then reassigns removed weight to the underlying asset—creating a scenario where the underlying is referenced in_portfolioIntentbut absent from_vaultWhitelistedAssets.Verification confirms no
requireorrevertguard exists to preventasset == this.asset(). Git history indicates a prior fix attempt (commit 74ce08e), but it is not present in the current codebase.Add validation to either revert when
asset == this.asset()or re-add the underlying to the whitelist after removal to preserve the invariant.
♻️ Duplicate comments (1)
test/orchestrator/Orchestrators.test.ts (1)
688-689: Give the long “performUpkeep” scenario test a descriptive nameThis
it("")block is still using an empty string as its description, which makes the suite harder to scan and maintain. Please rename it to something that reflects the scenario (e.g.,"full end-to-end epoch, fees, and buffer management").
🧹 Nitpick comments (8)
contracts/OrionConfig.sol (1)
63-64: Curator whitelist implementation is consistent; consider optional zero-address guardThe new
whitelistedCuratorsset, constructor seeding withinitialOwner, and the add/remove/isWhitelistedCurator helpers mirror the existing vault-owner whitelist and look correct from an access-control standpoint (onlyOwner, clear error types).If you want extra safety against accidental configuration, you could optionally reject
address(0)inaddWhitelistedCurator, similar to how other parts of the config guard against zero addresses, but it’s not strictly required for correctness.Also applies to: 113-115, 258-275
test/PassiveCuratorStrategy.test.ts (2)
264-267: Interface ID test works but the comment and computation could be simplifiedFor a single‑function interface, the EIP‑165 interface ID is just that function’s selector, so your test is logically fine. The comment about XOR-ing “all function selectors” is now stale, and the BigInt round‑trip is unnecessary.
You can simplify and clarify by e.g.:
const interfaceId = "0x" + ethers.id("submitIntent(address)").slice(2, 10); expect(await strategy.supportsInterface(interfaceId)).to.be.true;which directly reflects the interface definition and avoids the extra conversion steps.
301-318: Updated whitelist and intent expectations align with underlying-always-whitelisted behaviorThe changes to expect:
vaultWhitelist.length === 5after whitelisting 4 ERC4626 assets,- intent tokens excluding the underlying asset in the “top‑3 by TVL” case,
- and including the underlying asset when
kexceeds the number of ERC4626 assets,are consistent with a model where the underlying asset is always part of the vault’s investment universe but only appears in the strategy’s intent when appropriate. The adjusted whitelist length check in the strategy‑validation test (
length === 3after whitelisting 2 assets) also fits this pattern.If you want slightly stronger assertions, you could additionally assert that
vaultWhitelistexplicitly contains the underlying asset where you currently only checklength.Also applies to: 337-347, 353-354, 459-463
contracts/vaults/OrionTransparentVault.sol (2)
14-25: Update docstring: vault no longer supports passive curator strategiesThe header comment still describes both “active” and “passive” management with automatic curator-type detection, but the passive-curator path and related state have been removed. Consider updating this description to match the active-only, push‑intent design to avoid misleading integrators.
155-159: Stricter curator updates: only whitelisted curators allowedRequiring
config.isWhitelistedCurator(newCurator)inupdateCuratortightens access control, which is good. It does, however, mean:
- You can no longer “clear” the curator by setting it to
address(0).- All curator rotations must be pre‑whitelisted at the config level.
If this is the intended operational model, it’s fine; otherwise you may want an explicit way to unset the curator (e.g., a dedicated deactivation path) or treat
address(0)as a special case.contracts/test/KBestTvlWeightedAverageInvalid.sol (1)
19-53: Invalid strategy behaves as intended; consider documenting/guarding zero‑TVL edge caseThis test‑only strategy cleanly mirrors the production K‑best implementation while intentionally omitting the final weight‑normalization step, which matches the contract’s purpose.
One edge case to be aware of: if all selected TVLs are zero,
totalTVLstays0and the division in Line 130 will revert. That may be perfectly acceptable for a negative‑path test helper; if you ever reuse this pattern elsewhere, consider either:
- Adding an explicit
require(totalTVL > 0, ...), or- Falling back to equal weights when
totalTVL == 0.Also applies to: 115-138
contracts/strategies/KBestTvlWeightedAverage.sol (2)
59-73:_getAssetTVLs: dust fallback behavior is reasonable but slightly biases rankingsThe try/catch approach with a fallback TVL of
1for assets that revert ontotalAssets()avoids a single bad asset taking down the whole strategy, which is desirable. The tradeoff is that:
- Non‑ERC4626 or misbehaving assets are given a small but non‑zero TVL and may still be selected if all “good” assets also have very low TVL.
- This bias may or may not be acceptable depending on how strictly you intend to treat non‑ERC4626 assets in the vault whitelist.
If the design goal is “best effort” under partial failures, this is fine; otherwise you might consider:
- Assigning such assets TVL
0and explicitly reverting whentotalTVL == 0, or- Excluding them from the candidate set entirely.
133-135: Remainder allocation tointent[0]is fine; consider making the choice explicit in docsAdjusting
intent[0].weightto absorb the rounding remainder ensures the sum of weights matches the intent scale. That’s a sensible choice; it just implicitly favors the first selected asset.If the intended behavior is “round to the first ranked asset,” consider briefly documenting this in the NatSpec for
_calculatePositionsor the strategy comment, so downstream users know which asset gets the residual weight.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(4 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(1 hunks)artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json(3 hunks)artifacts/contracts/interfaces/IOrionStrategy.sol/IOrionStrategy.json(1 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(2 hunks)artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json(1 hunks)contracts/OrionConfig.sol(3 hunks)contracts/factories/TransparentVaultFactory.sol(1 hunks)contracts/interfaces/IOrionConfig.sol(1 hunks)contracts/interfaces/IOrionStrategy.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(0 hunks)contracts/strategies/KBestTvlWeightedAverage.sol(4 hunks)contracts/test/KBestTvlWeightedAverageInvalid.sol(1 hunks)contracts/vaults/OrionTransparentVault.sol(4 hunks)test/MinimumAmountDOS.test.ts(1 hunks)test/OrionConfigVault.test.ts(4 hunks)test/PassiveCuratorStrategy.test.ts(9 hunks)test/RedeemBeforeDepositOrder.test.ts(1 hunks)test/Removal.test.ts(1 hunks)test/TransparentVault.test.ts(1 hunks)test/orchestrator/OrchestratorConfiguration.test.ts(3 hunks)test/orchestrator/OrchestratorPerformUpkeep.test.ts(2 hunks)test/orchestrator/OrchestratorSecurity.test.ts(2 hunks)test/orchestrator/Orchestrators.test.ts(4 hunks)test/orchestrator/OrchestratorsZeroState.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- contracts/orchestrators/InternalStatesOrchestrator.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: Sourcery review
🔇 Additional comments (34)
artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json (1)
213-214: Artifact file updated — verify this is intentional recompilation.The
bytecodeanddeployedBytecodehex strings have been replaced, consistent with a contract recompilation. The ABI and contract interface remain unchanged, which is expected.Note: Compiled artifacts are typically generated during the build process and excluded from version control. If this project uses hardhat artifacts in the repository, ensure your build/CI pipeline regenerates these automatically to prevent drift between source and compiled output.
artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json (1)
168-169: Expected artifact regeneration from contract compilation.The bytecode and deployedBytecode have been updated due to changes in the underlying Solidity source code, which is expected. The ABI remains unchanged, indicating no breaking changes to the contract's public interface.
contracts/factories/TransparentVaultFactory.sol (1)
46-46: LGTM: Curator whitelist validation correctly enforced.The change from zero-address validation to whitelist checking properly implements the new curator access control model. The error type
UnauthorizedAccessis appropriate.Note: This is a breaking change—existing curator addresses must be whitelisted via
OrionConfig.addWhitelistedCurator()before vault creation.contracts/interfaces/IOrionConfig.sol (1)
115-128: LGTM: Curator whitelisting API follows established patterns.The three new functions mirror the existing vault owner whitelisting pattern and provide complete access control functionality: add, remove, and check curator whitelist status.
test/MinimumAmountDOS.test.ts (1)
62-62: LGTM: Test setup correctly updated for curator whitelisting.The curator whitelisting is properly added to the fixture setup before vault creation, ensuring compatibility with the new validation requirements.
test/RedeemBeforeDepositOrder.test.ts (1)
170-170: LGTM: Test setup correctly whitelists curator.The curator whitelisting is properly positioned in the setup flow after asset whitelisting and before vault creation.
test/TransparentVault.test.ts (1)
131-131: LGTM: Global test setup correctly whitelists curator.The curator whitelisting in the global beforeEach hook ensures all test cases in this suite can successfully create vaults with the new validation requirements.
artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json (1)
104-105: Artifact bytecode changed without source or compiler config modifications.The bytecode and deployedBytecode have changed, but the source contract and all compiler settings remain unchanged. The IPFS metadata hash embedded in the bytecode differs, indicating the artifact was regenerated differently. This appears to be either an unintended artifact regeneration or an accidental modification. Since the source code and ABI are identical, confirm whether this change was intentional or should be reverted to match the origin/main artifact.
test/orchestrator/OrchestratorsZeroState.test.ts (1)
77-78: LGTM! Curator whitelisting properly integrated into test setup.The addition of
addWhitelistedCurator(curator.address)in thebeforeEachblock correctly establishes the curator whitelist before vault creation, aligning with the new curator access control requirements introduced in this PR.test/orchestrator/OrchestratorPerformUpkeep.test.ts (2)
321-321: LGTM! Curator whitelisting correctly positioned.The curator is properly whitelisted after asset configuration and before vault creation, ensuring the curator can submit intents to the vaults in subsequent test scenarios.
418-418: LGTM! Strategy contract whitelisted as curator.Whitelisting the
KBestTvlWeightedAveragestrategy contract as a curator is appropriate, as it needs curator permissions to callsubmitIntenton the passive vault (see line 440 whereupdateCuratoris called with the strategy address).artifacts/contracts/interfaces/IOrionConfig.sol/IOrionConfig.json (1)
47-59: LGTM! Curator whitelisting ABI entries are well-formed.The three new ABI entries (
addWhitelistedCurator,isWhitelistedCurator,removeWhitelistedCurator) follow the established pattern of the existing asset and vault owner whitelisting APIs, maintaining interface consistency.Also applies to: 265-283, 394-406
test/Removal.test.ts (1)
156-157: LGTM! Curator whitelisting correctly integrated.The curator is properly whitelisted in the setup phase, ensuring the removal flow tests can execute curator operations (like
submitIntenton line 198) without authorization failures.test/orchestrator/OrchestratorConfiguration.test.ts (3)
311-311: LGTM! Curator whitelisting properly positioned.The curator is whitelisted before vault operations begin, consistent with the new access control requirements.
408-408: LGTM! Strategy contract whitelisted as curator.Whitelisting the
KBestTvlWeightedAveragestrategy enables it to act as the passive vault's curator (line 430) and submit intents (line 431).
431-431: LGTM! New submitIntent flow correctly implemented.The call to
kbestTvlStrategy.connect(owner).submitIntent(passiveVault)demonstrates the new simplified strategy interface where the strategy directly submits its computed intent to the vault, replacing the previous multi-function approach.artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json (1)
1-182: LGTM! Test strategy contract artifact properly structured.The
KBestTvlWeightedAverageInvalidartifact correctly implements the newsubmitIntent(IOrionTransparentVault vault)interface method and includes appropriate constructor validation (ZeroAddress check). This test contract enables validation of invalid weight scenarios in the test suite.test/OrionConfigVault.test.ts (6)
131-131: LGTM! Curator whitelisting added to test setup.The
addWhitelistedCurator(curator.address)call inbeforeEachensures the curator is whitelisted for all tests, establishing the proper access control baseline.
429-439: LGTM! Test updated to validate whitelisted curator flow.The test correctly renames to emphasize "whitelisted curator" and adds the whitelisting step (lines 432-433) before attempting the curator update, ensuring the operation succeeds.
441-448: LGTM! New test validates non-whitelisted curator rejection.This new test appropriately validates that attempting to update to a non-whitelisted curator results in an
UnauthorizedAccesserror, ensuring the whitelisting requirement is enforced.
459-464: Note: Error message changed for zero address curator.The expected error for setting curator to zero address changed from
InvalidAddresstoUnauthorizedAccess(line 462). This suggests zero address is now treated as "not whitelisted" rather than explicitly invalid, which is semantically consistent with the whitelisting approach.
466-474: LGTM! Event test updated with whitelisting prerequisite.The curator is properly whitelisted (lines 469-470) before the update that triggers the
CuratorUpdatedevent, ensuring the test validates the complete happy path.
500-517: LGTM! Access control test updated with whitelisting.The test properly whitelists
other.addressas a curator (lines 513-514) before the owner attempts to callupdateCurator, ensuring the access control validation focuses on the owner check rather than the whitelisting check.contracts/interfaces/IOrionStrategy.sol (1)
6-15: LGTM! Strategy interface successfully simplified.The replacement of the multi-function interface (
computeIntent,validateStrategy,getStatefulIntent) with a singlesubmitIntent(IOrionTransparentVault vault)method is a significant improvement:
- Push vs. Pull: Shifts from a pull model (compute and return) to a push model (compute and submit directly)
- Encapsulation: Strategy now controls the submission flow internally
- Reduced complexity: Single method is easier to implement and test
- State access: Strategy can query vault state directly through the vault reference
The change is well-coordinated across tests and implementations throughout the PR.
test/orchestrator/OrchestratorSecurity.test.ts (1)
334-335: Curator whitelisting in security setup aligns tests with new protocol rulesWhitelisting both the human curator and the KBest strategy in the
beforeEachkeeps these security tests compatible with the new curator whitelist requirement without changing their intent. The placement (before vault creation / curator update) looks correct.Also applies to: 431-432
test/orchestrator/Orchestrators.test.ts (1)
153-154: Whitelisting of curators and strategy plus passive-vault intent submission looks correctRegistering both the curator EOA and the
KBestTvlWeightedAveragecontract as whitelisted curators before creating vaults and updating the passive vault’s curator, then explicitly callingsubmitIntent(passiveVault), matches the new curator‑whitelisting plus strategy‑driven intent flow and should keep these orchestration tests representative of real usage.Also applies to: 253-254, 371-372
test/PassiveCuratorStrategy.test.ts (3)
198-199: Curator/strategy whitelisting and initial intent submission are wired correctlyWhitelisting the curator EOA before vault creation, then whitelisting the strategy contract address before
updateCuratorand immediately callingstrategy.submitIntent(transparentVault)ensures:
OrionConfig’s curator whitelist constraint is satisfied for both the initial curator and the strategy.- The vault has a populated intent before you start orchestrator‑driven flows or deposits.
This is the right sequencing for the new curator‑gated model.
Also applies to: 249-253
433-440: CallingsubmitIntentafter each k-parameter change keeps intent state in syncIn the “Strategy Parameter Updates” test, invoking
submitIntent(transparentVault)after eachupdateParameterscall ensures the intent stored on the vault reflects the currentkvalue before you read it back viagetIntent(). This is important under the new submit‑only strategy interface and looks correctly sequenced.Also applies to: 446-447
18-19: Negative test withKBestTvlWeightedAverageInvalidis a good regression guardImporting
KBestTvlWeightedAverageInvalidand using it to:
- deploy a misbehaving strategy,
- attach it to a fresh vault with a normal whitelist,
- and then assert that
submitIntentreverts withInvalidTotalWeight,gives you a solid regression test that the vault enforces the “weights sum to intentScale” invariant regardless of strategy implementation. The flow (whitelisting the invalid strategy, updating curator, then attempting to submit intent) matches how a real misconfigured strategy would be exercised.
Also applies to: 495-554
artifacts/contracts/interfaces/IOrionStrategy.sol/IOrionStrategy.json (1)
9-15: ABI correctly reflects the new single-method IOrionStrategy interfaceThe artifact now exposes only
submitIntent(IOrionTransparentVault vault)with anaddress-encoded parameter and no return values, which matches the described interface refactor away from compute/getStateful/validate methods. This should keep typechain and runtime calls aligned with the on-chain interface.artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (2)
119-131: ABI entry forsubmitIntentmatches source contract expectationsThe new ABI fragment for
submitIntent(IOrionTransparentVault vault)looks consistent (singleaddress-typed vault parameter, nonpayable, no outputs) with the updated strategy interface.
178-179: Bytecode updates are expected from logic/interface changesThe
bytecodeanddeployedBytecodechanges reflect the newsubmitIntententry point and helper logic; nothing stands out as anomalous at the artifact level.contracts/vaults/OrionTransparentVault.sol (1)
56-58: Constructor intent seeding: confirmcuratorIntentDecimalsscale is consistentSeeding
_portfolioIntentto 100% underlying (10 ** config.curatorIntentDecimals()) is a good default and matches the decommissioning behavior ingetIntent(). Just ensureconfig.curatorIntentDecimals()remains within a range that fits safely into theuint32weights used elsewhere and is consistent with the intent scale assumed by strategies.contracts/test/KBestTvlWeightedAverageInvalid.sol (1)
146-149:supportsInterfaceoverride correctly advertisesIOrionStrategyThe ERC165 override correctly returns true for
IOrionStrategy’s interface ID and delegates tosuper.supportsInterfacefor the rest, consistent with how the main strategy is declared.
1 participant
Summary by Sourcery
Refactor passive curation to a push-based intent submission model and enforce explicit curator whitelisting across vault and config modules.
New Features:
Enhancements:
Tests:
Chores:
Summary by CodeRabbit
New Features
Changes