-
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: add Mempool and its API #43
Conversation
cac9f86
to
62b8414
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 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)
crates/mempool/src/mempool.rs
line 24 at r2 (raw file):
/// Update the mempool's internal state according to the committed block's transactions. /// This method also resolves nonce gaps and updates account balances.
Suggestion:
/// This method also updates internal state (resolves nonce gaps, updates account balances).
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, 3 unresolved discussions (waiting on @ayeletstarkware and @giladchase)
crates/mempool/src/mempool.rs
line 19 at r2 (raw file):
/// Adds a new transaction to the mempool. /// TODO: support fee escalation and transactions with future nonces. pub fn add_tx(&mut self, tx: InternalTransaction) -> MempoolResult<()> {
Where, AccountState
includes nonce and balance.
Suggestion:
pub fn add_tx(&mut self, tx: InternalTransaction, account_state: AccountState) -> MempoolResult<()>
crates/mempool/src/mempool.rs
line 31 at r2 (raw file):
block_number: u64, txs_in_block: &[TransactionHash], state_changes: StateChanges,
Suggestion:
account_states: HashMap<ContractAddress, AccountState>,
62b8414
to
65b1d7d
Compare
7d8faf1
to
eca5cf1
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 3 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)
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 @giladchase)
65b1d7d
to
5dd5cb6
Compare
5dd5cb6
to
17d9cd1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
==========================================
- Coverage 61.93% 58.88% -3.06%
==========================================
Files 10 11 +1
Lines 289 304 +15
Branches 289 304 +15
==========================================
Hits 179 179
- Misses 105 120 +15
Partials 5 5 ☔ View full report in Codecov by Sentry. |
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 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)
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 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)
This change is