refactor: use generic trait bound for RpcClient

[Wodann]

This PR splits off the RpcClient into two separate crates:

1. edr_rpc_client: contains a genericised version of RpcClient that accepts any type which implements the trait RpcMethod. As RpcClient's caching mechanism requires knowledge about the block number and chain ID I added the chain_id_request and block_number_request methods to trait RpcMethod, alongside an associated type that specifies the CacheableMethod type.
   The RpcClient type can be reused by chain types with varying JSON-RPC methods, as listed here.
2. edr_rpc_eth: introduces an EthRpcClient for all chains that support the Ethereum L1 JSON-RPC specification. It allows variance in the Block and Transaction types, but this could be extended to Receipt or other types as necessary. It's main goal is to avoid duplication of pub enum RequestMethod between Optimism, Arbitrum, Ethereum L1.

Alternatively to the block_number_request and chain_id_request methods in 1, they could be reworked to block_number_without_caching and chain_id_without_caching.

[changeset-bot]

🦋 Changeset detected

Latest commit: 5225095

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@nomicfoundation/edr	Patch

Not sure what this means? Click here to learn what changesets are.

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

[agostbiro]

Thanks LGTM overall, just one change request:

Please use the same approach (unit vs integration test) for testing the client in edr_rpc_client and edr_rpc_eth, unless I'm missing a reason not to. I don't have strong views which is preferable here, but I like to default to unit tests due to easier code sharing.

[Wodann]

unless I'm missing a reason not to.

In the edr_rpc_client crate, the test functions use a private member to validate logic:

assert!(client.cached_block_number.read().await.is_some());

edr_rpc_eth only uses the public API in combination with 1) a mocking server; and 2) remote access to Alchemy. If you don't think that's a valid reason to consider them integration tests, please let me know.

[agostbiro]

In the edr_rpc_client crate, the test functions use a private member to validate logic:

assert!(client.cached_block_number.read().await.is_some());

edr_rpc_eth only uses the public API in combination with 1) a mocking server; and 2) remote access to Alchemy. If you don't think that's a valid reason to consider them integration tests, please let me know.

Ok so there is a hard reason why client tests in edr_rpc_client need to be "unit tests".

I can see why you implemented client tests as "integration tests" in edr_rpc_eth from a logical perspective, but from a practical perspective I think this differentiation will be annoying, because often changes to one crate will need changes in the other and the different testing pattern can trip us up. So I'd prefer to have client tests in edr_rpc_eth as "unit tests" unless them being "integration tests" brings a practical benefit (since the crates are internal I can't think of any, but happy to be convinced otherwise).

[Wodann]

The main benefit I see is separation of the tests into dedicated test files, instead of either being interspersed with source files or resulting in incredibly long source files with a test submodule.

I'm happy to move the tests into the client.rs source file if it hampers your dev experience, though. Changed in 5225095

[agostbiro]

Awesome thanks! We can move the tests to a submodule client/tests.rs if it gets too large.