client impl part 1 #112

randygrok · 2026-01-28T09:23:34Z

Description

Closes: #109

claude · 2026-01-28T09:23:53Z

Claude finished @randygrok's task —— View job

Code Review for Client Implementation Part 1

I've completed a thorough review of this comprehensive EvNode client implementation. This is an impressive piece of work that provides a complete TypeScript/Viem client for the custom EvNode transaction type (0x76).

Review Checklist:

Read repository guidelines (CLAUDE.md)
Review main client implementation (clients/src/index.ts)
Analyze package configuration and dependencies
Review test coverage and examples
Assess code quality and best practices
Check for security concerns
Evaluate performance considerations
Provide constructive feedback and recommendations

Overall Assessment

Strengths:

Comprehensive implementation of EvNode transaction encoding/decoding, signing, and client actions
Well-structured TypeScript with proper type definitions
Good separation of concerns between encoding, signing, and client operations
Thorough RLP encoding/decoding with proper validation
Clear examples and E2E tests demonstrating usage
Proper Viem integration following established patterns

Areas for Improvement:

🔴 High Priority Issues

1. Gas Estimation Inaccuracies (`clients/src/index.ts:201-218`)

export function estimateIntrinsicGas(calls: Call[]): bigint {
  let gas = BASE_TX_GAS; // base transaction cost
  
  for (const call of calls) {
    gas += BASE_TX_GAS; // each call costs at least 21000 gas  // ❌ INCORRECT

Issue: The function incorrectly adds 21000 gas for each call. The base transaction cost (21000) should only be applied once per transaction, not per call.

Recommendation:

export function estimateIntrinsicGas(calls: Call[], accessList?: AccessList): bigint {
  let gas = BASE_TX_GAS; // base transaction cost (only once)
  
  for (const call of calls) {
    if (call.to === null) gas += CREATE_GAS; // CREATE costs extra
    
    // Calldata gas calculation
    for (const byte of hexToBytes(call.data)) {
      gas += byte === 0 ? 4n : 16n;
    }
  }
  
  // Add access list costs if present
  if (accessList) {
    gas += BigInt(accessList.length) * 2400n; // per address
    for (const item of accessList) {
      gas += BigInt(item.storageKeys.length) * 1900n; // per storage key
    }
  }
  
  return gas;
}

2. Unsafe Error Handling in fetchMaxPriorityFee (`clients/src/index.ts:632-634`)

} catch {
  return 0n;  // ❌ Swallows all errors
}

Issue: This broad catch block masks network errors, RPC failures, and other serious issues.

Recommendation:

} catch (err) {
  // Only fallback to 0n for unsupported method errors
  if (err instanceof Error && (
    err.message.includes('not found') || 
    err.message.includes('not supported') ||
    err.message.includes('-32601') // Method not found
  )) {
    return 0n;
  }
  throw err; // Re-throw other errors
}

3. Deprecated eth_sign Method (`clients/src/index.ts:389-397`)

The eth_sign method has inconsistent behavior across Ethereum clients and is deprecated. This can cause signature failures depending on the RPC provider.

Recommendation: Add clear documentation warning about this limitation and consider supporting EIP-712 signing as an alternative.

🟡 Medium Priority Issues

4. Magic Numbers Should Be Constants

Line 117: if (decoded.length !== 11) - should use a named constant
Line 226: if (tx.calls[i].to === null) - consider isCreateCall() helper function
Gas calculation constants need better organization

5. File Structure (`clients/src/index.ts`)

As noted by @chatton, the 641-line index.ts should be split into focused modules:

src/
├── index.ts          // exports
├── client.ts         // createEvnodeClient, evnodeActions
├── encoding.ts       // RLP encode/decode functions
├── signing.ts        // signing and hash computation
├── gas.ts           // gas estimation utilities
└── types.ts         // interfaces and types

6. Suboptimal Gas Price Logic (`clients/src/index.ts:598`)

Using eth_gasPrice for EIP-1559 maxFeePerGas can result in overpaying. Should use baseFeePerGas + maxPriorityFeePerGas.

7. Duplicate Type Definitions

The inline anonymous types in function parameters duplicate existing interfaces, reducing maintainability.

🟢 Low Priority Suggestions

8. Code Readability

Line 410: Replace ternary with if/else for better readability
Consider extracting complex validation logic into separate functions
Add JSDoc documentation for public APIs

9. Test Coverage

Add unit tests for individual functions (encoding, gas estimation, signing)
Set up CI to run tests on every PR
Add linting/formatting configuration

Security Assessment ✅

The code follows good security practices:

Proper signature validation and normalization
Safe RLP encoding/decoding with validation
Appropriate error handling for cryptographic operations
No exposure of private keys or sensitive data

