Skip to content

Conversation

@Himess
Copy link
Contributor

@Himess Himess commented Dec 12, 2025

Closes #151

Summary

Adds integration tests for eth_call with ERC-20 token operations, addressing the gap identified after the v13.3 bug where proxy-based tokens broke on mainnet.

Changes

New Contracts:

  • TestERC20.sol - Simple ERC-20 with mint/burn for testing
  • TransparentProxy.sol - EIP-1967 compliant proxy (USDC-style)

New Tests (8 total):

  • test_erc20_transfer - basic transfer
  • test_erc20_mint - minting tokens
  • test_erc20_burn - burning tokens
  • test_erc20_approve_transfer_from - approval flow
  • test_transparent_proxy_erc20_transfer - transfer via proxy
  • test_transparent_proxy_erc20_mint - mint via proxy
  • test_transparent_proxy_erc20_burn - burn via proxy
  • test_transparent_proxy_erc20_approve_transfer_from - approval via proxy

Each test verifies state changes through eth_call after executing transactions, catching issues with delegatecall behavior that caused the mainnet bug.

Add comprehensive eth_call tests for ERC-20 operations:
- transfer, mint, burn, approve+transferFrom
- TransparentProxy (USDC-style) variants for all operations

Closes base#151
@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Dec 12, 2025

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/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

…all tests

- Use explicit block numbers instead of BlockNumberOrTag::Latest
- Replace U256::from_str parsing with U256::abi_decode for proper ABI decoding
- Remove unnecessary sleep() delays
- Track block numbers after each transaction to query correct state
…bility issue

Tests are temporarily ignored because TestHarness commits blocks via Engine API,
but eth_call queries don't see the committed contract state. FlashblocksHarness
works differently by maintaining pending state that eth_call can query.

Tests should be re-enabled once TestHarness state visibility is fixed or tests
are rewritten to use FlashblocksHarness with proper flashblock payload construction.
@Himess Himess marked this pull request as draft December 12, 2025 22:16
Rewrite eth_call ERC-20 tests to use FlashblocksHarness with manually constructed
flashblock payloads instead of TestHarness.

Tests now include:
- test_erc20_deployment: Basic token deployment and name/symbol/decimals queries
- test_proxy_erc20_deployment: TransparentProxy deployment and delegatecall queries
- test_erc20_mint: Token minting and balance verification
- test_proxy_erc20_mint: Minting through proxy contract

This approach properly tests eth_call against contract state by using flashblock
payloads that expose pending state to RPC queries.
@Himess
Copy link
Contributor Author

Himess commented Dec 12, 2025

Quick update on what I tried:

  1. Started with TestHarness + Engine API - blocks commit fine but eth_call returns 0 for contract state. Tried sleep delays, explicit block numbers, nothing worked.

  2. Then marked tests as #[ignore] but that defeats the purpose.

  3. Now trying FlashblocksHarness approach like flashblocks_rpc.rs does - manually building payloads. Waiting on CI.

Basically TestHarness commits to canonical chain but RPC doesn't see it, while FlashblocksHarness exposes state as pending block which eth_call can read.

@Himess Himess marked this pull request as ready for review December 12, 2025 23:01
Comment on lines 23 to 39
sol!(
#[sol(rpc)]
TestERC20,
concat!(
env!("CARGO_MANIFEST_DIR"),
"/../test-utils/contracts/out/TestERC20.sol/TestERC20.json"
)
);

sol!(
#[sol(rpc)]
TransparentProxy,
concat!(
env!("CARGO_MANIFEST_DIR"),
"/../test-utils/contracts/out/TransparentProxy.sol/TransparentProxy.json"
)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have base_reth_test_utils as a dependency here, could we localize these into the test-utils crate and then export whatever is needed? That way we don't need to use an external path like "/../test-utils/" which is subject to change + breakage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey noted , i will do it

Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

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

All the tests look good, but I think we can move a few things to test-utils that don't need to be in the rpc tests module so we're not relying on external files from inside the rpc crate.

@Himess Himess marked this pull request as draft December 12, 2025 23:31
@Himess
Copy link
Contributor Author

Himess commented Dec 12, 2025

@refcell i am still working to solve CI/Test .

- Increase gas limit from 500k to 3M for larger contract deployments
  (ERC-20 bytecode ~5KB requires more gas for CREATE opcode)
- Simplify TestERC20 constructor to use hardcoded values
- Fix cumulative_gas_used in mint payload (must be cumulative, not per-tx)
- Add deployer balance to base payload for EVM transaction execution
- Remove proxy name() assertion (proxy uses own storage, not impl storage)

Fixes tests: test_erc20_deployment, test_proxy_erc20_deployment,
test_erc20_mint, test_proxy_erc20_mint
@Himess
Copy link
Contributor Author

Himess commented Dec 13, 2025

All 4 ERC-20 tests now pass locally:

  • test_erc20_deployment
  • test_proxy_erc20_deployment
  • test_erc20_mint
  • test_proxy_erc20_mint

Existing flashblocks_rpc tests (19) still pass.

@haardikk21
Copy link
Collaborator

Can you replace the contract implementations with ERC20 or MockERC20 from solmate as that's already a dependency and perhaps the upgradeable proxy with the one from OpenZeppelin instead?

just to ensure it works with widely used things instead of needing to maintain these handwritten contracts in the repo. To have them compiled and outputted as artifacts you can do a simple script similar to the DeployCounter.s.sol which will ensure that running just build-contracts builds those two contracts as well

Move sol! macro contract bindings from test files to base_reth_test_utils::contracts module:
- Add contracts.rs with DoubleCounter, TestERC20, and TransparentProxy bindings
- Update flashblocks_rpc.rs to import DoubleCounter from test-utils
- Update eth_call_erc20.rs to import TestERC20 and TransparentProxy from test-utils
- Add alloy-sol-macro, alloy-sol-types, and alloy-contract dependencies to test-utils

This eliminates relative path references like '/../test-utils/contracts/out/' which are
subject to breakage when paths change.
@Himess
Copy link
Contributor Author

Himess commented Dec 13, 2025

Can you replace the contract implementations with ERC20 or MockERC20 from solmate as that's already a dependency and perhaps the upgradeable proxy with the one from OpenZeppelin instead?

just to ensure it works with widely used things instead of needing to maintain these handwritten contracts in the repo. To have them compiled and outputted as artifacts you can do a simple script similar to the DeployCounter.s.sol which will ensure that running just build-contracts builds those two contracts as well

Okey okey it make sense.
I will update shortly.
Thanks for feedback btw

…radeableProxy

Replace handwritten test contracts with widely-used library implementations:
- MockERC20 from Solmate (lib/solmate)
- TransparentUpgradeableProxy from OpenZeppelin (lib/openzeppelin-contracts)

Add DeployERC20.s.sol script following DeployDoubleCounter.s.sol pattern.
Update Rust bindings and tests to use new contract artifacts.
@Himess
Copy link
Contributor Author

Himess commented Dec 13, 2025

Done. Updated in commit 9a9252e:
@haardikk21

@Himess Himess marked this pull request as ready for review December 13, 2025 01:01
@haardikk21 haardikk21 merged commit 18d98c9 into base:main Dec 13, 2025
10 checks passed
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.

improve eth_call tests to have more real-world scenarios

4 participants