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

chore(sequencer): refactor fees #1739

Closed
wants to merge 7 commits into from
Closed

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Oct 24, 2024

Summary

Added trait for fee components to allow generic usage of them.

Background

Previously, there was a lot of boilerplate required when testing fee components, since we are usually testing them for each and every action. Additionally, this introduces a lot of room for human error, since it's easy to mismatch an action and its associated methods. This change aims to rely more heavily on trait methods and helper functions, so that generic types can be used to eliminate boilerplate and room for error.

Changes

  • Added new FeeComponents trait and implemented it for all fee components.
  • Added new type and method to FeeHandler trait.
  • Changed all actions in the fees request (fees::query::get_fees_for_transaction) to use helper functions with generic inputs.
  • Refactored fee change tests to use helper function and macro, as opposed to generated tests. The aim of this is to make it more difficult to make manual mistakes in the future, while still making debugging of these tests easier.
  • Refactored round trip state read/write tests.

Testing

Passing all tests

Related Issues

closes #1715

@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Oct 24, 2024
@ethanoroshiba ethanoroshiba marked this pull request as ready for review October 25, 2024 14:24
@ethanoroshiba ethanoroshiba requested a review from a team as a code owner October 25, 2024 14:24
.ok_or_eyre(
"fees not found for `ValidatorUpdate` action, hence it is disabled",
)?;
get_or_init_fees::<ValidatorUpdate, _, _>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

it doesn't look like there's a point to calling get_or_init_fees in this action or any of the ones below that are feeless, as they don't update fees_by_asset, which is the point of this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done so that the check would fail if the transaction would fail during execution (due to a given action not having fees). I think it may be worth it to keep this logic since it is used in mempool as well as check_and_execute and may catch a failure-bound transaction before it gets executed.

@ethanoroshiba
Copy link
Contributor Author

Closing in favor of #1794

@ethanoroshiba ethanoroshiba deleted the ENG-944/refactor_fees branch November 13, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sequencer: make fee change testing easier to debug
2 participants