From cb87de947ed78b05d8b40847d7cfb36a29cf0abc Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Thu, 19 Sep 2024 10:41:14 -0500 Subject: [PATCH 1/9] rework fees --- .../astria-sequencer/src/accounts/action.rs | 98 ++++----- .../src/app/action_handler.rs | 20 +- .../astria-sequencer/src/app/fee_handler.rs | 21 ++ crates/astria-sequencer/src/app/mod.rs | 29 ++- .../astria-sequencer/src/app/tests_app/mod.rs | 2 +- .../src/app/tests_block_fees.rs | 96 ++++++--- .../src/app/tests_breaking_changes.rs | 5 + ...__tests__storage_keys_are_unchanged-2.snap | 4 +- .../astria-sequencer/src/assets/state_ext.rs | 187 ++++++++---------- .../astria-sequencer/src/authority/action.rs | 26 ++- .../src/bridge/bridge_lock_action.rs | 114 +++++++---- .../src/bridge/bridge_sudo_change_action.rs | 71 ++++--- .../src/bridge/bridge_unlock_action.rs | 60 +++++- .../src/bridge/init_bridge_account_action.rs | 74 ++++--- .../astria-sequencer/src/fee_asset_change.rs | 12 +- .../src/ibc/ibc_relayer_change.rs | 12 +- .../src/ibc/ics20_withdrawal.rs | 68 +++++-- .../astria-sequencer/src/sequence/action.rs | 48 +++-- .../astria-sequencer/src/transaction/mod.rs | 36 ++-- 19 files changed, 636 insertions(+), 347 deletions(-) create mode 100644 crates/astria-sequencer/src/app/fee_handler.rs diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index fea06788d..7af7e5988 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -1,7 +1,4 @@ -use astria_core::{ - protocol::transaction::v1alpha1::action::TransferAction, - Protobuf as _, -}; +use astria_core::protocol::transaction::v1alpha1::action::TransferAction; use astria_eyre::eyre::{ ensure, OptionExt as _, @@ -12,6 +9,10 @@ use cnidarium::{ StateRead, StateWrite, }; +use tracing::{ + instrument, + Level, +}; use super::AddressBytes; use crate::{ @@ -20,7 +21,10 @@ use crate::{ StateWriteExt as _, }, address::StateReadExt as _, - app::ActionHandler, + app::{ + ActionHandler, + FeeHandler, + }, assets::{ StateReadExt as _, StateWriteExt as _, @@ -57,6 +61,44 @@ impl ActionHandler for TransferAction { } } +#[async_trait::async_trait] +impl FeeHandler for TransferAction { + // allow: false positive due to proc macro; fixed with rust/clippy 1.81 + #[allow(clippy::blocks_in_conditions)] + #[instrument(skip_all, err(level = Level::WARN))] + async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); + let fee = state + .get_transfer_base_fee() + .await + .wrap_err("failed to get transfer base fee")?; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + state + .decrease_balance(from, &self.fee_asset, fee) + .await + .wrap_err("failed to decrease balance for fee payment")?; + state.add_fee_to_block_fees( + self.fee_asset.clone(), + fee, + tx_context.transaction_id, + tx_context.source_action_index, + )?; + + Ok(()) + } +} + pub(crate) async fn execute_transfer( action: &TransferAction, from: TAddress, @@ -67,51 +109,15 @@ where TAddress: AddressBytes, { let from = from.address_bytes(); - - let fee = state - .get_transfer_base_fee() + state + .decrease_balance(from, &action.asset, action.amount) .await - .wrap_err("failed to get transfer base fee")?; + .wrap_err("failed decreasing `from` account balance")?; state - .get_and_increase_block_fees(&action.fee_asset, fee, TransferAction::full_name()) + .increase_balance(action.to, &action.asset, action.amount) .await - .wrap_err("failed to add to block fees")?; - - // if fee payment asset is same asset as transfer asset, deduct fee - // from same balance as asset transferred - if action.asset.to_ibc_prefixed() == action.fee_asset.to_ibc_prefixed() { - // check_stateful should have already checked this arithmetic - let payment_amount = action - .amount - .checked_add(fee) - .expect("transfer amount plus fee should not overflow"); - - state - .decrease_balance(from, &action.asset, payment_amount) - .await - .wrap_err("failed decreasing `from` account balance")?; - state - .increase_balance(action.to, &action.asset, action.amount) - .await - .wrap_err("failed increasing `to` account balance")?; - } else { - // otherwise, just transfer the transfer asset and deduct fee from fee asset balance - // later - state - .decrease_balance(from, &action.asset, action.amount) - .await - .wrap_err("failed decreasing `from` account balance")?; - state - .increase_balance(action.to, &action.asset, action.amount) - .await - .wrap_err("failed increasing `to` account balance")?; + .wrap_err("failed increasing `to` account balance")?; - // deduct fee from fee asset balance - state - .decrease_balance(from, &action.fee_asset, fee) - .await - .wrap_err("failed decreasing `from` account balance for fee payment")?; - } Ok(()) } diff --git a/crates/astria-sequencer/src/app/action_handler.rs b/crates/astria-sequencer/src/app/action_handler.rs index a24788a7e..a071c3a99 100644 --- a/crates/astria-sequencer/src/app/action_handler.rs +++ b/crates/astria-sequencer/src/app/action_handler.rs @@ -1,14 +1,9 @@ use cnidarium::StateWrite; -/// This trait is a verbatim copy of `cnidarium_component::ActionHandler`. -/// -/// It's duplicated here because all actions are foreign types, forbidding -/// the implementation of [`cnidarium_component::ActionHandler`][1] for -/// these types due to Rust orphan rules. -/// -/// [1]: https://github.com/penumbra-zone/penumbra/blob/14959350abcb8cfbf33f9aedc7463fccfd8e3f9f/crates/cnidarium-component/src/action_handler.rs#L30 +use crate::app::fee_handler::FeeHandler; + #[async_trait::async_trait] -pub(crate) trait ActionHandler { +pub(crate) trait ActionHandler: FeeHandler { // Commenting out for the time being as this is currentl nonot being used. Leaving this in // for reference as this is copied from cnidarium_component. // ``` @@ -21,6 +16,15 @@ pub(crate) trait ActionHandler { async fn check_stateless(&self) -> astria_eyre::eyre::Result<()>; + async fn check_execute_and_pay_fees( + &self, + mut state: S, + ) -> astria_eyre::eyre::Result<()> { + self.check_and_execute(&mut state).await?; + self.calculate_and_pay_fees(&mut state).await?; + Ok(()) + } + async fn check_and_execute(&self, mut state: S) -> astria_eyre::eyre::Result<()>; } diff --git a/crates/astria-sequencer/src/app/fee_handler.rs b/crates/astria-sequencer/src/app/fee_handler.rs new file mode 100644 index 000000000..38bc9726e --- /dev/null +++ b/crates/astria-sequencer/src/app/fee_handler.rs @@ -0,0 +1,21 @@ +use astria_core::primitive::v1::{ + asset, + TransactionId, +}; +use cnidarium::StateWrite; + +#[async_trait::async_trait] +pub(crate) trait FeeHandler { + async fn calculate_and_pay_fees( + &self, + mut state: S, + ) -> astria_eyre::eyre::Result<()>; +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct Fee { + pub(crate) asset: asset::Denom, + pub(crate) amount: u128, + pub(crate) source_transaction_id: TransactionId, + pub(crate) source_action_index: u64, +} diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index 66a9e2c5a..2f9f551aa 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -12,6 +12,7 @@ mod tests_breaking_changes; mod tests_execute_transaction; mod action_handler; +mod fee_handler; use std::{ collections::VecDeque, sync::Arc, @@ -48,8 +49,13 @@ use cnidarium::{ StagedWriteBatch, StateDelta, StateRead, + StateWrite, Storage, }; +pub(crate) use fee_handler::{ + Fee, + FeeHandler, +}; use prost::Message as _; use sha2::{ Digest as _, @@ -62,6 +68,7 @@ use tendermint::{ types::ExecTxResult, Code, Event, + EventAttributeIndexExt as _, }, account, block::Header, @@ -1081,18 +1088,19 @@ impl App { let fees = self .state .get_block_fees() - .await .wrap_err("failed to get block fees")?; - for (asset, amount) in fees { + for fee in fees { state_tx - .increase_balance(fee_recipient, asset, amount) + .increase_balance(fee_recipient, fee.asset.clone(), fee.amount) .await .wrap_err("failed to increase fee recipient balance")?; + let fee_event = construct_tx_fee_event(&fee); + state_tx.record(fee_event); } // clear block fees - state_tx.clear_block_fees().await; + state_tx.clear_block_fees(); let events = self.apply(state_tx); Ok(abci::response::EndBlock { @@ -1181,3 +1189,16 @@ fn signed_transaction_from_bytes(bytes: &[u8]) -> Result { Ok(tx) } + +/// Creates `abci::Event` of kind `tx.fees` for sequencer fee reporting +fn construct_tx_fee_event(fee: &Fee) -> Event { + Event::new( + "tx.fees", + [ + ("asset", fee.asset.to_string()).index(), + ("feeAmount", fee.amount.to_string()).index(), + ("sourceTransactionId", fee.source_transaction_id.to_string()).index(), + ("sourceActionIndex", fee.source_action_index.to_string()).index(), + ], + ) +} diff --git a/crates/astria-sequencer/src/app/tests_app/mod.rs b/crates/astria-sequencer/src/app/tests_app/mod.rs index 6869d0bf9..9e9b96808 100644 --- a/crates/astria-sequencer/src/app/tests_app/mod.rs +++ b/crates/astria-sequencer/src/app/tests_app/mod.rs @@ -280,7 +280,7 @@ async fn app_transfer_block_fees_to_sudo() { .unwrap(), transfer_fee, ); - assert_eq!(app.state.get_block_fees().await.unwrap().len(), 0); + assert_eq!(app.state.get_block_fees().unwrap().len(), 0); } #[tokio::test] diff --git a/crates/astria-sequencer/src/app/tests_block_fees.rs b/crates/astria-sequencer/src/app/tests_block_fees.rs index 29c2292c0..bd2799af9 100644 --- a/crates/astria-sequencer/src/app/tests_block_fees.rs +++ b/crates/astria-sequencer/src/app/tests_block_fees.rs @@ -22,27 +22,25 @@ use crate::{ accounts::{ StateReadExt as _, StateWriteExt as _, - }, - app::test_utils::{ - get_alice_signing_key, - get_bridge_signing_key, - initialize_app, - BOB_ADDRESS, - }, - assets::StateReadExt as _, - bridge::{ + }, app::{ + test_utils::{ + get_alice_signing_key, + get_bridge_signing_key, + initialize_app, + BOB_ADDRESS, + }, + Fee, + }, assets::StateReadExt as _, authority::StateReadExt as _, bridge::{ get_deposit_byte_len, StateWriteExt as _, - }, - sequence::{ + }, sequence::{ calculate_fee_from_state, StateWriteExt as _, - }, - test_utils::{ + }, test_utils::{ astria_address, astria_address_from_hex_string, nria, - }, + } }; #[tokio::test] @@ -70,8 +68,13 @@ async fn transaction_execution_records_fee_event() { }; let signed_tx = Arc::new(tx.into_signed(&alice)); + let tx_id = signed_tx.id(); + app.execute_transaction(signed_tx).await.unwrap(); + + let sudo_address = app.state.get_sudo_address().await.unwrap(); + let end_block = app.end_block(1, sudo_address).await.unwrap(); - let events = app.execute_transaction(signed_tx).await.unwrap(); + let events = end_block.events; let transfer_fee = app.state.get_transfer_base_fee().await.unwrap(); let event = events.first().unwrap(); assert_eq!(event.kind, "tx.fees"); @@ -86,8 +89,17 @@ async fn transaction_execution_records_fee_event() { assert_eq!( event.attributes[2], ( - "actionType", - "astria.protocol.transactions.v1alpha1.TransferAction" + "sourceTransactionId", + tx_id.to_string(), + ) + .index() + .into() + ); + assert_eq!( + event.attributes[3], + ( + "sourceActionIndex", + "0", ) .index() .into() @@ -127,10 +139,16 @@ async fn ensure_correct_block_fees_transfer() { let total_block_fees: u128 = app .state .get_block_fees() - .await .unwrap() .into_iter() - .map(|(_, fee)| fee) + .map( + |Fee { + asset: _, + amount: fee, + source_transaction_id: _, + source_action_index: _, + }| fee, + ) .sum(); assert_eq!(total_block_fees, transfer_base_fee); } @@ -168,10 +186,16 @@ async fn ensure_correct_block_fees_sequence() { let total_block_fees: u128 = app .state .get_block_fees() - .await .unwrap() .into_iter() - .map(|(_, fee)| fee) + .map( + |Fee { + asset: _, + amount: fee, + source_transaction_id: _, + source_action_index: _, + }| fee, + ) .sum(); let expected_fees = calculate_fee_from_state(&data, &app.state).await.unwrap(); assert_eq!(total_block_fees, expected_fees); @@ -211,10 +235,16 @@ async fn ensure_correct_block_fees_init_bridge_acct() { let total_block_fees: u128 = app .state .get_block_fees() - .await .unwrap() .into_iter() - .map(|(_, fee)| fee) + .map( + |Fee { + asset: _, + amount: fee, + source_transaction_id: _, + source_action_index: _, + }| fee, + ) .sum(); assert_eq!(total_block_fees, init_bridge_account_base_fee); } @@ -275,10 +305,16 @@ async fn ensure_correct_block_fees_bridge_lock() { let total_block_fees: u128 = app .state .get_block_fees() - .await .unwrap() .into_iter() - .map(|(_, fee)| fee) + .map( + |Fee { + asset: _, + amount: fee, + source_transaction_id: _, + source_action_index: _, + }| fee, + ) .sum(); let expected_fees = transfer_base_fee + (get_deposit_byte_len(&test_deposit) * bridge_lock_byte_cost_multiplier); @@ -327,10 +363,16 @@ async fn ensure_correct_block_fees_bridge_sudo_change() { let total_block_fees: u128 = app .state .get_block_fees() - .await .unwrap() .into_iter() - .map(|(_, fee)| fee) + .map( + |Fee { + asset: _, + amount: fee, + source_transaction_id: _, + source_action_index: _, + }| fee, + ) .sum(); assert_eq!(total_block_fees, sudo_change_base_fee); } diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 473b4d5ea..19f045138 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -300,5 +300,10 @@ async fn app_execute_transaction_with_every_action_snapshot() { app.prepare_commit(storage.clone()).await.unwrap(); app.commit(storage.clone()).await; + use crate::accounts::StateReadExt as _; + println!("alice amount: {}", app.state.get_account_balance(alice.try_address(ASTRIA_PREFIX).unwrap(), nria()).await.unwrap()); + println!("bob amount: {}", app.state.get_account_balance(bob_address, nria()).await.unwrap()); + println!("carol amount: {}", app.state.get_account_balance(carol_address, nria()).await.unwrap()); + insta::assert_json_snapshot!(app.app_hash.as_bytes()); } diff --git a/crates/astria-sequencer/src/assets/snapshots/astria_sequencer__assets__state_ext__tests__storage_keys_are_unchanged-2.snap b/crates/astria-sequencer/src/assets/snapshots/astria_sequencer__assets__state_ext__tests__storage_keys_are_unchanged-2.snap index 8c31993c7..a3e273ee7 100644 --- a/crates/astria-sequencer/src/assets/snapshots/astria_sequencer__assets__state_ext__tests__storage_keys_are_unchanged-2.snap +++ b/crates/astria-sequencer/src/assets/snapshots/astria_sequencer__assets__state_ext__tests__storage_keys_are_unchanged-2.snap @@ -1,5 +1,5 @@ --- source: crates/astria-sequencer/src/assets/state_ext.rs -expression: block_fees_key(&trace_prefixed) +expression: fee_asset_key(trace_prefixed) --- -block_fees/ce072174ebc356c6ead681d61ab417ee72fdedd8155eb76478ece374bb04dc1d +fee_asset/ce072174ebc356c6ead681d61ab417ee72fdedd8155eb76478ece374bb04dc1d diff --git a/crates/astria-sequencer/src/assets/state_ext.rs b/crates/astria-sequencer/src/assets/state_ext.rs index 3a795a87e..932b98957 100644 --- a/crates/astria-sequencer/src/assets/state_ext.rs +++ b/crates/astria-sequencer/src/assets/state_ext.rs @@ -1,9 +1,11 @@ -use astria_core::primitive::v1::asset; +use astria_core::primitive::v1::{ + asset, + TransactionId, +}; use astria_eyre::{ anyhow_to_eyre, eyre::{ bail, - OptionExt as _, Result, WrapErr as _, }, @@ -18,17 +20,15 @@ use cnidarium::{ StateWrite, }; use futures::StreamExt as _; -use tendermint::abci::{ - Event, - EventAttributeIndexExt as _, -}; use tracing::instrument; +use crate::app::Fee; + /// Newtype wrapper to read and write a denomination trace from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] struct DenominationTrace(String); -const BLOCK_FEES_PREFIX: &str = "block_fees/"; +const BLOCK_FEES_PREFIX: &str = "block_fees"; const FEE_ASSET_PREFIX: &str = "fee_asset/"; const NATIVE_ASSET_KEY: &[u8] = b"nativeasset"; @@ -36,13 +36,6 @@ fn asset_storage_key>(asset: TAsset) -> String format!("asset/{}", crate::storage_keys::hunks::Asset::from(asset)) } -fn block_fees_key>(asset: TAsset) -> String { - format!( - "{BLOCK_FEES_PREFIX}{}", - crate::storage_keys::hunks::Asset::from(asset) - ) -} - fn fee_asset_key>(asset: TAsset) -> String { format!( "{FEE_ASSET_PREFIX}{}", @@ -50,22 +43,6 @@ fn fee_asset_key>(asset: TAsset) -> String { ) } -/// Creates `abci::Event` of kind `tx.fees` for sequencer fee reporting -fn construct_tx_fee_event( - asset: &T, - fee_amount: u128, - action_type: String, -) -> Event { - Event::new( - "tx.fees", - [ - ("asset", asset.to_string()).index(), - ("feeAmount", fee_amount.to_string()).index(), - ("actionType", action_type).index(), - ], - ) -} - #[async_trait] pub(crate) trait StateReadExt: StateRead { #[instrument(skip_all)] @@ -122,31 +99,15 @@ pub(crate) trait StateReadExt: StateRead { } #[instrument(skip_all)] - async fn get_block_fees(&self) -> Result> { - let mut fees = Vec::new(); - - let mut stream = - std::pin::pin!(self.nonverifiable_prefix_raw(BLOCK_FEES_PREFIX.as_bytes())); - while let Some(Ok((key, value))) = stream.next().await { - // if the key isn't of the form `block_fees/{asset_id}`, then we have a bug - // in `put_block_fees` - let suffix = key - .strip_prefix(BLOCK_FEES_PREFIX.as_bytes()) - .expect("prefix must always be present"); - let asset = std::str::from_utf8(suffix) - .wrap_err("key suffix was not utf8 encoded; this should not happen")? - .parse::() - .wrap_err("failed to parse storage key suffix as address hunk")? - .get(); - - let Ok(bytes): Result<[u8; 16], _> = value.try_into() else { - bail!("failed turning raw block fees bytes into u128; not 16 bytes?"); - }; - - fees.push((asset, u128::from_be_bytes(bytes))); + fn get_block_fees(&self) -> Result> { + let mut block_fees = self.object_get(BLOCK_FEES_PREFIX); + match block_fees { + Some(_) => {} + None => { + block_fees = Some(vec![]); + } } - - Ok(fees) + Ok(block_fees.expect("block fees should not be `None` after populating")) } #[instrument(skip_all)] @@ -202,53 +163,43 @@ pub(crate) trait StateWriteExt: StateWrite { Ok(()) } - /// Adds `amount` to the block fees for `asset`. + /// Constructs and adds `Fee` object to the block fees vec. #[instrument(skip_all)] - async fn get_and_increase_block_fees( + fn add_fee_to_block_fees( &mut self, asset: TAsset, amount: u128, - action_type: String, + source_transaction_id: TransactionId, + source_action_index: u64, ) -> Result<()> where - TAsset: Into + std::fmt::Display + Send, + TAsset: Into + std::fmt::Display + Send, { - let tx_fee_event = construct_tx_fee_event(&asset, amount, action_type); - let block_fees_key = block_fees_key(asset); + let mut current_fees: Option> = self.object_get(BLOCK_FEES_PREFIX); - let current_amount = self - .nonverifiable_get_raw(block_fees_key.as_bytes()) - .await - .map_err(anyhow_to_eyre) - .wrap_err("failed to read raw block fees from state")? - .map(|bytes| { - let Ok(bytes): Result<[u8; 16], _> = bytes.try_into() else { - // this shouldn't happen - bail!("failed turning raw block fees bytes into u128; not 16 bytes?"); - }; - Ok(u128::from_be_bytes(bytes)) - }) - .transpose()? - .unwrap_or_default(); - - let new_amount = current_amount - .checked_add(amount) - .ok_or_eyre("block fees overflowed u128")?; - - self.nonverifiable_put_raw(block_fees_key.into(), new_amount.to_be_bytes().to_vec()); + match current_fees { + Some(_) => {} + None => { + current_fees = Some(vec![]); + } + } - self.record(tx_fee_event); + let mut current_fees = + current_fees.expect("block fees should not be `None` after populating"); + current_fees.push(Fee { + asset: asset.into(), + amount, + source_transaction_id, + source_action_index, + }); + self.object_put(BLOCK_FEES_PREFIX, current_fees); Ok(()) } #[instrument(skip_all)] - async fn clear_block_fees(&mut self) { - let mut stream = - std::pin::pin!(self.nonverifiable_prefix_raw(BLOCK_FEES_PREFIX.as_bytes())); - while let Some(Ok((key, _))) = stream.next().await { - self.nonverifiable_delete(key); - } + fn clear_block_fees(&mut self) { + self.object_delete(BLOCK_FEES_PREFIX); } #[instrument(skip_all)] @@ -274,16 +225,19 @@ impl StateWriteExt for T {} mod tests { use std::collections::HashSet; - use astria_core::primitive::v1::asset; + use astria_core::primitive::v1::{ + asset, + TransactionId, + }; use cnidarium::StateDelta; use super::{ asset_storage_key, - block_fees_key, fee_asset_key, StateReadExt as _, StateWriteExt as _, }; + use crate::app::Fee; fn asset() -> asset::Denom { "asset".parse().unwrap() @@ -341,22 +295,26 @@ mod tests { let mut state = StateDelta::new(snapshot); // doesn't exist at first - let fee_balances_orig = state.get_block_fees().await.unwrap(); + let fee_balances_orig = state.get_block_fees().unwrap(); assert!(fee_balances_orig.is_empty()); // can write let asset = asset_0(); let amount = 100u128; state - .get_and_increase_block_fees(&asset, amount, "test".into()) - .await + .add_fee_to_block_fees(asset.clone(), amount, TransactionId::new([0; 32]), 0) .unwrap(); // holds expected - let fee_balances_updated = state.get_block_fees().await.unwrap(); + let fee_balances_updated = state.get_block_fees().unwrap(); assert_eq!( fee_balances_updated[0], - (asset.to_ibc_prefixed(), amount), + Fee { + asset, + amount, + source_transaction_id: TransactionId::new([0; 32]), + source_action_index: 0 + }, "fee balances are not what they were expected to be" ); } @@ -374,28 +332,46 @@ mod tests { let amount_second = 200u128; state - .get_and_increase_block_fees(&asset_first, amount_first, "test".into()) - .await + .add_fee_to_block_fees( + asset_first.clone(), + amount_first, + TransactionId::new([0; 32]), + 0, + ) .unwrap(); state - .get_and_increase_block_fees(&asset_second, amount_second, "test".into()) - .await + .add_fee_to_block_fees( + asset_second.clone(), + amount_second, + TransactionId::new([0; 32]), + 1, + ) .unwrap(); // holds expected - let fee_balances = HashSet::<_>::from_iter(state.get_block_fees().await.unwrap()); + let fee_balances = HashSet::<_>::from_iter(state.get_block_fees().unwrap()); assert_eq!( fee_balances, HashSet::from_iter(vec![ - (asset_first.to_ibc_prefixed(), amount_first), - (asset_second.to_ibc_prefixed(), amount_second) + Fee { + asset: asset_first, + amount: amount_first, + source_transaction_id: TransactionId::new([0; 32]), + source_action_index: 0 + }, + Fee { + asset: asset_second, + amount: amount_second, + source_transaction_id: TransactionId::new([0; 32]), + source_action_index: 1 + }, ]), "returned fee balance vector not what was expected" ); // can delete - state.clear_block_fees().await; + state.clear_block_fees(); - let fee_balances_updated = state.get_block_fees().await.unwrap(); + let fee_balances_updated = state.get_block_fees().unwrap(); assert!( fee_balances_updated.is_empty(), "fee balances were expected to be deleted but were not" @@ -643,11 +619,6 @@ mod tests { let trace_prefixed = "a/denom/with/a/prefix" .parse::() .unwrap(); - assert_eq!( - block_fees_key(&trace_prefixed), - block_fees_key(trace_prefixed.to_ibc_prefixed()), - ); - insta::assert_snapshot!(block_fees_key(&trace_prefixed)); assert_eq!( fee_asset_key(&trace_prefixed), diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index 5253461e3..d9b13c438 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -15,7 +15,10 @@ use cnidarium::StateWrite; use crate::{ accounts::StateWriteExt as _, address::StateReadExt as _, - app::ActionHandler, + app::{ + ActionHandler, + FeeHandler, + }, authority::{ StateReadExt as _, StateWriteExt as _, @@ -75,6 +78,13 @@ impl ActionHandler for ValidatorUpdate { } } +#[async_trait::async_trait] +impl FeeHandler for ValidatorUpdate { + async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + #[async_trait::async_trait] impl ActionHandler for SudoAddressChangeAction { async fn check_stateless(&self) -> Result<()> { @@ -105,6 +115,13 @@ impl ActionHandler for SudoAddressChangeAction { } } +#[async_trait::async_trait] +impl FeeHandler for SudoAddressChangeAction { + async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + #[async_trait::async_trait] impl ActionHandler for FeeChangeAction { async fn check_stateless(&self) -> Result<()> { @@ -155,6 +172,13 @@ impl ActionHandler for FeeChangeAction { } } +#[async_trait::async_trait] +impl FeeHandler for FeeChangeAction { + async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + #[cfg(test)] mod test { use astria_core::primitive::v1::TransactionId; diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 14c3af5e6..db454f4bd 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -4,7 +4,6 @@ use astria_core::{ TransferAction, }, sequencerblock::v1alpha1::block::Deposit, - Protobuf as _, }; use astria_eyre::eyre::{ ensure, @@ -13,6 +12,10 @@ use astria_eyre::eyre::{ WrapErr as _, }; use cnidarium::StateWrite; +use tracing::{ + instrument, + Level, +}; use crate::{ accounts::{ @@ -24,8 +27,14 @@ use crate::{ StateWriteExt as _, }, address::StateReadExt as _, - app::ActionHandler, - assets::StateWriteExt as _, + app::{ + ActionHandler, + FeeHandler, + }, + assets::{ + StateReadExt as _, + StateWriteExt as _, + }, bridge::{ StateReadExt as _, StateWriteExt as _, @@ -65,15 +74,6 @@ impl ActionHandler for BridgeLockAction { "asset ID is not authorized for transfer to bridge account", ); - let from_balance = state - .get_account_balance(from, &self.fee_asset) - .await - .wrap_err("failed to get sender account balance")?; - let transfer_fee = state - .get_transfer_base_fee() - .await - .context("failed to get transfer base fee")?; - let transaction_id = state .get_transaction_context() .expect("current source should be set before executing action") @@ -94,15 +94,6 @@ impl ActionHandler for BridgeLockAction { ); let deposit_abci_event = create_deposit_event(&deposit); - let byte_cost_multiplier = state - .get_bridge_lock_byte_cost_multiplier() - .await - .wrap_err("failed to get byte cost multiplier")?; - let fee = byte_cost_multiplier - .saturating_mul(get_deposit_byte_len(&deposit)) - .saturating_add(transfer_fee); - ensure!(from_balance >= fee, "insufficient funds for fee payment"); - let transfer_action = TransferAction { to: self.to, asset: self.asset.clone(), @@ -111,36 +102,79 @@ impl ActionHandler for BridgeLockAction { }; check_transfer(&transfer_action, from, &state).await?; - // Executes the transfer and deducts transfer feeds. - // FIXME: This is a very roundabout way of paying for fees. IMO it would be - // better to just duplicate this entire logic here so that we don't call out - // to the transfer-action logic. execute_transfer(&transfer_action, from, &mut state).await?; - // the transfer fee is already deducted in `execute_transfer() above, - // so we just deduct the bridge lock byte multiplier fee. - // FIXME: similar to what is mentioned there: this should be reworked so that - // the fee deducation logic for these actions are defined fully independently - // (even at the cost of duplicating code). + state.record(deposit_abci_event); + state + .put_deposit_event(deposit) + .await + .wrap_err("failed to put deposit event into state")?; + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for BridgeLockAction { + // allow: false positive due to proc macro; fixed with rust/clippy 1.81 + #[allow(clippy::blocks_in_conditions)] + #[instrument(skip_all, err(level = Level::WARN))] + async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let rollup_id = state + .get_bridge_account_rollup_id(self.to) + .await + .wrap_err("failed to get bridge account rollup id")? + .ok_or_eyre("bridge lock must be sent to a bridge account")?; + let transfer_fee = state + .get_transfer_base_fee() + .await + .context("failed to get transfer base fee")?; + let from = tx_context.address_bytes(); + let transaction_id = tx_context.transaction_id; + let source_action_index = tx_context.source_action_index; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + let deposit = Deposit::new( + self.to, + rollup_id, + self.amount, + self.asset.clone(), + self.destination_chain_address.clone(), + transaction_id, + source_action_index, + ); + let byte_cost_multiplier = state .get_bridge_lock_byte_cost_multiplier() .await .wrap_err("failed to get byte cost multiplier")?; - let fee = byte_cost_multiplier.saturating_mul(get_deposit_byte_len(&deposit)); + + let fee = byte_cost_multiplier + .saturating_mul(get_deposit_byte_len(&deposit)) + .saturating_add(transfer_fee); + state - .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) - .await + .add_fee_to_block_fees( + self.fee_asset.clone(), + fee, + tx_context.transaction_id, + source_action_index, + ) .wrap_err("failed to add to block fees")?; state .decrease_balance(from, &self.fee_asset, fee) .await .wrap_err("failed to deduct fee from account balance")?; - state.record(deposit_abci_event); - state - .put_deposit_event(deposit) - .await - .wrap_err("failed to put deposit event into state")?; Ok(()) } } @@ -220,8 +254,8 @@ mod tests { .put_account_balance(from_address, &asset, 100 + transfer_fee) .unwrap(); assert_eyre_error( - &bridge_lock.check_and_execute(&mut state).await.unwrap_err(), - "insufficient funds for fee payment", + &bridge_lock.check_execute_and_pay_fees(&mut state).await.unwrap_err(), + "failed to deduct fee from account balance", ); // enough balance; should pass diff --git a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs index 39ae6e2e4..e11a37961 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -1,7 +1,4 @@ -use astria_core::{ - protocol::transaction::v1alpha1::action::BridgeSudoChangeAction, - Protobuf as _, -}; +use astria_core::protocol::transaction::v1alpha1::action::BridgeSudoChangeAction; use astria_eyre::eyre::{ bail, ensure, @@ -9,11 +6,18 @@ use astria_eyre::eyre::{ WrapErr as _, }; use cnidarium::StateWrite; +use tracing::{ + instrument, + Level, +}; use crate::{ accounts::StateWriteExt as _, address::StateReadExt as _, - app::ActionHandler, + app::{ + ActionHandler, + FeeHandler, + }, assets::{ StateReadExt as _, StateWriteExt as _, @@ -52,14 +56,6 @@ impl ActionHandler for BridgeSudoChangeAction { .wrap_err("failed check for base prefix of new withdrawer address")?; } - ensure!( - state - .is_allowed_fee_asset(&self.fee_asset) - .await - .wrap_err("failed to check allowed fee assets in state")?, - "invalid fee asset", - ); - // check that the sender of this tx is the authorized sudo address for the bridge account let Some(sudo_address) = state .get_bridge_account_sudo_address(self.bridge_address) @@ -76,27 +72,54 @@ impl ActionHandler for BridgeSudoChangeAction { "unauthorized for bridge sudo change action", ); + if let Some(sudo_address) = self.new_sudo_address { + state.put_bridge_account_sudo_address(self.bridge_address, sudo_address); + } + + if let Some(withdrawer_address) = self.new_withdrawer_address { + state.put_bridge_account_withdrawer_address(self.bridge_address, withdrawer_address); + } + + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for BridgeSudoChangeAction { + // allow: false positive due to proc macro; fixed with rust/clippy 1.81 + #[allow(clippy::blocks_in_conditions)] + #[instrument(skip_all, err(level = Level::WARN))] + async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); let fee = state .get_bridge_sudo_change_base_fee() .await .wrap_err("failed to get bridge sudo change fee")?; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + state - .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) - .await + .add_fee_to_block_fees( + self.fee_asset.clone(), + fee, + tx_context.transaction_id, + tx_context.source_action_index, + ) .wrap_err("failed to add to block fees")?; state - .decrease_balance(self.bridge_address, &self.fee_asset, fee) + .decrease_balance(from, &self.fee_asset, fee) .await .wrap_err("failed to decrease balance for bridge sudo change fee")?; - if let Some(sudo_address) = self.new_sudo_address { - state.put_bridge_account_sudo_address(self.bridge_address, sudo_address); - } - - if let Some(withdrawer_address) = self.new_withdrawer_address { - state.put_bridge_account_withdrawer_address(self.bridge_address, withdrawer_address); - } - Ok(()) } } diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index 4868b29b8..23f6c7285 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -9,14 +9,29 @@ use astria_eyre::eyre::{ WrapErr as _, }; use cnidarium::StateWrite; +use tracing::{ + instrument, + Level, +}; use crate::{ - accounts::action::{ - check_transfer, - execute_transfer, + accounts::{ + action::{ + check_transfer, + execute_transfer, + }, + StateReadExt as _, + StateWriteExt as _, }, address::StateReadExt as _, - app::ActionHandler, + app::{ + ActionHandler, + FeeHandler, + }, + assets::{ + StateReadExt as _, + StateWriteExt as _, + }, bridge::{ StateReadExt as _, StateWriteExt as _, @@ -100,6 +115,43 @@ impl ActionHandler for BridgeUnlockAction { } } +#[async_trait::async_trait] +impl FeeHandler for BridgeUnlockAction { + // allow: false positive due to proc macro; fixed with rust/clippy 1.81 + #[allow(clippy::blocks_in_conditions)] + #[instrument(skip_all, err(level = Level::WARN))] + async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); + let transfer_fee = state + .get_transfer_base_fee() + .await + .wrap_err("failed to get transfer base fee")?; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + state + .decrease_balance(from, &self.fee_asset, transfer_fee) + .await + .wrap_err("failed to decrease balance for fee payment")?; + state.add_fee_to_block_fees( + self.fee_asset.clone(), + transfer_fee, + tx_context.transaction_id, + tx_context.source_action_index, + )?; + Ok(()) + } +} + #[cfg(test)] mod tests { use astria_core::{ diff --git a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs index d8a2dedaa..9acb8109c 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -1,7 +1,6 @@ use astria_core::{ primitive::v1::Address, protocol::transaction::v1alpha1::action::InitBridgeAccountAction, - Protobuf as _, }; use astria_eyre::eyre::{ bail, @@ -10,14 +9,18 @@ use astria_eyre::eyre::{ WrapErr as _, }; use cnidarium::StateWrite; +use tracing::{ + instrument, + Level, +}; use crate::{ - accounts::{ - StateReadExt as _, - StateWriteExt as _, - }, + accounts::StateWriteExt as _, address::StateReadExt as _, - app::ActionHandler, + app::{ + ActionHandler, + FeeHandler, + }, assets::{ StateReadExt as _, StateWriteExt as _, @@ -53,16 +56,6 @@ impl ActionHandler for InitBridgeAccountAction { .wrap_err("failed check for base prefix of sudo address")?; } - ensure!( - state.is_allowed_fee_asset(&self.fee_asset).await?, - "invalid fee asset", - ); - - let fee = state - .get_init_bridge_account_base_fee() - .await - .wrap_err("failed to get base fee for initializing bridge account")?; - // this prevents the address from being registered as a bridge account // if it's been previously initialized as a bridge account. // @@ -83,16 +76,6 @@ impl ActionHandler for InitBridgeAccountAction { bail!("bridge account already exists"); } - let balance = state - .get_account_balance(from, &self.fee_asset) - .await - .wrap_err("failed getting `from` account balance for fee payment")?; - - ensure!( - balance >= fee, - "insufficient funds for bridge account initialization", - ); - state.put_bridge_account_rollup_id(from, &self.rollup_id); state .put_bridge_account_ibc_asset(from, &self.asset) @@ -102,14 +85,45 @@ impl ActionHandler for InitBridgeAccountAction { from, self.withdrawer_address.map_or(from, Address::bytes), ); - state - .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) + + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for InitBridgeAccountAction { + // allow: false positive due to proc macro; fixed with rust/clippy 1.81 + #[allow(clippy::blocks_in_conditions)] + #[instrument(skip_all, err(level = Level::WARN))] + async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); + let fee = state + .get_init_bridge_account_base_fee() .await - .wrap_err("failed to get and increase block fees")?; + .wrap_err("failed to get transfer base fee")?; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + state .decrease_balance(from, &self.fee_asset, fee) .await - .wrap_err("failed to deduct fee from account balance")?; + .wrap_err("failed to decrease balance for fee payment")?; + state.add_fee_to_block_fees( + self.fee_asset.clone(), + fee, + tx_context.transaction_id, + tx_context.source_action_index, + )?; + Ok(()) } } diff --git a/crates/astria-sequencer/src/fee_asset_change.rs b/crates/astria-sequencer/src/fee_asset_change.rs index 00099ffe1..33bcd43a1 100644 --- a/crates/astria-sequencer/src/fee_asset_change.rs +++ b/crates/astria-sequencer/src/fee_asset_change.rs @@ -9,7 +9,10 @@ use async_trait::async_trait; use cnidarium::StateWrite; use crate::{ - app::ActionHandler, + app::{ + ActionHandler, + FeeHandler, + }, assets::{ StateReadExt as _, StateWriteExt as _, @@ -57,3 +60,10 @@ impl ActionHandler for FeeAssetChangeAction { Ok(()) } } + +#[async_trait::async_trait] +impl FeeHandler for FeeAssetChangeAction { + async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} diff --git a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs index 57c2da04b..2beec904a 100644 --- a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs +++ b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs @@ -9,7 +9,10 @@ use cnidarium::StateWrite; use crate::{ address::StateReadExt as _, - app::ActionHandler, + app::{ + ActionHandler, + FeeHandler, + }, ibc::{ StateReadExt as _, StateWriteExt as _, @@ -56,3 +59,10 @@ impl ActionHandler for IbcRelayerChangeAction { Ok(()) } } + +#[async_trait::async_trait] +impl FeeHandler for IbcRelayerChangeAction { + async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 456fd764f..ace659086 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -8,7 +8,6 @@ use astria_core::{ memos::v1alpha1::Ics20WithdrawalFromRollup, transaction::v1alpha1::action, }, - Protobuf as _, }; use astria_eyre::{ anyhow_to_eyre, @@ -35,6 +34,10 @@ use penumbra_ibc::component::packet::{ Unchecked, }; use penumbra_proto::core::component::ibc::v1::FungibleTokenPacketData; +use tracing::{ + instrument, + Level, +}; use crate::{ accounts::{ @@ -42,8 +45,14 @@ use crate::{ StateWriteExt as _, }, address::StateReadExt as _, - app::ActionHandler, - assets::StateWriteExt as _, + app::{ + ActionHandler, + FeeHandler, + }, + assets::{ + StateReadExt as _, + StateWriteExt as _, + }, bridge::{ StateReadExt as _, StateWriteExt as _, @@ -211,11 +220,6 @@ impl ActionHandler for action::Ics20Withdrawal { .await .wrap_err("failed establishing which account to withdraw funds from")?; - let fee = state - .get_ics20_withdrawal_base_fee() - .await - .wrap_err("failed to get ics20 withdrawal base fee")?; - let current_timestamp = state .get_block_timestamp() .await @@ -231,21 +235,11 @@ impl ActionHandler for action::Ics20Withdrawal { .wrap_err("packet failed send check")? }; - state - .get_and_increase_block_fees(self.fee_asset(), fee, Self::full_name()) - .await - .wrap_err("failed to get and increase block fees")?; - state .decrease_balance(withdrawal_target, self.denom(), self.amount()) .await .wrap_err("failed to decrease sender or bridge balance")?; - state - .decrease_balance(from, self.fee_asset(), fee) - .await - .wrap_err("failed to subtract fee from sender balance")?; - // if we're the source, move tokens to the escrow account, // otherwise the tokens are just burned if is_source(packet.source_port(), packet.source_channel(), self.denom()) { @@ -270,6 +264,44 @@ impl ActionHandler for action::Ics20Withdrawal { } } +#[async_trait::async_trait] +impl FeeHandler for action::Ics20Withdrawal { + // allow: false positive due to proc macro; fixed with rust/clippy 1.81 + #[allow(clippy::blocks_in_conditions)] + #[instrument(skip_all, err(level = Level::WARN))] + async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); + let fee = state + .get_ics20_withdrawal_base_fee() + .await + .wrap_err("failed to get transfer base fee")?; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + state + .decrease_balance(from, &self.fee_asset, fee) + .await + .wrap_err("failed to decrease balance for fee payment")?; + state.add_fee_to_block_fees( + self.fee_asset.clone(), + fee, + tx_context.transaction_id, + tx_context.source_action_index, + )?; + + Ok(()) + } +} + fn is_source(source_port: &PortId, source_channel: &ChannelId, asset: &Denom) -> bool { if let Denom::TracePrefixed(trace) = asset { !trace.starts_with_str(&format!("{source_port}/{source_channel}")) diff --git a/crates/astria-sequencer/src/sequence/action.rs b/crates/astria-sequencer/src/sequence/action.rs index e294a1bbc..7e4e47f58 100644 --- a/crates/astria-sequencer/src/sequence/action.rs +++ b/crates/astria-sequencer/src/sequence/action.rs @@ -1,7 +1,4 @@ -use astria_core::{ - protocol::transaction::v1alpha1::action::SequenceAction, - Protobuf as _, -}; +use astria_core::protocol::transaction::v1alpha1::action::SequenceAction; use astria_eyre::eyre::{ ensure, OptionExt as _, @@ -9,13 +6,17 @@ use astria_eyre::eyre::{ WrapErr as _, }; use cnidarium::StateWrite; +use tracing::{ + instrument, + Level, +}; use crate::{ - accounts::{ - StateReadExt as _, - StateWriteExt as _, + accounts::StateWriteExt as _, + app::{ + ActionHandler, + FeeHandler, }, - app::ActionHandler, assets::{ StateReadExt, StateWriteExt, @@ -36,11 +37,21 @@ impl ActionHandler for SequenceAction { Ok(()) } - async fn check_and_execute(&self, mut state: S) -> Result<()> { - let from = state + async fn check_and_execute(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for SequenceAction { + // allow: false positive due to proc macro; fixed with rust/clippy 1.81 + #[allow(clippy::blocks_in_conditions)] + #[instrument(skip_all, err(level = Level::WARN))] + async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state .get_transaction_context() - .expect("transaction source must be present in state when executing an action") - .address_bytes(); + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); ensure!( state @@ -50,18 +61,17 @@ impl ActionHandler for SequenceAction { "invalid fee asset", ); - let curr_balance = state - .get_account_balance(from, &self.fee_asset) - .await - .wrap_err("failed getting `from` account balance for fee payment")?; let fee = calculate_fee_from_state(&self.data, &state) .await .wrap_err("calculated fee overflows u128")?; - ensure!(curr_balance >= fee, "insufficient funds"); state - .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) - .await + .add_fee_to_block_fees( + self.fee_asset.clone(), + fee, + tx_context.transaction_id, + tx_context.source_action_index, + ) .wrap_err("failed to add to block fees")?; state .decrease_balance(from, &self.fee_asset, fee) diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index f0997f673..2fd85add3 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -38,7 +38,10 @@ use crate::{ StateReadExt as _, StateWriteExt as _, }, - app::ActionHandler, + app::{ + ActionHandler, + FeeHandler, + }, bridge::{ StateReadExt as _, StateWriteExt as _, @@ -210,23 +213,23 @@ impl ActionHandler for SignedTransaction { match action { Action::Transfer(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("executing transfer action failed")?, Action::Sequence(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("executing sequence action failed")?, Action::ValidatorUpdate(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("executing validor update")?, Action::SudoAddressChange(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("executing sudo address change failed")?, Action::FeeChange(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("executing fee change failed")?, Action::Ibc(act) => { @@ -251,31 +254,31 @@ impl ActionHandler for SignedTransaction { .wrap_err("failed executing ibc action")?; } Action::Ics20Withdrawal(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("failed executing ics20 withdrawal")?, Action::IbcRelayerChange(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("failed executing ibc relayer change")?, Action::FeeAssetChange(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("failed executing fee asseet change")?, Action::InitBridgeAccount(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("failed executing init bridge account")?, Action::BridgeLock(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("failed executing bridge lock")?, Action::BridgeUnlock(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("failed executing bridge unlock")?, Action::BridgeSudoChange(act) => act - .check_and_execute(&mut state) + .check_execute_and_pay_fees(&mut state) .await .wrap_err("failed executing bridge sudo change")?, } @@ -286,3 +289,10 @@ impl ActionHandler for SignedTransaction { Ok(()) } } + +#[async_trait::async_trait] +impl FeeHandler for SignedTransaction { + async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} From 5662ba1a31483c75127282950bee64dc1f194950 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Thu, 19 Sep 2024 11:14:24 -0500 Subject: [PATCH 2/9] rustfmt --- .../src/app/tests_block_fees.rs | 30 ++++++++----------- .../src/app/tests_breaking_changes.rs | 24 +++++++++++++-- .../src/bridge/bridge_lock_action.rs | 5 +++- 3 files changed, 38 insertions(+), 21 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_block_fees.rs b/crates/astria-sequencer/src/app/tests_block_fees.rs index bd2799af9..f8af7e396 100644 --- a/crates/astria-sequencer/src/app/tests_block_fees.rs +++ b/crates/astria-sequencer/src/app/tests_block_fees.rs @@ -22,7 +22,8 @@ use crate::{ accounts::{ StateReadExt as _, StateWriteExt as _, - }, app::{ + }, + app::{ test_utils::{ get_alice_signing_key, get_bridge_signing_key, @@ -30,17 +31,22 @@ use crate::{ BOB_ADDRESS, }, Fee, - }, assets::StateReadExt as _, authority::StateReadExt as _, bridge::{ + }, + assets::StateReadExt as _, + authority::StateReadExt as _, + bridge::{ get_deposit_byte_len, StateWriteExt as _, - }, sequence::{ + }, + sequence::{ calculate_fee_from_state, StateWriteExt as _, - }, test_utils::{ + }, + test_utils::{ astria_address, astria_address_from_hex_string, nria, - } + }, }; #[tokio::test] @@ -88,21 +94,11 @@ async fn transaction_execution_records_fee_event() { ); assert_eq!( event.attributes[2], - ( - "sourceTransactionId", - tx_id.to_string(), - ) - .index() - .into() + ("sourceTransactionId", tx_id.to_string(),).index().into() ); assert_eq!( event.attributes[3], - ( - "sourceActionIndex", - "0", - ) - .index() - .into() + ("sourceActionIndex", "0",).index().into() ); } diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 19f045138..78257a952 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -301,9 +301,27 @@ async fn app_execute_transaction_with_every_action_snapshot() { app.commit(storage.clone()).await; use crate::accounts::StateReadExt as _; - println!("alice amount: {}", app.state.get_account_balance(alice.try_address(ASTRIA_PREFIX).unwrap(), nria()).await.unwrap()); - println!("bob amount: {}", app.state.get_account_balance(bob_address, nria()).await.unwrap()); - println!("carol amount: {}", app.state.get_account_balance(carol_address, nria()).await.unwrap()); + println!( + "alice amount: {}", + app.state + .get_account_balance(alice.try_address(ASTRIA_PREFIX).unwrap(), nria()) + .await + .unwrap() + ); + println!( + "bob amount: {}", + app.state + .get_account_balance(bob_address, nria()) + .await + .unwrap() + ); + println!( + "carol amount: {}", + app.state + .get_account_balance(carol_address, nria()) + .await + .unwrap() + ); insta::assert_json_snapshot!(app.app_hash.as_bytes()); } diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index db454f4bd..c11a398d3 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -254,7 +254,10 @@ mod tests { .put_account_balance(from_address, &asset, 100 + transfer_fee) .unwrap(); assert_eyre_error( - &bridge_lock.check_execute_and_pay_fees(&mut state).await.unwrap_err(), + &bridge_lock + .check_execute_and_pay_fees(&mut state) + .await + .unwrap_err(), "failed to deduct fee from account balance", ); From 4db830619e7f38fe62d0428a73bf02c407a3c26e Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Thu, 19 Sep 2024 11:21:07 -0500 Subject: [PATCH 3/9] remove manual testing code --- .../src/app/tests_breaking_changes.rs | 25 +------------------ 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 78257a952..5ec04f0e7 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -299,29 +299,6 @@ async fn app_execute_transaction_with_every_action_snapshot() { app.prepare_commit(storage.clone()).await.unwrap(); app.commit(storage.clone()).await; - - use crate::accounts::StateReadExt as _; - println!( - "alice amount: {}", - app.state - .get_account_balance(alice.try_address(ASTRIA_PREFIX).unwrap(), nria()) - .await - .unwrap() - ); - println!( - "bob amount: {}", - app.state - .get_account_balance(bob_address, nria()) - .await - .unwrap() - ); - println!( - "carol amount: {}", - app.state - .get_account_balance(carol_address, nria()) - .await - .unwrap() - ); - + insta::assert_json_snapshot!(app.app_hash.as_bytes()); } From 34c3cf06ccac8ac9da29ed08b1ef3cc99587a9b9 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Thu, 19 Sep 2024 11:22:37 -0500 Subject: [PATCH 4/9] rustfmt --- crates/astria-sequencer/src/app/tests_breaking_changes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 5ec04f0e7..473b4d5ea 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -299,6 +299,6 @@ async fn app_execute_transaction_with_every_action_snapshot() { app.prepare_commit(storage.clone()).await.unwrap(); app.commit(storage.clone()).await; - + insta::assert_json_snapshot!(app.app_hash.as_bytes()); } From 1b5825e3dc396a4c808121d7ef43103019f51d4a Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Thu, 19 Sep 2024 12:01:02 -0500 Subject: [PATCH 5/9] Minor improvements and verbage changes --- .../src/app/tests_block_fees.rs | 45 +++---------------- .../src/bridge/bridge_unlock_action.rs | 2 +- .../src/bridge/init_bridge_account_action.rs | 2 +- .../src/ibc/ics20_withdrawal.rs | 2 +- 4 files changed, 8 insertions(+), 43 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_block_fees.rs b/crates/astria-sequencer/src/app/tests_block_fees.rs index f8af7e396..6e3391c84 100644 --- a/crates/astria-sequencer/src/app/tests_block_fees.rs +++ b/crates/astria-sequencer/src/app/tests_block_fees.rs @@ -137,14 +137,7 @@ async fn ensure_correct_block_fees_transfer() { .get_block_fees() .unwrap() .into_iter() - .map( - |Fee { - asset: _, - amount: fee, - source_transaction_id: _, - source_action_index: _, - }| fee, - ) + .map(|fee| fee.amount) .sum(); assert_eq!(total_block_fees, transfer_base_fee); } @@ -184,14 +177,7 @@ async fn ensure_correct_block_fees_sequence() { .get_block_fees() .unwrap() .into_iter() - .map( - |Fee { - asset: _, - amount: fee, - source_transaction_id: _, - source_action_index: _, - }| fee, - ) + .map(|fee| fee.amount) .sum(); let expected_fees = calculate_fee_from_state(&data, &app.state).await.unwrap(); assert_eq!(total_block_fees, expected_fees); @@ -233,14 +219,7 @@ async fn ensure_correct_block_fees_init_bridge_acct() { .get_block_fees() .unwrap() .into_iter() - .map( - |Fee { - asset: _, - amount: fee, - source_transaction_id: _, - source_action_index: _, - }| fee, - ) + .map(|fee| fee.amount) .sum(); assert_eq!(total_block_fees, init_bridge_account_base_fee); } @@ -303,14 +282,7 @@ async fn ensure_correct_block_fees_bridge_lock() { .get_block_fees() .unwrap() .into_iter() - .map( - |Fee { - asset: _, - amount: fee, - source_transaction_id: _, - source_action_index: _, - }| fee, - ) + .map(|fee| fee.amount) .sum(); let expected_fees = transfer_base_fee + (get_deposit_byte_len(&test_deposit) * bridge_lock_byte_cost_multiplier); @@ -361,14 +333,7 @@ async fn ensure_correct_block_fees_bridge_sudo_change() { .get_block_fees() .unwrap() .into_iter() - .map( - |Fee { - asset: _, - amount: fee, - source_transaction_id: _, - source_action_index: _, - }| fee, - ) + .map(|fee| fee.amount) .sum(); assert_eq!(total_block_fees, sudo_change_base_fee); } diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index 23f6c7285..d882b30ba 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -128,7 +128,7 @@ impl FeeHandler for BridgeUnlockAction { let transfer_fee = state .get_transfer_base_fee() .await - .wrap_err("failed to get transfer base fee")?; + .wrap_err("failed to get transfer base fee for bridge unlock action")?; ensure!( state diff --git a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs index 9acb8109c..4ae91d28a 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -103,7 +103,7 @@ impl FeeHandler for InitBridgeAccountAction { let fee = state .get_init_bridge_account_base_fee() .await - .wrap_err("failed to get transfer base fee")?; + .wrap_err("failed to get init bridge account base fee")?; ensure!( state diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index ace659086..6f6b15f79 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -277,7 +277,7 @@ impl FeeHandler for action::Ics20Withdrawal { let fee = state .get_ics20_withdrawal_base_fee() .await - .wrap_err("failed to get transfer base fee")?; + .wrap_err("failed to get ics20 withdrawal base fee")?; ensure!( state From aa2c9ff27b31de84c4d10ec43d0da0cd3e83b7d0 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Thu, 19 Sep 2024 12:10:10 -0500 Subject: [PATCH 6/9] Fix warning --- crates/astria-sequencer/src/app/tests_block_fees.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_block_fees.rs b/crates/astria-sequencer/src/app/tests_block_fees.rs index 6e3391c84..c297a0a34 100644 --- a/crates/astria-sequencer/src/app/tests_block_fees.rs +++ b/crates/astria-sequencer/src/app/tests_block_fees.rs @@ -23,14 +23,11 @@ use crate::{ StateReadExt as _, StateWriteExt as _, }, - app::{ - test_utils::{ - get_alice_signing_key, - get_bridge_signing_key, - initialize_app, - BOB_ADDRESS, - }, - Fee, + app::test_utils::{ + get_alice_signing_key, + get_bridge_signing_key, + initialize_app, + BOB_ADDRESS, }, assets::StateReadExt as _, authority::StateReadExt as _, From f55673cb3594478cff4bff587caa4564d3943e4c Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Mon, 30 Sep 2024 12:45:35 -0500 Subject: [PATCH 7/9] remove allow attributes --- crates/astria-sequencer/src/accounts/action.rs | 2 -- crates/astria-sequencer/src/bridge/bridge_lock_action.rs | 2 -- crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs | 2 -- crates/astria-sequencer/src/bridge/bridge_unlock_action.rs | 2 -- .../astria-sequencer/src/bridge/init_bridge_account_action.rs | 2 -- crates/astria-sequencer/src/ibc/ics20_withdrawal.rs | 2 -- crates/astria-sequencer/src/sequence/action.rs | 2 -- 7 files changed, 14 deletions(-) diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index 7af7e5988..102c17211 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -63,8 +63,6 @@ impl ActionHandler for TransferAction { #[async_trait::async_trait] impl FeeHandler for TransferAction { - // allow: false positive due to proc macro; fixed with rust/clippy 1.81 - #[allow(clippy::blocks_in_conditions)] #[instrument(skip_all, err(level = Level::WARN))] async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { let tx_context = state diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 5d1e057f3..83fe4218f 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -114,8 +114,6 @@ impl ActionHandler for BridgeLockAction { #[async_trait::async_trait] impl FeeHandler for BridgeLockAction { - // allow: false positive due to proc macro; fixed with rust/clippy 1.81 - #[allow(clippy::blocks_in_conditions)] #[instrument(skip_all, err(level = Level::WARN))] async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { let tx_context = state diff --git a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs index e11a37961..399fb8d63 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -86,8 +86,6 @@ impl ActionHandler for BridgeSudoChangeAction { #[async_trait::async_trait] impl FeeHandler for BridgeSudoChangeAction { - // allow: false positive due to proc macro; fixed with rust/clippy 1.81 - #[allow(clippy::blocks_in_conditions)] #[instrument(skip_all, err(level = Level::WARN))] async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { let tx_context = state diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index d882b30ba..a3f4c38a9 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -117,8 +117,6 @@ impl ActionHandler for BridgeUnlockAction { #[async_trait::async_trait] impl FeeHandler for BridgeUnlockAction { - // allow: false positive due to proc macro; fixed with rust/clippy 1.81 - #[allow(clippy::blocks_in_conditions)] #[instrument(skip_all, err(level = Level::WARN))] async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { let tx_context = state diff --git a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs index 4ae91d28a..e56c8b609 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -92,8 +92,6 @@ impl ActionHandler for InitBridgeAccountAction { #[async_trait::async_trait] impl FeeHandler for InitBridgeAccountAction { - // allow: false positive due to proc macro; fixed with rust/clippy 1.81 - #[allow(clippy::blocks_in_conditions)] #[instrument(skip_all, err(level = Level::WARN))] async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { let tx_context = state diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 543981a67..62e675c51 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -266,8 +266,6 @@ impl ActionHandler for action::Ics20Withdrawal { #[async_trait::async_trait] impl FeeHandler for action::Ics20Withdrawal { - // allow: false positive due to proc macro; fixed with rust/clippy 1.81 - #[allow(clippy::blocks_in_conditions)] #[instrument(skip_all, err(level = Level::WARN))] async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { let tx_context = state diff --git a/crates/astria-sequencer/src/sequence/action.rs b/crates/astria-sequencer/src/sequence/action.rs index cd747a985..0ad3c1a06 100644 --- a/crates/astria-sequencer/src/sequence/action.rs +++ b/crates/astria-sequencer/src/sequence/action.rs @@ -44,8 +44,6 @@ impl ActionHandler for SequenceAction { #[async_trait::async_trait] impl FeeHandler for SequenceAction { - // allow: false positive due to proc macro; fixed with rust/clippy 1.81 - #[allow(clippy::blocks_in_conditions)] #[instrument(skip_all, err(level = Level::WARN))] async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { let tx_context = state From 747c6be344b07fcc5d216215afec8ac3da6581e1 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Wed, 2 Oct 2024 08:10:40 -0500 Subject: [PATCH 8/9] rustfmt --- crates/astria-sequencer/src/app/tests_block_fees.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/astria-sequencer/src/app/tests_block_fees.rs b/crates/astria-sequencer/src/app/tests_block_fees.rs index 0079af0de..30241e2b4 100644 --- a/crates/astria-sequencer/src/app/tests_block_fees.rs +++ b/crates/astria-sequencer/src/app/tests_block_fees.rs @@ -79,7 +79,9 @@ async fn transaction_execution_records_fee_event() { assert_eq!(event.kind, "tx.fees"); assert_eq!( event.attributes[0], - ("asset", nria().to_ibc_prefixed().to_string()).index().into() + ("asset", nria().to_ibc_prefixed().to_string()) + .index() + .into() ); assert_eq!( event.attributes[1], From d69191af5c7a392295012a62f32a878c120267a2 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Wed, 2 Oct 2024 10:58:36 -0500 Subject: [PATCH 9/9] requested changes --- .../astria-sequencer/src/accounts/action.rs | 50 +- .../src/app/action_handler.rs | 20 +- .../astria-sequencer/src/app/fee_handler.rs | 21 - crates/astria-sequencer/src/app/mod.rs | 33 +- .../astria-sequencer/src/app/tests_app/mod.rs | 1 + .../src/app/tests_block_fees.rs | 26 +- .../src/app/tests_execute_transaction.rs | 10 +- .../astria-sequencer/src/assets/state_ext.rs | 149 +--- .../astria-sequencer/src/authority/action.rs | 33 +- .../src/bridge/bridge_lock_action.rs | 295 +------- .../src/bridge/bridge_sudo_change_action.rs | 54 +- .../src/bridge/bridge_unlock_action.rs | 58 +- .../src/bridge/init_bridge_account_action.rs | 51 +- crates/astria-sequencer/src/bridge/mod.rs | 1 - .../astria-sequencer/src/fee_asset_change.rs | 12 +- crates/astria-sequencer/src/fees/mod.rs | 672 ++++++++++++++++++ crates/astria-sequencer/src/fees/state_ext.rs | 162 +++++ .../src/ibc/ibc_relayer_change.rs | 12 +- .../src/ibc/ics20_withdrawal.rs | 45 -- crates/astria-sequencer/src/lib.rs | 1 + .../astria-sequencer/src/sequence/action.rs | 98 +-- crates/astria-sequencer/src/sequence/mod.rs | 1 - .../src/transaction/checks.rs | 8 +- .../astria-sequencer/src/transaction/mod.rs | 53 +- 24 files changed, 915 insertions(+), 951 deletions(-) delete mode 100644 crates/astria-sequencer/src/app/fee_handler.rs create mode 100644 crates/astria-sequencer/src/fees/mod.rs create mode 100644 crates/astria-sequencer/src/fees/state_ext.rs diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index a26ef6c23..083d4668a 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -9,10 +9,6 @@ use cnidarium::{ StateRead, StateWrite, }; -use tracing::{ - instrument, - Level, -}; use super::AddressBytes; use crate::{ @@ -21,14 +17,8 @@ use crate::{ StateWriteExt as _, }, address::StateReadExt as _, - app::{ - ActionHandler, - FeeHandler, - }, - assets::{ - StateReadExt as _, - StateWriteExt as _, - }, + app::ActionHandler, + assets::StateReadExt as _, bridge::StateReadExt as _, transaction::StateReadExt as _, }; @@ -61,42 +51,6 @@ impl ActionHandler for TransferAction { } } -#[async_trait::async_trait] -impl FeeHandler for TransferAction { - #[instrument(skip_all, err(level = Level::WARN))] - async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { - let tx_context = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action"); - let from = tx_context.address_bytes(); - let fee = state - .get_transfer_base_fee() - .await - .wrap_err("failed to get transfer base fee")?; - - ensure!( - state - .is_allowed_fee_asset(&self.fee_asset) - .await - .wrap_err("failed to check allowed fee assets in state")?, - "invalid fee asset", - ); - - state - .decrease_balance(&from, &self.fee_asset, fee) - .await - .wrap_err("failed to decrease balance for fee payment")?; - state.add_fee_to_block_fees( - &self.fee_asset, - fee, - tx_context.transaction_id, - tx_context.source_action_index, - )?; - - Ok(()) - } -} - pub(crate) async fn execute_transfer( action: &TransferAction, from: &TAddress, diff --git a/crates/astria-sequencer/src/app/action_handler.rs b/crates/astria-sequencer/src/app/action_handler.rs index 758a1e163..1ef67ea4b 100644 --- a/crates/astria-sequencer/src/app/action_handler.rs +++ b/crates/astria-sequencer/src/app/action_handler.rs @@ -1,9 +1,14 @@ use cnidarium::StateWrite; -use crate::app::fee_handler::FeeHandler; - +/// This trait is a verbatim copy of `cnidarium_component::ActionHandler`. +/// +/// It's duplicated here because all actions are foreign types, forbidding +/// the implementation of [`cnidarium_component::ActionHandler`][1] for +/// these types due to Rust orphan rules. +/// +/// [1]: https://github.com/penumbra-zone/penumbra/blob/14959350abcb8cfbf33f9aedc7463fccfd8e3f9f/crates/cnidarium-component/src/action_handler.rs#L30 #[async_trait::async_trait] -pub(crate) trait ActionHandler: FeeHandler { +pub(crate) trait ActionHandler { // Commenting out for the time being as this is currently not being used. Leaving this in // for reference as this is copied from cnidarium_component. // ``` @@ -16,15 +21,6 @@ pub(crate) trait ActionHandler: FeeHandler { async fn check_stateless(&self) -> astria_eyre::eyre::Result<()>; - async fn check_execute_and_pay_fees( - &self, - mut state: S, - ) -> astria_eyre::eyre::Result<()> { - self.check_and_execute(&mut state).await?; - self.calculate_and_pay_fees(&mut state).await?; - Ok(()) - } - async fn check_and_execute(&self, mut state: S) -> astria_eyre::eyre::Result<()>; } diff --git a/crates/astria-sequencer/src/app/fee_handler.rs b/crates/astria-sequencer/src/app/fee_handler.rs deleted file mode 100644 index 38bc9726e..000000000 --- a/crates/astria-sequencer/src/app/fee_handler.rs +++ /dev/null @@ -1,21 +0,0 @@ -use astria_core::primitive::v1::{ - asset, - TransactionId, -}; -use cnidarium::StateWrite; - -#[async_trait::async_trait] -pub(crate) trait FeeHandler { - async fn calculate_and_pay_fees( - &self, - mut state: S, - ) -> astria_eyre::eyre::Result<()>; -} - -#[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub(crate) struct Fee { - pub(crate) asset: asset::Denom, - pub(crate) amount: u128, - pub(crate) source_transaction_id: TransactionId, - pub(crate) source_action_index: u64, -} diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index 0a6d02905..d81294893 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -14,7 +14,6 @@ mod tests_breaking_changes; #[cfg(test)] mod tests_execute_transaction; -mod fee_handler; use std::{ collections::VecDeque, sync::Arc, @@ -53,10 +52,6 @@ use cnidarium::{ StateWrite, Storage, }; -pub(crate) use fee_handler::{ - Fee, - FeeHandler, -}; use prost::Message as _; use sha2::{ Digest as _, @@ -69,7 +64,6 @@ use tendermint::{ types::ExecTxResult, Code, Event, - EventAttributeIndexExt as _, }, account, block::Header, @@ -95,10 +89,7 @@ use crate::{ StateWriteExt as _, }, address::StateWriteExt as _, - assets::{ - StateReadExt as _, - StateWriteExt as _, - }, + assets::StateWriteExt as _, authority::{ component::{ AuthorityComponent, @@ -113,6 +104,10 @@ use crate::{ StateWriteExt as _, }, component::Component as _, + fees::{ + construct_tx_fee_event, + StateReadExt as _, + }, grpc::StateWriteExt as _, ibc::component::IbcComponent, mempool::{ @@ -1100,16 +1095,13 @@ impl App { for fee in fees { state_tx - .increase_balance(fee_recipient, &fee.asset, fee.amount) + .increase_balance(fee_recipient, fee.asset(), fee.amount()) .await .wrap_err("failed to increase fee recipient balance")?; let fee_event = construct_tx_fee_event(&fee); state_tx.record(fee_event); } - // clear block fees - state_tx.clear_block_fees(); - let events = self.apply(state_tx); Ok(abci::response::EndBlock { validator_updates: validator_updates @@ -1197,16 +1189,3 @@ fn signed_transaction_from_bytes(bytes: &[u8]) -> Result { Ok(tx) } - -/// Creates `abci::Event` of kind `tx.fees` for sequencer fee reporting -fn construct_tx_fee_event(fee: &Fee) -> Event { - Event::new( - "tx.fees", - [ - ("asset", fee.asset.to_string()).index(), - ("feeAmount", fee.amount.to_string()).index(), - ("sourceTransactionId", fee.source_transaction_id.to_string()).index(), - ("sourceActionIndex", fee.source_action_index.to_string()).index(), - ], - ) -} diff --git a/crates/astria-sequencer/src/app/tests_app/mod.rs b/crates/astria-sequencer/src/app/tests_app/mod.rs index dc93498f4..97d4aba7c 100644 --- a/crates/astria-sequencer/src/app/tests_app/mod.rs +++ b/crates/astria-sequencer/src/app/tests_app/mod.rs @@ -55,6 +55,7 @@ use crate::{ ValidatorSet, }, bridge::StateWriteExt as _, + fees::StateReadExt as _, proposal::commitment::generate_rollup_datas_commitment, test_utils::{ astria_address, diff --git a/crates/astria-sequencer/src/app/tests_block_fees.rs b/crates/astria-sequencer/src/app/tests_block_fees.rs index 30241e2b4..9560cbce6 100644 --- a/crates/astria-sequencer/src/app/tests_block_fees.rs +++ b/crates/astria-sequencer/src/app/tests_block_fees.rs @@ -28,16 +28,14 @@ use crate::{ initialize_app, BOB_ADDRESS, }, - assets::StateReadExt as _, authority::StateReadExt as _, - bridge::{ + bridge::StateWriteExt as _, + fees::{ calculate_base_deposit_fee, - StateWriteExt as _, - }, - sequence::{ - calculate_fee_from_state, - StateWriteExt as _, + calculate_sequence_action_fee_from_state, + StateReadExt as _, }, + sequence::StateWriteExt as _, test_utils::{ astria_address, astria_address_from_hex_string, @@ -130,7 +128,7 @@ async fn ensure_correct_block_fees_transfer() { .get_block_fees() .unwrap() .into_iter() - .map(|fee| fee.amount) + .map(|fee| fee.amount()) .sum(); assert_eq!(total_block_fees, transfer_base_fee); } @@ -170,9 +168,11 @@ async fn ensure_correct_block_fees_sequence() { .get_block_fees() .unwrap() .into_iter() - .map(|fee| fee.amount) + .map(|fee| fee.amount()) .sum(); - let expected_fees = calculate_fee_from_state(&data, &app.state).await.unwrap(); + let expected_fees = calculate_sequence_action_fee_from_state(&data, &app.state) + .await + .unwrap(); assert_eq!(total_block_fees, expected_fees); } @@ -212,7 +212,7 @@ async fn ensure_correct_block_fees_init_bridge_acct() { .get_block_fees() .unwrap() .into_iter() - .map(|fee| fee.amount) + .map(|fee| fee.amount()) .sum(); assert_eq!(total_block_fees, init_bridge_account_base_fee); } @@ -277,7 +277,7 @@ async fn ensure_correct_block_fees_bridge_lock() { .get_block_fees() .unwrap() .into_iter() - .map(|fee| fee.amount) + .map(|fee| fee.amount()) .sum(); let expected_fees = transfer_base_fee + (calculate_base_deposit_fee(&test_deposit).unwrap() * bridge_lock_byte_cost_multiplier); @@ -330,7 +330,7 @@ async fn ensure_correct_block_fees_bridge_sudo_change() { .get_block_fees() .unwrap() .into_iter() - .map(|fee| fee.amount) + .map(|fee| fee.amount()) .sum(); assert_eq!(total_block_fees, sudo_change_base_fee); } diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 7baf724a1..88873fe90 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -53,8 +53,8 @@ use crate::{ StateReadExt as _, StateWriteExt as _, }, + fees::calculate_sequence_action_fee_from_state, ibc::StateReadExt as _, - sequence::calculate_fee_from_state, test_utils::{ astria_address, astria_address_from_hex_string, @@ -266,7 +266,9 @@ async fn app_execute_transaction_sequence() { let alice = get_alice_signing_key(); let alice_address = astria_address(&alice.address_bytes()); let data = Bytes::from_static(b"hello world"); - let fee = calculate_fee_from_state(&data, &app.state).await.unwrap(); + let fee = calculate_sequence_action_fee_from_state(&data, &app.state) + .await + .unwrap(); let tx = UnsignedTransaction::builder() .actions(vec![ @@ -757,7 +759,7 @@ async fn app_execute_transaction_bridge_lock_action_ok() { .get_bridge_lock_byte_cost_multiplier() .await .unwrap() - * crate::bridge::calculate_base_deposit_fee(&expected_deposit).unwrap(); + * crate::fees::calculate_base_deposit_fee(&expected_deposit).unwrap(); assert_eq!( app.state .get_account_balance(&alice_address, &nria()) @@ -918,7 +920,7 @@ async fn app_stateful_check_fails_insufficient_total_balance() { // figure out needed fee for a single transfer let data = Bytes::from_static(b"hello world"); - let fee = calculate_fee_from_state(&data, &app.state.clone()) + let fee = calculate_sequence_action_fee_from_state(&data, &app.state.clone()) .await .unwrap(); diff --git a/crates/astria-sequencer/src/assets/state_ext.rs b/crates/astria-sequencer/src/assets/state_ext.rs index aff5f2edc..265729cd5 100644 --- a/crates/astria-sequencer/src/assets/state_ext.rs +++ b/crates/astria-sequencer/src/assets/state_ext.rs @@ -1,7 +1,4 @@ -use astria_core::primitive::v1::{ - asset, - TransactionId, -}; +use astria_core::primitive::v1::asset; use astria_eyre::{ anyhow_to_eyre, eyre::{ @@ -19,12 +16,8 @@ use futures::StreamExt as _; use tracing::instrument; use super::storage; -use crate::{ - app::Fee, - storage::StoredValue, -}; +use crate::storage::StoredValue; -const BLOCK_FEES_PREFIX: &str = "block_fees"; const FEE_ASSET_PREFIX: &str = "fee_asset/"; const NATIVE_ASSET_KEY: &str = "nativeasset"; @@ -99,18 +92,6 @@ pub(crate) trait StateReadExt: StateRead { .wrap_err("invalid ibc asset bytes") } - #[instrument(skip_all)] - fn get_block_fees(&self) -> Result> { - let mut block_fees = self.object_get(BLOCK_FEES_PREFIX); - match block_fees { - Some(_) => {} - None => { - block_fees = Some(vec![]); - } - } - Ok(block_fees.expect("block fees should not be `None` after populating")) - } - #[instrument(skip_all)] async fn is_allowed_fee_asset<'a, TAsset>(&self, asset: &'a TAsset) -> Result where @@ -171,46 +152,6 @@ pub(crate) trait StateWriteExt: StateWrite { Ok(()) } - /// Constructs and adds `Fee` object to the block fees vec. - #[instrument(skip_all)] - fn add_fee_to_block_fees<'a, TAsset>( - &mut self, - asset: &'a TAsset, - amount: u128, - source_transaction_id: TransactionId, - source_action_index: u64, - ) -> Result<()> - where - TAsset: Sync + std::fmt::Display, - asset::IbcPrefixed: From<&'a TAsset>, - { - let mut current_fees: Option> = self.object_get(BLOCK_FEES_PREFIX); - - match current_fees { - Some(_) => {} - None => { - current_fees = Some(vec![]); - } - } - - let mut current_fees = - current_fees.expect("block fees should not be `None` after populating"); - current_fees.push(Fee { - asset: asset::IbcPrefixed::from(asset).into(), - amount, - source_transaction_id, - source_action_index, - }); - - self.object_put(BLOCK_FEES_PREFIX, current_fees); - Ok(()) - } - - #[instrument(skip_all)] - fn clear_block_fees(&mut self) { - self.object_delete(BLOCK_FEES_PREFIX); - } - #[instrument(skip_all)] fn delete_allowed_fee_asset<'a, TAsset>(&mut self, asset: &'a TAsset) where @@ -238,10 +179,7 @@ impl StateWriteExt for T {} mod tests { use std::collections::HashSet; - use astria_core::primitive::v1::{ - asset, - TransactionId, - }; + use astria_core::primitive::v1::asset; use cnidarium::StateDelta; use super::{ @@ -250,7 +188,6 @@ mod tests { StateReadExt as _, StateWriteExt as _, }; - use crate::app::Fee; fn asset() -> asset::Denom { "asset".parse().unwrap() @@ -301,86 +238,6 @@ mod tests { ); } - #[tokio::test] - async fn block_fee_read_and_increase() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - // doesn't exist at first - let fee_balances_orig = state.get_block_fees().unwrap(); - assert!(fee_balances_orig.is_empty()); - - // can write - let asset = asset_0(); - let amount = 100u128; - state - .add_fee_to_block_fees(&asset, amount, TransactionId::new([0; 32]), 0) - .unwrap(); - - // holds expected - let fee_balances_updated = state.get_block_fees().unwrap(); - assert_eq!( - fee_balances_updated[0], - Fee { - asset: asset.to_ibc_prefixed().into(), - amount, - source_transaction_id: TransactionId::new([0; 32]), - source_action_index: 0 - }, - "fee balances are not what they were expected to be" - ); - } - - #[tokio::test] - async fn block_fee_read_and_increase_can_delete() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - // can write - let asset_first = asset_0(); - let asset_second = asset_1(); - let amount_first = 100u128; - let amount_second = 200u128; - - state - .add_fee_to_block_fees(&asset_first, amount_first, TransactionId::new([0; 32]), 0) - .unwrap(); - state - .add_fee_to_block_fees(&asset_second, amount_second, TransactionId::new([0; 32]), 1) - .unwrap(); - // holds expected - let fee_balances = HashSet::<_>::from_iter(state.get_block_fees().unwrap()); - assert_eq!( - fee_balances, - HashSet::from_iter(vec![ - Fee { - asset: asset_first.to_ibc_prefixed().into(), - amount: amount_first, - source_transaction_id: TransactionId::new([0; 32]), - source_action_index: 0 - }, - Fee { - asset: asset_second.to_ibc_prefixed().into(), - amount: amount_second, - source_transaction_id: TransactionId::new([0; 32]), - source_action_index: 1 - }, - ]), - "returned fee balance vector not what was expected" - ); - - // can delete - state.clear_block_fees(); - - let fee_balances_updated = state.get_block_fees().unwrap(); - assert!( - fee_balances_updated.is_empty(), - "fee balances were expected to be deleted but were not" - ); - } - #[tokio::test] async fn get_ibc_asset_non_existent() { let storage = cnidarium::TempStorage::new().await.unwrap(); diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index 4f361588f..ac044f601 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -16,10 +16,7 @@ use cnidarium::StateWrite; use crate::{ accounts::StateWriteExt as _, address::StateReadExt as _, - app::{ - ActionHandler, - FeeHandler, - }, + app::ActionHandler, authority::{ StateReadExt as _, StateWriteExt as _, @@ -79,13 +76,6 @@ impl ActionHandler for ValidatorUpdate { } } -#[async_trait::async_trait] -impl FeeHandler for ValidatorUpdate { - async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { - Ok(()) - } -} - #[async_trait::async_trait] impl ActionHandler for SudoAddressChangeAction { async fn check_stateless(&self) -> Result<()> { @@ -116,13 +106,6 @@ impl ActionHandler for SudoAddressChangeAction { } } -#[async_trait::async_trait] -impl FeeHandler for SudoAddressChangeAction { - async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { - Ok(()) - } -} - #[async_trait::async_trait] impl ActionHandler for FeeChangeAction { async fn check_stateless(&self) -> Result<()> { @@ -169,13 +152,6 @@ impl ActionHandler for FeeChangeAction { } } -#[async_trait::async_trait] -impl FeeHandler for FeeChangeAction { - async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { - Ok(()) - } -} - #[async_trait::async_trait] impl ActionHandler for IbcSudoChangeAction { async fn check_stateless(&self) -> Result<()> { @@ -204,13 +180,6 @@ impl ActionHandler for IbcSudoChangeAction { } } -#[async_trait::async_trait] -impl FeeHandler for IbcSudoChangeAction { - async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { - Ok(()) - } -} - #[cfg(test)] mod tests { use astria_core::primitive::v1::TransactionId; diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 45700a7d3..35c57fea7 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -12,41 +12,21 @@ use astria_eyre::eyre::{ WrapErr as _, }; use cnidarium::StateWrite; -use tracing::{ - instrument, - Level, -}; use crate::{ - accounts::{ - action::{ - check_transfer, - execute_transfer, - }, - StateReadExt as _, - StateWriteExt as _, + accounts::action::{ + check_transfer, + execute_transfer, }, address::StateReadExt as _, - app::{ - ActionHandler, - FeeHandler, - }, - assets::{ - StateReadExt as _, - StateWriteExt as _, - }, + app::ActionHandler, bridge::{ StateReadExt as _, StateWriteExt as _, }, transaction::StateReadExt as _, - utils::create_deposit_event, }; -/// The base byte length of a deposit, as determined by -/// [`tests::get_base_deposit_fee()`]. -const DEPOSIT_BASE_FEE: u128 = 16; - #[async_trait::async_trait] impl ActionHandler for BridgeLockAction { async fn check_stateless(&self) -> Result<()> { @@ -111,270 +91,3 @@ impl ActionHandler for BridgeLockAction { Ok(()) } } - -#[async_trait::async_trait] -impl FeeHandler for BridgeLockAction { - #[instrument(skip_all, err(level = Level::WARN))] - async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { - let tx_context = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action"); - let rollup_id = state - .get_bridge_account_rollup_id(&self.to) - .await - .wrap_err("failed to get bridge account rollup id")? - .ok_or_eyre("bridge lock must be sent to a bridge account")?; - let transfer_fee = state - .get_transfer_base_fee() - .await - .context("failed to get transfer base fee")?; - let from = tx_context.address_bytes(); - let source_transaction_id = tx_context.transaction_id; - let source_action_index = tx_context.source_action_index; - - ensure!( - state - .is_allowed_fee_asset(&self.fee_asset) - .await - .wrap_err("failed to check allowed fee assets in state")?, - "invalid fee asset", - ); - - let deposit = Deposit { - bridge_address: self.to, - rollup_id, - amount: self.amount, - asset: self.asset.clone(), - destination_chain_address: self.destination_chain_address.clone(), - source_transaction_id, - source_action_index, - }; - let deposit_abci_event = create_deposit_event(&deposit); - - let byte_cost_multiplier = state - .get_bridge_lock_byte_cost_multiplier() - .await - .wrap_err("failed to get byte cost multiplier")?; - - let fee = byte_cost_multiplier - .saturating_mul(calculate_base_deposit_fee(&deposit).unwrap_or(u128::MAX)) - .saturating_add(transfer_fee); - - state - .add_fee_to_block_fees( - &self.fee_asset, - fee, - tx_context.transaction_id, - source_action_index, - ) - .wrap_err("failed to add to block fees")?; - state - .decrease_balance(&from, &self.fee_asset, fee) - .await - .wrap_err("failed to deduct fee from account balance")?; - - state.record(deposit_abci_event); - Ok(()) - } -} - -/// Returns a modified byte length of the deposit event. Length is calculated with reasonable values -/// for all fields except `asset` and `destination_chain_address`, ergo it may not be representative -/// of on-wire length. -pub(crate) fn calculate_base_deposit_fee(deposit: &Deposit) -> Option { - deposit - .asset - .display_len() - .checked_add(deposit.destination_chain_address.len()) - .and_then(|var_len| { - DEPOSIT_BASE_FEE.checked_add(u128::try_from(var_len).expect( - "converting a usize to a u128 should work on any currently existing machine", - )) - }) -} - -#[cfg(test)] -mod tests { - use astria_core::primitive::v1::{ - asset::{ - self, - }, - Address, - RollupId, - TransactionId, - ADDRESS_LEN, - ROLLUP_ID_LEN, - TRANSACTION_ID_LEN, - }; - use cnidarium::StateDelta; - - use super::*; - use crate::{ - address::StateWriteExt as _, - test_utils::{ - assert_eyre_error, - astria_address, - ASTRIA_PREFIX, - }, - transaction::{ - StateWriteExt as _, - TransactionContext, - }, - }; - - fn test_asset() -> asset::Denom { - "test".parse().unwrap() - } - - #[tokio::test] - async fn execute_fee_calc() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - let transfer_fee = 12; - - let from_address = astria_address(&[2; 20]); - let transaction_id = TransactionId::new([0; 32]); - state.put_transaction_context(TransactionContext { - address_bytes: from_address.bytes(), - transaction_id, - source_action_index: 0, - }); - state.put_base_prefix(ASTRIA_PREFIX.to_string()).unwrap(); - - state.put_transfer_base_fee(transfer_fee).unwrap(); - state.put_bridge_lock_byte_cost_multiplier(2).unwrap(); - - let bridge_address = astria_address(&[1; 20]); - let asset = test_asset(); - let bridge_lock = BridgeLockAction { - to: bridge_address, - asset: asset.clone(), - amount: 100, - fee_asset: asset.clone(), - destination_chain_address: "someaddress".to_string(), - }; - - let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - state - .put_bridge_account_rollup_id(&bridge_address, rollup_id) - .unwrap(); - state - .put_bridge_account_ibc_asset(&bridge_address, asset.clone()) - .unwrap(); - state.put_allowed_fee_asset(&asset).unwrap(); - - // not enough balance; should fail - state - .put_account_balance(&from_address, &asset, transfer_fee) - .unwrap(); - assert_eyre_error( - &bridge_lock - .check_execute_and_pay_fees(&mut state) - .await - .unwrap_err(), - "insufficient funds for transfer and fee payment", - ); - - // enough balance; should pass - let expected_deposit_fee = transfer_fee - + calculate_base_deposit_fee(&Deposit { - bridge_address, - rollup_id, - amount: 100, - asset: asset.clone(), - destination_chain_address: "someaddress".to_string(), - source_transaction_id: transaction_id, - source_action_index: 0, - }) - .unwrap() - * 2; - state - .put_account_balance(&from_address, &asset, 100 + expected_deposit_fee) - .unwrap(); - bridge_lock.check_and_execute(&mut state).await.unwrap(); - } - - #[test] - fn calculated_base_deposit_fee_matches_expected_value() { - assert_correct_base_deposit_fee(&Deposit { - amount: u128::MAX, - source_action_index: u64::MAX, - ..reference_deposit() - }); - assert_correct_base_deposit_fee(&Deposit { - asset: "test_asset".parse().unwrap(), - ..reference_deposit() - }); - assert_correct_base_deposit_fee(&Deposit { - destination_chain_address: "someaddresslonger".to_string(), - ..reference_deposit() - }); - - // Ensure calculated length is as expected with absurd string - // lengths (have tested up to 99999999, but this makes testing very slow) - let absurd_string: String = ['a'; u16::MAX as usize].iter().collect(); - assert_correct_base_deposit_fee(&Deposit { - asset: absurd_string.parse().unwrap(), - ..reference_deposit() - }); - assert_correct_base_deposit_fee(&Deposit { - destination_chain_address: absurd_string, - ..reference_deposit() - }); - } - - #[track_caller] - #[expect( - clippy::arithmetic_side_effects, - reason = "adding length of strings will never overflow u128 on currently existing machines" - )] - fn assert_correct_base_deposit_fee(deposit: &Deposit) { - let calculated_len = calculate_base_deposit_fee(deposit).unwrap(); - let expected_len = DEPOSIT_BASE_FEE - + deposit.asset.to_string().len() as u128 - + deposit.destination_chain_address.len() as u128; - assert_eq!(calculated_len, expected_len); - } - - /// Used to determine the base deposit byte length for `get_deposit_byte_len()`. This is based - /// on "reasonable" values for all fields except `asset` and `destination_chain_address`. These - /// are empty strings, whose length will be added to the base cost at the time of - /// calculation. - /// - /// This test determines 165 bytes for an average deposit with empty `asset` and - /// `destination_chain_address`, which is divided by 10 to get our base byte length of 16. This - /// is to allow for more flexibility in overall fees (we have more flexibility multiplying by a - /// lower number, and if we want fees to be higher we can just raise the multiplier). - #[test] - fn get_base_deposit_fee() { - use prost::Message as _; - let bridge_address = Address::builder() - .prefix("astria-bridge") - .slice(&[0u8; ADDRESS_LEN][..]) - .try_build() - .unwrap(); - let raw_deposit = astria_core::generated::sequencerblock::v1alpha1::Deposit { - bridge_address: Some(bridge_address.to_raw()), - rollup_id: Some(RollupId::from_unhashed_bytes([0; ROLLUP_ID_LEN]).to_raw()), - amount: Some(1000.into()), - asset: String::new(), - destination_chain_address: String::new(), - source_transaction_id: Some(TransactionId::new([0; TRANSACTION_ID_LEN]).to_raw()), - source_action_index: 0, - }; - assert_eq!(DEPOSIT_BASE_FEE, raw_deposit.encoded_len() as u128 / 10); - } - - fn reference_deposit() -> Deposit { - Deposit { - bridge_address: astria_address(&[1; 20]), - rollup_id: RollupId::from_unhashed_bytes(b"test_rollup_id"), - amount: 0, - asset: "test".parse().unwrap(), - destination_chain_address: "someaddress".to_string(), - source_transaction_id: TransactionId::new([0; 32]), - source_action_index: 0, - } - } -} diff --git a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs index 5c4bed62d..3eeea7b2d 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -6,22 +6,10 @@ use astria_eyre::eyre::{ WrapErr as _, }; use cnidarium::StateWrite; -use tracing::{ - instrument, - Level, -}; use crate::{ - accounts::StateWriteExt as _, address::StateReadExt as _, - app::{ - ActionHandler, - FeeHandler, - }, - assets::{ - StateReadExt as _, - StateWriteExt as _, - }, + app::ActionHandler, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -88,44 +76,6 @@ impl ActionHandler for BridgeSudoChangeAction { } } -#[async_trait::async_trait] -impl FeeHandler for BridgeSudoChangeAction { - #[instrument(skip_all, err(level = Level::WARN))] - async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { - let tx_context = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action"); - let from = tx_context.address_bytes(); - let fee = state - .get_bridge_sudo_change_base_fee() - .await - .wrap_err("failed to get bridge sudo change fee")?; - - ensure!( - state - .is_allowed_fee_asset(&self.fee_asset) - .await - .wrap_err("failed to check allowed fee assets in state")?, - "invalid fee asset", - ); - - state - .add_fee_to_block_fees( - &self.fee_asset, - fee, - tx_context.transaction_id, - tx_context.source_action_index, - ) - .wrap_err("failed to add to block fees")?; - state - .decrease_balance(&from, &self.fee_asset, fee) - .await - .wrap_err("failed to decrease balance for bridge sudo change fee")?; - - Ok(()) - } -} - #[cfg(test)] mod tests { use astria_core::primitive::v1::{ @@ -136,7 +86,9 @@ mod tests { use super::*; use crate::{ + accounts::StateWriteExt as _, address::StateWriteExt as _, + assets::StateWriteExt as _, test_utils::{ astria_address, ASTRIA_PREFIX, diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index ecb6e7f4b..787000f9a 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -9,29 +9,14 @@ use astria_eyre::eyre::{ WrapErr as _, }; use cnidarium::StateWrite; -use tracing::{ - instrument, - Level, -}; use crate::{ - accounts::{ - action::{ - check_transfer, - execute_transfer, - }, - StateReadExt as _, - StateWriteExt as _, + accounts::action::{ + check_transfer, + execute_transfer, }, address::StateReadExt as _, - app::{ - ActionHandler, - FeeHandler, - }, - assets::{ - StateReadExt as _, - StateWriteExt as _, - }, + app::ActionHandler, bridge::{ StateReadExt as _, StateWriteExt as _, @@ -115,41 +100,6 @@ impl ActionHandler for BridgeUnlockAction { } } -#[async_trait::async_trait] -impl FeeHandler for BridgeUnlockAction { - #[instrument(skip_all, err(level = Level::WARN))] - async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { - let tx_context = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action"); - let from = tx_context.address_bytes(); - let transfer_fee = state - .get_transfer_base_fee() - .await - .wrap_err("failed to get transfer base fee for bridge unlock action")?; - - ensure!( - state - .is_allowed_fee_asset(&self.fee_asset) - .await - .wrap_err("failed to check allowed fee assets in state")?, - "invalid fee asset", - ); - - state - .decrease_balance(&from, &self.fee_asset, transfer_fee) - .await - .wrap_err("failed to decrease balance for fee payment")?; - state.add_fee_to_block_fees( - &self.fee_asset, - transfer_fee, - tx_context.transaction_id, - tx_context.source_action_index, - )?; - Ok(()) - } -} - #[cfg(test)] mod tests { use astria_core::{ diff --git a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs index 2be5f30d0..e512d0c07 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -4,27 +4,14 @@ use astria_core::{ }; use astria_eyre::eyre::{ bail, - ensure, Result, WrapErr as _, }; use cnidarium::StateWrite; -use tracing::{ - instrument, - Level, -}; use crate::{ - accounts::StateWriteExt as _, address::StateReadExt as _, - app::{ - ActionHandler, - FeeHandler, - }, - assets::{ - StateReadExt as _, - StateWriteExt as _, - }, + app::ActionHandler, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -94,39 +81,3 @@ impl ActionHandler for InitBridgeAccountAction { Ok(()) } } - -#[async_trait::async_trait] -impl FeeHandler for InitBridgeAccountAction { - #[instrument(skip_all, err(level = Level::WARN))] - async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { - let tx_context = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action"); - let from = tx_context.address_bytes(); - let fee = state - .get_init_bridge_account_base_fee() - .await - .wrap_err("failed to get init bridge account base fee")?; - - ensure!( - state - .is_allowed_fee_asset(&self.fee_asset) - .await - .wrap_err("failed to check allowed fee assets in state")?, - "invalid fee asset", - ); - - state - .decrease_balance(&from, &self.fee_asset, fee) - .await - .wrap_err("failed to decrease balance for fee payment")?; - state.add_fee_to_block_fees( - &self.fee_asset, - fee, - tx_context.transaction_id, - tx_context.source_action_index, - )?; - - Ok(()) - } -} diff --git a/crates/astria-sequencer/src/bridge/mod.rs b/crates/astria-sequencer/src/bridge/mod.rs index 33a5abaeb..db4f7196a 100644 --- a/crates/astria-sequencer/src/bridge/mod.rs +++ b/crates/astria-sequencer/src/bridge/mod.rs @@ -7,7 +7,6 @@ pub(crate) mod query; mod state_ext; pub(crate) mod storage; -pub(crate) use bridge_lock_action::calculate_base_deposit_fee; pub(crate) use state_ext::{ StateReadExt, StateWriteExt, diff --git a/crates/astria-sequencer/src/fee_asset_change.rs b/crates/astria-sequencer/src/fee_asset_change.rs index 74db129cb..326d5472e 100644 --- a/crates/astria-sequencer/src/fee_asset_change.rs +++ b/crates/astria-sequencer/src/fee_asset_change.rs @@ -9,10 +9,7 @@ use async_trait::async_trait; use cnidarium::StateWrite; use crate::{ - app::{ - ActionHandler, - FeeHandler, - }, + app::ActionHandler, assets::{ StateReadExt as _, StateWriteExt as _, @@ -62,10 +59,3 @@ impl ActionHandler for FeeAssetChangeAction { Ok(()) } } - -#[async_trait::async_trait] -impl FeeHandler for FeeAssetChangeAction { - async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { - Ok(()) - } -} diff --git a/crates/astria-sequencer/src/fees/mod.rs b/crates/astria-sequencer/src/fees/mod.rs new file mode 100644 index 000000000..74b8b0f58 --- /dev/null +++ b/crates/astria-sequencer/src/fees/mod.rs @@ -0,0 +1,672 @@ +use astria_core::{ + primitive::v1::{ + asset, + TransactionId, + }, + protocol::transaction::v1alpha1::action::{ + self, + BridgeLockAction, + BridgeSudoChangeAction, + BridgeUnlockAction, + FeeAssetChangeAction, + FeeChangeAction, + IbcRelayerChangeAction, + IbcSudoChangeAction, + InitBridgeAccountAction, + SequenceAction, + SudoAddressChangeAction, + TransferAction, + ValidatorUpdate, + }, + sequencerblock::v1alpha1::block::Deposit, +}; +use astria_eyre::eyre::{ + ensure, + OptionExt as _, + Result, + WrapErr as _, +}; +use cnidarium::StateWrite; +use tendermint::abci::{ + Event, + EventAttributeIndexExt as _, +}; +use tracing::{ + instrument, + Level, +}; + +use crate::{ + accounts::{ + StateReadExt as _, + StateWriteExt as _, + }, + assets::StateReadExt as _, + bridge::StateReadExt as _, + ibc::StateReadExt as _, + sequence, + transaction::StateReadExt as _, + utils::create_deposit_event, +}; + +mod state_ext; +pub(crate) use state_ext::{ + StateReadExt, + StateWriteExt, +}; + +/// The base byte length of a deposit, as determined by +/// [`tests::get_base_deposit_fee()`]. +const DEPOSIT_BASE_FEE: u128 = 16; + +#[async_trait::async_trait] +pub(crate) trait FeeHandler { + async fn check_and_pay_fees( + &self, + mut state: S, + ) -> astria_eyre::eyre::Result<()>; +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct Fee { + asset: asset::Denom, + amount: u128, + source_transaction_id: TransactionId, + source_action_index: u64, +} + +impl Fee { + pub(crate) fn asset(&self) -> &asset::Denom { + &self.asset + } + + pub(crate) fn amount(&self) -> u128 { + self.amount + } +} + +#[async_trait::async_trait] +impl FeeHandler for TransferAction { + #[instrument(skip_all, err(level = Level::WARN))] + async fn check_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); + let fee = state + .get_transfer_base_fee() + .await + .wrap_err("failed to get transfer base fee")?; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + state + .decrease_balance(&from, &self.fee_asset, fee) + .await + .wrap_err("failed to decrease balance for fee payment")?; + state.add_fee_to_block_fees( + &self.fee_asset, + fee, + tx_context.transaction_id, + tx_context.source_action_index, + )?; + + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for BridgeLockAction { + #[instrument(skip_all, err(level = Level::WARN))] + async fn check_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let rollup_id = state + .get_bridge_account_rollup_id(&self.to) + .await + .wrap_err("failed to get bridge account rollup id")? + .ok_or_eyre("bridge lock must be sent to a bridge account")?; + let transfer_fee = state + .get_transfer_base_fee() + .await + .context("failed to get transfer base fee")?; + let from = tx_context.address_bytes(); + let source_transaction_id = tx_context.transaction_id; + let source_action_index = tx_context.source_action_index; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + let deposit = Deposit { + bridge_address: self.to, + rollup_id, + amount: self.amount, + asset: self.asset.clone(), + destination_chain_address: self.destination_chain_address.clone(), + source_transaction_id, + source_action_index, + }; + let deposit_abci_event = create_deposit_event(&deposit); + + let byte_cost_multiplier = state + .get_bridge_lock_byte_cost_multiplier() + .await + .wrap_err("failed to get byte cost multiplier")?; + + let fee = byte_cost_multiplier + .saturating_mul(calculate_base_deposit_fee(&deposit).unwrap_or(u128::MAX)) + .saturating_add(transfer_fee); + + state + .add_fee_to_block_fees( + &self.fee_asset, + fee, + tx_context.transaction_id, + source_action_index, + ) + .wrap_err("failed to add to block fees")?; + state + .decrease_balance(&from, &self.fee_asset, fee) + .await + .wrap_err("failed to deduct fee from account balance")?; + + state.record(deposit_abci_event); + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for BridgeSudoChangeAction { + #[instrument(skip_all, err(level = Level::WARN))] + async fn check_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); + let fee = state + .get_bridge_sudo_change_base_fee() + .await + .wrap_err("failed to get bridge sudo change fee")?; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + state + .add_fee_to_block_fees( + &self.fee_asset, + fee, + tx_context.transaction_id, + tx_context.source_action_index, + ) + .wrap_err("failed to add to block fees")?; + state + .decrease_balance(&from, &self.fee_asset, fee) + .await + .wrap_err("failed to decrease balance for bridge sudo change fee")?; + + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for BridgeUnlockAction { + #[instrument(skip_all, err(level = Level::WARN))] + async fn check_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); + let transfer_fee = state + .get_transfer_base_fee() + .await + .wrap_err("failed to get transfer base fee for bridge unlock action")?; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + state + .decrease_balance(&from, &self.fee_asset, transfer_fee) + .await + .wrap_err("failed to decrease balance for fee payment")?; + state.add_fee_to_block_fees( + &self.fee_asset, + transfer_fee, + tx_context.transaction_id, + tx_context.source_action_index, + )?; + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for InitBridgeAccountAction { + #[instrument(skip_all, err(level = Level::WARN))] + async fn check_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); + let fee = state + .get_init_bridge_account_base_fee() + .await + .wrap_err("failed to get init bridge account base fee")?; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + state + .decrease_balance(&from, &self.fee_asset, fee) + .await + .wrap_err("failed to decrease balance for fee payment")?; + state.add_fee_to_block_fees( + &self.fee_asset, + fee, + tx_context.transaction_id, + tx_context.source_action_index, + )?; + + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for action::Ics20Withdrawal { + #[instrument(skip_all, err(level = Level::WARN))] + async fn check_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); + let fee = state + .get_ics20_withdrawal_base_fee() + .await + .wrap_err("failed to get ics20 withdrawal base fee")?; + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + state + .decrease_balance(&from, &self.fee_asset, fee) + .await + .wrap_err("failed to decrease balance for fee payment")?; + state.add_fee_to_block_fees( + &self.fee_asset, + fee, + tx_context.transaction_id, + tx_context.source_action_index, + )?; + + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for SequenceAction { + #[instrument(skip_all, err(level = Level::WARN))] + async fn check_and_pay_fees(&self, mut state: S) -> Result<()> { + let tx_context = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action"); + let from = tx_context.address_bytes(); + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .wrap_err("failed accessing state to check if fee is allowed")?, + "invalid fee asset", + ); + + let fee = calculate_sequence_action_fee_from_state(&self.data, &state) + .await + .wrap_err("calculated fee overflows u128")?; + + state + .add_fee_to_block_fees( + &self.fee_asset, + fee, + tx_context.transaction_id, + tx_context.source_action_index, + ) + .wrap_err("failed to add to block fees")?; + state + .decrease_balance(&from, &self.fee_asset, fee) + .await + .wrap_err("failed updating `from` account balance")?; + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for ValidatorUpdate { + async fn check_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for SudoAddressChangeAction { + async fn check_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for FeeChangeAction { + async fn check_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for IbcSudoChangeAction { + async fn check_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for IbcRelayerChangeAction { + async fn check_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for FeeAssetChangeAction { + async fn check_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +/// Returns a modified byte length of the deposit event. Length is calculated with reasonable values +/// for all fields except `asset` and `destination_chain_address`, ergo it may not be representative +/// of on-wire length. +pub(crate) fn calculate_base_deposit_fee(deposit: &Deposit) -> Option { + deposit + .asset + .display_len() + .checked_add(deposit.destination_chain_address.len()) + .and_then(|var_len| { + DEPOSIT_BASE_FEE.checked_add(u128::try_from(var_len).expect( + "converting a usize to a u128 should work on any currently existing machine", + )) + }) +} + +/// Calculates the fee for a sequence `Action` based on the length of the `data`. +pub(crate) async fn calculate_sequence_action_fee_from_state( + data: &[u8], + state: &S, +) -> Result { + let base_fee = state + .get_sequence_action_base_fee() + .await + .wrap_err("failed to get base fee")?; + let fee_per_byte = state + .get_sequence_action_byte_cost_multiplier() + .await + .wrap_err("failed to get fee per byte")?; + calculate_sequence_action_fee(data, fee_per_byte, base_fee) + .ok_or_eyre("calculated fee overflows u128") +} + +/// Calculates the fee for a sequence `Action` based on the length of the `data`. +/// Returns `None` if the fee overflows `u128`. +fn calculate_sequence_action_fee(data: &[u8], fee_per_byte: u128, base_fee: u128) -> Option { + base_fee.checked_add( + fee_per_byte.checked_mul( + data.len() + .try_into() + .expect("a usize should always convert to a u128"), + )?, + ) +} + +/// Creates `abci::Event` of kind `tx.fees` for sequencer fee reporting +pub(crate) fn construct_tx_fee_event(fee: &Fee) -> Event { + Event::new( + "tx.fees", + [ + ("asset", fee.asset.to_string()).index(), + ("feeAmount", fee.amount.to_string()).index(), + ("sourceTransactionId", fee.source_transaction_id.to_string()).index(), + ("sourceActionIndex", fee.source_action_index.to_string()).index(), + ], + ) +} + +#[cfg(test)] +mod tests { + use astria_core::{ + primitive::v1::{ + asset::{ + self, + }, + Address, + RollupId, + TransactionId, + ADDRESS_LEN, + ROLLUP_ID_LEN, + TRANSACTION_ID_LEN, + }, + protocol::transaction::v1alpha1::action::BridgeLockAction, + sequencerblock::v1alpha1::block::Deposit, + }; + use cnidarium::StateDelta; + + use crate::{ + accounts::StateWriteExt as _, + address::StateWriteExt as _, + app::ActionHandler as _, + assets::StateWriteExt as _, + bridge::StateWriteExt as _, + fees::{ + calculate_base_deposit_fee, + calculate_sequence_action_fee, + DEPOSIT_BASE_FEE, + }, + test_utils::{ + assert_eyre_error, + astria_address, + ASTRIA_PREFIX, + }, + transaction::{ + StateWriteExt as _, + TransactionContext, + }, + }; + + fn test_asset() -> asset::Denom { + "test".parse().unwrap() + } + + #[tokio::test] + async fn bridge_lock_fee_calculation_works_as_expected() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + let transfer_fee = 12; + + let from_address = astria_address(&[2; 20]); + let transaction_id = TransactionId::new([0; 32]); + state.put_transaction_context(TransactionContext { + address_bytes: from_address.bytes(), + transaction_id, + source_action_index: 0, + }); + state.put_base_prefix(ASTRIA_PREFIX.to_string()).unwrap(); + + state.put_transfer_base_fee(transfer_fee).unwrap(); + state.put_bridge_lock_byte_cost_multiplier(2).unwrap(); + + let bridge_address = astria_address(&[1; 20]); + let asset = test_asset(); + let bridge_lock = BridgeLockAction { + to: bridge_address, + asset: asset.clone(), + amount: 100, + fee_asset: asset.clone(), + destination_chain_address: "someaddress".to_string(), + }; + + let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); + state + .put_bridge_account_rollup_id(&bridge_address, rollup_id) + .unwrap(); + state + .put_bridge_account_ibc_asset(&bridge_address, asset.clone()) + .unwrap(); + state.put_allowed_fee_asset(&asset).unwrap(); + + // not enough balance; should fail + state + .put_account_balance(&from_address, &asset, transfer_fee) + .unwrap(); + assert_eyre_error( + &bridge_lock.check_and_execute(&mut state).await.unwrap_err(), + "insufficient funds for transfer and fee payment", + ); + + // enough balance; should pass + let expected_deposit_fee = transfer_fee + + calculate_base_deposit_fee(&Deposit { + bridge_address, + rollup_id, + amount: 100, + asset: asset.clone(), + destination_chain_address: "someaddress".to_string(), + source_transaction_id: transaction_id, + source_action_index: 0, + }) + .unwrap() + * 2; + state + .put_account_balance(&from_address, &asset, 100 + expected_deposit_fee) + .unwrap(); + bridge_lock.check_and_execute(&mut state).await.unwrap(); + } + + #[test] + fn calculated_base_deposit_fee_matches_expected_value() { + assert_correct_base_deposit_fee(&Deposit { + amount: u128::MAX, + source_action_index: u64::MAX, + ..reference_deposit() + }); + assert_correct_base_deposit_fee(&Deposit { + asset: "test_asset".parse().unwrap(), + ..reference_deposit() + }); + assert_correct_base_deposit_fee(&Deposit { + destination_chain_address: "someaddresslonger".to_string(), + ..reference_deposit() + }); + + // Ensure calculated length is as expected with absurd string + // lengths (have tested up to 99999999, but this makes testing very slow) + let absurd_string: String = ['a'; u16::MAX as usize].iter().collect(); + assert_correct_base_deposit_fee(&Deposit { + asset: absurd_string.parse().unwrap(), + ..reference_deposit() + }); + assert_correct_base_deposit_fee(&Deposit { + destination_chain_address: absurd_string, + ..reference_deposit() + }); + } + + #[track_caller] + #[expect( + clippy::arithmetic_side_effects, + reason = "adding length of strings will never overflow u128 on currently existing machines" + )] + fn assert_correct_base_deposit_fee(deposit: &Deposit) { + let calculated_len = calculate_base_deposit_fee(deposit).unwrap(); + let expected_len = DEPOSIT_BASE_FEE + + deposit.asset.to_string().len() as u128 + + deposit.destination_chain_address.len() as u128; + assert_eq!(calculated_len, expected_len); + } + + /// Used to determine the base deposit byte length for `get_deposit_byte_len()`. This is based + /// on "reasonable" values for all fields except `asset` and `destination_chain_address`. These + /// are empty strings, whose length will be added to the base cost at the time of + /// calculation. + /// + /// This test determines 165 bytes for an average deposit with empty `asset` and + /// `destination_chain_address`, which is divided by 10 to get our base byte length of 16. This + /// is to allow for more flexibility in overall fees (we have more flexibility multiplying by a + /// lower number, and if we want fees to be higher we can just raise the multiplier). + #[test] + fn get_base_deposit_fee() { + use prost::Message as _; + let bridge_address = Address::builder() + .prefix("astria-bridge") + .slice(&[0u8; ADDRESS_LEN][..]) + .try_build() + .unwrap(); + let raw_deposit = astria_core::generated::sequencerblock::v1alpha1::Deposit { + bridge_address: Some(bridge_address.to_raw()), + rollup_id: Some(RollupId::from_unhashed_bytes([0; ROLLUP_ID_LEN]).to_raw()), + amount: Some(1000.into()), + asset: String::new(), + destination_chain_address: String::new(), + source_transaction_id: Some(TransactionId::new([0; TRANSACTION_ID_LEN]).to_raw()), + source_action_index: 0, + }; + assert_eq!(DEPOSIT_BASE_FEE, raw_deposit.encoded_len() as u128 / 10); + } + + fn reference_deposit() -> Deposit { + Deposit { + bridge_address: astria_address(&[1; 20]), + rollup_id: RollupId::from_unhashed_bytes(b"test_rollup_id"), + amount: 0, + asset: "test".parse().unwrap(), + destination_chain_address: "someaddress".to_string(), + source_transaction_id: TransactionId::new([0; 32]), + source_action_index: 0, + } + } + + #[test] + fn calculate_sequence_action_fee_works_as_expected() { + assert_eq!(calculate_sequence_action_fee(&[], 1, 0), Some(0)); + assert_eq!(calculate_sequence_action_fee(&[0], 1, 0), Some(1)); + assert_eq!(calculate_sequence_action_fee(&[0u8; 10], 1, 0), Some(10)); + assert_eq!(calculate_sequence_action_fee(&[0u8; 10], 1, 100), Some(110)); + } +} diff --git a/crates/astria-sequencer/src/fees/state_ext.rs b/crates/astria-sequencer/src/fees/state_ext.rs new file mode 100644 index 000000000..6c9065e82 --- /dev/null +++ b/crates/astria-sequencer/src/fees/state_ext.rs @@ -0,0 +1,162 @@ +use astria_core::primitive::v1::{ + asset, + TransactionId, +}; +use astria_eyre::eyre::Result; +use async_trait::async_trait; +use cnidarium::{ + StateRead, + StateWrite, +}; +use tracing::instrument; + +use super::Fee; + +const BLOCK_FEES_PREFIX: &str = "block_fees"; + +#[async_trait] +pub(crate) trait StateReadExt: StateRead { + #[instrument(skip_all)] + fn get_block_fees(&self) -> Result> { + let mut block_fees = self.object_get(BLOCK_FEES_PREFIX); + match block_fees { + Some(_) => {} + None => { + block_fees = Some(vec![]); + } + } + Ok(block_fees.expect("block fees should not be `None` after populating")) + } +} + +impl StateReadExt for T {} + +#[async_trait] +pub(crate) trait StateWriteExt: StateWrite { + /// Constructs and adds `Fee` object to the block fees vec. + #[instrument(skip_all)] + fn add_fee_to_block_fees<'a, TAsset>( + &mut self, + asset: &'a TAsset, + amount: u128, + source_transaction_id: TransactionId, + source_action_index: u64, + ) -> Result<()> + where + TAsset: Sync + std::fmt::Display, + asset::IbcPrefixed: From<&'a TAsset>, + { + let current_fees: Option> = self.object_get(BLOCK_FEES_PREFIX); + + let fee = Fee { + asset: asset::IbcPrefixed::from(asset).into(), + amount, + source_transaction_id, + source_action_index, + }; + let new_fees = if let Some(mut fees) = current_fees { + fees.push(fee); + fees + } else { + vec![fee] + }; + + self.object_put(BLOCK_FEES_PREFIX, new_fees); + Ok(()) + } +} + +impl StateWriteExt for T {} + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + + use astria_core::primitive::v1::TransactionId; + use cnidarium::StateDelta; + + use crate::fees::{ + Fee, + StateReadExt as _, + StateWriteExt as _, + }; + + fn asset_0() -> astria_core::primitive::v1::asset::Denom { + "asset_0".parse().unwrap() + } + + fn asset_1() -> astria_core::primitive::v1::asset::Denom { + "asset_1".parse().unwrap() + } + + #[tokio::test] + async fn block_fee_read_and_increase() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + // doesn't exist at first + let fee_balances_orig = state.get_block_fees().unwrap(); + assert!(fee_balances_orig.is_empty()); + + // can write + let asset = asset_0(); + let amount = 100u128; + state + .add_fee_to_block_fees(&asset, amount, TransactionId::new([0; 32]), 0) + .unwrap(); + + // holds expected + let fee_balances_updated = state.get_block_fees().unwrap(); + assert_eq!( + fee_balances_updated[0], + Fee { + asset: asset.to_ibc_prefixed().into(), + amount, + source_transaction_id: TransactionId::new([0; 32]), + source_action_index: 0 + }, + "fee balances are not what they were expected to be" + ); + } + + #[tokio::test] + async fn block_fee_read_and_increase_can_delete() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + // can write + let asset_first = asset_0(); + let asset_second = asset_1(); + let amount_first = 100u128; + let amount_second = 200u128; + + state + .add_fee_to_block_fees(&asset_first, amount_first, TransactionId::new([0; 32]), 0) + .unwrap(); + state + .add_fee_to_block_fees(&asset_second, amount_second, TransactionId::new([0; 32]), 1) + .unwrap(); + // holds expected + let fee_balances = HashSet::<_>::from_iter(state.get_block_fees().unwrap()); + assert_eq!( + fee_balances, + HashSet::from_iter(vec![ + Fee { + asset: asset_first.to_ibc_prefixed().into(), + amount: amount_first, + source_transaction_id: TransactionId::new([0; 32]), + source_action_index: 0 + }, + Fee { + asset: asset_second.to_ibc_prefixed().into(), + amount: amount_second, + source_transaction_id: TransactionId::new([0; 32]), + source_action_index: 1 + }, + ]), + "returned fee balance vector not what was expected" + ); + } +} diff --git a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs index 9bdcb9b7f..24491b9b5 100644 --- a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs +++ b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs @@ -9,10 +9,7 @@ use cnidarium::StateWrite; use crate::{ address::StateReadExt as _, - app::{ - ActionHandler, - FeeHandler, - }, + app::ActionHandler, ibc::{ StateReadExt as _, StateWriteExt as _, @@ -61,10 +58,3 @@ impl ActionHandler for IbcRelayerChangeAction { Ok(()) } } - -#[async_trait::async_trait] -impl FeeHandler for IbcRelayerChangeAction { - async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { - Ok(()) - } -} diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index b71204a9f..1fbb69d4e 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -34,10 +34,6 @@ use penumbra_ibc::component::packet::{ Unchecked, }; use penumbra_proto::core::component::ibc::v1::FungibleTokenPacketData; -use tracing::{ - instrument, - Level, -}; use crate::{ accounts::{ @@ -47,12 +43,7 @@ use crate::{ address::StateReadExt as _, app::{ ActionHandler, - FeeHandler, - StateReadExt as _, - }, - assets::{ StateReadExt as _, - StateWriteExt as _, }, bridge::{ StateReadExt as _, @@ -266,42 +257,6 @@ impl ActionHandler for action::Ics20Withdrawal { } } -#[async_trait::async_trait] -impl FeeHandler for action::Ics20Withdrawal { - #[instrument(skip_all, err(level = Level::WARN))] - async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { - let tx_context = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action"); - let from = tx_context.address_bytes(); - let fee = state - .get_ics20_withdrawal_base_fee() - .await - .wrap_err("failed to get ics20 withdrawal base fee")?; - - ensure!( - state - .is_allowed_fee_asset(&self.fee_asset) - .await - .wrap_err("failed to check allowed fee assets in state")?, - "invalid fee asset", - ); - - state - .decrease_balance(&from, &self.fee_asset, fee) - .await - .wrap_err("failed to decrease balance for fee payment")?; - state.add_fee_to_block_fees( - &self.fee_asset, - fee, - tx_context.transaction_id, - tx_context.source_action_index, - )?; - - Ok(()) - } -} - fn is_source(source_port: &PortId, source_channel: &ChannelId, asset: &Denom) -> bool { if let Denom::TracePrefixed(trace) = asset { !trace.has_leading_port(source_port) || !trace.has_leading_channel(source_channel) diff --git a/crates/astria-sequencer/src/lib.rs b/crates/astria-sequencer/src/lib.rs index 8322623b5..6fbb1a904 100644 --- a/crates/astria-sequencer/src/lib.rs +++ b/crates/astria-sequencer/src/lib.rs @@ -10,6 +10,7 @@ mod build_info; pub(crate) mod component; pub mod config; pub(crate) mod fee_asset_change; +pub(crate) mod fees; pub(crate) mod grpc; pub(crate) mod ibc; mod mempool; diff --git a/crates/astria-sequencer/src/sequence/action.rs b/crates/astria-sequencer/src/sequence/action.rs index 5a30d5573..3cc1a61c0 100644 --- a/crates/astria-sequencer/src/sequence/action.rs +++ b/crates/astria-sequencer/src/sequence/action.rs @@ -1,29 +1,11 @@ use astria_core::protocol::transaction::v1alpha1::action::SequenceAction; use astria_eyre::eyre::{ ensure, - OptionExt as _, Result, - WrapErr as _, }; use cnidarium::StateWrite; -use tracing::{ - instrument, - Level, -}; -use crate::{ - accounts::StateWriteExt as _, - app::{ - ActionHandler, - FeeHandler, - }, - assets::{ - StateReadExt, - StateWriteExt, - }, - sequence, - transaction::StateReadExt as _, -}; +use crate::app::ActionHandler; #[async_trait::async_trait] impl ActionHandler for SequenceAction { @@ -41,81 +23,3 @@ impl ActionHandler for SequenceAction { Ok(()) } } - -#[async_trait::async_trait] -impl FeeHandler for SequenceAction { - #[instrument(skip_all, err(level = Level::WARN))] - async fn calculate_and_pay_fees(&self, mut state: S) -> Result<()> { - let tx_context = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action"); - let from = tx_context.address_bytes(); - - ensure!( - state - .is_allowed_fee_asset(&self.fee_asset) - .await - .wrap_err("failed accessing state to check if fee is allowed")?, - "invalid fee asset", - ); - - let fee = calculate_fee_from_state(&self.data, &state) - .await - .wrap_err("calculated fee overflows u128")?; - - state - .add_fee_to_block_fees( - &self.fee_asset, - fee, - tx_context.transaction_id, - tx_context.source_action_index, - ) - .wrap_err("failed to add to block fees")?; - state - .decrease_balance(&from, &self.fee_asset, fee) - .await - .wrap_err("failed updating `from` account balance")?; - Ok(()) - } -} - -/// Calculates the fee for a sequence `Action` based on the length of the `data`. -pub(crate) async fn calculate_fee_from_state( - data: &[u8], - state: &S, -) -> Result { - let base_fee = state - .get_sequence_action_base_fee() - .await - .wrap_err("failed to get base fee")?; - let fee_per_byte = state - .get_sequence_action_byte_cost_multiplier() - .await - .wrap_err("failed to get fee per byte")?; - calculate_fee(data, fee_per_byte, base_fee).ok_or_eyre("calculated fee overflows u128") -} - -/// Calculates the fee for a sequence `Action` based on the length of the `data`. -/// Returns `None` if the fee overflows `u128`. -fn calculate_fee(data: &[u8], fee_per_byte: u128, base_fee: u128) -> Option { - base_fee.checked_add( - fee_per_byte.checked_mul( - data.len() - .try_into() - .expect("a usize should always convert to a u128"), - )?, - ) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn calculate_fee_ok() { - assert_eq!(calculate_fee(&[], 1, 0), Some(0)); - assert_eq!(calculate_fee(&[0], 1, 0), Some(1)); - assert_eq!(calculate_fee(&[0u8; 10], 1, 0), Some(10)); - assert_eq!(calculate_fee(&[0u8; 10], 1, 100), Some(110)); - } -} diff --git a/crates/astria-sequencer/src/sequence/mod.rs b/crates/astria-sequencer/src/sequence/mod.rs index 5a59df2ed..39c7cfff7 100644 --- a/crates/astria-sequencer/src/sequence/mod.rs +++ b/crates/astria-sequencer/src/sequence/mod.rs @@ -3,7 +3,6 @@ pub(crate) mod component; mod state_ext; pub(crate) mod storage; -pub(crate) use action::calculate_fee_from_state; pub(crate) use state_ext::{ StateReadExt, StateWriteExt, diff --git a/crates/astria-sequencer/src/transaction/checks.rs b/crates/astria-sequencer/src/transaction/checks.rs index 07858213e..e14679fe3 100644 --- a/crates/astria-sequencer/src/transaction/checks.rs +++ b/crates/astria-sequencer/src/transaction/checks.rs @@ -246,7 +246,7 @@ async fn sequence_update_fees( fees_by_asset: &mut HashMap, data: &[u8], ) -> Result<()> { - let fee = crate::sequence::calculate_fee_from_state(data, state) + let fee = crate::fees::calculate_sequence_action_fee_from_state(data, state) .await .wrap_err("fee for sequence action overflowed; data too large")?; fees_by_asset @@ -277,7 +277,7 @@ fn bridge_lock_update_fees( use astria_core::sequencerblock::v1alpha1::block::Deposit; let expected_deposit_fee = transfer_fee.saturating_add( - crate::bridge::calculate_base_deposit_fee(&Deposit { + crate::fees::calculate_base_deposit_fee(&Deposit { bridge_address: act.to, // rollup ID doesn't matter here, as this is only used as a size-check rollup_id: RollupId::from_unhashed_bytes([0; 32]), @@ -373,7 +373,7 @@ mod tests { .unwrap(), &crate::test_utils::nria(), transfer_fee - + crate::sequence::calculate_fee_from_state(&data, &state_tx) + + crate::fees::calculate_sequence_action_fee_from_state(&data, &state_tx) .await .unwrap(), ) @@ -451,7 +451,7 @@ mod tests { .unwrap(), &crate::test_utils::nria(), transfer_fee - + crate::sequence::calculate_fee_from_state(&data, &state_tx) + + crate::fees::calculate_sequence_action_fee_from_state(&data, &state_tx) .await .unwrap(), ) diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index 5edecf6e6..7493f1646 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -40,13 +40,13 @@ use crate::{ }, app::{ ActionHandler, - FeeHandler, StateReadExt as _, }, bridge::{ StateReadExt as _, StateWriteExt as _, }, + fees::FeeHandler, ibc::{ host_interface::AstriaHost, StateReadExt as _, @@ -218,28 +218,22 @@ impl ActionHandler for SignedTransaction { state.put_transaction_context(transaction_context); match action { - Action::Transfer(act) => act - .check_execute_and_pay_fees(&mut state) + Action::Transfer(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("executing transfer action failed")?, - Action::Sequence(act) => act - .check_execute_and_pay_fees(&mut state) + Action::Sequence(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("executing sequence action failed")?, - Action::ValidatorUpdate(act) => act - .check_execute_and_pay_fees(&mut state) + Action::ValidatorUpdate(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("executing validor update")?, - Action::SudoAddressChange(act) => act - .check_execute_and_pay_fees(&mut state) + Action::SudoAddressChange(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("executing sudo address change failed")?, - Action::IbcSudoChange(act) => act - .check_and_execute(&mut state) + Action::IbcSudoChange(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("executing ibc sudo change failed")?, - Action::FeeChange(act) => act - .check_execute_and_pay_fees(&mut state) + Action::FeeChange(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("executing fee change failed")?, Action::Ibc(act) => { @@ -263,32 +257,25 @@ impl ActionHandler for SignedTransaction { .map_err(anyhow_to_eyre) .wrap_err("failed executing ibc action")?; } - Action::Ics20Withdrawal(act) => act - .check_execute_and_pay_fees(&mut state) + Action::Ics20Withdrawal(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("failed executing ics20 withdrawal")?, - Action::IbcRelayerChange(act) => act - .check_execute_and_pay_fees(&mut state) + Action::IbcRelayerChange(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("failed executing ibc relayer change")?, - Action::FeeAssetChange(act) => act - .check_execute_and_pay_fees(&mut state) + Action::FeeAssetChange(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("failed executing fee asseet change")?, - Action::InitBridgeAccount(act) => act - .check_execute_and_pay_fees(&mut state) + Action::InitBridgeAccount(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("failed executing init bridge account")?, - Action::BridgeLock(act) => act - .check_execute_and_pay_fees(&mut state) + Action::BridgeLock(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("failed executing bridge lock")?, - Action::BridgeUnlock(act) => act - .check_execute_and_pay_fees(&mut state) + Action::BridgeUnlock(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("failed executing bridge unlock")?, - Action::BridgeSudoChange(act) => act - .check_execute_and_pay_fees(&mut state) + Action::BridgeSudoChange(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("failed executing bridge sudo change")?, } @@ -300,9 +287,11 @@ impl ActionHandler for SignedTransaction { } } -#[async_trait::async_trait] -impl FeeHandler for SignedTransaction { - async fn calculate_and_pay_fees(&self, _state: S) -> Result<()> { - Ok(()) - } +async fn check_execute_and_pay_fees( + action: &T, + mut state: S, +) -> Result<()> { + action.check_and_execute(&mut state).await?; + action.check_and_pay_fees(&mut state).await?; + Ok(()) }