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

refactor(store): event interfaces for Store libraries #2348

Merged
merged 13 commits into from
Mar 1, 2024

Conversation

yonadaa
Copy link
Contributor

@yonadaa yonadaa commented Feb 29, 2024

Exploring my comment on #2158 (comment)

Basically some errors (FieldLayoutLib_*, PackedCounter_*, etc.), are not included in the IStore ABI because they are not defined in that interface or anything it inherits. Instead, they are defined in libraries and are referenced only in the implementation of Store.

Some solutions:

  • Define error interfaces for each library, eg. IPackedCounterErrors (this PR)
  • Move every error to a single IStoreErrors
  • Use abstract contracts like OpenZeppelin (I'm not sure how that works)
  • Stop using interfaces and have clients import the Store implementation

Copy link

changeset-bot bot commented Feb 29, 2024

🦋 Changeset detected

Latest commit: 091a713

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

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

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

@yonadaa yonadaa changed the title refactor(store): IFieldLayoutErrors and IPackedCounterErrors refactor(store): event interfaces for Store libraries Feb 29, 2024
@yonadaa yonadaa marked this pull request as ready for review February 29, 2024 15:01
@yonadaa yonadaa requested a review from alvrs as a code owner February 29, 2024 15:01
Comment on lines 4 to 8
/**
* @title IFieldLayoutErrors
* @author MUD (https://mud.dev) by Lattice (https://lattice.xyz)
* @notice This interface includes errors for the FieldLayout library.
*/
Copy link
Member

@holic holic Feb 29, 2024

Choose a reason for hiding this comment

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

Can we add the why to each of these?

Suggested change
/**
* @title IFieldLayoutErrors
* @author MUD (https://mud.dev) by Lattice (https://lattice.xyz)
* @notice This interface includes errors for the FieldLayout library.
*/
/**
* @title IFieldLayoutErrors
* @author MUD (https://mud.dev) by Lattice (https://lattice.xyz)
* @notice This interface includes errors for the FieldLayout library.
* @dev We centralize and reference these errors in an interface (instead of in libraries or files) so that we can ensure that all possible errors are included in the resulting Store ABI for proper decoding in the frontend. This is a workaround until the Solidity compiler does this for us.
*/

holic
holic previously approved these changes Feb 29, 2024
Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

looking good! just want to include a comment in each interface about why we're doing this approach, for future spelunkers (including ourselves)

did we double check we got all error types?

@yonadaa
Copy link
Contributor Author

yonadaa commented Mar 1, 2024

I agree we can improve the comments! afaik this covers all events in Store, then we tackle World later.

This is a workaround until the Solidity compiler does this for us.

Gonna reword this because I don't think this is actually a bug - we just didn't include those events in IStore. It's not obvious to the compiler that the events that StoreCore uses should be included in the IStore ABI, because it's just one implementation of the interface.

@holic
Copy link
Member

holic commented Mar 1, 2024

wondering if we should include some/all errors in the other interfaces as well: https://github.com/latticexyz/mud/blob/main/packages/store/src/IStoreRead.sol#L13

@yonadaa
Copy link
Contributor Author

yonadaa commented Mar 1, 2024

wondering if we should include some/all errors in the other interfaces as well

Technically yes, but I tried it and there is an issue with inheritance (Linearization of inheritance graph impossible) because IStore inherits IStoreRead.

If we wanna do this properly, we should actually remove IStore is IStoreErrors, and have either IStoreRead or IStoreWrite inherit the errors that they can fail with (then IStore gets them via inheritance)

holic
holic previously approved these changes Mar 1, 2024
Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

one more comment for IStoreErrors imo (since it's the same pattern as our other error interfaces) but otherwise lgtm

@holic
Copy link
Member

holic commented Mar 1, 2024

don't forget a changeset!

@yonadaa yonadaa merged commit c991c71 into main Mar 1, 2024
11 checks passed
@yonadaa yonadaa deleted the yonadaaa/error-interfaces branch March 1, 2024 11:22
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.

2 participants