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.

Sign up for GitHub

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

Jump to bottom

chore(sequencer): further refactor of fees #1794

Closed
wants to merge 1 commit into from
Closed

chore(sequencer): further refactor of fees #1794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 85 additions & 68 deletions crates/astria-core/src/protocol/fees/v1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,33 @@
use std::{
fmt::{
self,
Debug,
Formatter,
},
marker::PhantomData,
};

use penumbra_ibc::IbcRelay;
use prost::Name as _;

use crate::{
generated::protocol::fees::v1 as raw,
primitive::v1::asset,
protocol::transaction::v1::action::{
BridgeLock,
BridgeSudoChange,
BridgeUnlock,
FeeAssetChange,
FeeChange,
IbcRelayerChange,
IbcSudoChange,
Ics20Withdrawal,
InitBridgeAccount,
RollupDataSubmission,
SudoAddressChange,
Transfer,
ValidatorUpdate,
},
Protobuf,
};

Expand Down Expand Up @@ -49,13 +74,15 @@ macro_rules! impl_protobuf_for_fee_components {
multiplier: multiplier
.ok_or_else(|| Self::Error::missing_field(Self::Raw::full_name(), "multiplier"))?
.into(),
_phantom: PhantomData,
})
}

fn to_raw(&self) -> Self::Raw {
let Self {
base,
multiplier,
_phantom,
} = self;
Self::Raw {
base: Some(base.into()),
Expand Down Expand Up @@ -83,89 +110,79 @@ impl_protobuf_for_fee_components!(
IbcSudoChangeFeeComponents => raw::IbcSudoChangeFeeComponents,
);

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct TransferFeeComponents {
pub base: u128,
pub multiplier: u128,
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct RollupDataSubmissionFeeComponents {
pub base: u128,
pub multiplier: u128,
pub struct FeeComponents<T: ?Sized> {
base: u128,
multiplier: u128,
_phantom: PhantomData<T>,
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct Ics20WithdrawalFeeComponents {
pub base: u128,
pub multiplier: u128,
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct InitBridgeAccountFeeComponents {
pub base: u128,
pub multiplier: u128,
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct BridgeLockFeeComponents {
pub base: u128,
pub multiplier: u128,
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct BridgeUnlockFeeComponents {
pub base: u128,
pub multiplier: u128,
}
impl<T: ?Sized> FeeComponents<T> {
#[must_use]
pub fn new(base: u128, multiplier: u128) -> Self {
Self {
base,
multiplier,
_phantom: PhantomData,
}
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct BridgeSudoChangeFeeComponents {
pub base: u128,
pub multiplier: u128,
}
#[must_use]
pub fn base(&self) -> u128 {
self.base
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct IbcRelayFeeComponents {
pub base: u128,
pub multiplier: u128,
#[must_use]
pub fn multiplier(&self) -> u128 {
self.multiplier
}
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct ValidatorUpdateFeeComponents {
pub base: u128,
pub multiplier: u128,
impl<T: Protobuf> Debug for FeeComponents<T> {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
f.debug_struct(&format!("{}FeeComponents", T::Raw::NAME))
Fraser999 marked this conversation as resolved.
Show resolved Hide resolved
.field("base", &self.base)
.field("multiplier", &self.multiplier)
.finish()
}
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct FeeAssetChangeFeeComponents {
pub base: u128,
pub multiplier: u128,
impl Debug for FeeComponents<IbcRelay> {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
f.debug_struct("IbcRelayFeeComponents")
.field("base", &self.base)
.field("multiplier", &self.multiplier)
.finish()
}
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct FeeChangeFeeComponents {
pub base: u128,
pub multiplier: u128,
impl<T: ?Sized> Clone for FeeComponents<T> {
fn clone(&self) -> Self {
*self
}
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct IbcRelayerChangeFeeComponents {
pub base: u128,
pub multiplier: u128,
}
impl<T: ?Sized> Copy for FeeComponents<T> {}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct SudoAddressChangeFeeComponents {
pub base: u128,
pub multiplier: u128,
impl<T: ?Sized> PartialEq for FeeComponents<T> {
fn eq(&self, other: &Self) -> bool {
self.base == other.base && self.multiplier == other.multiplier
}
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct IbcSudoChangeFeeComponents {
pub base: u128,
pub multiplier: u128,
}
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>;
Comment on lines +172 to +185
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


#[derive(Debug, Clone)]
pub struct TransactionFeeResponse {
Expand Down
111 changes: 14 additions & 97 deletions crates/astria-core/src/protocol/genesis/v1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,7 +826,6 @@ mod tests {
.unwrap()
}

#[expect(clippy::too_many_lines, reason = "for testing purposes")]
fn proto_genesis_state() -> raw::GenesisAppState {
raw::GenesisAppState {
accounts: vec![
Expand Down Expand Up @@ -859,104 +858,22 @@ mod tests {
}),
allowed_fee_assets: vec!["nria".into()],
fees: Some(raw::GenesisFees {
transfer: Some(
TransferFeeComponents {
base: 12,
multiplier: 0,
}
.to_raw(),
),
transfer: Some(TransferFeeComponents::new(12, 0).to_raw()),
rollup_data_submission: Some(
RollupDataSubmissionFeeComponents {
base: 32,
multiplier: 1,
}
.to_raw(),
),
init_bridge_account: Some(
InitBridgeAccountFeeComponents {
base: 48,
multiplier: 0,
}
.to_raw(),
),
bridge_lock: Some(
BridgeLockFeeComponents {
base: 12,
multiplier: 1,
}
.to_raw(),
),
bridge_unlock: Some(
BridgeUnlockFeeComponents {
base: 12,
multiplier: 0,
}
.to_raw(),
),
bridge_sudo_change: Some(
BridgeSudoChangeFeeComponents {
base: 24,
multiplier: 0,
}
.to_raw(),
),
ics20_withdrawal: Some(
Ics20WithdrawalFeeComponents {
base: 24,
multiplier: 0,
}
.to_raw(),
),
ibc_relay: Some(
IbcRelayFeeComponents {
base: 0,
multiplier: 0,
}
.to_raw(),
),
validator_update: Some(
ValidatorUpdateFeeComponents {
base: 0,
multiplier: 0,
}
.to_raw(),
),
fee_asset_change: Some(
FeeAssetChangeFeeComponents {
base: 0,
multiplier: 0,
}
.to_raw(),
),
fee_change: Some(
FeeChangeFeeComponents {
base: 0,
multiplier: 0,
}
.to_raw(),
),
ibc_relayer_change: Some(
IbcRelayerChangeFeeComponents {
base: 0,
multiplier: 0,
}
.to_raw(),
),
sudo_address_change: Some(
SudoAddressChangeFeeComponents {
base: 0,
multiplier: 0,
}
.to_raw(),
),
ibc_sudo_change: Some(
IbcSudoChangeFeeComponents {
base: 0,
multiplier: 0,
}
.to_raw(),
RollupDataSubmissionFeeComponents::new(32, 1).to_raw(),
),
init_bridge_account: Some(InitBridgeAccountFeeComponents::new(48, 0).to_raw()),
bridge_lock: Some(BridgeLockFeeComponents::new(12, 1).to_raw()),
bridge_unlock: Some(BridgeUnlockFeeComponents::new(12, 0).to_raw()),
bridge_sudo_change: Some(BridgeSudoChangeFeeComponents::new(24, 0).to_raw()),
ics20_withdrawal: Some(Ics20WithdrawalFeeComponents::new(24, 0).to_raw()),
ibc_relay: Some(IbcRelayFeeComponents::new(0, 0).to_raw()),
validator_update: Some(ValidatorUpdateFeeComponents::new(0, 0).to_raw()),
fee_asset_change: Some(FeeAssetChangeFeeComponents::new(0, 0).to_raw()),
fee_change: Some(FeeChangeFeeComponents::new(0, 0).to_raw()),
ibc_relayer_change: Some(IbcRelayerChangeFeeComponents::new(0, 0).to_raw()),
sudo_address_change: Some(SudoAddressChangeFeeComponents::new(0, 0).to_raw()),
ibc_sudo_change: Some(IbcSudoChangeFeeComponents::new(0, 0).to_raw()),
}),
}
}
Expand Down
5 changes: 1 addition & 4 deletions crates/astria-core/src/protocol/transaction/v1/action/group/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,7 @@ fn from_list_of_actions_bundleable_sudo() {

let asset: Denom = "nria".parse().unwrap();
let actions = vec![
Action::FeeChange(FeeChange::Transfer(TransferFeeComponents {
base: 100,
multiplier: 0,
})),
Action::FeeChange(FeeChange::Transfer(TransferFeeComponents::new(100, 0))),
Action::FeeAssetChange(FeeAssetChange::Addition(asset)),
Action::IbcRelayerChange(IbcRelayerChange::Addition(address)),
];
Expand Down
Loading
Loading