refactor(store,world): replace isStore with storeAddress

[dk1a]

fixes #940
the previous approach used the absence of isStore to indicate that the contract isn't the Store
I change it so all store consumers need storeAddress method, and its absence is an error use a storage variable to detect storeAddress
Moved MudV2Test (otherwise circular dependency) and took the opportunity to rename it to MudTest

[changeset-bot]

🦋 Changeset detected

Latest commit: fe49822

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

This PR includes changesets to release 26 packages

Name	Type
@latticexyz/std-contracts	Major
@latticexyz/store	Major
@latticexyz/world	Major
@latticexyz/cli	Major
@latticexyz/network	Major
@latticexyz/react	Major
@latticexyz/std-client	Major
@latticexyz/store-cache	Major
@latticexyz/store-sync	Major
@latticexyz/dev-tools	Major
@latticexyz/ecs-browser	Major
@latticexyz/block-logs-stream	Major
@latticexyz/common	Major
@latticexyz/config	Major
create-mud	Major
@latticexyz/gas-report	Major
@latticexyz/noise	Major
@latticexyz/phaserx	Major
@latticexyz/protocol-parser	Major
@latticexyz/recs	Major
@latticexyz/schema-type	Major
@latticexyz/services	Major
@latticexyz/solecs	Major
solhint-config-mud	Major
solhint-plugin-mud	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

[holic]

looks like this upped the gas everywhere?

[dk1a]

looks like this upped the gas everywhere?

pretty much. It increases StoreSwitch calls by ~300 gas (which means any table interactions); relatively it's not big, but affects everything
I don't try to optimize it here, that'd be a separate PR. I'd want to compare using a method to using storage, and maybe rely on a magic return value instead of passing msg.sender arg.
But the best way to do this would probably be some combo of code data location and transient storage, neither of which are out yet

[holic]

is this to help us with the "what context am I running in" issue where tests don't work well with the previous approach?

[dk1a]

is this to help us with the "what context am I running in" issue where tests don't work well with the previous approach?

yep

[dk1a]

I think Consumer is clearer than Context (which makes me think it's something World inherits, not systems), and it matches StoreConsumer

[alvrs]

Context was leaning on ERC2771Context where this pattern originates

[holic]

I feel like Context still makes sense for this?

[alvrs]

yeah i see how Consumer would also make sense but I'd prefer to keep Context to draw the analogy to ERC2771Context

[dk1a]

an alternative to enforcing IStoreConsumer would be to fall back on msg.sender if it's absent. That might help with gas too.
But I like this explicit approach since it gives a nice error instead of confusing behavior down the line, making things hopefully easy to debug (or answer questions about on mud-help)
I was thinking of exploring the alternative in a followup though, along with other gas optimizations

[alvrs]

Not sure if we should put this warning here - the Store package should be independent of the World package, and the warning itself is not detailed enough to be helpful without the full context. Maybe we should a more detailed explanation in the docs and link it somewhere in the world package.

[dk1a]

added a comment to WorldConsumer instead
also agree with docs, should it be in this PR or separate?

[dk1a]

storage slot is mostly better (see #1158), rewrote this PR to use storage instead of a method

[alvrs]

This is great! Just two minor proposed changes for comments. Also let's add a changeset to describe the change.