test: add e2e tests for the client for new tx type. #118
Conversation
Summary of ChangesHello @randygrok, 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 custom transaction type, Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
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
|
|
Claude finished @randygrok's task —— View job Code Review: E2E Tests for EvNode Transaction TypeReview Checklist:
Overall AssessmentThis PR successfully introduces a robust end-to-end testing infrastructure for the new EvNode transaction type (0x76). The implementation replaces previous test files with a well-structured, comprehensive test suite that validates both sponsored and unsponsored transaction flows. Strengths1. Excellent Test Infrastructure Design
2. Comprehensive Test Coverage
3. Professional Code Quality
Issues and Recommendations1. Security Concerns🚨 Critical: The test setup uses hardcoded private keys: const EXECUTOR_KEY = '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80'While this is a well-known Hardhat test key, it should be explicitly documented as such for clarity. Recommendation: Add a comment clarifying this is a standard test key: // Hardhat account #0 - standard test key, safe for testing
const EXECUTOR_KEY = '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80' as const;2. Performance ConsiderationsNode Startup Time: The test setup spawns a full Recommendations:
3. Test Coverage GapsMissing Error Cases: The current tests focus on success paths but lack explicit failure scenarios: // Missing test cases:
it('should handle insufficient gas limit', async () => { /* ... */ });
it('should reject invalid sponsor signature', async () => { /* ... */ });
it('should handle CREATE call validation errors', async () => { /* ... */ });
it('should handle sponsor with insufficient balance', async () => { /* ... */ });4. Code Quality ImprovementsMagic Numbers: Several hardcoded values could be better documented:
Type Safety: The // Current
async function waitForReceipt(client: any, hash: Hex, timeoutMs: number)
// Better
interface MinimalClient {
request(args: { method: string; params: unknown[] }): Promise<unknown>;
}
async function waitForReceipt(client: MinimalClient, hash: Hex, timeoutMs: number)5. Process ManagementPotential Race Condition: In if (!exited && child.exitCode === null) {
child.kill('SIGKILL'); // Process might have exited hereRecommendation: Add additional null check or use a more robust process management library. 6. CI/CD IntegrationGitHub Workflow: The workflow looks well-structured, but consider:
Minor Issues1. Error MessagesSome error messages could be more descriptive: // Current
throw new Error(`ev-reth binary not found. Run 'make build' or 'make build-dev' first.\n` +
`Checked: ${releaseBin}\n ${debugBin}`,
);
// Better - include current working directory context2. Resource CleanupThe cleanup function should handle partial failures: const cleanup = async () => {
try {
coordinator.stop();
} catch (error) {
console.warn('Failed to stop coordinator:', error);
}
try {
await killProcess(child);
} catch (error) {
console.warn('Failed to kill process:', error);
}
// ... etc
};Recommendations SummaryHigh Priority
Medium Priority
Low Priority
ConclusionThis is a high-quality PR that significantly improves the testing infrastructure for the EvNode transaction type. The code is well-structured, comprehensive, and follows TypeScript best practices. The identified issues are mostly minor improvements that would enhance robustness and maintainability. The PR demonstrates a solid understanding of both the EV-reth system and testing best practices. Overall Rating: ✅ Approved with minor recommendations |
![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 comprehensive end-to-end testing suite for the client, which is a significant improvement over the previous test scripts. The new tests use a dedicated test runner (node:test), programmatically set up and tear down an ev-reth node, and include a coordinator to simulate a consensus client for block production. The test cases cover various scenarios including unsponsored and sponsored transactions, both single and batch calls, with clear assertions to validate the behavior. The code is well-structured and robust. I have one minor suggestion to improve error handling in the test setup code for better debuggability.
| } catch { | ||
| // not ready yet | ||
| } |
There was a problem hiding this comment.
Swallowing errors with an empty catch block can hide underlying issues and make debugging harder. While the intention is to ignore connection errors during polling, it would be more robust to log the error if a debug flag is enabled. This can help diagnose problems if the RPC server fails to start for reasons other than just taking a while to be ready.
} catch (e) {
if (process.env.DEBUG) {
console.log(`[waitForRpc] retrying after error: ${e.message}`);
}
// not ready yet
}
4 participants
Problem
The viem client (clients/) only had unit tests and manual scripts (basic.test.ts, flows.test.ts, sponsored.test.ts) that required a running ev-reth node externally. These scripts used console.log for verification instead of proper assertions and were not part of CI.
Solution
Add an automated e2e test infrastructure that:
Launches ev-reth as a subprocess with an in-memory datadir and the test genesis
Runs a mini-coordinator (TypeScript) that replicates ev-node’s happy path: polls txpoolExt_getTxs and drives the Engine API 4-step block production cycle (forkchoiceUpdated → getPayload → newPayload → forkchoiceUpdated)
Executes proper node:test tests with assertions against the live node
Cleans up automatically (stops coordinator, kills ev-reth, removes temp files)