feat(l1-recover): Recovery of Main Node / External Node from L1 #3174
Conversation
| log: &Log, | ||
| ) -> PriorityOpId { | ||
| assert_eq!(event_type, RollupEventType::NewPriorityTx); | ||
| L1Tx::try_from(log.clone()).unwrap().common_data.serial_id |
| .await | ||
| .unwrap(); | ||
| let mut end_block = last_l1_block_number; | ||
| // we use step override as such big step value guarantees that we don't miss |
There was a problem hiding this comment.
I don't understand this comment. Can you elaborate?
| end_block = last_l1_block_number; | ||
| let mut current_block = start_block; | ||
| loop { | ||
| let filter_to_block = cmp::min(current_block + step - 1, end_block); |
There was a problem hiding this comment.
fyi, instead of cmp::min you can use min method (no imports required), for example
let filter_to_block = end_block.min(...).
| ) | ||
| .await; | ||
| let tx = | ||
| L1Fetcher::get_transaction_by_hash(&self.eth_client, logs[0].transaction_hash.unwrap()) |
There was a problem hiding this comment.
if you don't want to make these methods of L1Fetcher, then consider wrapping eth_client into yet another type instead and making functions like get_transaction_by_hash methods of that new type instead.
| hyperchain_contract() | ||
| } | ||
|
|
||
| fn commit_functions() -> Result<Vec<Function>> { |
There was a problem hiding this comment.
any specific reason why those functions are members of L1Fetcher class?
| ) -> Vec<CommitBlock> { | ||
| let end_block = L1Fetcher::get_last_l1_block_number(&self.eth_client) | ||
| .await | ||
| .unwrap(); |
| RollupEventsFilter::L1BatchNumber(l1_batch_number + 1), | ||
| ) | ||
| .await | ||
| .unwrap() |
There was a problem hiding this comment.
Return an error. In general in places that you can statically guarantee that an error will never occur (and therefore unwrap call is legitimate), document in a comment why it is the case.
| // This code is compatible with both Infura and Alchemy API providers. | ||
| // Note: we don't handle rate-limits here - assumption is that we're never going to hit them. | ||
| if let Err(err) = &result { | ||
| tracing::warn!("Provider returned error message: {err}"); |
There was a problem hiding this comment.
in general, by default choose "{err:#}" for error printing, as "{err}" tends to not display the cause.
| } else if should_retry(err_code, err_message) && retries_left > 0 { | ||
| tracing::warn!("Retrying. Retries left: {retries_left}"); | ||
| result = self | ||
| .get_events_inner(from, to, topics1, topics2, addresses, retries_left - 1) |
There was a problem hiding this comment.
nit: it seems wasteful to use recursion for retries instead of a loop, no?
Also the layout/block nesting of this function could be improved, by inverting success and failure branches, i.e.:
let err = match result {
Ok(res) => return res;
Err(err) => err
};
// error handling.
|
|
||
| async fn get_logs( | ||
| &self, | ||
| start_block: U64, |
There was a problem hiding this comment.
nit: it would be nice to have a dedicated type for L1 block numbers (or at least an alias), instead of plain U64
| .ok_or_else(|| anyhow!("found latest block, but it contained no block number")) | ||
| } | ||
|
|
||
| async fn retry_call<T, E, Fut>(callback: impl Fn() -> Fut, err: L1FetchError) -> Result<T> |
There was a problem hiding this comment.
why not map_err() at the call site?
tomg10
changed the title
2 participants
No description provided.