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

refactor(mempool): add tx! macro #889

Merged
merged 1 commit into from
Sep 22, 2024
Merged

Conversation

elintul
Copy link
Collaborator

@elintul elintul commented Sep 19, 2024

This change is Reviewable

Copy link
Collaborator Author

@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: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @giladchase and @MohammadNassar1)


crates/mempool/src/mempool_test.rs line 155 at r1 (raw file):

/// Creates a valid input for mempool's `add_tx` with optional default values.
/// Usage:

Comments became outdated; usages can be seen in tests.

Code quote:

Usage:

crates/mempool/src/mempool_test.rs line 175 at r1 (raw file):

        let account = Account { sender_address, state: AccountState {nonce: account_nonce}};

        let tx = create_executable_tx(

Using tx!(...) macro instead.

Code quote:

create_executable_tx

crates/mempool/src/mempool_test.rs line 193 at r1 (raw file):

    (tip: $tip:expr, tx_hash: $tx_hash:expr, sender_address: $sender_address:expr) => {
        add_tx_input!(tip: $tip, tx_hash: $tx_hash, sender_address: $sender_address, tx_nonce: 0_u8, account_nonce: 0_u8)
    };

Deleted rules are unused.

@elintul elintul force-pushed the elin/mempool/test/add-tx-macro branch 2 times, most recently from b5ecb8e to 072c45f Compare September 19, 2024 12:55
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 3.30%. Comparing base (7da2d63) to head (95d6619).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #889       +/-   ##
==========================================
- Coverage   74.29%   3.30%   -70.99%     
==========================================
  Files         358      87      -271     
  Lines       36288   11273    -25015     
  Branches    36288   11273    -25015     
==========================================
- Hits        26960     373    -26587     
- Misses       7190   10882     +3692     
+ Partials     2138      18     -2120     
Flag Coverage Δ
3.30% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @elintul, @giladchase, and @MohammadNassar1)


crates/mempool/src/mempool_test.rs line 193 at r1 (raw file):

Previously, elintul (Elin) wrote…

Deleted rules are unused.

is it possible to split to another PR?

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.

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @elintul, @giladchase, and @MohammadNassar1)


crates/mempool/src/mempool_test.rs line 175 at r1 (raw file):

Previously, elintul (Elin) wrote…

Using tx!(...) macro instead.

Is this function still being used anywhere, or can it be removed?

@elintul elintul force-pushed the elin/mempool/test/add-tx-macro branch from 072c45f to 2745407 Compare September 19, 2024 13:16
Copy link
Collaborator Author

@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: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)


crates/mempool/src/mempool_test.rs line 175 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Is this function still being used anywhere, or can it be removed?

In the tx!(...) macro (+ probably gateway tests).


crates/mempool/src/mempool_test.rs line 193 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

is it possible to split to another PR?

Done, PTAL.

Copy link
Contributor

@MohammadNassar1 MohammadNassar1 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 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)


crates/mempool/src/mempool_test.rs line 182 at r4 (raw file):

/// Creates an input for `add_tx` with the given field subset (the rest receive default values).
macro_rules! add_tx_input {

wdyt?

Suggestion:

mempool_input

crates/mempool/src/mempool_test.rs line 799 at r4 (raw file):

fn test_commit_block_from_different_leader() {
    // Setup.
    let tx_address0_nonce3 = tx!(tip: 1, tx_hash: 1, sender_address: "0x0", tx_nonce: 3_u8);

Now it's not clear that this transaction is not ready.

Code quote:

let tx_address0_nonce3 = tx!(tip: 1, tx_hash: 1, sender_address: "0x0", tx_nonce: 3_u8);

Copy link
Collaborator Author

@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, 4 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)


crates/mempool/src/mempool_test.rs line 182 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

wdyt?

Maybe later.


crates/mempool/src/mempool_test.rs line 799 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Now it's not clear that this transaction is not ready.

It's clear from how mempool content is built.

Copy link
Contributor

@MohammadNassar1 MohammadNassar1 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, 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)


crates/mempool/src/mempool_test.rs line 799 at r4 (raw file):

Previously, elintul (Elin) wrote…

It's clear from how mempool content is built.

means that if someone needs to debug it, they will have to scroll down to see what is being passed to MempoolContent.

The decision is yours - approving this PR.

Copy link
Contributor

@MohammadNassar1 MohammadNassar1 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

Copy link
Contributor

@MohammadNassar1 MohammadNassar1 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, @elintul, and @giladchase)

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.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul, @giladchase, and @MohammadNassar1)


crates/mempool/src/mempool_test.rs line 155 at r1 (raw file):

Previously, elintul (Elin) wrote…

Comments became outdated; usages can be seen in tests.

The comments are really helpful when writing the code. When hovering over the macro call, they show all the available options. Why do you think they should be deleted?


crates/mempool/src/mempool_test.rs line 799 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

means that if someone needs to debug it, they will have to scroll down to see what is being passed to MempoolContent.

The decision is yours - approving this PR.

I think it might be confusing in some tests.

Copy link
Collaborator Author

@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, 2 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)


crates/mempool/src/mempool_test.rs line 155 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

The comments are really helpful when writing the code. When hovering over the macro call, they show all the available options. Why do you think they should be deleted?

Why is a comment better than code usage?


crates/mempool/src/mempool_test.rs line 799 at r4 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I think it might be confusing in some tests.

What is more confusing, is that the test writer needs to think about account nonce for non-add_tx tests. We're creating Account object, just to throw it away in the same line, so it's not actually needed, and might be inconsistent since it isn't used. There must be a better way.
To debug a failed test, one would have to read it regardless.
The queue represents the account nonces.

  Added `tx!()` macro for `non-`add_tx` tests
  (cleans them from `account_nonce` confusing unused field).
@elintul elintul force-pushed the elin/mempool/test/add-tx-macro branch from 37ff471 to 95d6619 Compare September 21, 2024 15:35
Copy link
Contributor

@MohammadNassar1 MohammadNassar1 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 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)

@elintul elintul merged commit 57f5dbc into main Sep 22, 2024
14 checks passed
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.

3 participants