feat: reorganizes use of clients and adds fallback clients #250
Conversation
ckartik
commented
Jul 17, 2024
•
ckartik
commented
- The existing clients used to communicate with arbitrary EVM-base machines have been spread throughout the code.
- Historically this would be ok, since we don't replicate the usage of these across too many services. However, given the critical nature and their susceptibility to exceptional returns, this PR purposes to move them to a single package, that can be throughly tested in place, and added to for stability improvements such as the fallback mechanism included in this PR.
|
|
||
| // BatchReceipts retrieves multiple receipts for a list of transaction hashes. | ||
| func (e *fallbackEVMHelper) BatchReceipts(ctx context.Context, txHashes []common.Hash) ([]Result, error) { | ||
| e.logger.Info("Starting BatchReceipts", "txHashes", txHashes) |
There was a problem hiding this comment.
There is some excessive logging here. You are printing the txn hashes array and then printing each element. Also, this is definitely not Info level.
| logger *slog.Logger | ||
| } | ||
|
|
||
| func NewFallbackEVMHelperWithLogger(clients []*rpc.Client, logger *slog.Logger) *fallbackEVMHelper { |
There was a problem hiding this comment.
I dont mind having this. But you will need to add a way for users to actually configure multiple RPC endpoints. Currently I dont think this can be instantiated.
| var err error | ||
|
|
||
| for i, client := range e.clients { | ||
| e.logger.Info("Attempting TraceTransaction", "client_index", i) |
There was a problem hiding this comment.
Move to debug if you want this log. Printing client index doesnt really give any information. Better to print the actual endpoints.
| var err error | ||
|
|
||
| for i, client := range e.clients { | ||
| e.logger.Info("Attempting batch call", "client_index", i) |
There was a problem hiding this comment.
Again client index wont give any helpful information. Also I would say this is not info log.
|
|
||
| // Retry individual failed transactions | ||
| for i, receipt := range receipts { | ||
| if receipt.Err != nil { |
There was a problem hiding this comment.
What other errors could be get here? We should definitely ignore ErrNotFound. These could be cancelled transactions. No matter how many times you retry you will get the same error. We need to go through the evmclient code and understand what errors we should retry. If the overall batch call has no error, I dont think we need to retry anything unless you can prove me wrong.
We should test this piece of code in integration test. Actually see if this is helping in any way. At the very least there should be unit tests here. Previously the function was pretty standard Batch call, but with all the retry logic there should be unit tests.
| var err error | ||
| for retries := 50; retries > 0; retries-- { | ||
| blkNum, err = i.EthClient.BlockNumber(ctx) | ||
| if err == nil { |
There was a problem hiding this comment.
We should only retry on errors related to rate limit. This is definitely not a good idea. There could be valid errors. Also, the name should be changed as this is not infinite anymore. You can take the amount of retries as argument.
| logger *slog.Logger | ||
| } | ||
|
|
||
| func NewInfiniteRetryL1Client(ethClient EthClient, logger *slog.Logger) EthClient { |
There was a problem hiding this comment.
If you want to move this code to a full-blown package, this should have unit tests.
| break | ||
| } | ||
| i.logger.Error("failed to get block number, retrying...", "error", err) | ||
| time.Sleep(2 * time.Second) |
There was a problem hiding this comment.
If there are rate limit errors, retries on fixed intervals would again cause the same errors. We should have exponential fallback of some sort.
| var err error | ||
| for retries := 50; retries > 0; retries-- { | ||
| hdr, err = i.EthClient.HeaderByNumber(ctx, number) | ||
| if err == nil { |
There was a problem hiding this comment.
Again, same comment for all functions here. We should figure out what errors we should look for to retry.
| "context" | ||
| ) | ||
|
|
||
| type laggerdL1Client struct { |
There was a problem hiding this comment.
Again, if you move this to a new pkg, you should add unit tests.
