Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

fix: remove OpCode enum and update VMOperation's op field type #1904

Merged
merged 5 commits into from
Dec 2, 2022

Conversation

mouseless0x
Copy link

@mouseless0x mouseless0x commented Nov 28, 2022

Motivation

Addresses #1902

Ensures that tracing with VmTrace enabled will continue to be future proof in the presence of any opcodes added/renamed when ethereum undergoes a new hardfork.

Solution

Removed the ethers::core::types::OpCode enum by deleting it's source file

Changed VMOperation's op field to be of type string.

Create new enum ExecutedInstruction that deserializes opcodes from VmTrace into either ExectutedInstruction::Known(Opcode) or ExecutedInstruction::Unknown(String)

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

imo this is probably easier to maintain than an enum, so I'm supportive of this,

@mouseless0x mouseless0x changed the title fix: remove OpCode enum and update VMOperation's op field fix: remove OpCode enum and update VMOperation's op field type Nov 28, 2022
@gakonst
Copy link
Owner

gakonst commented Nov 30, 2022

Should we instead do enum Opcode { Known(Opcode), Unknown(String)? I like the ability to match.

@mouseless0x
Copy link
Author

Should we instead do enum Opcode { Known(Opcode), Unknown(String)? I like the ability to match.

This route looks like a valid solution to unwanted serialization errors - I will work on implementing it, and we can test how it works.

@mouseless0x
Copy link
Author

Unsure whether it is necessary to test deserializing unknown opcodes, can remove the test if not needed

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

typo on the type, lgtm otherwise

ethers-core/src/types/trace/mod.rs Outdated Show resolved Hide resolved
MouseLess.eth and others added 2 commits December 2, 2022 22:40
Copy link
Author

@mouseless0x mouseless0x left a comment

Choose a reason for hiding this comment

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

Good catch, typo has been fixed

@gakonst gakonst merged commit 913f15d into gakonst:master Dec 2, 2022
Copy link
Author

@mouseless0x mouseless0x left a comment

Choose a reason for hiding this comment

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

forgot to add files to prev comment

@mouseless0x mouseless0x deleted the fix/opcode_type branch December 2, 2022 20:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants