feat(world): change requireOwnerOrSelf to requireOwner #1457
Conversation
🦋 Changeset detectedLatest commit: 51b7b05 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. |
| @@ -19,12 +19,16 @@ import { NAMESPACE, MODULE_NAME, SYSTEM_NAME, TABLE_NAME } from "./constants.sol | |||
| contract UniqueEntityModule is IModule, WorldContextConsumer { | |||
There was a problem hiding this comment.
should this and others use oops, confused this with the other PRis Module instead of is IModule?
alvrs
force-pushed
the
alvrs/fix-access-control
branch
from
cccdb2f
to
6820bf8
Compare
| ResourceSelector.from(ROOT_NAMESPACE, CORE_SYSTEM_NAME), | ||
| coreSystem, | ||
| true | ||
| funcSelectorAndArgs: abi.encodeCall( |
There was a problem hiding this comment.
not blocking this PR but would it make more sense to call this calldata?
There was a problem hiding this comment.
we use funcSelectorAndArgs in a couple places, could consider changing that to callData (calldata wouldn't work because it's a reserved word). Let's track in an issue!
| @@ -58,68 +59,68 @@ contract KeysInTableHook is IStoreHook { | |||
|
|
|||
| function onBeforeDeleteRecord(bytes32 tableId, bytes32[] memory key, Schema) public { | |||
| bytes32 keysHash = keccak256(abi.encode(key)); | |||
| (bool has, uint40 index) = UsedKeysIndex.get(tableId, keysHash); | |||
| (bool has, uint40 index) = UsedKeysIndex.get(UsedKeysIndexTableId, tableId, keysHash); | |||
There was a problem hiding this comment.
does StoreSwitch not work here anymore?
There was a problem hiding this comment.
I assume it's cause they got namespaced?
There was a problem hiding this comment.
yeah exactly, it's not using the root namespace anymore but we don't support separate namespaces in the config yet, so need to do it manually here for now by creating a separate table id
There was a problem hiding this comment.
i think technically it doesn't have to be namespaced though but could continue using the root namespace since we only support installing it as root module for now anyway
There was a problem hiding this comment.
Reverted this change. Had originally started to refactor the module to support non-root installations but then realized it would be a bigger change because we need to register a store hook on an arbitrary table.
| ); | ||
| if (!success) revertWithBytes(returnData); |
There was a problem hiding this comment.
would it make sense to move this pattern into a lib for the commonly used methods for ergonomics?
WorldRoot.registerStoreHook(world, ...)
WorldRoot.registerSystem(world, ...)There was a problem hiding this comment.
agree it would be more ergonomic to use, but unfortunately more gas expensive because of all the parameters that need to be passed around in memory, and that library would be pretty painful to maintain (because it would have to include all the function signatures of all systems of IBaseWorld which includes all the functions the CoreModule registers)
|
|
||
| function getName() public pure returns (bytes16) { | ||
| return MODULE_NAME; | ||
| } | ||
|
|
||
| function installRoot(bytes memory args) public { |
There was a problem hiding this comment.
is it ok that installRoot won't delegatecall anything here?
There was a problem hiding this comment.
yeah this module doesn't require any root access, it's just registering the table and system in its own namespace
|
no major changes since last approvals, going ahead with mege |
No description provided.