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

feat: add convert_boxed and insert_boxed for InstructionTable #1194

Merged

Conversation

Rjected
Copy link
Collaborator

@Rjected Rjected commented Mar 13, 2024

This adds a test which serves as an example for adding custom instructions which require some context. This also adds convert_boxed and insert_boxed which should make it easier to do this using the revm builder.

Also adds an example for an instruction with captured context
@Rjected Rjected added the feature New feature or lib ability label Mar 13, 2024
instruction: BoxedInstruction<'a, H>,
) -> Result<(), InsertError> {
match self {
Self::Plain(_) => Err(InsertError::NotBoxed),
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we convert to boxed instead?

Copy link
Member

Choose a reason for hiding this comment

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

I would panic here. Silently converting can have a side-effect on performance

Copy link
Collaborator

Choose a reason for hiding this comment

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

but there's no other way to use custom instructions, the only way is to box all of them, so I think it'd be reasonable to convert here

Copy link
Member

Choose a reason for hiding this comment

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

It should be intentionally called with to_boxed not silently converted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this would make evm setup fallible and force unwrap with a preceding call to to_boxed, i think this makes it a lot less convenient to use, and since additional instructions are only supported by boxing all of them, converting them here seems reasonable to me

Copy link
Member

Choose a reason for hiding this comment

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

That is the point to fail and make it explicit to call, this would look something like.

table.convert_to_box();
table.insert_boxed(instr);

It is redundant but explicit on what is going on.

Not the point I would die on, so lets convert it to boxed inside insert_boxed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to convert into the boxed variant inside insert_boxed. updated docs and the test which no longer needs a map

@Rjected Rjected requested review from mattsse and rakita March 15, 2024 19:41
@Rjected Rjected changed the title feat: add into_box and insert_boxed for InstructionTable feat: add convert_boxed and insert_boxed for InstructionTable Mar 15, 2024
@rakita rakita merged commit d398eef into bluealloy:main Mar 16, 2024
25 checks passed
@github-actions github-actions bot mentioned this pull request Mar 16, 2024
fubuloubu pushed a commit to ApeWorX/revm that referenced this pull request Apr 11, 2024
…loy#1194)

* feat: add into_box and insert_boxed for InstructionTable

Also adds an example for an instruction with captured context

* fix tests with clones

* fix valgrind

* fix valgrind again

* convert to boxed in insert_boxed

* update docs for insert_boxed

* make convert_boxed more concise

* Update crates/interpreter/src/instructions/opcode.rs

* Update crates/interpreter/src/instructions/opcode.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or lib ability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants