-
-
Notifications
You must be signed in to change notification settings - Fork 410
feat: write voluntary exit to disk instead of publishing #8585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
Conversation
…le but file content incorrect
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 new capability for the 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to add an offline mode for the voluntary-exit command by saving the exit message to a file instead of broadcasting it. This is a great feature for improving operational security.
As you noted in the description, the new E2E test is currently failing. My review focuses on helping you get the test into a solid state so you can confidently build out the feature. The main issue is that the voluntary-exit command's implementation has not yet been modified to work without a network connection, causing the test command to fail. I've provided a detailed suggestion to restructure the test file to correctly capture failures and assert the intended behavior.
I've also pointed out a few minor cleanup items:
- A large block of commented-out code in
configs/vitest.config.e2e.tsshould be removed. - Some extra newlines in
packages/cli/src/cmds/validator/voluntaryExit.tscan be cleaned up. - The temporary file
tmp-dev-voluntary-exit/voluntary_exit.jsonhas been committed to the repository. This and other temporary directories (tmp-*) should be added to your.gitignorefile to prevent this from happening in the future.
Once the test is solid and the command implementation is updated to work offline, this will be a strong contribution. Let me know if you have any questions about the feedback!
| describe("voluntaryExit saveToFile-noNetwork cmd", () => { | ||
| vi.setConfig({testTimeout: 30_000}); | ||
|
|
||
| it(" creates and ensures voluntaryExit command has been savedToFile", async () => { | ||
| // Define temporary directory for the test | ||
|
|
||
| const tmpDir = path.join(process.cwd(), "tmp-dev-voluntary-exit"); | ||
| const cliPath = path.resolve(process.cwd(), "packages/cli/bin/lodestar.js"); | ||
|
|
||
| const saveToFile = path.join(tmpDir, "voluntary_exit.json"); | ||
|
|
||
| const cmd = `node ${cliPath} validator voluntary-exit \ | ||
| --network=dev \ | ||
| --yes \ | ||
| --saveToFile=${saveToFile} \ | ||
| --interopIndexes=0..1 \ | ||
| --dataDir=${tmpDir}`; | ||
| console.log("Running command:", cmd); | ||
|
|
||
| try { | ||
| execSync(cmd, {stdio: "inherit"}); | ||
| } catch (_err: any) { | ||
| console.error("CLI command failed:", _err.message); | ||
| } | ||
|
|
||
| const files = fs.readdirSync(tmpDir); | ||
| console.log("Files in directory:", files); | ||
|
|
||
| const exitFiles = files.filter((f) => f.startsWith("voluntary_exit") && f.endsWith(".json")); | ||
| expect(exitFiles.length).toBeGreaterThan(-1); | ||
|
|
||
|
|
||
| console.log(`✅ Found voluntary exit file(s): ${exitFiles.join(", ")}`); | ||
| const data = fs.readFileSync(path.join(tmpDir, exitFiles[0]), "utf-8"); | ||
| console.log("Voluntary exit file content:\n", data); | ||
| }); | ||
|
|
||
| // TEST 2: No network publication. | ||
|
|
||
| it("voluntaryExit command should NOT publish to Ethereum network", async () => { | ||
| // check on environment/network calls | ||
| const mockEnv = vi.spyOn(process, "env", "get").mockReturnValue({ | ||
| ...process.env, | ||
| ETH_RPC_URL: "", // ensure no RPC URL defined | ||
| }); | ||
|
|
||
| let publishedToNetwork = false; | ||
| const mockExec = vi.fn(async () => { | ||
| console.log("Simulating CLI run with no network calls"); | ||
|
|
||
| try { | ||
| // Replace with your actual CLI command | ||
| const cliPath = path.resolve(process.cwd(), "packages/cli/bin/lodestar.js"); | ||
| execSync(`node ${cliPath} validator voluntary-exit --network=dev --yes`, { | ||
| stdio: "inherit", | ||
| }); | ||
|
|
||
| publishedToNetwork = false; // keep your simulation | ||
| } catch (err) { | ||
| console.error("CLI execution failed during mock:", err); | ||
| } | ||
|
|
||
| return; | ||
| }); | ||
|
|
||
| try { | ||
| await mockExec(); // simulate execCliCommand | ||
| } catch {} | ||
|
|
||
| // Assert: no network calls were made | ||
| expect(publishedToNetwork).toBe(false); | ||
| console.log("✅ Confirmed: no data published to Ethereum network"); | ||
|
|
||
| // Restore environment | ||
| mockEnv.mockRestore(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding these tests and for asking for help. The current tests have a few issues that prevent them from correctly verifying the feature and catching errors. Here's a breakdown and a suggested revision:
Current Issues:
- Error Swallowing: The
try...catchblock aroundexecSyncin the first test prevents the test from failing when the CLI command errors out. This is why you're seeing theCLI executed with errormessage in the output file but the test still passes. - Incorrect Assertion:
expect(exitFiles.length).toBeGreaterThan(-1)will always be true, even if no files are found (since0 > -1). It should betoBe(1). - Ineffective Second Test: The second test for "no network publication" doesn't actually test for network activity. The
publishedToNetworkflag is never changed, so the assertion always passes.
To properly test this feature, we can combine both intents into a single, more robust test. The key is to run the command without providing a beacon node URL and assert that it succeeds and creates the correct file.
The reason your command is failing is that the voluntary-exit command implementation still requires a beacon node connection to fetch genesis information, even when --saveToFile is used. The test below is structured to correctly test the intended behavior. It will fail until the command's logic is updated to support this offline functionality.
Here is a revised version of the test suite that you can use as a starting point. It addresses the issues above and provides a clear success condition for you to build the feature against.
describe("voluntaryExit saveToFile-noNetwork cmd", () => {
vi.setConfig({testTimeout: 30_000});
const tmpDir = path.join(process.cwd(), "tmp-dev-voluntary-exit");
const cliPath = path.resolve(process.cwd(), "packages/cli/bin/lodestar.js");
const saveToFile = path.join(tmpDir, "voluntary_exit.json");
beforeEach(() => {
// Ensure the temp directory is clean before each test
fs.rmSync(tmpDir, {recursive: true, force: true});
fs.mkdirSync(tmpDir, {recursive: true});
});
afterAll(() => {
// Clean up after all tests
fs.rmSync(tmpDir, {recursive: true, force: true});
});
it("should create a voluntary exit file without a network connection", () => {
// Note: --beaconNodes is intentionally omitted to test offline functionality.
// The command will fail until the implementation is updated to not require a network connection.
const cmd = `node ${cliPath} validator voluntary-exit \
--network=dev \
--yes \
--saveToFile=${saveToFile} \
--interopIndexes=0 \
--dataDir=${tmpDir}`;
console.log("Running command:", cmd);
// By not wrapping in try/catch, the test will correctly fail if the command returns a non-zero exit code.
execSync(cmd, {stdio: "inherit"});
// Verify the file was created
expect(fs.existsSync(saveToFile)).toBe(true);
// Verify the file content
const fileContent = fs.readFileSync(saveToFile, "utf-8");
const exitJson = JSON.parse(fileContent);
// Example assertions. Adjust based on the expected structure.
expect(exitJson.message).toBeDefined();
expect(exitJson.message.epoch).toBeGreaterThan(0);
expect(exitJson.message.validatorIndex).toBe("0");
expect(exitJson.signature).toMatch(/^0x[a-f0-9]{192}$/);
console.log("Voluntary exit file content:\n", fileContent);
});
});| // import path from "node:path"; | ||
| // import {defineProject} from "vitest/config"; | ||
| // export const e2eMinimalProject = defineProject({ | ||
| // test: { | ||
| // // Preferable over `e2e-mainnet` to speed up tests, only use `mainnet` preset in e2e tests | ||
| // // if absolutely required for interop testing, eg. in case of web3signer we need to use | ||
| // // `mainnet` preset to allow testing across multiple forks and ensure mainnet compatibility | ||
| // name: "e2e", | ||
| // include: ["**/test/e2e/**/*.test.ts"], | ||
| // setupFiles: [ | ||
| // path.join(__dirname, "../scripts/vitest/setupFiles/customMatchers.ts"), | ||
| // path.join(__dirname, "../scripts/vitest/setupFiles/dotenv.ts"), | ||
| // path.join(__dirname, "../scripts/vitest/setupFiles/lodestarPreset.ts"), | ||
| // ], | ||
| // env: { | ||
| // LODESTAR_PRESET: "minimal", | ||
| // }, | ||
| // pool: "forks", | ||
| // poolOptions: { | ||
| // forks: { | ||
| // singleFork: true, | ||
| // }, | ||
| // }, | ||
| // sequence: { | ||
| // concurrent: false, | ||
| // shuffle: false, | ||
| // }, | ||
| // }, | ||
| // }); | ||
|
|
||
| // export const e2eMainnetProject = defineProject({ | ||
| // test: { | ||
| // // Currently only `e2e` tests for the `validator` package runs with the `mainnet` preset. | ||
| // name: "e2e-mainnet", | ||
| // include: ["**/test/e2e-mainnet/**/*.test.ts"], | ||
| // setupFiles: [ | ||
| // path.join(__dirname, "../scripts/vitest/setupFiles/customMatchers.ts"), | ||
| // path.join(__dirname, "../scripts/vitest/setupFiles/dotenv.ts"), | ||
| // path.join(__dirname, "../scripts/vitest/setupFiles/lodestarPreset.ts"), | ||
| // ], | ||
| // env: { | ||
| // LODESTAR_PRESET: "mainnet", | ||
| // }, | ||
| // pool: "forks", | ||
| // poolOptions: { | ||
| // forks: { | ||
| // singleFork: true, | ||
| // }, | ||
| // }, | ||
| // sequence: { | ||
| // concurrent: false, | ||
| // shuffle: false, | ||
| // }, | ||
| // }, | ||
| // }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@nflaig , I created a new PR following your recommendation that I work on clean branch . I would like to hear your feedback on this. |
a05e72f to
4e90de8
Compare
…taryExit.ts file and ran testscript with 2 test passes
Implements #6632
Motivation
This PR allows voluntary exit command to create tmp-dev-voluntary-exit directory in the root directory and then saves a file called voluntaryExit.json to store content without making network calls so that no data is published to the network.
Steps to test or reproduce
$ cd lodestar
$ git checkout harriet/issue-6632
$ yarn vitest run packages/cli/test/e2e/voluntaryExit.saveToFile-noNetwork.test.ts --config configs/vitest.config.e2e.ts
Expectected Results
voluntaryExit.json file is created inside tmp-dev-voluntary-exit directory in the root directory. The file's content are:
An array of validator exit messages, message: epoch and validatorIndex and signature.
Output console prints out 2 test passes: the correct content for the created file and also confirm that no data was published to the network.
Additional notes
I look forward to receiving your feedback on this issue.