Performance Considerations ✅

The implementation is efficient:

Minimal memory allocation in hot paths
Proper use of BigInt for large numbers
Efficient RLP encoding using Viem's optimized functions

Recommendations Summary

Fix gas estimation logic - This is critical for transaction success
Improve error handling - Prevent masking of network/RPC issues
Add proper documentation for eth_sign limitations
Refactor file structure - Split into focused modules
Add comprehensive unit tests and CI integration
Extract constants for magic numbers
Consider EIP-712 signing as an alternative to eth_sign

This is a solid foundation for the EvNode client. The core architecture is well-designed, and with these improvements, it will be production-ready. The examples and E2E tests demonstrate the API works end-to-end, which is excellent validation.

Great work overall! The implementation shows deep understanding of Ethereum transaction mechanics and Viem integration patterns.

gemini-code-assist · 2026-01-28T09:24:08Z

Summary of Changes

Hello @randygrok, I'm Gemini Code Assist¹! 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 establishes the foundational client-side logic for a new EvNode transaction system. It defines the structure and behavior of these specialized transactions, enabling developers to programmatically construct, sign with multiple parties (executor and sponsor), and dispatch them to the network. The implementation focuses on robust data handling, signature management, and integration with existing blockchain interaction libraries to streamline the development of applications utilizing this new transaction type.

Highlights

New EvNode Transaction Type: Introduces a custom EvNode transaction type (0x76) with specific structures for calls, access lists, and dual signatures (executor and optional fee payer/sponsor).
Comprehensive Client Library: Provides a full client implementation (createEvnodeClient) for building, signing, and sending these custom transactions, including functions for creating sponsorable intents and handling sponsor signatures.
RLP Encoding and Decoding: Implements robust RLP encoding and decoding mechanisms for EvNode transactions, ensuring proper serialization and deserialization for on-chain submission and off-chain processing.
Viem Integration: Seamlessly integrates with the viem library, leveraging its utilities for address recovery, hash computation, and RPC interactions, and registers the EvNode transaction type with viem's transaction serializer.

🧠 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.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a Viem client for a custom EvNode transaction type. The implementation is comprehensive, covering transaction encoding/decoding, signing, and client-side actions. My review focuses on improving robustness, correctness of gas estimations, and maintainability. I've identified a few areas for improvement, including more accurate gas calculations, safer error handling in RPC calls, and addressing potential issues with the eth_sign method. I've also suggested some minor refactoring to reduce code duplication.

gemini-code-assist · 2026-01-28T09:30:43Z

clients/src/index.ts

+export function hashSignerFromRpcClient(
+  client: Client,
+  address: Address,
+): HashSigner {
+  return {
+    address,
+    signHash: async (hash) => {
+      // eth_sign is expected to sign raw bytes (no EIP-191 prefix).
+      const signature = await client.request({
+        method: 'eth_sign',
+        params: [address, hash],
+      });
+      if (!isHex(signature)) {
+        throw new Error('eth_sign returned non-hex signature');
+      }
+      return signature;
+    },
+  };
+}


The eth_sign RPC method has inconsistent behavior across different Ethereum node implementations and is generally considered deprecated. Some nodes (like Geth) will sign the raw hash as expected here, but others (like Parity/OpenEthereum) will prefix the hash according to EIP-191, which would lead to invalid signatures for this use case. This can cause hard-to-debug issues for users of your client depending on which RPC provider they use.

It's highly recommended to add a prominent warning in the function's documentation about this limitation and the specific node behavior it relies on.

gemini-code-assist · 2026-01-28T09:30:43Z

clients/src/index.ts

+async function fetchMaxPriorityFee(client: Client): Promise<bigint> {
+  try {
+    const result = await client.request({ method: 'eth_maxPriorityFeePerGas' });
+    if (!isHex(result)) throw new Error('eth_maxPriorityFeePerGas returned non-hex');
+    return hexToBigIntSafe(result);
+  } catch {
+    return 0n;
+  }
+}


The broad catch block here will swallow any error from the eth_maxPriorityFeePerGas call and default to 0n. This can hide underlying problems like network connectivity issues or RPC misconfigurations, leading to transactions being sent with a potentially undesirable maxPriorityFeePerGas. It's safer to only catch specific errors that indicate the method is not supported (like on a pre-EIP-1559 chain) and re-throw others.

try { const result = await client.request({ method: 'eth_maxPriorityFeePerGas' }); if (!isHex(result)) throw new Error('eth_maxPriorityFeePerGas returned non-hex'); return hexToBigIntSafe(result); } catch (err) { // Only fallback to 0n if the method is not supported. // A robust implementation would check for a specific RPC error code (e.g., -32601). if (err instanceof Error && (err.message.includes('not found') || err.message.includes('not supported'))) { return 0n; } throw err; }

gemini-code-assist · 2026-01-28T09:30:43Z

clients/src/index.ts

+export function estimateIntrinsicGas(calls: Call[]): bigint {
+  let gas = 21000n;
+
+  for (const call of calls) {
+    if (call.to === null) gas += 32000n;
+
+    for (const byte of hexToBytes(call.data)) {
+      gas += byte === 0 ? 4n : 16n;
+    }
+  }
+
+  return gas;
+}


The current implementation of estimateIntrinsicGas does not account for the gas costs associated with an accessList (per EIP-2930). This will lead to an underestimation of the intrinsic gas for transactions using access lists, potentially causing them to fail. The function should be updated to accept an accessList and include its cost in the calculation.

export function estimateIntrinsicGas(calls: Call[], accessList?: AccessList): bigint { let gas = 21000n; for (const call of calls) { if (call.to === null) gas += 32000n; for (const byte of hexToBytes(call.data)) { gas += byte === 0 ? 4n : 16n; } } if (accessList) { const ACCESS_LIST_ADDRESS_COST = 2400n; const ACCESS_LIST_STORAGE_KEY_COST = 1900n; gas += BigInt(accessList.length) * ACCESS_LIST_ADDRESS_COST; for (const item of accessList) { gas += BigInt(item.storageKeys.length) * ACCESS_LIST_STORAGE_KEY_COST; } } return gas; }

gemini-code-assist · 2026-01-28T09:30:43Z

clients/src/index.ts

+    async sendEvNodeTransaction(args: {
+      calls: Call[];
+      executor: HashSigner;
+      chainId?: bigint;
+      nonce?: bigint;
+      maxFeePerGas?: bigint;
+      maxPriorityFeePerGas?: bigint;
+      gasLimit?: bigint;
+      accessList?: AccessList;
+    }): Promise<Hex> {


The args parameter uses an inline anonymous type that is very similar to the EvnodeSendArgs interface defined earlier. This creates code duplication and can make maintenance harder.

To improve this, consider defining a specific type for these arguments that extends EvnodeSendArgs but makes executor a required property. This would make the code more DRY and easier to reason about.

gemini-code-assist · 2026-01-28T09:30:43Z

clients/src/index.ts

+    async createSponsorableIntent(args: {
+      calls: Call[];
+      executor: HashSigner;
+      chainId?: bigint;
+      nonce?: bigint;
+      maxFeePerGas?: bigint;
+      maxPriorityFeePerGas?: bigint;
+      gasLimit?: bigint;
+      accessList?: AccessList;
+    }): Promise<SponsorableIntent> {


Similar to sendEvNodeTransaction, this function uses an inline anonymous type for its args parameter which duplicates most of EvnodeIntentArgs. Reusing and extending the existing interface would make the code more maintainable and reduce redundancy.

gemini-code-assist · 2026-01-28T09:30:43Z

clients/src/index.ts

+  const nonce = overrides.nonce ?? (await fetchNonce(client, address));
+  const maxPriorityFeePerGas =
+    overrides.maxPriorityFeePerGas ?? (await fetchMaxPriorityFee(client));
+  const maxFeePerGas = overrides.maxFeePerGas ?? (await fetchGasPrice(client));


Defaulting maxFeePerGas to the result of eth_gasPrice can be suboptimal for EIP-1559 transactions, as it might lead to overpaying for gas. A more conventional approach is to calculate maxFeePerGas as baseFeePerGas + maxPriorityFeePerGas. This would require fetching the baseFeePerGas from the latest block.

gemini-code-assist · 2026-01-28T09:30:43Z

clients/src/index.ts

+  const maxPriorityFeePerGas =
+    overrides.maxPriorityFeePerGas ?? (await fetchMaxPriorityFee(client));
+  const maxFeePerGas = overrides.maxFeePerGas ?? (await fetchGasPrice(client));
+  const gasLimit = overrides.gasLimit ?? estimateIntrinsicGas(calls);


To fix the underestimation of intrinsic gas, you should pass the accessList to estimateIntrinsicGas here, assuming you've updated the function as per my other comment.

const gasLimit = overrides.gasLimit ?? estimateIntrinsicGas(calls, accessList);

- Add 21000 gas per call in estimateIntrinsicGas for batch support - Unify env vars to use EXECUTOR_PRIVATE_KEY across all tests - Rewrite evnode-flows.ts with balance verification for all 4 flows

chatton

Nice over all looking good!

A few things and then I think we can merge if the E2Es are passing

can we add some unit tests and CI for the client (first set can be minimal I think, something we can add to)
update the package.json to allow for running the basic/flows/sponsored tests.
Add a basic linter or formatter in the package.json also (and run in CI)

clients/src/index.ts

chatton · 2026-02-09T14:30:13Z

clients/src/index.ts

+    throw new Error('Invalid EvNode transaction payload');
+  }
+
+  if (decoded.length !== 11) {


can we make the 11 a named constant?

chatton · 2026-02-09T14:33:23Z

clients/src/index.ts

+
+  return {
+    client: options.client,
+    actions,


is there a reason we embedded the functions in the object like this? Does it make more sense to have a concrete type to return EvNodeClient and have everything defined as first class functions?

TIL this is how it is recommended to do it with viem, super weird but I guess we should stick to the recommendations :D

chatton · 2026-02-09T14:37:02Z

clients/src/index.ts

@@ -0,0 +1,634 @@
+import {


I don't think we should have all this in an index.ts I think we should break it up into something like

src/ index.ts // export { createEvnodeClient } from './client' client.ts encoding.ts signing.ts

or whatever files make sense, but I don't think the full implementation should live here WDYT?

chatton · 2026-02-09T14:41:43Z

package.json

+    "doc": "docs"
+  },
+  "scripts": {
+    "test": "echo \"Error: no test specified\" && exit 1"


can we make sure this is running the correct tests and also running in CI

chatton · 2026-02-09T14:44:02Z

clients/tests/basic.test.ts

@@ -0,0 +1,73 @@
+import { createClient, http } from 'viem';


can we make sure these tests are being run via CI?

ah just seeing these are E2E tests, we should still make an npm run/test target for them, and should probably have some unit tests which should run on every PR

client impl part 1

6883023

gemini-code-assist bot reviewed Jan 28, 2026

View reviewed changes

jgimeno and others added 7 commits January 29, 2026 10:00

add tests for client

77965a0

fix: gas estimation for batch transactions and improve tests

7656099

- Add 21000 gas per call in estimateIntrinsicGas for batch support - Unify env vars to use EXECUTOR_PRIVATE_KEY across all tests - Rewrite evnode-flows.ts with balance verification for all 4 flows

make package ready for npm

3473e67

add examples for multiple types of tx

fb4b8dd

Merge branch 'main' into randygrok/client-viem-ev-reth

4e3245b

Merge branch 'main' into randygrok/client-viem-ev-reth

c96878e

refactor: consolidate duplicate type definitions in client

c4010be

randygrok marked this pull request as ready for review February 2, 2026 10:10

randygrok requested a review from a team as a code owner February 2, 2026 10:10

chatton approved these changes Feb 9, 2026

View reviewed changes

randygrok added 4 commits February 10, 2026 10:27

refactor: extract BASE_TX_GAS constant for intrinsic gas estimation

2eaf6d1

refactor: replace ternary with if/else in calldata gas calculation

87013ef

refactor: extract CREATE_GAS constant for contract deployment gas cost

cf8b1a7

refactor: add isCreateCall helper for contract creation checks

e556c23

client impl part 1 #112

Are you sure you want to change the base?

client impl part 1 #112

Uh oh!

Conversation

randygrok commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

claude bot commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for Client Implementation Part 1

Overall Assessment

🔴 High Priority Issues

1. Gas Estimation Inaccuracies (clients/src/index.ts:201-218)

2. Unsafe Error Handling in fetchMaxPriorityFee (clients/src/index.ts:632-634)

3. Deprecated eth_sign Method (clients/src/index.ts:389-397)

🟡 Medium Priority Issues

4. Magic Numbers Should Be Constants

5. File Structure (clients/src/index.ts)

6. Suboptimal Gas Price Logic (clients/src/index.ts:598)

7. Duplicate Type Definitions

🟢 Low Priority Suggestions

8. Code Readability

9. Test Coverage

Security Assessment ✅

Performance Considerations ✅

Recommendations Summary

Uh oh!

gemini-code-assist bot commented Jan 28, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

chatton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatton Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

chatton Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

chatton Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

chatton Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

chatton Feb 9, 2026

Choose a reason for hiding this comment

randygrok commented Jan 28, 2026 •

edited

Loading

claude bot commented Jan 28, 2026 •

edited

Loading

1. Gas Estimation Inaccuracies (`clients/src/index.ts:201-218`)

2. Unsafe Error Handling in fetchMaxPriorityFee (`clients/src/index.ts:632-634`)

3. Deprecated eth_sign Method (`clients/src/index.ts:389-397`)

5. File Structure (`clients/src/index.ts`)

6. Suboptimal Gas Price Logic (`clients/src/index.ts:598`)