chore: make the evm client and relayer testable #85
Conversation
Codecov Report
@@ Coverage Diff @@
## main #85 +/- ##
=======================================
Coverage 59.75% 59.75%
=======================================
Files 6 6
Lines 164 164
=======================================
Hits 98 98
Misses 53 53
Partials 13 13 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
# Conflicts: # relayer/relayer.go
relayer/relayer.go
Outdated
| time.Sleep(2 * time.Second) | ||
| continue | ||
| } | ||
| ethClient.Close() |
There was a problem hiding this comment.
[blocking question]
should this close be deferred? it looks like we could end up spinning up many of these, should we only be spinning up 1?
There was a problem hiding this comment.
should this close be deferred?
I prefered to close it everytime not to leave hanging rpc connections. This is because we don't know how much time there will be between every attestation relay.
For example, now with the default 400block and 15sec block time, data commitments will be relayed every 1hour and a half. For valsets, that depends on the network.
That's why I chose to use the connection then close it immediatly.
it looks like we could end up spinning up many of these, should we only be spinning up 1?
Only one will be spinned up because relaying attestations is sequential, there is no way to parallelize it.
There was a problem hiding this comment.
okay cool, good to know
I prefered to close it everytime not to leave hanging rpc connections.
yeah that's what I was worried about with potentially spinning one up in each loop. why not only open a single one before we start the loop and then defer to close? atm, it look like we will spin up many if we receive an error on line 99
There was a problem hiding this comment.
I prefered to close it everytime not to leave hanging rpc connections.
opening once before the loop starts and defering the close should guarantee that we do not do this, and as a bonus, its very flexible for future changes, and follows the standard practices of go
| return nil, err | ||
| ec.logger.Debug("waiting for transaction to be confirmed", "hash", tx.Hash().String()) | ||
|
|
||
| receipt, err := bind.WaitMined(ctx, backend, tx) |
There was a problem hiding this comment.
[blocking question]
do we know if we should still be using WaitMined given eth is no longer PoW?
There was a problem hiding this comment.
I checked the repo and no changes have been done to it. Probably they should rename it but they still didn't.
Anyway, it's not related to the underlying consensus, it just keeps polling if the transaction was included in some block.
Yes, when running tests, we will use the simulated backend ops. However, when we use a real network, we will pass the real opts created using an |
| privateKey: privateKey, | ||
| evmRPC: evmRPC, | ||
| gasLimit: gasLimit, | ||
| Wrapper: wrapper, |
There was a problem hiding this comment.
I think I've seen different names for the wrapper somewhere (I'll double check). If yes, we might want to be consistent throughout
Overview
This PR refactors the relayer and EVM client to make them testable.
It mainly makes the eth client and transaction opts pluggable so that we can simulate them using geth simulated backend.
Contributes to: #55
Checklist