Skip to content

Conversation

@sddioulde
Copy link
Contributor

@sddioulde sddioulde commented Jan 28, 2026

Description

Removed the BaseAccount inheritance from EvmServerAccount.

Closes this issue.

Changes Made

1. Removed the problematic inheritance

  • Changed class EvmServerAccount(BaseAccount, BaseModel)class EvmServerAccount(BaseModel)
  • Removed the runtime import of BaseAccount
  • Kept BaseAccount as a TYPE_CHECKING-only import for type hints

2. Added comprehensive documentation

The new docstring explains:

  • Why we don't inherit from BaseAccount (async vs sync incompatibility)
  • How to get BaseAccount compatibility (use EvmLocalAccount wrapper)
  • Code examples showing both the async pattern and the sync wrapper pattern

3. Fixed type annotations

  • Added from __future__ import annotations for forward references
  • Updated transfer() method signature to use proper type hints with BaseAccount

Why This Works

1. No Breaking Changes in Practice

The code that would break (passing EvmServerAccount where BaseAccount is expected) was already broken because the methods return coroutines instead of results.

2. Existing Tests Pass

All tests pass because:

  • Tests already use EvmLocalAccount (not EvmServerAccount) when a BaseAccount is needed
  • Direct usage of EvmServerAccount with await continues to work unchanged

3. Clean Architecture

  • EvmServerAccount = Pure async API (network-based operations)
  • EvmLocalAccount = Sync wrapper for eth_account compatibility
  • Users choose the right tool for their use case

The Full Picture

This fix resolves the Liskov Substitution Principle violation mentioned in the GitHub issue. The class was claiming to be a BaseAccount but couldn't actually fulfill that contract because:

  • BaseAccount methods are synchronous
  • CDP API calls are inherently async (network operations)
  • Making them "fake sync" with nested event loops is unreliable

The solution aligns with the codebase's existing pattern

  • EvmLocalAccount already existed as the proper sync wrapper, so removing the false inheritance makes the architecture honest and clear.

Tests

Checklist

A couple of things to include in your PR for completeness:

  • Updated the typescript README if relevant
  • Updated the python README if relevant
  • Added a changelog entry
  • Added e2e tests if introducing new functionality

@cb-heimdall
Copy link

cb-heimdall commented Jan 28, 2026

✅ Heimdall Review Status

Requirement Status More Info
Reviews 2/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@sddioulde sddioulde marked this pull request as ready for review January 28, 2026 21:34
@sddioulde sddioulde force-pushed the chore/fixEvmServerAccountInheritance branch 2 times, most recently from 2ebd61f to b878f34 Compare January 28, 2026 22:08
@sddioulde sddioulde requested a review from 0xRAG January 28, 2026 22:36
0xRAG
0xRAG previously approved these changes Jan 29, 2026
Copy link
Contributor

@0xRAG 0xRAG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, just had one question!

@sddioulde sddioulde requested a review from sammccord January 29, 2026 15:30
@sddioulde sddioulde force-pushed the chore/fixEvmServerAccountInheritance branch 2 times, most recently from 1696d52 to 70baa5d Compare January 29, 2026 15:54
0xRAG
0xRAG previously approved these changes Jan 29, 2026
sammccord
sammccord previously approved these changes Jan 29, 2026
@sddioulde sddioulde dismissed stale reviews from sammccord and 0xRAG via aa4d3f9 January 29, 2026 16:26
@sddioulde sddioulde force-pushed the chore/fixEvmServerAccountInheritance branch from aa4d3f9 to 42d5cc1 Compare January 29, 2026 16:26
@sddioulde sddioulde merged commit 75789a3 into main Jan 29, 2026
39 checks passed
@sddioulde sddioulde deleted the chore/fixEvmServerAccountInheritance branch January 29, 2026 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Python: EVMServerAccount does not actually implement BaseAccount

4 participants