-
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
refactor(cli,world,world-modules): split and separately deploy core systems #2128
Conversation
🦋 Changeset detectedLatest commit: 5849b21 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 |
"@latticexyz/world": major | ||
--- | ||
|
||
- Split `CoreSystem` into `AccessManagementSystem`, `BalanceTransferSystem`, `BatchCallSystem`, `CoreRegistrationSystem` |
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.
Thoughts on CoreRegistrationSystem
vs just RegistrationSystem
?
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.
CoreRegistrationSystem
is less generic, easier to search for, fits CoreModule
a bit better
Not a strong opinion
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.
none of the other systems are called "core" though 🤷
(I think this is fine either way and easy enough to rename later if we decide to)
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 guess I see Core
as some aggregation prefix
agree that it's fine either way
export const coreModuleDeployedBytecodeSize = size(coreModuleBuild.deployedBytecode.object as Hex); | ||
export const coreModuleBytecode = encodeDeployData({ | ||
bytecode: coreModuleBuild.bytecode.object as Hex, | ||
abi: [], | ||
abi: parseAbi(["constructor(address,address,address,address)"]), |
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.
is it possible to get this form an import of the code module? so this can throw a type error when it changes instead of a runtime error if the abi doesn't match
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.
Maybe with abitype (we don't have it in cli package yet)
I'd rather not do this as part of this PR, or right now given I wanna move on to codegen refactor
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 think what he means is importing the typed ABI, i.e.
import coreModuleAbi from "@latticexyz/world/out/CoreModule.sol/CoreModule.abi.json" assert { type: "json" };
export const coreModuleBytecode = encodeDeployData({
bytecode: coreModuleBuild.bytecode.object as Hex,
abi: coreModuleAbi,
...
});
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.
Oh I see, well yeah point is I remember next to no context for this
Co-authored-by: alvarius <[email protected]>
No description provided.