-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
L2 Governance #1991
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, commented at the wrong place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. My bad, I got confused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added a getCCIPFees
method that takes the same params as _sendCommandToL2
but returns the fee amount
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: