-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(mempool): add from_iter func to TransactionPool #419
feat(mempool): add from_iter func to TransactionPool #419
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #419 +/- ##
=======================================
Coverage 83.63% 83.63%
=======================================
Files 37 37
Lines 1711 1711
Branches 1711 1711
=======================================
Hits 1431 1431
Misses 203 203
Partials 77 77 ☔ View full report in Codecov by Sentry. |
94776ad
to
255c53e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware and @elintul)
a discussion (no related file):
Can it be part of test_utils
crate?
It may be used in end-to-end tests.
WDTY?
crates/mempool/src/mempool_test.rs
line 22 at r2 (raw file):
pool.insert(tx).unwrap(); } pool
Suggestion:
let mut pool = TransactionPool::default();
txs.iter().for_each(|tx| pool.insert(tx));
pool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul and @MohammadNassar1)
a discussion (no related file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
Can it be part of
test_utils
crate?
It may be used in end-to-end tests.
WDTY?
I think it should be moved only when being used in end-to-end tests.
crates/mempool/src/mempool_test.rs
line 22 at r2 (raw file):
pool.insert(tx).unwrap(); } pool
I think the for
loop is more readable. In terms of runtime performance, they're supposed to be the same
https://doc.rust-lang.org/book/ch13-04-performance.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @elintul)
255c53e
to
90acb6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @elintul)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware)
crates/mempool/src/mempool_test.rs
line 15 at r3 (raw file):
use crate::mempool::{Mempool, MempoolInput, TransactionReference}; use crate::transaction_pool::TransactionPool;
Make the two from_iter
impl. consistent (either for
or for_each
in both).
This change is