Ts/instr#50
Conversation
|
Review changes with Changed Files
|
Summary of ChangesHello @tcrypt25519, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the bytecode instruction processing logic within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the instruction handling logic. The Instruction enum is now more efficient with Copy and #[repr(u8)]. The instruction iterator has been refactored for better robustness, fixing a potential infinite loop on invalid opcodes and preventing panics from malformed PUSH instructions. The logic for processing PUSH data has been moved and optimized to avoid heap allocations. These are excellent changes that improve performance, correctness, and maintainability.
| #[inline] | ||
| pub const fn is_push(self) -> bool { | ||
| let op = self.opcode(); | ||
| op >= 0x5f && op <= 0x7f // PUSH0..PUSH32 | ||
| } | ||
|
|
||
| #[inline] | ||
| pub const fn push_len(self) -> usize { | ||
| if self.is_push() { | ||
| (self.opcode() - 0x5f) as usize | ||
| } else { | ||
| 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Using magic numbers like 0x5f and 0x7f makes the code harder to read and maintain. It's better to use the Instruction enum variants directly to represent these opcodes. This improves clarity and ensures that if the opcode values ever change, this code will update automatically.
Since opcode() is a const fn, you can use Instruction::PUSH0.opcode() and Instruction::PUSH32.opcode() within these const functions.
| #[inline] | |
| pub const fn is_push(self) -> bool { | |
| let op = self.opcode(); | |
| op >= 0x5f && op <= 0x7f // PUSH0..PUSH32 | |
| } | |
| #[inline] | |
| pub const fn push_len(self) -> usize { | |
| if self.is_push() { | |
| (self.opcode() - 0x5f) as usize | |
| } else { | |
| 0 | |
| } | |
| } | |
| #[inline] | |
| pub const fn is_push(self) -> bool { | |
| let op = self.opcode(); | |
| op >= Self::PUSH0.opcode() && op <= Self::PUSH32.opcode() | |
| } | |
| #[inline] | |
| pub const fn push_len(self) -> usize { | |
| if self.is_push() { | |
| (self.opcode() - Self::PUSH0.opcode()) as usize | |
| } else { | |
| 0 | |
| } | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR refactors the EVM instruction iteration layer in crates/jet to make opcodes cheaper to work with (Copy + repr(u8)) and to change PUSH handling from “materialize a 32-byte immediate in the iterator” to “yield a borrowed slice and let the builder construct the 32-byte word”.
Changes:
- Make
InstructionCopywith#[repr(u8)], and addopcode(),is_push(), andpush_len()asconst fns. - Replace
instructions::Iterator/IteratorItemwithinstructions::Iter/IterItem, and have PUSH yield(pc, instr, &[u8])rather than[u8; 32]. - Move PUSH immediate endian conversion/allocation work into
builder/contract.rs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/jet/src/instructions.rs | Refactors instruction enum + introduces Iter/IterItem with borrowed PUSH data slices. |
| crates/jet/src/builder/contract.rs | Updates bytecode walking to use Iter and reconstructs PUSH immediates for ops::push. |
| let mut new_data = [0u8; 32]; | ||
| new_data[..data.len()].copy_from_slice(data); | ||
| new_data[..data.len()].reverse(); | ||
|
|
There was a problem hiding this comment.
PUSH immediate decoding is incorrect when the bytecode ends before all expected immediate bytes are present. data only contains the available bytes, but EVM semantics treat missing PUSH bytes as 0x00 at the end of the immediate (big-endian), which affects the numeric value after reversing to little-endian. Current code reverses only data.len() bytes, so e.g. PUSH2 0xAA would become 0x00AA instead of 0xAA00. Use the included Instruction (push length) to pad missing bytes before reversing / constructing the 32-byte word.
| let push_len = instr.push_len(); | ||
| let push_start = pc + 1; | ||
| let push_end = push_start + push_len; | ||
|
|
||
| // Copy the push data into a 32-byte array, converting from big endian to little endian | ||
| let push_data = { | ||
| let mut data = [0; 32]; | ||
| match push_len { | ||
| 0 => (), | ||
| _ => { | ||
| let reversed = &self.rom[push_start..push_end] | ||
| .iter() | ||
| .rev() | ||
| .cloned() | ||
| .collect::<Vec<u8>>(); | ||
| data[..push_len].copy_from_slice(reversed.as_slice()) | ||
| } | ||
| }; | ||
| data | ||
| }; | ||
| let push_end = std::cmp::min(push_start + push_len, self.rom.len()); | ||
|
|
||
| self.pc += push_len + 1; | ||
| Some(IteratorItem::PushData(pc, push_data)) | ||
| self.pc = push_end; | ||
| Some(IterItem::PushData( | ||
| pc, | ||
| instr, | ||
| &self.rom[push_start..push_end], | ||
| )) |
There was a problem hiding this comment.
The iterator now yields potentially-short PUSH immediates at EOF (via min(push_start + push_len, rom.len())). Given the subtle semantics around missing PUSH bytes being treated as zero, it would be good to add a ROM test that ends with a truncated PUSHn (e.g. PUSH2 0xAA at EOF) to lock in correct padding/endianness behavior.
* tweak: Better utilize comp-time work.
No description provided.