feat:impl tools-cli and adding enrgen command#132
Conversation
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new CLI tool called zeam-tools with an enrgen command for generating Ethereum Node Records (ENR). The implementation separates this functionality from the main zeam CLI as previously discussed.
- Adds a new
zeam-toolsCLI executable with structured argument parsing - Implements
enrgensubcommand to generate ENR from secret key, IP address, and UDP port - Includes comprehensive error handling and test coverage for ENR generation functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkgs/tools-cli/src/main.zig | Main implementation of the tools CLI with ENR generation functionality |
| build.zig.zon | Adds zig-enr dependency for ENR generation capabilities |
| build.zig | Configures build for the new tools CLI executable and its tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
pkgs/tools-cli/src/main.zig
Outdated
| return error.ENRSetIPFailed; | ||
| }; | ||
|
|
||
| var udp_bytes: [2]u8 = undefined; |
There was a problem hiding this comment.
The choice of big-endian byte order for the UDP port should be documented, as this is a specific requirement for ENR encoding that may not be obvious to future maintainers.
| var udp_bytes: [2]u8 = undefined; | |
| var udp_bytes: [2]u8 = undefined; | |
| // ENR specification requires the UDP port to be encoded in big-endian byte order. |
pkgs/tools-cli/src/main.zig
Outdated
| const ENRGenCmd = struct { | ||
| sk: []const u8, | ||
| ip: []const u8, | ||
| udp: u16, |
There was a problem hiding this comment.
i think instead of udp field we have quic field, see https://github.com/leanEthereum/leanSpec/blob/main/docs/client/networking.md
There was a problem hiding this comment.
Oh, it seems the spec is not good here, it doesn't need a quic field. there is already a standard udp field there. Combine secp256k1, ip, udp, it can generate a ipv4 multiaddr. This is already used in beacon chain, i think we need change the spec here.
There was a problem hiding this comment.
ok, I saw here https://github.com/sigp/lighthouse/blob/stable/beacon_node/lighthouse_network/src/discovery/enr_ext.rs#L9, let me add quic field.
There was a problem hiding this comment.
There will presumably be more than one tool, so it should have another directory, e.g. pkgs/tools/enrgen/src/main.zig. I don't think we need -cli, tools are typically CLIs.
There was a problem hiding this comment.
Current tools CLI design assume that multiple tools will be sub commands, So that we have only one CLI binary. Suggested dir seems not match this assumption. What is your opinion?
There was a problem hiding this comment.
my opinion is that, unless I'm mistaken, this is not part of the main client. If it is, then this needs to be a subcommand and if that is the case, it needs to be in pkgs/cli and not aside.
There was a problem hiding this comment.
I agree this is a new CLI, but I don't think we should have a directory like pkgs/tools/enrgen/src/main.zig. What I thought is pkgs/tools/src/main.zig, the enrgen is a subcommand of this CLI. When we add new tools, we don't need add new main.zig and just add another subcommand in pkgs/tools/src/main.zig.
There was a problem hiding this comment.
i think for now we can go for all cli tools in in pkgs/tools with zeam-tools exe target as per this PR, if we feel that we need to change this to something better/more convenient, we can figure it out later
gballet
left a comment
There was a problem hiding this comment.
Looks like you need to add a dependency for this to compile in docker. My suggestion is to not build this executable in docker, unless it's necessary for daily operations. If you disagree, fine, just add the dependency and call that build-all thing that I suggested.
Yes, I prefer to not build executable in docker, let me look how to make a separate build for this. |
| else \ | ||
| GIT_VERSION=$(echo "$GIT_VERSION" | head -c 7); \ | ||
| fi && \ | ||
| zig build -Doptimize=ReleaseFast -Dgit_version="$GIT_VERSION" |
There was a problem hiding this comment.
@gballet Currently, I used zig build all to tested on CI to see if it works on 0.14.1. It is really works https://github.com/blockblaz/zeam/actions/runs/17393132752. I'll change it to zig build to avoid including the zeam-tools in docker image.
|
actually we do need this in docker because all the tools to be used by genesis genertor need to be in docker, although we can build a separate docker image for this |
gballet
left a comment
There was a problem hiding this comment.
Left a few more comments. I think the tool part is ready, but the build needs to have some changes.
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
tools bin in the Docker build Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Motivation
Currently we need a CLI command to generate ENR. Since discussed before, it need a new CLI which separates from
zeamCLI. This PR introduces a new CLI which namedzeam-toolsand adds a sub-commandenrgen.Implementation
USAGE: ./zig-out/bin/zeam-tools [OPTIONS] [COMMANDS] COMMANDS: enrgen Generate a new ENR (Ethereum Node Record) OPTIONS: -h, --help Show help information -v, --version Show version informationUSAGE: ./zig-out/bin/zeam-tools enrgen [OPTIONS] OPTIONS: -s, --sk STRING Secret key (hex string with or without 0x prefix)(required) -i, --ip STRING IPv4 address for the ENR(required) -u, --udp INTEGER UDP port for discovery(required) -o, --out STRING Output file path (prints to stdout if not specified)(default: null) -h, --help Show help information for the enrgen commandFix #130