-
Notifications
You must be signed in to change notification settings - Fork 214
feat(world): find systems based on inheritance #3649
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
Conversation
🦋 Changeset detectedLatest commit: 1d087e9 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 |
@@ -16,7 +16,8 @@ type BuildOptions = { | |||
}; | |||
|
|||
export async function build({ rootDir, config, foundryProfile }: BuildOptions): Promise<void> { | |||
await Promise.all([tablegen({ rootDir, config }), worldgen({ rootDir, config })]); | |||
await tablegen({ rootDir, config }); | |||
await worldgen({ rootDir, config }); |
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.
this has to be done serially now that we're parsing all *.sol
source files in worldgen to determine if a source file is a system, otherwise we get a bunch of "file does not exist" errors from tablegen deleting/creating files
3e14de5
to
efae95c
Compare
efae95c
to
2b0053b
Compare
I mention slang a bit in #3660 (doesn't conflict with this PR) |
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.
Looks good! Should we add a config option to enable/disable this new behavior? Thinking about codebases that organized in some way that didn't expect all contracts that extend System
to be automatically deployed and registered, or codebases that may have too many (irrelevant) files for which it would be a slowdown to have to parse all of them every time? (I think it's fine to enable this new behavior by default but would be nice to be able to opt out. If the opt out flag would be set, we could just early return after if (contractName.endsWith("System") && contractName !== baseSystemName) return true;
)
Happy to put this behind a default-on config option.
Note that this would still read+parse each file (the expensive bit). This is mostly to avoid deploying abstract contracts. I don't think the perf cost is huge here, so can leave this as is and the improve this if we find opted-out folks with perf issues. |
added an issue to follow up on this later: #3665 |
TODO: add tests