diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index 32b2a482b..50e751303 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -18,10 +18,7 @@ use crate::{ }, address::StateReadExt as _, app::ActionHandler, - assets::{ - StateReadExt as _, - StateWriteExt as _, - }, + assets::StateReadExt as _, bridge::StateReadExt as _, transaction::StateReadExt as _, }; @@ -63,50 +60,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) + .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/mod.rs b/crates/astria-sequencer/src/app/mod.rs index 688c6c1ec..5a54f2d95 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -89,10 +89,7 @@ use crate::{ StateWriteExt as _, }, address::StateWriteExt as _, - assets::{ - StateReadExt as _, - StateWriteExt as _, - }, + assets::StateWriteExt as _, authority::{ component::{ AuthorityComponent, @@ -107,6 +104,10 @@ use crate::{ StateWriteExt as _, }, component::Component as _, + fees::{ + construct_tx_fee_event, + StateReadExt as _, + }, grpc::StateWriteExt as _, ibc::component::IbcComponent, mempool::{ @@ -1169,19 +1170,17 @@ 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; - let events = self.apply(state_tx); Ok(abci::response::EndBlock { validator_updates: validator_updates diff --git a/crates/astria-sequencer/src/app/tests_app/mod.rs b/crates/astria-sequencer/src/app/tests_app/mod.rs index a7bba1485..73165395b 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, @@ -277,7 +278,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 b4eaf505d..58abb2426 100644 --- a/crates/astria-sequencer/src/app/tests_block_fees.rs +++ b/crates/astria-sequencer/src/app/tests_block_fees.rs @@ -28,15 +28,14 @@ use crate::{ initialize_app, BOB_ADDRESS, }, - assets::StateReadExt as _, - bridge::{ + authority::StateReadExt as _, + 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, @@ -66,14 +65,21 @@ 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 events = 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 = 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], @@ -81,12 +87,11 @@ async fn transaction_execution_records_fee_event() { ); assert_eq!( event.attributes[2], - ( - "actionType", - "astria.protocol.transactions.v1alpha1.Transfer" - ) - .index() - .into() + ("sourceTransactionId", tx_id.to_string(),).index().into() + ); + assert_eq!( + event.attributes[3], + ("sourceActionIndex", "0",).index().into() ); } @@ -121,10 +126,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,12 +166,13 @@ 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(); + let expected_fees = calculate_sequence_action_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); } diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 2c64d35fe..b4de2c950 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/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 new file mode 100644 index 000000000..09bc83543 --- /dev/null +++ b/crates/astria-sequencer/src/assets/snapshots/astria_sequencer__assets__state_ext__tests__storage_keys_are_unchanged-2.snap @@ -0,0 +1,6 @@ +--- +source: crates/astria-sequencer/src/assets/state_ext.rs +expression: fee_asset_key(trace_prefixed) +--- +fee_asset/ce072174ebc356c6ead681d61ab417ee72fdedd8155eb76478ece374bb04dc1d + diff --git a/crates/astria-sequencer/src/assets/state_ext.rs b/crates/astria-sequencer/src/assets/state_ext.rs index 0597bc580..0e736a937 100644 --- a/crates/astria-sequencer/src/assets/state_ext.rs +++ b/crates/astria-sequencer/src/assets/state_ext.rs @@ -1,17 +1,10 @@ -use std::{ - borrow::Cow, - fmt::Display, -}; +use std::borrow::Cow; -use astria_core::{ - primitive::v1::asset, - Protobuf, -}; +use astria_core::primitive::v1::asset; use astria_eyre::{ anyhow_to_eyre, eyre::{ bail, - OptionExt as _, Result, WrapErr as _, }, @@ -22,34 +15,17 @@ use cnidarium::{ StateWrite, }; use futures::StreamExt as _; -use tendermint::abci::{ - Event, - EventAttributeIndexExt as _, -}; use tracing::instrument; use super::storage::{ self, keys::{ self, - extract_asset_from_block_fees_key, extract_asset_from_fee_asset_key, }, }; use crate::storage::StoredValue; -/// Creates `abci::Event` of kind `tx.fees` for sequencer fee reporting -fn construct_tx_fee_event(asset: &T, fee_amount: u128) -> Event { - Event::new( - "tx.fees", - [ - ("asset", asset.to_string()).index(), - ("feeAmount", fee_amount.to_string()).index(), - ("actionType", P::full_name()).index(), - ], - ) -} - #[async_trait] pub(crate) trait StateReadExt: StateRead { #[instrument(skip_all)] @@ -104,26 +80,6 @@ pub(crate) trait StateReadExt: StateRead { .wrap_err("invalid ibc asset bytes") } - #[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(keys::BLOCK_FEES_PREFIX.as_bytes())); - while let Some(Ok((key, bytes))) = stream.next().await { - let asset = - extract_asset_from_block_fees_key(&key).wrap_err("failed to extract asset")?; - - let fee = StoredValue::deserialize(&bytes) - .and_then(|value| storage::Fee::try_from(value).map(u128::from)) - .context("invalid block fees bytes")?; - - fees.push((asset, fee)); - } - - Ok(fees) - } - #[instrument(skip_all)] async fn is_allowed_fee_asset<'a, TAsset>(&self, asset: &'a TAsset) -> Result where @@ -177,56 +133,6 @@ pub(crate) trait StateWriteExt: StateWrite { Ok(()) } - /// Adds `amount` to the block fees for `asset`. - #[instrument(skip_all)] - async fn get_and_increase_block_fees<'a, P, TAsset>( - &mut self, - asset: &'a TAsset, - amount: u128, - ) -> Result<()> - where - TAsset: Sync + Display, - &'a TAsset: Into>, - P: Protobuf, - { - let tx_fee_event = construct_tx_fee_event::(asset, amount); - let block_fees_key = keys::block_fees(asset); - - 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| { - StoredValue::deserialize(&bytes) - .and_then(|value| storage::Fee::try_from(value).map(u128::from)) - .context("invalid block fees bytes") - }) - .transpose()? - .unwrap_or_default(); - - let new_amount = current_amount - .checked_add(amount) - .ok_or_eyre("block fees overflowed u128")?; - let bytes = StoredValue::from(storage::Fee::from(new_amount)) - .serialize() - .wrap_err("failed to serialize block fees")?; - self.nonverifiable_put_raw(block_fees_key.into_bytes(), bytes); - - self.record(tx_fee_event); - - Ok(()) - } - - #[instrument(skip_all)] - async fn clear_block_fees(&mut self) { - let mut stream = - std::pin::pin!(self.nonverifiable_prefix_raw(keys::BLOCK_FEES_PREFIX.as_bytes())); - while let Some(Ok((key, _))) = stream.next().await { - self.nonverifiable_delete(key); - } - } - #[instrument(skip_all)] fn delete_allowed_fee_asset<'a, TAsset>(&mut self, asset: &'a TAsset) where @@ -254,7 +160,6 @@ impl StateWriteExt for T {} mod tests { use std::collections::HashSet; - use astria_core::protocol::transaction::v1alpha1::action::Transfer; use cnidarium::StateDelta; use super::*; @@ -308,74 +213,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().await.unwrap(); - assert!(fee_balances_orig.is_empty()); - - // can write - let asset = asset_0(); - let amount = 100u128; - state - .get_and_increase_block_fees::(&asset, amount) - .await - .unwrap(); - - // holds expected - let fee_balances_updated = state.get_block_fees().await.unwrap(); - assert_eq!( - fee_balances_updated[0], - (asset.to_ibc_prefixed(), amount), - "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 - .get_and_increase_block_fees::(&asset_first, amount_first) - .await - .unwrap(); - state - .get_and_increase_block_fees::(&asset_second, amount_second) - .await - .unwrap(); - // holds expected - let fee_balances = HashSet::<_>::from_iter(state.get_block_fees().await.unwrap()); - assert_eq!( - fee_balances, - HashSet::from_iter(vec![ - (asset_first.to_ibc_prefixed(), amount_first), - (asset_second.to_ibc_prefixed(), amount_second) - ]), - "returned fee balance vector not what was expected" - ); - - // can delete - state.clear_block_fees().await; - - let fee_balances_updated = state.get_block_fees().await.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/assets/storage/keys.rs b/crates/astria-sequencer/src/assets/storage/keys.rs index 028678f0c..e2da9f74d 100644 --- a/crates/astria-sequencer/src/assets/storage/keys.rs +++ b/crates/astria-sequencer/src/assets/storage/keys.rs @@ -10,7 +10,6 @@ use astria_eyre::eyre::{ use crate::storage::keys::Asset; pub(in crate::assets) const NATIVE_ASSET: &str = "assets/native_asset"; -pub(in crate::assets) const BLOCK_FEES_PREFIX: &str = "assets/block_fees/"; pub(in crate::assets) const FEE_ASSET_PREFIX: &str = "assets/fee_asset/"; /// Example: `assets/ibc/0101....0101`. @@ -29,23 +28,11 @@ where format!("{FEE_ASSET_PREFIX}{}", Asset::from(asset)) } -pub(in crate::assets) fn block_fees<'a, TAsset>(asset: &'a TAsset) -> String -where - &'a TAsset: Into>, -{ - format!("{BLOCK_FEES_PREFIX}{}", Asset::from(asset)) -} - pub(in crate::assets) fn extract_asset_from_fee_asset_key(key: &[u8]) -> Result { extract_asset_from_key(key, FEE_ASSET_PREFIX) .wrap_err("failed to extract asset from fee asset key") } -pub(in crate::assets) fn extract_asset_from_block_fees_key(key: &[u8]) -> Result { - extract_asset_from_key(key, BLOCK_FEES_PREFIX) - .wrap_err("failed to extract asset from fee asset key") -} - fn extract_asset_from_key(key: &[u8], prefix: &str) -> Result { let key_str = std::str::from_utf8(key) .wrap_err_with(|| format!("key `{}` not valid utf8", telemetry::display::hex(key),))?; @@ -74,7 +61,6 @@ mod tests { insta::assert_snapshot!(NATIVE_ASSET); insta::assert_snapshot!(asset(&test_asset())); insta::assert_snapshot!(fee_asset(&test_asset())); - insta::assert_snapshot!(block_fees(&test_asset())); } #[test] @@ -82,13 +68,11 @@ mod tests { assert!(NATIVE_ASSET.starts_with(COMPONENT_PREFIX)); assert!(asset(&test_asset()).starts_with(COMPONENT_PREFIX)); assert!(fee_asset(&test_asset()).starts_with(COMPONENT_PREFIX)); - assert!(block_fees(&test_asset()).starts_with(COMPONENT_PREFIX)); } #[test] fn prefixes_should_be_prefixes_of_relevant_keys() { assert!(fee_asset(&test_asset()).starts_with(FEE_ASSET_PREFIX)); - assert!(block_fees(&test_asset()).starts_with(BLOCK_FEES_PREFIX)); } #[test] @@ -98,9 +82,5 @@ mod tests { let key = fee_asset(&asset); let recovered_asset = extract_asset_from_fee_asset_key(key.as_bytes()).unwrap(); assert_eq!(asset, recovered_asset); - - let key = block_fees(&asset); - let recovered_asset = extract_asset_from_block_fees_key(key.as_bytes()).unwrap(); - assert_eq!(asset, recovered_asset); } } diff --git a/crates/astria-sequencer/src/assets/storage/mod.rs b/crates/astria-sequencer/src/assets/storage/mod.rs index 8551a44c9..15f61bebf 100644 --- a/crates/astria-sequencer/src/assets/storage/mod.rs +++ b/crates/astria-sequencer/src/assets/storage/mod.rs @@ -1,8 +1,5 @@ pub(super) mod keys; mod values; +pub(super) use values::TracePrefixedDenom; pub(crate) use values::Value; -pub(super) use values::{ - Fee, - TracePrefixedDenom, -}; diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 4adffe917..28f412159 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -14,29 +14,19 @@ use astria_eyre::eyre::{ use cnidarium::StateWrite; use crate::{ - accounts::{ - action::{ - check_transfer, - execute_transfer, - }, - StateReadExt as _, - StateWriteExt as _, + accounts::action::{ + check_transfer, + execute_transfer, }, address::StateReadExt as _, app::ActionHandler, - assets::StateWriteExt as _, 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 BridgeLock { async fn check_stateless(&self) -> Result<()> { @@ -68,15 +58,6 @@ impl ActionHandler for BridgeLock { "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 source_transaction_id = state .get_transaction_context() .expect("current source should be set before executing action") @@ -95,16 +76,6 @@ impl ActionHandler for BridgeLock { 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); - ensure!(from_balance >= fee, "insufficient funds for fee payment"); let transfer_action = Transfer { to: self.to, @@ -114,231 +85,9 @@ impl ActionHandler for BridgeLock { }; 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?; - // 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 deduction logic for these actions are defined fully independently - // (even at the cost of duplicating code). - 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)); - state - .get_and_increase_block_fees::(&self.fee_asset, fee) - .await - .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.cache_deposit_event(deposit); 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 = BridgeLock { - 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 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 ce2047d37..244774dd9 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -8,13 +8,8 @@ use astria_eyre::eyre::{ use cnidarium::StateWrite; use crate::{ - accounts::StateWriteExt as _, address::StateReadExt as _, app::ActionHandler, - assets::{ - StateReadExt as _, - StateWriteExt as _, - }, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -49,14 +44,6 @@ impl ActionHandler for BridgeSudoChange { .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) @@ -73,19 +60,6 @@ impl ActionHandler for BridgeSudoChange { "unauthorized for bridge sudo change action", ); - let fee = state - .get_bridge_sudo_change_base_fee() - .await - .wrap_err("failed to get bridge sudo change fee")?; - state - .get_and_increase_block_fees::(&self.fee_asset, fee) - .await - .wrap_err("failed to add to block fees")?; - state - .decrease_balance(&self.bridge_address, &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) @@ -112,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/init_bridge_account_action.rs b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs index 5422e89fa..adb7d30bb 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -4,23 +4,14 @@ use astria_core::{ }; use astria_eyre::eyre::{ bail, - ensure, Result, WrapErr as _, }; use cnidarium::StateWrite; use crate::{ - accounts::{ - StateReadExt as _, - StateWriteExt as _, - }, address::StateReadExt as _, app::ActionHandler, - assets::{ - StateReadExt as _, - StateWriteExt as _, - }, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -52,16 +43,6 @@ impl ActionHandler for InitBridgeAccount { .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. // @@ -82,39 +63,21 @@ impl ActionHandler for InitBridgeAccount { 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) .wrap_err("failed to put bridge account rollup id")?; state .put_bridge_account_ibc_asset(&from, &self.asset) .wrap_err("failed to put asset ID")?; - state - .put_bridge_account_sudo_address(&from, self.sudo_address.map_or(from, Address::bytes)) - .wrap_err("failed to put bridge account sudo address")?; - state - .put_bridge_account_withdrawer_address( - &from, - self.withdrawer_address.map_or(from, Address::bytes), - ) - .wrap_err("failed to put bridge account withdrawer address")?; - state - .get_and_increase_block_fees::(&self.fee_asset, fee) - .await - .wrap_err("failed to get and increase block fees")?; - state - .decrease_balance(&from, &self.fee_asset, fee) - .await - .wrap_err("failed to deduct fee from account balance")?; + state.put_bridge_account_sudo_address( + &from, + self.sudo_address.map_or(from, Address::bytes), + )?; + state.put_bridge_account_withdrawer_address( + &from, + self.withdrawer_address.map_or(from, Address::bytes), + )?; + 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/fees/mod.rs b/crates/astria-sequencer/src/fees/mod.rs new file mode 100644 index 000000000..8f964b433 --- /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, + BridgeLock, + BridgeSudoChange, + BridgeUnlock, + FeeAssetChange, + FeeChange, + IbcRelayerChange, + IbcSudoChange, + InitBridgeAccount, + Sequence, + SudoAddressChange, + Transfer, + 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 Transfer { + #[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 BridgeLock { + #[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 BridgeSudoChange { + #[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 BridgeUnlock { + #[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 InitBridgeAccount { + #[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 Sequence { + #[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 SudoAddressChange { + async fn check_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for FeeChange { + async fn check_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for IbcSudoChange { + async fn check_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for IbcRelayerChange { + async fn check_and_pay_fees(&self, _state: S) -> Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl FeeHandler for FeeAssetChange { + 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::BridgeLock, + 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 = BridgeLock { + 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/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index d7093c545..1fbb69d4e 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -45,7 +45,6 @@ use crate::{ ActionHandler, StateReadExt as _, }, - assets::StateWriteExt as _, bridge::{ StateReadExt as _, StateWriteExt as _, @@ -214,11 +213,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 @@ -234,21 +228,11 @@ impl ActionHandler for action::Ics20Withdrawal { .wrap_err("packet failed send check")? }; - state - .get_and_increase_block_fees::(self.fee_asset(), fee) - .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()) { diff --git a/crates/astria-sequencer/src/lib.rs b/crates/astria-sequencer/src/lib.rs index 55bf2dfa5..9dfe244a3 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 ccc472849..d8f65205b 100644 --- a/crates/astria-sequencer/src/sequence/action.rs +++ b/crates/astria-sequencer/src/sequence/action.rs @@ -1,25 +1,11 @@ use astria_core::protocol::transaction::v1alpha1::action::Sequence; use astria_eyre::eyre::{ ensure, - OptionExt as _, Result, - WrapErr as _, }; use cnidarium::StateWrite; -use crate::{ - accounts::{ - StateReadExt as _, - StateWriteExt as _, - }, - app::ActionHandler, - assets::{ - StateReadExt, - StateWriteExt, - }, - sequence, - transaction::StateReadExt as _, -}; +use crate::app::ActionHandler; #[async_trait::async_trait] impl ActionHandler for Sequence { @@ -33,78 +19,7 @@ impl ActionHandler for Sequence { Ok(()) } - async fn check_and_execute(&self, mut state: S) -> Result<()> { - let from = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action") - .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 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) - .await - .wrap_err("failed to add to block fees")?; - state - .decrease_balance(&from, &self.fee_asset, fee) - .await - .wrap_err("failed updating `from` account balance")?; + async fn check_and_execute(&self, _state: S) -> Result<()> { 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 9530de421..d26e68749 100644 --- a/crates/astria-sequencer/src/transaction/checks.rs +++ b/crates/astria-sequencer/src/transaction/checks.rs @@ -225,7 +225,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 @@ -256,7 +256,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]), @@ -352,7 +352,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(), ) @@ -430,7 +430,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 47225cacf..5d662e711 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -45,6 +45,7 @@ use crate::{ StateReadExt as _, StateWriteExt as _, }, + fees::FeeHandler, ibc::{ host_interface::AstriaHost, StateReadExt as _, @@ -216,28 +217,22 @@ impl ActionHandler for SignedTransaction { state.put_transaction_context(transaction_context); match action { - Action::Transfer(act) => act - .check_and_execute(&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_and_execute(&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_and_execute(&mut state) + Action::ValidatorUpdate(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("executing validor update")?, - Action::SudoAddressChange(act) => act - .check_and_execute(&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_and_execute(&mut state) + Action::FeeChange(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("executing fee change failed")?, Action::Ibc(act) => { @@ -261,32 +256,25 @@ impl ActionHandler for SignedTransaction { .map_err(anyhow_to_eyre) .wrap_err("failed executing ibc action")?; } - Action::Ics20Withdrawal(act) => act - .check_and_execute(&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_and_execute(&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_and_execute(&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_and_execute(&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_and_execute(&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_and_execute(&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_and_execute(&mut state) + Action::BridgeSudoChange(act) => check_execute_and_pay_fees(act, &mut state) .await .wrap_err("failed executing bridge sudo change")?, } @@ -297,3 +285,12 @@ impl ActionHandler for SignedTransaction { 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(()) +}