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: Simplify transaction handling and gas calculations #2262

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

cypherpepe
Copy link

Linked Issues/PRs

N/A

Description

This refactor simplifies transaction handling and gas calculations by:

  • Removing redundant transaction type checks in MaybeCheckedTransaction::id() by centralizing the logic.
  • Refactoring the max_gas() method to handle common transaction types (Script, Create, Upgrade, Upload, and Blob) in a single match arm, reducing code duplication.
  • Preserving special handling for Mint transactions, which do not have max_gas.
  • Improving overall code clarity and maintainability.

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

- Removed redundant transaction type checks in MaybeCheckedTransaction::id() by centralizing the logic.

- Refactored max_gas() method to handle common transaction types (Script, Create, Upgrade, Upload, and Blob) in a single match arm, reducing code duplication.

- Preserved special handling for Mint transactions, which do not have max_gas.

- Improved overall code clarity and maintainability.
Moved transaction JSON deserialization into a separate function to reduce code duplication and improve code clarity across all transaction-related commands. This refactor makes the code easier to maintain and enhances readability by centralizing the JSON parsing logic. It affects all commands that process transactions, such as Submit, EstimatePredicates, DryRun, and others.
This update refactors the test suite by extracting the common logic for creating tests into a helper function create_test. This helps reduce code duplication and improves code readability and maintainability across the test suite.
@xgreenx xgreenx marked this pull request as draft October 1, 2024 08:39
@xgreenx
Copy link
Collaborator

xgreenx commented Oct 1, 2024

Could you try to run ./ci_checks.sh locally and fix all issue, please?=)

MitchTurner
MitchTurner previously approved these changes Oct 1, 2024
Copy link
Member

@MitchTurner MitchTurner left a comment

Choose a reason for hiding this comment

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

LGTM

@CLAassistant
Copy link

CLAassistant commented Oct 1, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

Please revert formatting changes, it is too many modifications

@MitchTurner
Copy link
Member

There seem to be a lot of formatting and other changes that weren't there when I first reviewed.

@cypherpepe
Copy link
Author

Could you try to run ./ci_checks.sh locally and fix all issue, please?=)

Hi! After running the check and fixing the issues in my commits, all problems have been resolved. Please take a look at the changes and let me know if anything else needs attention. Thanks for your patience!

@cypherpepe
Copy link
Author

There seem to be a lot of formatting and other changes that weren't there when I first reviewed.

The script ./ci_checks.sh detected type incompatibilities, and fixing them resulted in an increase in the amount of code. These modifications were necessary for the code to function correctly.

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.

4 participants