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

L2 gas fix tx v3 #528

Merged
merged 10 commits into from
Jul 5, 2024
Merged

L2 gas fix tx v3 #528

merged 10 commits into from
Jul 5, 2024

Conversation

mikiw
Copy link
Contributor

@mikiw mikiw commented Jul 2, 2024

Usage related changes

  • form now on resource_bounds input for l2_gas behave same as mainet

Checklist:

  • Checked out the contribution guidelines
  • Applied formatting - ./scripts/format.sh
  • No linter errors - ./scripts/clippy_check.sh
  • No unused dependencies - ./scripts/check_unused_deps.sh
  • No spelling errors - ./scripts/check_spelling.sh
  • Performed code self-review
  • Rebased to the latest commit of the target branch (or merged it into my branch)
    • Once you make the PR reviewable, please prefer merging over rebasing
  • Updated the docs if needed - ./website/README.md
  • Linked the issues resolvable by this PR - linking info
  • Updated the tests if needed; all passing - execution info

@mikiw mikiw linked an issue Jul 2, 2024 that may be closed by this pull request
@mikiw mikiw marked this pull request as draft July 2, 2024 15:51
@mikiw
Copy link
Contributor Author

mikiw commented Jul 3, 2024

I think that this is_max_fee_zero_value function should be renamed and abstracted as a validate() function, seems more robust and convenient but it's leading to error mapping problem...

@mikiw mikiw marked this pull request as ready for review July 3, 2024 08:42
@mikiw
Copy link
Contributor Author

mikiw commented Jul 3, 2024

abstractions improvements that could allow to return of different error types due to different validation problems is leading to major code refactoring for invoke, deploy, declare transactions which all rely on is_max_fee_zero_value() function

it leads to the idea of just a detailed error string:

    #[error(
        "{tx_type}: max_fee cannot be zero (exception is v3 transaction where l2 gas must be zero)"
    )]

but I'm not happy with that solution 😔

@mikiw mikiw requested a review from marioiordanov July 3, 2024 09:57
Copy link
Contributor

@marioiordanov marioiordanov left a comment

Choose a reason for hiding this comment

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

looks good

@mikiw mikiw merged commit a895269 into main Jul 5, 2024
1 check passed
@mikiw mikiw deleted the l2-gas-fix-tx-v3 branch July 5, 2024 09:53
3alpha pushed a commit that referenced this pull request Jul 5, 2024
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.

L2 ResourceBounds have to be removed
2 participants