chore(sequencer): refactor fees #1739
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noot
reviewed
Nov 4, 2024
noot
reviewed
Nov 4, 2024
noot
reviewed
Nov 4, 2024
| .ok_or_eyre( | ||
| "fees not found for `ValidatorUpdate` action, hence it is disabled", | ||
| )?; | ||
| get_or_init_fees::<ValidatorUpdate, _, _>( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Closing in favor of #1794 |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 6, 2024
## Summary This reduces a lot of boilerplate around fees. ## Background The boilerplate code is very prone to copy-paste errors. Changing the 14 different (but structurally identical) `FeeComponent` structs into a single one with a generic arg reduces a lot of the boilerplate code. This PR replaces #1739 (an initial, more restricted refactor) and #1794 (a PR on top of #1739). It comprises the net changes of these two PRs in [the first commit](a8ec424), followed by most of the changes suggested by @eoroshiba's review of #1794 in [the second commit](c92dc98). [The third commit](1316e07) is also in response to @eoroshiba's suggestion to remove the fee aliases, and is a single commit in case others prefer that change to be reverted. ## Changes - In core, replaced fee component structs with a new `FeeComponents<T>` where `T` will be some action type. - In sequencer, changed the `FeeHandler` trait fairly radically to support the new form of fees. ## Testing Existing tests are sufficient - there should be no changed functionality to the business logic. ## Changelogs No updates required. ## Related Issues Closes #1715.
ethanoroshiba
pushed a commit
that referenced
this pull request
Dec 6, 2024
## Summary This reduces a lot of boilerplate around fees. ## Background The boilerplate code is very prone to copy-paste errors. Changing the 14 different (but structurally identical) `FeeComponent` structs into a single one with a generic arg reduces a lot of the boilerplate code. This PR replaces #1739 (an initial, more restricted refactor) and #1794 (a PR on top of #1739). It comprises the net changes of these two PRs in [the first commit](a8ec424), followed by most of the changes suggested by @eoroshiba's review of #1794 in [the second commit](c92dc98). [The third commit](1316e07) is also in response to @eoroshiba's suggestion to remove the fee aliases, and is a single commit in case others prefer that change to be reverted. ## Changes - In core, replaced fee component structs with a new `FeeComponents<T>` where `T` will be some action type. - In sequencer, changed the `FeeHandler` trait fairly radically to support the new form of fees. ## Testing Existing tests are sufficient - there should be no changed functionality to the business logic. ## Changelogs No updates required. ## Related Issues Closes #1715.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
FeeComponentstrait and implemented it for all fee components.FeeHandlertrait.fees::query::get_fees_for_transaction) to use helper functions with generic inputs.Testing
Passing all tests
Related Issues
closes #1715