script: Add missing script for transferring ownership of CCIP token pool - #277
Conversation
📝 WalkthroughWalkthroughThe PR adds two batch action functions to ChangesOwnership Transfer Batch Actions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/scripts/ops/batches/CCIPTokenPool.sol (2)
224-232: 💤 Low valueConsider casting to
Ownable2Stepfor clarity.The code only uses
owner()andtransferOwnership()from theOwnable2Stepinterface. Casting toOwnable2Stepdirectly would be more accurate and would work correctly for bothLockReleaseTokenPool(canonical) andBurnMintTokenPool(non-canonical) since both inheritOwnable2Step.♻️ Suggested refactor
- if (LockReleaseTokenPool(tokenPool).owner() == daoMS) { + if (Ownable2Step(tokenPool).owner() == daoMS) { console2.log("Owner already transferred to", daoMS, ". Skipping."); return; } console2.log("Transferring ownership of", tokenPool, "to", daoMS); addToBatch( tokenPool, abi.encodeWithSelector(Ownable2Step.transferOwnership.selector, daoMS) );🤖 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 224 - 232, Replace the raw LockReleaseTokenPool calls with an explicit Ownable2Step cast: use Ownable2Step(tokenPool).owner() for the owner check and encode the call with abi.encodeWithSelector(Ownable2Step.transferOwnership.selector, daoMS) before calling addToBatch; update the if-check and the addToBatch invocation that reference tokenPool and LockReleaseTokenPool to use Ownable2Step(tokenPool) so both LockReleaseTokenPool and BurnMintTokenPool are handled clearly.
244-262: ⚡ Quick winConsider verifying
pendingOwner()before attempting to accept ownership.The function checks if
owner() == daoMSfor early exit but doesn't verify thatpendingOwner() == daoMSbefore adding the batch action. If ownership was not transferred to daoMS (or was transferred to someone else), the transaction will revert on-chain with a less informative error.Adding a
pendingOwner()check would provide clearer feedback when the script is run in an incorrect state.Also, line 255 log message "Accepting ownership of X to Y" is grammatically awkward—consider "for" or restructure.
♻️ Suggested improvement
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) { + if (Ownable2Step(tokenPool).owner() == daoMS) { console2.log("Owner already transferred to", daoMS, ". Skipping."); return; } - console2.log("Accepting ownership of", tokenPool, "to", daoMS); + // Check if the pending owner is the DAO MS + if (Ownable2Step(tokenPool).pendingOwner() != daoMS) { + console2.log("Pending owner is not", daoMS, ". Skipping."); + return; + } + + console2.log("Accepting ownership of", tokenPool, "for", daoMS); addToBatch(tokenPool, abi.encodeWithSelector(Ownable2Step.acceptOwnership.selector));Does Chainlink CCIP Ownable2Step have a pendingOwner() public getter function?🤖 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 244 - 262, In acceptTokenPoolOwnership(), check that the pending owner is daoMS before queuing the acceptOwnership call: retrieve pendingOwner() from LockReleaseTokenPool(tokenPool) and if it is not daoMS log a clear message and return instead of adding the batch; keep the existing owner() equality check first. Also adjust the console2.log message from "Accepting ownership of X to Y" to use "for" or rephrase to "Accepting ownership of <tokenPool> for <daoMS>" to be grammatically correct; ensure you still call addToBatch(tokenPool, abi.encodeWithSelector(Ownable2Step.acceptOwnership.selector)) and proposeBatch() only when pendingOwner() == daoMS.
🤖 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/batches/CCIPTokenPool.sol`:
- Around line 224-232: Replace the raw LockReleaseTokenPool calls with an
explicit Ownable2Step cast: use Ownable2Step(tokenPool).owner() for the owner
check and encode the call with
abi.encodeWithSelector(Ownable2Step.transferOwnership.selector, daoMS) before
calling addToBatch; update the if-check and the addToBatch invocation that
reference tokenPool and LockReleaseTokenPool to use Ownable2Step(tokenPool) so
both LockReleaseTokenPool and BurnMintTokenPool are handled clearly.
- Around line 244-262: In acceptTokenPoolOwnership(), check that the pending
owner is daoMS before queuing the acceptOwnership call: retrieve pendingOwner()
from LockReleaseTokenPool(tokenPool) and if it is not daoMS log a clear message
and return instead of adding the batch; keep the existing owner() equality check
first. Also adjust the console2.log message from "Accepting ownership of X to Y"
to use "for" or rephrase to "Accepting ownership of <tokenPool> for <daoMS>" to
be grammatically correct; ensure you still call addToBatch(tokenPool,
abi.encodeWithSelector(Ownable2Step.acceptOwnership.selector)) and
proposeBatch() only when pendingOwner() == daoMS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8e20d99-e453-415a-8f0c-97c3f721ae87
📒 Files selected for processing (1)
src/scripts/ops/batches/CCIPTokenPool.sol
Summary by CodeRabbit