N.1 There is no good way to update a facet's isFreezable except call _removeFunctions then _addFunctions back
It is expensive to call _removeFunctions and _addFunctions, as it involves a lot of operations.
As the protocol becomes more mature, it is natural to freeze specific facets. A function can be created to specifically switch isFreezable of all selectors of a given facet.
VS Code
in Diamond library add:
function _changeFreezability(address _facet, bool _isFreezable) {
DiamondStorage storage ds = getDiamondStorage();
uint256 selectorLen = ds.facetToSelectors[_facet].selectors.length;
for (init i; i<selectLen; i=i.uncheckedInc()) {
bytes4 selector = ds.facetToSelectors[_facet].selectors[i];
ds.selectorToFacet[selector].isFreezable = _isFreezable;
}
}
It is confusing when comments are inconsistent with the code, even when the code itself is correct
In require(approvedBySecurityCouncil || upgradeNoticePeriodPassed, "a6"); // notice period should expire
The code says governor can execute a proposal immediately as long as enough approvals have been gathered. Yet the comment says notice period should expire.
Another example, where comments about _l1Token in L2ERC20Bridge was copied from L2EthBridge, thus inaccurate.
VS Code
fix the comment to match the code
It is confusing when docs are inconsistent with the code, even when the code itself is correct
According to the doc "Several (of none) logs from the L2_TO_L1_MESSENGER with the key == hashedMessage where hashedMessage is a hash of an arbitrary-length message that is sent from L2", however, in the code:
(bytes32 hashedMessage, ) = UnsafeBytes.readBytes32(emittedL2Logs, i + 56);. For L2_TO_L1_MESSENGER the key is bytes32(msg.sender) at 24 and value is hashedMessage at 56. So, the code is correct, but the document should be edited.
VS Code
replace key with value.
_processL2Logs does extensive checking of l2logs to make sure everything lines up.
As a sanity check, the new requirement makes it more explicit about the structure of l2logs.
VS Code
add require(emittedL2Logs.length % L2_TO_L1_LOG_SERIALIZE_SIZE == 0); before Executor.sol#L111
The doc specifies _processL2Logs processes Several (or none) logs from other addresses with arbitrary parameters.. However the code does not show any trace of such logic.
After communicating with the dev team, it is apparent this paragraph is meant for the next phase of the project, thus should be removed.
_calculateBlockHash is an internal function in Executor, and it is not called in the codebase. Can be removed.
A new facet needs to be reployed to add members to securityCouncilMembers
As the code stands now, there does not seem to be a setter to set an account as securityCouncilMembers. The emergency approval route is not really needed either since UPGRADE_NOTICE_PERIOD is currently set as 0. However, as these logics are mobilized, a setter will need to be reployed.
isFacetFreezable returns false for non-exising _facet.
isFunctionFreezable reverts for non-existing _selector.
The the sake of consistency, they should exhibit similar behavior.
All other functions in Governance.sol checkes the equivalence of old and new values before changing the exisitng value. setVerifierParams is the only exception. It makes sense to check new check first as well in setVerifierParams.