Conversation
… shutdown detection
…date-emergency-config.js`
Improve shutdown detection in the `emergency-config.json`
chore(ci): standardize pnpm-only workflows and guard installs
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces an emergency ChangesEmergency Configuration System
CI Bootstrap Consolidation
Node / Package Manager & Developer Tooling
Governance Proposal: OIP-194A
Scripts / Environment & Batch Tooling
Sequence Diagram(s)sequenceDiagram
actor Governor
participant OIP as OIP_194A
participant Kernel
participant Oracle as CoolerV2_LTV_Oracle
Governor->>OIP: _deploy()
OIP->>Kernel: cache Kernel address
Governor->>OIP: _build()
OIP->>Oracle: read maxOriginationLtvRateOfChange
Oracle-->>OIP: currentRate
OIP->>Kernel: queue Action 1 (increase guard)
OIP->>Kernel: queue Action 2 (set target LTV/timestamp)
OIP->>Kernel: queue Action 3 (restore guard)
Governor->>OIP: _run()
OIP->>Kernel: execute queued actions
Kernel->>Oracle: apply timed actions
Governor->>OIP: _validate()
OIP->>Oracle: verify scheduled LTV, monotonicity, slope, guard restored
Oracle-->>OIP: validation data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/scripts/ops/lib/BatchScriptV2.sol (1)
104-117: ⚡ Quick winMake the ignored compatibility parameter explicit at runtime.
useDaoMS_is intentionally ignored, but it is currently silent. Adding an explicit no-op/log whentruereduces operator confusion in script invocations.Proposed minimal patch
modifier setUpWithTreasuryWorkingGroupMS( bool useDaoMS_, bool signOnly_, string memory argsFilePath_, string memory ledgerDerivationPath_, bytes memory signature_ ) { + if (useDaoMS_) { + console2.log("setUpWithTreasuryWorkingGroupMS: useDaoMS_ is ignored"); + } string memory chainName = ChainUtils._getChainName(block.chainid); _loadEnv(chainName); _loadArgs(argsFilePath_); address owner = _envAddressNotZero("olympus.multisig.treasuryWorkingGroup"); _setUpBatchScript(signOnly_, owner, ledgerDerivationPath_, signature_); _; }🤖 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/lib/BatchScriptV2.sol` around lines 104 - 117, In the modifier setUpWithTreasuryWorkingGroupMS, make the currently ignored parameter useDaoMS_ explicit at runtime by adding a small conditional that detects if useDaoMS_ is true and emits a clear warning (e.g., using console.log/console.warn from Hardhat's console or another project logging facility) so operators see the parameter was intentionally ignored; ensure you also reference/consume useDaoMS_ to avoid “unused variable” warnings and add an import for console (if using Hardhat) or call the existing logger used elsewhere in this file.
🤖 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.
Nitpick comments:
In `@src/scripts/ops/lib/BatchScriptV2.sol`:
- Around line 104-117: In the modifier setUpWithTreasuryWorkingGroupMS, make the
currently ignored parameter useDaoMS_ explicit at runtime by adding a small
conditional that detects if useDaoMS_ is true and emits a clear warning (e.g.,
using console.log/console.warn from Hardhat's console or another project logging
facility) so operators see the parameter was intentionally ignored; ensure you
also reference/consume useDaoMS_ to avoid “unused variable” warnings and add an
import for console (if using Hardhat) or call the existing logger used elsewhere
in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 487660f3-b812-4a62-981f-7233d04e6aef
📒 Files selected for processing (5)
.gitignoresrc/scripts/env.jsonsrc/scripts/ops/ChangeKernelExecutor.s.solsrc/scripts/ops/lib/BatchScriptV2.solsrc/test/proposals/OIP_194A.t.sol
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- src/test/proposals/OIP_194A.t.sol
- src/scripts/ops/ChangeKernelExecutor.s.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scripts/env.json
zeroxnoodle
previously approved these changes
May 5, 2026
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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/scripts/ops/batches/CCIPTokenPool.sol`:
- Line 245: acceptTokenPoolOwnership currently runs under
setUpWithChainId(false) which executes as the deployer/team multisig and
therefore cannot call Ownable2Step.acceptOwnership (only daoMS, the pending
owner set by transferTokenPoolOwnershipToDaoMS, can). Change the modifier on
acceptTokenPoolOwnership to run as the daoMS caller (e.g. use
setUpWithChainId(true) or another helper that impersonates daoMS) so the call to
acceptOwnership will be made by daoMS; ensure acceptTokenPoolOwnership invokes
Ownable2Step.acceptOwnership while msg.sender is daoMS and keep
transferTokenPoolOwnershipToDaoMS unchanged.
In `@src/scripts/ops/CalculateCoolerLtvUpdate.s.sol`:
- Around line 18-31: The README/example values for TARGET_OLTV are far too large
for uint96 and must be replaced with realistic 18-decimal OLTV values; update
the usage examples in the CalculateCoolerLtvUpdate.s.sol comment (references:
contract/script name UpdateCoolerLtv, function signatures updateOltv(uint96) and
updateOltvFromEnv()) to use values that fit uint96 and represent 18-decimal LTVs
(e.g. replace the 39‑digit examples with 1050000000000000000 for 105% LTV or
similar valid uint96 values) and ensure the note recommends using the env
variant for large-but-valid values.
- Around line 163-166: The env value read in updateOltvFromEnv uses
vm.envUint("TARGET_OLTV") which returns a uint256 and is directly cast to uint96
causing silent truncation; fix by keeping the value as uint256 first, validate
that it is <= type(uint96).max (and optionally >= 0 if needed), and only then
cast to uint96 and call updateOltv(targetOltv); if the check fails, revert or
vm.stop with a clear error mentioning TARGET_OLTV to avoid accidental
truncation.
🪄 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: 42d2258f-bc46-4bc1-9207-1777d2687108
📒 Files selected for processing (5)
.coderabbit.yaml.github/workflows/lint.ymlfoundry.tomlsrc/scripts/ops/CalculateCoolerLtvUpdate.s.solsrc/scripts/ops/batches/CCIPTokenPool.sol
✅ Files skipped from review due to trivial changes (1)
- foundry.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/lint.yml
| } | ||
|
|
||
| /// @notice Accepts the ownership of the TokenPool | ||
| function acceptTokenPoolOwnership() external setUpWithChainId(false) { |
Contributor
There was a problem hiding this comment.
setUpWithChainId(false) will cause acceptOwnership() to fail.
Ownable2Step.acceptOwnership() requires the pending owner (daoMS) to be the caller. After transferTokenPoolOwnershipToDaoMS() sets daoMS as the pending owner, only daoMS can call acceptOwnership(). Using setUpWithChainId(false) executes from the team/deployer multisig, which will revert with OwnableUnauthorizedAccount.
🐛 Proposed fix
- function acceptTokenPoolOwnership() external setUpWithChainId(false) {
+ function acceptTokenPoolOwnership() external setUpWithChainId(true) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function acceptTokenPoolOwnership() external setUpWithChainId(false) { | |
| function acceptTokenPoolOwnership() external setUpWithChainId(true) { |
🤖 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` at line 245,
acceptTokenPoolOwnership currently runs under setUpWithChainId(false) which
executes as the deployer/team multisig and therefore cannot call
Ownable2Step.acceptOwnership (only daoMS, the pending owner set by
transferTokenPoolOwnershipToDaoMS, can). Change the modifier on
acceptTokenPoolOwnership to run as the daoMS caller (e.g. use
setUpWithChainId(true) or another helper that impersonates daoMS) so the call to
acceptOwnership will be made by daoMS; ensure acceptTokenPoolOwnership invokes
Ownable2Step.acceptOwnership while msg.sender is daoMS and keep
transferTokenPoolOwnershipToDaoMS unchanged.
Comment on lines
+18
to
+31
| * Usage (direct value - may fail with very large numbers): | ||
| * forge script src/scripts/ops/CalculateCoolerLtvUpdate.s.sol:UpdateCoolerLtv \ | ||
| * --rpc-url sepolia \ | ||
| * --account <your-wallet> \ | ||
| * --broadcast \ | ||
| * --sig "updateOltv(uint96)" 872636398584498440592620626480000000000 | ||
| * | ||
| * Usage (via environment variable - recommended for large numbers): | ||
| * TARGET_OLTV=872636398584498440592620626480000000000 \ | ||
| * forge script src/scripts/ops/CalculateCoolerLtvUpdate.s.sol:UpdateCoolerLtv \ | ||
| * --rpc-url sepolia \ | ||
| * --account <your-wallet> \ | ||
| * --broadcast \ | ||
| * --sig "updateOltvFromEnv()" |
Contributor
There was a problem hiding this comment.
Example values exceed uint96 capacity.
The example TARGET_OLTV values (39 digits) vastly exceed uint96.max (~7.9e28, 29 digits). Per the interface, OLTV uses 18 decimals, so reasonable values like 1.05e18 ($1.05 LTV) fit within uint96. These examples appear incorrect and would cause silent truncation if used with updateOltvFromEnv().
Consider updating to realistic values, e.g., 1050000000000000000 for 105% LTV.
🤖 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/CalculateCoolerLtvUpdate.s.sol` around lines 18 - 31, The
README/example values for TARGET_OLTV are far too large for uint96 and must be
replaced with realistic 18-decimal OLTV values; update the usage examples in the
CalculateCoolerLtvUpdate.s.sol comment (references: contract/script name
UpdateCoolerLtv, function signatures updateOltv(uint96) and updateOltvFromEnv())
to use values that fit uint96 and represent 18-decimal LTVs (e.g. replace the
39‑digit examples with 1050000000000000000 for 105% LTV or similar valid uint96
values) and ensure the note recommends using the env variant for large-but-valid
values.
Comment on lines
+163
to
+166
| function updateOltvFromEnv() public { | ||
| uint96 targetOltv = uint96(vm.envUint("TARGET_OLTV")); | ||
| updateOltv(targetOltv); | ||
| } |
Contributor
There was a problem hiding this comment.
Unsafe cast from uint256 to uint96 silently truncates large values.
vm.envUint() returns uint256, and the direct cast to uint96 will silently truncate values exceeding uint96.max (~7.9e28). For an operational script, this could result in setting an unintended LTV.
🛡️ Proposed fix to add bounds validation
function updateOltvFromEnv() public {
- uint96 targetOltv = uint96(vm.envUint("TARGET_OLTV"));
+ uint256 rawValue = vm.envUint("TARGET_OLTV");
+ require(rawValue <= type(uint96).max, "TARGET_OLTV exceeds uint96 max");
+ uint96 targetOltv = uint96(rawValue);
updateOltv(targetOltv);
}🤖 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/CalculateCoolerLtvUpdate.s.sol` around lines 163 - 166, The
env value read in updateOltvFromEnv uses vm.envUint("TARGET_OLTV") which returns
a uint256 and is directly cast to uint96 causing silent truncation; fix by
keeping the value as uint256 first, validate that it is <= type(uint96).max (and
optionally >= 0 if needed), and only then cast to uint96 and call
updateOltv(targetOltv); if the check fails, revert or vm.stop with a clear error
mentioning TARGET_OLTV to avoid accidental truncation.
* 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
chore: remove .DS_Store file
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.
emergency-config.json#224Summary by CodeRabbit
New Features
Documentation
Chores
Tests