fix: this PR fixes Issue #6632 #8571
Conversation
…published to the network
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 enhances 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
|
![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 valuable --saveToFile feature for the voluntary-exit command, and the core logic changes in voluntaryExit.ts represent a good refactoring that separates signing from submission. However, the pull request also contains several significant issues that need to be addressed. Most critically, it includes large, conflicting, and incomplete changes to the project's testing and linting configurations, which are likely to break the development and CI environment. These configuration changes should be reverted and handled in a separate PR. Additionally, the new e2e tests have some flaws, and several test artifacts and temporary files have been committed to the repository that should be removed.
| "scripts": { | ||
| "clean": "rm -rf lib && rm -f *.tsbuildinfo", | ||
| "build": "tsc -p tsconfig.build.json && yarn write-git-data", | ||
| "build:release": "yarn clean && yarn run build", | ||
| "build:watch": "tsc -p tsconfig.build.json --watch", | ||
| "write-git-data": "node lib/util/gitData/writeGitData.js", | ||
| "check-build": "node -e \"(async function() { await import('./lib/index.js') })()\" lodestar --help", | ||
| "check-types": "tsc", | ||
| "docs:build": "node --loader ts-node/esm ./docsgen/index.ts", | ||
| "lint": "biome check src/ test/", | ||
| "lint:fix": "yarn run lint --write", | ||
| "test:unit": "vitest run --project unit --project unit-minimal", | ||
| "test:e2e": "vitest run --project e2e --project e2e-mainnet", | ||
| "test:sim:multifork": "LODESTAR_PRESET=minimal DOTENV_CONFIG_PATH=../../.env.test node -r dotenv/config --loader ts-node/esm test/sim/multiFork.test.ts", | ||
| "test:sim:mixedclient": "LODESTAR_PRESET=minimal DOTENV_CONFIG_PATH=../../.env.test node -r dotenv/config --loader ts-node/esm test/sim/mixedClient.test.ts", | ||
| "test:sim:endpoints": "LODESTAR_PRESET=minimal DOTENV_CONFIG_PATH=../../.env.test node -r dotenv/config --loader ts-node/esm test/sim/endpoints.test.ts", | ||
| "test:sim:deneb": "LODESTAR_PRESET=minimal DOTENV_CONFIG_PATH=../../.env.test node -r dotenv/config --loader ts-node/esm test/sim/deneb.test.ts", | ||
| "test:sim:backup_eth_provider": "LODESTAR_PRESET=minimal DOTENV_CONFIG_PATH=../../.env.test node -r dotenv/config --loader ts-node/esm test/sim/backupEthProvider.test.ts", | ||
| "test": "yarn test:unit && yarn test:e2e", | ||
| "check-readme": "typescript-docs-verifier" | ||
| "test": "jest --config ./jest.config.mjs" | ||
| }, |
There was a problem hiding this comment.
The testing setup has been changed from vitest to jest by modifying the scripts. However, new e2e tests in this PR are written using vitest (packages/cli/test/e2e/voluntaryExit.saveToFile-noNetwork.test.ts). This creates a conflict where the new tests cannot be run with the updated test script. Furthermore, this PR introduces eslint configuration while the repository seems to use biome for linting. Such fundamental changes to the development environment should be discussed and handled in a separate, dedicated PR to avoid breaking the existing CI/CD pipeline and local development workflows. Please revert the changes related to the testing and linting configuration in all package.json, tsconfig.json, and new eslint.config.* files.
| 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.
This test case, which is intended to verify that no data is published to the network, is flawed.
- The
publishedToNetworkvariable is initialized tofalseand never modified, soexpect(publishedToNetwork).toBe(false)will always pass. - The
mockExecfunction calls the actualexecSyncfor a command that should attempt to publish to the network (as--saveToFileis not used). The test doesn't actually prevent or spy on network calls.
A better approach would be to run the command with --saveToFile and ensure no network requests are made, or run it without --saveToFile against a mock beacon node endpoint that would fail the test if it receives a request. The current implementation does not achieve its goal.
| # Pull Request:Allow Voluntary Exit To Write-to-File Feature and Prepare a manual node Test. | ||
|
|
||
| ## Summary | ||
| This PR adds a `--saveToFile` JSON file option to the validator `voluntary-exit` command for later submission workflows. It does not submit to the beacon node. | ||
|
|
||
|
|
||
| ## Changes Made | ||
| 1. packages/cli/src/cmds/validator/voluntaryExit.ts` — implement saveToFile behavior | ||
| 2. packages/cli/manual-tests/voluntaryExit.writefile.test.js` — manual script that: | ||
| -runs the handler with `saveToFile` | ||
| -verifies the file is created and readable. | ||
| -submits to a mock beacon client rather than publish to network. | ||
|
|
||
| > **Note:** The manual test demonstrates the behavior and was run locally.I kept the manual script so reviewers can quickly run and validate.I'm working on Jest/CI setup. I’d appreciate guidance or help from maintainers about the preferred location/approach for the automated test in repo CI. | ||
| ## 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: | ||
| - Correct integration testing pattern for Lodestar CLI commands.(I am happy to follow maintainers' suggestions to convert into an automated test). | ||
| - Ensuring that my local Jest configuration aligns with Lodestar’s monorepo test standards. | ||
|
|
||
| ## To Do. | ||
| - Convert manual test to an automated Jest integration test in packages/cli/test/validator. | ||
| - Add any required test harness / mocks used by repo CI. | ||
|
|
||
| ## 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. | ||
|
|
There was a problem hiding this comment.
This file appears to contain content that should have been in the pull request description. It doesn't seem to be part of the project's documentation and should be removed from the repository.
| [ | ||
| { | ||
| "message": { | ||
| "epoch": 2, | ||
| "validatorIndex": 0 | ||
| }, | ||
| "signature": { | ||
| "0": 166, | ||
| "1": 226, | ||
| "2": 53, | ||
| "3": 185, | ||
| "4": 29, | ||
| "5": 121, | ||
| "6": 138, | ||
| "7": 61, | ||
| "8": 109, | ||
| "9": 227, | ||
| "10": 218, | ||
| "11": 199, | ||
| "12": 22, | ||
| "13": 145, | ||
| "14": 183, | ||
| "15": 143, | ||
| "16": 222, | ||
| "17": 16, | ||
| "18": 189, | ||
| "19": 146, | ||
| "20": 75, | ||
| "21": 116, | ||
| "22": 43, | ||
| "23": 60, | ||
| "24": 235, | ||
| "25": 188, | ||
| "26": 245, | ||
| "27": 103, | ||
| "28": 195, | ||
| "29": 243, | ||
| "30": 107, | ||
| "31": 32, | ||
| "32": 79, | ||
| "33": 68, | ||
| "34": 200, | ||
| "35": 133, | ||
| "36": 115, | ||
| "37": 71, | ||
| "38": 107, | ||
| "39": 63, | ||
| "40": 138, | ||
| "41": 49, | ||
| "42": 155, | ||
| "43": 77, | ||
| "44": 239, | ||
| "45": 126, | ||
| "46": 16, | ||
| "47": 223, | ||
| "48": 9, | ||
| "49": 50, | ||
| "50": 75, | ||
| "51": 122, | ||
| "52": 117, | ||
| "53": 97, | ||
| "54": 233, | ||
| "55": 84, | ||
| "56": 44, | ||
| "57": 101, | ||
| "58": 251, | ||
| "59": 31, | ||
| "60": 160, | ||
| "61": 138, | ||
| "62": 139, | ||
| "63": 213, | ||
| "64": 202, | ||
| "65": 103, | ||
| "66": 216, | ||
| "67": 21, | ||
| "68": 233, | ||
| "69": 205, | ||
| "70": 122, | ||
| "71": 157, | ||
| "72": 145, | ||
| "73": 199, | ||
| "74": 99, | ||
| "75": 172, | ||
| "76": 249, | ||
| "77": 45, | ||
| "78": 205, | ||
| "79": 140, | ||
| "80": 45, | ||
| "81": 62, | ||
| "82": 248, | ||
| "83": 203, | ||
| "84": 79, | ||
| "85": 162, | ||
| "86": 212, | ||
| "87": 167, | ||
| "88": 166, | ||
| "89": 130, | ||
| "90": 188, | ||
| "91": 133, | ||
| "92": 248, | ||
| "93": 243, | ||
| "94": 172, | ||
| "95": 154 | ||
| } | ||
| }, | ||
| { | ||
| "message": { | ||
| "epoch": 2, | ||
| "validatorIndex": 1 | ||
| }, | ||
| "signature": { | ||
| "0": 130, | ||
| "1": 125, | ||
| "2": 222, | ||
| "3": 60, | ||
| "4": 88, | ||
| "5": 46, | ||
| "6": 123, | ||
| "7": 74, | ||
| "8": 198, | ||
| "9": 139, | ||
| "10": 183, | ||
| "11": 247, | ||
| "12": 163, | ||
| "13": 22, | ||
| "14": 246, | ||
| "15": 45, | ||
| "16": 124, | ||
| "17": 108, | ||
| "18": 253, | ||
| "19": 211, | ||
| "20": 153, | ||
| "21": 185, | ||
| "22": 106, | ||
| "23": 202, | ||
| "24": 230, | ||
| "25": 187, | ||
| "26": 187, | ||
| "27": 3, | ||
| "28": 105, | ||
| "29": 40, | ||
| "30": 153, | ||
| "31": 74, | ||
| "32": 205, | ||
| "33": 50, | ||
| "34": 244, | ||
| "35": 230, | ||
| "36": 94, | ||
| "37": 138, | ||
| "38": 192, | ||
| "39": 151, | ||
| "40": 69, | ||
| "41": 152, | ||
| "42": 142, | ||
| "43": 130, | ||
| "44": 189, | ||
| "45": 235, | ||
| "46": 44, | ||
| "47": 122, | ||
| "48": 5, | ||
| "49": 195, | ||
| "50": 138, | ||
| "51": 234, | ||
| "52": 210, | ||
| "53": 229, | ||
| "54": 84, | ||
| "55": 87, | ||
| "56": 238, | ||
| "57": 141, | ||
| "58": 233, | ||
| "59": 193, | ||
| "60": 213, | ||
| "61": 27, | ||
| "62": 163, | ||
| "63": 66, | ||
| "64": 65, | ||
| "65": 52, | ||
| "66": 163, | ||
| "67": 15, | ||
| "68": 198, | ||
| "69": 151, | ||
| "70": 121, | ||
| "71": 20, | ||
| "72": 215, | ||
| "73": 36, | ||
| "74": 255, | ||
| "75": 97, | ||
| "76": 226, | ||
| "77": 161, | ||
| "78": 222, | ||
| "79": 85, | ||
| "80": 98, | ||
| "81": 238, | ||
| "82": 137, | ||
| "83": 92, | ||
| "84": 242, | ||
| "85": 77, | ||
| "86": 53, | ||
| "87": 247, | ||
| "88": 35, | ||
| "89": 173, | ||
| "90": 210, | ||
| "91": 4, | ||
| "92": 228, | ||
| "93": 73, | ||
| "94": 94, | ||
| "95": 189 | ||
| } | ||
| } | ||
| ] No newline at end of file |
There was a problem hiding this comment.
This file seems to be an example output or a test artifact. Such files should typically not be committed to the source repository. Please consider adding exit.json and other similar generated files to .gitignore.
| import inquirer from "inquirer"; | ||
| import {getBeaconConfigFromArgs} from "../../config/index.js"; | ||
| import {GlobalArgs} from "../../options/index.js"; | ||
| import {YargsError, ensure0xPrefix, wrapError} from "../../util/index.js"; |
There was a problem hiding this comment.
wrapError is imported from ../../util/index.js but is no longer used in this file after refactoring to use try/catch blocks. Please remove this unused import.
| import {YargsError, ensure0xPrefix, wrapError} from "../../util/index.js"; | |
| import {YargsError, ensure0xPrefix} from "../../util/index.js"; |
|
|
||
| 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 of code should be removed.
| // 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 block of commented-out code seems to be leftover from development and should be removed before merging.
| try { | ||
| execSync(cmd, {stdio: "inherit"}); | ||
| } catch (_err: any) { | ||
| console.error("CLI command failed:", _err.message); | ||
| } |
There was a problem hiding this comment.
The try/catch block around execSync catches the error and logs it, but allows the test to continue. If the CLI command fails, the test should fail immediately. It's better to let execSync throw an unhandled exception to fail the test.
execSync(cmd, {stdio: "inherit"});| console.log("Files in directory:", files); | ||
|
|
||
| const exitFiles = files.filter((f) => f.startsWith("voluntary_exit") && f.endsWith(".json")); | ||
| expect(exitFiles.length).toBeGreaterThan(-1); |
There was a problem hiding this comment.
The assertion expect(exitFiles.length).toBeGreaterThan(-1) is functionally correct but a bit unconventional. Using toBeGreaterThan(0) would be more explicit about the intent of the test.
expect(exitFiles.length).toBeGreaterThan(0);| [ | ||
| { | ||
| "message": { | ||
| "epoch": 3, | ||
| "validatorIndex": 0 | ||
| }, | ||
| "signature": { | ||
| "0": 170, | ||
| "1": 49, | ||
| "2": 74, | ||
| "3": 82, | ||
| "4": 22, | ||
| "5": 54, | ||
| "6": 90, | ||
| "7": 25, | ||
| "8": 225, | ||
| "9": 23, | ||
| "10": 126, | ||
| "11": 77, | ||
| "12": 235, | ||
| "13": 217, | ||
| "14": 115, | ||
| "15": 223, | ||
| "16": 195, | ||
| "17": 249, | ||
| "18": 194, | ||
| "19": 103, | ||
| "20": 134, | ||
| "21": 239, | ||
| "22": 17, | ||
| "23": 237, | ||
| "24": 228, | ||
| "25": 10, | ||
| "26": 5, | ||
| "27": 17, | ||
| "28": 32, | ||
| "29": 94, | ||
| "30": 19, | ||
| "31": 205, | ||
| "32": 127, | ||
| "33": 131, | ||
| "34": 5, | ||
| "35": 57, | ||
| "36": 217, | ||
| "37": 94, | ||
| "38": 235, | ||
| "39": 104, | ||
| "40": 33, | ||
| "41": 125, | ||
| "42": 93, | ||
| "43": 217, | ||
| "44": 228, | ||
| "45": 12, | ||
| "46": 245, | ||
| "47": 1, | ||
| "48": 23, | ||
| "49": 239, | ||
| "50": 252, | ||
| "51": 191, | ||
| "52": 244, | ||
| "53": 66, | ||
| "54": 22, | ||
| "55": 80, | ||
| "56": 80, | ||
| "57": 85, | ||
| "58": 38, | ||
| "59": 77, | ||
| "60": 122, | ||
| "61": 243, | ||
| "62": 42, | ||
| "63": 163, | ||
| "64": 112, | ||
| "65": 170, | ||
| "66": 248, | ||
| "67": 19, | ||
| "68": 138, | ||
| "69": 101, | ||
| "70": 103, | ||
| "71": 18, | ||
| "72": 104, | ||
| "73": 4, | ||
| "74": 52, | ||
| "75": 74, | ||
| "76": 147, | ||
| "77": 185, | ||
| "78": 234, | ||
| "79": 153, | ||
| "80": 152, | ||
| "81": 155, | ||
| "82": 242, | ||
| "83": 152, | ||
| "84": 162, | ||
| "85": 251, | ||
| "86": 6, | ||
| "87": 229, | ||
| "88": 93, | ||
| "89": 41, | ||
| "90": 190, | ||
| "91": 54, | ||
| "92": 67, | ||
| "93": 158, | ||
| "94": 144, | ||
| "95": 118 | ||
| } | ||
| }, | ||
| { | ||
| "message": { | ||
| "epoch": 3, | ||
| "validatorIndex": 1 | ||
| }, | ||
| "signature": { | ||
| "0": 183, | ||
| "1": 242, | ||
| "2": 1, | ||
| "3": 74, | ||
| "4": 90, | ||
| "5": 58, | ||
| "6": 50, | ||
| "7": 248, | ||
| "8": 110, | ||
| "9": 245, | ||
| "10": 34, | ||
| "11": 37, | ||
| "12": 52, | ||
| "13": 43, | ||
| "14": 243, | ||
| "15": 228, | ||
| "16": 93, | ||
| "17": 39, | ||
| "18": 229, | ||
| "19": 42, | ||
| "20": 132, | ||
| "21": 243, | ||
| "22": 203, | ||
| "23": 243, | ||
| "24": 237, | ||
| "25": 169, | ||
| "26": 106, | ||
| "27": 93, | ||
| "28": 250, | ||
| "29": 219, | ||
| "30": 102, | ||
| "31": 214, | ||
| "32": 124, | ||
| "33": 254, | ||
| "34": 43, | ||
| "35": 196, | ||
| "36": 95, | ||
| "37": 201, | ||
| "38": 243, | ||
| "39": 160, | ||
| "40": 201, | ||
| "41": 224, | ||
| "42": 110, | ||
| "43": 195, | ||
| "44": 5, | ||
| "45": 230, | ||
| "46": 72, | ||
| "47": 246, | ||
| "48": 8, | ||
| "49": 63, | ||
| "50": 118, | ||
| "51": 63, | ||
| "52": 161, | ||
| "53": 116, | ||
| "54": 100, | ||
| "55": 131, | ||
| "56": 235, | ||
| "57": 12, | ||
| "58": 198, | ||
| "59": 67, | ||
| "60": 126, | ||
| "61": 57, | ||
| "62": 32, | ||
| "63": 249, | ||
| "64": 127, | ||
| "65": 108, | ||
| "66": 50, | ||
| "67": 34, | ||
| "68": 146, | ||
| "69": 227, | ||
| "70": 107, | ||
| "71": 127, | ||
| "72": 78, | ||
| "73": 62, | ||
| "74": 2, | ||
| "75": 133, | ||
| "76": 152, | ||
| "77": 136, | ||
| "78": 84, | ||
| "79": 255, | ||
| "80": 254, | ||
| "81": 191, | ||
| "82": 213, | ||
| "83": 213, | ||
| "84": 176, | ||
| "85": 140, | ||
| "86": 72, | ||
| "87": 1, | ||
| "88": 187, | ||
| "89": 217, | ||
| "90": 194, | ||
| "91": 69, | ||
| "92": 133, | ||
| "93": 106, | ||
| "94": 88, | ||
| "95": 180 | ||
| } | ||
| } | ||
| ] No newline at end of file |
There was a problem hiding this comment.
This file is located in a temporary directory (tmp-dev-voluntary-exit) and appears to be a test artifact. Temporary files and directories should not be committed to the repository. Please remove this file and add tmp-dev-voluntary-exit/ to your .gitignore file.
| "test:sim:backup_eth_provider": "LODESTAR_PRESET=minimal DOTENV_CONFIG_PATH=../../.env.test node -r dotenv/config --loader ts-node/esm test/sim/backupEthProvider.test.ts", | ||
| "test": "yarn test:unit && yarn test:e2e", | ||
| "check-readme": "typescript-docs-verifier" | ||
| "test": "jest --config ./jest.config.mjs" |
There was a problem hiding this comment.
we are not using jest, why is this file modified?
| @@ -0,0 +1,19 @@ | |||
| import tseslint from "typescript-eslint"; | |||
There was a problem hiding this comment.
why are you adding eslint configs? we are use biomejs for linting
|
@nflaig , I have read through all areas of correction you have highlighted and am now also working on a clean branch. |
|
closed in favor of #8585 |
3 participants
Motivation