Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Fix actual fee for l1 handler: it equals the L2 cost. #1210

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

noaov1
Copy link
Collaborator

@noaov1 noaov1 commented Dec 4, 2023

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (d50a7db) 70.39% compared to head (9d0711d) 70.51%.
Report is 2 commits behind head on main.

Files Patch % Lines
crates/blockifier/src/fee/actual_cost.rs 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1210      +/-   ##
==========================================
+ Coverage   70.39%   70.51%   +0.11%     
==========================================
  Files          58       58              
  Lines        7131     7159      +28     
  Branches     7131     7159      +28     
==========================================
+ Hits         5020     5048      +28     
  Misses       1692     1692              
  Partials      419      419              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware 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 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @noaov1 and @Yoni-Starkware)


crates/blockifier/src/transaction/transactions_test.rs line 1402 at r1 (raw file):

        TransactionExecutionError::TransactionFeeError(
            TransactionFeeError::InsufficientL1Fee { paid_fee, actual_fee, })
            if paid_fee == Fee(0) && actual_fee == Fee(1732700000000000)

isn't this misleading...? the max_fee is 1

Code quote:

1732700000000000

Copy link
Collaborator Author

@noaov1 noaov1 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: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/src/transaction/transactions_test.rs line 1402 at r1 (raw file):

Previously, dorimedini-starkware wrote…

isn't this misleading...? the max_fee is 1

Where do you see that max_fee = 1?

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware 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: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)


crates/blockifier/src/transaction/transactions_test.rs line 1330 at r1 (raw file):

    let value = StarkFelt::from_u128(0x44);
    let calldata = calldata![from_address, key, value];
    let tx = l1_handler_tx(&calldata, Fee(1));

@noaov1
If max_fee=1 and actual_fee>1 I would expect an error... no?

Code quote:

let tx = l1_handler_tx(&calldata, Fee(1));

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware 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: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)


crates/blockifier/src/transaction/transactions_test.rs line 1402 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Where do you see that max_fee = 1?

tagged you

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware 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: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @noaov1 and @Yoni-Starkware)


crates/blockifier/src/transaction/transactions_test.rs line 1403 at r1 (raw file):

            TransactionFeeError::InsufficientL1Fee { paid_fee, actual_fee, })
            if paid_fee == Fee(0) && actual_fee == Fee(1732700000000000)
    );

I don't follow the logic.
If paid_fee is 1, the tx is successful; but if it's zero, we get this error, which seems to indicate only a minimum of paid_fee=1732700000000000 would allow the tx to pass.
Is this just future-preparation for when the error is more meaningful?

Code quote:

        TransactionExecutionError::TransactionFeeError(
            TransactionFeeError::InsufficientL1Fee { paid_fee, actual_fee, })
            if paid_fee == Fee(0) && actual_fee == Fee(1732700000000000)
    );

Copy link
Collaborator Author

@noaov1 noaov1 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: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/src/transaction/transactions_test.rs line 1330 at r1 (raw file):

Previously, dorimedini-starkware wrote…

@noaov1
If max_fee=1 and actual_fee>1 I would expect an error... no?

Fee(1) is the fee paid on l1. Currently, we only verify it is positive. L1 handlers do not have a max_fee field. We set it to 0 (in the account_tx_context for our own use.
In the future: we will verify paid_fee_on_l1 >= required_fee_on_l2.


crates/blockifier/src/transaction/transactions_test.rs line 1403 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I don't follow the logic.
If paid_fee is 1, the tx is successful; but if it's zero, we get this error, which seems to indicate only a minimum of paid_fee=1732700000000000 would allow the tx to pass.
Is this just future-preparation for when the error is more meaningful?

Yes

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware 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 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

@noaov1 noaov1 merged commit 77c1244 into main Dec 4, 2023
11 checks passed
@noaov1 noaov1 deleted the noa/fix_actual_fee_for_l1_handlers branch December 4, 2023 13:52
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
* make world upgradeable

* update records_contract address
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants