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

feat: Lazy wasm pt4 #11491

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

feat: Lazy wasm pt4 #11491

wants to merge 34 commits into from

Conversation

Thunkar
Copy link
Contributor

@Thunkar Thunkar commented Jan 24, 2025

Hopefully one of the final parts of this refactor. Handles all the hashing done through bb.js. This is a monster and I'm sorry, but if there's something we do it's hash stuff.

There should not be any unhandled promises left by this PR, thanks to eslint and careful reviewing anywhere we could now have somethingThatIsNowAPromise.toString().

UX regressions introduced by this PR:

  • Authwitness interactions in public are now async, so they cannot be chained. Will try to improve in the future, can be seen in e2e_authwit.test.ts
  • Function selectors now have to be computed asynchronously, which is awkward since we used to have getters for them. I think this can be easily done in codegen and will try to improve in the future.
  • Same with TxHashes, but those are more complicated to precompute

Other considerations:

  • I had to use parseAsync with zod instead of parse, which is known to be slower. I don't think this is going to be a big issue, since it's only used when doing RPC calls, but would love some inputs CC/ @spalladino
  • @trackSpan decorator can now take an async function for its attributes
  • Performance should not suffer since we were crossing the WASM boundary anyways even when using the sync version. In some cases, the usage of Promise.all could have potentially improved it.
  • I have kept the sync version of hashes under @aztec/foundation/crypto/sync, so that @aztec/merkle-trees wouldn't complain. Since the TS implementation of our merkle trees is likely going away, I didn't bother updating them.

@Thunkar Thunkar added e2e-all CI: Enables this CI job. network-all Run this CI job. labels Jan 24, 2025
@Thunkar Thunkar self-assigned this Jan 24, 2025
Copy link
Collaborator

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

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

AVM related stuff all looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Enables this CI job. network-all Run this CI job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants