-
Notifications
You must be signed in to change notification settings - Fork 783
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
Update rustbn-wasm usage #3304
Update rustbn-wasm usage #3304
Changes from all commits
a5f58e7
ff65056
fc072c7
90d162b
30578b9
65158d1
83b2932
e515ce1
2cfa35c
c4d2fab
de1b813
8e07850
a90a829
2c6a6c6
6af1c8f
18fc5bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,10 @@ | ||
import { Chain, Common } from '@ethereumjs/common' | ||
import { EVM } from '@ethereumjs/evm' | ||
|
||
const common = new Common({ chain: Chain.Mainnet, eips: [3074] }) | ||
const evm = new EVM({ common }) | ||
console.log(`EIP 3074 is active - ${evm.common.isActivatedEIP(3074)}`) | ||
const main = async () => { | ||
const common = new Common({ chain: Chain.Mainnet, eips: [3074] }) | ||
const evm = await EVM.create({ common }) | ||
console.log(`EIP 3074 is active - ${evm.common.isActivatedEIP(3074)}`) | ||
} | ||
|
||
main() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
zeros, | ||
} from '@ethereumjs/util' | ||
import debugDefault from 'debug' | ||
import { initRustBN } from 'rustbn-wasm' | ||
|
||
import { EOF, getEOFCode } from './eof.js' | ||
import { ERROR, EvmError } from './exceptions.js' | ||
|
@@ -137,7 +138,24 @@ | |
|
||
protected readonly _emit: (topic: string, data: any) => Promise<void> | ||
|
||
constructor(opts: EVMOpts = {}) { | ||
private bn128: { | ||
ec_pairing: (input_str: string) => string | ||
ec_add: (input_str: string) => string | ||
ec_mul: (input_hex: string) => string | ||
} | ||
|
||
static async create(createOpts?: EVMOpts) { | ||
const opts = createOpts ?? ({} as EVMOpts) | ||
if (opts?.bn128 === undefined) { | ||
opts.bn128 = await initRustBN() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it an idea to add something similar like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is breaking 🤔 Do we want this? When we still had There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
throw new Error('rustbn not provided') | ||
} | ||
this.bn128 = opts.bn128 // Required to instantiate the EVM | ||
this.events = new AsyncEventEmitter() | ||
|
||
this._optsCached = opts | ||
|
@@ -1079,6 +1097,7 @@ | |
...this._optsCached, | ||
common, | ||
stateManager: this.stateManager.shallowCopy(), | ||
bn128: this.bn128, | ||
} | ||
;(opts.stateManager as any).common = common | ||
return new EVM(opts) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to create a
EVM.create
method similar toVM.create
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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