From 79531330196249e128cb7f46b2b3e14a95aff464 Mon Sep 17 00:00:00 2001 From: Ethan Oroshiba Date: Tue, 30 Jul 2024 16:21:01 -0500 Subject: [PATCH] feat(sequencer, core): Add fee reporting (#1305) ## Summary Added event recording for fees on `Transfer` and `Sequence` actions. ## Background Fees were previously not reported, and since Astria doesn't fill out gas wanted/used on `ExecTxResult`, there was no outside way of viewing a block or transaction's associated fees. ## Changes - Added `get_action_type()` methods to `TransferAction` and `SequencerAction` to facilitate passing the kind of action to `get_and_increase_block_fees()`. - Added construction and recording of ABCI events having kind `tx.fees` and attributes `asset`, `feeAmount`, and `actionType`. ## Testing Added unit test to ensure `tx.fees` event is recorded correctly in the transaction state events. ## Related Issues closes #1293 --------- Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com> --- .../src/executor/bundle_factory/mod.rs | 1 + crates/astria-composer/src/test_utils.rs | 1 + crates/astria-core/src/lib.rs | 9 ++- .../protocol/transaction/v1alpha1/action.rs | 64 ++++++------------- .../astria-sequencer/src/accounts/action.rs | 3 +- .../src/app/tests_execute_transaction.rs | 50 +++++++++++++++ .../astria-sequencer/src/sequence/action.rs | 3 +- crates/astria-sequencer/src/state_ext.rs | 44 ++++++++++--- 8 files changed, 119 insertions(+), 56 deletions(-) diff --git a/crates/astria-composer/src/executor/bundle_factory/mod.rs b/crates/astria-composer/src/executor/bundle_factory/mod.rs index d3c3696d82..58a3e5a9b2 100644 --- a/crates/astria-composer/src/executor/bundle_factory/mod.rs +++ b/crates/astria-composer/src/executor/bundle_factory/mod.rs @@ -14,6 +14,7 @@ use astria_core::{ action::SequenceAction, Action, }, + Protobuf as _, }; use serde::ser::{ Serialize, diff --git a/crates/astria-composer/src/test_utils.rs b/crates/astria-composer/src/test_utils.rs index a12f924fe2..830e61b348 100644 --- a/crates/astria-composer/src/test_utils.rs +++ b/crates/astria-composer/src/test_utils.rs @@ -5,6 +5,7 @@ use astria_core::{ ROLLUP_ID_LEN, }, protocol::transaction::v1alpha1::action::SequenceAction, + Protobuf as _, }; fn encoded_len(action: &SequenceAction) -> usize { diff --git a/crates/astria-core/src/lib.rs b/crates/astria-core/src/lib.rs index a229dbfeaa..29c87603ef 100644 --- a/crates/astria-core/src/lib.rs +++ b/crates/astria-core/src/lib.rs @@ -1,3 +1,5 @@ +use prost::Name; + #[cfg(not(target_pointer_width = "64"))] compile_error!( "library is only guaranteed to run on 64 bit machines due to casts from/to u64 and usize" @@ -27,7 +29,7 @@ pub trait Protobuf: Sized { /// Errors that can occur when transforming from a raw type. type Error; /// The raw deserialized protobuf type. - type Raw; + type Raw: prost::Name; /// Convert from a reference to the raw protobuf type. /// @@ -56,4 +58,9 @@ pub trait Protobuf: Sized { fn into_raw(self) -> Self::Raw { Self::to_raw(&self) } + + #[must_use] + fn full_name() -> String { + Self::Raw::full_name() + } } diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index f4ef672d28..81256f14e3 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -391,23 +391,12 @@ pub struct SequenceAction { pub fee_asset: asset::Denom, } -impl SequenceAction { - #[must_use] - pub fn into_raw(self) -> raw::SequenceAction { - let Self { - rollup_id, - data, - fee_asset, - } = self; - raw::SequenceAction { - rollup_id: Some(rollup_id.to_raw()), - data, - fee_asset: fee_asset.to_string(), - } - } +impl Protobuf for SequenceAction { + type Error = SequenceActionError; + type Raw = raw::SequenceAction; #[must_use] - pub fn to_raw(&self) -> raw::SequenceAction { + fn to_raw(&self) -> raw::SequenceAction { let Self { rollup_id, data, @@ -420,22 +409,23 @@ impl SequenceAction { } } - /// Convert from a raw, unchecked protobuf [`raw::SequenceAction`]. + /// Convert from a reference to the raw protobuf type. /// /// # Errors - /// Returns an error if the `proto.rollup_id` field was not 32 bytes. - pub fn try_from_raw(proto: raw::SequenceAction) -> Result { + /// Returns `SequenceActionError` if the `proto.rollup_id` field was not 32 bytes. + fn try_from_raw_ref(raw: &Self::Raw) -> Result { let raw::SequenceAction { rollup_id, data, fee_asset, - } = proto; + } = raw; let Some(rollup_id) = rollup_id else { return Err(SequenceActionError::field_not_set("rollup_id")); }; let rollup_id = - RollupId::try_from_raw(&rollup_id).map_err(SequenceActionError::rollup_id_length)?; + RollupId::try_from_raw(rollup_id).map_err(SequenceActionError::rollup_id_length)?; let fee_asset = fee_asset.parse().map_err(SequenceActionError::fee_asset)?; + let data = data.clone(); Ok(Self { rollup_id, data, @@ -449,31 +439,18 @@ impl SequenceAction { pub struct TransferAction { pub to: Address, pub amount: u128, - // asset to be transferred. + /// asset to be transferred. pub asset: asset::Denom, /// asset to use for fee payment. pub fee_asset: asset::Denom, } -impl TransferAction { - #[must_use] - pub fn into_raw(self) -> raw::TransferAction { - let Self { - to, - amount, - asset, - fee_asset, - } = self; - raw::TransferAction { - to: Some(to.to_raw()), - amount: Some(amount.into()), - asset: asset.to_string(), - fee_asset: fee_asset.to_string(), - } - } +impl Protobuf for TransferAction { + type Error = TransferActionError; + type Raw = raw::TransferAction; #[must_use] - pub fn to_raw(&self) -> raw::TransferAction { + fn to_raw(&self) -> raw::TransferAction { let Self { to, amount, @@ -488,23 +465,22 @@ impl TransferAction { } } - /// Convert from a raw, unchecked protobuf [`raw::TransferAction`]. + /// Convert from a reference to the raw protobuf type. /// /// # Errors - /// - /// Returns an error if the raw action's `to` address did not have the expected + /// Returns `TransferActionError` if the raw action's `to` address did not have the expected /// length. - pub fn try_from_raw(proto: raw::TransferAction) -> Result { + fn try_from_raw_ref(raw: &Self::Raw) -> Result { let raw::TransferAction { to, amount, asset, fee_asset, - } = proto; + } = raw; let Some(to) = to else { return Err(TransferActionError::field_not_set("to")); }; - let to = Address::try_from_raw(&to).map_err(TransferActionError::address)?; + let to = Address::try_from_raw(to).map_err(TransferActionError::address)?; let amount = amount.map_or(0, Into::into); let asset = asset.parse().map_err(TransferActionError::asset)?; let fee_asset = fee_asset.parse().map_err(TransferActionError::fee_asset)?; diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index a36e9f2c59..4948df966b 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -6,6 +6,7 @@ use anyhow::{ use astria_core::{ primitive::v1::Address, protocol::transaction::v1alpha1::action::TransferAction, + Protobuf, }; use tracing::instrument; @@ -112,7 +113,7 @@ impl ActionHandler for TransferAction { .await .context("failed to get transfer base fee")?; state - .get_and_increase_block_fees(&self.fee_asset, fee) + .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) .await .context("failed to add to block fees")?; diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 48907159b4..fc65c2ce6e 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -29,6 +29,7 @@ use astria_core::{ }; use cnidarium::StateDelta; use penumbra_ibc::params::IBCParameters; +use tendermint::abci::EventAttributeIndexExt as _; use crate::{ accounts::state_ext::StateReadExt as _, @@ -1042,3 +1043,52 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { "bridge should've transferred out whole balance" ); } + +#[tokio::test] +async fn transaction_execution_records_fee_event() { + let mut app = initialize_app(None, vec![]).await; + + // transfer funds from Alice to Bob + let (alice_signing_key, _) = get_alice_signing_key_and_address(); + let bob_address = address_from_hex_string(BOB_ADDRESS); + let value = 333_333; + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions: vec![ + TransferAction { + to: bob_address, + amount: value, + asset: get_native_asset().clone(), + fee_asset: get_native_asset().clone(), + } + .into(), + ], + }; + + let signed_tx = Arc::new(tx.into_signed(&alice_signing_key)); + + let events = app.execute_transaction(signed_tx).await.unwrap(); + 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", get_native_asset().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() + ); +} diff --git a/crates/astria-sequencer/src/sequence/action.rs b/crates/astria-sequencer/src/sequence/action.rs index 3b4353445d..602f90a16b 100644 --- a/crates/astria-sequencer/src/sequence/action.rs +++ b/crates/astria-sequencer/src/sequence/action.rs @@ -6,6 +6,7 @@ use anyhow::{ use astria_core::{ primitive::v1::Address, protocol::transaction::v1alpha1::action::SequenceAction, + Protobuf, }; use tracing::instrument; @@ -61,7 +62,7 @@ impl ActionHandler for SequenceAction { .await .context("failed to calculate fee")?; state - .get_and_increase_block_fees(&self.fee_asset, fee) + .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) .await .context("failed to add to block fees")?; diff --git a/crates/astria-sequencer/src/state_ext.rs b/crates/astria-sequencer/src/state_ext.rs index ec6daf5801..73758a8348 100644 --- a/crates/astria-sequencer/src/state_ext.rs +++ b/crates/astria-sequencer/src/state_ext.rs @@ -10,7 +10,13 @@ use cnidarium::{ StateWrite, }; use futures::StreamExt as _; -use tendermint::Time; +use tendermint::{ + abci::{ + Event, + EventAttributeIndexExt as _, + }, + Time, +}; use tracing::instrument; const NATIVE_ASSET_KEY: &[u8] = b"nativeasset"; @@ -36,6 +42,21 @@ fn fee_asset_key>(asset: TAsset) -> String { ) } +/// Creates `abci::Event` of kind `tx.fees` for sequencer fee reporting +fn construct_tx_fee_event(asset: &TAsset, fee_amount: u128, action_type: String) -> Event +where + TAsset: Into + std::fmt::Display + Send, +{ + 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)] @@ -243,12 +264,13 @@ pub(crate) trait StateWriteExt: StateWrite { async fn get_and_increase_block_fees( &mut self, asset: TAsset, - amount: u128, + fee_amount: u128, + action_type: String, ) -> Result<()> where - TAsset: Into + std::fmt::Display + Send, + TAsset: Into + std::fmt::Display + Send + Clone, { - let block_fees_key = block_fees_key(asset); + let block_fees_key = block_fees_key(asset.clone()); let current_amount = self .nonverifiable_get_raw(block_fees_key.as_bytes()) .await @@ -264,10 +286,14 @@ pub(crate) trait StateWriteExt: StateWrite { .unwrap_or_default(); let new_amount = current_amount - .checked_add(amount) + .checked_add(fee_amount) .context("block fees overflowed u128")?; - self.nonverifiable_put_raw(block_fees_key.into(), new_amount.to_be_bytes().to_vec()); + + // record the fee event to the state cache + let tx_fee_event = construct_tx_fee_event(&asset, fee_amount, action_type); + self.record(tx_fee_event); + Ok(()) } @@ -624,7 +650,7 @@ mod test { let asset = asset_0(); let amount = 100u128; state - .get_and_increase_block_fees(&asset, amount) + .get_and_increase_block_fees(&asset, amount, "test".into()) .await .unwrap(); @@ -650,11 +676,11 @@ mod test { let amount_second = 200u128; state - .get_and_increase_block_fees(&asset_first, amount_first) + .get_and_increase_block_fees(&asset_first, amount_first, "test".into()) .await .unwrap(); state - .get_and_increase_block_fees(&asset_second, amount_second) + .get_and_increase_block_fees(&asset_second, amount_second, "test".into()) .await .unwrap(); // holds expected