chore(sequencer): further refactor of fees #1794
Conversation
| pub type TransferFeeComponents = FeeComponents<Transfer>; | ||
| pub type RollupDataSubmissionFeeComponents = FeeComponents<RollupDataSubmission>; | ||
| pub type Ics20WithdrawalFeeComponents = FeeComponents<Ics20Withdrawal>; | ||
| pub type InitBridgeAccountFeeComponents = FeeComponents<InitBridgeAccount>; | ||
| pub type BridgeLockFeeComponents = FeeComponents<BridgeLock>; | ||
| pub type BridgeUnlockFeeComponents = FeeComponents<BridgeUnlock>; | ||
| pub type BridgeSudoChangeFeeComponents = FeeComponents<BridgeSudoChange>; | ||
| pub type IbcRelayFeeComponents = FeeComponents<IbcRelay>; | ||
| pub type ValidatorUpdateFeeComponents = FeeComponents<ValidatorUpdate>; | ||
| pub type FeeAssetChangeFeeComponents = FeeComponents<FeeAssetChange>; | ||
| pub type FeeChangeFeeComponents = FeeComponents<FeeChange>; | ||
| pub type IbcRelayerChangeFeeComponents = FeeComponents<IbcRelayerChange>; | ||
| pub type SudoAddressChangeFeeComponents = FeeComponents<SudoAddressChange>; | ||
| pub type IbcSudoChangeFeeComponents = FeeComponents<IbcSudoChange>; |
There was a problem hiding this comment.
I'm a bit torn with these type aliases. While they do keep with our current naming and that may be reason enough to have them, I think it may be more representative of the new generic structure to continue using FeeComponents<ACTION_NAME> where the aliases are used. There are still many places in the refactor (such as fees/state_ext) where we do not use the aliases, both by necessity and not, so I think it could be helpful to outside readers to eliminate the indirection and keep it consistent across the code base.
There was a problem hiding this comment.
Yeah - I was mainly trying to reduce churn by providing these aliases, but I was 50-50 myself on adding them :) Given that you have enough of an opinion on this to have added a comment (even if your opinion is weakly held) I've gone with removing them in 1316e07 of #1811. It's a standalone commit, so if others disagree, it can be reverted easily enough.
| /// The Pascal-case type name, e.g. `RollupDataSubmission`. | ||
| // NOTE: We only require this function due to `IbcRelay` not implementing `Protobuf`. | ||
| fn name() -> &'static str; | ||
|
|
||
| /// The full name including the protobuf package, e.g. | ||
| /// `astria.protocol.transaction.v1.RollupDataSubmission`. | ||
| // NOTE: We only require this function due to `IbcRelay` not implementing `Protobuf`. | ||
| fn full_name() -> String; | ||
|
|
||
| /// The snake-case type name, e.g. `rollup_data_submission`. | ||
| fn snake_case_name() -> &'static str; |
There was a problem hiding this comment.
While it makes perfect sense that we need this functionality for the IbcRelay action, I think it is a bit odd to include this in FeeHandler, especially when these methods are applicable to many other things than fees. Perhaps we could do something like make a new trait ProstNameExt or NameExt, implement it for all actions which implement protobuf and add our own implementation for IbcRelay, then require it for FeeHandler? Just a thought, and may also be out of the scope of this PR.
There was a problem hiding this comment.
I guess I'd rather keep this functionality as restricted as possible until we have a concrete use-case for adding it more widely. The core lib is already pretty busy, and I'd rather avoid adding another trait there for it to only be consumed in one specific place in the sequencer.
|
Closed in favour of #1811. |
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)
FeeComponentstructs into a single one with a generic arg reduces a lot of the boilerplate code.Changes
FeeComponents<T>whereTwill be some action type.FeeHandlertrait 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.