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): hellostore in IStoreEvents #2357

Merged
merged 5 commits into from
Mar 4, 2024

Conversation

yonadaa
Copy link
Contributor

@yonadaa yonadaa commented Mar 1, 2024

No description provided.

@yonadaa yonadaa requested review from alvrs and holic as code owners March 1, 2024 13:50
Copy link

changeset-bot bot commented Mar 1, 2024

🦋 Changeset detected

Latest commit: ad88233

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

* @notice Emitted when the store is initialized.
* @param storeVersion The version of the Store contract.
*/
event HelloStore(bytes32 indexed storeVersion);
Copy link
Member

Choose a reason for hiding this comment

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

nit: would put this at the top since it's the first event you'll see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was also wondering that haha

@yonadaa yonadaa changed the title refactor(store): HelloStore in IStoreEvents refactor(store): hellostore in IStoreEvents Mar 1, 2024
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.

don't forget a changeset!

should we make it so StoreData.sol uses emit IStoreEvents.HelloStore rather than emit HelloStore for consistency?

and maybe IStore inherits IStoreEvents directly (rather than transitively) so that we get an error in StoreData.sol if we don't use the interface reference for emitting the event?
nevermind, don't think this is possible with how we've set up store abstractions

@yonadaa
Copy link
Contributor Author

yonadaa commented Mar 1, 2024

should we make it so StoreData.sol uses emit IStoreEvents.HelloStore rather than emit HelloStore for consistency?

sure, seems more flexible to me!

@yonadaa yonadaa merged commit c58da9a into main Mar 4, 2024
11 checks passed
@yonadaa yonadaa deleted the yonadaaa/consolidate-events branch March 4, 2024 11:38
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