feat(world,store): add ERC165 checks for all registration methods #1458
Conversation
🦋 Changeset detectedLatest commit: a709876 The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
alvrs
force-pushed
the
alvrs/fix-access-control
branch
from
cccdb2f to
6820bf8
Compare
alvrs
force-pushed
the
alvrs/erc-165
branch
from
f001e2e to
886dea4
Compare
| @@ -0,0 +1,21 @@ | |||
| [ | |||
There was a problem hiding this comment.
I added .gitattributes in another PR but might be worth adding here to land it sooner to suppress these codegen diffs for now:
https://github.com/latticexyz/mud/pull/1354/files#diff-618cd5b83d62060ba3d027e314a21ceaf75d36067ff820db126642944145393e
|
|
||
| // ERC-165 Interface ID (see https://eips.ethereum.org/EIPS/eip-165) | ||
| bytes4 constant SYSTEM_HOOK_INTERFACE_ID = ISystemHook.onBeforeCallSystem.selector ^ | ||
| ISystemHook.onAfterCallSystem.selector; |
There was a problem hiding this comment.
I noticed IModule above includes ERC165.supportsInterface.selector but this does not. Any reason?
There was a problem hiding this comment.
good catch, should be included
packages/store/src/IStoreHook.sol
Outdated
| IStoreHook.onBeforeSetField.selector ^ | ||
| IStoreHook.onAfterSetField.selector ^ | ||
| IStoreHook.onBeforeDeleteRecord.selector ^ | ||
| IStoreHook.onAfterDeleteRecord.selector; |
There was a problem hiding this comment.
same here, seems to be missing ERC165.supportsInterface.selector?
| IWorldContextConsumer._msgSender.selector ^ | ||
| IWorldContextConsumer._msgValue.selector ^ | ||
| IWorldContextConsumer._world.selector ^ | ||
| ERC165.supportsInterface.selector; |
There was a problem hiding this comment.
is it necessary to define the inherited selectors as well as our own?
and if so would it be easier to do the whole interface ID instead of each selector (caveat: I don't know what the bitshifting is doing here)?
bytes4 constant DELEGATION_CONTROL_INTERFACE_ID = IDelegationControl.verify.selector ^
WORLD_CONTEXT_CONSUMER_INTERFACE_ID ^
ERC165_INTERFACE_ID;There was a problem hiding this comment.
agree this is cleaner, and should work since ^ (bitwise XOR) is commutative and associative
packages/store/src/ERC165.sol
Outdated
| bytes4 constant ERC165_INTERFACE_ID = ERC165.supportsInterface.selector; | ||
|
|
||
| // See https://eips.ethereum.org/EIPS/eip-165 | ||
| interface ERC165 { |
There was a problem hiding this comment.
should it be IERC165 for consistency?
| function _msgSender() external view returns (address); | ||
| function _msgValue() external view returns (uint256); | ||
| function _world() external view returns (address); |
There was a problem hiding this comment.
need them to be public to be part of the interface to be able to distinguish a WorldContextConsumer contract
alvrs
dismissed stale reviews from dk1a and holic
The base branch was changed.
alvrs
force-pushed
the
alvrs/erc-165
branch
from
aa3284f to
a2f73b3
Compare
|
merging since there were no significant changes since the last approval, just implementation of review feedback and rebasing |
No description provided.