DispatchProxy: implement a non-enumerable diamond proxy #217
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a dispatch-based proxy system: a Dispatch library with per-selector routing and ownership storage; a DispatchProxy using it; facet modules for updating the dispatch table, ownership, diamond cut/loupe interfaces and facets; a mock dispatch module; expanded mocks import; and comprehensive tests validating dispatch, ownership, fallback/receive, and update flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Proxy as DispatchProxy
participant VMT as Dispatch.VMT
participant Module as Target Facet (delegatecall)
Caller->>Proxy: call fn(args)/send ETH
Proxy->>VMT: lookup msg.sig
alt exact match
VMT-->>Proxy: impl address
else no match
VMT-->>Proxy: lookup _FALLBACK_SIG
end
alt impl found
Proxy->>Module: delegatecall(args/value)
Module-->>Proxy: return/revert
Proxy-->>Caller: bubble result
else none found
Proxy-->>Caller: revert DispatchProxyMissingImplementation(msg.sig)
end
sequenceDiagram
autonumber
actor Owner
participant Proxy as DispatchProxy
participant Update as DispatchUpdateModule (facet)
participant VMT as Dispatch.VMT
Owner->>Proxy: updateDispatchTable(modules)
Proxy->>Update: delegatecall updateDispatchTable
Update->>VMT: enforceOwner(Owner)
loop for each module and selector
Update->>VMT: setFunction(selector, impl)
Note right of VMT: emit VMTUpdate
end
Update-->>Proxy: done
Proxy-->>Owner: success
sequenceDiagram
autonumber
actor Owner
participant Proxy as DispatchProxy
participant Cut as DiamondCutFacet
participant VMT as Dispatch.VMT
participant Init as _init (optional)
Owner->>Proxy: diamondCut(cuts, _init, _calldata)
Proxy->>Cut: delegatecall diamondCut
Cut->>VMT: enforceOwner(Owner)
loop each FacetCut
alt Add/Replace/Remove
Cut->>VMT: setFunction/remove per selector
end
end
Cut-->>Proxy: emit DiamondCut
alt _init != address(0)
Proxy->>Init: delegatecall _calldata
end
Proxy-->>Owner: success/revert bubbled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
contracts/proxy/dispatch/modules/DiamondCutFacet.sol (1)
1-16: Consider improving error naming.The error names have some issues:
- Line 14:
DiamondCutFacetAlreadyExistshould beDiamondCutFacetAlreadyExists(grammatically correct)- Line 16:
DiamondCutFacetAlreadyDoesNotExithas a typo - should be "Exist" not "Exit"- Line 15:
DiamondCutFacetAlreadySetis unclear for the Replace action validation failureConsider renaming for clarity:
- error DiamondCutFacetAlreadyExist(bytes4 selector); - error DiamondCutFacetAlreadySet(bytes4 selector); - error DiamondCutFacetAlreadyDoesNotExit(bytes4 selector); + error DiamondCutFacetAlreadyExists(bytes4 selector); + error DiamondCutFacetNotChanged(bytes4 selector); + error DiamondCutFacetDoesNotExist(bytes4 selector);contracts/proxy/dispatch/modules/DiamondLoupeFacet.sol (4)
3-3: Consider using a locked pragma version.Floating pragmas can lead to inconsistent behavior across deployments. For production contracts, it's recommended to lock the Solidity version.
-pragma solidity ^0.8.0; +pragma solidity 0.8.28;
14-14: Remove unnecessary statement.The standalone
this;statement is used to suppress the "unused variable" warning for the implicitthisin aviewfunction, but this is non-idiomatic. Since the function unconditionally reverts, this statement serves no purpose and can be removed.function facets() public view override returns (Facet[] memory) { - this; revert("This implementation doesn't keep an index, use an offchain index instead"); }
13-27: Consider using custom errors for gas efficiency.The three stub functions use string revert messages. Custom errors are more gas-efficient and provide better error handling in production contracts.
Example implementation:
error IndexingNotSupported(); function facets() public view override returns (Facet[] memory) { revert IndexingNotSupported(); } function facetFunctionSelectors(address) public view override returns (bytes4[] memory) { revert IndexingNotSupported(); } function facetAddresses() public view override returns (address[] memory) { revert IndexingNotSupported(); }
5-10: Remove Context inheritance from DiamondLoupeFacet.sol. The contract doesn’t call_msgSender()or_msgData(), so you can drop theimport {Context}andContextbase to reduce unnecessary bytecode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
contracts/mocks/DispatchModuleMock.sol(1 hunks)contracts/mocks/import.sol(1 hunks)contracts/proxy/dispatch/DispatchProxy.sol(1 hunks)contracts/proxy/dispatch/interfaces/IDiamondCut.sol(1 hunks)contracts/proxy/dispatch/interfaces/IDiamondLoupe.sol(1 hunks)contracts/proxy/dispatch/modules/DiamondCutFacet.sol(1 hunks)contracts/proxy/dispatch/modules/DiamondLoupeFacet.sol(1 hunks)contracts/proxy/dispatch/modules/DispatchOwnershipModule.sol(1 hunks)contracts/proxy/dispatch/modules/DispatchUpdateModule.sol(1 hunks)contracts/proxy/dispatch/utils/Dispatch.sol(1 hunks)test/proxy/DispatchProxy.test.js(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codespell
contracts/proxy/dispatch/modules/DiamondLoupeFacet.sol
[failure] 26-26:
doesnt ==> doesn't, does not
[failure] 21-21:
doesnt ==> doesn't, does not
[failure] 15-15:
doesnt ==> doesn't, does not
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: slither
- GitHub Check: coverage
🔇 Additional comments (13)
contracts/proxy/dispatch/interfaces/IDiamondCut.sol (4)
1-3: LGTM!Standard license and pragma declarations are appropriate.
5-10: LGTM!The interface and enum correctly implement the EIP-2535 Diamond standard for facet management actions.
12-18: LGTM!The struct and event correctly implement the EIP-2535 Diamond standard specifications.
20-27: LGTM!Function signature and documentation correctly implement the EIP-2535 Diamond Cut standard.
contracts/proxy/dispatch/modules/DispatchOwnershipModule.sol (4)
1-11: LGTM!Imports and contract declaration are appropriate. The
@custom:statelessmarker correctly indicates that state is managed via the Dispatch VMT.
16-19: LGTM!The modifier correctly delegates ownership enforcement to the Dispatch VMT system using the caller's address from Context.
24-26: LGTM!The function correctly reads ownership from the Dispatch VMT.
35-37: LGTM!The function correctly implements ownership renunciation by setting the owner to the zero address, with appropriate access control.
contracts/proxy/dispatch/interfaces/IDiamondLoupe.sol (3)
1-9: LGTM!Interface declaration and Facet struct correctly implement the EIP-2535 Diamond Loupe standard.
11-18: LGTM!Functions correctly define the Diamond Loupe standard for querying facet information.
20-29: LGTM!Functions correctly implement the Diamond Loupe standard with appropriate documentation about the zero address return for missing facets.
contracts/proxy/dispatch/modules/DiamondCutFacet.sol (1)
18-21: LGTM!Function setup correctly retrieves the Dispatch VMT and enforces ownership before any modifications.
contracts/proxy/dispatch/modules/DiamondLoupeFacet.sol (1)
29-31: Approve facetAddress implementation.Dispatch.getFunctionreturnsaddress(0)for unregistered selectors per Solidity mapping defaults and EIP-2535 semantics; no additional error handling required.
| for (uint256 i = 0; i < _diamondCut.length; ++i) { | ||
| FacetCut memory facetcut = _diamondCut[i]; | ||
| for (uint256 j = 0; j < facetcut.functionSelectors.length; ++j) { | ||
| bytes4 selector = facetcut.functionSelectors[j]; | ||
| address currentFacet = store.getFunction(selector); | ||
| if (facetcut.action == FacetCutAction.Add && currentFacet != address(0)) { | ||
| revert DiamondCutFacetAlreadyExist(selector); | ||
| } else if (facetcut.action == FacetCutAction.Replace && currentFacet != facetcut.facetAddress) { | ||
| revert DiamondCutFacetAlreadySet(selector); | ||
| } else if (facetcut.action == FacetCutAction.Remove && currentFacet == address(0)) { | ||
| revert DiamondCutFacetAlreadyDoesNotExit(selector); | ||
| } | ||
| store.setFunction(selector, facetcut.facetAddress); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix critical validation and update logic errors.
The implementation has multiple critical issues:
-
Replace validation is incomplete (lines 29-30): It only checks if the new facet differs from the current one but doesn't verify that a facet exists at all. This allows "replacing" non-existent selectors, which should fail or be treated as Add.
-
Remove action uses wrong address (line 34): For
FacetCutAction.Remove, the function should set the selector toaddress(0), notfacetcut.facetAddress. The current code would set it to whatever address is in the struct, which is incorrect.
Apply this diff to fix both issues:
bytes4 selector = facetcut.functionSelectors[j];
address currentFacet = store.getFunction(selector);
if (facetcut.action == FacetCutAction.Add && currentFacet != address(0)) {
revert DiamondCutFacetAlreadyExist(selector);
- } else if (facetcut.action == FacetCutAction.Replace && currentFacet != facetcut.facetAddress) {
+ } else if (facetcut.action == FacetCutAction.Replace && (currentFacet == address(0) || currentFacet == facetcut.facetAddress)) {
revert DiamondCutFacetAlreadySet(selector);
} else if (facetcut.action == FacetCutAction.Remove && currentFacet == address(0)) {
revert DiamondCutFacetAlreadyDoesNotExit(selector);
}
- store.setFunction(selector, facetcut.facetAddress);
+ store.setFunction(
+ selector,
+ facetcut.action == FacetCutAction.Remove ? address(0) : facetcut.facetAddress
+ );📝 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.
| for (uint256 i = 0; i < _diamondCut.length; ++i) { | |
| FacetCut memory facetcut = _diamondCut[i]; | |
| for (uint256 j = 0; j < facetcut.functionSelectors.length; ++j) { | |
| bytes4 selector = facetcut.functionSelectors[j]; | |
| address currentFacet = store.getFunction(selector); | |
| if (facetcut.action == FacetCutAction.Add && currentFacet != address(0)) { | |
| revert DiamondCutFacetAlreadyExist(selector); | |
| } else if (facetcut.action == FacetCutAction.Replace && currentFacet != facetcut.facetAddress) { | |
| revert DiamondCutFacetAlreadySet(selector); | |
| } else if (facetcut.action == FacetCutAction.Remove && currentFacet == address(0)) { | |
| revert DiamondCutFacetAlreadyDoesNotExit(selector); | |
| } | |
| store.setFunction(selector, facetcut.facetAddress); | |
| } | |
| } | |
| for (uint256 i = 0; i < _diamondCut.length; ++i) { | |
| FacetCut memory facetcut = _diamondCut[i]; | |
| for (uint256 j = 0; j < facetcut.functionSelectors.length; ++j) { | |
| bytes4 selector = facetcut.functionSelectors[j]; | |
| address currentFacet = store.getFunction(selector); | |
| if (facetcut.action == FacetCutAction.Add && currentFacet != address(0)) { | |
| revert DiamondCutFacetAlreadyExist(selector); | |
| } else if ( | |
| facetcut.action == FacetCutAction.Replace && | |
| (currentFacet == address(0) || currentFacet == facetcut.facetAddress) | |
| ) { | |
| revert DiamondCutFacetAlreadySet(selector); | |
| } else if (facetcut.action == FacetCutAction.Remove && currentFacet == address(0)) { | |
| revert DiamondCutFacetAlreadyDoesNotExit(selector); | |
| } | |
| store.setFunction( | |
| selector, | |
| facetcut.action == FacetCutAction.Remove | |
| ? address(0) | |
| : facetcut.facetAddress | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/proxy/dispatch/modules/DiamondCutFacet.sol around lines 22 to 36,
the Replace validation only checks equality against the new facet but doesn't
ensure a current facet exists, and the Remove branch incorrectly writes
facetcut.facetAddress instead of clearing the selector. Change the Replace logic
to first require currentFacet != address(0) (revert if missing) and also revert
if currentFacet == facetcut.facetAddress (already set), then call
store.setFunction with the new facet as before; for Remove, keep the existence
check but call store.setFunction(selector, address(0)) to clear the selector
instead of writing facetcut.facetAddress.
| function facets() public view override returns (Facet[] memory) { | ||
| this; | ||
| revert("This implementation doesnt keep an index, use an offchain index instead"); | ||
| } |
There was a problem hiding this comment.
Fix spelling error in revert message.
The revert message contains "doesnt" which should be "doesn't" or "does not".
- revert("This implementation doesnt keep an index, use an offchain index instead");
+ revert("This implementation doesn't keep an index, use an offchain index instead");Based on static analysis hints.
📝 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.
| function facets() public view override returns (Facet[] memory) { | |
| this; | |
| revert("This implementation doesnt keep an index, use an offchain index instead"); | |
| } | |
| function facets() public view override returns (Facet[] memory) { | |
| this; | |
| revert("This implementation doesn't keep an index, use an offchain index instead"); | |
| } |
🧰 Tools
🪛 GitHub Check: codespell
[failure] 15-15:
doesnt ==> doesn't, does not
🤖 Prompt for AI Agents
In contracts/proxy/dispatch/modules/DiamondLoupeFacet.sol around lines 13 to 16,
the revert message uses the misspelled contraction "doesnt"; update the revert
string to use proper spelling such as "doesn't" or "does not" (e.g., change
"This implementation doesnt keep an index, use an offchain index instead" to
"This implementation doesn't keep an index, use an offchain index instead" or
the equivalent with "does not").
| function facetFunctionSelectors(address _facet) public view override returns (bytes4[] memory) { | ||
| this; | ||
| _facet; | ||
| revert("This implementation doesnt keep an index, use an offchain index instead"); | ||
| } |
There was a problem hiding this comment.
Fix spelling error and remove unnecessary statements.
The revert message contains "doesnt" which should be "doesn't" or "does not". Additionally, the standalone this; and _facet; statements can be removed.
function facetFunctionSelectors(address _facet) public view override returns (bytes4[] memory) {
- this;
- _facet;
- revert("This implementation doesnt keep an index, use an offchain index instead");
+ revert("This implementation doesn't keep an index, use an offchain index instead");
}Based on static analysis hints.
📝 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.
| function facetFunctionSelectors(address _facet) public view override returns (bytes4[] memory) { | |
| this; | |
| _facet; | |
| revert("This implementation doesnt keep an index, use an offchain index instead"); | |
| } | |
| function facetFunctionSelectors(address _facet) public view override returns (bytes4[] memory) { | |
| revert("This implementation doesn't keep an index, use an offchain index instead"); | |
| } |
🧰 Tools
🪛 GitHub Check: codespell
[failure] 21-21:
doesnt ==> doesn't, does not
🤖 Prompt for AI Agents
In contracts/proxy/dispatch/modules/DiamondLoupeFacet.sol around lines 18 to 22,
remove the unnecessary standalone statements `this;` and `_facet;` and update
the revert message to correct spelling/grammar (e.g., change "This
implementation doesnt keep an index, use an offchain index instead" to "This
implementation does not keep an index; use an off-chain index instead" or "This
implementation doesn't keep an index; use an off-chain index instead").
| function facetAddresses() public view override returns (address[] memory) { | ||
| this; | ||
| revert("This implementation doesnt keep an index, use an offchain index instead"); | ||
| } |
There was a problem hiding this comment.
Fix spelling error and remove unnecessary statement.
The revert message contains "doesnt" which should be "doesn't" or "does not". Additionally, the standalone this; statement can be removed.
function facetAddresses() public view override returns (address[] memory) {
- this;
- revert("This implementation doesnt keep an index, use an offchain index instead");
+ revert("This implementation doesn't keep an index, use an offchain index instead");
}Based on static analysis hints.
📝 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.
| function facetAddresses() public view override returns (address[] memory) { | |
| this; | |
| revert("This implementation doesnt keep an index, use an offchain index instead"); | |
| } | |
| function facetAddresses() public view override returns (address[] memory) { | |
| revert("This implementation doesn't keep an index, use an offchain index instead"); | |
| } |
🧰 Tools
🪛 GitHub Check: codespell
[failure] 26-26:
doesnt ==> doesn't, does not
🤖 Prompt for AI Agents
In contracts/proxy/dispatch/modules/DiamondLoupeFacet.sol around lines 24 to 27,
remove the unnecessary standalone "this;" statement and fix the revert string
spelling and formatting: replace "doesnt" with "does not" (or "doesn't") and
change "offchain" to "off-chain" so the revert reads e.g. "This implementation
does not keep an index; use an off-chain index instead."
| error DiamondCutFacetAlreadyExist(bytes4 selector); | ||
| error DiamondCutFacetAlreadySet(bytes4 selector); |
There was a problem hiding this comment.
| error DiamondCutFacetAlreadyExist(bytes4 selector); | |
| error DiamondCutFacetAlreadySet(bytes4 selector); | |
| error DiamondCutFacetAlreadyExist(bytes4 selector, address facet); | |
| error DiamondCutFacetAlreadySet(bytes4 selector, address facet); |
| /** | ||
| * @dev Updates the vtable | ||
| */ | ||
| function updateDispatchTable(ModuleDefinition[] calldata modules) public { |
There was a problem hiding this comment.
| function updateDispatchTable(ModuleDefinition[] calldata modules) public { | |
| function addModules(ModuleDefinition[] calldata modules) public { |
| // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Dispatch.VMT")) - 1)) & ~bytes32(uint256(0xff)) | ||
| bytes32 private constant _DISPATCH_VMT_SLOT = 0xe6b1591f932b472559c00c679d5b3da28bf0ed2fd643b2ef77392cbec1743c00; | ||
|
|
||
| struct VMT { |
There was a problem hiding this comment.
| struct VMT { | |
| struct DispatchStorage { |
There was a problem hiding this comment.
For the record, VMT stands for virtual method table
But given that this structure also contains the owner, maybe the name is not great.
|
|
||
| struct VMT { | ||
| address _owner; | ||
| mapping(bytes4 => address) _vtable; |
There was a problem hiding this comment.
| mapping(bytes4 => address) _vtable; | |
| mapping(bytes4 selector => address) _modules; |
| /** | ||
| * @dev Get singleton instance | ||
| */ | ||
| function instance() internal pure returns (VMT storage store) { |
There was a problem hiding this comment.
| function instance() internal pure returns (VMT storage store) { | |
| function getStorage() internal pure returns (DispatchStorage storage store) { |
| /** | ||
| * @dev Delegation management | ||
| */ | ||
| event VMTUpdate(bytes4 indexed selector, address oldImplementation, address newImplementation); |
There was a problem hiding this comment.
| event VMTUpdate(bytes4 indexed selector, address oldImplementation, address newImplementation); | |
| event FunctionModuleUpdated(bytes4 indexed selector, address oldImplementation, address newImplementation); |
| */ | ||
| event VMTUpdate(bytes4 indexed selector, address oldImplementation, address newImplementation); | ||
|
|
||
| function getFunction(VMT storage store, bytes4 selector) internal view returns (address) { |
There was a problem hiding this comment.
| function getFunction(VMT storage store, bytes4 selector) internal view returns (address) { | |
| function getFunctionModule(DispatchStorage storage store, bytes4 selector) internal view returns (address) { |
| return store._vtable[selector]; | ||
| } | ||
|
|
||
| function setFunction(VMT storage store, bytes4 selector, address module) internal { |
There was a problem hiding this comment.
| function setFunction(VMT storage store, bytes4 selector, address module) internal { | |
| function setFunctionModule(DispatchStorage storage store, bytes4 selector, address module) internal { |
Summary by CodeRabbit