fix(world): limit call context of CoreSystem to delegatecall [C-02]
#2111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 34c019c 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 |
alvrs
changed the title
CoreSystem to delegatecallCoreSystem to delegatecall [C-02]
alvrs
force-pushed
the
alvrs/limit-core-system-call-context
branch
from
8faf955 to
123f241
Compare
holic
reviewed
Jan 11, 2024
| Depending on whether it is a root or non-root system, the call is performed via `delegatecall` or `call`. | ||
| Since Systems are expected to be stateless and only interact with the World state, it is not necessary to prevent direct calls to the systems. | ||
| However, since the `CoreSystem` is known to always be registered as a root system in the World, it is always expected to be delegatecalled, | ||
| so we made this expectation explicit by reverting if it is not delegatecalled. |
There was a problem hiding this comment.
your notes here are great! can we add these notes to LimitedCallContext as well, so folks know the why behind using this contract/set of modifiers?
alvrs
force-pushed
the
alvrs/limit-core-system-call-context
branch
from
8b6961d to
7695ebb
Compare
|
this one brought the core module over the size limit as well, blocked on #2046 |
alvrs
force-pushed
the
alvrs/limit-core-system-call-context
branch
from
52808d4 to
a66e583
Compare
holic
previously approved these changes
Jan 16, 2024
alvrs
force-pushed
the
alvrs/limit-core-system-call-context
branch
from
a66e583 to
6c0610b
Compare
alvrs
force-pushed
the
alvrs/limit-core-system-call-context
branch
from
6c0610b to
34c019c
Compare
Agree with this and started implementing it but it actually requires a lot of boilerplate for testing that each method reverts as expected, and I didn't find a way to test that no methods can be called directly, so i would leave this for a followup |
holic
approved these changes
Jan 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.