feat(sequencer): rework fee handling

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,42 @@ 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<S: StateWrite>(&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<S, TAddress>(
     action: &TransferAction,
     from: &TAddress,
@@ -66,50 +106,16 @@ where
     S: StateWrite,
     TAddress: AddressBytes,
 {
-    let fee = state
-        .get_transfer_base_fee()
+    let from = from.address_bytes();
+    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(())
 }
 

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 currently not 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<S: StateWrite>(
+        &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<S: StateWrite>(&self, mut state: S)
     -> astria_eyre::eyre::Result<()>;
 }

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<S: StateWrite>(
+        &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,
+}

crates/astria-sequencer/src/app/mod.rs

@@ -14,6 +14,7 @@ mod tests_breaking_changes;
 #[cfg(test)]
 mod tests_execute_transaction;
 
+mod fee_handler;
 use std::{
     collections::VecDeque,
     sync::Arc,
@@ -49,8 +50,13 @@ use cnidarium::{
     StagedWriteBatch,
     StateDelta,
     StateRead,
+    StateWrite,
     Storage,
 };
+pub(crate) use fee_handler::{
+    Fee,
+    FeeHandler,
+};
 use prost::Message as _;
 use sha2::{
     Digest as _,
@@ -63,6 +69,7 @@ use tendermint::{
         types::ExecTxResult,
         Code,
         Event,
+        EventAttributeIndexExt as _,
     },
     account,
     block::Header,
@@ -1089,18 +1096,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, 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 {
@@ -1189,3 +1197,16 @@ fn signed_transaction_from_bytes(bytes: &[u8]) -> Result<SignedTransaction> {
 
     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(),
+        ],
+    )
+}

crates/astria-sequencer/src/app/tests_app/mod.rs

@@ -277,7 +277,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]

crates/astria-sequencer/src/app/tests_block_fees.rs

@@ -29,6 +29,7 @@ use crate::{
         BOB_ADDRESS,
     },
     assets::StateReadExt as _,
+    authority::StateReadExt as _,
     bridge::{
         calculate_base_deposit_fee,
         StateWriteExt as _,
@@ -66,27 +67,33 @@ async fn transaction_execution_records_fee_event() {
         .try_build()
         .unwrap();
     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");
     assert_eq!(
         event.attributes[0],
-        ("asset", nria().to_string()).index().into()
+        ("asset", nria().to_ibc_prefixed().to_string())
+            .index()
+            .into()
     );
     assert_eq!(
         event.attributes[1],
         ("feeAmount", transfer_fee.to_string()).index().into()
     );
     assert_eq!(
         event.attributes[2],
-        (
-            "actionType",
-            "astria.protocol.transactions.v1alpha1.TransferAction"
-        )
-            .index()
-            .into()
+        ("sourceTransactionId", tx_id.to_string(),).index().into()
+    );
+    assert_eq!(
+        event.attributes[3],
+        ("sourceActionIndex", "0",).index().into()
     );
 }
 
@@ -121,10 +128,9 @@ 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| fee.amount)
         .sum();
     assert_eq!(total_block_fees, transfer_base_fee);
 }
@@ -162,10 +168,9 @@ 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| fee.amount)
         .sum();
     let expected_fees = calculate_fee_from_state(&data, &app.state).await.unwrap();
     assert_eq!(total_block_fees, expected_fees);
@@ -205,10 +210,9 @@ 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| fee.amount)
         .sum();
     assert_eq!(total_block_fees, init_bridge_account_base_fee);
 }
@@ -271,10 +275,9 @@ 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| fee.amount)
         .sum();
     let expected_fees = transfer_base_fee
         + (calculate_base_deposit_fee(&test_deposit).unwrap() * bridge_lock_byte_cost_multiplier);
@@ -325,10 +328,9 @@ 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| fee.amount)
         .sum();
     assert_eq!(total_block_fees, sudo_change_base_fee);
 }

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