Skip to content

feat(store): improve memory copy #3400

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

Merged
merged 10 commits into from
Mar 17, 2025
Merged

feat(store): improve memory copy #3400

merged 10 commits into from
Mar 17, 2025

Conversation

dk1a
Copy link
Contributor

@dk1a dk1a commented Dec 21, 2024

Memory.copy behavior is unchanged (I changed dev comment however to better reflect mcopy's description, rather than the previous workaround)
Only the relevant bytecode is changing (which I think what getStoreLogs.test.ts is about)

I wanted to use mcopy from the beginning, but back in 2023 that eip wasn't implemented yet, so the lengthy code was a less efficient workaround
With solidity 0.8.24 the opcode is usable

Copy link

changeset-bot bot commented Dec 21, 2024

🦋 Changeset detected

Latest commit: 34ca028

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/entrykit Patch
@latticexyz/explorer Patch
@latticexyz/react Patch
@latticexyz/stash Patch
@latticexyz/store-indexer Patch
@latticexyz/store-sync Patch
@latticexyz/world-consumer Patch
@latticexyz/world-module-callwithsignature Patch
@latticexyz/world-module-erc20 Patch
@latticexyz/world-module-metadata 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/faucet Patch
@latticexyz/gas-report Patch
@latticexyz/paymaster Patch
@latticexyz/protocol-parser Patch
@latticexyz/recs Patch
@latticexyz/schema-type Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
@latticexyz/utils Patch
vite-plugin-mud 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

@dk1a
Copy link
Contributor Author

dk1a commented Dec 21, 2024

What do you do with store logs? The remaining failing tests seem unrelated and happen for me even on main

@dk1a dk1a marked this pull request as ready for review December 23, 2024 16:21
@dk1a dk1a requested review from ludns, alvrs and frolic as code owners December 23, 2024 16:21
@vdrg
Copy link
Contributor

vdrg commented Jan 3, 2025

@dk1a nice! I think you just need to update the vitest snapshots, go to the store package and run pnpm test --update. Also, you need to generate the docs with pnpm docs:generate:api.

@dk1a dk1a force-pushed the dk1a/mcopy branch 2 times, most recently from 4a4c005 to 6f7d9b0 Compare January 15, 2025 13:56
@dk1a
Copy link
Contributor Author

dk1a commented Jan 15, 2025

Reinstalling everything locally seems to have helped with docs - I'd run them before but had different results
Also got confused by test's expectations about setup (just running vitest update wasn't enough), but I think I got it correctly now

@frolic
Copy link
Member

frolic commented Jan 15, 2025

I'm curious why we see gas improvements in store but not world?

@dk1a
Copy link
Contributor Author

dk1a commented Jan 15, 2025

I'm curious why we see gas improvements in store but not world?

Memory.copy is only used for reading dynamic fieds afaik, and world's gas tests don't include any dynamic reads I guess
Looking at world, it's mostly static fields in general, FunctionSignatures being a minor exception

@frolic frolic added blocked by audit needs audit This code needs to be audited and removed blocked by audit labels Jan 23, 2025
@alvrs alvrs changed the base branch from main to audit-2 March 17, 2025 20:04
@alvrs alvrs merged commit 2f98e92 into latticexyz:audit-2 Mar 17, 2025
3 of 16 checks passed
@alvrs
Copy link
Member

alvrs commented Mar 17, 2025

combining all changes that require an audit into #3630

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs audit This code needs to be audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants