Skip to content

Conversation

roninjin10
Copy link
Collaborator

Allow developers to override the chain ID when forking to avoid wallet confusion when the same chain ID is used for both fork and original network.

Changes:

  • Extended ForkOptions interface with optional chainId property
  • Modified createTevmNode to prioritize fork chainId over auto-detection
  • Added comprehensive test coverage across node, actions, memory-client, and state packages
  • Updated documentation and examples

Priority order:

  1. fork.chainId (highest priority - this PR)
  2. common.id
  3. auto-detected from RPC
  4. default chain ID

Fixes #1595

Generated with Claude Code

Allow developers to override the chain ID when forking to avoid
wallet confusion when the same chain ID is used for both fork
and original network.

**Changes:**
- Extended ForkOptions interface with optional chainId property
- Modified createTevmNode to prioritize fork chainId over auto-detection
- Added comprehensive test coverage across node, actions, memory-client, and state packages
- Updated documentation and examples

**Priority order:**
1. fork.chainId (highest priority - this PR)
2. common.id
3. auto-detected from RPC
4. default chain ID

**Usage:**
```typescript
const client = createMemoryClient({
  fork: {
    transport: http('https://mainnet.optimism.io'),
    chainId: 1337 // Custom chain ID to avoid wallet conflicts
  }
})
```

Fixes #1595

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Will Cory <[email protected]>
@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2025

⚠️ No Changeset found

Latest commit: a33e5c8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Oct 5, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
node Ready Ready Preview Oct 5, 2025 0:13am
tevm-monorepo-app Ready Ready Preview Oct 5, 2025 0:13am
1 Skipped Deployment
Project Deployment Preview Updated (UTC)
tevm-monorepo-tevm Ignored Ignored Oct 5, 2025 0:13am

@claude
Copy link
Contributor

claude bot commented Oct 5, 2025

Claude finished @roninjin10's task —— View job


PR Review: Add chainId Override Option to ForkOptions

Review Checklist

  • Read and analyze all changed files
  • Review ForkOptions interface changes
  • Examine createTevmNode implementation
  • Check test coverage and quality
  • Identify potential bugs or issues
  • Assess performance and security considerations
  • Provide constructive feedback

📝 Code Quality & Best Practices

Excellent Implementation

  • Clean API Design: The optional chainId property in ForkOptions is well-designed and follows TypeScript best practices
  • Clear Priority Order: The implementation correctly follows the documented priority: fork.chainId > common.id > auto-detected > default
  • Consistent Patterns: Changes align with existing codebase conventions and patterns
  • Good Documentation: JSDoc comments clearly explain the purpose and use case

🔍 Code Analysis

ForkOptions Interface (packages/state/src/state-types/ForkOptions.ts):

  • Good: Optional chainId?: number property with clear documentation
  • Good: Example in JSDoc shows practical usage
  • Good: Explains the use case (avoiding wallet confusion)

createTevmNode Implementation (packages/node/src/createTevmNode.js:92-112):

  • Excellent: Clean priority logic with proper async/await handling
  • Good: Appropriate debug logging for troubleshooting
  • Good: Type conversion to BigInt(chainId) for internal consistency

🧪 Test Coverage

Comprehensive Testing

  • ForkOptions.spec.ts: Complete type validation and interface testing
  • createTevmNode.spec.ts: End-to-end functionality testing with priority scenarios
  • createMemoryClient.spec.ts: Integration testing for memory client
  • chainIdHandler.spec.ts: Handler-level testing

Test Quality Highlights:

  • Tests cover all priority scenarios (fork override, common fallback, auto-detection)
  • Integration tests verify end-to-end behavior
  • Good use of realistic test data (Optimism testnet)

🚀 Performance Considerations

Efficient Implementation

  • Minimal Overhead: chainId check is O(1) and only adds minimal branching
  • Lazy Evaluation: Chain ID resolution happens asynchronously during initialization
  • No Breaking Changes: Backward compatible, existing code continues to work

🔒 Security Analysis

Secure Implementation

  • Input Validation: TypeScript ensures chainId is a number
  • No Injection Risks: Simple numeric value, no string manipulation or external input parsing
  • Proper Fallbacks: Graceful fallback chain prevents undefined states

🎯 Recommendations

Minor Suggestions:

  1. Consider Chain ID Validation (packages/state/src/state-types/ForkOptions.ts:27):

    /**
     * Optional chain ID override. If not provided, the chain ID will be auto-detected
     * from the forked network. This is useful to avoid wallet confusion when working
     * with forked chains that have the same chain ID as the original network.
     * @remarks Chain ID should be a positive integer. Common values: 1 (mainnet), 10 (optimism), etc.
     */
    chainId?: number
  2. Add Runtime Validation (optional enhancement):

    // In createTevmNode.js, consider adding:
    if (options.fork?.chainId !== undefined && options.fork.chainId <= 0) {
      throw new Error('Fork chainId must be a positive integer')
    }

Missing Changeset Notice:

  • The bot correctly identified that this PR needs a changeset for version bumping
  • Consider adding changesets for: @tevm/actions, @tevm/memory-client, @tevm/node, @tevm/state

🎉 Overall Assessment

Grade: A-

This is an excellent implementation that:

  • ✅ Solves a real user pain point (wallet confusion with same chain IDs)
  • ✅ Maintains backward compatibility
  • ✅ Has comprehensive test coverage
  • ✅ Follows existing code patterns and conventions
  • ✅ Is well-documented with clear examples
  • ✅ Has minimal performance impact
  • ✅ Is secure and properly handles edge cases

The implementation is production-ready and adds valuable functionality for developers working with forked chains. The only minor issue is the missing changeset, which is a process requirement rather than a code quality concern.


@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 5, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch claude/issue-1595-20251004-2310

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Change forked chain ID

1 participant