Allow Voluntary Exit To Write-to-File Feature and Done a manual node Test. #8517
Conversation
Summary of ChangesHello @edna-harriet, 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 introduces a significant enhancement to the validator's voluntary exit command, allowing users to generate and save signed exit messages to a local file for deferred or custom submission workflows. This provides greater flexibility and control over the exit process, moving away from immediate network publication as the sole option. The change is supported by a new manual integration test to ensure its functionality. Highlights
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 introduces a useful --saveToFile feature for the voluntary-exit command, allowing for offline signing workflows. The core logic changes in voluntaryExit.ts are well-structured, separating the signing from the submission process.
My main feedback is regarding the manual test, which currently doesn't test the actual implementation and should be corrected to provide meaningful verification. I've also pointed out some areas for code cleanup and added a suggestion to make the new feature more robust against incorrect usage.
Great job on the contribution and for seeking feedback on testing patterns! Once the manual test is updated to call the real logic, it will be a solid demonstration of the feature.
| const voluntaryExitHandler = async (args) => { | ||
| console.log("⚙️ Mock voluntary exit handler running with args:", args); | ||
| if (args.saveToFile) { | ||
| fs.writeFileSync(args.saveToFile, JSON.stringify({ message: "Mock voluntary exit data" }, null, 2)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This manual test script uses a mock implementation of voluntaryExitHandler that doesn't execute the actual logic from packages/cli/src/cmds/validator/voluntaryExit.ts. As a result, the test only verifies that fs.writeFileSync can write a hardcoded object to a file, but it doesn't test the new --saveToFile feature's core logic, such as argument parsing, validator signing, and collecting the signed exits.
To make this test useful, it should import and execute the actual command handler from voluntaryExit.ts. This will likely require exporting the handler logic from that file so it can be imported here. It seems you were on the right track with the runVoluntaryExitHandler function you added.
| // export async function handler(args: any) { | ||
| // // ... does voluntary exit work | ||
| // } | ||
|
|
||
| export async function runVoluntaryExitHandler(args: any) { | ||
| // ...logic... | ||
| } |
There was a problem hiding this comment.
This file contains commented-out code and an empty exported function runVoluntaryExitHandler. This appears to be leftover scaffolding from development or testing attempts and should be removed to keep the codebase clean.
If the intention is to make the handler testable (which is a great idea!), the logic from the handler property should be moved into an exported function, which can then be called by both the CLI command definition and your test script.
| } catch (err: any) { | ||
| if (err && err.message && err.message.includes("ALREADY_EXISTS")) { | ||
| alreadySubmitted.push(v); | ||
| } else { | ||
| console.log( | ||
| `Voluntary exit errored for ${v.pubkey} (${v.index}) ${i + 1}/${signersToExit.length}: ${err && err.message ? err.message : err}` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Using any for caught errors is generally discouraged as it bypasses type safety. It's safer to catch errors as unknown and then perform type checks (like instanceof Error) before accessing properties like message. This prevents potential runtime errors if the thrown value is not an Error object.
} catch (err) {
if (err instanceof Error && err.message.includes("ALREADY_EXISTS")) {
alreadySubmitted.push(v);
} else {
const message = err instanceof Error ? err.message : String(err);
console.log(
`Voluntary exit errored for ${v.pubkey} (${v.index}) ${i + 1}/${signersToExit.length}: ${message}`
);
}| if (args.saveToFile && signedExits.length > 0) { | ||
| // Write all signed voluntary exits to the specified file as a JSON array | ||
| const {writeFile} = await import("../../util/file.js"); | ||
| writeFile(args.saveToFile, signedExits); | ||
| console.log(`Saved ${signedExits.length} signed voluntary exit(s) to file: ${args.saveToFile}`); | ||
| } |
There was a problem hiding this comment.
The writeFile utility determines the serialization format from the file extension. If a user provides a path for --saveToFile that does not end in .json (e.g., my-exits), the file will contain [object Object] instead of a valid JSON array. This is because the underlying utility will not know to serialize the data as JSON.
To make this feature more robust, you should validate that the saveToFile path ends with .json. You could add a check at the beginning of the handler function, for example:
if (args.saveToFile && !args.saveToFile.endsWith(".json")) {
throw new YargsError("The --saveToFile path must end with .json");
}|
|
||
| const validators = (await client.beacon.postStateValidators({stateId: "head", validatorIds: pubkeys})).value(); | ||
|
|
||
| // const dataByPubkey = new Map(validators.map((item: { validator: { pubkey: any; }; }) => [toPubkeyHex(item.validator.pubkey), item])); |
There was a problem hiding this comment.
This commented-out line appears to be leftover from development and should be removed.
edna-harriet
changed the title
We have existing tests for voluntary exit command, please take a look at those
We don't use Jest for testing, we use Vitest and there isn't anything you need to configure yourself, it works our of the box. Please take a look at CONTRIBUTING.md and make sure to run |
2 participants
Summary
This PR adds a
--saveToFileJSON file option to the validatorvoluntary-exitcommand for later submission workflows. It does not submit to the beacon node.Changes Made
— manual script that: -runs the handler withsaveToFile`-verifies the file is created and readable.
-submits to a mock beacon client rather than publish to network.
How to run the node test manually:
From repo root, run:
-$yarn install
-$yarn build
-$yarn workspace @chainsafe/lodestar node packages/cli/manual-tests/voluntaryExit.writefile.test.js
## Expected Output:
✅ File created successfully: .../manual-tests/exit-test.json
📄 File content: { message: 'Mock voluntary exit data' }
📨 Mock beacon received voluntary exit: { message: 'Mock voluntary exit data' }
✅ Voluntary exit successfully submitted to mock beacon client.
🧹 Cleaned up test file.
Why manual test included:
While authoring an automated Jest test I ran into some repository-specific Jest/ESM config complexity. The manual test is included to provide clear, reproducible proof of functionality . It also aims to:
(a) let reviewers run the feature instantly.
(b) improve it to adapt to the repo's CI.
Assistance needed:
I would appreciate guidance on:
To Do.
Additional Notes
I’m learning to contribute to Ethereum core tooling via Lodestar, and this PR forms part of my contribution proof and learning process.
Thank you for reviewing, I look forward to reading your feedbacks.