Open
Conversation
…logic - Updated IOrionStrategist to inherit from IERC165 and added setVault function for linking to a vault. - Modified submitIntent to operate on the linked vault. - Introduced error handling for vault linking in ErrorsLib. - Updated KBestTvlWeightedAverage and KBestTvlWeightedAverageInvalid contracts to reflect new interface changes. - Implemented vault linking logic in OrionVault to ensure strategists are correctly associated with their vaults.
- Added comprehensive tests for strategist assignment, including scenarios for EOA, non-ERC165, and ERC165 non-IOrionStrategist contracts. - Enhanced the test suite for vault creation and strategist linking logic.
…t and KBestApy variants
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIOrionStrategist now requires ERC‑165, adds a single‑time setVault(address), and moves to a parameterless submitIntent(). Added ApyStrategistBase and three strategist implementations (EqualWeight, KBest variants). Vaults probe ERC‑165 strategists and auto‑link them; tests and mocks updated for linking, checkpoints, and intent math. Changes
Sequence Diagram(s)sequenceDiagram
participant Deployer
participant Vault as OrionTransparentVault
participant Strategist as IOrionStrategist
participant Introspect as IERC165
Deployer->>Vault: initialize(..., strategist, ...)
Vault->>Vault: set initial portfolioIntent (underlying = 100%)
Vault->>Introspect: supportsInterface(IOrionStrategist)
Introspect-->>Vault: true
alt strategist supports IOrionStrategist
Vault->>Strategist: setVault(this)
Strategist-->>Vault: acknowledged
end
Vault-->>Deployer: initialization complete
sequenceDiagram
participant Caller
participant Strategy as ApyStrategistBase
participant Config as IOrionConfig
participant Asset as ERC4626Asset
participant Vault as IOrionTransparentVault
Caller->>Strategy: updateCheckpoints([assets])
loop per asset
Strategy->>Asset: convertToAssets(1)
Asset-->>Strategy: sharePrice
Strategy->>Strategy: record checkpoint
end
Caller->>Strategy: submitIntent()
Strategy->>Config: getAllWhitelistedAssets()
Config-->>Strategy: assets
loop compute APYs
Strategy->>Asset: read current sharePrice / checkpoint
Strategy-->>Strategy: compute APY
end
Strategy->>Strategy: select top‑K, build IntentPositions
Strategy->>Vault: submitIntent(intentPositions)
Vault-->>Strategy: accepted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/PassiveStrategist.test.ts (2)
300-303:⚠️ Potential issue | 🔴 CriticalInconsistent
submitIntentcall uses old signature.This test still passes
transparentVaultas an argument tosubmitIntent, but the updatedKBestTvlWeightedAveragecontract now uses a parameterlesssubmitIntent(). This will either fail to compile (if TypeChain types are updated) or call a non-existent overload.🐛 Proposed fix
- await expect(passiveStrategist.connect(strategist).submitIntent(transparentVault)).to.be.revertedWithCustomError( + await expect(passiveStrategist.connect(strategist).submitIntent()).to.be.revertedWithCustomError( passiveStrategist, "OrderIntentCannotBeEmpty", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/PassiveStrategist.test.ts` around lines 300 - 303, The test is calling submitIntent with an outdated signature; update the call to use the new parameterless submitIntent() on the connected strategist instance (replace passiveStrategist.connect(strategist).submitIntent(transparentVault) with passiveStrategist.connect(strategist).submitIntent()), and search for any other usages of submitIntent(transparentVault) in the test suite to change them to the parameterless submitIntent() so the assertion using revertedWithCustomError(passiveStrategist, "OrderIntentCannotBeEmpty") still targets the correct call.
333-348:⚠️ Potential issue | 🟠 MajorTest does not call
submitIntent()afterupdateParameters.The loop updates
kviaupdateParametersbut then checksvault.getIntent()without callingsubmitIntent(). The vault's intent won't reflect the newkvalue untilsubmitIntent()is called, so this test is checking stale intent data frombeforeEach.🐛 Proposed fix
it("should maintain valid intent weights after parameter changes", async function () { // Test various k values to ensure weights always sum to 100% for (let k = 1; k <= 4; k++) { await passiveStrategist.connect(strategist).updateParameters(k); + await passiveStrategist.connect(strategist).submitIntent(); const [_tokens, weights] = await transparentVault.getIntent();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/PassiveStrategist.test.ts` around lines 333 - 348, The test updates parameters via passiveStrategist.connect(strategist).updateParameters(k) but then reads stale intent from transparentVault.getIntent() without applying the new parameters; call passiveStrategist.connect(strategist).submitIntent() (or the appropriate submitIntent method on passiveStrategist) after each updateParameters(k) and before transparentVault.getIntent() so the vault reflects the updated intent; ensure you await the submitIntent() call so the subsequent transparentVault.getIntent() returns the new weights and the summed totalWeight check is valid.contracts/strategies/KBestTvlWeightedAverage.sol (1)
131-136:⚠️ Potential issue | 🟡 MinorPotential overflow in weight calculation.
The calculation
uint32((topTvls[i] * intentScale) / totalTVL)can overflow iftopTvls[i] * intentScaleexceedstype(uint256).max. While unlikely with typical TVL values,KBestApyWeightedAverageusesMath.mulDivfor this same calculation, which handles overflow safely.🛡️ Proposed fix using mulDiv
for (uint16 i = 0; i < kActual; ++i) { - uint32 weight = uint32((topTvls[i] * intentScale) / totalTVL); + uint32 weight = uint32(Math.mulDiv(topTvls[i], intentScale, totalTVL)); intent[i] = IOrionTransparentVault.IntentPosition({ token: tokens[i], weight: weight }); sumWeights += weight; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/strategies/KBestTvlWeightedAverage.sol` around lines 131 - 136, The weight multiplication can overflow when computing uint32((topTvls[i] * intentScale) / totalTVL); replace the raw multiply/divide with a safe mulDiv call (as used in KBestApyWeightedAverage) to compute weight = uint32(Math.mulDiv(topTvls[i], intentScale, totalTVL)); update the assignment to intent[i] = IOrionTransparentVault.IntentPosition({ token: tokens[i], weight: weight }) and keep sumWeights accumulation unchanged; ensure Math.mulDiv is imported/available in this contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/interfaces/IOrionStrategist.sol`:
- Around line 12-17: The setVault(address vault_) function is currently
permissionless and allows third parties to pre-bind a strategist; change it so
only the vault itself can bind the strategist and add a cross-check to ensure
the vault acknowledges this strategist: require vault_ != address(0),
require(msg.sender == vault_) (or otherwise authenticate the caller as the
vault), and call the vault contract (e.g., IVault(vault_).strategist() or
equivalent) to assert it already points to this strategist (or that it expects
this strategist) before storing the vault and emitting the link; keep the
existing StrategistVaultAlreadyLinked behavior for idempotent calls from the
same vault.
In `@contracts/strategies/ApyStrategistBase.sol`:
- Around line 114-129: The NatSpec for _getAssetApy is misleading: the function
calculates a simple annualized return ((P1−P0)/P0 scaled to a year) not a
compounded APY; update the comment above function _getAssetApy to say it returns
a "simple annualized return (non‑compounded) in WAD" and mention it is used as
an APY proxy for ranking, so readers know compounding is not applied. Also keep
the existing behavior and names (_getAssetApy, cp.sharePrice, SECONDS_PER_YEAR,
WAD) unchanged—only adjust the documentation text to reflect “simple annualized
return” semantics.
In `@contracts/strategies/EqualWeight.sol`:
- Around line 43-45: The cast to uint16 for the asset count (uint16 n =
uint16(assets.length)) in EqualWeight.sol risks truncation when assets.length >
65,535; replace the uint16 cast with a uint256 counter (e.g., use uint256 n =
assets.length) or add a defensive require that assets.length <= type(uint16).max
before casting, and keep the existing empty-check revert
(ErrorsLib.OrderIntentCannotBeEmpty()) intact; update any loops or uses of n
(and related index variables) such as in functions referencing
assets/getAllWhitelistedAssets() to use the matching uint256 type.
In `@contracts/strategies/KBestApyEqualWeighted.sol`:
- Around line 27-30: The submitIntent function in KBestApyEqualWeighted is
implementing a declaration from IOrionStrategist (via ApyStrategistBase) but
lacks the required Solidity override specifier; update the function signature
for submitIntent() in KBestApyEqualWeighted to include the override keyword
(e.g., function submitIntent() external override) so it properly overrides the
interface method.
In `@test/NewStrategies.test.ts`:
- Around line 812-837: The test only sets APY state once (inside the hasApy && k
=== 1 branch) but then relies on that state for subsequent iterations, making it
fragile; change the loop so that for every iteration where hasApy is true you
run the APY setup (call strategy.updateCheckpoints(...), advancePastMinWindow(),
and simulateGains(...) each time) or explicitly reset state between iterations
so each k with hasApy=true performs its own checkpointing and gain simulation;
locate and modify the block around strategy.updateCheckpoints,
advancePastMinWindow, and simulateGains in the loop (and/or add a reset/teardown
before each iteration) to ensure APY data is established per-iteration rather
than only when k === 1.
- Around line 40-49: Add explicit assertions before using non-null assertions on
`event` and `parsed` in the helper inside NewStrategies.test.ts: after locating
`event` from `receipt?.logs` (via `factory.interface.parseLog`) assert `event`
is defined (e.g., `expect(event).toBeDefined()` or throw a descriptive error)
and then parse it into `parsed` and assert `parsed` and `parsed.args[0]` are
defined before calling `ethers.getContractAt("OrionTransparentVault",
parsed!.args[0])`; this replaces blind `event!`/`parsed!` usage with clear,
early failures and makes the test error messages informative.
In `@test/StrategistLinking.test.ts`:
- Around line 172-194: Add a test that covers the initialize-time linking path
by creating a fresh KBestTvlWeightedAverage strategy and passing it into
createVault() so the vault is initialized with the strategist (exercise
OrionTransparentVault.initialize()), then deposit TVL as in the existing test
and assert that strategy.connect(user).submitIntent() does not revert and
returns the expected intent (e.g., tokens length equals 2); mirror the setup
used in the current test (mintAndDeposit, underlyingAsset/assets, vault
getIntent) but omit the explicit vault.updateStrategist() call to verify
initialize-time linking works.
---
Outside diff comments:
In `@contracts/strategies/KBestTvlWeightedAverage.sol`:
- Around line 131-136: The weight multiplication can overflow when computing
uint32((topTvls[i] * intentScale) / totalTVL); replace the raw multiply/divide
with a safe mulDiv call (as used in KBestApyWeightedAverage) to compute weight =
uint32(Math.mulDiv(topTvls[i], intentScale, totalTVL)); update the assignment to
intent[i] = IOrionTransparentVault.IntentPosition({ token: tokens[i], weight:
weight }) and keep sumWeights accumulation unchanged; ensure Math.mulDiv is
imported/available in this contract.
In `@test/PassiveStrategist.test.ts`:
- Around line 300-303: The test is calling submitIntent with an outdated
signature; update the call to use the new parameterless submitIntent() on the
connected strategist instance (replace
passiveStrategist.connect(strategist).submitIntent(transparentVault) with
passiveStrategist.connect(strategist).submitIntent()), and search for any other
usages of submitIntent(transparentVault) in the test suite to change them to the
parameterless submitIntent() so the assertion using
revertedWithCustomError(passiveStrategist, "OrderIntentCannotBeEmpty") still
targets the correct call.
- Around line 333-348: The test updates parameters via
passiveStrategist.connect(strategist).updateParameters(k) but then reads stale
intent from transparentVault.getIntent() without applying the new parameters;
call passiveStrategist.connect(strategist).submitIntent() (or the appropriate
submitIntent method on passiveStrategist) after each updateParameters(k) and
before transparentVault.getIntent() so the vault reflects the updated intent;
ensure you await the submitIntent() call so the subsequent
transparentVault.getIntent() returns the new weights and the summed totalWeight
check is valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 370fa347-8591-4573-b141-cd9d708283aa
📒 Files selected for processing (15)
contracts/interfaces/IOrionStrategist.solcontracts/libraries/ErrorsLib.solcontracts/strategies/ApyStrategistBase.solcontracts/strategies/EqualWeight.solcontracts/strategies/KBestApyEqualWeighted.solcontracts/strategies/KBestApyWeightedAverage.solcontracts/strategies/KBestTvlWeightedAverage.solcontracts/test/KBestTvlWeightedAverageInvalid.solcontracts/test/MockERC165NonStrategist.solcontracts/test/MockNonERC165Contract.solcontracts/vaults/OrionTransparentVault.solcontracts/vaults/OrionVault.soltest/NewStrategies.test.tstest/PassiveStrategist.test.tstest/StrategistLinking.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/strategies/ApyStrategistBase.sol`:
- Around line 52-57: setVault currently allows any caller to set _vault once and
thus can be hijacked; replace this with a two-step, contract-only linking flow:
add proposeVault(address proposed) callable only by the strategist/admin that
sets a new storage variable proposedVault, and change setVault to be callable
only by the proposedVault contract itself (require(msg.sender == proposedVault
&& Address.isContract(msg.sender))) which then sets _vault and clears
proposedVault (and keep the existing zero-address checks and
StrategistVaultAlreadyLinked guard). Update ErrorsLib usage to validate and
revert appropriately when propose/accept flow is misused (e.g., no proposed
vault or already linked).
- Line 52: The function setVault in ApyStrategistBase.sol implements
IOrionStrategist.setVault but is missing the Solidity override specifier; update
the function declaration for setVault to include the override keyword (e.g.,
function setVault(address vault_) external override { ... }) so the compiler
recognizes it as implementing IOrionStrategist.setVault and matches the
interface signature.
- Around line 69-70: The loops in ApyStrategistBase (e.g., where `uint16 n =
uint16(assets.length); for (uint16 i = 0; i < n; ++i)`) silently truncate when
`assets.length` exceeds 65,535; update these to use `uint256` for the length and
loop index (replace `uint16 n` and `uint16 i` with `uint256`) throughout the
file (including the loop ranges at the locations noted and in
`_submitIntentInternal`), or alternatively add an explicit require that reverts
if `assets.length > type(uint16).max` and keep `uint16` if you must; also ensure
upstream `OrionConfig.getAllWhitelistedAssets()` does not cast to `uint16` (or
adds the same guard) so the length is consistent end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02f6fd32-ae2d-4dcc-ad09-834880e620ef
📒 Files selected for processing (1)
contracts/strategies/ApyStrategistBase.sol
Comment on lines
+68
to
+73
| function updateCheckpoints(address[] calldata assets) external { | ||
| uint16 n = uint16(assets.length); | ||
| for (uint16 i = 0; i < n; ++i) { | ||
| _recordCheckpoint(assets[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
The one-slot checkpoint design does not prevent reset griefing.
MIN_WINDOW only blocks a second write. After the window expires, anyone can call updateCheckpoints, overwrite the only stored sample, and make _getAssetApy() return 0 for the targeted assets until the next hour. That lets them skew top-K selection. Retain an older eligible checkpoint (or a short ring buffer), or restrict checkpoint writes to a trusted actor.
Also applies to: 84-95, 121-123
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/strategies/EqualWeight.sol`:
- Around line 31-36: The function setVault in EqualWeight implements
IOrionStrategist but lacks the required Solidity override specifier; update the
function signature to include override (e.g., change "function setVault(address
vault_) external {" to "function setVault(address vault_) external override {"
or "external override(IOrionStrategist)" if there are multiple inherited
interfaces) so the compiler recognizes this as the interface implementation
while leaving the body logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d3a9437-0155-4e15-bd7a-1b7c3f2ea7f1
📒 Files selected for processing (7)
contracts/strategies/ApyStrategistBase.solcontracts/strategies/EqualWeight.solcontracts/strategies/KBestApyEqualWeighted.solcontracts/strategies/KBestApyWeightedAverage.solcontracts/test/MockNoDecimalsAsset.soltest/NewStrategies.test.tstest/StrategistLinking.test.ts
Comment on lines
+31
to
+36
| function setVault(address vault_) external { | ||
| if (vault_ == address(0)) revert ErrorsLib.ZeroAddress(); | ||
| if (_vault == vault_) return; | ||
| if (_vault != address(0)) revert ErrorsLib.StrategistVaultAlreadyLinked(); | ||
| _vault = vault_; | ||
| } |
There was a problem hiding this comment.
Missing override specifier on setVault.
Since EqualWeight implements IOrionStrategist, the setVault function should include the override keyword. This is required by Solidity 0.8+ when implementing interface methods.
Proposed fix
- function setVault(address vault_) external {
+ function setVault(address vault_) external override {📝 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.
Suggested change
| function setVault(address vault_) external { | |
| if (vault_ == address(0)) revert ErrorsLib.ZeroAddress(); | |
| if (_vault == vault_) return; | |
| if (_vault != address(0)) revert ErrorsLib.StrategistVaultAlreadyLinked(); | |
| _vault = vault_; | |
| } | |
| function setVault(address vault_) external override { | |
| if (vault_ == address(0)) revert ErrorsLib.ZeroAddress(); | |
| if (_vault == vault_) return; | |
| if (_vault != address(0)) revert ErrorsLib.StrategistVaultAlreadyLinked(); | |
| _vault = vault_; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/strategies/EqualWeight.sol` around lines 31 - 36, The function
setVault in EqualWeight implements IOrionStrategist but lacks the required
Solidity override specifier; update the function signature to include override
(e.g., change "function setVault(address vault_) external {" to "function
setVault(address vault_) external override {" or "external
override(IOrionStrategist)" if there are multiple inherited interfaces) so the
compiler recognizes this as the interface implementation while leaving the body
logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/strategies/KBestApyWeightedAverage.sol`:
- Around line 25-27: Reject zero selections and guard before dividing/indexing:
validate and reject k == 0 in the constructor (KBestApyWeightedAverage) and in
updateParameters so the stored k cannot be set to 0; additionally, in the method
that computes kActual/intentScale/intent (the selection/weighting flow where
variables kActual, intentScale and intent are used) add a require/revert if
kActual == 0 before performing intentScale / kActual or indexing intent[0] to
avoid panics from division-by-zero or empty-array access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 92254ced-e1c9-4ae9-9f34-8904f4c271e5
📒 Files selected for processing (1)
contracts/strategies/KBestApyWeightedAverage.sol
Comment on lines
+25
to
+27
| constructor(address owner_, address config_, uint16 k_) ApyStrategistBase(owner_, config_) { | ||
| k = k_; | ||
| } |
There was a problem hiding this comment.
Reject empty selections before dividing and indexing.
If kActual is 0, Line 59 panics on intentScale / kActual, and Line 74 would dereference intent[0]. That state is reachable today because both the constructor and updateParameters accept k = 0, and an empty asset set can also propagate a zero kActual through the base flow.
Suggested fix
+error InvalidK();
+error NoAssetsSelected();
+
constructor(address owner_, address config_, uint16 k_) ApyStrategistBase(owner_, config_) {
+ if (k_ == 0) revert InvalidK();
k = k_;
}
function updateParameters(uint16 kNew) external onlyOwner {
+ if (kNew == 0) revert InvalidK();
k = kNew;
}
function _buildIntent(
address[] memory tokens,
uint256[] memory topApys,
uint16 kActual
) internal view override returns (IOrionTransparentVault.IntentPosition[] memory intent) {
+ if (kActual == 0) revert NoAssetsSelected();
uint32 intentScale = uint32(10 ** config.strategistIntentDecimals());
intent = new IOrionTransparentVault.IntentPosition[](kActual);Also applies to: 36-37, 42-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/strategies/KBestApyWeightedAverage.sol` around lines 25 - 27,
Reject zero selections and guard before dividing/indexing: validate and reject k
== 0 in the constructor (KBestApyWeightedAverage) and in updateParameters so the
stored k cannot be set to 0; additionally, in the method that computes
kActual/intentScale/intent (the selection/weighting flow where variables
kActual, intentScale and intent are used) add a require/revert if kActual == 0
before performing intentScale / kActual or indexing intent[0] to avoid panics
from division-by-zero or empty-array access.
Supports an optional quote feed for cross-rate normalisation (e.g. ETH/USD / USDC/USD to obtain ETH/USDC)
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
2 participants
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
Bug Fixes
Refactor
Tests