Update rustbn-wasm usage #3304
Update rustbn-wasm usage #3304
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -137,9 +137,15 @@ export class EVM implements EVMInterface { | |||
|
|
|||
| protected readonly _emit: (topic: string, data: any) => Promise<void> | |||
|
|
|||
| constructor(opts: EVMOpts = {}) { | |||
| this.events = new AsyncEventEmitter() | |||
| private bn128: { | |||
There was a problem hiding this comment.
Does it make sense to create a EVM.create method similar to VM.create?
There was a problem hiding this comment.
That thought definitely occurred to me while I was doing this. Let me take a look at our current usage of the EVM constructor and see if it makes sense. It would definitely simplify the API
|
Another benifit of this change is you should see slightly faster startup time because wasm compilation won't block module resolution |
packages/ethereum-tests
Outdated
There was a problem hiding this comment.
The ethereum-tests update is in this PR?
|
|
||
| static async create(opts: EVMOpts) { | ||
| if (opts.bn128 === undefined) { | ||
| opts.bn128 = await initRustBN() |
There was a problem hiding this comment.
Is it an idea to add something similar like customCrypto for these rustbn-wasm? Then we would treat them at the same foot as the crypto?
There was a problem hiding this comment.
I thought about that but this isn't really "custom crypto". This is just our existing dependency but since it now requires an async constructor, I'm having to pass it in. In this case, I don't think it makes sense to instantiate it in Common or even attach it there. There isn't an alternative implementation of the crypto anywhere that I'm aware of aside from the one older ASM version we were previously using.
| return new EVM(opts) | ||
| } | ||
| constructor(opts: EVMOpts) { | ||
| if (opts.bn128 === undefined) { |
There was a problem hiding this comment.
This is breaking 🤔 Do we want this?
When we still had mcl-wasm I think we added an initPromise to the EVM. Whenever something was called, first this initPromise was awaited. In this case this initPromise would normally consist of the initRustBN in case the bn128 is not provided 🤔
There was a problem hiding this comment.
What is breaking here? I guess it's not obvious to me why we would want to allow the EVM run without this dep? We've never allowed it before so doesn't make sense to allow it now to my way of thinking.
packages/evm/src/evm.ts
Outdated
| ec_mul: (input_hex: string) => string | ||
| } | ||
|
|
||
| static async create(opts: EVMOpts) { |
There was a problem hiding this comment.
I think we should make opts optional.
| static async create(opts: EVMOpts) { | |
| static async create(opts?: EVMOpts) { |
There was a problem hiding this comment.
LGTM! 🎉
@roninjin10 FYI: we are now actually planning to do out of schedule breaking releases for EVM/VM on this (which is also an experiment for us).
We would not want to do this too often, but for this very general fix to make and the new EVM.create() constructor it seems justified here.
If you do opinions and/or remarks on this let us know! 🙏 🙂
|
(so this will slightly postpone the schedule for this release round, pushing this to next week) |
This PR updates the
rustbn-wasmverion to use the new API of an async init function, followingkzg-wasm.It also adds a new
evm.createasync constructor.