Transactions with blacklisted eth receivers are included in a batch profitability comparison

[stana-miric]

When building a batch, transactions are first collected from the outgoing transaction pool in order to calculate the total bridge fee and check if a more profitable batch of the same token type already exists in the pool. When doing this, there is no check if the translation’s destination address is included in the blacklist or not. Later, when transactions are collected to be included in the batch, additional blacklist filtering is applied. This can cause potential creation of the less profitable batch than the one of the same token type with the biggest nonce already in the pool. Also, the double retrieval of the calculations seems unnecessary.

[jkilpatr]

This is an excellent catch! Thank you for taking the time to investigate the issue.

I'll look into it myself today and follow up with more comments.

[jkilpatr]

So I've dug into this and it seems very straightforward to add the same check from here to here

This seems like a 20 minute patch, maybe an hour or two if you want to make the blacklist check more generic and avoid code duplciation.

From your comments I assume you're digging into reducing the repeated fee computation that's currently a big part of batch creation? Batch creation is pretty complex, and optimizing it will quickly baloon into a lot of work, not that I don't think it's important just wondering on priorities.

Thanks for finding and reporting this bug!

[stana-miric]

Agree that refactoring should be avoided at this point and with the proposed solution.
We fixed this and adapted tests and 2 other minor issues are resolved which are related to the building batches.

One is related to the GetFees function(the sum will always be 0),
and the other one with the GetLastOutgoingBatchByTokenType (the lastBatch will always point to the last batch in the array)
Also, here should check be greater or equal(.GTE)?

[jkilpatr]

GT or GTE either should work there. Good catch on get fees as well.

If you have the fixes locally could you get them up as PRs? I'd love to review/merge so that we can have them ready for the next upgrade.

[jkilpatr]

resolved in #57