[txpool] Initial impl of Transaction Pool #99
Conversation
rakita
commented
Dec 29, 2021
rakita
commented
- [txpool] Some checks on inclusion
- [txpool] Initial dependency insert
- [txpool] dep insertion needs testing
- [txpool] wip testing
- [txpool] some testing data
- [txpool] Initial subscriber. Some tidying up
- [txpool] cleanup
- [txpool] integration for fuel-core
- [txpool] find dependent. fmt
- [txpool] inclue utxo_id in dependency
- [txpool] Initial test
- [txpool] inclusion tests. Some fixes
- [txpool] fmt,clippy,cargo-sort
- [txpool] tx removal. subs tested
- [txpool] small cleanups and refactoring
|
CI build failed because we use branch of fuel-tx repo that is still private (i run locally Additionaly, depending on how we are going to structure Fuel project we can move |
rakita
force-pushed
the
rakita/txpool
branch
3 times, most recently
from
619b049
to
958d82a
Compare
cargo patch fails to load fuel-tx, i am blocked by fuel-tx not being published upstream. |
fuel-txpool/src/txpool.rs
Outdated
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn missing_dep_faulty_tx1_tx2() { |
There was a problem hiding this comment.
We should do better with our test names to clarify which requirements are being tested and what the expected behavior is. Referencing indices into helper data (tx1, tx2, etc) isn't a great way to clarify intent, and is harder to refactor if the test data changes over time.
Something like "insert_with_missing_input_coin_tx_fails" would be a lot more clear.
There was a problem hiding this comment.
These are data specific tests, if data changes it is like rewriting new test. But name of this particualr test is not the greatest, renamed it to: "fails_to_insert_tx2_with_missing_utxo_dependency_on_faulty_tx1"
There was a problem hiding this comment.
Ultimately what I'm getting at is we should rely more on conveying the requirements being tested, than the particular data being used. This is a good improvement, but some of the other tests could probably use similar improvements.
There was a problem hiding this comment.
What test do you have in mind? btw to speed up this, be free to push commit dirrectly to branch i wouldn't mind, it is esentially change about preferences of test naming.
|
After my last round of comments is addressed I'll be ready to merge this in. Sorry for such a long cycle on this one, maybe in the future we should try to break something like this up into smaller PRs. |
It is fine, it paid off to have longed cycle, you have found two bugs by just reviewing it. Not sure if this could be devided into smaller chunk as it covers core mechanisms that txpool is going to function by, i did best to somehow contain it and leave some work for later. I am currently diving into relayer, and will address/make changes probably later today or maybe tomorrow morning. |
* Run tests without std during CI * Remove lazy static from test-helpers * Rearrange ci workflows * Update ci.yml * Update ci.yml
