feat: Add Unit Tests for Mempool Functionality

[eugypalu]

Resolves: #1355

Pull Request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build-related changes
• Documentation content changes
• Testing

What is the new behavior?

mempool unit tests

Does this introduce a breaking change?

• Yes
• No

[tcoratger]

@eugypalu Nice addition to our test suite. I would have two small remarks before merging:

• I think we will wait to merge Add KakarotTransactions trait to upstream send_raw_transaction #1352 first because there will be a small change in the provider but normally it will just require to modify very slightly the way you call the pool which should be in the ethclient and no longer in the ethprovider (but it is not a big change for your tests).
• On the other hand, I know that it is rather recommended to separate the tests to have unit test as much as possible and limit the scope of each test but in our case it is a little different. Each time we need to start Katana and the docker container so it still takes a little time. So I would be in favor of grouping some of your tests together, for example:
  • If I'm not mistaken, your test_mempool_pooled_transaction_hashes test checks the hashes of transactions added to the pool against the expected. So we could make these assertions directly at the end of your test_mempool_add_transactions test. This would add additional assertions but it would limit the number of tests.
  • I think you can do the same thing to group several tests together as soon as possible.

[eugypalu]

On the other hand, I know that it is rather recommended to separate the tests to have unit test as much as possible and limit the scope of each test but in our case it is a little different. Each time we need to start Katana and the docker container so it still takes a little time. So I would be in favor of grouping some of your tests together, for example:

• If I'm not mistaken, your test_mempool_pooled_transaction_hashes test checks the hashes of transactions added to the pool against the expected. So we could make these assertions directly at the end of your test_mempool_add_transactions test. This would add additional assertions but it would limit the number of tests.
• I think you can do the same thing to group several tests together as soon as possible.

Fixed, thanks

[tcoratger]

Nice, lgtm, pending #1352 to be merged

[tcoratger]

@eugypalu Here I think that you will have some conflicts to solve after the merge of #1352 (comment). Can you rebase? The main thing is that now you have to call mempool() and send_raw_transaction() from the eth_client and not from the eth_provider anymore.

[eugypalu]

@eugypalu Here I think that you will have some conflicts to solve after the merge of #1352 (comment). Can you rebase? The main thing is that now you have to call mempool() and send_raw_transaction() from the eth_client and not from the eth_provider anymore.

Hey @tcoratger, I've rebased and fixed the conflicts 👍