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

fix: balance print in errors #1986

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Conversation

AvivYossef-starkware
Copy link
Collaborator

@AvivYossef-starkware AvivYossef-starkware commented Jun 16, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.61%. Comparing base (b898b1c) to head (c3e874e).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1986   +/-   ##
=======================================
  Coverage   78.60%   78.61%           
=======================================
  Files          62       62           
  Lines        8892     8895    +3     
  Branches     8892     8895    +3     
=======================================
+ Hits         6990     6993    +3     
  Misses       1455     1455           
  Partials      447      447           

☔ 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 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware)


crates/blockifier/src/transaction/errors.rs line 36 at r1 (raw file):

    },
    #[error("Max fee ({}) exceeds balance ({}).", max_fee.0, balance_to_big_uint(balance_low, balance_high))]
    MaxFeeExceedsBalance { max_fee: Fee, balance_low: StarkFelt, balance_high: StarkFelt },

in both of these error variants: change the balance from two fields to a single BigUint field and create a BigUint when the error is created, no reason to do logic here

Suggestion:

    #[error(
        "L1 gas bounds (max amount: {max_amount}, max price: {max_price}) exceed balance ({}).",
        balance_to_big_uint(balance_low, balance_high)
    )]
    L1GasBoundsExceedBalance {
        max_amount: u64,
        max_price: u128,
        balance: BigUint,
    },
    #[error("Max fee ({}) exceeds balance ({}).", max_fee.0, balance_to_big_uint(balance_low, balance_high))]
    MaxFeeExceedsBalance { max_fee: Fee, balance: BigUint },

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

    let int = balance_to_big_uint(&StarkFelt::from(16_u64), &StarkFelt::from(1_u64));
    // 340282366920938463463374607431768211472 = 1 << 128 + 16
    assert!(format!("{}", int) == "340282366920938463463374607431768211472");

better IMO

Suggestion:

    assert!(format!("{}", int) == (uint!(u128::MAX) + 17).to_string());

Copy link
Collaborator Author

@AvivYossef-starkware AvivYossef-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: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)


crates/blockifier/src/transaction/errors.rs line 36 at r1 (raw file):

Previously, dorimedini-starkware wrote…

in both of these error variants: change the balance from two fields to a single BigUint field and create a BigUint when the error is created, no reason to do logic here

Done.


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

Previously, dorimedini-starkware wrote…

better IMO

Done.

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 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)


crates/blockifier/src/transaction/transactions_test.rs line 2022 at r2 (raw file):

    let int = balance_to_big_uint(&StarkFelt::from(16_u64), &StarkFelt::from(1_u64));
    assert!(
        format!("{}", int) == format!("{}", (BigUint::from(u128::MAX) + BigUint::from(17_u128)))

Suggestion:

(BigUint::from(u128::MAX) + BigUint::from(17_u128)).to_string()

Copy link
Collaborator Author

@AvivYossef-starkware AvivYossef-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: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)


crates/blockifier/src/transaction/transactions_test.rs line 2022 at r2 (raw file):

    let int = balance_to_big_uint(&StarkFelt::from(16_u64), &StarkFelt::from(1_u64));
    assert!(
        format!("{}", int) == format!("{}", (BigUint::from(u128::MAX) + BigUint::from(17_u128)))

Done.

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 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

@AvivYossef-starkware AvivYossef-starkware merged commit d924c79 into main Jun 17, 2024
9 checks passed
@AvivYossef-starkware AvivYossef-starkware deleted the aviv/fix_balance_error_print branch June 17, 2024 07:54
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
…mplicity (starkware-libs#1986)

# Description

currently, the `saya_getTransactionsExecutions` API is based on a cursor to determine how the traces are being fetched. 

https://github.com/dojoengine/dojo/blob/855da3112c87faea87646db5a406ac77b4daf149/crates/saya/provider/src/rpc/mod.rs#L160-L175

one issue it currently has it that the api params does not put any constraint on whether the returned traces are guarantee to only be from a specific block. however, the implementation seems to indicate otherwise.

anyway, this pr basically want to remove the complexity of the pagination logic (which we dont even need!) and to replace it with a basic `get Xs from block Y` api instead.


so we can simplify the endpoint itself to just return the traces per block instead of based on a cursor. the cursor is only adding more complexity to the api itself.

## Related issue

<!--
Please link related issues: Fixes #<issue_number>
More info: https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
-->

## Tests

<!--
Please refer to the CONTRIBUTING.md file to know more about the testing process. Ensure you've tested at least the package you're modifying if running all the tests consumes too much memory on your system.
-->

- [ ] Yes
- [ ] No, because they aren't needed
- [ ] No, because I need help

## Added to documentation?

<!--
If the changes are small, code comments are enough, otherwise, the documentation is needed. It
may be a README.md file added to your module/package, a DojoBook PR or both.
-->

- [ ] README.md
- [x] [Dojo Book](https://github.com/dojoengine/book)
- [ ] No documentation needed

## Checklist

- [x] I've formatted my code (`scripts/prettier.sh`, `scripts/rust_fmt.sh`, `scripts/cairo_fmt.sh`)
- [x] I've linted my code (`scripts/clippy.sh`, `scripts/docs.sh`)
- [x] I've commented my code
- [ ] I've requested a review after addressing the comments
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
…#1987)

# Description

basically refactor the tx traces fetching logic to using the new API introduced in starkware-libs#1986.

ref: dojoengine/dojo#1986 (comment) 

to simplify the process of extracting the l1 -> l2 messages bcs currently we have to compute the message hash in order to find the corresponding tx as seen here:

https://github.com/dojoengine/dojo/blob/855da3112c87faea87646db5a406ac77b4daf149/crates/saya/core/src/prover/program_input.rs#L54-L74

these changes (including starkware-libs#1986) make the process more straightforward as we can just filter based on the tx hash which is more straightforward.

## Related issue

<!--
Please link related issues: Fixes #<issue_number>
More info: https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
-->

## Tests

<!--
Please refer to the CONTRIBUTING.md file to know more about the testing process. Ensure you've tested at least the package you're modifying if running all the tests consumes too much memory on your system.
-->

- [ ] Yes
- [x] No, because they aren't needed
- [ ] No, because I need help

## Added to documentation?

<!--
If the changes are small, code comments are enough, otherwise, the documentation is needed. It
may be a README.md file added to your module/package, a DojoBook PR or both.
-->

- [ ] README.md
- [ ] [Dojo Book](https://github.com/dojoengine/book)
- [x] No documentation needed

## Checklist

- [x] I've formatted my code (`scripts/prettier.sh`, `scripts/rust_fmt.sh`, `scripts/cairo_fmt.sh`)
- [x] I've linted my code (`scripts/clippy.sh`, `scripts/docs.sh`)
- [x] I've commented my code
- [ ] I've requested a review after addressing the comments
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