Price Feed Resilience: Prepare PRICE v1.2 deployment and oracle proposal simulation - #268
Merged
Merged
Conversation
… shutdown detection
…date-emergency-config.js`
Improve shutdown detection in the `emergency-config.json`
chore(ci): standardize pnpm-only workflows and guard installs
…26-04-02 chore: hardening baseline before Dependabot remediation
Implement the fork-simulated proposal and validation needed to raise the Cooler V2 origination LTV while temporarily increasing the oracle rate guard so the schedule can execute.
Keep the established OIP proposal naming and single-file script pattern while silencing the targeted solhint warnings for this proposal file.
Update the proposal description to state that the required schedule rate is measured at submission time and add a Markdown copy for previewing the rendered text.
Drop the temporary Markdown extraction file before opening the proposal PR so the branch only carries the proposal and test changes.
script: Add missing script for transferring ownership of CCIP token pool
chore(lint): exclude irrelevant forge lint rules
Add Sepolia Cooler OLTV update script
Add forge lint inline annotations
# Conflicts: # .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.yml
* chore: harden pnpm installs * chore: pin pnpm version in workflows * chore: harden pnpm configuration * chore: move pnpm config to workspace * chore: format pnpm workspace config
docs(workflow): streamline repo publish process
Patch brace-expansion vulnerability
Base automatically changed from
price-config-v2-timelock-review
to
price-feed-improvements-fixes
May 20, 2026 12:13
Base automatically changed from
price-feed-improvements-fixes
to
price-feed-improvements
May 20, 2026 12:52
0xJem
marked this pull request as ready for review
May 20, 2026 12:57
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/tests-fork.yml (1)
9-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit permissions and disable checkout credential persistence.
Add a
permissions:block withcontents: readafter theon:trigger section, and addpersist-credentials: falseto the checkout step.This follows GitHub Actions security best practices for least-privilege access and reduces credential exposure risk. Other workflows in the project (audit.yml, lint.yml) already implement this pattern.
Suggested patch
on: push: branches: - master pull_request: +permissions: + contents: read + jobs: fork-tests: @@ - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/tests-fork.yml around lines 9 - 30, Add a repository-level permissions block and disable checkout credential persistence: insert a top-level permissions: block with contents: read (immediately after the on: trigger) to enforce least-privilege, and in the fork-tests job modify the Checkout step (the step using actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd) to include persist-credentials: false so credentials are not stored for subsequent steps.
🧹 Nitpick comments (3)
src/proposals/OracleProposal.sol (1)
33-49: ⚡ Quick winUse underscore-prefixed names for new internal constants.
New internal constants (e.g.,
CHAINLINK_BTC_USD,PYTH_ETH_USD_UPDATE_THRESHOLD) should follow the internal underscore naming rule for consistency with repo standards.As per coding guidelines:
**/*.sol: "Internal state variables MUST use underscore prefix (e.g.,uint256 internal _counter;)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/proposals/OracleProposal.sol` around lines 33 - 49, The new internal constants violate the repo naming convention—internal state must use an underscore prefix; rename each internal constant (e.g., CHAINLINK_BTC_USD, CHAINLINK_ETH_USD, PYTH, REDSTONE_ETH_USD, PYTH_ETH_USD_ID, PYTH_USDS_USD_ID, PYTH_ETH_USD_UPDATE_THRESHOLD, PYTH_USDS_USD_UPDATE_THRESHOLD, etc.) to the underscore-prefixed form (e.g., _CHAINLINK_BTC_USD, _PYTH_ETH_USD_UPDATE_THRESHOLD) and update any references to those symbols in this contract so compilation and style checks pass.src/scripts/ops/batches/ConfigurePriceV1_2.sol (1)
54-54: ⚡ Quick winRename
PRICE_SCALEto match internal underscore naming.
PRICE_SCALEis an internal constant and should use the underscore-prefixed internal naming convention.As per coding guidelines:
**/*.sol: "Internal state variables MUST use underscore prefix (e.g.,uint256 internal _counter;)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/scripts/ops/batches/ConfigurePriceV1_2.sol` at line 54, The constant PRICE_SCALE is an internal state constant that doesn't follow the project's internal underscore naming convention; rename the symbol PRICE_SCALE to _PRICE_SCALE everywhere it's declared and referenced (preserve type and value: uint256 internal constant _PRICE_SCALE = 1e18) and update all usages in ConfigurePriceV1_2 (and any other files importing or referencing it) to use _PRICE_SCALE to satisfy the internal underscore naming rule.AGENTS.md (1)
57-72: ⚡ Quick winKeep
AGENTS.mdtool-agnostic; move tool-specific commands elsewhere.These additions hardcode tool-specific workflow/CLI details in
AGENTS.md, which conflicts with the file’s portability requirement. Keep agent contracts/capabilities here and move concrete tool commands to a separate tooling playbook.As per coding guidelines:
**/AGENTS.md: "Document agent responsibilities, capabilities, and interfaces in a tool-agnostic manner" and "Keep agent documentation free from tool-specific implementation details to ensure portability across different tool ecosystems."Also applies to: 461-462
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 57 - 72, The Validation Gates section in AGENTS.md contains tool-specific commands (e.g., `pnpm run lint`, `pnpm build`/`forge build`, `pnpm run test`, and the CodeRabbit example `coderabbit review --agent --base <PR base>`) which violates the requirement to keep agent docs tool-agnostic; remove these concrete CLI examples from the "Validation Gates" section and replace them with generic guidance like "run the repository's existing lint/format, build, and test commands" and a note to run any repo-specific pre-push review tools when appropriate, then create a separate tooling playbook (e.g., TOOLING_PLAYBOOK or docs/tooling-playbook) that houses the exact commands and examples (including the CodeRabbit invocation) and update AGENTS.md to link to that playbook; ensure the "agent responsibilities/capabilities" and the "Validation Gates" header remain but contain no hardcoded tool invocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/audit.yml:
- Around line 15-16: The Checkout step using
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd should disable
credential persistence; update the Checkout job step (the actions/checkout
invocation) to include persist-credentials: false so the runner does not keep
the git token in the environment for later steps.
In @.github/workflows/coverage.yml:
- Around line 9-14: The coverage workflow should explicitly set minimal
permissions and disable checkout credential persistence: add a top-level
permissions block with "contents: read" for the coverage workflow and update the
Checkout step that uses
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd to include
"persist-credentials: false"; locate the workflow by the "coverage" job name and
the Checkout step using the actions/checkout reference to apply these changes.
In @.github/workflows/lint.yml:
- Around line 16-17: The Checkout step using
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd currently leaves
credentials persisted; update the "Checkout" step to include the setting
persist-credentials: false so the GitHub token is not stored for subsequent
steps, ensuring the checkout action's configuration explicitly disables
credential persistence.
In @.github/workflows/size.yml:
- Around line 9-14: The workflow job "size-check" should be hardened by adding
explicit minimal permissions and disabling checkout credential persistence: add
a top-level permissions block with "contents: read" for the workflow and update
the "Checkout" step (uses:
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd) to include "with:
persist-credentials: false" so the runner does not automatically configure
GITHUB_TOKEN credentials.
In @.github/workflows/tests-crosschain.yml:
- Around line 9-17: The workflow job "crosschain-tests" is running with default
checkout credentials and no explicit permissions; update the workflow to harden
token scope by adding a top-level permissions block (e.g., set
permissions.contents: read) and modify the actions/checkout step (the step using
actions/checkout@de0fac2e4500d...) to disable credential persistence by setting
persist-credentials: false; ensure these changes apply before the "Bootstrap"
step so the job "crosschain-tests" runs with read-only content access and no
persisted GITHUB_TOKEN credentials.
In @.github/workflows/tests-proposals.yml:
- Around line 9-17: The workflow job "proposal-tests" is missing a permissions
block and its Checkout step (uses: actions/checkout) leaves credentials
persisted; add a minimal permissions block for the job (scoping to only the
permissions needed for the steps) and update the Checkout step to disable
credential persistence by setting persist-credentials to false and a shallow
fetch (e.g., fetch-depth: 1) to reduce surface area; locate the "proposal-tests"
job and the step named "Checkout" to apply these changes, and ensure other steps
like the "Bootstrap" step still function with the tightened permissions.
In @.github/workflows/tests-unit.yml:
- Around line 9-17: The workflow job "unit-tests" is missing an explicit
permissions block and the "actions/checkout" step is allowing credential
persistence; add a top-level permissions entry for the job (e.g., limit to
contents: read and any other minimal permissions the job needs) and update the
Checkout step (uses: actions/checkout) to include persist-credentials: false to
prevent checkout from leaving GITHUB_TOKEN credentials in the workspace; ensure
these changes are applied within the unit-tests job and keep the existing
Bootstrap step (uses: ./.github/actions/bootstrap) unchanged.
In @.github/workflows/validate-emergency-config.yml:
- Around line 29-30: The workflow currently checks out code with
actions/checkout@de0fac2e... which leaves GITHUB_TOKEN in git config; update the
Checkout step to set persist-credentials: false so credentials are not stored
for subsequent steps. Locate the Checkout step that uses actions/checkout and
add the persist-credentials: false input to that step to reduce token exposure
during this job.
In `@documentation/emergency/emergency-config.json`:
- Line 773: The "lastUpdated" value in the emergency-config.json (the JSON
property "lastUpdated") is stale; update that field to the actual ISO‑8601 UTC
timestamp representing when this config was changed in this PR (e.g.,
YYYY‑MM‑DDTHH:MM:SSZ), committing the updated timestamp so the emergency
procedures metadata audit trail reflects the real modification time.
In `@shell/validate-emergency-config.js`:
- Around line 273-280: The current validation only checks that
contracts[scContractName] exists but allows a zero address; update the loop that
iterates component.availableOn to also reject zero/empty addresses by verifying
config.chains[chainName].contracts[scContractName] is a non-zero Ethereum
address (not "0x0000000000000000000000000000000000000000" or equivalent), and
push the same error message to errors when it is zero/empty; reference
scContractName, component.availableOn, config.chains and the errors push to
locate and modify the check.
In `@src/proposals/OIP_194A.sol`:
- Around line 142-160: Replace the string-based require(...) checks in _validate
with custom error types: declare appropriate error definitions (e.g.,
InvalidTargetValue(), InvalidTargetTime(), StartTimeInFuture(),
TargetNotInFuture(), NonDecreasingTarget(), NonPositiveSlope(),
MaxRateOfChangeNotRestored(), SlopeMismatch(), CurrentLtvNotStartingValue()) in
the contract or in the parent interface and revert using those errors instead of
the string messages; update checks that reference TARGET_ORIGINATION_LTV,
TARGET_ORIGINATION_LTV_TIME, startTime, targetTime, startingValue, slope,
ltvOracle.maxOriginationLtvRateOfChange(), expectedSlope, and
ltvOracle.currentOriginationLtv() to use the new custom errors for each failing
condition.
In `@src/proposals/OracleProposal.sol`:
- Around line 275-278: The Pyth-mocking block in OracleProposal.sol currently
hardcodes Pyth prices (the code around the Pyth/mock feed setup that spans the
earlier comment and the hardcoded values at lines noted in review), which
overrides real fork feed answers; change that block to read the existing feed
answers from the forked chain and reapply those numeric values to the mocked
Pyth feeds while only updating their timestamps to the simulated timelock
execution time. Concretely, locate the Pyth mock setup (the function/section
that writes Pyth feed answers) and replace the hardcoded constants with code
that: (1) queries the current feed answers from the forked provider for each
feed id, (2) uses those answer values when writing the mocked feed, and (3) sets
the answer timestamp to the simulated execution time; keep the rest of the
mocking logic intact so tests exercise real fork prices with shifted timestamps.
- Around line 120-121: The description() function call contains adjacent string
literals without a separating comma; add a trailing comma after the string
ending with "price.md)\n" so the subsequent string "- [Oracle
Documentation](...oracle_factories.md)\n" is a separate argument and the file
will compile (update the string list in description()).
In `@src/scripts/ops/batches/CCIPTokenPool.sol`:
- Around line 245-260: The acceptTokenPoolOwnership function is using
setUpWithChainId(false) which selects the non-DAO signer context, but
Ownable2Step.acceptOwnership must be called by the pending owner (DAO multisig);
update signer selection so the DAO-capable signer is used (or make signer
explicit) instead of hardcoding false, and before calling
addToBatch/proposeBatch precheck that
LockReleaseTokenPool(tokenPool).pendingOwner() equals the DAO address from
_envAddressNotZero("olympus.multisig.dao") (and bail/console.log if not) to
avoid creating a batch that will revert. Ensure acceptTokenPoolOwnership,
setUpWithChainId, _getTokenPoolAddressNotZero, _envAddressNotZero,
LockReleaseTokenPool.owner()/pendingOwner(), addToBatch, and proposeBatch are
adjusted accordingly.
---
Outside diff comments:
In @.github/workflows/tests-fork.yml:
- Around line 9-30: Add a repository-level permissions block and disable
checkout credential persistence: insert a top-level permissions: block with
contents: read (immediately after the on: trigger) to enforce least-privilege,
and in the fork-tests job modify the Checkout step (the step using
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd) to include
persist-credentials: false so credentials are not stored for subsequent steps.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 57-72: The Validation Gates section in AGENTS.md contains
tool-specific commands (e.g., `pnpm run lint`, `pnpm build`/`forge build`, `pnpm
run test`, and the CodeRabbit example `coderabbit review --agent --base <PR
base>`) which violates the requirement to keep agent docs tool-agnostic; remove
these concrete CLI examples from the "Validation Gates" section and replace them
with generic guidance like "run the repository's existing lint/format, build,
and test commands" and a note to run any repo-specific pre-push review tools
when appropriate, then create a separate tooling playbook (e.g.,
TOOLING_PLAYBOOK or docs/tooling-playbook) that houses the exact commands and
examples (including the CodeRabbit invocation) and update AGENTS.md to link to
that playbook; ensure the "agent responsibilities/capabilities" and the
"Validation Gates" header remain but contain no hardcoded tool invocations.
In `@src/proposals/OracleProposal.sol`:
- Around line 33-49: The new internal constants violate the repo naming
convention—internal state must use an underscore prefix; rename each internal
constant (e.g., CHAINLINK_BTC_USD, CHAINLINK_ETH_USD, PYTH, REDSTONE_ETH_USD,
PYTH_ETH_USD_ID, PYTH_USDS_USD_ID, PYTH_ETH_USD_UPDATE_THRESHOLD,
PYTH_USDS_USD_UPDATE_THRESHOLD, etc.) to the underscore-prefixed form (e.g.,
_CHAINLINK_BTC_USD, _PYTH_ETH_USD_UPDATE_THRESHOLD) and update any references to
those symbols in this contract so compilation and style checks pass.
In `@src/scripts/ops/batches/ConfigurePriceV1_2.sol`:
- Line 54: The constant PRICE_SCALE is an internal state constant that doesn't
follow the project's internal underscore naming convention; rename the symbol
PRICE_SCALE to _PRICE_SCALE everywhere it's declared and referenced (preserve
type and value: uint256 internal constant _PRICE_SCALE = 1e18) and update all
usages in ConfigurePriceV1_2 (and any other files importing or referencing it)
to use _PRICE_SCALE to satisfy the internal underscore naming rule.
🪄 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: 60e2f22f-41a4-4f8d-998b-dfe0ad90854a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (43)
.claude/commands/update-emergency-config.md.coderabbit.yaml.codex/environments/environment.toml.github/actions/bootstrap/action.yml.github/workflows/audit.yml.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.yml.gitignore.node-version.npmrc.nvmrcAGENTS.mdREADME.mddocumentation/emergency/emergency-abis.jsondocumentation/emergency/emergency-config.jsondocumentation/emergency/emergency-config.schema.jsondocumentation/price.mdfoundry.tomlpackage.jsonpnpm-workspace.yamlshell/deployV3.shshell/validate-emergency-config.jssrc/external/governance/interfaces/ITimelock.solsrc/proposals/OIP_194A.solsrc/proposals/OracleProposal.solsrc/scripts/deploy/DeployV3.s.solsrc/scripts/env.jsonsrc/scripts/ops/CalculateCoolerLtvUpdate.s.solsrc/scripts/ops/ChangeKernelExecutor.s.solsrc/scripts/ops/batches/CCIPTokenPool.solsrc/scripts/ops/batches/ConfigureOracles.solsrc/scripts/ops/batches/ConfigurePriceV1_2.solsrc/scripts/ops/batches/args/ConfigurePriceV1_2.jsonsrc/scripts/ops/lib/BatchScriptV2.solsrc/scripts/proposals/executeOnAnvilFork.shsrc/test/modules/PRICE/OlympusPricev1_2Fork.t.solsrc/test/proposals/OIP_194A.t.sol
💤 Files with no reviewable changes (1)
- .npmrc
Comment on lines
+273
to
+280
| // Validate contractKey resolves to a known contract on every availableOn chain | ||
| const scContractName = sc.contractKey.split(".").pop(); | ||
| for (const chainName of component.availableOn) { | ||
| const contracts = config.chains[chainName] && config.chains[chainName].contracts; | ||
| if (!contracts || !contracts[scContractName]) { | ||
| errors.push( | ||
| `Component "${component.id}" statusCheck contractKey "${sc.contractKey}" has no address in chain "${chainName}"` | ||
| ); |
Contributor
There was a problem hiding this comment.
Reject zero-address statusCheck targets.
Line 277 only checks key presence. A zero address currently passes and would still fail operational status reads.
Suggested fix
- const scContractName = sc.contractKey.split(".").pop();
+ const scContractName = sc.contractKey.split(".").pop();
for (const chainName of component.availableOn) {
const contracts = config.chains[chainName] && config.chains[chainName].contracts;
- if (!contracts || !contracts[scContractName]) {
+ const scAddress = contracts && contracts[scContractName];
+ if (!scAddress || scAddress === ZERO_ADDRESS) {
errors.push(
- `Component "${component.id}" statusCheck contractKey "${sc.contractKey}" has no address in chain "${chainName}"`
+ `Component "${component.id}" statusCheck contractKey "${sc.contractKey}" has no valid non-zero address in chain "${chainName}"`
);
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shell/validate-emergency-config.js` around lines 273 - 280, The current
validation only checks that contracts[scContractName] exists but allows a zero
address; update the loop that iterates component.availableOn to also reject
zero/empty addresses by verifying
config.chains[chainName].contracts[scContractName] is a non-zero Ethereum
address (not "0x0000000000000000000000000000000000000000" or equivalent), and
push the same error message to errors when it is zero/empty; reference
scContractName, component.availableOn, config.chains and the errors push to
locate and modify the check.
Comment on lines
+275
to
+278
| /// The proposal actions depend on PRICE during simulation because deploying the | ||
| /// cache-backed oracle clones seeds/validates the OHM/USDS cache. Preserve the feed answers | ||
| /// from the fork, but move their timestamps to the simulated timelock execution time so the | ||
| /// simulation exercises the proposal logic instead of failing on fork-only clock drift. |
Contributor
There was a problem hiding this comment.
Pyth mocking does not preserve fork feed values.
The comment says simulation should preserve feed answers and only shift timestamps, but Lines 297-304 hardcode Pyth prices. This can mask proposal failures/successes that depend on real fork prices.
Also applies to: 293-304
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/proposals/OracleProposal.sol` around lines 275 - 278, The Pyth-mocking
block in OracleProposal.sol currently hardcodes Pyth prices (the code around the
Pyth/mock feed setup that spans the earlier comment and the hardcoded values at
lines noted in review), which overrides real fork feed answers; change that
block to read the existing feed answers from the forked chain and reapply those
numeric values to the mocked Pyth feeds while only updating their timestamps to
the simulated timelock execution time. Concretely, locate the Pyth mock setup
(the function/section that writes Pyth feed answers) and replace the hardcoded
constants with code that: (1) queries the current feed answers from the forked
provider for each feed id, (2) uses those answer values when writing the mocked
feed, and (3) sets the answer timestamp to the simulated execution time; keep
the rest of the mocking logic intact so tests exercise real fork prices with
shifted timestamps.
Comment on lines
+245
to
+260
| function acceptTokenPoolOwnership() external setUpWithChainId(false) { | ||
| address tokenPool = _getTokenPoolAddressNotZero(chain); | ||
| address daoMS = _envAddressNotZero("olympus.multisig.dao"); | ||
|
|
||
| // Check if the owner is already the DAO MS | ||
| if (LockReleaseTokenPool(tokenPool).owner() == daoMS) { | ||
| console2.log("Owner already transferred to", daoMS, ". Skipping."); | ||
| return; | ||
| } | ||
|
|
||
| console2.log("Accepting ownership of", tokenPool, "to", daoMS); | ||
| addToBatch(tokenPool, abi.encodeWithSelector(Ownable2Step.acceptOwnership.selector)); | ||
|
|
||
| // Run | ||
| proposeBatch(); | ||
|
|
Contributor
There was a problem hiding this comment.
acceptTokenPoolOwnership is wired to the wrong signer context.
At Line 245, setUpWithChainId(false) hardcodes the non-DAO path, but acceptOwnership() must be executed by the pending owner (DAO multisig). This can generate a batch that always reverts at execution time. Make signer selection DAO-capable (or explicit), and precheck pendingOwner before batching.
Proposed fix
- function acceptTokenPoolOwnership() external setUpWithChainId(false) {
+ function acceptTokenPoolOwnership(bool useDaoMS_) external setUpWithChainId(useDaoMS_) {
address tokenPool = _getTokenPoolAddressNotZero(chain);
address daoMS = _envAddressNotZero("olympus.multisig.dao");
// Check if the owner is already the DAO MS
if (LockReleaseTokenPool(tokenPool).owner() == daoMS) {
console2.log("Owner already transferred to", daoMS, ". Skipping.");
return;
}
+ if (Ownable2Step(tokenPool).pendingOwner() != daoMS) {
+ console2.log("Pending owner is not DAO MS. Skipping.");
+ return;
+ }
console2.log("Accepting ownership of", tokenPool, "to", daoMS);
addToBatch(tokenPool, abi.encodeWithSelector(Ownable2Step.acceptOwnership.selector));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/scripts/ops/batches/CCIPTokenPool.sol` around lines 245 - 260, The
acceptTokenPoolOwnership function is using setUpWithChainId(false) which selects
the non-DAO signer context, but Ownable2Step.acceptOwnership must be called by
the pending owner (DAO multisig); update signer selection so the DAO-capable
signer is used (or make signer explicit) instead of hardcoding false, and before
calling addToBatch/proposeBatch precheck that
LockReleaseTokenPool(tokenPool).pendingOwner() equals the DAO address from
_envAddressNotZero("olympus.multisig.dao") (and bail/console.log if not) to
avoid creating a batch that will revert. Ensure acceptTokenPoolOwnership,
setUpWithChainId, _getTokenPoolAddressNotZero, _envAddressNotZero,
LockReleaseTokenPool.owner()/pendingOwner(), addToBatch, and proposeBatch are
adjusted accordingly.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/proposals/OracleProposal.sol (1)
293-304:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPyth mocking does not preserve fork feed values.
The comment at lines 270-280 states that the simulation should "preserve the feed answers from the fork" and only shift timestamps. The Chainlink mocking (lines 307-316) correctly reads fork data via
latestRoundData(). However, the Pyth mocking hardcodes prices here (ETH/USD:233654247160, USDS/USD:100000000) instead of reading them from the fork via the Pyth contract's existing methods.This can mask proposal failures or successes that depend on actual fork feed values, reducing test fidelity.
🔍 Suggested approach
Modify
_mockPythFeedAtor its call sites to:
- Query the current Pyth feed answer from the fork (e.g., via
getPriceUnsafeor similar)- Extract the price, conf, and expo fields from the fork response
- Pass those values into the mocked
IPyth.Pricestruct with onlypublishTimeset toexecutionTimestampThis preserves fork feed values while shifting timestamps, consistent with the Chainlink mocking pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/proposals/OracleProposal.sol` around lines 293 - 304, The Pyth mocks hardcode prices instead of preserving forked feed values; change the Pyth mocking to read the live forked feed for each ID (e.g., call PYTH.getPriceUnsafe(PYTH_ETH_USD_ID) and PYTH.getPriceUnsafe(PYTH_USDS_USD_ID) or the equivalent Pyth read method) and extract price, conf and expo, then call _mockPythFeedAt (or update its internals) passing IPyth.Price with those extracted price/conf/expo and only override publishTime to executionTimestamp so timestamps shift but feed values remain identical to the fork.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/proposals/OracleProposal.sol`:
- Around line 293-304: The Pyth mocks hardcode prices instead of preserving
forked feed values; change the Pyth mocking to read the live forked feed for
each ID (e.g., call PYTH.getPriceUnsafe(PYTH_ETH_USD_ID) and
PYTH.getPriceUnsafe(PYTH_USDS_USD_ID) or the equivalent Pyth read method) and
extract price, conf and expo, then call _mockPythFeedAt (or update its
internals) passing IPyth.Price with those extracted price/conf/expo and only
override publishTime to executionTimestamp so timestamps shift but feed values
remain identical to the fork.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62b58019-a6a4-4d2f-a81d-9341efbfbff9
📒 Files selected for processing (1)
src/proposals/OracleProposal.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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/validate-emergency-config.yml:
- Around line 29-32: Add an explicit read-only permissions block for the
GITHUB_TOKEN to harden least-privilege access: locate the workflow that contains
the Checkout step (the actions/checkout@... step with persist-credentials:
false) and add a top-level or job-level permissions entry specifying read-only
access (e.g., set contents: read) so the workflow no longer relies on default
GITHUB_TOKEN permissions.
🪄 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: 3f9edda4-15e4-4b7d-98c0-ab806199f9ae
📒 Files selected for processing (8)
.github/workflows/audit.yml.github/workflows/coverage.yml.github/workflows/lint.yml.github/workflows/size.yml.github/workflows/tests-crosschain.yml.github/workflows/tests-proposals.yml.github/workflows/tests-unit.yml.github/workflows/validate-emergency-config.yml
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
Why
The Oracle proposal simulation warps forward by the timelock delay, but a static Anvil fork does not receive the Chainlink/Pyth/RedStone updates that would occur during real elapsed time. That can make otherwise valid proposal actions fail on stale feeds while deploying/cache-validating the OHM/USDS oracle clones.
Validation
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores