-
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
fix(world-modules): SystemSwitch
properly calls systems from root
#2205
Conversation
🦋 Changeset detectedLatest commit: 1bc3139 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 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 |
@@ -39,19 +39,27 @@ library SystemSwitch { | |||
function call(ResourceId systemId, bytes memory callData) internal returns (bytes memory returnData) { | |||
address worldAddress = WorldContextConsumerLib._world(); | |||
|
|||
// If we're in the World context, call the system directly via delegatecall | |||
// If we're in the World context | |||
if (address(this) == worldAddress) { | |||
(address systemAddress, ) = Systems.get(systemId); |
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.
looking at this I wonder if we should just use the SystemCall.call
library here
function call(ResourceId systemId, bytes memory callData) internal returns (bytes memory returnData) {
address worldAddress = WorldContextConsumerLib._world();
// If we're in the World context, call via the internal library
if (address(this) == worldAddress) {
return SystemCall.call(systemId, callData);
}
// Otherwise, call the system via world.call
returnData = IWorldKernel(worldAddress).call(systemId, callData);
}
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! It adds a small amount of gas for the publicAccess
and value
checks but it's great to reuse the code.
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.
(not blocking this PR) should we change all the other places to use the same lib?
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.
Are there other places that do this? Searching for delegatecallWithContext
or == ROOT_NAMESPACE
doesn't bring up much for me
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 wonder if this line could cause an error because it updates the balances expecting a root
if (value > 0) {
SystemSwitch
should properly call systems
SystemSwitch
should properly call systemsSystemSwitch
properly calls systems from root
We should merge #2216 into this |
Fixes #2089
Added a test case that fails without this code change