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

MCOPY opcode (EIP-5656) #289

Merged
merged 1 commit into from
Sep 2, 2023
Merged

MCOPY opcode (EIP-5656) #289

merged 1 commit into from
Sep 2, 2023

Conversation

gzanitti
Copy link
Contributor

@gzanitti gzanitti commented Jul 18, 2023

Overview

This PR introduces a new opcode (MCOPY) introduced in EIP-5656.

The changes needed seem to be minimal (just added a new variant to the Opcode enum in evm.rs, so that the parser/bytecodegen will be able to understand the new opcode), but it was a good way to get acquainted with the codebase.

Checklist

  • Added a new opcode mcopy at 0x5E. Changes here, here, here and here (all in the same file).

Test

I'd like to add a small test, something as simple as compiling:

#define macro MAIN() = {
    0x00
    0x00
    0x20
    mcopy
}

which becomes 5f5f6020205e, but I couldn't find the right place to put it (and maybe it's not necessary for such a trivial change).

Anyway, I look forward to your recommendations & corrections, so please, feel free to leave any comments you may have 😃

Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in the review.

Overall looks awesome, could you add a small test covering its usage. Once thats in lets get it merged!

@gzanitti
Copy link
Contributor Author

Apologies for the delay in the review.

Overall looks awesome, could you add a small test covering its usage. Once thats in lets get it merged!

Don't worry, there's no rush. As for the test, my idea was to add a test similar to the one I mentioned at the beginning of this thread (something that checks the transformation of the opcode into its corresponding bytecode), but that is something that already happens here. Do you consider that any other type of test is necessary?

@Maddiaa0
Copy link
Member

Maddiaa0 commented Sep 2, 2023

Apologies for the delay in the review.
Overall looks awesome, could you add a small test covering its usage. Once thats in lets get it merged!

Don't worry, there's no rush. As for the test, my idea was to add a test similar to the one I mentioned at the beginning of this thread (something that checks the transformation of the opcode into its corresponding bytecode), but that is something that already happens here. Do you consider that any other type of test is necessary?

Ah thats absolutely good enough!

@Maddiaa0 Maddiaa0 merged commit 2395ef6 into huff-language:stage Sep 2, 2023
@Maddiaa0 Maddiaa0 mentioned this pull request Oct 1, 2023
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.

2 participants