GH-744-Review-4

[Syther007]

No description provided.

[bertllll]

Continue node/src/blockchain/blockchain_interface/blockchain_interface_web3/lower_level_interface_web3.rs ln. 40

[bertllll]

I have a couple of points about possibilities for minor optimisations.

• line 401: move is redundant
• if length != transactions_found { coupled with "Aborting scanning; {} transactions succeed and {} transactions failed" is controversial. It's because the scan hasn't differed from a successful one in the length of the process (you do the same amount of work to figure out all the transactions were successful as you do if there are some pending or failed transactions), therefore speaking of "aborting" is misinterpreting the truth. We didn't abort. We completed everything.

However there is more than that: you cannot count the succeeded transactions only and then assume that the rest of them have failed with no exception. That's not right. Be aware that the rest is a mix of legitimately failed transactions, yes, but also those still pending. I'm asking, do you think it's appropriate having the pending transactions counted in the failures? Because that's what you're doing in the debug!() log down here.

If you still think we'd benefit from having that log, I'd recommend to keep track of all three categories: Confirmed, Pending, Failed.

• inside the function process_transaction_receipts(), there is a passage in the chain of Future-devoted methods that says .map_err(|e| e) which is a redundant call with no effect, you can delete that line
• in the impl of get_transaction_receipt_in_batch(), you can see this snipped:

    let _ = hash_vec.into_iter().map(|hash| {
            self.web3_batch.eth().transaction_receipt(hash);
        });

Honestly, I'm surprised that this code even does anything because I've been taught that iterators in Rust are "lazy" and therefore their code isn't executed until a certain terminal method is reached and performed. Most of the time this method is the renowned collect(), then fold(), partition() and also for_each().
As you have used none of them in here, it makes me wonder how you got your code working, assuming this has proper test coverage. Anyway, without having to argue over this, we can easily put end to this question with the replacement of .map() by .for_each(). This swap will also rule out the assignment to an anonymous variable since for_each() is not allowed to return a value, naturally: It's meant to allow side effects only. I hope you'll be able to make out what I instruct you for.

[utkarshg6]

Thanks for this change! 😃

I really appreciated your insights on tracking the different transaction statuses and the iterator usage. I went ahead and implemented those suggestions.

As for the move and the redundant error mapping, I decided to leave those as is to keep the original function signature intact.

[bertllll]

Yeah. I commented on this one in on a different occasion. It's irrelevant if you either do or don't have this line in. Needless.

[utkarshg6]

The signature issue may cause a lot of trouble. I don't know if it's worth it.

[bertllll]

Do you think you could pack this whole fold for the debug into a separate function.

I've got this in my mind:

You can use the trick with

logger.debug(||{
 <your function returns a String (the debug message)>
})

Therefore:

fn transaction_receipts_results_debug(transaction_receipts_results: <The proper type>){
       logger.debug({
              let (successful_count, failed_count, pending_count) = transaction_receipts_results
                       .iter()
                       .fold((0, 0, 0), |(success, fail, pending), transaction_receipt| {
                               match transaction_receipt {
                                    TransactionReceiptResult::RpcResponse(tx_receipt) => match tx_receipt.status {
                                        TxStatus::Failed => (success, fail + 1, pending),
                                        TxStatus::Pending => (success, fail, pending + 1),
                                        TxStatus::Succeeded(_) => (success + 1, fail, pending),
                                    },
                                   TransactionReceiptResult::LocalError(_)=> (success, fail, pending + 1),
                              }
              format!("Scan results: Successful: {successful_count}, Pending: {pending_count}, Failed: {failed_count}");
      })
}

A praise for that nice use of .fold(). 😉