refactor(world): separate call utils into WorldContextProvider and SystemCall
#1370
Conversation
🦋 Changeset detectedLatest commit: a2404a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 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 |
| * Reverts with the error if the call was not successful. | ||
| * Else returns any return data. | ||
| */ | ||
| function callWithHooksOrRevert( |
There was a problem hiding this comment.
this is the previous behavior of _call. In addition we now have library functions that return a success flag instead of reverting, which is useful if we want to execute more logic if a call failed (eg. will be needed in #1364)
| * If the system is not public, the caller must have access to the namespace or name. | ||
| * Returns the success status and any return data. | ||
| */ | ||
| function call( |
There was a problem hiding this comment.
Ohhh this is now a nice entry point for adding a system call event
packages/world/src/utils.sol
Outdated
| @@ -5,7 +5,7 @@ import { StoreSwitch } from "@latticexyz/store/src/StoreSwitch.sol"; | |||
| import { ResourceSelector } from "./ResourceSelector.sol"; | |||
| import { SystemRegistry } from "./Tables.sol"; | |||
|
|
|||
| library Utils { | |||
| library SystemUtils { | |||
There was a problem hiding this comment.
renamed this to make the purpose a bit clearer, and to add more utility functions to the same file. (Unfortunately it's not possible to turn the SystemUtils.systemNamespace() function into a free function because it uses address(this))
There was a problem hiding this comment.
Undid this change - felt unclean to have a mixed utils file, so i moved revertWithBytes to its own file. Still unhappy with this file being called just "Utils", but changing that should be its own PR.
alvrs
force-pushed
the
alvrs/call-utils
branch
from
31a033a
to
3c13457
Compare
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
alvrs
changed the base branch from
alvrs/empty-commit
to
alvrs/fix-ci-test-action
alvrs
force-pushed
the
alvrs/call-utils
branch
from
3c13457
to
79125bc
Compare
alvrs
force-pushed
the
alvrs/fix-ci-test-action
branch
from
9483d3e
to
8d8c89e
Compare
alvrs
force-pushed
the
alvrs/call-utils
branch
from
79125bc
to
70e8c7d
Compare
…stemCall.call/callWithHooks
alvrs
force-pushed
the
alvrs/call-utils
branch
from
70e8c7d
to
a2404a7
Compare
|
Merging since no change since last approval |
|
👏 this is great |
| * Calls a system via its resource selector and perform access control checks. | ||
| * Does not revert if the call fails, but returns a `success` flag along with the returndata. | ||
| */ | ||
| function call( |
There was a problem hiding this comment.
should this have a corresponding callOrRevert?
|
|
||
| /** | ||
| * Calls a system via its resource selector, perform access control checks and trigger hooks registered for the system. | ||
| * Does not revert if the call fails, but returns a `success` flag along with the returndata. |
There was a problem hiding this comment.
I assume hooks are only called on success?
Call.withSenderutil that managed appending themsg.senderto the calldata, and took a flag to usecallordelegatecall. Since appendingmsg.senderto the calldata is tightly coupled withWorldContext, the utils for calling/delegatecalling withmsg.senderappended to the calldata were moved toWorldContextProvider.callWithContext/delegatecallWithContext.WorldContextis renamed toWorldContextConsumer(to clarify the interaction betweenWorldContextProvider(appending context) andWorldContextConsumer(extracting context))Worldpreviously had an internal_callmethod which handled access control checks and converting aresourceSelectorinto the system's address. Since this logic is useful for other libraries (most notably the delegation checks coming in feat(world): add callFrom entry point #1364) it is moved to library functionsSystemCall.call/callWithHooksresourceSelectorinstead of the system address. This seems more useful and simplifies the code. If needed a hook can still load the system's address from storage viaSystems.get(resourceSelector)(which would be cheap since the storage slot is warm)