Skip to content
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

feat(world): remove system name from function signatures/selectors [M-05] #2160

Merged
merged 13 commits into from
Jan 22, 2024

Conversation

holic
Copy link
Member

@holic holic commented Jan 18, 2024

closes #1980

Copy link

changeset-bot bot commented Jan 18, 2024

🦋 Changeset detected

Latest commit: b59e02f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@latticexyz/world-modules Major
@latticexyz/world Major
@latticexyz/cli Major
@latticexyz/dev-tools Major
@latticexyz/store-sync Major
@latticexyz/store-indexer Major
@latticexyz/abi-ts Major
@latticexyz/block-logs-stream Major
@latticexyz/common Major
@latticexyz/config Major
create-mud Major
@latticexyz/ecs-browser Major
@latticexyz/faucet Major
@latticexyz/gas-report Major
@latticexyz/network Major
@latticexyz/noise Major
@latticexyz/phaserx Major
@latticexyz/protocol-parser Major
@latticexyz/react Major
@latticexyz/recs Major
@latticexyz/schema-type Major
@latticexyz/services Major
@latticexyz/solecs Major
solhint-config-mud Major
solhint-plugin-mud Major
@latticexyz/std-client Major
@latticexyz/std-contracts Major
@latticexyz/store-cache Major
@latticexyz/store Major
@latticexyz/utils Major

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


Functions within namespaced systems are now registered on the world as `{namespace}_{functionName}` rather than `{namespace}_{systemName}_{functionName}`. This is more ergonomic and is more consistent with namespaced resources in other parts of the codebase (e.g. accessing tables in schemaful indexer).

If you have a project using the `namespace` key in your `mud.config.ts` or are manually registering systems and function selectors on a namespace, you will likely need to codegen your system interfaces (`pnpm build`) and update any calls to these systems through the world's namespaced function signatures.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to bubble this up as a breaking change aside from major tag?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

major should be only breaking changes, no other way make it more prominent unfortunately

@yonadaaa
Copy link
Contributor

No suggestions yet but I wanna experiment with this a bit, think about what happens when you register multiple systems with the same name etc.

Maybe document everything in tests if not already covered

@holic
Copy link
Member Author

holic commented Jan 19, 2024

think about what happens when you register multiple systems with the same name

@alvrs and I chatted a bit about this IRL and we think it'll just end up with a compile error, because codegen will create interfaces for each system and those will get combined into a IWorld interface, but that won't compile if there are function signature/selector overlaps

@alvrs
Copy link
Member

alvrs commented Jan 19, 2024

@qbzzt there might be a couple places in the docs we need to update with this!

alvrs
alvrs previously approved these changes Jan 19, 2024
@yonadaaa
Copy link
Contributor

it'll just end up with a compile error, because codegen will create interfaces for each system and those will get combined into a IWorld interface, but that won't compile if there are function signature/selector overlaps

The compiler error is unfortunate, I wonder if we can surface something more helpful in codegen? Trying it locally I get:

image

@holic
Copy link
Member Author

holic commented Jan 19, 2024

The compiler error is unfortunate, I wonder if we can surface something more helpful in codegen? Trying it locally I get:

image

Hmm, those don't appear to be namespaced? But the compiler does communicate well enough that there's an overlap.

We could def add more validation+errors to codegen.

@yonadaaa
Copy link
Contributor

yonadaaa commented Jan 19, 2024

Random thought:
This makes me wonder if we can/should hide the concept of Systems, instead focusing on functions and having "one contract interface per namespace".

Instead of registerSystem, it could be registerFunction. Each function is registered separately along with the address of it's bytecode.

Need to think on it more but maybe it would remove issues like this. Definitely out of scope for this PR!

@yonadaaa
Copy link
Contributor

Hmm, those don't appear to be namespaced?

Oh, oops, they weren't. And you're right that the compiler error is pretty decent.

yonadaaa
yonadaaa previously approved these changes Jan 19, 2024
qbzzt added a commit that referenced this pull request Jan 19, 2024
The old format is `namespace_system_function`.
The new format is `namespace__function`.

This should be merged at the same time as #2160, which implements the change.
@alvrs alvrs dismissed stale reviews from yonadaaa and themself via d25dfda January 19, 2024 18:49
@alvrs alvrs force-pushed the holic/function-selector-system-name branch from d25dfda to b179a1a Compare January 19, 2024 18:52
@alvrs alvrs changed the title feat(world): remove system name from function signatures/selectors feat(world): remove system name from function signatures/selectors [M-05] Jan 22, 2024
@alvrs alvrs merged commit 0f27afd into main Jan 22, 2024
11 checks passed
@alvrs alvrs deleted the holic/function-selector-system-name branch January 22, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove system name from function signatures/selectors
3 participants