Use erigon as the external client for op_geth_test.go #52
Conversation
There was a problem hiding this comment.
Overall LGTM. I might suggest renaming op_geth_test.go to l2_eth_test.go or something more generic to indicate that these tests are meant to cover the L2 eth clients generally, not just op-geth. Once we're happy with this, we should certainly send it upstream.
| @@ -235,7 +236,7 @@ type EthInstance interface { | |||
| WSEndpoint() string | |||
| HTTPAuthEndpoint() string | |||
| WSAuthEndpoint() string | |||
| Close() | |||
| Close() error | |||
There was a problem hiding this comment.
It's not immediately obvious to me why we changed this signature?
There was a problem hiding this comment.
It's easier for us to use the same code node.Close() to stop both external clients and the geth client started by geth.Init().
There was a problem hiding this comment.
The existing GethInstance was already calling gi.Node.Close(), if rather than storing the Node, we stored the EthInstance, we'd get access to that. But, not a big deal either way.
There was a problem hiding this comment.
Good suggestion! I replaced Node with EthInstance.
| @@ -90,7 +116,10 @@ func NewOpGeth(t *testing.T, ctx context.Context, cfg *SystemConfig) (*OpGeth, e | |||
| ) | |||
| require.Nil(t, err) | |||
|
|
|||
| l2Client, err := ethclient.Dial(node.HTTPEndpoint()) | |||
| l2Client, err := ethclient.Dial(HTTPEndpoint) | |||
| require.Nil(t, err) | |||
There was a problem hiding this comment.
Nit: The existing test is using require.Nil(t, err), so maybe it's best to stick with it, but the more idiomatic assertion would be require.NoError(t, err).
| @@ -235,7 +236,7 @@ type EthInstance interface { | |||
| WSEndpoint() string | |||
| HTTPAuthEndpoint() string | |||
| WSAuthEndpoint() string | |||
| Close() | |||
| Close() error | |||
There was a problem hiding this comment.
The existing GethInstance was already calling gi.Node.Close(), if rather than storing the Node, we stored the EthInstance, we'd get access to that. But, not a big deal either way.
|
oh wow, great!!! |
This PR can bring up erigon as the external client for all tests in
op_geth_test.go.