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

EVM property doesn't contain event emitter properties #2181

Closed
acolytec3 opened this issue Aug 18, 2022 · 5 comments
Closed

EVM property doesn't contain event emitter properties #2181

acolytec3 opened this issue Aug 18, 2022 · 5 comments

Comments

@acolytec3
Copy link
Contributor

acolytec3 commented Aug 18, 2022

As identified by the Ganache team, the current vm.evm property is typed as EVMInterface | EVM and this leads to DX challenges where

const vm = await VM.create({})
vm.evm.on("step"...  //yields a type error 

yields a type error in the IDE because Typescript can't infer the type EVM for vm.evm due to the EVMInterface not extending the EventEmitter class. For users that want to access the step event and other the other public properties/events of the EVM class while referencing it from the VM, they are currently forced to do:

;(vm.evm as EVM).on...

each time they want to access something exposed by the EVM class but not included in the interface. The "Typescript Way" ™️ of handling stuff like this is through a type guard, I propose adding a type guard as a static method on the EVM class to allow teams to quickly access the type of the EVM property once at the start of their code and then not have to worry about typecasting.

It would look something like this:

class EVM extends EventEmitter {
// class implementation
const static isEVM = (evm: EVMInterface | EVM): evm is EVM => {
  return (evm as EVM)._transientStorage !== undefined
// more class implementation stuff
}

and be used something like:

import { EVM } from '@ethereumjs/evm'
import { VM } from '@ethereumjs/vm'

tape('test vm', async (t) => {
  const vm = await VM.create({})

  if (!EVM.isEVM(vm.evm)) throw new Error("EVM must be a real EVM!!!")

  vm.evm.on... //not a type error!
})

WDYGT?

cc: @davidmurdoch @holgerd77 @jochem-brouwer

@davidmurdoch
Copy link
Contributor

davidmurdoch commented Aug 18, 2022

Why not make the EVMInterface match the actual implementation?

If the above type guard is required we'll just write our own VM types to fix it ourselves. A runtime check for a compile time problem seems silly.

@jochem-brouwer
Copy link
Member

If we do that, we require the interface to implement those events, which is not exactly necessary to work with our libraries.

For a non-runtime fix see #2182

@davidmurdoch
Copy link
Contributor

Your Generics solution looks to be more useful to Ganache!

@jochem-brouwer
Copy link
Member

That's great, in that case I will continue on that tomorrow (unless @acolytec3 or someone else has reservations here). We can most likely also solve the problem with the EEIInterface.

@acolytec3 acolytec3 linked a pull request Aug 19, 2022 that will close this issue
@jochem-brouwer
Copy link
Member

This is closed by #1955, #2184, #2235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants