Skip to content

Conversation

@GrapeBaBa
Copy link
Member

@GrapeBaBa GrapeBaBa commented Aug 26, 2025

Motivition

This pull requests add helper functions to save ENR to file and load ENR from file. It providers both single ENR operation and batch ENRs operation.

Implementation

  1. It re-expose ENR, EncodedENR and SignableENR from the dependency module. EncodedENR only should be used as readonly access.
  2. Single/Batch saving to file and loading from file functions for ENR, EncodedENR and SignableENR.

Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>

This comment was marked as outdated.

Signed-off-by: Chen Kai <281165273grape@gmail.com>
Copy link

@gballet gballet left a comment

Choose a reason for hiding this comment

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

in function names, please rename Enr to ENR, this is an acronym.

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>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
@GrapeBaBa GrapeBaBa requested review from Copilot and gballet August 27, 2025 05:10

This comment was marked as outdated.

Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
@GrapeBaBa GrapeBaBa requested a review from Copilot August 28, 2025 04:02
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements comprehensive file I/O functionality for Ethereum Node Records (ENR), enabling saving and loading of ENR data to/from disk. It provides both single and batch operations for different ENR types.

  • Adds helper functions for single and batch ENR file operations with support for custom delimiters
  • Re-exposes core ENR types (ENR, EncodedENR, SignableENR) from the dependency module for convenient access
  • Includes comprehensive test coverage with error handling scenarios and various ENR formats

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/root.zig Implements core ENR file I/O functions with comprehensive test suite
build.zig.zon Configures project dependencies and metadata
build.zig Sets up build configuration with dependency imports
README.md Documents project usage, building, and testing instructions
.github/workflows/ci.yml Adds CI pipeline for automated testing and formatting checks

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

pub fn loadEncodedENRFromDisk(encoded_enr: *EncodedENR, file_path: []const u8) !void {
var buffer: [enrlib.max_enr_size]u8 = undefined;
const content = try readFileContent(file_path, &buffer);
encoded_enr.* = try EncodedENR.decodeTxtInto(content);
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

The function name decodeTxtInto suggests it should decode into a provided parameter, but here it's returning a value. Consider using EncodedENR.decodeTxt(content) for consistency with the naming pattern, or update the function to match the ENR.decodeTxtInto pattern that takes a pointer parameter.

Suggested change
encoded_enr.* = try EncodedENR.decodeTxtInto(content);
encoded_enr.* = try EncodedENR.decodeTxt(content);

Copilot uses AI. Check for mistakes.
const trimmed = std.mem.trim(u8, enr_txt, " \t\r\n");
if (trimmed.len == 0) continue; // Skip empty entries

const encoded_enr = try EncodedENR.decodeTxtInto(trimmed);
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

Same inconsistency as above - decodeTxtInto is returning a value rather than decoding into a provided parameter. This inconsistent API usage could confuse developers.

Copilot uses AI. Check for mistakes.
try readTempEncodedEnr(&tmp_dir, test_file, &loaded_encoded_enr);

var enr: ENR = undefined;
loaded_encoded_enr.decodeIntoENR(&enr);
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

The function call decodeIntoENR appears to not handle potential errors with try. If this function can fail, it should be called with try loaded_encoded_enr.decodeIntoENR(&enr);

Suggested change
loaded_encoded_enr.decodeIntoENR(&enr);
try loaded_encoded_enr.decodeIntoENR(&enr);

Copilot uses AI. Check for mistakes.
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>

.dependencies = .{
.secp256k1 = .{
.url = "git+https://github.com/zig-bitcoin/libsecp256k1-zig#b96922b375a601f3303bf43be1dceb3d101df6c6",
Copy link
Member

Choose a reason for hiding this comment

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

does this solve the signature mismatch issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, right now it can generate same signature with other implementations

Copy link
Member

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm

@g11tech g11tech merged commit 5a351cf into main Aug 31, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants