Skip to content
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

Merged
merged 1 commit into from
Apr 30, 2024
Merged

feat: add Mempool and its API #43

merged 1 commit into from
Apr 30, 2024

Conversation

giladchase
Copy link
Collaborator

@giladchase giladchase commented Apr 11, 2024

This change is Reviewable

Copy link
Collaborator

@elintul elintul left a 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).

Copy link
Collaborator

@elintul elintul left a 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>,

@MohammadNassar1 MohammadNassar1 force-pushed the mohammad/mempool/implement-priority-queue branch from 7d8faf1 to eca5cf1 Compare April 15, 2024 15:17
Copy link
Contributor

@ayeletstarkware ayeletstarkware left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

@MohammadNassar1 MohammadNassar1 deleted the branch main April 18, 2024 07:23
@giladchase giladchase reopened this Apr 18, 2024
@giladchase giladchase changed the base branch from mohammad/mempool/implement-priority-queue to main April 18, 2024 09:39
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 58.88%. Comparing base (eba70cb) to head (17d9cd1).

Files Patch % Lines
crates/mempool/src/mempool.rs 0.00% 15 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ayeletstarkware ayeletstarkware left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

Copy link
Collaborator

@elintul elintul left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

@giladchase giladchase merged commit 27b550e into main Apr 30, 2024
8 checks passed
@giladchase giladchase deleted the gilad/mempool-api branch April 30, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants