L2 Governance #1991
L2 Governance #1991
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1991 +/- ##
==========================================
- Coverage 61.86% 61.55% -0.31%
==========================================
Files 58 59 +1
Lines 2916 3015 +99
Branches 758 777 +19
==========================================
+ Hits 1804 1856 +52
- Misses 1109 1155 +46
- Partials 3 4 +1 ☔ View full report in Codecov by Sentry. |
| revert InvalidProposal(); | ||
| } | ||
|
|
||
| controller.executeBatch( |
There was a problem hiding this comment.
Proposals have a value field. If the field is meant to be used, some ETH needs to be transferred to the controller. The TimelockController has the default receive() external payable {}; ETH could be transferred to the TimelockController before execution, or ETH needs to be transferred on this call. I want to ensure that not sending ETH here is the intended behavior.
There was a problem hiding this comment.
This is an oversight on my part, We so far never had any proposal that needed a value to be set. But it's good to have it working. Will update the code
There was a problem hiding this comment.
Have made the execute method payable and to forward the msg.value to the TimelockController
| * @param proposalId L2 Proposal ID | ||
| * @param maxGasLimit Max Gas Limit to use | ||
| */ | ||
| function _sendCommandToL2( |
There was a problem hiding this comment.
Is this necessary since you also have queueL2Proposal and cancelL2Proposal and for now L2Governance only accepts these two message types?
There was a problem hiding this comment.
I assume you mean the sendCommandToL2 and not this internal function? I can get rid of that, yeah
There was a problem hiding this comment.
Yes, commented at the wrong place.
There was a problem hiding this comment.
Have removed that sendCommandToL2. Also, removed the InvalidGovernanceCommand check and error since those are not user-input now
| * Can be called by anyone | ||
| * @param proposalId Proposal ID | ||
| */ | ||
| function execute(uint256 proposalId) external { |
There was a problem hiding this comment.
It might be advisable to verify the status of the Chainlink bridge it might be cursed and not processing messages see here.
This step may not be essential and can only be an issue in a rare case where a proposal already queued must be canceled and is about to be executable.
There was a problem hiding this comment.
Will do. However, even if a CANCEL_PROPOSAL_COMMAND is sent by a compromised bridge, it'll not be immediately executable, since it has to go through the timelock
There was a problem hiding this comment.
The operation is immediately canceled on a cancel call.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/governance/TimelockController.sol#L334-L344
There was a problem hiding this comment.
Ah, you're right. My bad, I got confused
There was a problem hiding this comment.
This is done. execute and _ccipRecieve both now check if the Chainlink's ARM contract has cursed set
| IRouterClient router = IRouterClient(ccipRouter); | ||
|
|
||
| // Compute fees | ||
| uint256 fees = router.getFee(chainSelector, message); |
There was a problem hiding this comment.
You might want to add a view function that returns the result of getFee to query before sending a command to make sure you have enough ETH in the contract for execution.
There was a problem hiding this comment.
Have added a getCCIPFees method that takes the same params as _sendCommandToL2 but returns the fee amount
shahthepro
requested review from
sparrowDom,
DanielVF and
naddison36
as code owners
If you made a contract change, make sure to complete the checklist below before merging it in master.
Refer to our documentation for more details about contract security best practices.
Contract change checklist: