Price Feed Resilience: add PriceConfig timelock - #257
Merged
0xJem merged 30 commits intoMay 20, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds a reusable TimelockQueue and integrates timelocked queuing into PriceConfig v2 for sensitive PRICE configuration actions, adds non-mutating PRICE ChangesTimelock-Queued PRICE Configuration
Sequence DiagramsequenceDiagram
participant Governor as Governor/<br/>Proposer
participant PriceConfig as PriceConfig v2<br/>(Policy)
participant TimelockQueue as TimelockQueue<br/>(Queue Storage)
participant PRICE as PRICE Module
participant Executor as Any Address<br/>(Executor)
Governor->>PriceConfig: queueUpdateAsset(asset, params)
PriceConfig->>PriceConfig: _validateQueue (enabled, caller auth)
PriceConfig->>PRICE: validateUpdateAsset(asset, params)
PRICE-->>PriceConfig: validation OK
PriceConfig->>TimelockQueue: _queueAction(target, selector, payload)
TimelockQueue->>TimelockQueue: compute executableAt = now + timelockDelay
TimelockQueue->>TimelockQueue: store QueuedAction {payload, timestamps, ...}
TimelockQueue-->>Governor: emit TimelockActionQueued(actionId)
Note over Executor: wait timelockDelay...
Executor->>TimelockQueue: executeQueuedAction(actionId)
TimelockQueue->>TimelockQueue: validate timing/state (ready, not cancelled/executed)
TimelockQueue->>PriceConfig: _executeAction(target, selector, payload)
PriceConfig->>PRICE: updateAsset(asset, params)
PRICE-->>PriceConfig: applied
TimelockQueue->>TimelockQueue: mark executed, clear payload
TimelockQueue-->>Executor: emit TimelockActionExecuted(actionId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
0xJem
changed the title
0xJem
marked this pull request as ready for review
April 28, 2026 12:50
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/modules/PRICE/IPRICE.v2.sol (1)
459-463: Avoid spreadingSubKeycodefurther through the public interface.This adds another external entrypoint that depends directly on
SubKeycode, which keeps pushing a repo-internal type into the integration surface. If this boundary is being expanded anyway, prefer a plain ABI type likebytes20and wrap it internally in the implementation.As per coding guidelines, "Contracts should have a separate interface defined in a separate file to allow for easy integration. All interfaces are MIT-licensed and should avoid using internal types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/PRICE/IPRICE.v2.sol` around lines 459 - 463, The public interface function validateExecOnSubmodule currently exposes the repo-internal enum/type SubKeycode in its signature; change the external ABI to use a plain type (e.g. bytes20) instead of SubKeycode, update the IPRICE.v2.sol declaration of validateExecOnSubmodule to accept bytes20, and in the implementing contract convert/wrap that bytes20 to SubKeycode internally (e.g., with a private/internal helper that maps or casts to SubKeycode) so the repo-internal type is not leaked through the interface.src/policies/price/PriceConfig.v2.sol (1)
399-416: Reuse the shared feed-expectation count helper here.This re-implements the same count check already handled by
_validateUpdateFeedExpectationCount(). Keeping queue-time and execute-time validation in two places makes them easy to drift apart on the next change.♻️ Suggested simplification
function _executeUpdateAsset( address asset_, IPRICEv2.UpdateAssetParams memory params_, PriceFeedExpectation[] memory feedExpectations_ ) internal { - uint256 expectedCount = params_.updateFeeds ? params_.feeds.length : 0; - if (feedExpectations_.length != expectedCount) - revert IPriceConfigv2_FeedExpectationCountInvalid( - asset_, - feedExpectations_.length, - expectedCount - ); + _validateUpdateFeedExpectationCount(asset_, params_, feedExpectations_); PRICE.updateAsset(asset_, params_); if (params_.updateFeeds) _validatePriceFeedExpectations(asset_, params_.feeds, feedExpectations_);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/policies/price/PriceConfig.v2.sol` around lines 399 - 416, Replace the inlined count check in _executeUpdateAsset with the shared helper _validateUpdateFeedExpectationCount to avoid duplication: remove the manual expectedCount/revert block and instead call _validateUpdateFeedExpectationCount(asset_, params_.updateFeeds ? params_.feeds.length : 0, feedExpectations_); keep the subsequent PRICE.updateAsset and the conditional _validatePriceFeedExpectations call as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/policies/price/PriceConfig.v2.sol`:
- Around line 421-423: The NatSpec rationale above addAsset() incorrectly claims
it is for "view/staticcall-only submodule interactions" but addAsset() performs
an immediate PRICE mutation; update the comment in PriceConfig.v2 to accurately
describe that addAsset() is an immediate, non-timelocked state-changing
function, list its behavior (adds a new asset entry, emits relevant events),
include the access control and any conditions that cause a revert (e.g., asset
already exists, invalid parameters), and remove the misleading
"view/staticcall-only" text so generated docs reflect the true semantics of
addAsset().
- Around line 548-558: queueExecOnSubmodule currently only encodes SubKeycode +
calldata so a subsequent queueUpgradeSubmodule can swap the implementation
before execution; modify queueExecOnSubmodule to also read and encode the
current submodule implementation identifier (preferably the implementation
address or a code/version hash) into the payload when calling _queueAction (for
action type IPriceConfigv2.TimelockAction.ExecOnSubmodule), and then update the
ExecOnSubmodule execution path (the function that currently resolves the live
submodule at execution time) to verify the installed implementation matches the
encoded identifier and revert if mismatched; keep calls to
PRICE.validateExecOnSubmodule and _queueAction but include the additional
implementation id in the abi.encode and add the runtime check during execution
so queued calls are bound to the reviewed implementation.
In `@src/test/mocks/MockPrice.v2.sol`:
- Around line 313-332: The current validate* stubs (validateAddAsset,
validateRemoveAsset, validateUpdateAsset, validateInstallSubmodule,
validateUpgradeSubmodule, validateExecOnSubmodule) are unconditionally pure and
always succeed; either implement the same state-dependent preflight checks used
by OlympusPricev2 for these hooks (so the mock rejects the same invalid
queue-time operations) or make the mock explicitly and clearly permissive by
renaming it (e.g., PermissiveMockPrice) and documenting that these validate*
functions intentionally allow everything. Concretely, replace each pure no-op
with a view implementation that performs the relevant sanity checks
OlympusPricev2 uses for add/update/remove assets and submodule
installs/upgrades/execs (or rename the contract and update tests to expect
permissive behavior) so tests cannot be accidentally misled by a silently
permissive mock.
---
Nitpick comments:
In `@src/modules/PRICE/IPRICE.v2.sol`:
- Around line 459-463: The public interface function validateExecOnSubmodule
currently exposes the repo-internal enum/type SubKeycode in its signature;
change the external ABI to use a plain type (e.g. bytes20) instead of
SubKeycode, update the IPRICE.v2.sol declaration of validateExecOnSubmodule to
accept bytes20, and in the implementing contract convert/wrap that bytes20 to
SubKeycode internally (e.g., with a private/internal helper that maps or casts
to SubKeycode) so the repo-internal type is not leaked through the interface.
In `@src/policies/price/PriceConfig.v2.sol`:
- Around line 399-416: Replace the inlined count check in _executeUpdateAsset
with the shared helper _validateUpdateFeedExpectationCount to avoid duplication:
remove the manual expectedCount/revert block and instead call
_validateUpdateFeedExpectationCount(asset_, params_.updateFeeds ?
params_.feeds.length : 0, feedExpectations_); keep the subsequent
PRICE.updateAsset and the conditional _validatePriceFeedExpectations call as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b567809-140d-4c66-a3ed-4eb16ea03f94
📒 Files selected for processing (10)
ROLES.mddocumentation/price.mdsnapshots/PriceV2GasTest.jsonsrc/modules/PRICE/IPRICE.v2.solsrc/modules/PRICE/OlympusPrice.v2.solsrc/policies/interfaces/IPriceConfigv2.solsrc/policies/price/PriceConfig.v2.solsrc/scripts/ops/lib/BatchScriptV2.solsrc/test/mocks/MockPrice.v2.solsrc/test/policies/price/PriceConfig.v2.t.sol
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/policies/utils/TimelockQueue.sol (1)
1-3: Tighten the pragma on this new base contract.
>=0.8.15widens compilation below the repo’s standard floor for new Solidity files. For a brand-new reusable contract like this, I’d pin it to the 0.8.24+ range so it gets compiled with the same semantics/codegen assumptions as the rest of the new code. As per coding guidelines, "**/*.sol: Solidity version >= 0.8.24 (some contracts may use 0.8.15 for historical reasons)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/policies/utils/TimelockQueue.sol` around lines 1 - 3, The pragma in the new TimelockQueue.sol is too permissive (currently "pragma solidity >=0.8.15"); tighten it to match repo standards by updating the file-level pragma in TimelockQueue.sol to require Solidity 0.8.24 or newer (e.g., "pragma solidity >=0.8.24;") so the contract compiles with the same semantics/codegen assumptions as other new contracts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/policies/utils/TimelockQueue/TimelockQueue.t.sol`:
- Around line 4-6: Update the Solidity version floor and the forge-std import:
change the pragma statement from "pragma solidity >=0.8.20;" to "pragma solidity
>=0.8.24;" and replace the import of Test from "forge-std/Test.sol" with the
versioned alias "@forge-std-1.9.6/Test.sol" so the file's pragma and the import
of Test align with repo conventions.
---
Nitpick comments:
In `@src/policies/utils/TimelockQueue.sol`:
- Around line 1-3: The pragma in the new TimelockQueue.sol is too permissive
(currently "pragma solidity >=0.8.15"); tighten it to match repo standards by
updating the file-level pragma in TimelockQueue.sol to require Solidity 0.8.24
or newer (e.g., "pragma solidity >=0.8.24;") so the contract compiles with the
same semantics/codegen assumptions as other new contracts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 160289e5-4a80-41bd-9c7d-a64269c1a7e3
📒 Files selected for processing (15)
.github/workflows/coverage.yml.github/workflows/lint.yml.github/workflows/size.yml.github/workflows/tests-crosschain.yml.github/workflows/tests-fork.yml.github/workflows/tests-proposals.yml.github/workflows/tests-unit.yml.github/workflows/validate-emergency-config.ymlsrc/policies/interfaces/IPriceConfigv2.solsrc/policies/interfaces/utils/ITimelockQueue.solsrc/policies/price/PriceConfig.v2.solsrc/policies/utils/TimelockQueue.solsrc/scripts/ops/lib/BatchScriptV2.solsrc/test/policies/price/PriceConfig.v2.t.solsrc/test/policies/utils/TimelockQueue/TimelockQueue.t.sol
✅ Files skipped from review due to trivial changes (8)
- .github/workflows/tests-crosschain.yml
- .github/workflows/validate-emergency-config.yml
- .github/workflows/tests-proposals.yml
- .github/workflows/tests-fork.yml
- .github/workflows/lint.yml
- .github/workflows/coverage.yml
- .github/workflows/tests-unit.yml
- src/policies/interfaces/utils/ITimelockQueue.sol
…review # Conflicts: # documentation/price.md # snapshots/PriceV2GasTest.json # src/modules/PRICE/IPRICE.v2.sol # src/modules/PRICE/OlympusPrice.v2.sol # src/policies/interfaces/IPriceConfigv2.sol # src/policies/price/PriceConfig.v2.sol # src/test/policies/price/PriceConfig.v2.t.sol
Base automatically changed from
price-feed-expectations
to
price-feed-improvements-fixes
April 28, 2026 20:16
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/modules/PRICE/IPRICE.v2.sol (1)
412-500: Document revert conditions for the newvalidate*methods.The new preflight functions are great, but their NatSpec currently omits concrete revert conditions. Adding those will make integrator behavior expectations much clearer.
As per coding guidelines, “Function documentation should outline the behaviour of the function, including any conditions that would result in a revert”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/PRICE/IPRICE.v2.sol` around lines 412 - 500, The NatSpec for the new validator functions is missing explicit revert conditions; update the comments for validateAddAsset, validateRemoveAsset, validateUpdateAsset, validateInstallSubmodule, validateUpgradeSubmodule and validateExecOnSubmodule to list concrete revert scenarios (e.g., invalid/zero addresses, asset already registered/not approved, caller lacks permission, invalid movingAverageDuration_/observations_/params_, submodule not installed or not a contract, no-op update flags), and reference the specific revert reasons or error identifiers used by the implementation (so integrators know exactly when each function will revert).src/test/policies/price/PriceConfig.v2.t.sol (2)
1172-2039: Align new test names with the branching-tree naming format.Most newly added tests use
test_queue.../test_execute...naming instead of the requiredtest_given<Condition>_<Action>_<ExpectedResult>()convention.As per coding guidelines, “Follow branching tree naming for tests:
test_given<Condition>_<Action>_<ExpectedResult>()”.Also applies to: 2200-2556
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/policies/price/PriceConfig.v2.t.sol` around lines 1172 - 2039, Rename the newly added test functions to follow the branching-tree naming convention test_given<Condition>_<Action>_<ExpectedResult(), e.g. rename test_queueRemoveAsset_givenDisabled_reverts -> test_givenContractDisabled_queueRemoveAsset_reverts and similarly update test_queueRemoveAsset_unauthorizedUser_reverts, test_queueRemoveAsset_whenAssetIsUnapproved_reverts, test_queueRemoveAsset_whenAssetIsUnitOfAccount_reverts, test_queueRemoveAsset_givenRawPayload_revalidatesAsset, test_queueRemoveAsset, test_queueRemoveAsset_queuesExpectedAction, and all test_queueUpdateAsset*, test_queueTimelockDelay*, and test_executeQueuedAction* variants to the format test_given<Condition>_<Action>_<ExpectedResult> so names like test_executeQueuedAction_beforeDelay_givenQueueRemoveAsset_reverts become test_givenActionQueued_beforeTimelock_executeRemoveAsset_reverts (keep unique identifiers like queueRemoveAsset, queueUpdateAsset, queueTimelockDelay, executeQueuedAction, and the QueuedActionCase enum values in names to preserve intent); update any references/calls/assertions that use these function names (tests or helpers) accordingly.
183-185: Prefix internal constants with_for consistency.Line 183 and Line 184 introduce internal constants without underscore prefixes. Please align with the repository naming convention for internal state.
As per coding guidelines, “Internal state variables MUST use underscore prefix (e.g.,
uint256 internal _counter;)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/policies/price/PriceConfig.v2.t.sol` around lines 183 - 185, Rename the two internal constant identifiers to follow the internal-state underscore convention: change TIMELOCK_DELAY and EXECUTION_WINDOW to use an underscore prefix (e.g., _TIMELOCK_DELAY and _EXECUTION_WINDOW) wherever they are declared and referenced (search for TIMELOCK_DELAY and EXECUTION_WINDOW in the test/contract code and update usages to the new names).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/modules/PRICE/IPRICE.v2.sol`:
- Around line 412-500: The NatSpec for the new validator functions is missing
explicit revert conditions; update the comments for validateAddAsset,
validateRemoveAsset, validateUpdateAsset, validateInstallSubmodule,
validateUpgradeSubmodule and validateExecOnSubmodule to list concrete revert
scenarios (e.g., invalid/zero addresses, asset already registered/not approved,
caller lacks permission, invalid movingAverageDuration_/observations_/params_,
submodule not installed or not a contract, no-op update flags), and reference
the specific revert reasons or error identifiers used by the implementation (so
integrators know exactly when each function will revert).
In `@src/test/policies/price/PriceConfig.v2.t.sol`:
- Around line 1172-2039: Rename the newly added test functions to follow the
branching-tree naming convention
test_given<Condition>_<Action>_<ExpectedResult(), e.g. rename
test_queueRemoveAsset_givenDisabled_reverts ->
test_givenContractDisabled_queueRemoveAsset_reverts and similarly update
test_queueRemoveAsset_unauthorizedUser_reverts,
test_queueRemoveAsset_whenAssetIsUnapproved_reverts,
test_queueRemoveAsset_whenAssetIsUnitOfAccount_reverts,
test_queueRemoveAsset_givenRawPayload_revalidatesAsset, test_queueRemoveAsset,
test_queueRemoveAsset_queuesExpectedAction, and all test_queueUpdateAsset*,
test_queueTimelockDelay*, and test_executeQueuedAction* variants to the format
test_given<Condition>_<Action>_<ExpectedResult> so names like
test_executeQueuedAction_beforeDelay_givenQueueRemoveAsset_reverts become
test_givenActionQueued_beforeTimelock_executeRemoveAsset_reverts (keep unique
identifiers like queueRemoveAsset, queueUpdateAsset, queueTimelockDelay,
executeQueuedAction, and the QueuedActionCase enum values in names to preserve
intent); update any references/calls/assertions that use these function names
(tests or helpers) accordingly.
- Around line 183-185: Rename the two internal constant identifiers to follow
the internal-state underscore convention: change TIMELOCK_DELAY and
EXECUTION_WINDOW to use an underscore prefix (e.g., _TIMELOCK_DELAY and
_EXECUTION_WINDOW) wherever they are declared and referenced (search for
TIMELOCK_DELAY and EXECUTION_WINDOW in the test/contract code and update usages
to the new names).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d9484c8-9441-4196-be8c-0b191aa1f24c
📒 Files selected for processing (10)
documentation/price.mdsnapshots/PriceV2GasTest.jsonsrc/modules/PRICE/IPRICE.v2.solsrc/modules/PRICE/OlympusPrice.v1_2.solsrc/modules/PRICE/OlympusPrice.v2.solsrc/modules/PRICE/PRICE.v2.solsrc/policies/interfaces/IPriceConfigv2.solsrc/policies/price/PriceConfig.v2.solsrc/test/mocks/MockPrice.v2.solsrc/test/policies/price/PriceConfig.v2.t.sol
✅ Files skipped from review due to trivial changes (2)
- src/policies/interfaces/IPriceConfigv2.sol
- documentation/price.md
🚧 Files skipped from review as they are similar to previous changes (2)
- snapshots/PriceV2GasTest.json
- src/test/mocks/MockPrice.v2.sol
Contributor
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 `@src/policies/price/PriceConfig.v2.sol`:
- Around line 603-613: queueUpdateAsset currently enqueues only SubKeycodes
(params_.feeds and params_.strategy) so a later queueUpgradeSubmodule call can
change the implementation before execution; to fix, when creating the queued
payload in queueUpdateAsset (the call to _queueAction wrapping
PRICE.updateAsset.selector), snapshot the currently installed implementation
addresses/identifiers for every feed and strategy submodule referenced in
params_.feeds and params_.strategy and include those snapshots in the encoded
payload, then modify executeQueuedAction / PRICE.updateAsset execution path to
verify the live implementations still match the stored snapshots and reject
execution if any differ; reference queueUpdateAsset, params_.feeds,
params_.strategy, _queueAction, PRICE.updateAsset.selector,
queueUpgradeSubmodule and executeQueuedAction when locating where to add
snapshot capture and runtime validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 019c4920-8376-4c3e-a29b-356ea4232bf3
📒 Files selected for processing (12)
documentation/price.mdsnapshots/PriceV2GasTest.jsonsrc/modules/PRICE/IPRICE.v2.solsrc/modules/PRICE/OlympusPrice.v1_2.solsrc/modules/PRICE/OlympusPrice.v2.solsrc/modules/PRICE/PRICE.v2.solsrc/policies/interfaces/IPriceConfigv2.solsrc/policies/price/PriceConfig.v2.solsrc/policies/utils/TimelockQueue.solsrc/test/mocks/MockPrice.v2.solsrc/test/policies/price/PriceConfig.v2.t.solsrc/test/policies/utils/TimelockQueue/TimelockQueue.t.sol
✅ Files skipped from review due to trivial changes (2)
- snapshots/PriceV2GasTest.json
- documentation/price.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/mocks/MockPrice.v2.sol
- src/modules/PRICE/IPRICE.v2.sol
- src/policies/interfaces/IPriceConfigv2.sol
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/policies/price/PriceConfig.v2.t.sol (3)
1582-1635: Add descriptive messages to the new assertions.The new timelock tests introduce several bare
assertEqcalls here, which makes failures much harder to triage in fuzz runs. As per coding guidelines,src/test/**/*.t.sol:All assertions must have descriptive messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/policies/price/PriceConfig.v2.t.sol` around lines 1582 - 1635, The assertions in test_queueUpdateAsset_whenUpdateFeedsIsTrue lack descriptive messages; update each assertEq comparing feeds.length, fromSubKeycode(feeds[0].target) vs fromSubKeycode(params.feeds[0].target), feeds[0].selector, and feeds[0].params to include clear failure messages (e.g., "feeds length after execution", "feed target mismatch", "feed selector mismatch", "feed params mismatch") so test failures are actionable; modify the asserts in this test function (and any similar asserts nearby) to pass a descriptive string as the final argument per the test guideline.
2383-2428: Cover thepriceManagersuccess path forqueueUpgradeSubmodule.The queue auth hook in
src/policies/price/PriceConfig.v2.sol:236-271allows both admin andprice_adminfor queued PRICE actions, but these upgrade tests only prove the admin happy path. A regression that accidentally excludesprice_adminwould still pass this suite.Also applies to: 2430-2460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/policies/price/PriceConfig.v2.t.sol` around lines 2383 - 2428, Add a positive test that asserts priceManager (price_admin) can queue the submodule upgrade: in the existing test (or a new one) call vm.prank(priceManager); uint64 actionId = priceConfig.queueUpgradeSubmodule(address(newChainlink)); then assert the submodule is unchanged until _executeQueuedAction(actionId) is called and finally assert the submodule address and VERSION() reflect the upgrade; reference priceConfig.queueUpgradeSubmodule, priceManager, _executeQueuedAction, and the new MockUpgradedSubmodulePrice instance. Also add the same priceManager-success assertion for the second related test block around lines 2430-2460 to cover both cases.
321-324: Warp to the queued action’sexecutableAt, not the bootstrap constant.This helper hard-codes the original
TIMELOCK_DELAY, so it becomes wrong as soon as a test queues an action afterqueueTimelockDelayhas taken effect. ReadinggetQueuedAction(actionId_).executableAthere keeps the helper aligned with the feature this PR adds.Suggested refactor
function _executeQueuedAction(uint64 actionId_) internal { - _warpPastTimelockDelay(); + ITimelockQueue.QueuedAction memory action = priceConfig.getQueuedAction(actionId_); + if (block.timestamp < action.executableAt) vm.warp(action.executableAt); priceConfig.executeQueuedAction(actionId_); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/policies/price/PriceConfig.v2.t.sol` around lines 321 - 324, The helper _executeQueuedAction currently warps to a hard-coded TIMELOCK_DELAY via _warpPastTimelockDelay, which breaks when tests change timelock via queueTimelockDelay; instead read the queued action's executableAt timestamp from priceConfig.getQueuedAction(actionId_).executableAt and warp to that time before calling priceConfig.executeQueuedAction(actionId_), so the helper always aligns with the queued action's actual executable time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/policies/price/PriceConfig.v2.t.sol`:
- Around line 1582-1635: The assertions in
test_queueUpdateAsset_whenUpdateFeedsIsTrue lack descriptive messages; update
each assertEq comparing feeds.length, fromSubKeycode(feeds[0].target) vs
fromSubKeycode(params.feeds[0].target), feeds[0].selector, and feeds[0].params
to include clear failure messages (e.g., "feeds length after execution", "feed
target mismatch", "feed selector mismatch", "feed params mismatch") so test
failures are actionable; modify the asserts in this test function (and any
similar asserts nearby) to pass a descriptive string as the final argument per
the test guideline.
- Around line 2383-2428: Add a positive test that asserts priceManager
(price_admin) can queue the submodule upgrade: in the existing test (or a new
one) call vm.prank(priceManager); uint64 actionId =
priceConfig.queueUpgradeSubmodule(address(newChainlink)); then assert the
submodule is unchanged until _executeQueuedAction(actionId) is called and
finally assert the submodule address and VERSION() reflect the upgrade;
reference priceConfig.queueUpgradeSubmodule, priceManager, _executeQueuedAction,
and the new MockUpgradedSubmodulePrice instance. Also add the same
priceManager-success assertion for the second related test block around lines
2430-2460 to cover both cases.
- Around line 321-324: The helper _executeQueuedAction currently warps to a
hard-coded TIMELOCK_DELAY via _warpPastTimelockDelay, which breaks when tests
change timelock via queueTimelockDelay; instead read the queued action's
executableAt timestamp from priceConfig.getQueuedAction(actionId_).executableAt
and warp to that time before calling priceConfig.executeQueuedAction(actionId_),
so the helper always aligns with the queued action's actual executable time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f38aec2a-bb6b-4d67-b2a8-24a3a9daa247
📒 Files selected for processing (1)
src/test/policies/price/PriceConfig.v2.t.sol
…/olympus-v3 into price-config-v2-timelock-review # Conflicts: # snapshots/PriceV2GasTest.json # src/modules/PRICE/OlympusPrice.v1_2.sol
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
# Conflicts: # snapshots/PriceV2GasTest.json
…tegy-validation Price Feed Resilience: PriceConfig timelock (with strategy validation)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/test/modules/PRICE.v2/PRICE.v2.t.sol (1)
1932-1934: ⚡ Quick winRename the new tests to the repo's branching-tree convention.
This and the other newly added cases in this file use the older ad-hoc naming pattern. Please rename them to
test_given<Condition>_<Action>_<ExpectedResult>()so the added coverage stays consistent with the repo's test structure.As per coding guidelines, "Follow branching tree naming for tests:
test_given<Condition>_<Action>_<ExpectedResult>()."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/modules/PRICE.v2/PRICE.v2.t.sol` around lines 1932 - 1934, Rename the test function test_storeObservation_withFirstNonZeroStrategy_singleFeed_useMovingAverage_excludesMovingAverageFromStoredObservation to follow the branching-tree convention; update the function name to test_givenFirstNonZeroStrategySingleFeed_storeObservation_excludesMovingAverageFromStoredObservation (locate the function by its current name and change the identifier and any references to it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/PRICE/OlympusPrice.v2.sol`:
- Around line 808-882: The current _validateUpdateAsset only checks
structure/installation and can allow queued updates that will fail at execution;
update _validateUpdateAsset to mirror the runtime path in updateAsset (lines
~1147-1158) by resolving the final strategy and feeds exactly as updateAsset
would (use params_.feeds or abi.decode(asset.feeds), and params_.strategy or
abi.decode(asset.strategy,(Component))) and then call the same runtime
validators: _validateAssetConfiguration, _validateAssetPriceFeeds (on
finalFeeds), _validateAssetPriceStrategy (on finalStrategy) and
_validateAssetMovingAverage as appropriate; ensure you also validate feed
selectors/non-zero addresses and raw/CURRENT count semantics the same way
updateAsset enforces so the queued payload is guaranteed to succeed when
executed.
- Around line 109-113: The registerNonContractAsset function currently collapses
the contract-address check into PRICE_InvalidAsset; instead separate the checks
so you use the contract-specific error for contract addresses: first revert
PRICE_InvalidAsset(asset_) for address(0), then if (asset_.code.length != 0)
revert PRICE_ContractAsset(asset_), then if (isNonContractAsset[asset_]) revert
PRICE_InvalidAsset(asset_); finally set isNonContractAsset[asset_] = true.
Update the revert ordering and error selectors in registerNonContractAsset to
preserve the original, more precise failure reasons.
- Around line 73-77: The supportsInterface override in function
supportsInterface(bytes4) currently hard-codes IVersioned, IPRICEv2 and IERC165
and omits the parent ModuleWithSubmodules' advertised interface (ISubmodule),
causing parent interface queries to fail; fix by either adding
type(ISubmodule).interfaceId to the OR-list in supportsInterface or by
delegating to the parent (super.supportsInterface(interfaceId_)) so
ModuleWithSubmodules' interfaces are preserved—update the supportsInterface
function accordingly.
---
Nitpick comments:
In `@src/test/modules/PRICE.v2/PRICE.v2.t.sol`:
- Around line 1932-1934: Rename the test function
test_storeObservation_withFirstNonZeroStrategy_singleFeed_useMovingAverage_excludesMovingAverageFromStoredObservation
to follow the branching-tree convention; update the function name to
test_givenFirstNonZeroStrategySingleFeed_storeObservation_excludesMovingAverageFromStoredObservation
(locate the function by its current name and change the identifier and any
references to it).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d26489dc-6218-4766-9dac-383ed5e5ec16
📒 Files selected for processing (5)
snapshots/PriceV2GasTest.jsonsrc/modules/PRICE/OlympusPrice.v2.solsrc/test/modules/PRICE.v2/PRICE.v2.t.solsrc/test/modules/PRICE.v2/PriceV2BaseTest.solsrc/test/modules/PRICE.v2/updateAsset.t.sol
✅ Files skipped from review due to trivial changes (1)
- snapshots/PriceV2GasTest.json
This was referenced Jul 13, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
removeAsset,updateAsset,upgradeSubmodule, andexecOnSubmodule; keep emergency cancellation available, including while PriceConfig is disabled.ROLES.md, anddocumentation/price.mdto describe the timelock behavior and role model.IPRICEv2/OlympusPricev2so PriceConfig preflights PRICE-managed invariants without duplicating them.Validation
forge build --sizes --contracts src/modules/PRICE/OlympusPrice.v1_2.solforge test -vvv --match-contract PriceConfigv2Testforge test -vvv --match-path src/test/modules/PRICE.v2/updateAsset.t.solforge test -vvv --match-path src/test/modules/PRICE.v2/SubmoduleInstallation.t.solforge test -vvv --match-contract PriceV2GasTestpnpm run prettierSummary by CodeRabbit
New Features
Improvements
Documentation
Tests