refactor: Move jetdbg to own crate; workspace deps.#84
Conversation
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 significantly refactors the project's dependency management and modular structure. 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
|
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring. It moves the jetdbg binary into its own dedicated crate and leverages Cargo workspaces to manage dependencies, which significantly improves the project structure and maintainability. The dependencies in all Cargo.toml files have been correctly updated to use the new workspace definitions.
In addition to the structural changes, this PR also includes several valuable improvements to the jetdbg source code:
- The EVM bytecode for the test contracts has been corrected to use
PUSH20for addresses, and the associated comments are now more descriptive. - A bug in the command-line argument parsing has been fixed, ensuring that top-level flags are correctly handled.
- A loop has been refactored for better readability.
I have one minor suggestion for improving consistency in the test setup.
There was a problem hiding this comment.
Pull request overview
Refactors the workspace to move the jetdbg CLI into its own crate and centralizes shared dependency/version metadata at the workspace level to reduce duplication across member crates.
Changes:
- Added a new
crates/jetdbgcrate (with its ownCargo.toml) and updatedjetdbgCLI logic/comments. - Introduced
[workspace.package]and[workspace.dependencies]in the rootCargo.toml, and updated member crates to use*.workspace = true. - Removed
jetdbgbin configuration fromcrates/jetand updated the lockfile accordingly.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/jetdbg/src/main.rs | Updates the example CALL ROM to use a 20-byte address push and forwards top-level CLI flags when no subcommand is provided. |
| crates/jetdbg/Cargo.toml | Defines the new standalone jetdbg crate and its workspace-managed dependencies. |
| crates/jet_runtime/Cargo.toml | Migrates package metadata + dependencies to workspace-managed entries. |
| crates/jet_push_macros/Cargo.toml | Migrates proc-macro dependencies to workspace-managed entries. |
| crates/jet_ir/Cargo.toml | Migrates metadata and inkwell dependency to workspace-managed entries. |
| crates/jet/Cargo.toml | Removes the jetdbg bin stanza and migrates deps/dev-deps to workspace-managed entries. |
| Cargo.toml | Adds crates/jetdbg to workspace members and centralizes shared package metadata/deps. |
| Cargo.lock | Reflects the new jetdbg package and dependency graph updates. |
Comments suppressed due to low confidence (3)
crates/jetdbg/src/main.rs:95
- The comment suggests
MSTOREonly sets a single byte (Mem[0x01] = 0xFF), butMSTOREwrites a full 32-byte word starting at the offset (in this codebase’s little-endian word representation, the first byte happens to be 0xFF here). Consider clarifying that it stores a 32-byte word to avoid misleading readers.
crates/jetdbg/src/main.rs:100 - Same as above: this
MSTOREwrites a 32-byte word starting at 0x0A; the comment currently reads like a single-byte store. Clarifying the 32-byte store (and endian convention if relevant) would make this example easier to reason about.
crates/jetdbg/src/main.rs:105 RETURNuses (offset, length); the commentmem[0x00..0x0A]can be read as inclusive of 0x0A. Consider wording it as “10 bytes starting at 0x00” (or “mem[0x00..0x0A)”) to match the length semantics.
No description provided.