fix(sequencer): move fee event recording to tx level (#1718)

## Summary
Moves ABCI fee events to `FinalizeBlock.tx_results` to identify fees on
a per transaction basis.

## Background
Post-fee rework, fee ABCI event construction was happening outside of
transaction execution and (wrongly) included in the
`FinalizeBlock.events` field of the finalize block response. This made
it difficult to identify which fee events were related to which
transaction.

This patch moves fee event construction back into the (atomic)
transaction execution, so that the generated ABCI fee events per
transaction are again part of `FinalizeBlock.tx_results`.

For example for Astrotek fees will again show up next to each
transaction.

## Changes
- Moved fee event construction back into transaction execution.

## Testing
Adjusted fee event unit test to ensure fee event is returned as part of
transaction execution.

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

@@ -109,7 +109,6 @@ use crate::{
     component::Component as _,
     fees::{
         component::FeesComponent,
-        construct_tx_fee_event,
         StateReadExt as _,
     },
     grpc::StateWriteExt as _,
@@ -1227,8 +1226,6 @@ impl App {
                 .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);
         }
 
         let events = self.apply(state_tx);

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

@@ -1266,17 +1266,12 @@ async fn transaction_execution_records_fee_event() {
         .try_build()
         .unwrap();
     let signed_tx = Arc::new(tx.sign(&alice));
-    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 event = events.first().unwrap();
     assert_eq!(event.kind, "tx.fees");
     assert_eq!(event.attributes[0].key, "actionName");
     assert_eq!(event.attributes[1].key, "asset");
     assert_eq!(event.attributes[2].key, "feeAmount");
-    assert_eq!(event.attributes[3].key, "sourceTransactionId");
-    assert_eq!(event.attributes[4].key, "sourceActionIndex");
+    assert_eq!(event.attributes[3].key, "positionInTransaction");
 }

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

@@ -1,8 +1,5 @@
 use astria_core::{
-    primitive::v1::{
-        asset,
-        TransactionId,
-    },
+    primitive::v1::asset,
     protocol::transaction::{
         self,
         v1::action::{
@@ -30,10 +27,6 @@ use astria_eyre::eyre::{
 };
 use cnidarium::StateWrite;
 use penumbra_ibc::IbcRelay;
-use tendermint::abci::{
-    Event,
-    EventAttributeIndexExt as _,
-};
 use tracing::{
     instrument,
     Level,
@@ -74,7 +67,6 @@ pub(crate) struct Fee {
     action_name: String,
     asset: asset::Denom,
     amount: u128,
-    source_transaction_id: TransactionId,
     source_action_index: u64,
 }
 
@@ -347,7 +339,6 @@ async fn check_and_pay_fees<S: StateWrite, T: FeeHandler + Protobuf>(
         .get_transaction_context()
         .expect("transaction source must be present in state when executing an action");
     let from = transaction_context.address_bytes();
-    let transaction_id = transaction_context.transaction_id;
     let source_action_index = transaction_context.source_action_index;
 
     ensure!(
@@ -358,7 +349,7 @@ async fn check_and_pay_fees<S: StateWrite, T: FeeHandler + Protobuf>(
         "invalid fee asset",
     );
     state
-        .add_fee_to_block_fees::<_, T>(fee_asset, total_fees, transaction_id, source_action_index)
+        .add_fee_to_block_fees::<_, T>(fee_asset, total_fees, source_action_index)
         .wrap_err("failed to add to block fees")?;
     state
         .decrease_balance(&from, fee_asset, total_fees)
@@ -379,17 +370,3 @@ fn base_deposit_fee(asset: &asset::Denom, destination_chain_address: &str) -> u1
     .expect("converting a usize to a u128 should work on any currently existing machine")
     .saturating_add(DEPOSIT_BASE_FEE)
 }
-
-/// 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",
-        [
-            ("actionName", fee.action_name.to_string()).index(),
-            ("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/fees/state_ext.rs

@@ -9,10 +9,7 @@ use std::{
 };
 
 use astria_core::{
-    primitive::v1::{
-        asset,
-        TransactionId,
-    },
+    primitive::v1::asset,
     protocol::fees::v1::{
         BridgeLockFeeComponents,
         BridgeSudoChangeFeeComponents,
@@ -45,6 +42,10 @@ use cnidarium::{
 };
 use futures::Stream;
 use pin_project_lite::pin_project;
+use tendermint::abci::{
+    Event,
+    EventAttributeIndexExt as _,
+};
 use tracing::instrument;
 
 use super::{
@@ -384,7 +385,6 @@ pub(crate) trait StateWriteExt: StateWrite {
         &mut self,
         asset: &'a TAsset,
         amount: u128,
-        source_transaction_id: TransactionId,
         source_action_index: u64,
     ) -> Result<()>
     where
@@ -397,9 +397,13 @@ pub(crate) trait StateWriteExt: StateWrite {
             action_name: T::full_name(),
             asset: asset::IbcPrefixed::from(asset).into(),
             amount,
-            source_transaction_id,
             source_action_index,
         };
+
+        // Fee ABCI event recorded for reporting
+        let fee_event = construct_tx_fee_event(&fee);
+        self.record(fee_event);
+
         let new_fees = if let Some(mut fees) = current_fees {
             fees.push(fee);
             fees
@@ -565,6 +569,19 @@ pub(crate) trait StateWriteExt: StateWrite {
 
 impl<T: StateWrite> StateWriteExt for T {}
 
+/// Creates `abci::Event` of kind `tx.fees` for sequencer fee reporting
+fn construct_tx_fee_event(fee: &Fee) -> Event {
+    Event::new(
+        "tx.fees",
+        [
+            ("actionName", fee.action_name.to_string()).index(),
+            ("asset", fee.asset.to_string()).index(),
+            ("feeAmount", fee.amount.to_string()).index(),
+            ("positionInTransaction", fee.source_action_index.to_string()).index(),
+        ],
+    )
+}
+
 #[cfg(test)]
 mod tests {
     use std::collections::HashSet;
@@ -606,7 +623,7 @@ mod tests {
         let asset = asset_0();
         let amount = 100u128;
         state
-            .add_fee_to_block_fees::<_, Transfer>(&asset, amount, TransactionId::new([0; 32]), 0)
+            .add_fee_to_block_fees::<_, Transfer>(&asset, amount, 0)
             .unwrap();
 
         // holds expected
@@ -617,7 +634,6 @@ mod tests {
                 action_name: "astria.protocol.transaction.v1.Transfer".to_string(),
                 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"
@@ -637,20 +653,10 @@ mod tests {
         let amount_second = 200u128;
 
         state
-            .add_fee_to_block_fees::<_, Transfer>(
-                &asset_first,
-                amount_first,
-                TransactionId::new([0; 32]),
-                0,
-            )
+            .add_fee_to_block_fees::<_, Transfer>(&asset_first, amount_first, 0)
             .unwrap();
         state
-            .add_fee_to_block_fees::<_, Transfer>(
-                &asset_second,
-                amount_second,
-                TransactionId::new([0; 32]),
-                1,
-            )
+            .add_fee_to_block_fees::<_, Transfer>(&asset_second, amount_second, 1)
             .unwrap();
         // holds expected
         let fee_balances = HashSet::<_>::from_iter(state.get_block_fees());
@@ -661,14 +667,12 @@ mod tests {
                     action_name: "astria.protocol.transaction.v1.Transfer".to_string(),
                     asset: asset_first.to_ibc_prefixed().into(),
                     amount: amount_first,
-                    source_transaction_id: TransactionId::new([0; 32]),
                     source_action_index: 0
                 },
                 Fee {
                     action_name: "astria.protocol.transaction.v1.Transfer".to_string(),
                     asset: asset_second.to_ibc_prefixed().into(),
                     amount: amount_second,
-                    source_transaction_id: TransactionId::new([0; 32]),
                     source_action_index: 1
                 },
             ]),