Dev - #122
Conversation
🛡️ 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. 📝 WalkthroughWalkthroughRenames and reworks ownership/role terminology: vault-owner → manager → strategist across configs, vaults, orchestrators, strategies, events and tests; replaces manager-intent/fees with strategist-intent/vault-fees; updates whitelist storage and APIs and adjusts access controls and initializer parameters accordingly. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Reviewer's GuideRefactors the protocol’s role and naming model from vault owner/manager/strategy to manager/strategist, renames manager-intent and fee concepts to strategist/vault-fee terminology, and wires these changes through core contracts (vaults, config, orchestrators, factory, liquidity orchestrator, strategist interfaces) and all affected tests while bumping the package version. Sequence diagram for vault fee accrual and claim with manager/strategist rolessequenceDiagram
actor Manager
participant Strategist
participant OrionTransparentVault as OrionTransparentVault
participant InternalStatesOrchestrator as InternalStatesOrchestrator
participant LiquidityOrchestrator as LiquidityOrchestrator
%% Epoch processing: compute and accrue vault fees
InternalStatesOrchestrator->>OrionTransparentVault: vaultFee(totalAssets)
OrionTransparentVault-->>InternalStatesOrchestrator: feeAmount
InternalStatesOrchestrator->>OrionTransparentVault: accrueVaultFees(feeAmountMinusProtocolShare)
OrionTransparentVault->>OrionTransparentVault: pendingVaultFees += feeAmountMinusProtocolShare
OrionTransparentVault-->>InternalStatesOrchestrator: VaultFeesAccrued event
%% Strategist submits allocation intent (if active strategist)
Strategist->>OrionTransparentVault: submitIntent(intent[])
OrionTransparentVault->>OrionTransparentVault: validateIntentAssets
OrionTransparentVault-->>Strategist: OrderSubmitted event
%% Manager claims accumulated vault fees
Manager->>OrionTransparentVault: claimVaultFees(amount)
OrionTransparentVault->>OrionTransparentVault: require(amount <= pendingVaultFees)
OrionTransparentVault->>OrionTransparentVault: pendingVaultFees -= amount
OrionTransparentVault->>LiquidityOrchestrator: transferVaultFees(amount)
LiquidityOrchestrator->>LiquidityOrchestrator: validateCallerIsVault
LiquidityOrchestrator->>Manager: transfer underlyingAsset(amount)
Updated class diagram for vault, config, orchestrators, and strategistclassDiagram
direction LR
%% === Interfaces ===
class IOrionVault {
+manager() address
+strategist() address
+pendingVaultFees() uint256
+vaultFee(totalAssets uint256) uint256
+claimVaultFees(amount uint256)
+updateStrategist(newStrategist address)
+updateVaultWhitelist(assets address[])
+updateFeeModel(mode uint8, performanceFee uint16, managementFee uint16)
+setDepositAccessControl(newDepositAccessControl address)
+accrueVaultFees(feeAmount uint256)
}
class IOrionConfig {
+strategistIntentDecimals() uint8
+addWhitelistedManager(manager address)
+removeWhitelistedManager(manager address)
+isWhitelistedManager(manager address) bool
}
class ILiquidityOrchestrator {
+transferVaultFees(amount uint256)
}
class IOrionTransparentVault {
<<interface>>
+submitIntent(intent IntentPosition[])
}
class IOrionStrategist {
<<interface>>
+submitIntent(vault IOrionTransparentVault)
}
%% === Vault implementation ===
class OrionVault {
<<abstract>>
+manager address
+strategist address
+config IOrionConfig
+internalStatesOrchestrator IInternalStateOrchestrator
+liquidityOrchestrator ILiquidityOrchestrator
+pendingVaultFees uint256
+depositAccessControl address
+__OrionVault_init(manager_ address, strategist_ address, config_ IOrionConfig, name_ string, symbol_ string, feeType_ uint8, performanceFee_ uint16, managementFee_ uint16, depositAccessControl_ address)
+vaultFee(activeTotalAssets uint256) uint256
+claimVaultFees(amount uint256)
+setDepositAccessControl(newDepositAccessControl address)
+accrueVaultFees(feeAmount uint256)
-onlyManager()
-onlyStrategist()
}
class OrionTransparentVault {
+_portfolio EnumerableMap_AddressToUintMap
+_portfolioIntent EnumerableMap_AddressToUintMap
+initialize(manager_ address, strategist_ address, config_ IOrionConfig, name_ string, symbol_ string, feeType_ uint8, performanceFee_ uint16, managementFee_ uint16, depositAccessControl_ address)
+submitIntent(intent IntentPosition[])
+updateStrategist(newStrategist address)
+updateVaultWhitelist(assets address[])
}
OrionVault <|-- OrionTransparentVault
IOrionVault <|.. OrionVault
IOrionTransparentVault <|.. OrionTransparentVault
%% === Config ===
class OrionConfig {
+underlyingAsset IERC20
+strategistIntentDecimals uint8
-whitelistedManager EnumerableSet_AddressSet
+addWhitelistedManager(manager address)
+removeWhitelistedManager(manager address)
+isWhitelistedManager(manager address) bool
}
IOrionConfig <|.. OrionConfig
%% === Orchestrators ===
class InternalStatesOrchestrator {
+config IOrionConfig
+registry IPriceAdapterRegistry
+intentFactor uint256
+underlyingAsset address
+processEpoch()
-_updateVaultState(vault IOrionVault)
}
class LiquidityOrchestrator {
+config IOrionConfig
+underlyingAsset address
+transferVaultFees(amount uint256)
}
ILiquidityOrchestrator <|.. LiquidityOrchestrator
%% === Strategist implementation ===
class KBestTvlWeightedAverage {
+config IOrionConfig
+k uint16
+constructor(owner address, _config IOrionConfig, _k uint16)
+submitIntent(vault IOrionTransparentVault)
}
IOrionStrategist <|.. KBestTvlWeightedAverage
%% === Relationships ===
OrionVault --> IOrionConfig : uses
OrionVault --> ILiquidityOrchestrator : uses
InternalStatesOrchestrator --> IOrionVault : calls_vaultFee_accrueVaultFees
InternalStatesOrchestrator --> IOrionConfig : uses
LiquidityOrchestrator --> IOrionConfig : uses
LiquidityOrchestrator --> IOrionVault : reads_manager
KBestTvlWeightedAverage --> IOrionConfig : uses
KBestTvlWeightedAverage --> IOrionTransparentVault : submits_intent
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.github.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The storage layout of
OrionVaulthas changed (removedvaultOwner, addedstrategist, renamedpendingManagerFeestopendingVaultFeesetc.); double-check that this is safe for existing deployed proxies and, if not, document or introduce a migration/adapter rather than modifying the layout in-place. - In
TransparentVaultFactory.createVaultthe function parameter is namedstrategistbutmanageris nowmsg.sender; consider renaming variables/parameters or expanding NatSpec to clearly differentiate the "manager" (creator/owner) from the "strategist" to avoid confusion for integrators. - Several access controls have been shifted from
onlyVaultOwnertoonlyManager(e.g.updateFeeModel,setDepositAccessControl,updateStrategist); it would be helpful to double-check that this matches the intended trust model, especially for functions that can block deposits or change fee parameters.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The storage layout of `OrionVault` has changed (removed `vaultOwner`, added `strategist`, renamed `pendingManagerFees` to `pendingVaultFees` etc.); double-check that this is safe for existing deployed proxies and, if not, document or introduce a migration/adapter rather than modifying the layout in-place.
- In `TransparentVaultFactory.createVault` the function parameter is named `strategist` but `manager` is now `msg.sender`; consider renaming variables/parameters or expanding NatSpec to clearly differentiate the "manager" (creator/owner) from the "strategist" to avoid confusion for integrators.
- Several access controls have been shifted from `onlyVaultOwner` to `onlyManager` (e.g. `updateFeeModel`, `setDepositAccessControl`, `updateStrategist`); it would be helpful to double-check that this matches the intended trust model, especially for functions that can block deposits or change fee parameters.
## Individual Comments
### Comment 1
<location> `contracts/vaults/OrionVault.sol:50-54` </location>
<code_context>
- address public vaultOwner;
/// @notice Vault manager
address public manager;
+ /// @notice Vault strategist
+ address public strategist;
/// @notice OrionConfig contract
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider validating non-zero manager/strategist in initializer to avoid permanently bricking control flows.
In `__OrionVault_init`, `manager_` and `strategist_` are stored without validation, while `onlyManager`/`onlyStrategist` guard critical actions (fee updates, whitelist changes, intent submission). A zero address here would permanently disable those flows. Please either enforce non-zero addresses on init or clearly document zero as an intentional, supported configuration.
</issue_to_address>
### Comment 2
<location> `contracts/factories/TransparentVaultFactory.sol:50-55` </location>
<code_context>
}
/// @notice Creates a new transparent vault
- /// @param manager The address of the vault manager
+ /// @param strategist The address of the vault strategist
</code_context>
<issue_to_address>
**suggestion:** The naming of `manager` (caller) vs `strategist` (param) is now inverted from previous semantics and may be confusing.
Consider renaming the parameter to something like `initialStrategist` to better reflect its role, and/or adding a brief comment clarifying that `msg.sender` becomes the manager while the argument becomes the strategist when the vault is created.
Suggested implementation:
```
/// @notice Creates a new transparent vault
/// @dev The caller (msg.sender) is set as the vault manager; `initialStrategist` is set as the vault strategist.
/// @param initialStrategist The address of the vault strategist
/// @param name The name of the vault
/// @param symbol The symbol of the vault
/// @param feeType The fee type
/// @param depositAccessControl The address of the deposit access control contract (address(0) = permissionless)
/// @return vault The address of the new transparent vault
function createVault(
address initialStrategist,
string calldata name,
string calldata symbol,
```
1. Inside `createVault`, replace all uses of the old parameter name `strategist` with `initialStrategist` (e.g. when passing it to the vault constructor or initializer).
2. Update all external call sites of `createVault` to use the new parameter name in their interfaces/ABIs or comments where applicable; the positional argument remains the same at the Solidity level, but any interface definitions or NatSpec elsewhere should also use `initialStrategist` for consistency.
</issue_to_address>
### Comment 3
<location> `test/orchestrator/Orchestrators.test.ts:1640-1643` </location>
<code_context>
// Expected total pending fees = previous pending fees + new fees from this epoch
- const expectedTotalPendingManagerFees = pendingManagerFeesBefore + expectedManagerFeeAfterRevenueShareThisEpoch;
- console.log(`Expected Total Pending Manager Fees: ${expectedTotalPendingManagerFees.toString()}`);
+ const expectedTotalPendingVaultFees = pendingVaultFeesBefore + expectedVaultFeeAfterRevenueShareThisEpoch;
+ console.log(`Expected Total Pending Vault Fees: ${expectedTotalPendingVaultFees.toString()}`);
- expect(expectedTotalPendingManagerFees).to.equal(expectedManagerFeeAfterRevenueShareThisEpoch);
+ expect(expectedTotalPendingVaultFees).to.equal(expectedVaultFeeAfterRevenueShareThisEpoch);
// Calculate expected values for fulfill deposit and redeem
</code_context>
<issue_to_address>
**issue (bug_risk):** Fee pending-amount assertion no longer validates contract state and likely has incorrect expectation
Here you correctly compute `expectedTotalPendingVaultFees = pendingVaultFeesBefore + expectedVaultFeeAfterRevenueShareThisEpoch`, but then assert it equals `expectedVaultFeeAfterRevenueShareThisEpoch` instead of the actual on-chain `actualPendingVaultFees`. This only passes when `pendingVaultFeesBefore` is zero and no longer validates the contract state. The assertion should instead compare `actualPendingVaultFees` to `expectedTotalPendingVaultFees` so the test continues to verify that pending fees accumulate correctly across epochs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/orchestrator/Orchestrators.test.ts (1)
900-917: Fee accounting test no longer validates on‑chain state; fix assertion to useactualPendingVaultFeesIn the second‑epoch fee analysis you compute:
const pendingVaultFeesBefore = pendingVaultFeesBeforeSecondEpoch.get(vaultAddress) || 0n; // ... const totalVaultFee = managementFeeAmount + performanceFeeAmount; // ... const expectedVaultFeeAfterRevenueShareThisEpoch = totalVaultFee - revenueShareFee; const expectedTotalPendingVaultFees = pendingVaultFeesBefore + expectedVaultFeeAfterRevenueShareThisEpoch; expect(expectedTotalPendingVaultFees).to.equal(expectedVaultFeeAfterRevenueShareThisEpoch);This:
- Silently asserts
pendingVaultFeesBefore === 0, and- Never compares the expected total to
actualPendingVaultFeesfrom the contract, so the test no longer protects the implementation.You likely intended to assert that on‑chain pending fees equal the cumulative expected amount. Suggested fix:
Proposed diff
- const actualPendingVaultFees = await vault.pendingVaultFees(); + const actualPendingVaultFees = await vault.pendingVaultFees(); @@ - const expectedTotalPendingVaultFees = pendingVaultFeesBefore + expectedVaultFeeAfterRevenueShareThisEpoch; - console.log(`Expected Total Pending Vault Fees: ${expectedTotalPendingVaultFees.toString()}`); - - expect(expectedTotalPendingVaultFees).to.equal(expectedVaultFeeAfterRevenueShareThisEpoch); + const expectedTotalPendingVaultFees = pendingVaultFeesBefore + expectedVaultFeeAfterRevenueShareThisEpoch; + console.log(`Expected Total Pending Vault Fees: ${expectedTotalPendingVaultFees.toString()}`); + + expect(actualPendingVaultFees).to.equal(expectedTotalPendingVaultFees);Also applies to: 1486-1644
🧹 Nitpick comments (12)
contracts/orchestrators/LiquidityOrchestrator.sol (1)
452-456: NatSpec parameter mismatch flagged by pipeline.The pipeline warning indicates that
@param finalTotalAssetsis missing from the NatSpec for_processSingleVaultOperations. Consider adding it:/// @notice Processes deposit and redeem operations for a single vault /// @param vault The vault address /// @param totalAssetsForDeposit The total assets for deposit operations /// @param totalAssetsForRedeem The total assets for redeem operations +/// @param finalTotalAssets The final total assets value after operations function _processSingleVaultOperations(test/OrionConfigVault.test.ts (2)
200-227: Manager whitelist tests are correct; consider updating wordingLogic around
isWhitelistedManager,addWhitelistedManager, andremoveWhitelistedManager(including owner vs non‑owner callers and error types) matches the new config API.Several
it(...)messages and comments still say “vault owner”; consider updating them to “manager” to avoid confusion with the new strategist terminology.Also applies to: 229-262
537-564: Strategist management & access‑control coverage looks goodThese tests now correctly:
- Require the manager (
owner) to callupdateStrategist,- Ensure non‑manager callers (user, current strategist) get
NotAuthorized,- Assert the
StrategistUpdatedevent andstrategist()getter.Only minor nit: the description “vault owner” in the access‑control test could be renamed to “manager” for consistency.
Also applies to: 589-603
test/PassiveCuratorStrategy.test.ts (1)
213-231: Use BigInt for intent scale calculations to avoid Number pitfallsIn multiple places you do:
const strategistIntentDecimals = await orionConfig.strategistIntentDecimals(); const expectedTotalWeight = 10 ** Number(strategistIntentDecimals); expect(totalWeight).to.equal(expectedTotalWeight);and similarly for
k = 1and the looped weight‑sum test.To avoid any risk from JS
numberprecision and to keep types aligned with on‑chainuint32/uint256, consider:const strategistIntentDecimals = await orionConfig.strategistIntentDecimals(); const expectedTotalWeight = 10n ** strategistIntentDecimals; expect(totalWeight).to.equal(expectedTotalWeight);and likewise where you expect a single weight to equal the full scale.
Also applies to: 271-307, 374-397, 423-438
contracts/vaults/OrionTransparentVault.sol (1)
168-176:updateStrategistsemantics are straightforward; consider optional zero‑address guardAllowing the manager to change
strategistand emittingStrategistUpdatedis fine and matches tests. If you want to prevent accidentally bricking strategist‑only flows, you might optionally rejectnewStrategist == address(0)here and use explicit “disable strategist” flows elsewhere.test/orchestrator/Orchestrators.test.ts (2)
45-57: Orchestrator ↔ strategist integration is coherent
- New
strategistsigner is used consistently for vault creation and active intent submission.kbestTvlPassiveStrategistis deployed once and attached to the passive vault viaupdateStrategist, then drives intents viasubmitIntent.One nit: the comment
// Deploy KBestTvlWeightedAverage passive strategist with k=2disagrees with the actualk = 1argument; consider updating the comment for clarity.Also applies to: 126-135, 341-363, 361-363, 383-384
1221-1225: Prefer BigInt for strategist intent scale in orchestrator portfolio checksYou compute intent scale via:
const strategistIntentDecimals = await orionConfig.strategistIntentDecimals(); const intentDecimals = 10n ** BigInt(strategistIntentDecimals);in one place, and
10 ** Number(strategistIntentDecimals_SecondEpoch)in another.For consistency and to avoid any risk of
numberprecision issues, consider using the BigInt form everywhere:const strategistIntentDecimals = await orionConfig.strategistIntentDecimals(); const intentDecimals = 10n ** strategistIntentDecimals;and likewise for the second‑epoch section.
Also applies to: 1933-1936
contracts/vaults/OrionVault.sol (2)
30-41: Strategist state & vault fee renames are consistent; watch storage layout & solhint limit
- Adding
strategistalongsidemanagerand renaming pending manager fees topendingVaultFeesline up with the rest of the PR and the orchestrator/tests.- New
onlyStrategistis a clean way to gate strategist‑only functions in derived vaults.- Solhint now reports 16 state declarations vs your configured max 15; if you care about that rule, consider grouping related fields into a struct or moving some constants to a library.
Because
OrionVaultis upgradeable, addingstrategist(and removing any older owner field) must preserve the exact storage layout of previous versions. Please double‑check the old layout and, if needed, document the migration or bump to a new implementation slot to avoid collisions.Also applies to: 48-76, 80-92, 124-139
590-603:claimVaultFeessemantics are straightforward; consider eventing if needed
claimVaultFees:
- Enforces
amount > 0andamount <= pendingVaultFees,- Decrements
pendingVaultFees, and- Delegates payout to
liquidityOrchestrator.transferVaultFees(amount).This matches the previous manager‑fee behavior under new naming. If observability is important, consider emitting a “VaultFeesClaimed”‑style event here (if not already emitted elsewhere).
test/VaultOwnerRemoval.test.ts (1)
16-21: Update test suite title and documentation to reflect new terminology.The test suite title and documentation still reference "Vault Owner Removal" and "vault owner," but the code has been refactored to use "manager" terminology. The comments at lines 17-19 mention "vault owner" while the actual implementation uses manager-based access control.
🔎 Suggested documentation updates
/** - * @title Vault Owner Removal Tests - * @notice Tests for the automatic vault decommissioning when vault owner is removed - * @dev This test suite validates that when a vault owner is removed from the whitelist, - * all vaults owned by that vault owner are automatically marked for decommissioning. + * @title Manager Removal Tests + * @notice Tests for the automatic vault decommissioning when a manager is removed + * @dev This test suite validates that when a manager is removed from the whitelist, + * all vaults managed by that manager are automatically marked for decommissioning. */ -describe("Vault Owner Removal - Automatic Decommissioning", function () { +describe("Manager Removal - Automatic Decommissioning", function () {contracts/interfaces/IOrionVault.sol (2)
48-48: Consider gas implications of indexing all three event parameters.The event was renamed from
FeeModelUpdatedtoVaultFeeModelUpdatedand all three parameters (mode,performanceFee,managementFee) are now indexed. Indexed parameters increase gas costs for event emission but enable filtering in event logs.Verify that filtering by all three fee parameters is a common use case. If not, consider indexing only the most frequently filtered parameter(s) to reduce gas costs.
61-64: Consider gas implications of indexing both event parameters.The event was renamed from
ManagerFeesAccruedtoVaultFeesAccruedwith both parameters indexed. While this enables filtering by fee amounts, indexed uint256 values are stored as hashes in logs, making exact value filtering less useful.Consider whether filtering by exact fee amounts is necessary. If not, removing indexing from these numeric parameters would reduce gas costs while maintaining event data availability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
contracts/OrionConfig.solcontracts/factories/TransparentVaultFactory.solcontracts/interfaces/ILiquidityOrchestrator.solcontracts/interfaces/IOrionConfig.solcontracts/interfaces/IOrionStrategist.solcontracts/interfaces/IOrionVault.solcontracts/libraries/ErrorsLib.solcontracts/libraries/EventsLib.solcontracts/orchestrators/InternalStatesOrchestrator.solcontracts/orchestrators/LiquidityOrchestrator.solcontracts/strategies/KBestTvlWeightedAverage.solcontracts/test/KBestTvlWeightedAverageInvalid.solcontracts/test/OrionTransparentVaultV2.solcontracts/vaults/OrionTransparentVault.solcontracts/vaults/OrionVault.solpackage.jsontest/AccessControl.test.tstest/BatchLimitAccounting.test.tstest/BatchLimitConsistency.test.tstest/FeeCooldown.test.tstest/MinimumAmountDOS.test.tstest/OrionConfigVault.test.tstest/OrionVaultExchangeRate.test.tstest/PassiveCuratorStrategy.test.tstest/ProtocolPause.test.tstest/RedeemBeforeDepositOrder.test.tstest/Removal.test.tstest/TransparentVault.test.tstest/Upgrade.test.tstest/VaultOwnerRemoval.test.tstest/orchestrator/OrchestratorConfiguration.test.tstest/orchestrator/OrchestratorSecurity.test.tstest/orchestrator/Orchestrators.test.tstest/orchestrator/OrchestratorsZeroState.test.ts
💤 Files with no reviewable changes (1)
- contracts/libraries/ErrorsLib.sol
🧰 Additional context used
🪛 GitHub Actions: CI
package.json
[warning] 1-1: Code style issues found in package.json. Run Prettier with --write to fix.
contracts/orchestrators/LiquidityOrchestrator.sol
[warning] 306-306: Missing @param tag in function 'checkUpkeep'
[warning] 306-306: Mismatch in @param count for function 'checkUpkeep'. Expected: 1, Found: 0
[warning] 320-320: Missing @param tag in function 'performUpkeep'
[warning] 320-320: Mismatch in @param count for function 'performUpkeep'. Expected: 1, Found: 0
[warning] 452-452: Mismatch in @param names for function '_processSingleVaultOperations'. Expected: [vault, totalAssetsForDeposit, totalAssetsForRedeem, finalTotalAssets], Found: [vault, totalAssetsForDeposit, totalAssetsForRedeem]
contracts/test/OrionTransparentVaultV2.sol
[warning] 1-1: SOLHINT reported 14 warnings across Solidity sources in lint:sol step. Review and fix style or NatSpec/docs as needed.
contracts/interfaces/ILiquidityOrchestrator.sol
[warning] 1-1: SOLHINT reported 14 warnings across Solidity sources in lint:sol step. Review and fix style or NatSpec/docs as needed.
contracts/interfaces/IOrionConfig.sol
[warning] 1-1: SOLHINT reported 14 warnings across Solidity sources in lint:sol step. Review and fix style or NatSpec/docs as needed.
contracts/strategies/KBestTvlWeightedAverage.sol
[warning] 1-1: SOLHINT reported 14 warnings across Solidity sources in lint:sol step. Review and fix style or NatSpec/docs as needed.
contracts/vaults/OrionTransparentVault.sol
[warning] 1-1: SOLHINT reported 14 warnings across Solidity sources in lint:sol step. Review and fix style or NatSpec/docs as needed.
contracts/orchestrators/InternalStatesOrchestrator.sol
[warning] 33-33: Contract has 23 states declarations but allowed no more than 15
[warning] 281-281: Missing @param tag in function 'checkUpkeep'
[warning] 281-281: Mismatch in @param count for function 'checkUpkeep'. Expected: 1, Found: 0
[warning] 299-299: Missing @param tag in function 'performUpkeep'
[warning] 299-299: Mismatch in @param count for function 'performUpkeep'. Expected: 1, Found: 0
[warning] 385-385: Function body contains 82 lines but allowed no more than 50 lines
contracts/interfaces/IOrionVault.sol
[warning] 1-1: SOLHINT reported 14 warnings across Solidity sources in lint:sol step. Review and fix style or NatSpec/docs as needed.
contracts/test/KBestTvlWeightedAverageInvalid.sol
[warning] 1-1: SOLHINT reported 14 warnings across Solidity sources in lint:sol step. Review and fix style or NatSpec/docs as needed.
contracts/interfaces/IOrionStrategist.sol
[warning] 1-1: SOLHINT reported 14 warnings across Solidity sources in lint:sol step. Review and fix style or NatSpec/docs as needed.
contracts/vaults/OrionVault.sol
[warning] 42-42: Contract has 16 states declarations but allowed no more than 15
contracts/libraries/EventsLib.sol
[warning] 1-1: SOLHINT reported 14 warnings across Solidity sources in lint:sol step. Review and fix style or NatSpec/docs as needed.
contracts/factories/TransparentVaultFactory.sol
[warning] 74-74: SOLHINT: GC: String exceeds 32 bytes
contracts/OrionConfig.sol
[warning] 34-34: Contract has 20 states declarations but allowed no more than 15
🪛 GitHub Check: Build, Lint and Test
contracts/factories/TransparentVaultFactory.sol
[warning] 74-74:
GC: String exceeds 32 bytes
⏰ 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 (82)
package.json (1)
4-4: Verify that the version bump aligns with semver strategy for breaking changes.The PR description indicates a comprehensive refactoring involving public API changes (e.g., terminology changes to IOrionStrategist, vault strategist role, vault fees). Under strict semver, breaking API changes in v1.x typically warrant a major version bump (2.0.0) rather than a minor increment (1.2.0).
Confirm whether the project follows strict semver or treats terminology refactoring as non-breaking changes.
contracts/test/OrionTransparentVaultV2.sol (1)
1-31: Address solhint warnings.The pipeline reports 14 solhint warnings across Solidity sources. Please review and fix style or NatSpec/documentation issues as needed to maintain code quality.
test/FeeCooldown.test.ts (4)
31-31: LGTM! Clean variable rename in test fixture.The signer variable has been correctly renamed from
managertostrategistand properly exposed in the fixture return object, maintaining consistency with the broader terminology refactoring.Also applies to: 45-45
59-76: LGTM! Terminology correctly reflects the dual role model.The code correctly distinguishes between:
- Whitelist "manager": The
ownerwho creates vaults (lines 62-64)- Vault "strategist": The operational role passed to the vault (line 69)
This aligns with the PR's objective of clarifying role semantics: vault creators are whitelisted as "managers," while the in-vault operational role is now called "strategist."
337-338: LGTM! Authorization test correctly updated.The test properly verifies that the
strategistrole cannot update vault fees (only the vault owner can), which is the expected authorization model. The rename maintains the test's correctness.
342-342: LGTM! Protocol fee authorization test correctly updated.The test properly verifies that the
strategistsigner cannot update protocol-level fees, which require owner privileges. The variable rename preserves the test's intent and correctness.Also applies to: 349-350
test/RedeemBeforeDepositOrder.test.ts (4)
51-51: LGTM! Variable renaming is clean and consistent.The
managertostrategistvariable renaming aligns with the broader protocol terminology refactoring.Also applies to: 96-96
1-395: Test refactoring is complete and consistent.The terminology changes from
managertostrategistare applied consistently throughout the test file with no logic changes. The test maintains its coverage of redeem-before-deposit ordering verification.
158-164: Access control verified onsubmitIntent.The
submitIntentfunction inOrionTransparentVault.solcorrectly enforces theonlyStrategistmodifier, restricting calls to the strategist role only. The test code at lines 158-164 properly calls this function with the strategist account viavault.connect(strategist).submitIntent().
137-137: ThecreateVaultinterface parameter is correctly named and used. The first parameter isaddress strategist, and the test correctly passesstrategist.addresswhen creating the vault. This parameter is used during vault initialization.test/BatchLimitAccounting.test.ts (3)
26-26: LGTM! Terminology update aligns with protocol refactoring.The renaming from
managertostrategistis consistent with the broader protocol terminology changes described in the PR objectives.
66-76: Fixture change is safe and isolated to this test file.The fixture's return value change from
managertostrategisthas no external impact. No other test files import this fixture, andVaultOwnerRemoval.test.tsuses its own separatedeployFixturefunction with different property names.
38-46: The code is correct as written. ThecreateVault()function signature confirms that the strategist address is the first parameter, matching the usage throughout all test files.test/MinimumAmountDOS.test.ts (1)
21-21: LGTM! Terminology update is consistent.The rename from
managertostrategistis applied consistently throughout the test fixture setup, vault creation, and return object.Also applies to: 33-33, 61-61
test/BatchLimitConsistency.test.ts (1)
76-76: LGTM! Consistent terminology refactoring.The strategist role replaces manager consistently in variable declarations, signer assignments, and vault creation calls.
Also applies to: 89-89, 103-103
test/ProtocolPause.test.ts (1)
74-74: LGTM! Test fixture updated consistently.The strategist role replaces manager throughout the test setup, including variable declarations, signer assignments, and both static call and actual vault creation calls.
Also applies to: 87-87, 140-140, 151-151
test/OrionVaultExchangeRate.test.ts (1)
16-16: LGTM! Terminology consistently updated in test fixture.The strategist role replaces manager in all appropriate locations: signer declaration, vault creation parameter, and fixture return object.
Also applies to: 32-32, 71-71
test/AccessControl.test.ts (1)
15-15: LGTM! Access control tests updated consistently.The strategist role replaces manager across all test scenarios (permissionless mode, with access control, and updateable vault), maintaining consistency in the terminology refactoring.
Also applies to: 27-27, 45-45, 96-96, 197-197
contracts/orchestrators/InternalStatesOrchestrator.sol (2)
200-200: The methodstrategistIntentDecimals()is correctly defined in IOrionConfig (line 35) as an external view function returninguint8, and is implemented in OrionConfig (line 52) with a value of 9. The usage at line 200 in InternalStatesOrchestrator.sol is correct.
442-451: Interface compatibility confirmed for vault fee methods.Both
vaultFee(uint256 totalAssets)andaccrueVaultFees(uint256 feeAmount)exist in IOrionVault.sol with the correct signatures matching their usage in the code. IOrionTransparentVault inherits these methods, so compatibility is confirmed across both interfaces.contracts/interfaces/IOrionStrategist.sol (1)
6-16: The interface rename fromIOrionStrategytoIOrionStrategisthas been successfully completed. All contracts implementing this interface (KBestTvlWeightedAverage.sol,KBestTvlWeightedAverageInvalid.sol) use the new name, and all imports throughout the codebase have been properly updated. No remaining references to the old interface name exist.contracts/interfaces/ILiquidityOrchestrator.sol (1)
68-71: All changes verified successfully. ThetransferVaultFeesfunction is properly implemented in LiquidityOrchestrator (line 271) with correct vault authorization checks, and OrionVault.sol (line 595) correctly calls the renamed function. No remaining references to the oldtransferManagerFeesfunction exist in the codebase.test/Removal.test.ts (4)
33-38: Test role terminology updated correctly.The signer variable renaming from
managertostrategistand the corresponding destructuring update are consistent with the PR's terminology refactoring.
112-114: Vault creation updated to pass strategist address.The test now correctly passes
strategist.addressas the strategist parameter tocreateVault, withownerremaining as the caller (manager).
152-152: Intent submission correctly uses strategist signer.
560-568: Fee method renaming aligned with contract changes.The test correctly uses the renamed methods
pendingVaultFees()andclaimVaultFees()instead of the previouspendingManagerFees()andclaimManagerFees().test/orchestrator/OrchestratorConfiguration.test.ts (4)
109-120: Test setup correctly refactored for strategist terminology.The variable declarations and signer extraction are updated consistently to use
strategistandkbestTvlPassiveStrategist.
190-198: Passive strategist deployment updated correctly.The comment and variable naming now reflect "passive strategist" terminology, and the deployment correctly uses
k=1for the top-1 asset selection.
375-376: Passive vault strategist configuration is correct.The test correctly calls
updateStrategistto set the passive strategist contract address, then callssubmitIntenton the passive strategist to generate the vault's intent.
618-621: Access control test updated for strategist role.The test correctly verifies that
strategist(non-owner) cannot update epoch duration, maintaining the access control validation with the new role naming.contracts/interfaces/IOrionConfig.sol (3)
32-35: Intent decimals renamed to strategist terminology.The function
strategistIntentDecimals()and its documentation correctly reflect that intents are now associated with strategists rather than managers.
100-113: Whitelist functions renamed from VaultOwner to Manager.The interface correctly renames:
addWhitelistedVaultOwner→addWhitelistedManagerremoveWhitelistedVaultOwner→removeWhitelistedManagerisWhitelistedVaultOwner→isWhitelistedManagerThe NatSpec documentation is updated accordingly.
121-128: Documentation updated for strategist intents.The NatSpec correctly notes that removing a vault "renders strategist intents inactive" rather than "manager intents".
contracts/libraries/EventsLib.sol (3)
66-68: Event renamed from VaultOwnerRemoved to ManagerRemoved.This aligns with the whitelist terminology change from vault owners to managers.
74-76: OrderSubmitted event correctly references strategist.Since intents are now submitted by strategists, the event parameter and documentation are correctly updated.
113-135: OrionVaultCreated event updated with manager and strategist.The event now includes both
manager(the creator/fee recipient) andstrategist(the intent submitter) as indexed parameters, correctly reflecting the role separation.test/orchestrator/OrchestratorsZeroState.test.ts (3)
26-31: Test signer setup updated for strategist terminology.The variable and signer extraction are correctly updated.
45-47: Zero-state vault creation uses strategist address.The test correctly passes
strategist.addressas the strategist parameter.
91-91: Intent submission uses strategist signer.test/orchestrator/OrchestratorSecurity.test.ts (4)
132-143: Test setup refactored for strategist terminology.Variable declarations and signer extraction are consistently updated.
214-222: Passive strategist deployment updated correctly.The deployment uses
k=1for single asset selection, consistent with other test files.
420-420: Intent submissions use strategist signer throughout.All vault intent submissions consistently use the
strategistsigner.
379-399: Passive vault configuration uses updateStrategist without immediate intent submission.Verified: OrchestratorSecurity.test.ts intentionally skips the
submitIntent()call afterupdateStrategist(), unlike Orchestrators.test.ts, OrchestratorConfiguration.test.ts, and PassiveCuratorStrategy.test.ts, which all submit an intent immediately after setting the strategist. This difference is deliberate—this test file tests the vault in a state without an initial intent submission, which is a valid distinct test scenario from the other orchestrator tests.contracts/orchestrators/LiquidityOrchestrator.sol (1)
271-280: Function correctly retrieves manager and transfers vault fees.The
transferVaultFeesfunction properly callsIOrionVault(vault).manager()to get the destination address and transfers funds accordingly. Themanager()getter exists in the IOrionVault interface (line 78), confirming the implementation is valid.contracts/factories/TransparentVaultFactory.sol (1)
59-95: Terminology refactoring is correct.The separation of
manager(the whitelisted caller) andstrategist(the intent submitter) is properly implemented. TheinitDataencoding passes both addresses in the correct order matchingOrionTransparentVault.initialize, and the event emission reflects the updated terminology.The static analysis warning on line 74 about string exceeding 32 bytes is expected for function signature strings in
abi.encodeWithSignatureand is not actionable.contracts/test/KBestTvlWeightedAverageInvalid.sol (2)
6-7: Interface & terminology switch toIOrionStrategistlooks consistentImport, implements clause, constructor docs, and
submitIntentinheritance all align with the new strategist terminology; behavior is unchanged apart from the interface type.Also applies to: 18-37
124-125: Confirm upper bound forstrategistIntentDecimalsto keepintentScalesafe
intentScaleis computed asuint32(10 ** config.strategistIntentDecimals()). IfstrategistIntentDecimals> 9, this overflowsuint32and silently truncates, which would desync tests and vault checks that assume full-scale precision.Please confirm
strategistIntentDecimalsis constrained (e.g., ≤ 9) at the config level, or consider:
- Using a wider type (
uint64/uint96) for weights, or- Reverting if
strategistIntentDecimalsexceeds the supported range.test/OrionConfigVault.test.ts (1)
31-35: Fixture roles & vault creation aligned with new strategist modelUsing
[owner, strategist, other, user]and passingstrategist.addressintocreateVaultwhile later treatingowneras the manager (e.g., callingupdateStrategistfromowner) is consistent with the manager/strategist split introduced in the contracts.Also applies to: 90-93
contracts/strategies/KBestTvlWeightedAverage.sol (2)
6-7: Strategist interface, ownership wiring, and access control are coherent
- Importing/implementing
IOrionStrategistand updating NatSpec to “strategist” keeps the public surface aligned with the new role.- The
constructor(address owner, ...) Ownable(owner)pattern withOwnable2Stepis the standard OZ 5.x approach for setting an explicit initial owner.- Restricting
submitIntentandupdateParametersviaonlyOwnermatches test usage (owner is whoever should drive this passive strategist).No functional issues spotted here.
Also applies to: 14-19, 25-37
125-126: VerifystrategistIntentDecimalscannot overflowuint32 intentScale
intentScaleisuint32(10 ** config.strategistIntentDecimals()). As with the invalid variant, ifstrategistIntentDecimals> 9 this will overflowuint32and corrupt weights.Please ensure the config enforces a safe upper bound, or widen the weight type if higher precision is needed.
test/Upgrade.test.ts (3)
18-25: Upgrade-path tests correctly reflect manager/strategist splitAcross the beacon upgrade scenarios you now:
- Deploy vaults with
strategist.address,- Assert
manager()remainsowner.addressbefore and after upgrades, and- Assert
strategist()is preserved across V2 upgrades where applicable.This nicely validates storage layout and role separation through beacon upgrades.
Also applies to: 140-145, 148-183, 195-231, 263-304
334-349: Factory beacon & factory‑upgrade tests aligned with new manager whitelist and vault API
- Using
isWhitelistedManager(owner.address)andaddWhitelistedManagermatches the new config interface.- All
createVaultcalls now passstrategist.address, and subsequent checks rely onmanager()as the admin surface.- Post‑upgrade checks still ensure manager state and factory wiring (beacons) are intact.
No correctness issues spotted.
Also applies to: 351-386, 412-457, 459-503, 521-526
315-331: Updated negative‑access tests for strategist are consistentWhere these tests previously used a manager‑like signer, they now use
strategistas the non‑owner/non‑registry actor and assert reverts on:
InternalStatesOrchestrator.performUpkeep,updateEpochDuration, and- Similar config mutations.
This cleanly captures that strategists are not governance.
Also applies to: 533-555, 652-656
test/PassiveCuratorStrategy.test.ts (2)
22-44: Passive strategist wiring and vault integration look correct
KBestTvlWeightedAverageis deployed withstrategist.addressas its owner, matchingonlyOwneronsubmitIntent/updateParameters.- The vault is first created with a strategist EOA, then
updateStrategistpoints it to the passive strategist contract before callingsubmitIntent.- This mirrors how an operator might hand off from a wallet strategist to a strategy contract.
Flow and access patterns appear sound.
Also applies to: 155-207
441-451: Invalid passive strategist test correctly targets new weight validationThe “invalid passive strategist” scenario now:
- Deploys
KBestTvlWeightedAverageInvalidwithstrategist.addressas owner,- Associates it to a fresh vault via
updateStrategist, and- Expects
submitIntentto revert withInvalidTotalWeight.This is a good regression test that the vault’s total‑weight check (now keyed on strategist intent decimals) is enforced for passive strategists.
Also applies to: 491-499
contracts/vaults/OrionTransparentVault.sol (2)
10-11: Strategist model & intent storage are well‑documented
- NatSpec now explicitly distinguishes wallet vs contract strategists and references
IOrionStrategist._portfolioIntentis clearly identified as “strategist intent (w₁)”.
These docs line up with the new passive‑strategist flow in tests.Also applies to: 14-22, 31-33
41-52: Strategist access control & weight validation look correct; confirm intent scale range
initializenow wires bothmanager_andstrategist_intoOrionVault, and seeds the intent to 100% underlying usingconfig.strategistIntentDecimals().submitIntentis restricted toonlyStrategistand:
- Clears previous intent,
- Enforces unique tokens and vault whitelist membership, and
- Requires
totalWeight == 10 ** config.strategistIntentDecimals().As with the strategies,
10 ** config.strategistIntentDecimals()is cast to/stored as auint32. Please confirm the config constrainsstrategistIntentDecimalsso this scale fits in 32 bits (≤ 9), otherwise this will overflow and break the invariant that the vault and tests rely on.Also applies to: 63-66, 75-77, 81-107, 123-130
test/orchestrator/Orchestrators.test.ts (2)
2142-2146:claimVaultFeesusage matches new vault fee APIThe switch from manager‑fee nomenclature to:
const pendingTransparentVaultFees = await hurdleHwmVault.pendingVaultFees(); if (pendingTransparentVaultFees > 0) { await hurdleHwmVault.connect(owner).claimVaultFees(pendingTransparentVaultFees); }is consistent with the contract changes and keeps the test logic intact.
2334-2335: Access‑control tests correctly treat strategist as non‑governanceUsing
strategistas the unauthorized caller forperformUpkeepandupdateEpochDurationhelps assert that strategists cannot drive orchestrator state transitions; only owner and automation registry are allowed. This matches the new role model.Also applies to: 2653-2655
contracts/vaults/OrionVault.sol (3)
164-216: Updated initializer signature matches new roles; ensure all call sites passstrategist_correctly
__OrionVault_initnow takes bothmanager_andstrategist_, sets them, and otherwise preserves the previous validation and fee‑model setup. Tests (e.g., inOrionTransparentVaultand factories) exercise this path and appear wired correctly.No issues here, just worth keeping in sync with all factory/upgrade code paths calling the initializer.
455-490: Vault fee model &vaultFeeview helper remain functionally equivalent
updateFeeModelremainsonlyManagerand keeps the cooldown + cap checks, just under the new vault‑fee terminology.vaultFee(uint256 activeTotalAssets)simply recomputes management + performance fees using the active fee model, matching previous behavior under the old name.No functional regressions spotted.
Also applies to: 512-518
646-652:accrueVaultFeescorrectly updates state and emits the new eventThe accrual path remains restricted to the internal states orchestrator and now updates
pendingVaultFeesplus emitsVaultFeesAccrued(feeAmount, pendingVaultFees). This is coherent with the revamped fee terminology and upstream tests.test/VaultOwnerRemoval.test.ts (3)
25-55: LGTM! Fixture correctly implements manager/strategist roles.The fixture setup properly separates the manager role (who creates vaults and is whitelisted) from the strategist role (who manages vault intent). The whitelisting operations correctly use
addWhitelistedManager, aligning with the new terminology.
57-87: LGTM! Vault creation helper correctly separates manager and strategist roles.The
createVaulthelper function correctly accepts both manager (vault creator/owner) and strategist (intent manager) as separate parameters, properly reflecting the architectural separation of concerns in the refactored codebase.
118-125: LGTM! Event assertion correctly updated to ManagerRemoved.The test properly verifies the new
ManagerRemovedevent emission with the correct parameter.contracts/OrionConfig.sol (6)
51-52: LGTM! Constant renamed to reflect strategist's role in intent management.The rename from
managerIntentDecimalstostrategistIntentDecimalsaccurately reflects that strategists (not managers) are responsible for setting vault allocation intents in the refactored architecture.
70-70: LGTM! Whitelist correctly tracks managers instead of vault owners.The rename from
whitelistedVaultOwnerstowhitelistedManageraligns with the new terminology where managers are the whitelisted entities who create and own vaults.
303-307: LGTM! Access control preserved for manager whitelisting.The function correctly allows both guardian and owner to add managers to the whitelist, maintaining appropriate access control for this administrative operation.
346-348: LGTM! View function correctly checks manager whitelist status.The
isWhitelistedManagerfunction properly checks thewhitelistedManagerset, providing the correct public API for whitelist verification.
34-34: Note: Pipeline warning about state variable count.The pipeline flagged that this contract has 20 state declarations but the limit is 15. This is informational and may require architecture review if storage optimization is a concern for upgradeable contracts.
Based on the pipeline failure log, the contract exceeds the recommended state variable limit. While not a blocker, consider whether state consolidation is needed for future upgrades.
310-343: The implementation is correct.IOrionVault.manager()returns the vault creator—the address that calledcreateVault. InTransparentVaultFactory,manageris set tomsg.sender(line 68) and passed to the vault'sinitialize()method (line 76), where it is stored as a public state variable (OrionVault.sol:49). The publicmanagerstate variable automatically generates a getter function that returns this value. Sincemanagerandstrategistare separate state variables, the decommissioning logic correctly identifies vaults owned by the removed manager.test/TransparentVault.test.ts (6)
28-31: LGTM! Test fixture correctly implements manager/strategist role separation.The refactored signer setup properly distinguishes between the vault owner/manager (who creates and configures the vault) and the strategist (who manages investment intents).
88-88: LGTM! Test suite title updated to reflect strategist-focused operations.The rename to "TransparentVault - Strategist Pipeline" accurately reflects the test focus on strategist operations and intent management.
119-122: LGTM! Assertions correctly verify manager/strategist separation.The test properly verifies that:
manager()returns the vault creator (owner)strategist()returns the separate strategist address responsible for intent managementThis confirms the architectural separation between vault ownership and strategy management.
236-243: Clarify: "manager" in test description refers to vault manager (owner), not strategist.The test description says "allow manager to update fee model" and uses
owneras the signer, which is correct. In the refactored architecture:
- Manager (owner): Creates vault, manages fees, updates whitelist
- Strategist: Manages investment intents only
This might be slightly confusing given the terminology changes. The test is correct as-is.
254-276: LGTM! Strategist operations correctly use strategist signer.The test properly verifies that:
submitIntentis called by the strategist- Intent weights use strategist intent decimals (10^9)
- The intent is stored and retrieved correctly
323-338: LGTM! Access control correctly enforces strategist-only intent submission.The test properly verifies that non-strategist addresses cannot submit intents, correctly enforcing the role-based access control.
contracts/interfaces/IOrionVault.sol (5)
40-42: LGTM! Event renamed to accurately reflect strategist role.The rename from
ManagerUpdatedtoStrategistUpdatedcorrectly reflects that this event tracks changes to the strategist (who manages investment intents), not the vault manager/owner.
80-86: LGTM! Getters correctly reflect new architecture.The interface now properly exposes:
manager(): Returns the vault creator/ownerstrategist(): Returns the intent managerpendingVaultFees(): Returns accrued vault feesThis clearly separates vault ownership from strategy management responsibilities.
137-145: LGTM! Function rename and documentation accurately reflect the refactoring.The function rename from
updateManagertoupdateStrategistwith updated documentation correctly clarifies that:
- Managers own and configure vaults
- Strategists manage investment allocation logic
- Managers can update the strategist address
161-163: LGTM! Fee-related functions renamed to "vault fees" for clarity.The renames from "manager fees" to "vault fees" (
claimVaultFees,vaultFee,accrueVaultFees) provide clearer terminology that:
- Fees belong to the vault (accrue to vault manager)
- Avoids confusion with the new "manager" vs "strategist" role distinction
Also applies to: 186-191, 203-205
1-206: Verify that all calling code has been updated for breaking API changes.This interface introduces breaking changes that have been comprehensively updated throughout the codebase:
Events renamed:
ManagerUpdated→StrategistUpdated,ManagerFeesAccrued→VaultFeesAccruedFunctions renamed:
updateManager→updateStrategist,claimManagerFees→claimVaultFees,managerFee→vaultFee,accrueManagerFees→accrueVaultFeesGetters changed:
vaultOwner()removed,strategist()added;pendingManagerFees()removed,pendingVaultFees()addedNo references to old API names remain in the codebase. However, the interface defines
event VaultFeeModelUpdated(uint8 indexed mode, uint16 indexed performanceFee, uint16 indexed managementFee)but theupdateFeeModel()implementation emitsEventsLib.VaultFeeChangeScheduled(address(this))instead. Ensure this event definition aligns with implementation behavior or update the implementation to emit the interface-defined event.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json
⏰ 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 (1)
package.json (1)
111-111: Prettier formatting issue resolved.The trailing newline has been properly added to package.json. No further action needed.
1 participant
Summary by Sourcery
Rename vault roles from owner/manager/strategy terminology to manager/strategist, and align fee, whitelist, access control, and intent handling semantics accordingly across vault contracts, config, orchestrators, factories, interfaces, events, and tests.
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.