fix(store): add missing FieldLayout and Schema validations [L-07] #2046
Conversation
🦋 Changeset detectedLatest commit: d01ad1f 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 |
dk1a
changed the title
| IWorldErrors, | ||
| AccessManagementSystem, | ||
| BalanceTransferSystem, | ||
| BatchCallSystem, | ||
| ModuleInstallationSystem, | ||
| StoreRegistrationSystem, | ||
| WorldRegistrationSystem |
There was a problem hiding this comment.
I kinda wonder if we should just go all the way and register all of these individually (and get rid of this file) rather than grouping some of them here?
| target: coreSystem, | ||
| callData: abi.encodeCall(WorldRegistrationSystem.registerSystem, (CORE_SYSTEM_ID, CoreSystem(coreSystem), true)) | ||
| target: coreRegistrationSystem, | ||
| callData: abi.encodeCall(WorldRegistrationSystem.registerSystem, (systemId, WorldContextConsumer(target), true)) |
There was a problem hiding this comment.
re: above, this is the bit that confused me and makes me wonder if we should just have all the systems registered individually rather than grouping some under CoreRegistrationSystem
it was a little unclear that CoreRegistrationSystem is a WorldRegistrationSystem and we're doing a delegatecall using the WorldRegistrationSystem interface rather than CoreRegistrationSystem interface
| // --- AccessManagementSystem --- | ||
| "grantAccess(bytes32,address)", | ||
| "revokeAccess(bytes32,address)", | ||
| "transferOwnership(bytes32,address)", | ||
| "transferOwnership(bytes32,address)" |
There was a problem hiding this comment.
just thinking aloud but I wonder if it'd be worth our codegen creating a solidity file for system function signatures, so we can import from that instead of maintaining a list here
.changeset/silly-chefs-confess.md
Outdated
| --- | ||
|
|
||
| - Added `CoreSystem2` and moved `BalanceTransferSystem` from `CoreSystem` to `CoreSystem2` | ||
| - Added `CoreModule2`, which registers `CoreSystem2` |
There was a problem hiding this comment.
I think this needs to be updated?
|
core module split was moved to #2128 |
packages/store/src/Schema.sol
Outdated
There was a problem hiding this comment.
There was a suggestion to simplify and split this loop into two in N-8:
7a11ba4#diff-565f54743c2cafcf49b62c6c994b044ce220955a9faa2167d9c34fbe13f02ac1
Just a heads up in case that gets merged first
There was a problem hiding this comment.
Since this is an L-rated fix, gonna suggest we merge this first and then rebase/figure out how to apply the N-08 fix after.
packages/store/src/FieldLayout.sol
Outdated
| for (uint256 i; i < _numStaticFields; ) { | ||
| uint256 staticByteLength = fieldLayout.atIndex(i); | ||
| if (staticByteLength == 0) { | ||
| revert FieldLayoutLib.FieldLayoutLib_StaticLengthIsZero(); | ||
| revert FieldLayoutLib.FieldLayoutLib_StaticLengthIsZero(i); | ||
| } else if (staticByteLength > WORD_SIZE) { | ||
| revert FieldLayoutLib.FieldLayoutLib_StaticLengthDoesNotFitInAWord(); |
There was a problem hiding this comment.
should we add the index param to this error as well for consistency?
No description provided.