feat(world-consumer): convert store-consumer package into world-consumer #3584
Conversation
🦋 Changeset detectedLatest commit: d4b8b11 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 |
vdrg
force-pushed
the
vdrg/world-consumer
branch
from
1ad8426 to
70155f6
Compare
.changeset/shy-terms-walk.md
Outdated
| --- | ||
|
|
||
| Renamed `store-consumer` package to `world-consumer`. The `world-consumer` package now only includes a single `WorldConsumer` contract that always attaches to a World. | ||
| The `world-module-erc20` package was updated to reflect this refactor. |
There was a problem hiding this comment.
I'd prob move this to its own changeset like
Migrated from
store-consumertoworld-consumer.
| error WorldConsumer_NamespaceDoesNotExists(bytes14 namespace); | ||
| error WorldConsumer_CallerHasNoNamespaceAccess(bytes14 namespace, address caller); | ||
| error WorldConsumer_CallerIsNotWorld(address caller); | ||
| error WorldConsumer_ValueNotAllowed(); |
There was a problem hiding this comment.
I wonder if its worth adding a worldAddress to each? might make it easier to see what world is trying to be called if you end up configuring or calling this wrong
I also wonder about bytes14 namespace vs ResourceId namespaceId since the latter is generally what we pass around inside world
There was a problem hiding this comment.
ah let me check other errors
There was a problem hiding this comment.
mm so we have:
error World_AccessDenied(string resource, address caller);
error World_InvalidNamespace(bytes14 namespace);
As the current error is just referring to a namespace, I feel it might be more consistent to keep it as just bytes14 but not sure.
Adding the world address makes sense.
There was a problem hiding this comment.
Added it as a IBaseWorld world
There was a problem hiding this comment.
I think we can keep it address world to simplify if you wanted to avoid casting everywhere
I also wonder if we should use _world() and WorldContext._world()
mud/packages/world/src/WorldContext.sol
Lines 95 to 97 in b790181
Instead of reimplementing + renaming as getWorld()?
There was a problem hiding this comment.
ah good point! will do, this was from before using world context
There was a problem hiding this comment.
done, ended up using _world() directly for most things so turned the events args to address worldAddress.
Co-authored-by: Kevin Ingersoll <[email protected]>
Co-authored-by: Kevin Ingersoll <[email protected]>
vdrg
force-pushed
the
vdrg/world-consumer
branch
from
29ec2dd to
5156f55
Compare
| import { System } from "@latticexyz/world/src/System.sol"; | ||
|
|
||
| abstract contract WorldConsumer is System { | ||
| bytes14 public immutable namespace; |
There was a problem hiding this comment.
not blocking, but do we need this anymore if we can now do namespaceId.getNamespace()?
There was a problem hiding this comment.
well if we are doing the immutable stuff for efficiency, we might as well keep the namespace and use it directly
There was a problem hiding this comment.
oh does immutable not have the same read gas cost? I assumed if we used both of these variables in one call, we'd have 2x storage look ups
There was a problem hiding this comment.
yeah immutable actually works similar to constants, the data gets added to the bytecode so you are not reading from storage. That reminds me that immutable vars shouldn't be readable from within the constructor, so not sure why the tests are passing 🤔
There was a problem hiding this comment.
Ah since solidity 0.8.21 they can be read in the constructor, all good
Simplifies the existing
store-consumerpackage to just support consuming from a World. Updates theworld-module-erc20contracts to inherit from the newWorldConsumercontract.