Skip to content

Conversation

@samholmes
Copy link
Contributor

@samholmes samholmes commented Apr 15, 2025

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Description

Added an EVM JSON-RPC plugin which evaluates blocks for transactions affecting subscriptions. It uses regular RPC servers and viem library for integration.


@samholmes samholmes force-pushed the sam/evm-rpc-plugin branch from 86882db to bd663c8 Compare April 16, 2025 19:32
Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

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

Almost perfect. There are a few tiny things to fix, but really nice PR.

Comment on lines +16 to +17
const mockWatchBlocks = jest.fn()
const mockGetLogs = jest.fn()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how I feel about these unit tests. Jest is pretty powerful, to the point where we're replacing this whole library with mocks. Cool. But now what are we testing? There isn't a whole lot of code left in the plugin itself.

Since the time has already been spent, let's keep these tests as they are. However, let's also avoid doing excessive testing in the future - there is a balance between too much testing and too little testing, and I think this what over-zealous testing looks like.

Maybe we can have a sidebar about this or maybe bring it up to the team on Monday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point.

Though to be clear, we're testing the AddressPlugin and it's integration with the viem library. This assumes the viem library is tested and correctly does what it says it will do. We mock it's behavior and the goal is to mock it idempotently.

Mocking the library for testing purposes runs the risk of a mismatch between library interface and the mock interface. If this mismatch happens, we may be "passing" but under the wrong assumptions and therefore not getting the proper coverage. However, if our assumptions are correct about the library's interface, then we're testing the plugin and it's integration with the library correctly.

We can discuss further to explore our goals with testing in a broader context. For this case, my goal is simply to cover the behavior of the AddressPlugin implementation and lock it in for some specificity.

@samholmes samholmes force-pushed the sam/evm-rpc-plugin branch from e0feac2 to dc0eacb Compare April 25, 2025 23:39
@samholmes samholmes merged commit 4c8d4c8 into main Apr 25, 2025
1 check 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.

3 participants