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!: nuke top level await #10128

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

olehmisar
Copy link
Contributor

@olehmisar olehmisar commented Nov 21, 2024

Closes #9959

May be postponed if #9987 makes performance acceptable.

Removed top-level await. BarretenbergSync.getSingleton and all functions and methods that depend on it are now marked as async.

Changes

  • FunctionSelector.equals cannot accept string and { name: string; parameters: ABIParameter[] } anymore. Otherwise it has to be async as well
  • EntrypointPayload, AppEntrypointPayload, and FeeEntrypointPayload are worth taking closer look at as they were changed more than other classes
  • Carefully review orchestrator.ts in prover-client because i have no idea what that code is
  • await wallet.setPublicAuthWit({ caller, action }, true).send().wait() is now await (await wallet.setPublicAuthWit({ caller, action }, true).send()).wait()
  • contract.methods.xxx.selector now returns a Promise<FunctionSelector>

There may be bugs introduced

  • if functions that previously returned boolean were changed to return Promise and now they always return truthy values (because Promise is truthy)
  • if functions that previously thrown an error synchronously were changed to async and now they throw asynchronously, so it breaks the flow

@olehmisar
Copy link
Contributor Author

still countless errors to fix

@ludamad
Copy link
Collaborator

ludamad commented Nov 22, 2024

@charlielye any comments on the approach of mass-asyncifying bb?

@olehmisar
Copy link
Contributor Author

everything except end-to-end typechecks. Waiting whether this PR is even gonna be merged before continuing. Also, see the PR description for substantial changes and potential bugs introduced.

@Groxan
Copy link

Groxan commented Nov 27, 2024

Why not to simply remove await BarretenbergSync.initSingleton(); and make users call it on their side instead? In this case you will give users more control and, the most important, won't break the API.

@olehmisar
Copy link
Contributor Author

@Groxan it can be done as a temporary workaround. But this PR should be the end solution

@Groxan
Copy link

Groxan commented Nov 27, 2024

@olehmisar Why do you think that couldn't be an end solution? Allowing users to initialize the "environment" manually and, in particular, allowing them to choose the best moment for it is actually a better practice. At least, that's how it currently works for node devs.

@olehmisar
Copy link
Contributor Author

@Groxan UI apps don’t have a top-to-bottom flow like cli apps. Imagine your app crashing randomly here and there because some wasm code was not initialised.

@Groxan
Copy link

Groxan commented Nov 27, 2024

@olehmisar Nothing will crash if you initialize it in advance

@Groxan
Copy link

Groxan commented Nov 27, 2024

Initialization on-demand, as you propose, is fine when the initialization itself takes <100ms. But when the initialization takes several seconds, the UX will leave much to be desired.

@olehmisar
Copy link
Contributor Author

@olehmisar Nothing will crash if you initialize it in advance

how do you initialize in advance in a multipage app that has import {...} from "@aztec/aztec.js" scattered all around the place? It's not a top-to-bottom CLI application.

Initialization on-demand, as you propose, is fine when the initialization itself takes <100ms. But when the initialization takes several seconds, the UX will leave much to be desired.

I don't understand what you wanted to say with this sentence. You have to initialise wasm in any case. The UX will be better if you can do other things while waiting for wasm to be initialised, unlike how it currently halts the whole UI.

@Groxan
Copy link

Groxan commented Nov 29, 2024

It doesn't matter what type of app you have, there is always an entry point. But actually, "in advance" doesn't necessarily mean "at the entry point". "In advance" means "at ANY time before" calling aztec's components, requiring bb to be initialized.

So, I'm just saying that as a developer I'd like to have a choice - how, when and where to initialize bb, because in different use cases different approaches may be needed to give better UX.

@olehmisar
Copy link
Contributor Author

This discussion leads to nowhere so I am not gonna entertain it anymore. Nuking top level await in favour of async initialisation does not take any control from the user. Requiring the user to initialize bb manually is a half measure that is gonna lead to a lot of headaches (look at Aleo)

@ludamad
Copy link
Collaborator

ludamad commented Nov 30, 2024

Sorry, this is my fault for letting this thread be followed as far as it did, but I think the consensus is we prefer to just drop the top level await, there is already an async API that can be used it preferred. Apologies again

@olehmisar
Copy link
Contributor Author

Just want to say that I didn't intend my previous message to be offensive. Not the best choice of words, so I want to apologize to anyone that may be offended by it.

@rahul-kothari
Copy link
Contributor

fwiw, I think there are some great alternate fixes and PRs for these issues coming down the line!
Definitely a HIGH priority for us. Will loop back with a list of upcoming PRs on this topic

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.

Remove top-level await from aztec.js
4 participants