-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(world): add CallBatchSystem to core module #1500
Conversation
🦋 Changeset detectedLatest commit: 4ce27d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 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 |
IBaseWorld world = IBaseWorld(_world()); | ||
|
||
for (uint256 i; i < resourceSelectors.length; i++) { | ||
world.callFrom(_msgSender(), resourceSelectors[i], funcSelectorAndArgss[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make this a root system and then do this so the users don't need to create a delegation?
(bool success, bytes memory returnData) = address(world).delegatecall(abi.encodeCall(
world.call,
(
_msgSender(),
resourceSelector[i],
funcSelectorAndArgs[i]
)
));
if(!success) revertWithBytes(returnData);
We can actually do both, have a BatchCallSystem
and a BatchCallRootSystem
, and install then in installModule
and installRootModule
respectively
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this to be a module? if we intend to use this for deploys anyway, would it make sense to just have this under core/implementations
as a plain root system? I also can't imagine you'd wanna install this more than once anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it can also be part of the core module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach without the delegation. I'm going to add the root version to the core module.
I'll also remove the non-root version, because the root version will be installed on worlds by default.
import { IBaseWorld } from "../../interfaces/IBaseWorld.sol"; | ||
|
||
contract BatchCallSystem is System { | ||
function batchCall(bytes32[] memory resourceSelectors, bytes[] memory funcSelectorAndArgss) public { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be worse on calldata length if this was something like
struct SystemCall {
bytes32 systemId;
bytes callData;
}
function batchCall(SystemCall[] memory systemCalls) public {
(this feels cleaner but I would prob optimize for calldata/gas here, since these calls will be abstracted away anyway)
*/ | ||
function callBatch( | ||
bytes32[] memory resourceSelectors, | ||
bytes[] memory callDatas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question as before: would it be better or worse gas-wise to have an array of SystemCall
structs instead of two arrays? I assume a little worse on calldata but we wouldn't have to do a length check on the arrays to make sure they match (though I guess the whole thing would revert with an out of bounds error in that case?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my feeling is it would be a little better calldata wise because the structs are encoded like tuples, so there shouldn't be overhead on the encoding of the individual elements, but we might save one word because we only have one array (and therefore only one length encoding)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to implement the struct for the calldata improvements and to improve usability (only dealing with a single array)
|
||
assertEq(abi.decode(datas[0], (address)), caller, "wrong address returned"); | ||
assertEq(abi.decode(datas[1], (address)), address(world), "wrong store returned"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add some tests to make sure this goes through access control checks properly and fails on system calls we don't have access to?
packages/world/src/modules/core/implementations/CallBatchSystem.sol
Outdated
Show resolved
Hide resolved
Co-authored-by: alvarius <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure where to put this file specifically for the struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realizing now we have two things named SystemCall
@alvrs should we keep this one named that way and the other lib named as SystemCaller or SystemCallLib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd propose calling this one SystemCallData
(aligned with how the tablegen structs are called, and also alllgins with "callData")
closes #1160
closes #1470
The
BatchCallSystem
enables calling many systems in a World in a single transaction.It leverages
callFrom
to execute each system call with the user asmsgSender()
. Users delegate to theBatchCallSystem
system, then callbatchCall
with the resource selectors and calldata for the systems they wish to call.