From ecd1064e8d9604668b0fed52e7a304317488b9b0 Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 1 Aug 2024 09:32:55 +0200 Subject: [PATCH 01/23] remove address_bytes field from mempool enqueuedtransaction --- crates/astria-sequencer/src/mempool/mod.rs | 25 ++++++---------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/crates/astria-sequencer/src/mempool/mod.rs b/crates/astria-sequencer/src/mempool/mod.rs index 6ccf5f9e22..28dc93b1e0 100644 --- a/crates/astria-sequencer/src/mempool/mod.rs +++ b/crates/astria-sequencer/src/mempool/mod.rs @@ -78,16 +78,13 @@ impl PartialOrd for TransactionPriority { pub(crate) struct EnqueuedTransaction { tx_hash: [u8; 32], signed_tx: Arc, - address_bytes: [u8; ADDRESS_LEN], } impl EnqueuedTransaction { fn new(signed_tx: SignedTransaction) -> Self { - let address_bytes = signed_tx.verification_key().address_bytes(); Self { tx_hash: signed_tx.sha256_of_proto_encoding(), signed_tx: Arc::new(signed_tx), - address_bytes, } } @@ -118,7 +115,7 @@ impl EnqueuedTransaction { } pub(crate) fn address_bytes(&self) -> [u8; 20] { - self.address_bytes + self.signed_tx.address_bytes() } } @@ -301,11 +298,10 @@ impl Mempool { /// removes a transaction from the mempool #[instrument(skip_all)] pub(crate) async fn remove(&self, tx_hash: [u8; 32]) { - let (signed_tx, address_bytes) = dummy_signed_tx(); + let signed_tx = dummy_signed_tx(); let enqueued_tx = EnqueuedTransaction { tx_hash, signed_tx, - address_bytes, }; self.queue.write().await.remove(&enqueued_tx); } @@ -411,26 +407,22 @@ impl Mempool { /// this `signed_tx` field is ignored in the `PartialEq` and `Hash` impls of `EnqueuedTransaction` - /// only the tx hash is considered. So we create an `EnqueuedTransaction` on the fly with the /// correct tx hash and this dummy signed tx when removing from the queue. -fn dummy_signed_tx() -> (Arc, [u8; ADDRESS_LEN]) { - static TX: OnceLock<(Arc, [u8; ADDRESS_LEN])> = OnceLock::new(); - let (signed_tx, address_bytes) = TX.get_or_init(|| { +fn dummy_signed_tx() -> Arc { + static TX: OnceLock> = OnceLock::new(); + let signed_tx = TX.get_or_init(|| { let actions = vec![]; let params = TransactionParams::builder() .nonce(0) .chain_id("dummy") .build(); let signing_key = SigningKey::from([0; 32]); - let address_bytes = signing_key.verification_key().address_bytes(); let unsigned_tx = UnsignedTransaction { actions, params, }; - ( - Arc::new(unsigned_tx.into_signed(&signing_key)), - address_bytes, - ) + Arc::new(unsigned_tx.into_signed(&signing_key)) }); - (signed_tx.clone(), *address_bytes) + signed_tx.clone() } #[cfg(test)] @@ -514,17 +506,14 @@ mod test { let tx0 = EnqueuedTransaction { tx_hash: [0; 32], signed_tx: Arc::new(get_mock_tx(0)), - address_bytes: get_mock_tx(0).address_bytes(), }; let other_tx0 = EnqueuedTransaction { tx_hash: [0; 32], signed_tx: Arc::new(get_mock_tx(1)), - address_bytes: get_mock_tx(1).address_bytes(), }; let tx1 = EnqueuedTransaction { tx_hash: [1; 32], signed_tx: Arc::new(get_mock_tx(0)), - address_bytes: get_mock_tx(0).address_bytes(), }; assert!(tx0 == other_tx0); assert!(tx0 != tx1); From 5b1f68018cc1b4ac96978c8d300d76e6edd23b7b Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 1 Aug 2024 14:32:33 +0200 Subject: [PATCH 02/23] combine stateful check and execute into one atomic operation --- Cargo.lock | 1 - crates/astria-sequencer/Cargo.toml | 1 - .../astria-sequencer/src/accounts/action.rs | 166 +++++----- crates/astria-sequencer/src/accounts/mod.rs | 54 ++++ .../src/accounts/state_ext.rs | 80 ++--- .../src/app/action_handler.rs | 22 ++ crates/astria-sequencer/src/app/mod.rs | 33 +- .../astria-sequencer/src/authority/action.rs | 99 +++--- .../src/authority/state_ext.rs | 9 +- .../src/bridge/bridge_lock_action.rs | 49 +-- .../src/bridge/bridge_sudo_change_action.rs | 35 +-- .../src/bridge/bridge_unlock_action.rs | 69 ++-- .../src/bridge/init_bridge_account_action.rs | 51 ++- crates/astria-sequencer/src/bridge/query.rs | 31 +- .../astria-sequencer/src/bridge/state_ext.rs | 139 ++++---- .../astria-sequencer/src/fee_asset_change.rs | 37 ++- .../src/ibc/ibc_relayer_change.rs | 33 +- .../src/ibc/ics20_withdrawal.rs | 83 +++-- crates/astria-sequencer/src/ibc/state_ext.rs | 23 +- .../astria-sequencer/src/sequence/action.rs | 68 ++-- .../astria-sequencer/src/service/mempool.rs | 3 +- .../src/transaction/action_handler.rs | 24 -- .../src/transaction/checks.rs | 64 ++-- .../astria-sequencer/src/transaction/mod.rs | 296 ++++++------------ .../src/transaction/state_ext.rs | 51 +++ 25 files changed, 718 insertions(+), 803 deletions(-) create mode 100644 crates/astria-sequencer/src/app/action_handler.rs delete mode 100644 crates/astria-sequencer/src/transaction/action_handler.rs create mode 100644 crates/astria-sequencer/src/transaction/state_ext.rs diff --git a/Cargo.lock b/Cargo.lock index 0270043a9d..9be9b3954f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -791,7 +791,6 @@ dependencies = [ "borsh", "bytes", "cnidarium", - "cnidarium-component", "divan", "futures", "hex", diff --git a/crates/astria-sequencer/Cargo.toml b/crates/astria-sequencer/Cargo.toml index cef3ced64a..d487930dcb 100644 --- a/crates/astria-sequencer/Cargo.toml +++ b/crates/astria-sequencer/Cargo.toml @@ -28,7 +28,6 @@ borsh = { version = "1", features = ["derive"] } cnidarium = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.78.0", features = [ "metrics", ] } -cnidarium-component = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.78.0" } ibc-proto = { version = "0.41.0", features = ["server"] } matchit = "0.7.2" priority-queue = "2.0.2" diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index 8d4853ea68..56ac42cb6d 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -4,30 +4,39 @@ use anyhow::{ Result, }; use astria_core::{ - primitive::v1::Address, + primitive::v1::ADDRESS_LEN, protocol::transaction::v1alpha1::action::TransferAction, Protobuf, }; -use tracing::instrument; +use cnidarium::{ + StateRead, + StateWrite, +}; +use super::GetAddressBytes; use crate::{ accounts::{ - self, StateReadExt as _, + StateWriteExt as _, + }, + address::StateReadExt as _, + app::ActionHandler, + assets::{ + StateReadExt as _, + StateWriteExt as _, }, - address, - assets, bridge::StateReadExt as _, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; -pub(crate) async fn transfer_check_stateful( +pub(crate) async fn transfer_check_stateful( action: &TransferAction, state: &S, - from: Address, + from: TAddress, ) -> Result<()> where - S: accounts::StateReadExt + assets::StateReadExt + 'static, + S: StateRead, + TAddress: GetAddressBytes, { ensure!( state @@ -44,7 +53,7 @@ where let transfer_asset = action.asset.clone(); let from_fee_balance = state - .get_account_balance(from, &action.fee_asset) + .get_account_balance(&from, &action.fee_asset) .await .context("failed getting `from` account balance for fee payment")?; @@ -83,81 +92,86 @@ where #[async_trait::async_trait] impl ActionHandler for TransferAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful(&self, state: &S, from: Address) -> Result<()> - where - S: accounts::StateReadExt + address::StateReadExt + 'static, - { - state.ensure_base_prefix(&self.to).await.context( - "failed ensuring that the destination address matches the permitted base prefix", - )?; - ensure!( - state - .get_bridge_account_rollup_id(&from) - .await - .context("failed to get bridge account rollup id")? - .is_none(), - "cannot transfer out of bridge account; BridgeUnlock must be used", - ); + async fn check_and_execute(&self, state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); + check_and_execute_transfer(self, from, state).await?; + Ok(()) + } +} - transfer_check_stateful(self, state, from) +pub(crate) async fn check_and_execute_transfer( + action: &TransferAction, + from: [u8; ADDRESS_LEN], + mut state: S, +) -> anyhow::Result<()> { + state.ensure_base_prefix(&action.to).await.context( + "failed ensuring that the destination address matches the permitted base prefix", + )?; + ensure!( + state + .get_bridge_account_rollup_id(from) .await - .context("stateful transfer check failed") - } + .context("failed to get bridge account rollup id")? + .is_none(), + "cannot transfer out of bridge account; BridgeUnlock must be used", + ); + + transfer_check_stateful(action, &state, from) + .await + .context("stateful transfer check failed")?; + + let fee = state + .get_transfer_base_fee() + .await + .context("failed to get transfer base fee")?; + state + .get_and_increase_block_fees(&action.fee_asset, fee, TransferAction::full_name()) + .await + .context("failed to add to block fees")?; - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> Result<()> - where - S: accounts::StateWriteExt + assets::StateWriteExt, - { - let fee = state - .get_transfer_base_fee() + // 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 - .context("failed to get transfer base fee")?; + .context("failed decreasing `from` account balance")?; state - .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) + .increase_balance(action.to, &action.asset, action.amount) .await - .context("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 self.asset.to_ibc_prefixed() == self.fee_asset.to_ibc_prefixed() { - // check_stateful should have already checked this arithmetic - let payment_amount = self - .amount - .checked_add(fee) - .expect("transfer amount plus fee should not overflow"); - - state - .decrease_balance(from, &self.asset, payment_amount) - .await - .context("failed decreasing `from` account balance")?; - state - .increase_balance(self.to, &self.asset, self.amount) - .await - .context("failed increasing `to` account balance")?; - } else { - // otherwise, just transfer the transfer asset and deduct fee from fee asset balance - // later - state - .decrease_balance(from, &self.asset, self.amount) - .await - .context("failed decreasing `from` account balance")?; - state - .increase_balance(self.to, &self.asset, self.amount) - .await - .context("failed increasing `to` account balance")?; - - // deduct fee from fee asset balance - state - .decrease_balance(from, &self.fee_asset, fee) - .await - .context("failed decreasing `from` account balance for fee payment")?; - } + .context("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 + .context("failed decreasing `from` account balance")?; + state + .increase_balance(action.to, &action.asset, action.amount) + .await + .context("failed increasing `to` account balance")?; - Ok(()) + // deduct fee from fee asset balance + state + .decrease_balance(from, &action.fee_asset, fee) + .await + .context("failed decreasing `from` account balance for fee payment")?; } + Ok(()) } diff --git a/crates/astria-sequencer/src/accounts/mod.rs b/crates/astria-sequencer/src/accounts/mod.rs index ae0b420808..734492e0b2 100644 --- a/crates/astria-sequencer/src/accounts/mod.rs +++ b/crates/astria-sequencer/src/accounts/mod.rs @@ -3,7 +3,61 @@ pub(crate) mod component; pub(crate) mod query; mod state_ext; +use astria_core::{ + crypto::{ + SigningKey, + VerificationKey, + }, + primitive::v1::{ + Address, + ADDRESS_LEN, + }, + protocol::transaction::v1alpha1::SignedTransaction, +}; pub(crate) use state_ext::{ StateReadExt, StateWriteExt, }; + +pub(crate) trait GetAddressBytes: Send + Sync { + fn get_address_bytes(&self) -> [u8; ADDRESS_LEN]; +} + +impl GetAddressBytes for Address { + fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.bytes() + } +} + +impl GetAddressBytes for [u8; ADDRESS_LEN] { + fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { + *self + } +} + +impl GetAddressBytes for SignedTransaction { + fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes() + } +} + +impl GetAddressBytes for SigningKey { + fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes() + } +} + +impl GetAddressBytes for VerificationKey { + fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes() + } +} + +impl<'a, T> GetAddressBytes for &'a T +where + T: GetAddressBytes, +{ + fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { + (*self).get_address_bytes() + } +} diff --git a/crates/astria-sequencer/src/accounts/state_ext.rs b/crates/astria-sequencer/src/accounts/state_ext.rs index fa0fe00a6c..f4f60a2931 100644 --- a/crates/astria-sequencer/src/accounts/state_ext.rs +++ b/crates/astria-sequencer/src/accounts/state_ext.rs @@ -3,14 +3,9 @@ use anyhow::{ Result, }; use astria_core::{ - crypto::{ - SigningKey, - VerificationKey, - }, primitive::v1::{ asset, Address, - ADDRESS_LEN, }, protocol::account::v1alpha1::AssetBalance, }; @@ -26,6 +21,8 @@ use cnidarium::{ use futures::StreamExt; use tracing::instrument; +use super::GetAddressBytes; + /// Newtype wrapper to read and write a u32 from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] struct Nonce(u32); @@ -41,43 +38,6 @@ struct Fee(u128); const ACCOUNTS_PREFIX: &str = "accounts"; const TRANSFER_BASE_FEE_STORAGE_KEY: &str = "transferfee"; -trait GetAddressBytes: Send + Sync { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN]; -} - -impl GetAddressBytes for Address { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - self.bytes() - } -} - -impl GetAddressBytes for [u8; ADDRESS_LEN] { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - *self - } -} - -impl GetAddressBytes for SigningKey { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - self.verification_key().get_address_bytes() - } -} - -impl GetAddressBytes for VerificationKey { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - self.address_bytes() - } -} - -impl<'a, T> GetAddressBytes for &'a T -where - T: GetAddressBytes, -{ - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - (*self).get_address_bytes() - } -} - struct StorageKey<'a, T>(&'a T); impl<'a, T: GetAddressBytes> std::fmt::Display for StorageKey<'a, T> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -90,8 +50,8 @@ impl<'a, T: GetAddressBytes> std::fmt::Display for StorageKey<'a, T> { } } -fn balance_storage_key>( - address: Address, +fn balance_storage_key>( + address: TAddress, asset: TAsset, ) -> String { format!( @@ -161,8 +121,13 @@ pub(crate) trait StateReadExt: StateRead + crate::assets::StateReadExt { } #[instrument(skip_all)] - async fn get_account_balance<'a, TAsset>(&self, address: Address, asset: TAsset) -> Result + async fn get_account_balance<'a, TAddress, TAsset>( + &self, + address: TAddress, + asset: TAsset, + ) -> Result where + TAddress: GetAddressBytes, TAsset: Into + std::fmt::Display + Send, { let Some(bytes) = self @@ -211,13 +176,14 @@ impl StateReadExt for T {} #[async_trait] pub(crate) trait StateWriteExt: StateWrite { #[instrument(skip_all)] - fn put_account_balance( + fn put_account_balance( &mut self, - address: Address, + address: TAddress, asset: TAsset, balance: u128, ) -> Result<()> where + TAddress: GetAddressBytes, TAsset: Into + std::fmt::Display + Send, { let bytes = borsh::to_vec(&Balance(balance)).context("failed to serialize balance")?; @@ -226,29 +192,30 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - fn put_account_nonce(&mut self, address: Address, nonce: u32) -> Result<()> { + fn put_account_nonce(&mut self, address: T, nonce: u32) -> Result<()> { let bytes = borsh::to_vec(&Nonce(nonce)).context("failed to serialize nonce")?; self.put_raw(nonce_storage_key(address), bytes); Ok(()) } #[instrument(skip_all)] - async fn increase_balance( + async fn increase_balance( &mut self, - address: Address, + address: TAddress, asset: TAsset, amount: u128, ) -> Result<()> where + TAddress: GetAddressBytes, TAsset: Into + std::fmt::Display + Send, { let asset = asset.into(); let balance = self - .get_account_balance(address, asset) + .get_account_balance(&address, asset) .await .context("failed to get account balance")?; self.put_account_balance( - address, + &address, asset, balance .checked_add(amount) @@ -259,22 +226,23 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - async fn decrease_balance( + async fn decrease_balance( &mut self, - address: Address, + address: TAddress, asset: TAsset, amount: u128, ) -> Result<()> where + TAddress: GetAddressBytes, TAsset: Into + std::fmt::Display + Send, { let asset = asset.into(); let balance = self - .get_account_balance(address, asset) + .get_account_balance(&address, asset) .await .context("failed to get account balance")?; self.put_account_balance( - address, + &address, asset, balance .checked_sub(amount) diff --git a/crates/astria-sequencer/src/app/action_handler.rs b/crates/astria-sequencer/src/app/action_handler.rs new file mode 100644 index 0000000000..01daf1fb4b --- /dev/null +++ b/crates/astria-sequencer/src/app/action_handler.rs @@ -0,0 +1,22 @@ +use std::sync::Arc; + +use cnidarium::{ + StateRead, + StateWrite, +}; + +/// This trait is a verbatim copy of [`cnidarium_component::ActionHandler`]. +/// +/// It's duplicated here because all actions are foreign types, forbidding +/// the the implementation of [`cnidarium_component::ActionHandler`] for these +/// types due to Rust orphan rules. +#[async_trait::async_trait] +pub(crate) trait ActionHandler { + type CheckStatelessContext: Clone + Send + Sync + 'static; + async fn check_stateless(&self, context: Self::CheckStatelessContext) -> anyhow::Result<()>; + + async fn check_historical(&self, _state: Arc) -> anyhow::Result<()> { + Ok(()) + } + async fn check_and_execute(&self, mut state: S) -> anyhow::Result<()>; +} diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index cc59071f13..5a9931fa42 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -7,11 +7,13 @@ mod tests_breaking_changes; #[cfg(test)] mod tests_execute_transaction; +mod action_handler; use std::{ collections::VecDeque, sync::Arc, }; +pub(crate) use action_handler::ActionHandler; use anyhow::{ anyhow, ensure, @@ -19,7 +21,6 @@ use anyhow::{ }; use astria_core::{ generated::protocol::transaction::v1alpha1 as raw, - primitive::v1::Address, protocol::{ abci::AbciErrorCode, transaction::v1alpha1::{ @@ -105,10 +106,7 @@ use crate::{ StateReadExt as _, StateWriteExt as _, }, - transaction::{ - self, - InvalidNonce, - }, + transaction::InvalidNonce, }; /// The inter-block state being written to by the application. @@ -982,24 +980,14 @@ impl App { signed_tx: Arc, ) -> anyhow::Result> { let signed_tx_2 = signed_tx.clone(); - let stateless = tokio::spawn( - async move { transaction::check_stateless(&signed_tx_2).await }.in_current_span(), - ); - let signed_tx_2 = signed_tx.clone(); - let state2 = self.state.clone(); - let stateful = tokio::spawn( - async move { transaction::check_stateful(&signed_tx_2, &state2).await } - .in_current_span(), - ); + let stateless = + tokio::spawn(async move { signed_tx_2.check_stateless(()).await }.in_current_span()); stateless .await .context("stateless check task aborted while executing")? .context("stateless check failed")?; - stateful - .await - .context("stateful check task aborted while executing")? - .context("stateful check failed")?; + // At this point, the stateful checks should have completed, // leaving us with exclusive access to the Arc. let mut state_tx = self @@ -1007,20 +995,19 @@ impl App { .try_begin_transaction() .expect("state Arc should be present and unique"); - transaction::execute(&signed_tx, &mut state_tx) + signed_tx + .check_and_execute(&mut state_tx) .await .context("failed executing transaction")?; - let (_, events) = state_tx.apply(); - info!(event_count = events.len(), "executed transaction"); - Ok(events) + Ok(state_tx.apply().1) } #[instrument(name = "App::end_block", skip_all)] pub(crate) async fn end_block( &mut self, height: u64, - fee_recipient: Address, + fee_recipient: [u8; 20], ) -> anyhow::Result { let state_tx = StateDelta::new(self.state.clone()); let mut arc_state_tx = Arc::new(state_tx); diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index baf6220db9..bf652145bc 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -4,30 +4,41 @@ use anyhow::{ Context as _, Result, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::action::{ - FeeChange, - FeeChangeAction, - SudoAddressChangeAction, - ValidatorUpdate, - }, +use astria_core::protocol::transaction::v1alpha1::action::{ + FeeChange, + FeeChangeAction, + SudoAddressChangeAction, + ValidatorUpdate, }; -use tracing::instrument; +use cnidarium::StateWrite; use crate::{ - address, - authority, - transaction::action_handler::ActionHandler, + accounts::StateWriteExt as _, + address::StateReadExt as _, + app::ActionHandler, + authority::{ + StateReadExt as _, + StateWriteExt as _, + }, + bridge::StateWriteExt as _, + ibc::StateWriteExt as _, + sequence::StateWriteExt as _, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for ValidatorUpdate { - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + Ok(()) + } + + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); // ensure signer is the valid `sudo` key in state let sudo_address = state .get_sudo_address() @@ -52,15 +63,7 @@ impl ActionHandler for ValidatorUpdate { // check that this is not the only validator, cannot remove the last one ensure!(validator_set.len() != 1, "cannot remove the last validator"); } - Ok(()) - } - #[instrument(skip_all)] - async fn execute( - &self, - state: &mut S, - _: Address, - ) -> Result<()> { // add validator update in non-consensus state to be used in end_block let mut validator_updates = state .get_validator_updates() @@ -76,17 +79,19 @@ impl ActionHandler for ValidatorUpdate { #[async_trait::async_trait] impl ActionHandler for SudoAddressChangeAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } /// check that the signer of the transaction is the current sudo address, /// as only that address can change the sudo address - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); state .ensure_base_prefix(&self.new_address) .await @@ -97,11 +102,6 @@ impl ActionHandler for SudoAddressChangeAction { .await .context("failed to get sudo address from state")?; ensure!(sudo_address == from, "signer is not the sudo key"); - Ok(()) - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, _: Address) -> Result<()> { state .put_sudo_address(self.new_address) .context("failed to put sudo address in state")?; @@ -111,30 +111,25 @@ impl ActionHandler for SudoAddressChangeAction { #[async_trait::async_trait] impl ActionHandler for FeeChangeAction { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + Ok(()) + } + /// check that the signer of the transaction is the current sudo address, /// as only that address can change the fee - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); // ensure signer is the valid `sudo` key in state let sudo_address = state .get_sudo_address() .await .context("failed to get sudo address from state")?; ensure!(sudo_address == from, "signer is not the sudo key"); - Ok(()) - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, _: Address) -> Result<()> { - use crate::{ - accounts::StateWriteExt as _, - bridge::StateWriteExt as _, - ibc::StateWriteExt as _, - sequence::StateWriteExt as _, - }; match self.fee_change { FeeChange::TransferBaseFee => { diff --git a/crates/astria-sequencer/src/authority/state_ext.rs b/crates/astria-sequencer/src/authority/state_ext.rs index afdb4856a8..595aeeb3f8 100644 --- a/crates/astria-sequencer/src/authority/state_ext.rs +++ b/crates/astria-sequencer/src/authority/state_ext.rs @@ -21,7 +21,6 @@ use cnidarium::{ use tracing::instrument; use super::ValidatorSet; -use crate::address; /// Newtype wrapper to read and write an address from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -32,9 +31,9 @@ const VALIDATOR_SET_STORAGE_KEY: &str = "valset"; const VALIDATOR_UPDATES_KEY: &[u8] = b"valupdates"; #[async_trait] -pub(crate) trait StateReadExt: StateRead + address::StateReadExt { +pub(crate) trait StateReadExt: StateRead { #[instrument(skip_all)] - async fn get_sudo_address(&self) -> Result
{ + async fn get_sudo_address(&self) -> Result<[u8; ADDRESS_LEN]> { let Some(bytes) = self .get_raw(SUDO_STORAGE_KEY) .await @@ -45,9 +44,7 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { }; let SudoAddress(address_bytes) = SudoAddress::try_from_slice(&bytes).context("invalid sudo key bytes")?; - self.try_base_prefixed(&address_bytes) - .await - .context("failed constructing address from prefixed stored in state") + Ok(address_bytes) } #[instrument(skip_all)] diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 2326593150..cc898be1ef 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -4,64 +4,54 @@ use anyhow::{ Result, }; use astria_core::{ - primitive::v1::Address, protocol::transaction::v1alpha1::action::{ BridgeLockAction, TransferAction, }, sequencerblock::v1alpha1::block::Deposit, }; -use tracing::instrument; +use cnidarium::StateWrite; use crate::{ accounts::{ - action::transfer_check_stateful, StateReadExt as _, StateWriteExt as _, }, - address, + address::StateReadExt as _, + app::ActionHandler, bridge::{ StateReadExt as _, StateWriteExt as _, }, - state_ext::{ - StateReadExt, - StateWriteExt, - }, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for BridgeLockAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); state .ensure_base_prefix(&self.to) .await .context("failed check for base prefix of destination address")?; - let transfer_action = TransferAction { - to: self.to, - asset: self.asset.clone(), - amount: self.amount, - fee_asset: self.fee_asset.clone(), - }; - // ensure the recipient is a bridge account. let rollup_id = state - .get_bridge_account_rollup_id(&self.to) + .get_bridge_account_rollup_id(self.to) .await .context("failed to get bridge account rollup id")? .ok_or_else(|| anyhow::anyhow!("bridge lock must be sent to a bridge account"))?; let allowed_asset = state - .get_bridge_account_ibc_asset(&self.to) + .get_bridge_account_ibc_asset(self.to) .await .context("failed to get bridge account asset ID")?; ensure!( @@ -95,12 +85,6 @@ impl ActionHandler for BridgeLockAction { .saturating_add(transfer_fee); ensure!(from_balance >= fee, "insufficient funds for fee payment"); - // this performs the same checks as a normal `TransferAction` - transfer_check_stateful(&transfer_action, state, from).await - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> Result<()> { let transfer_action = TransferAction { to: self.to, asset: self.asset.clone(), @@ -108,13 +92,12 @@ impl ActionHandler for BridgeLockAction { fee_asset: self.fee_asset.clone(), }; - transfer_action - .execute(state, from) + crate::accounts::action::check_and_execute_transfer(&transfer_action, from, &mut state) .await .context("failed to execute bridge lock action as transfer action")?; let rollup_id = state - .get_bridge_account_rollup_id(&self.to) + .get_bridge_account_rollup_id(self.to) .await .context("failed to get bridge account rollup id")? .expect("recipient must be a bridge account; this is a bug in check_stateful"); 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 518a9417e9..432c9a4ab4 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -3,38 +3,34 @@ use anyhow::{ Context as _, Result, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::action::BridgeSudoChangeAction, -}; -use tracing::instrument; +use astria_core::protocol::transaction::v1alpha1::action::BridgeSudoChangeAction; +use cnidarium::StateWrite; use crate::{ accounts::StateWriteExt as _, - address, + address::StateReadExt as _, + app::ActionHandler, assets::StateReadExt as _, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, }, - state_ext::{ - StateReadExt, - StateWriteExt, - }, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for BridgeSudoChangeAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); state .ensure_base_prefix(&self.bridge_address) .await @@ -76,11 +72,6 @@ impl ActionHandler for BridgeSudoChangeAction { "unauthorized for bridge sudo change action", ); - Ok(()) - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, _: Address) -> Result<()> { let fee = state .get_bridge_sudo_change_base_fee() .await diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index 550cf5dce4..3500f342db 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -4,37 +4,32 @@ use anyhow::{ Context as _, Result, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::action::{ - BridgeUnlockAction, - TransferAction, - }, +use astria_core::protocol::transaction::v1alpha1::action::{ + BridgeUnlockAction, + TransferAction, }; -use tracing::instrument; +use cnidarium::StateWrite; use crate::{ - accounts::action::transfer_check_stateful, - address, + address::StateReadExt as _, + app::ActionHandler, bridge::StateReadExt as _, - state_ext::{ - StateReadExt, - StateWriteExt, - }, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for BridgeUnlockAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); state .ensure_base_prefix(&self.to) .await @@ -48,7 +43,7 @@ impl ActionHandler for BridgeUnlockAction { // the bridge address to withdraw funds from // if unset, use the tx sender's address - let bridge_address = self.bridge_address.unwrap_or(from); + let bridge_address = self.bridge_address.map_or(from, |addr| addr.bytes()); // grab the bridge account's asset let asset = state @@ -77,31 +72,13 @@ impl ActionHandler for BridgeUnlockAction { fee_asset: self.fee_asset.clone(), }; - // this performs the same checks as a normal `TransferAction` - transfer_check_stateful(&transfer_action, state, bridge_address).await - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> Result<()> { - // the bridge address to withdraw funds from - let bridge_address = self.bridge_address.unwrap_or(from); - - let asset = state - .get_bridge_account_ibc_asset(&bridge_address) - .await - .context("failed to get bridge's asset id, must be a bridge account")?; - - let transfer_action = TransferAction { - to: self.to, - asset: asset.into(), - amount: self.amount, - fee_asset: self.fee_asset.clone(), - }; - - transfer_action - .execute(state, bridge_address) - .await - .context("failed to execute bridge unlock action as transfer action")?; + crate::accounts::action::check_and_execute_transfer( + &transfer_action, + bridge_address, + &mut state, + ) + .await + .context("failed to execute bridge lock action as transfer action")?; Ok(()) } 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 986a314bfe..3431ea4019 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -4,41 +4,37 @@ use anyhow::{ Context as _, Result, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::action::InitBridgeAccountAction, -}; -use tracing::instrument; +use astria_core::protocol::transaction::v1alpha1::action::InitBridgeAccountAction; +use cnidarium::StateWrite; use crate::{ accounts::{ StateReadExt as _, StateWriteExt as _, }, - address, + address::StateReadExt as _, + app::ActionHandler, assets::StateReadExt as _, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, }, - state_ext::{ - StateReadExt, - StateWriteExt, - }, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for InitBridgeAccountAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); if let Some(withdrawer_address) = &self.withdrawer_address { state .ensure_base_prefix(withdrawer_address) @@ -87,23 +83,18 @@ impl ActionHandler for InitBridgeAccountAction { "insufficient funds for bridge account initialization", ); - Ok(()) - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> Result<()> { - let fee = state - .get_init_bridge_account_base_fee() - .await - .context("failed to get base fee for initializing bridge account")?; - state.put_bridge_account_rollup_id(&from, &self.rollup_id); state .put_bridge_account_ibc_asset(&from, &self.asset) .context("failed to put asset ID")?; - state.put_bridge_account_sudo_address(&from, &self.sudo_address.unwrap_or(from)); - state - .put_bridge_account_withdrawer_address(&from, &self.withdrawer_address.unwrap_or(from)); + state.put_bridge_account_sudo_address( + &from, + &self.sudo_address.map_or(from, |addr| addr.bytes()), + ); + state.put_bridge_account_withdrawer_address( + &from, + &self.withdrawer_address.map_or(from, |addr| addr.bytes()), + ); state .decrease_balance(from, &self.fee_asset, fee) diff --git a/crates/astria-sequencer/src/bridge/query.rs b/crates/astria-sequencer/src/bridge/query.rs index 3ea29d867d..9ca0bee321 100644 --- a/crates/astria-sequencer/src/bridge/query.rs +++ b/crates/astria-sequencer/src/bridge/query.rs @@ -14,6 +14,7 @@ use tendermint::abci::{ }; use crate::{ + address::StateReadExt, assets::StateReadExt as _, bridge::StateReadExt as _, state_ext::StateReadExt as _, @@ -83,8 +84,8 @@ async fn get_bridge_account_info( } }; - let sudo_address = match snapshot.get_bridge_account_sudo_address(&address).await { - Ok(Some(sudo_address)) => sudo_address, + let sudo_address_bytes = match snapshot.get_bridge_account_sudo_address(&address).await { + Ok(Some(bytes)) => bytes, Ok(None) => { return Err(error_query_response( None, @@ -101,7 +102,19 @@ async fn get_bridge_account_info( } }; - let withdrawer_address = match snapshot + let sudo_address = match snapshot.try_base_prefixed(&sudo_address_bytes).await { + Err(err) => { + return Err(error_query_response( + Some(err), + AbciErrorCode::INTERNAL_ERROR, + "failed to construct bech32m address from address prefix and account bytes read \ + from state", + )); + } + Ok(address) => address, + }; + + let withdrawer_address_bytes = match snapshot .get_bridge_account_withdrawer_address(&address) .await { @@ -122,6 +135,18 @@ async fn get_bridge_account_info( } }; + let withdrawer_address = match snapshot.try_base_prefixed(&withdrawer_address_bytes).await { + Err(err) => { + return Err(error_query_response( + Some(err), + AbciErrorCode::INTERNAL_ERROR, + "failed to construct bech32m address from address prefix and account bytes read \ + from state", + )); + } + Ok(address) => address, + }; + Ok(Some(BridgeAccountInfo { rollup_id, asset: trace_asset.into(), diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index 3b8ba0b5f6..ceeea2943b 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -14,6 +14,7 @@ use astria_core::{ asset, Address, RollupId, + ADDRESS_LEN, }, sequencerblock::v1alpha1::block::Deposit, }; @@ -34,7 +35,10 @@ use tracing::{ instrument, }; -use crate::address; +use crate::{ + accounts::GetAddressBytes, + address, +}; /// Newtype wrapper to read and write a u128 from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -60,23 +64,26 @@ const INIT_BRIDGE_ACCOUNT_BASE_FEE_STORAGE_KEY: &str = "initbridgeaccfee"; const BRIDGE_LOCK_BYTE_COST_MULTIPLIER_STORAGE_KEY: &str = "bridgelockmultiplier"; const BRIDGE_SUDO_CHANGE_FEE_STORAGE_KEY: &str = "bridgesudofee"; -struct BridgeAccountKey<'a> { +struct BridgeAccountKey<'a, T> { prefix: &'static str, - address: &'a Address, + address: &'a T, } -impl<'a> std::fmt::Display for BridgeAccountKey<'a> { +impl<'a, T> std::fmt::Display for BridgeAccountKey<'a, T> +where + T: GetAddressBytes, +{ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(self.prefix)?; f.write_str("/")?; - for byte in self.address.bytes() { + for byte in self.address.get_address_bytes() { f.write_fmt(format_args!("{byte:02x}"))?; } Ok(()) } } -fn rollup_id_storage_key(address: &Address) -> String { +fn rollup_id_storage_key(address: &T) -> String { format!( "{}/rollupid", BridgeAccountKey { @@ -86,7 +93,7 @@ fn rollup_id_storage_key(address: &Address) -> String { ) } -fn asset_id_storage_key(address: &Address) -> String { +fn asset_id_storage_key(address: &T) -> String { format!( "{}/assetid", BridgeAccountKey { @@ -108,7 +115,7 @@ fn deposit_nonce_storage_key(rollup_id: &RollupId) -> Vec { format!("depositnonce/{}", rollup_id.encode_hex::()).into() } -fn bridge_account_sudo_address_storage_key(address: &Address) -> String { +fn bridge_account_sudo_address_storage_key(address: &T) -> String { format!( "{}", BridgeAccountKey { @@ -118,7 +125,7 @@ fn bridge_account_sudo_address_storage_key(address: &Address) -> String { ) } -fn bridge_account_withdrawer_address_storage_key(address: &Address) -> String { +fn bridge_account_withdrawer_address_storage_key(address: &T) -> String { format!( "{}", BridgeAccountKey { @@ -128,7 +135,9 @@ fn bridge_account_withdrawer_address_storage_key(address: &Address) -> String { ) } -fn last_transaction_hash_for_bridge_account_storage_key(address: &Address) -> Vec { +fn last_transaction_hash_for_bridge_account_storage_key( + address: &T, +) -> Vec { format!( "{}/lasttx", BridgeAccountKey { @@ -143,9 +152,12 @@ fn last_transaction_hash_for_bridge_account_storage_key(address: &Address) -> Ve #[async_trait] pub(crate) trait StateReadExt: StateRead + address::StateReadExt { #[instrument(skip_all)] - async fn get_bridge_account_rollup_id(&self, address: &Address) -> Result> { + async fn get_bridge_account_rollup_id( + &self, + address: T, + ) -> Result> { let Some(rollup_id_bytes) = self - .get_raw(&rollup_id_storage_key(address)) + .get_raw(&rollup_id_storage_key(&address)) .await .context("failed reading raw account rollup ID from state")? else { @@ -159,9 +171,12 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { } #[instrument(skip_all)] - async fn get_bridge_account_ibc_asset(&self, address: &Address) -> Result { + async fn get_bridge_account_ibc_asset( + &self, + address: T, + ) -> Result { let bytes = self - .get_raw(&asset_id_storage_key(address)) + .get_raw(&asset_id_storage_key(&address)) .await .context("failed reading raw asset ID from state")? .ok_or_else(|| anyhow!("asset ID not found"))?; @@ -171,34 +186,35 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { } #[instrument(skip_all)] - async fn get_bridge_account_sudo_address( + async fn get_bridge_account_sudo_address( &self, - bridge_address: &Address, - ) -> Result> { + bridge_address: T, + ) -> Result> { let Some(sudo_address_bytes) = self - .get_raw(&bridge_account_sudo_address_storage_key(bridge_address)) + .get_raw(&bridge_account_sudo_address_storage_key(&bridge_address)) .await .context("failed reading raw bridge account sudo address from state")? else { debug!("bridge account sudo address not found, returning None"); return Ok(None); }; - - let sudo_address = self.try_base_prefixed(&sudo_address_bytes).await.context( - "failed check for constructing sudo address from address bytes and prefix stored \ - retrieved from state", - )?; + let sudo_address = sudo_address_bytes.try_into().map_err(|bytes: Vec<_>| { + anyhow::format_err!( + "failed to convert address `{}` bytes read from state to fixed length address", + bytes.len() + ) + })?; Ok(Some(sudo_address)) } #[instrument(skip_all)] - async fn get_bridge_account_withdrawer_address( + async fn get_bridge_account_withdrawer_address( &self, - bridge_address: &Address, - ) -> Result> { + bridge_address: T, + ) -> Result> { let Some(withdrawer_address_bytes) = self .get_raw(&bridge_account_withdrawer_address_storage_key( - bridge_address, + &bridge_address, )) .await .context("failed reading raw bridge account withdrawer address from state")? @@ -206,15 +222,15 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { debug!("bridge account withdrawer address not found, returning None"); return Ok(None); }; - - let withdrawer_address = self - .try_base_prefixed(&withdrawer_address_bytes) - .await - .context( - "failed check for constructing withdrawer address from address bytes and prefix \ - stored retrieved from state", - )?; - Ok(Some(withdrawer_address)) + let addr = withdrawer_address_bytes + .try_into() + .map_err(|bytes: Vec<_>| { + anyhow::Error::msg(format!( + "failed converting `{}` bytes retrieved from storage to fixed address length", + bytes.len() + )) + })?; + Ok(Some(addr)) } #[instrument(skip_all)] @@ -347,48 +363,59 @@ impl StateReadExt for T {} #[async_trait] pub(crate) trait StateWriteExt: StateWrite { #[instrument(skip_all)] - fn put_bridge_account_rollup_id(&mut self, address: &Address, rollup_id: &RollupId) { - self.put_raw(rollup_id_storage_key(address), rollup_id.to_vec()); + fn put_bridge_account_rollup_id( + &mut self, + address: T, + rollup_id: &RollupId, + ) { + self.put_raw(rollup_id_storage_key(&address), rollup_id.to_vec()); } #[instrument(skip_all)] - fn put_bridge_account_ibc_asset( + fn put_bridge_account_ibc_asset( &mut self, - address: &Address, + address: TAddress, asset: TAsset, ) -> Result<()> where + TAddress: GetAddressBytes, TAsset: Into + std::fmt::Display, { let ibc = asset.into(); self.put_raw( - asset_id_storage_key(address), + asset_id_storage_key(&address), borsh::to_vec(&AssetId(ibc.get())).context("failed to serialize asset IDs")?, ); Ok(()) } #[instrument(skip_all)] - fn put_bridge_account_sudo_address( + fn put_bridge_account_sudo_address( &mut self, - bridge_address: &Address, - sudo_address: &Address, - ) { + bridge_address: TBridgeAddress, + sudo_address: TSudoAddress, + ) where + TBridgeAddress: GetAddressBytes, + TSudoAddress: GetAddressBytes, + { self.put_raw( - bridge_account_sudo_address_storage_key(bridge_address), - sudo_address.bytes().to_vec(), + bridge_account_sudo_address_storage_key(&bridge_address), + sudo_address.get_address_bytes().to_vec(), ); } #[instrument(skip_all)] - fn put_bridge_account_withdrawer_address( + fn put_bridge_account_withdrawer_address( &mut self, - bridge_address: &Address, - withdrawer_address: &Address, - ) { + bridge_address: TBridgeAddress, + withdrawer_address: TWithdrawerAddress, + ) where + TBridgeAddress: GetAddressBytes, + TWithdrawerAddress: GetAddressBytes, + { self.put_raw( - bridge_account_withdrawer_address_storage_key(bridge_address), - withdrawer_address.bytes().to_vec(), + bridge_account_withdrawer_address_storage_key(&bridge_address), + withdrawer_address.get_address_bytes().to_vec(), ); } @@ -465,13 +492,13 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - fn put_last_transaction_hash_for_bridge_account( + fn put_last_transaction_hash_for_bridge_account( &mut self, - address: &Address, + address: T, tx_hash: &[u8; 32], ) { self.nonverifiable_put_raw( - last_transaction_hash_for_bridge_account_storage_key(address), + last_transaction_hash_for_bridge_account_storage_key(&address), tx_hash.to_vec(), ); } diff --git a/crates/astria-sequencer/src/fee_asset_change.rs b/crates/astria-sequencer/src/fee_asset_change.rs index 7ccdfdf804..8779e5dbb9 100644 --- a/crates/astria-sequencer/src/fee_asset_change.rs +++ b/crates/astria-sequencer/src/fee_asset_change.rs @@ -4,26 +4,33 @@ use anyhow::{ Context as _, Result, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::action::FeeAssetChangeAction, -}; +use astria_core::protocol::transaction::v1alpha1::action::FeeAssetChangeAction; use async_trait::async_trait; +use cnidarium::StateWrite; use crate::{ - assets, - assets::StateReadExt as _, - authority, - transaction::action_handler::ActionHandler, + app::ActionHandler, + assets::{ + StateReadExt as _, + StateWriteExt as _, + }, + authority::StateReadExt as _, + transaction::StateReadExt as _, }; #[async_trait] impl ActionHandler for FeeAssetChangeAction { - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + Ok(()) + } + + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); let authority_sudo_address = state .get_sudo_address() .await @@ -32,10 +39,6 @@ impl ActionHandler for FeeAssetChangeAction { authority_sudo_address == from, "unauthorized address for fee asset change" ); - Ok(()) - } - - async fn execute(&self, state: &mut S, _from: Address) -> Result<()> { match self { FeeAssetChangeAction::Addition(asset) => { state.put_allowed_fee_asset(asset); diff --git a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs index cb37a1ac30..fbf8ff58f8 100644 --- a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs +++ b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs @@ -3,29 +3,33 @@ use anyhow::{ Context as _, Result, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::action::IbcRelayerChangeAction, -}; +use astria_core::protocol::transaction::v1alpha1::action::IbcRelayerChangeAction; use async_trait::async_trait; +use cnidarium::StateWrite; use crate::{ - address, - ibc, - transaction::action_handler::ActionHandler, + address::StateReadExt as _, + app::ActionHandler, + ibc::{ + StateReadExt as _, + StateWriteExt as _, + }, + transaction::StateReadExt as _, }; #[async_trait] impl ActionHandler for IbcRelayerChangeAction { - async fn check_stateless(&self) -> Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); match self { IbcRelayerChangeAction::Addition(addr) | IbcRelayerChangeAction::Removal(addr) => { state.ensure_base_prefix(addr).await.context( @@ -42,10 +46,7 @@ impl ActionHandler for IbcRelayerChangeAction { ibc_sudo_address == from, "unauthorized address for IBC relayer change" ); - Ok(()) - } - async fn execute(&self, state: &mut S, _from: Address) -> Result<()> { match self { IbcRelayerChangeAction::Addition(address) => { state.put_ibc_relayer_address(address); diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 859ca1d920..4c508ce6eb 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -6,12 +6,13 @@ use anyhow::{ Result, }; use astria_core::{ - primitive::v1::{ - asset::Denom, - Address, - }, + primitive::v1::asset::Denom, protocol::transaction::v1alpha1::action, }; +use cnidarium::{ + StateRead, + StateWrite, +}; use ibc_types::core::channel::{ ChannelId, PortId, @@ -22,17 +23,21 @@ use penumbra_ibc::component::packet::{ SendPacketWrite as _, Unchecked, }; -use tracing::instrument; use crate::{ - accounts, - address, + accounts::{ + GetAddressBytes, + StateReadExt as _, + StateWriteExt as _, + }, + address::StateReadExt as _, + app::ActionHandler, bridge::StateReadExt as _, ibc::{ StateReadExt as _, StateWriteExt as _, }, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; fn withdrawal_to_unchecked_ibc_packet( @@ -51,10 +56,10 @@ fn withdrawal_to_unchecked_ibc_packet( ) } -async fn ics20_withdrawal_check_stateful_bridge_account( +async fn ics20_withdrawal_check_stateful_bridge_account( action: &action::Ics20Withdrawal, state: &S, - from: Address, + from: [u8; 20], ) -> Result<()> { // bridge address checks: // - if the sender of this transaction is not a bridge account, and the tx `bridge_address` @@ -76,7 +81,7 @@ async fn ics20_withdrawal_check_stateful_bridge_account Result<()> { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { ensure!(self.timeout_time() != 0, "timeout time must be non-zero",); // NOTE (from penumbra): we could validate the destination chain address as bech32 to @@ -106,12 +112,12 @@ impl ActionHandler for action::Ics20Withdrawal { Ok(()) } - #[instrument(skip_all)] - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); + state .ensure_base_prefix(&self.return_address) .await @@ -123,18 +129,20 @@ impl ActionHandler for action::Ics20Withdrawal { )?; } - ics20_withdrawal_check_stateful_bridge_account(self, state, from).await?; + ics20_withdrawal_check_stateful_bridge_account(self, &state, from).await?; let fee = state .get_ics20_withdrawal_base_fee() .await .context("failed to get ics20 withdrawal base fee")?; - let packet: IBCPacket = withdrawal_to_unchecked_ibc_packet(self); - state - .send_packet_check(packet) - .await - .context("packet failed send check")?; + let packet = { + let packet = withdrawal_to_unchecked_ibc_packet(self); + state + .send_packet_check(packet) + .await + .context("packet failed send check")? + }; let transfer_asset = self.denom(); @@ -173,21 +181,6 @@ impl ActionHandler for action::Ics20Withdrawal { ); } - Ok(()) - } - - #[instrument(skip_all)] - async fn execute( - &self, - state: &mut S, - from: Address, - ) -> Result<()> { - let fee = state - .get_ics20_withdrawal_base_fee() - .await - .context("failed to get ics20 withdrawal base fee")?; - let checked_packet = withdrawal_to_unchecked_ibc_packet(self).assume_checked(); - state .decrease_balance(from, self.denom(), self.amount()) .await @@ -200,11 +193,7 @@ impl ActionHandler for action::Ics20Withdrawal { // if we're the source, move tokens to the escrow account, // otherwise the tokens are just burned - if is_source( - checked_packet.source_port(), - checked_packet.source_channel(), - self.denom(), - ) { + if is_source(packet.source_port(), packet.source_channel(), self.denom()) { let channel_balance = state .get_ibc_channel_balance(self.source_channel(), self.denom()) .await @@ -221,7 +210,7 @@ impl ActionHandler for action::Ics20Withdrawal { .context("failed to update channel balance")?; } - state.send_packet_execute(checked_packet).await; + state.send_packet_execute(packet).await; Ok(()) } } diff --git a/crates/astria-sequencer/src/ibc/state_ext.rs b/crates/astria-sequencer/src/ibc/state_ext.rs index 3abde83481..fce3e7c701 100644 --- a/crates/astria-sequencer/src/ibc/state_ext.rs +++ b/crates/astria-sequencer/src/ibc/state_ext.rs @@ -23,7 +23,7 @@ use tracing::{ instrument, }; -use crate::address; +use crate::accounts::GetAddressBytes; /// Newtype wrapper to read and write a u128 from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -40,13 +40,13 @@ struct Fee(u128); const IBC_SUDO_STORAGE_KEY: &str = "ibcsudo"; const ICS20_WITHDRAWAL_BASE_FEE_STORAGE_KEY: &str = "ics20withdrawalfee"; -struct IbcRelayerKey<'a>(&'a Address); +struct IbcRelayerKey<'a, T>(&'a T); -impl<'a> std::fmt::Display for IbcRelayerKey<'a> { +impl<'a, T: GetAddressBytes> std::fmt::Display for IbcRelayerKey<'a, T> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str("ibc-relayer")?; f.write_str("/")?; - for byte in self.0.bytes() { + for byte in self.0.get_address_bytes() { f.write_fmt(format_args!("{byte:02x}"))?; } Ok(()) @@ -63,12 +63,12 @@ fn channel_balance_storage_key>( ) } -fn ibc_relayer_key(address: &Address) -> String { +fn ibc_relayer_key(address: &T) -> String { IbcRelayerKey(address).to_string() } #[async_trait] -pub(crate) trait StateReadExt: StateRead + address::StateReadExt { +pub(crate) trait StateReadExt: StateRead { #[instrument(skip_all)] async fn get_ibc_channel_balance( &self, @@ -91,7 +91,7 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { } #[instrument(skip_all)] - async fn get_ibc_sudo_address(&self) -> Result
{ + async fn get_ibc_sudo_address(&self) -> Result<[u8; ADDRESS_LEN]> { let Some(bytes) = self .get_raw(IBC_SUDO_STORAGE_KEY) .await @@ -102,16 +102,13 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { }; let SudoAddress(address_bytes) = SudoAddress::try_from_slice(&bytes).context("invalid ibc sudo key bytes")?; - Ok(self.try_base_prefixed(&address_bytes).await.context( - "failed constructing ibc sudo address from address bytes and address prefix stored in \ - state", - )?) + Ok(address_bytes) } #[instrument(skip_all)] - async fn is_ibc_relayer(&self, address: &Address) -> Result { + async fn is_ibc_relayer(&self, address: T) -> Result { Ok(self - .get_raw(&ibc_relayer_key(address)) + .get_raw(&ibc_relayer_key(&address)) .await .context("failed to read ibc relayer key from state")? .is_some()) diff --git a/crates/astria-sequencer/src/sequence/action.rs b/crates/astria-sequencer/src/sequence/action.rs index 5b52bfe760..e81a0f5387 100644 --- a/crates/astria-sequencer/src/sequence/action.rs +++ b/crates/astria-sequencer/src/sequence/action.rs @@ -4,50 +4,30 @@ use anyhow::{ Result, }; use astria_core::{ - primitive::v1::Address, protocol::transaction::v1alpha1::action::SequenceAction, - Protobuf, + Protobuf as _, }; -use tracing::instrument; +use cnidarium::StateWrite; use crate::{ - accounts, accounts::{ + StateReadExt as _, + StateWriteExt as _, + }, + app::ActionHandler, + assets::{ StateReadExt, StateWriteExt, }, - assets, sequence, - transaction::action_handler::ActionHandler, + transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for SequenceAction { - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> Result<()> - where - S: accounts::StateReadExt + assets::StateReadExt + 'static, - { - ensure!( - state.is_allowed_fee_asset(&self.fee_asset).await?, - "invalid fee asset", - ); + type CheckStatelessContext = (); - let curr_balance = state - .get_account_balance(from, &self.fee_asset) - .await - .context("failed getting `from` account balance for fee payment")?; - let fee = calculate_fee_from_state(&self.data, state) - .await - .context("calculated fee overflows u128")?; - ensure!(curr_balance >= fee, "insufficient funds"); - Ok(()) - } - - async fn check_stateless(&self) -> Result<()> { + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { // TODO: do we want to place a maximum on the size of the data? // https://github.com/astriaorg/astria/issues/222 ensure!( @@ -57,14 +37,28 @@ impl ActionHandler for SequenceAction { Ok(()) } - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> Result<()> - where - S: assets::StateWriteExt, - { - let fee = calculate_fee_from_state(&self.data, state) + async fn check_and_execute(&self, mut state: S) -> Result<()> { + let from = state + .get_current_source() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); + + ensure!( + state + .is_allowed_fee_asset(&self.fee_asset) + .await + .context("failed accessing state to check if fee is allowed")?, + "invalid fee asset", + ); + + let curr_balance = state + .get_account_balance(from, &self.fee_asset) + .await + .context("failed getting `from` account balance for fee payment")?; + let fee = calculate_fee_from_state(&self.data, &state) .await - .context("failed to calculate fee")?; + .context("calculated fee overflows u128")?; + ensure!(curr_balance >= fee, "insufficient funds"); state .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) diff --git a/crates/astria-sequencer/src/service/mempool.rs b/crates/astria-sequencer/src/service/mempool.rs index 8ce3d114d3..cc6480c7b0 100644 --- a/crates/astria-sequencer/src/service/mempool.rs +++ b/crates/astria-sequencer/src/service/mempool.rs @@ -38,6 +38,7 @@ use tracing::{ use crate::{ accounts, address, + app::ActionHandler as _, mempool::{ Mempool as AppMempool, RemovalReason, @@ -168,7 +169,7 @@ async fn handle_check_tx Result<()> { - Ok(()) - } - async fn check_stateful( - &self, - _state: &S, - _from: Address, - ) -> Result<()> { - Ok(()) - } - async fn execute(&self, _state: &mut S, _from: Address) -> Result<()> { - Ok(()) - } -} diff --git a/crates/astria-sequencer/src/transaction/checks.rs b/crates/astria-sequencer/src/transaction/checks.rs index f96f26ebd3..8bd8ce3a46 100644 --- a/crates/astria-sequencer/src/transaction/checks.rs +++ b/crates/astria-sequencer/src/transaction/checks.rs @@ -7,7 +7,6 @@ use anyhow::{ use astria_core::{ primitive::v1::{ asset, - Address, RollupId, }, protocol::transaction::v1alpha1::{ @@ -19,21 +18,22 @@ use astria_core::{ UnsignedTransaction, }, }; +use cnidarium::StateRead; use tracing::instrument; use crate::{ - accounts, - address, + accounts::StateReadExt as _, + address::StateReadExt as _, bridge::StateReadExt as _, ibc::StateReadExt as _, state_ext::StateReadExt as _, }; #[instrument(skip_all)] -pub(crate) async fn check_nonce_mempool(tx: &SignedTransaction, state: &S) -> anyhow::Result<()> -where - S: accounts::StateReadExt + address::StateReadExt + 'static, -{ +pub(crate) async fn check_nonce_mempool( + tx: &SignedTransaction, + state: &S, +) -> anyhow::Result<()> { let signer_address = state .try_base_prefixed(&tx.verification_key().address_bytes()) .await @@ -50,13 +50,10 @@ where } #[instrument(skip_all)] -pub(crate) async fn check_chain_id_mempool( +pub(crate) async fn check_chain_id_mempool( tx: &SignedTransaction, state: &S, -) -> anyhow::Result<()> -where - S: accounts::StateReadExt + 'static, -{ +) -> anyhow::Result<()> { let chain_id = state .get_chain_id() .await @@ -66,28 +63,18 @@ where } #[instrument(skip_all)] -pub(crate) async fn check_balance_mempool( +pub(crate) async fn check_balance_mempool( tx: &SignedTransaction, state: &S, -) -> anyhow::Result<()> -where - S: accounts::StateReadExt + address::StateReadExt + 'static, -{ - let signer_address = state - .try_base_prefixed(&tx.verification_key().address_bytes()) - .await - .context( - "failed constructing the signer address from signed transaction verification and \ - prefix provided by app state", - )?; - check_balance_for_total_fees_and_transfers(tx.unsigned_transaction(), signer_address, state) +) -> anyhow::Result<()> { + check_balance_for_total_fees_and_transfers(tx, state) .await .context("failed to check balance for total fees and transfers")?; Ok(()) } #[instrument(skip_all)] -pub(crate) async fn get_fees_for_transaction( +pub(crate) async fn get_fees_for_transaction( tx: &UnsignedTransaction, state: &S, ) -> anyhow::Result> { @@ -163,20 +150,16 @@ pub(crate) async fn get_fees_for_transaction( - tx: &UnsignedTransaction, - from: Address, +pub(crate) async fn check_balance_for_total_fees_and_transfers( + tx: &SignedTransaction, state: &S, -) -> anyhow::Result<()> -where - S: accounts::StateReadExt + 'static, -{ - let mut cost_by_asset = get_fees_for_transaction(tx, state) +) -> anyhow::Result<()> { + let mut cost_by_asset = get_fees_for_transaction(tx.unsigned_transaction(), state) .await .context("failed to get fees for transaction")?; // add values transferred within the tx to the cost - for action in &tx.actions { + for action in tx.actions() { match action { Action::Transfer(act) => { cost_by_asset @@ -198,7 +181,7 @@ where } Action::BridgeUnlock(act) => { let asset = state - .get_bridge_account_ibc_asset(&from) + .get_bridge_account_ibc_asset(&tx) .await .context("failed to get bridge account asset id")?; cost_by_asset @@ -222,7 +205,7 @@ where for (asset, total_fee) in cost_by_asset { let balance = state - .get_account_balance(from, asset) + .get_account_balance(tx, asset) .await .context("failed to get account balance")?; ensure!( @@ -246,15 +229,12 @@ fn transfer_update_fees( .or_insert(transfer_fee); } -async fn sequence_update_fees( +async fn sequence_update_fees( state: &S, fee_asset: &asset::Denom, fees_by_asset: &mut HashMap, data: &[u8], -) -> anyhow::Result<()> -where - S: accounts::StateReadExt, -{ +) -> anyhow::Result<()> { let fee = crate::sequence::calculate_fee_from_state(data, state) .await .context("fee for sequence action overflowed; data too large")?; diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index 1426a91804..fd50dc09a8 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -1,21 +1,16 @@ -pub(crate) mod action_handler; mod checks; pub(crate) mod query; +mod state_ext; use std::fmt; -pub(crate) use action_handler::ActionHandler; use anyhow::{ ensure, Context as _, }; -use astria_core::{ - primitive::v1::Address, - protocol::transaction::v1alpha1::{ - action::Action, - SignedTransaction, - UnsignedTransaction, - }, +use astria_core::protocol::transaction::v1alpha1::{ + action::Action, + SignedTransaction, }; pub(crate) use checks::{ check_balance_for_total_fees_and_transfers, @@ -23,12 +18,22 @@ pub(crate) use checks::{ check_chain_id_mempool, check_nonce_mempool, }; -use tracing::instrument; +use cnidarium::StateWrite; +pub(crate) use state_ext::{ + StateReadExt, + StateWriteExt, +}; use crate::{ - accounts, - address, - bridge, + accounts::{ + StateReadExt as _, + StateWriteExt as _, + }, + app::ActionHandler, + bridge::{ + StateReadExt as _, + StateWriteExt as _, + }, ibc::{ host_interface::AstriaHost, StateReadExt as _, @@ -36,64 +41,6 @@ use crate::{ state_ext::StateReadExt as _, }; -#[instrument(skip_all)] -pub(crate) async fn check_stateless(tx: &SignedTransaction) -> anyhow::Result<()> { - tx.unsigned_transaction() - .check_stateless() - .await - .context("stateless check failed") -} - -#[instrument(skip_all)] -pub(crate) async fn check_stateful(tx: &SignedTransaction, state: &S) -> anyhow::Result<()> -where - S: accounts::StateReadExt + address::StateReadExt + 'static, -{ - let signer_address = state - .try_base_prefixed(&tx.verification_key().address_bytes()) - .await - .context( - "failed constructing signed address from state and verification key contained in \ - signed transaction", - )?; - tx.unsigned_transaction() - .check_stateful(state, signer_address) - .await -} - -pub(crate) async fn execute(tx: &SignedTransaction, state: &mut S) -> anyhow::Result<()> -where - S: accounts::StateReadExt - + accounts::StateWriteExt - + address::StateReadExt - + bridge::StateReadExt - + bridge::StateWriteExt, -{ - let signer_address = state - .try_base_prefixed(&tx.verification_key().address_bytes()) - .await - .context( - "failed constructing signed address from state and verification key contained in \ - signed transaction", - )?; - - if state - .get_bridge_account_rollup_id(&signer_address) - .await - .context("failed to check account rollup id")? - .is_some() - { - state.put_last_transaction_hash_for_bridge_account( - &signer_address, - &tx.sha256_of_proto_encoding(), - ); - } - - tx.unsigned_transaction() - .execute(state, signer_address) - .await -} - #[derive(Debug)] pub(crate) struct InvalidChainId(pub(crate) String); @@ -125,30 +72,32 @@ impl fmt::Display for InvalidNonce { impl std::error::Error for InvalidNonce {} #[async_trait::async_trait] -impl ActionHandler for UnsignedTransaction { - async fn check_stateless(&self) -> anyhow::Result<()> { - ensure!(!self.actions.is_empty(), "must have at least one action"); +impl ActionHandler for SignedTransaction { + type CheckStatelessContext = (); + + async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> anyhow::Result<()> { + ensure!(!self.actions().is_empty(), "must have at least one action"); - for action in &self.actions { + for action in self.actions() { match action { Action::Transfer(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for TransferAction")?, Action::Sequence(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for SequenceAction")?, Action::ValidatorUpdate(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for ValidatorUpdateAction")?, Action::SudoAddressChange(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for SudoAddressChangeAction")?, Action::FeeChange(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for FeeChangeAction")?, Action::Ibc(act) => { @@ -161,31 +110,31 @@ impl ActionHandler for UnsignedTransaction { .context("stateless check failed for IbcAction")?; } Action::Ics20Withdrawal(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for Ics20WithdrawalAction")?, Action::IbcRelayerChange(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for IbcRelayerChangeAction")?, Action::FeeAssetChange(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for FeeAssetChangeAction")?, Action::InitBridgeAccount(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for InitBridgeAccountAction")?, Action::BridgeLock(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for BridgeLockAction")?, Action::BridgeUnlock(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for BridgeLockAction")?, Action::BridgeSudoChange(act) => act - .check_stateless() + .check_stateless(()) .await .context("stateless check failed for BridgeSudoChangeAction")?, } @@ -193,10 +142,12 @@ impl ActionHandler for UnsignedTransaction { Ok(()) } - async fn check_stateful(&self, state: &S, from: Address) -> anyhow::Result<()> - where - S: accounts::StateReadExt + 'static, - { + async fn check_and_execute(&self, mut state: S) -> anyhow::Result<()> { + // Add the current signed transaction into the ephemeral state in case + // downstream actions require access to it. + // XXX: This must be deleted at the end of `check_stateful`. + state.put_current_source(self); + // Transactions must match the chain id of the node. let chain_id = state.get_chain_id().await?; ensure!( @@ -206,169 +157,112 @@ impl ActionHandler for UnsignedTransaction { // Nonce should be equal to the number of executed transactions before this tx. // First tx has nonce 0. - let curr_nonce = state.get_account_nonce(from).await?; + let curr_nonce = state + .get_account_nonce(self.address_bytes()) + .await + .context("failed to get nonce for transaction signer")?; ensure!(curr_nonce == self.nonce(), InvalidNonce(self.nonce())); // Should have enough balance to cover all actions. - check_balance_for_total_fees_and_transfers(self, from, state) + check_balance_for_total_fees_and_transfers(self, &state) .await .context("failed to check balance for total fees and transfers")?; - for action in &self.actions { + if state + .get_bridge_account_rollup_id(self) + .await + .context("failed to check account rollup id")? + .is_some() + { + state.put_last_transaction_hash_for_bridge_account( + self, + &self.sha256_of_proto_encoding(), + ); + } + + let from_nonce = state + .get_account_nonce(self) + .await + .context("failed getting `from` nonce")?; + let next_nonce = from_nonce + .checked_add(1) + .context("overflow occurred incrementing stored nonce")?; + state + .put_account_nonce(self, next_nonce) + .context("failed updating `from` nonce")?; + + for action in self.actions() { match action { Action::Transfer(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for TransferAction")?, Action::Sequence(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for SequenceAction")?, Action::ValidatorUpdate(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for ValidatorUpdateAction")?, Action::SudoAddressChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for SudoAddressChangeAction")?, Action::FeeChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for FeeChangeAction")?, - Action::Ibc(_) => { + Action::Ibc(act) => { + // FIXME: this check could should be moved to check_and_execute, as it now has + // access to the state. ensure!( state - .is_ibc_relayer(&from) + .is_ibc_relayer(self) .await .context("failed to check if address is IBC relayer")?, "only IBC sudo address can execute IBC actions" ); + let action = act + .clone() + .with_handler::(); + action + .check_and_execute(&mut state) + .await + .context("execution failed for IbcAction")?; } Action::Ics20Withdrawal(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for Ics20WithdrawalAction")?, Action::IbcRelayerChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for IbcRelayerChangeAction")?, Action::FeeAssetChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for FeeAssetChangeAction")?, Action::InitBridgeAccount(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for InitBridgeAccountAction")?, Action::BridgeLock(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for BridgeLockAction")?, Action::BridgeUnlock(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for BridgeUnlockAction")?, Action::BridgeSudoChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await .context("stateful check failed for BridgeSudoChangeAction")?, } } - Ok(()) - } - - #[instrument(skip_all)] - async fn execute(&self, state: &mut S, from: Address) -> anyhow::Result<()> - where - S: accounts::StateReadExt + accounts::StateWriteExt, - { - let from_nonce = state - .get_account_nonce(from) - .await - .context("failed getting `from` nonce")?; - let next_nonce = from_nonce - .checked_add(1) - .context("overflow occurred incrementing stored nonce")?; - state - .put_account_nonce(from, next_nonce) - .context("failed updating `from` nonce")?; - - for action in &self.actions { - match action { - Action::Transfer(act) => { - act.execute(state, from) - .await - .context("execution failed for TransferAction")?; - } - Action::Sequence(act) => { - act.execute(state, from) - .await - .context("execution failed for SequenceAction")?; - } - Action::ValidatorUpdate(act) => { - act.execute(state, from) - .await - .context("execution failed for ValidatorUpdateAction")?; - } - Action::SudoAddressChange(act) => { - act.execute(state, from) - .await - .context("execution failed for SudoAddressChangeAction")?; - } - Action::FeeChange(act) => { - act.execute(state, from) - .await - .context("execution failed for FeeChangeAction")?; - } - Action::Ibc(act) => { - let action = act - .clone() - .with_handler::(); - action - .check_and_execute(&mut *state) - .await - .context("execution failed for IbcAction")?; - } - Action::Ics20Withdrawal(act) => { - act.execute(state, from) - .await - .context("execution failed for Ics20WithdrawalAction")?; - } - Action::IbcRelayerChange(act) => { - act.execute(state, from) - .await - .context("execution failed for IbcRelayerChangeAction")?; - } - Action::FeeAssetChange(act) => { - act.execute(state, from) - .await - .context("execution failed for FeeAssetChangeAction")?; - } - Action::InitBridgeAccount(act) => { - act.execute(state, from) - .await - .context("execution failed for InitBridgeAccountAction")?; - } - Action::BridgeLock(act) => { - act.execute(state, from) - .await - .context("execution failed for BridgeLockAction")?; - } - Action::BridgeUnlock(act) => { - act.execute(state, from) - .await - .context("execution failed for BridgeUnlockAction")?; - } - Action::BridgeSudoChange(act) => { - act.execute(state, from) - .await - .context("execution failed for BridgeSudoChangeAction")?; - } - } - } - + state.delete_current_source().await; Ok(()) } } diff --git a/crates/astria-sequencer/src/transaction/state_ext.rs b/crates/astria-sequencer/src/transaction/state_ext.rs new file mode 100644 index 0000000000..9dd1de4614 --- /dev/null +++ b/crates/astria-sequencer/src/transaction/state_ext.rs @@ -0,0 +1,51 @@ +use astria_core::{ + primitive::v1::ADDRESS_LEN, + protocol::transaction::v1alpha1::SignedTransaction, +}; +use cnidarium::{ + StateRead, + StateWrite, +}; + +fn current_source() -> &'static str { + "transaction/current_source" +} + +#[derive(Clone)] +pub(crate) struct TransactionContext { + address_bytes: [u8; ADDRESS_LEN], +} + +impl TransactionContext { + pub(crate) fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes + } +} + +impl From<&SignedTransaction> for TransactionContext { + fn from(value: &SignedTransaction) -> Self { + Self { + address_bytes: value.address_bytes(), + } + } +} + +pub(crate) trait StateWriteExt: StateWrite { + fn put_current_source(&mut self, transaction: &SignedTransaction) { + let context: TransactionContext = transaction.into(); + self.object_put(current_source(), context); + } + + async fn delete_current_source(&mut self) { + self.object_delete(current_source()); + } +} + +pub(crate) trait StateReadExt: StateRead { + fn get_current_source(&self) -> Option { + self.object_get(current_source()) + } +} + +impl StateReadExt for T {} +impl StateWriteExt for T {} From 07c3b6137ea8e0c00fa70e6237fe2a7271b978b2 Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 1 Aug 2024 15:07:05 +0200 Subject: [PATCH 03/23] update all tests --- crates/astria-sequencer/src/app/tests_app.rs | 2 +- .../src/app/tests_execute_transaction.rs | 15 +- .../astria-sequencer/src/authority/action.rs | 51 ++----- .../src/authority/state_ext.rs | 20 +-- .../src/bridge/bridge_lock_action.rs | 13 +- .../src/bridge/bridge_sudo_change_action.rs | 19 ++- .../src/bridge/bridge_unlock_action.rs | 139 ++---------------- .../src/ibc/ics20_withdrawal.rs | 38 ++--- crates/astria-sequencer/src/ibc/state_ext.rs | 27 ++-- .../src/transaction/checks.rs | 9 +- 10 files changed, 88 insertions(+), 245 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_app.rs b/crates/astria-sequencer/src/app/tests_app.rs index 762a27ba1c..43c15058b2 100644 --- a/crates/astria-sequencer/src/app/tests_app.rs +++ b/crates/astria-sequencer/src/app/tests_app.rs @@ -679,7 +679,7 @@ async fn app_end_block_validator_updates() { ]; let mut app = initialize_app(None, initial_validator_set).await; - let proposer_address = astria_address(&[0u8; 20]); + let proposer_address = [0u8; 20]; let validator_updates = vec![ ValidatorUpdate { diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index f0c83ddfa6..635c9bbac4 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -33,7 +33,10 @@ use tendermint::abci::EventAttributeIndexExt as _; use crate::{ accounts::StateReadExt as _, - app::test_utils::*, + app::{ + test_utils::*, + ActionHandler as _, + }, assets::StateReadExt as _, authority::StateReadExt as _, bridge::{ @@ -435,7 +438,7 @@ async fn app_execute_transaction_sudo_address_change() { assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); let sudo_address = app.state.get_sudo_address().await.unwrap(); - assert_eq!(sudo_address, new_address); + assert_eq!(sudo_address, new_address.bytes()); } #[tokio::test] @@ -878,8 +881,6 @@ async fn app_execute_transaction_invalid_chain_id() { async fn app_stateful_check_fails_insufficient_total_balance() { use rand::rngs::OsRng; - use crate::transaction; - let mut app = initialize_app(None, vec![]).await; let alice = get_alice_signing_key(); @@ -939,7 +940,8 @@ async fn app_stateful_check_fails_insufficient_total_balance() { .into_signed(&keypair); // try double, see fails stateful check - let res = transaction::check_stateful(&signed_tx_fail, &app.state) + let res = signed_tx_fail + .check_and_execute(Arc::get_mut(&mut app.state).unwrap()) .await .unwrap_err() .root_cause() @@ -963,7 +965,8 @@ async fn app_stateful_check_fails_insufficient_total_balance() { } .into_signed(&keypair); - transaction::check_stateful(&signed_tx_pass, &app.state) + signed_tx_pass + .check_and_execute(Arc::get_mut(&mut app.state).unwrap()) .await .expect("stateful check should pass since we transferred enough to cover fee"); } diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index bf652145bc..76c342584c 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -167,23 +167,10 @@ mod test { use super::*; use crate::{ - accounts::{ - StateReadExt as _, - StateWriteExt as _, - }, - bridge::{ - StateReadExt as _, - StateWriteExt as _, - }, - ibc::{ - StateReadExt as _, - StateWriteExt as _, - }, - sequence::{ - StateReadExt as _, - StateWriteExt as _, - }, - test_utils::astria_address, + accounts::StateReadExt as _, + bridge::StateReadExt as _, + ibc::StateReadExt as _, + sequence::StateReadExt as _, }; #[tokio::test] @@ -199,10 +186,7 @@ mod test { new_value: 10, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!(state.get_transfer_base_fee().await.unwrap(), 10); let sequence_base_fee = 5; @@ -213,10 +197,7 @@ mod test { new_value: 3, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!(state.get_sequence_action_base_fee().await.unwrap(), 3); let sequence_byte_cost_multiplier = 2; @@ -227,10 +208,7 @@ mod test { new_value: 4, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!( state .get_sequence_action_byte_cost_multiplier() @@ -247,10 +225,7 @@ mod test { new_value: 2, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!(state.get_init_bridge_account_base_fee().await.unwrap(), 2); let bridge_lock_byte_cost_multiplier = 1; @@ -261,10 +236,7 @@ mod test { new_value: 2, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!( state.get_bridge_lock_byte_cost_multiplier().await.unwrap(), 2 @@ -280,10 +252,7 @@ mod test { new_value: 2, }; - fee_change - .execute(&mut state, astria_address(&[1; 20])) - .await - .unwrap(); + fee_change.check_and_execute(&mut state).await.unwrap(); assert_eq!(state.get_ics20_withdrawal_base_fee().await.unwrap(), 2); } } diff --git a/crates/astria-sequencer/src/authority/state_ext.rs b/crates/astria-sequencer/src/authority/state_ext.rs index 595aeeb3f8..6c59913339 100644 --- a/crates/astria-sequencer/src/authority/state_ext.rs +++ b/crates/astria-sequencer/src/authority/state_ext.rs @@ -5,10 +5,7 @@ use anyhow::{ Context, Result, }; -use astria_core::primitive::v1::{ - Address, - ADDRESS_LEN, -}; +use astria_core::primitive::v1::ADDRESS_LEN; use async_trait::async_trait; use borsh::{ BorshDeserialize, @@ -21,6 +18,7 @@ use cnidarium::{ use tracing::instrument; use super::ValidatorSet; +use crate::accounts::GetAddressBytes; /// Newtype wrapper to read and write an address from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -85,10 +83,10 @@ impl StateReadExt for T {} #[async_trait] pub(crate) trait StateWriteExt: StateWrite { #[instrument(skip_all)] - fn put_sudo_address(&mut self, address: Address) -> Result<()> { + fn put_sudo_address(&mut self, address: T) -> Result<()> { self.put_raw( SUDO_STORAGE_KEY.to_string(), - borsh::to_vec(&SudoAddress(address.bytes())) + borsh::to_vec(&SudoAddress(address.get_address_bytes())) .context("failed to convert sudo address to vec")?, ); Ok(()) @@ -123,7 +121,10 @@ impl StateWriteExt for T {} #[cfg(test)] mod tests { - use astria_core::protocol::transaction::v1alpha1::action::ValidatorUpdate; + use astria_core::{ + primitive::v1::ADDRESS_LEN, + protocol::transaction::v1alpha1::action::ValidatorUpdate, + }; use cnidarium::StateDelta; use super::{ @@ -134,7 +135,6 @@ mod tests { address::StateWriteExt as _, authority::ValidatorSet, test_utils::{ - astria_address, verification_key, ASTRIA_PREFIX, }, @@ -159,7 +159,7 @@ mod tests { .expect_err("no sudo address should exist at first"); // can write new - let mut address_expected = astria_address(&[42u8; 20]); + let mut address_expected = [42u8; ADDRESS_LEN]; state .put_sudo_address(address_expected) .expect("writing sudo address should not fail"); @@ -173,7 +173,7 @@ mod tests { ); // can rewrite with new value - address_expected = astria_address(&[41u8; 20]); + address_expected = [41u8; ADDRESS_LEN]; state .put_sudo_address(address_expected) .expect("writing sudo address should not fail"); diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index cc898be1ef..ff64ce6505 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -140,7 +140,6 @@ pub(crate) fn get_deposit_byte_len(deposit: &Deposit) -> u128 { #[cfg(test)] mod tests { - use address::StateWriteExt; use astria_core::primitive::v1::{ asset, RollupId, @@ -149,6 +148,7 @@ mod tests { use super::*; use crate::{ + address::StateWriteExt, assets::StateWriteExt as _, test_utils::{ astria_address, @@ -197,7 +197,7 @@ mod tests { assert!( bridge_lock - .check_stateful(&state, from_address) + .check_and_execute(&mut state) .await .unwrap_err() .to_string() @@ -216,10 +216,7 @@ mod tests { state .put_account_balance(from_address, &asset, 100 + expected_deposit_fee) .unwrap(); - bridge_lock - .check_stateful(&state, from_address) - .await - .unwrap(); + bridge_lock.check_and_execute(&mut state).await.unwrap(); } #[tokio::test] @@ -256,7 +253,7 @@ mod tests { .unwrap(); assert!( bridge_lock - .execute(&mut state, from_address) + .check_and_execute(&mut state) .await .unwrap_err() .to_string() @@ -275,6 +272,6 @@ mod tests { state .put_account_balance(from_address, &asset, 100 + expected_deposit_fee) .unwrap(); - bridge_lock.execute(&mut state, from_address).await.unwrap(); + bridge_lock.check_and_execute(&mut state).await.unwrap(); } } 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 432c9a4ab4..459f0c7b71 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -17,7 +17,6 @@ use crate::{ }, transaction::StateReadExt as _, }; - #[async_trait::async_trait] impl ActionHandler for BridgeSudoChangeAction { type CheckStatelessContext = (); @@ -58,7 +57,7 @@ impl ActionHandler for BridgeSudoChangeAction { // 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) + .get_bridge_account_sudo_address(self.bridge_address) .await .context("failed to get bridge account sudo address")? else { @@ -82,11 +81,11 @@ impl ActionHandler for BridgeSudoChangeAction { .context("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); + state.put_bridge_account_sudo_address(self.bridge_address, sudo_address); } if let Some(withdrawer_address) = self.new_withdrawer_address { - state.put_bridge_account_withdrawer_address(&self.bridge_address, &withdrawer_address); + state.put_bridge_account_withdrawer_address(self.bridge_address, withdrawer_address); } Ok(()) @@ -95,12 +94,12 @@ impl ActionHandler for BridgeSudoChangeAction { #[cfg(test)] mod tests { - use address::StateWriteExt; use astria_core::primitive::v1::asset; use cnidarium::StateDelta; use super::*; use crate::{ + address::StateWriteExt as _, assets::StateWriteExt as _, test_utils::{ astria_address, @@ -134,7 +133,7 @@ mod tests { fee_asset: asset.clone(), }; - action.check_stateful(&state, sudo_address).await.unwrap(); + action.check_and_execute(state).await.unwrap(); } #[tokio::test] @@ -161,7 +160,7 @@ mod tests { assert!( action - .check_stateful(&state, bridge_address) + .check_and_execute(state) .await .unwrap_err() .to_string() @@ -193,21 +192,21 @@ mod tests { fee_asset, }; - action.execute(&mut state, bridge_address).await.unwrap(); + action.check_and_execute(&mut state).await.unwrap(); assert_eq!( state .get_bridge_account_sudo_address(&bridge_address) .await .unwrap(), - Some(new_sudo_address), + Some(new_sudo_address.bytes()), ); assert_eq!( state .get_bridge_account_withdrawer_address(&bridge_address) .await .unwrap(), - Some(new_withdrawer_address), + Some(new_withdrawer_address.bytes()), ); } } diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index 3500f342db..ff72a7ddd3 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -119,7 +119,6 @@ mod test { let asset = test_asset(); let transfer_amount = 100; - let address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); let bridge_unlock = BridgeUnlockAction { @@ -133,7 +132,7 @@ mod test { // not a bridge account, should fail assert!( bridge_unlock - .check_stateful(&state, address) + .check_and_execute(state) .await .unwrap_err() .to_string() @@ -152,14 +151,13 @@ mod test { let asset = test_asset(); let transfer_amount = 100; - let sender_address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); let bridge_address = astria_address(&[3; 20]); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); - state.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); + state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); let bridge_unlock = BridgeUnlockAction { to: to_address, @@ -172,7 +170,7 @@ mod test { // invalid sender, doesn't match action's `from`, should fail assert!( bridge_unlock - .check_stateful(&state, sender_address) + .check_and_execute(state) .await .unwrap_err() .to_string() @@ -191,7 +189,6 @@ mod test { let asset = test_asset(); let transfer_amount = 100; - let sender_address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); let bridge_address = astria_address(&[3; 20]); @@ -212,7 +209,7 @@ mod test { // invalid sender, doesn't match action's bridge account's withdrawer, should fail assert!( bridge_unlock - .check_stateful(&state, sender_address) + .check_and_execute(state) .await .unwrap_err() .to_string() @@ -220,118 +217,6 @@ mod test { ); } - #[tokio::test] - async fn fee_check_stateful_from_none() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - - let asset = test_asset(); - let transfer_fee = 10; - let transfer_amount = 100; - state.put_transfer_base_fee(transfer_fee).unwrap(); - - let bridge_address = astria_address(&[1; 20]); - let to_address = astria_address(&[2; 20]); - let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); - state - .put_bridge_account_ibc_asset(&bridge_address, &asset) - .unwrap(); - state.put_allowed_fee_asset(&asset); - state.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); - - let bridge_unlock = BridgeUnlockAction { - to: to_address, - amount: transfer_amount, - fee_asset: asset.clone(), - memo: "{}".into(), - bridge_address: None, - }; - - // not enough balance to transfer asset; should fail - state - .put_account_balance(bridge_address, &asset, transfer_amount) - .unwrap(); - assert!( - bridge_unlock - .check_stateful(&state, bridge_address) - .await - .unwrap_err() - .to_string() - .contains("insufficient funds for transfer and fee payment") - ); - - // enough balance; should pass - state - .put_account_balance(bridge_address, &asset, transfer_amount + transfer_fee) - .unwrap(); - bridge_unlock - .check_stateful(&state, bridge_address) - .await - .unwrap(); - } - - #[tokio::test] - async fn fee_check_stateful_from_some() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - - let asset = test_asset(); - let transfer_fee = 10; - let transfer_amount = 100; - state.put_transfer_base_fee(transfer_fee).unwrap(); - - let bridge_address = astria_address(&[1; 20]); - let to_address = astria_address(&[2; 20]); - let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); - state - .put_bridge_account_ibc_asset(&bridge_address, &asset) - .unwrap(); - state.put_allowed_fee_asset(&asset); - - let withdrawer_address = astria_address(&[3; 20]); - state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); - - let bridge_unlock = BridgeUnlockAction { - to: to_address, - amount: transfer_amount, - fee_asset: asset.clone(), - memo: "{}".into(), - bridge_address: Some(bridge_address), - }; - - // not enough balance to transfer asset; should fail - state - .put_account_balance(bridge_address, &asset, transfer_amount) - .unwrap(); - assert!( - bridge_unlock - .check_stateful(&state, withdrawer_address) - .await - .unwrap_err() - .to_string() - .contains("insufficient funds for transfer and fee payment") - ); - - // enough balance; should pass - state - .put_account_balance(bridge_address, &asset, transfer_amount + transfer_fee) - .unwrap(); - bridge_unlock - .check_stateful(&state, withdrawer_address) - .await - .unwrap(); - } - #[tokio::test] async fn execute_from_none() { let storage = cnidarium::TempStorage::new().await.unwrap(); @@ -367,7 +252,7 @@ mod test { .unwrap(); assert!( bridge_unlock - .execute(&mut state, bridge_address) + .check_and_execute(&mut state) .await .unwrap_err() .to_string() @@ -378,10 +263,7 @@ mod test { state .put_account_balance(bridge_address, &asset, transfer_amount + transfer_fee) .unwrap(); - bridge_unlock - .execute(&mut state, bridge_address) - .await - .unwrap(); + bridge_unlock.check_and_execute(&mut state).await.unwrap(); } #[tokio::test] @@ -419,7 +301,7 @@ mod test { .unwrap(); assert!( bridge_unlock - .execute(&mut state, bridge_address) + .check_and_execute(&mut state) .await .unwrap_err() .to_string() @@ -430,9 +312,6 @@ mod test { state .put_account_balance(bridge_address, &asset, transfer_amount + transfer_fee) .unwrap(); - bridge_unlock - .execute(&mut state, bridge_address) - .await - .unwrap(); + bridge_unlock.check_and_execute(&mut state).await.unwrap(); } } diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 4c508ce6eb..9d7a2ce7d5 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -70,7 +70,7 @@ async fn ics20_withdrawal_check_stateful_bridge_account( // bridge, and check that the withdrawer address is the same as the transaction sender. let is_sender_bridge = state - .get_bridge_account_rollup_id(&from) + .get_bridge_account_rollup_id(from) .await .context("failed to get bridge account rollup id")? .is_some(); @@ -84,7 +84,7 @@ async fn ics20_withdrawal_check_stateful_bridge_account( let bridge_address = action.bridge_address.map_or(from, |addr| addr.bytes()); let Some(withdrawer) = state - .get_bridge_account_withdrawer_address(&bridge_address) + .get_bridge_account_withdrawer_address(bridge_address) .await .context("failed to get bridge withdrawer")? else { @@ -225,13 +225,13 @@ fn is_source(source_port: &PortId, source_channel: &ChannelId, asset: &Denom) -> #[cfg(test)] mod tests { - use address::StateWriteExt; use astria_core::primitive::v1::RollupId; use cnidarium::StateDelta; use ibc_types::core::client::Height; use super::*; use crate::{ + address::StateWriteExt as _, bridge::StateWriteExt as _, test_utils::{ astria_address, @@ -246,13 +246,13 @@ mod tests { let state = StateDelta::new(snapshot); let denom = "test".parse::().unwrap(); - let from = astria_address(&[1u8; 20]); + let from = [1u8; 20]; let action = action::Ics20Withdrawal { amount: 1, denom: denom.clone(), bridge_address: None, destination_chain_address: "test".to_string(), - return_address: from, + return_address: astria_address(&from), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), @@ -274,7 +274,7 @@ mod tests { state.put_base_prefix(ASTRIA_PREFIX).unwrap(); // sender is a bridge address, which is also the withdrawer, so it's ok - let bridge_address = astria_address(&[1u8; 20]); + let bridge_address = [1u8; 20]; state.put_bridge_account_rollup_id( &bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), @@ -287,7 +287,7 @@ mod tests { denom: denom.clone(), bridge_address: None, destination_chain_address: "test".to_string(), - return_address: bridge_address, + return_address: astria_address(&bridge_address), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), @@ -309,7 +309,7 @@ mod tests { state.put_base_prefix(ASTRIA_PREFIX).unwrap(); // withdraw is *not* the bridge address, Ics20Withdrawal must be sent by the withdrawer - let bridge_address = astria_address(&[1u8; 20]); + let bridge_address = [1u8; 20]; state.put_bridge_account_rollup_id( &bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), @@ -322,7 +322,7 @@ mod tests { denom: denom.clone(), bridge_address: None, destination_chain_address: "test".to_string(), - return_address: bridge_address, + return_address: astria_address(&bridge_address), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), @@ -348,8 +348,8 @@ mod tests { state.put_base_prefix(ASTRIA_PREFIX).unwrap(); // sender the withdrawer address, so it's ok - let bridge_address = astria_address(&[1u8; 20]); - let withdrawer_address = astria_address(&[2u8; 20]); + let bridge_address = [1u8; 20]; + let withdrawer_address = [2u8; 20]; state.put_bridge_account_rollup_id( &bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), @@ -360,9 +360,9 @@ mod tests { let action = action::Ics20Withdrawal { amount: 1, denom: denom.clone(), - bridge_address: Some(bridge_address), + bridge_address: Some(astria_address(&bridge_address)), destination_chain_address: "test".to_string(), - return_address: bridge_address, + return_address: astria_address(&bridge_address), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), @@ -384,7 +384,7 @@ mod tests { state.put_base_prefix(ASTRIA_PREFIX).unwrap(); // sender is not the withdrawer address, so must fail - let bridge_address = astria_address(&[1u8; 20]); + let bridge_address = [1u8; 20]; let withdrawer_address = astria_address(&[2u8; 20]); state.put_bridge_account_rollup_id( &bridge_address, @@ -396,9 +396,9 @@ mod tests { let action = action::Ics20Withdrawal { amount: 1, denom: denom.clone(), - bridge_address: Some(bridge_address), + bridge_address: Some(astria_address(&bridge_address)), destination_chain_address: "test".to_string(), - return_address: bridge_address, + return_address: astria_address(&bridge_address), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), @@ -423,15 +423,15 @@ mod tests { let state = StateDelta::new(snapshot); // sender is not the withdrawer address, so must fail - let not_bridge_address = astria_address(&[1u8; 20]); + let not_bridge_address = [1u8; 20]; let denom = "test".parse::().unwrap(); let action = action::Ics20Withdrawal { amount: 1, denom: denom.clone(), - bridge_address: Some(not_bridge_address), + bridge_address: Some(astria_address(¬_bridge_address)), destination_chain_address: "test".to_string(), - return_address: not_bridge_address, + return_address: astria_address(¬_bridge_address), timeout_height: Height::new(1, 1).unwrap(), timeout_time: 1, source_channel: "channel-0".to_string().parse().unwrap(), diff --git a/crates/astria-sequencer/src/ibc/state_ext.rs b/crates/astria-sequencer/src/ibc/state_ext.rs index fce3e7c701..9f5cb26077 100644 --- a/crates/astria-sequencer/src/ibc/state_ext.rs +++ b/crates/astria-sequencer/src/ibc/state_ext.rs @@ -5,7 +5,6 @@ use anyhow::{ }; use astria_core::primitive::v1::{ asset, - Address, ADDRESS_LEN, }; use async_trait::async_trait; @@ -148,23 +147,23 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - fn put_ibc_sudo_address(&mut self, address: Address) -> Result<()> { + fn put_ibc_sudo_address(&mut self, address: T) -> Result<()> { self.put_raw( IBC_SUDO_STORAGE_KEY.to_string(), - borsh::to_vec(&SudoAddress(address.bytes())) + borsh::to_vec(&SudoAddress(address.get_address_bytes())) .context("failed to convert sudo address to vec")?, ); Ok(()) } #[instrument(skip_all)] - fn put_ibc_relayer_address(&mut self, address: &Address) { - self.put_raw(ibc_relayer_key(address), vec![]); + fn put_ibc_relayer_address(&mut self, address: T) { + self.put_raw(ibc_relayer_key(&address), vec![]); } #[instrument(skip_all)] - fn delete_ibc_relayer_address(&mut self, address: &Address) { - self.delete(ibc_relayer_key(address)); + fn delete_ibc_relayer_address(&mut self, address: T) { + self.delete(ibc_relayer_key(&address)); } #[instrument(skip_all)] @@ -231,7 +230,7 @@ mod tests { state.put_base_prefix(ASTRIA_PREFIX).unwrap(); // can write new - let mut address = astria_address(&[42u8; 20]); + let mut address = [42u8; 20]; state .put_ibc_sudo_address(address) .expect("writing sudo address should not fail"); @@ -245,7 +244,7 @@ mod tests { ); // can rewrite with new value - address = astria_address(&[41u8; 20]); + address = [41u8; 20]; state .put_ibc_sudo_address(address) .expect("writing sudo address should not fail"); @@ -271,7 +270,7 @@ mod tests { let address = astria_address(&[42u8; 20]); assert!( !state - .is_ibc_relayer(&address) + .is_ibc_relayer(address) .await .expect("calls to properly formatted addresses should not fail"), "inputted address should've returned false" @@ -288,20 +287,20 @@ mod tests { // can write let address = astria_address(&[42u8; 20]); - state.put_ibc_relayer_address(&address); + state.put_ibc_relayer_address(address); assert!( state - .is_ibc_relayer(&address) + .is_ibc_relayer(address) .await .expect("a relayer address was written and must exist inside the database"), "stored relayer address could not be verified" ); // can delete - state.delete_ibc_relayer_address(&address); + state.delete_ibc_relayer_address(address); assert!( !state - .is_ibc_relayer(&address) + .is_ibc_relayer(address) .await .expect("calls on unset addresses should not fail"), "relayer address was not deleted as was intended" diff --git a/crates/astria-sequencer/src/transaction/checks.rs b/crates/astria-sequencer/src/transaction/checks.rs index 8bd8ce3a46..1d0f9965f3 100644 --- a/crates/astria-sequencer/src/transaction/checks.rs +++ b/crates/astria-sequencer/src/transaction/checks.rs @@ -295,10 +295,6 @@ fn bridge_unlock_update_fees( #[cfg(test)] mod tests { - use address::{ - StateReadExt, - StateWriteExt, - }; use astria_core::{ primitive::v1::{ asset::Denom, @@ -317,8 +313,9 @@ mod tests { use super::*; use crate::{ - accounts::{ - StateReadExt as _, + accounts::StateWriteExt as _, + address::{ + StateReadExt, StateWriteExt as _, }, app::test_utils::*, From 58dfdbbe3e43463e1dc48b0be0fc11cc9e558058 Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 1 Aug 2024 20:15:23 +0200 Subject: [PATCH 04/23] fix all tests --- .../astria-sequencer/src/accounts/action.rs | 157 +++++++++--------- .../astria-sequencer/src/authority/action.rs | 12 +- .../src/bridge/bridge_lock_action.rs | 92 +++------- .../src/bridge/bridge_sudo_change_action.rs | 51 +++--- .../src/bridge/bridge_unlock_action.rs | 121 ++++++++------ crates/astria-sequencer/src/test_utils.rs | 8 + .../astria-sequencer/src/transaction/mod.rs | 4 + .../src/transaction/state_ext.rs | 4 +- 8 files changed, 213 insertions(+), 236 deletions(-) diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index 56ac42cb6d..ca138ddd91 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -29,67 +29,6 @@ use crate::{ transaction::StateReadExt as _, }; -pub(crate) async fn transfer_check_stateful( - action: &TransferAction, - state: &S, - from: TAddress, -) -> Result<()> -where - S: StateRead, - TAddress: GetAddressBytes, -{ - ensure!( - state - .is_allowed_fee_asset(&action.fee_asset) - .await - .context("failed to check allowed fee assets in state")?, - "invalid fee asset", - ); - - let fee = state - .get_transfer_base_fee() - .await - .context("failed to get transfer base fee")?; - let transfer_asset = action.asset.clone(); - - let from_fee_balance = state - .get_account_balance(&from, &action.fee_asset) - .await - .context("failed getting `from` account balance for fee payment")?; - - // if fee asset is same as transfer asset, ensure accounts has enough funds - // to cover both the fee and the amount transferred - if action.fee_asset.to_ibc_prefixed() == transfer_asset.to_ibc_prefixed() { - let payment_amount = action - .amount - .checked_add(fee) - .context("transfer amount plus fee overflowed")?; - - ensure!( - from_fee_balance >= payment_amount, - "insufficient funds for transfer and fee payment" - ); - } else { - // otherwise, check the fee asset account has enough to cover the fees, - // and the transfer asset account has enough to cover the transfer - ensure!( - from_fee_balance >= fee, - "insufficient funds for fee payment" - ); - - let from_transfer_balance = state - .get_account_balance(from, transfer_asset) - .await - .context("failed to get account balance in transfer check")?; - ensure!( - from_transfer_balance >= action.amount, - "insufficient funds for transfer" - ); - } - - Ok(()) -} - #[async_trait::async_trait] impl ActionHandler for TransferAction { type CheckStatelessContext = (); @@ -103,32 +42,28 @@ impl ActionHandler for TransferAction { .get_current_source() .expect("transaction source must be present in state when executing an action") .address_bytes(); - check_and_execute_transfer(self, from, state).await?; + + ensure!( + state + .get_bridge_account_rollup_id(from) + .await + .context("failed to get bridge account rollup id")? + .is_none(), + "cannot transfer out of bridge account; BridgeUnlock must be used", + ); + + check_transfer(self, from, &state).await?; + execute_transfer(self, from, state).await?; + Ok(()) } } -pub(crate) async fn check_and_execute_transfer( +pub(crate) async fn execute_transfer( action: &TransferAction, from: [u8; ADDRESS_LEN], mut state: S, ) -> anyhow::Result<()> { - state.ensure_base_prefix(&action.to).await.context( - "failed ensuring that the destination address matches the permitted base prefix", - )?; - ensure!( - state - .get_bridge_account_rollup_id(from) - .await - .context("failed to get bridge account rollup id")? - .is_none(), - "cannot transfer out of bridge account; BridgeUnlock must be used", - ); - - transfer_check_stateful(action, &state, from) - .await - .context("stateful transfer check failed")?; - let fee = state .get_transfer_base_fee() .await @@ -175,3 +110,67 @@ pub(crate) async fn check_and_execute_transfer( } Ok(()) } + +pub(crate) async fn check_transfer( + action: &TransferAction, + from: TAddress, + state: &S, +) -> Result<()> +where + S: StateRead, + TAddress: GetAddressBytes, +{ + state.ensure_base_prefix(&action.to).await.context( + "failed ensuring that the destination address matches the permitted base prefix", + )?; + ensure!( + state + .is_allowed_fee_asset(&action.fee_asset) + .await + .context("failed to check allowed fee assets in state")?, + "invalid fee asset", + ); + + let fee = state + .get_transfer_base_fee() + .await + .context("failed to get transfer base fee")?; + let transfer_asset = action.asset.clone(); + + let from_fee_balance = state + .get_account_balance(&from, &action.fee_asset) + .await + .context("failed getting `from` account balance for fee payment")?; + + // if fee asset is same as transfer asset, ensure accounts has enough funds + // to cover both the fee and the amount transferred + if action.fee_asset.to_ibc_prefixed() == transfer_asset.to_ibc_prefixed() { + let payment_amount = action + .amount + .checked_add(fee) + .context("transfer amount plus fee overflowed")?; + + ensure!( + from_fee_balance >= payment_amount, + "insufficient funds for transfer and fee payment" + ); + } else { + // otherwise, check the fee asset account has enough to cover the fees, + // and the transfer asset account has enough to cover the transfer + ensure!( + from_fee_balance >= fee, + "insufficient funds for fee payment" + ); + + let from_transfer_balance = state + .get_account_balance(from, transfer_asset) + .await + .context("failed to get account balance in transfer check")?; + ensure!( + from_transfer_balance >= action.amount, + "insufficient funds for transfer" + ); + } + + Ok(()) +} diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index 76c342584c..d8431570d1 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -171,14 +171,24 @@ mod test { bridge::StateReadExt as _, ibc::StateReadExt as _, sequence::StateReadExt as _, + transaction::{ + StateWriteExt as _, + TransactionContext, + }, }; #[tokio::test] - async fn fee_change_action_execute() { + async fn fee_change_action_executes() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); let transfer_fee = 12; + + state.put_current_source(TransactionContext { + address_bytes: [1; 20], + }); + state.put_sudo_address([1; 20]).unwrap(); + state.put_transfer_base_fee(transfer_fee).unwrap(); let fee_change = FeeChangeAction { diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index ff64ce6505..840c0a4600 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -14,6 +14,10 @@ use cnidarium::StateWrite; use crate::{ accounts::{ + action::{ + check_transfer, + execute_transfer, + }, StateReadExt as _, StateWriteExt as _, }, @@ -92,9 +96,8 @@ impl ActionHandler for BridgeLockAction { fee_asset: self.fee_asset.clone(), }; - crate::accounts::action::check_and_execute_transfer(&transfer_action, from, &mut state) - .await - .context("failed to execute bridge lock action as transfer action")?; + check_transfer(&transfer_action, from, &state).await?; + execute_transfer(&transfer_action, from, &mut state).await?; let rollup_id = state .get_bridge_account_rollup_id(self.to) @@ -151,9 +154,14 @@ mod tests { address::StateWriteExt, assets::StateWriteExt as _, test_utils::{ + assert_anyhow_error, astria_address, ASTRIA_PREFIX, }, + transaction::{ + StateWriteExt as _, + TransactionContext, + }, }; fn test_asset() -> asset::Denom { @@ -161,70 +169,18 @@ mod tests { } #[tokio::test] - async fn bridge_lock_check_stateful_fee_calc() { + 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; - state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - state.put_transfer_base_fee(transfer_fee).unwrap(); - state.put_bridge_lock_byte_cost_multiplier(2); - - let bridge_address = astria_address(&[1; 20]); - let asset = test_asset(); - let bridge_lock = BridgeLockAction { - to: bridge_address, - asset: asset.clone(), - amount: 100, - fee_asset: asset.clone(), - destination_chain_address: "someaddress".to_string(), - }; - - let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); - state - .put_bridge_account_ibc_asset(&bridge_address, &asset) - .unwrap(); - state.put_allowed_fee_asset(&asset); - let from_address = astria_address(&[2; 20]); + state.put_current_source(TransactionContext { + address_bytes: from_address.bytes(), + }); + state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - // not enough balance; should fail - state - .put_account_balance(from_address, &asset, 100) - .unwrap(); - - assert!( - bridge_lock - .check_and_execute(&mut state) - .await - .unwrap_err() - .to_string() - .contains("insufficient funds for fee payment") - ); - - // enough balance; should pass - let expected_deposit_fee = transfer_fee - + get_deposit_byte_len(&Deposit::new( - bridge_address, - rollup_id, - 100, - asset.clone(), - "someaddress".to_string(), - )) * 2; - state - .put_account_balance(from_address, &asset, 100 + expected_deposit_fee) - .unwrap(); - bridge_lock.check_and_execute(&mut state).await.unwrap(); - } - - #[tokio::test] - async fn bridge_lock_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; state.put_transfer_base_fee(transfer_fee).unwrap(); state.put_bridge_lock_byte_cost_multiplier(2); @@ -239,25 +195,19 @@ mod tests { }; let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state.put_bridge_account_rollup_id(bridge_address, &rollup_id); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); state.put_allowed_fee_asset(&asset); - let from_address = astria_address(&[2; 20]); - // not enough balance; should fail state .put_account_balance(from_address, &asset, 100 + transfer_fee) .unwrap(); - assert!( - bridge_lock - .check_and_execute(&mut state) - .await - .unwrap_err() - .to_string() - .eq("failed to deduct fee from account balance") + assert_anyhow_error( + bridge_lock.check_and_execute(&mut state).await.unwrap_err(), + "insufficient funds for fee payment", ); // enough balance; should pass diff --git a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs index 459f0c7b71..cd835877c7 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -105,6 +105,10 @@ mod tests { astria_address, ASTRIA_PREFIX, }, + transaction::{ + StateWriteExt as _, + TransactionContext, + }, }; fn test_asset() -> asset::Denom { @@ -112,11 +116,14 @@ mod tests { } #[tokio::test] - async fn check_stateless_ok() { + async fn fails_with_unauthorized_if_signer_is_not_sudo_address() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + state.put_current_source(TransactionContext { + address_bytes: [1; 20], + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); let asset = test_asset(); @@ -124,32 +131,7 @@ mod tests { let bridge_address = astria_address(&[99; 20]); let sudo_address = astria_address(&[98; 20]); - state.put_bridge_account_sudo_address(&bridge_address, &sudo_address); - - let action = BridgeSudoChangeAction { - bridge_address, - new_sudo_address: None, - new_withdrawer_address: None, - fee_asset: asset.clone(), - }; - - action.check_and_execute(state).await.unwrap(); - } - - #[tokio::test] - async fn check_stateless_unauthorized() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - - let asset = test_asset(); - state.put_allowed_fee_asset(&asset); - - let bridge_address = astria_address(&[99; 20]); - let sudo_address = astria_address(&[98; 20]); - state.put_bridge_account_sudo_address(&bridge_address, &sudo_address); + state.put_bridge_account_sudo_address(bridge_address, sudo_address); let action = BridgeSudoChangeAction { bridge_address, @@ -169,16 +151,25 @@ mod tests { } #[tokio::test] - async fn execute_ok() { + async fn executes() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + let sudo_address = astria_address(&[98; 20]); + state.put_current_source(TransactionContext { + address_bytes: sudo_address.bytes(), + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); state.put_bridge_sudo_change_base_fee(10); let fee_asset = test_asset(); + state.put_allowed_fee_asset(&fee_asset); + let bridge_address = astria_address(&[99; 20]); + + state.put_bridge_account_sudo_address(bridge_address, sudo_address); + let new_sudo_address = astria_address(&[98; 20]); let new_withdrawer_address = astria_address(&[97; 20]); state @@ -196,14 +187,14 @@ mod tests { assert_eq!( state - .get_bridge_account_sudo_address(&bridge_address) + .get_bridge_account_sudo_address(bridge_address) .await .unwrap(), Some(new_sudo_address.bytes()), ); assert_eq!( state - .get_bridge_account_withdrawer_address(&bridge_address) + .get_bridge_account_withdrawer_address(bridge_address) .await .unwrap(), Some(new_withdrawer_address.bytes()), diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index ff72a7ddd3..f6571204c9 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -11,6 +11,10 @@ use astria_core::protocol::transaction::v1alpha1::action::{ use cnidarium::StateWrite; use crate::{ + accounts::action::{ + check_transfer, + execute_transfer, + }, address::StateReadExt as _, app::ActionHandler, bridge::StateReadExt as _, @@ -25,7 +29,7 @@ impl ActionHandler for BridgeUnlockAction { Ok(()) } - async fn check_and_execute(&self, mut state: S) -> Result<()> { + async fn check_and_execute(&self, state: S) -> Result<()> { let from = state .get_current_source() .expect("transaction source must be present in state when executing an action") @@ -45,15 +49,14 @@ impl ActionHandler for BridgeUnlockAction { // if unset, use the tx sender's address let bridge_address = self.bridge_address.map_or(from, |addr| addr.bytes()); - // grab the bridge account's asset let asset = state - .get_bridge_account_ibc_asset(&bridge_address) + .get_bridge_account_ibc_asset(bridge_address) .await .context("failed to get bridge's asset id, must be a bridge account")?; // check that the sender of this tx is the authorized withdrawer for the bridge account let Some(withdrawer_address) = state - .get_bridge_account_withdrawer_address(&bridge_address) + .get_bridge_account_withdrawer_address(bridge_address) .await .context("failed to get bridge account withdrawer address")? else { @@ -72,13 +75,8 @@ impl ActionHandler for BridgeUnlockAction { fee_asset: self.fee_asset.clone(), }; - crate::accounts::action::check_and_execute_transfer( - &transfer_action, - bridge_address, - &mut state, - ) - .await - .context("failed to execute bridge lock action as transfer action")?; + check_transfer(&transfer_action, bridge_address, &state).await?; + execute_transfer(&transfer_action, bridge_address, state).await?; Ok(()) } @@ -86,22 +84,30 @@ impl ActionHandler for BridgeUnlockAction { #[cfg(test)] mod test { - use astria_core::primitive::v1::{ - asset, - RollupId, + use astria_core::{ + primitive::v1::{ + asset, + RollupId, + }, + protocol::transaction::v1alpha1::action::BridgeUnlockAction, }; use cnidarium::StateDelta; - use super::*; use crate::{ accounts::StateWriteExt as _, address::StateWriteExt as _, + app::ActionHandler as _, assets::StateWriteExt as _, bridge::StateWriteExt as _, test_utils::{ + assert_anyhow_error, astria_address, ASTRIA_PREFIX, }, + transaction::{ + StateWriteExt as _, + TransactionContext, + }, }; fn test_asset() -> asset::Denom { @@ -109,11 +115,14 @@ mod test { } #[tokio::test] - async fn fail_non_bridge_accounts() { + async fn fails_if_bridge_address_is_not_set_and_signer_is_not_bridge() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + state.put_current_source(TransactionContext { + address_bytes: [1; 20], + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); let asset = test_asset(); @@ -141,23 +150,25 @@ mod test { } #[tokio::test] - async fn fail_withdrawer_unset_invalid_withdrawer() { + async fn fails_if_bridge_account_has_no_withdrawer_address() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + state.put_current_source(TransactionContext { + address_bytes: [1; 20], + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); let asset = test_asset(); let transfer_amount = 100; let to_address = astria_address(&[2; 20]); - let bridge_address = astria_address(&[3; 20]); + // state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); state .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); - state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); let bridge_unlock = BridgeUnlockAction { to: to_address, @@ -168,34 +179,32 @@ mod test { }; // invalid sender, doesn't match action's `from`, should fail - assert!( - bridge_unlock - .check_and_execute(state) - .await - .unwrap_err() - .to_string() - .contains("unauthorized to unlock bridge account") + assert_anyhow_error( + bridge_unlock.check_and_execute(state).await.unwrap_err(), + "bridge account does not have an associated withdrawer address", ); } #[tokio::test] - async fn fail_withdrawer_set_invalid_withdrawer() { + async fn fails_if_withdrawer_is_not_signer() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + state.put_current_source(TransactionContext { + address_bytes: [1; 20], + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); let asset = test_asset(); let transfer_amount = 100; let to_address = astria_address(&[2; 20]); - let bridge_address = astria_address(&[3; 20]); let withdrawer_address = astria_address(&[4; 20]); - state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + state.put_bridge_account_withdrawer_address(bridge_address, withdrawer_address); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); let bridge_unlock = BridgeUnlockAction { @@ -207,35 +216,37 @@ mod test { }; // invalid sender, doesn't match action's bridge account's withdrawer, should fail - assert!( - bridge_unlock - .check_and_execute(state) - .await - .unwrap_err() - .to_string() - .contains("unauthorized to unlock bridge account") + assert_anyhow_error( + bridge_unlock.check_and_execute(state).await.unwrap_err(), + "unauthorized to unlock bridge account", ); } #[tokio::test] - async fn execute_from_none() { + async fn execute_with_bridge_address_unset() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + let bridge_address = astria_address(&[1; 20]); + state.put_current_source(TransactionContext { + address_bytes: bridge_address.bytes(), + }); + state.put_base_prefix(ASTRIA_PREFIX).unwrap(); + let asset = test_asset(); let transfer_fee = 10; let transfer_amount = 100; state.put_transfer_base_fee(transfer_fee).unwrap(); - let bridge_address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state.put_bridge_account_rollup_id(bridge_address, &rollup_id); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); + state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); state.put_allowed_fee_asset(&asset); let bridge_unlock = BridgeUnlockAction { @@ -250,13 +261,12 @@ mod test { state .put_account_balance(bridge_address, &asset, transfer_amount) .unwrap(); - assert!( + assert_anyhow_error( bridge_unlock .check_and_execute(&mut state) .await - .unwrap_err() - .to_string() - .eq("failed to execute bridge unlock action as transfer action") + .unwrap_err(), + "insufficient funds for transfer and fee payment", ); // enough balance; should pass @@ -267,24 +277,30 @@ mod test { } #[tokio::test] - async fn execute_from_some() { + async fn execute_with_bridge_address_set() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); + let bridge_address = astria_address(&[1; 20]); + state.put_current_source(TransactionContext { + address_bytes: bridge_address.bytes(), + }); + state.put_base_prefix(ASTRIA_PREFIX).unwrap(); + let asset = test_asset(); let transfer_fee = 10; let transfer_amount = 100; state.put_transfer_base_fee(transfer_fee).unwrap(); - let bridge_address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state.put_bridge_account_rollup_id(bridge_address, &rollup_id); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); + state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); state.put_allowed_fee_asset(&asset); let bridge_unlock = BridgeUnlockAction { @@ -299,13 +315,12 @@ mod test { state .put_account_balance(bridge_address, &asset, transfer_amount) .unwrap(); - assert!( + assert_anyhow_error( bridge_unlock .check_and_execute(&mut state) .await - .unwrap_err() - .to_string() - .eq("failed to execute bridge unlock action as transfer action") + .unwrap_err(), + "insufficient funds for transfer and fee payment", ); // enough balance; should pass diff --git a/crates/astria-sequencer/src/test_utils.rs b/crates/astria-sequencer/src/test_utils.rs index 24a078e30f..e9ba4268de 100644 --- a/crates/astria-sequencer/src/test_utils.rs +++ b/crates/astria-sequencer/src/test_utils.rs @@ -38,3 +38,11 @@ pub(crate) fn verification_key(seed: u64) -> VerificationKey { let signing_key = SigningKey::new(rng); signing_key.verification_key() } + +#[track_caller] +pub(crate) fn assert_anyhow_error(error: anyhow::Error, expected: &'static str) { + let msg = error.to_string(); + if !msg.contains(expected) { + panic!("error contained different message\n\texpected: {expected}\n\tfull_error: {msg}"); + }; +} diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index fd50dc09a8..152eb9561b 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -19,6 +19,10 @@ pub(crate) use checks::{ check_nonce_mempool, }; use cnidarium::StateWrite; +// Conditional to quiet warnings. This object is used throughout the codebase, +// but is never explicitly named - hence Rust warns about it being unused. +#[cfg(test)] +pub(crate) use state_ext::TransactionContext; pub(crate) use state_ext::{ StateReadExt, StateWriteExt, diff --git a/crates/astria-sequencer/src/transaction/state_ext.rs b/crates/astria-sequencer/src/transaction/state_ext.rs index 9dd1de4614..9a4e44e5d7 100644 --- a/crates/astria-sequencer/src/transaction/state_ext.rs +++ b/crates/astria-sequencer/src/transaction/state_ext.rs @@ -13,7 +13,7 @@ fn current_source() -> &'static str { #[derive(Clone)] pub(crate) struct TransactionContext { - address_bytes: [u8; ADDRESS_LEN], + pub(crate) address_bytes: [u8; ADDRESS_LEN], } impl TransactionContext { @@ -31,7 +31,7 @@ impl From<&SignedTransaction> for TransactionContext { } pub(crate) trait StateWriteExt: StateWrite { - fn put_current_source(&mut self, transaction: &SignedTransaction) { + fn put_current_source(&mut self, transaction: impl Into) { let context: TransactionContext = transaction.into(); self.object_put(current_source(), context); } From a3bc48fb9e8770147aa122f77db48f4d692969f2 Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 1 Aug 2024 20:27:19 +0200 Subject: [PATCH 05/23] rename GetAddressBytes -> AddressBytes, remove unnecessary checks at startup --- .../astria-sequencer/src/accounts/action.rs | 4 +- crates/astria-sequencer/src/accounts/mod.rs | 32 ++++++------- .../src/accounts/state_ext.rs | 22 ++++----- .../src/authority/state_ext.rs | 6 +-- .../astria-sequencer/src/bridge/state_ext.rs | 48 ++++++++----------- .../src/ibc/ics20_withdrawal.rs | 4 +- crates/astria-sequencer/src/ibc/state_ext.rs | 18 +++---- crates/astria-sequencer/src/sequencer.rs | 17 ------- 8 files changed, 64 insertions(+), 87 deletions(-) diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index ca138ddd91..842283897c 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -13,7 +13,7 @@ use cnidarium::{ StateWrite, }; -use super::GetAddressBytes; +use super::AddressBytes; use crate::{ accounts::{ StateReadExt as _, @@ -118,7 +118,7 @@ pub(crate) async fn check_transfer( ) -> Result<()> where S: StateRead, - TAddress: GetAddressBytes, + TAddress: AddressBytes, { state.ensure_base_prefix(&action.to).await.context( "failed ensuring that the destination address matches the permitted base prefix", diff --git a/crates/astria-sequencer/src/accounts/mod.rs b/crates/astria-sequencer/src/accounts/mod.rs index 734492e0b2..ddb9fe68a9 100644 --- a/crates/astria-sequencer/src/accounts/mod.rs +++ b/crates/astria-sequencer/src/accounts/mod.rs @@ -19,45 +19,45 @@ pub(crate) use state_ext::{ StateWriteExt, }; -pub(crate) trait GetAddressBytes: Send + Sync { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN]; +pub(crate) trait AddressBytes: Send + Sync { + fn address_bytes(&self) -> [u8; ADDRESS_LEN]; } -impl GetAddressBytes for Address { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { +impl AddressBytes for Address { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { self.bytes() } } -impl GetAddressBytes for [u8; ADDRESS_LEN] { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { +impl AddressBytes for [u8; ADDRESS_LEN] { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { *self } } -impl GetAddressBytes for SignedTransaction { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { +impl AddressBytes for SignedTransaction { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { self.address_bytes() } } -impl GetAddressBytes for SigningKey { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { +impl AddressBytes for SigningKey { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { self.address_bytes() } } -impl GetAddressBytes for VerificationKey { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { +impl AddressBytes for VerificationKey { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { self.address_bytes() } } -impl<'a, T> GetAddressBytes for &'a T +impl<'a, T> AddressBytes for &'a T where - T: GetAddressBytes, + T: AddressBytes, { - fn get_address_bytes(&self) -> [u8; ADDRESS_LEN] { - (*self).get_address_bytes() + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + (*self).address_bytes() } } diff --git a/crates/astria-sequencer/src/accounts/state_ext.rs b/crates/astria-sequencer/src/accounts/state_ext.rs index f4f60a2931..e8a60f3d03 100644 --- a/crates/astria-sequencer/src/accounts/state_ext.rs +++ b/crates/astria-sequencer/src/accounts/state_ext.rs @@ -21,7 +21,7 @@ use cnidarium::{ use futures::StreamExt; use tracing::instrument; -use super::GetAddressBytes; +use super::AddressBytes; /// Newtype wrapper to read and write a u32 from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -39,18 +39,18 @@ const ACCOUNTS_PREFIX: &str = "accounts"; const TRANSFER_BASE_FEE_STORAGE_KEY: &str = "transferfee"; struct StorageKey<'a, T>(&'a T); -impl<'a, T: GetAddressBytes> std::fmt::Display for StorageKey<'a, T> { +impl<'a, T: AddressBytes> std::fmt::Display for StorageKey<'a, T> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(ACCOUNTS_PREFIX)?; f.write_str("/")?; - for byte in self.0.get_address_bytes() { + for byte in self.0.address_bytes() { f.write_fmt(format_args!("{byte:02x}"))?; } Ok(()) } } -fn balance_storage_key>( +fn balance_storage_key>( address: TAddress, asset: TAsset, ) -> String { @@ -61,7 +61,7 @@ fn balance_storage_key(address: T) -> String { +fn nonce_storage_key(address: T) -> String { format!("{}/nonce", StorageKey(&address)) } @@ -127,7 +127,7 @@ pub(crate) trait StateReadExt: StateRead + crate::assets::StateReadExt { asset: TAsset, ) -> Result where - TAddress: GetAddressBytes, + TAddress: AddressBytes, TAsset: Into + std::fmt::Display + Send, { let Some(bytes) = self @@ -142,7 +142,7 @@ pub(crate) trait StateReadExt: StateRead + crate::assets::StateReadExt { } #[instrument(skip_all)] - async fn get_account_nonce(&self, address: T) -> Result { + async fn get_account_nonce(&self, address: T) -> Result { let bytes = self .get_raw(&nonce_storage_key(address)) .await @@ -183,7 +183,7 @@ pub(crate) trait StateWriteExt: StateWrite { balance: u128, ) -> Result<()> where - TAddress: GetAddressBytes, + TAddress: AddressBytes, TAsset: Into + std::fmt::Display + Send, { let bytes = borsh::to_vec(&Balance(balance)).context("failed to serialize balance")?; @@ -192,7 +192,7 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - fn put_account_nonce(&mut self, address: T, nonce: u32) -> Result<()> { + fn put_account_nonce(&mut self, address: T, nonce: u32) -> Result<()> { let bytes = borsh::to_vec(&Nonce(nonce)).context("failed to serialize nonce")?; self.put_raw(nonce_storage_key(address), bytes); Ok(()) @@ -206,7 +206,7 @@ pub(crate) trait StateWriteExt: StateWrite { amount: u128, ) -> Result<()> where - TAddress: GetAddressBytes, + TAddress: AddressBytes, TAsset: Into + std::fmt::Display + Send, { let asset = asset.into(); @@ -233,7 +233,7 @@ pub(crate) trait StateWriteExt: StateWrite { amount: u128, ) -> Result<()> where - TAddress: GetAddressBytes, + TAddress: AddressBytes, TAsset: Into + std::fmt::Display + Send, { let asset = asset.into(); diff --git a/crates/astria-sequencer/src/authority/state_ext.rs b/crates/astria-sequencer/src/authority/state_ext.rs index 6c59913339..f98d18a78f 100644 --- a/crates/astria-sequencer/src/authority/state_ext.rs +++ b/crates/astria-sequencer/src/authority/state_ext.rs @@ -18,7 +18,7 @@ use cnidarium::{ use tracing::instrument; use super::ValidatorSet; -use crate::accounts::GetAddressBytes; +use crate::accounts::AddressBytes; /// Newtype wrapper to read and write an address from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -83,10 +83,10 @@ impl StateReadExt for T {} #[async_trait] pub(crate) trait StateWriteExt: StateWrite { #[instrument(skip_all)] - fn put_sudo_address(&mut self, address: T) -> Result<()> { + fn put_sudo_address(&mut self, address: T) -> Result<()> { self.put_raw( SUDO_STORAGE_KEY.to_string(), - borsh::to_vec(&SudoAddress(address.get_address_bytes())) + borsh::to_vec(&SudoAddress(address.address_bytes())) .context("failed to convert sudo address to vec")?, ); Ok(()) diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index ceeea2943b..a9852436d9 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -36,7 +36,7 @@ use tracing::{ }; use crate::{ - accounts::GetAddressBytes, + accounts::AddressBytes, address, }; @@ -71,19 +71,19 @@ struct BridgeAccountKey<'a, T> { impl<'a, T> std::fmt::Display for BridgeAccountKey<'a, T> where - T: GetAddressBytes, + T: AddressBytes, { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(self.prefix)?; f.write_str("/")?; - for byte in self.address.get_address_bytes() { + for byte in self.address.address_bytes() { f.write_fmt(format_args!("{byte:02x}"))?; } Ok(()) } } -fn rollup_id_storage_key(address: &T) -> String { +fn rollup_id_storage_key(address: &T) -> String { format!( "{}/rollupid", BridgeAccountKey { @@ -93,7 +93,7 @@ fn rollup_id_storage_key(address: &T) -> String { ) } -fn asset_id_storage_key(address: &T) -> String { +fn asset_id_storage_key(address: &T) -> String { format!( "{}/assetid", BridgeAccountKey { @@ -115,7 +115,7 @@ fn deposit_nonce_storage_key(rollup_id: &RollupId) -> Vec { format!("depositnonce/{}", rollup_id.encode_hex::()).into() } -fn bridge_account_sudo_address_storage_key(address: &T) -> String { +fn bridge_account_sudo_address_storage_key(address: &T) -> String { format!( "{}", BridgeAccountKey { @@ -125,7 +125,7 @@ fn bridge_account_sudo_address_storage_key(address: &T) -> S ) } -fn bridge_account_withdrawer_address_storage_key(address: &T) -> String { +fn bridge_account_withdrawer_address_storage_key(address: &T) -> String { format!( "{}", BridgeAccountKey { @@ -135,9 +135,7 @@ fn bridge_account_withdrawer_address_storage_key(address: &T ) } -fn last_transaction_hash_for_bridge_account_storage_key( - address: &T, -) -> Vec { +fn last_transaction_hash_for_bridge_account_storage_key(address: &T) -> Vec { format!( "{}/lasttx", BridgeAccountKey { @@ -152,7 +150,7 @@ fn last_transaction_hash_for_bridge_account_storage_key( #[async_trait] pub(crate) trait StateReadExt: StateRead + address::StateReadExt { #[instrument(skip_all)] - async fn get_bridge_account_rollup_id( + async fn get_bridge_account_rollup_id( &self, address: T, ) -> Result> { @@ -171,7 +169,7 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { } #[instrument(skip_all)] - async fn get_bridge_account_ibc_asset( + async fn get_bridge_account_ibc_asset( &self, address: T, ) -> Result { @@ -186,7 +184,7 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { } #[instrument(skip_all)] - async fn get_bridge_account_sudo_address( + async fn get_bridge_account_sudo_address( &self, bridge_address: T, ) -> Result> { @@ -208,7 +206,7 @@ pub(crate) trait StateReadExt: StateRead + address::StateReadExt { } #[instrument(skip_all)] - async fn get_bridge_account_withdrawer_address( + async fn get_bridge_account_withdrawer_address( &self, bridge_address: T, ) -> Result> { @@ -363,11 +361,7 @@ impl StateReadExt for T {} #[async_trait] pub(crate) trait StateWriteExt: StateWrite { #[instrument(skip_all)] - fn put_bridge_account_rollup_id( - &mut self, - address: T, - rollup_id: &RollupId, - ) { + fn put_bridge_account_rollup_id(&mut self, address: T, rollup_id: &RollupId) { self.put_raw(rollup_id_storage_key(&address), rollup_id.to_vec()); } @@ -378,7 +372,7 @@ pub(crate) trait StateWriteExt: StateWrite { asset: TAsset, ) -> Result<()> where - TAddress: GetAddressBytes, + TAddress: AddressBytes, TAsset: Into + std::fmt::Display, { let ibc = asset.into(); @@ -395,12 +389,12 @@ pub(crate) trait StateWriteExt: StateWrite { bridge_address: TBridgeAddress, sudo_address: TSudoAddress, ) where - TBridgeAddress: GetAddressBytes, - TSudoAddress: GetAddressBytes, + TBridgeAddress: AddressBytes, + TSudoAddress: AddressBytes, { self.put_raw( bridge_account_sudo_address_storage_key(&bridge_address), - sudo_address.get_address_bytes().to_vec(), + sudo_address.address_bytes().to_vec(), ); } @@ -410,12 +404,12 @@ pub(crate) trait StateWriteExt: StateWrite { bridge_address: TBridgeAddress, withdrawer_address: TWithdrawerAddress, ) where - TBridgeAddress: GetAddressBytes, - TWithdrawerAddress: GetAddressBytes, + TBridgeAddress: AddressBytes, + TWithdrawerAddress: AddressBytes, { self.put_raw( bridge_account_withdrawer_address_storage_key(&bridge_address), - withdrawer_address.get_address_bytes().to_vec(), + withdrawer_address.address_bytes().to_vec(), ); } @@ -492,7 +486,7 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - fn put_last_transaction_hash_for_bridge_account( + fn put_last_transaction_hash_for_bridge_account( &mut self, address: T, tx_hash: &[u8; 32], diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 9d7a2ce7d5..d08a367c44 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -26,7 +26,7 @@ use penumbra_ibc::component::packet::{ use crate::{ accounts::{ - GetAddressBytes, + AddressBytes, StateReadExt as _, StateWriteExt as _, }, @@ -92,7 +92,7 @@ async fn ics20_withdrawal_check_stateful_bridge_account( }; ensure!( - withdrawer == from.get_address_bytes(), + withdrawer == from.address_bytes(), "sender does not match bridge withdrawer address; unauthorized" ); diff --git a/crates/astria-sequencer/src/ibc/state_ext.rs b/crates/astria-sequencer/src/ibc/state_ext.rs index 9f5cb26077..76427753b5 100644 --- a/crates/astria-sequencer/src/ibc/state_ext.rs +++ b/crates/astria-sequencer/src/ibc/state_ext.rs @@ -22,7 +22,7 @@ use tracing::{ instrument, }; -use crate::accounts::GetAddressBytes; +use crate::accounts::AddressBytes; /// Newtype wrapper to read and write a u128 from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -41,11 +41,11 @@ const ICS20_WITHDRAWAL_BASE_FEE_STORAGE_KEY: &str = "ics20withdrawalfee"; struct IbcRelayerKey<'a, T>(&'a T); -impl<'a, T: GetAddressBytes> std::fmt::Display for IbcRelayerKey<'a, T> { +impl<'a, T: AddressBytes> std::fmt::Display for IbcRelayerKey<'a, T> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str("ibc-relayer")?; f.write_str("/")?; - for byte in self.0.get_address_bytes() { + for byte in self.0.address_bytes() { f.write_fmt(format_args!("{byte:02x}"))?; } Ok(()) @@ -62,7 +62,7 @@ fn channel_balance_storage_key>( ) } -fn ibc_relayer_key(address: &T) -> String { +fn ibc_relayer_key(address: &T) -> String { IbcRelayerKey(address).to_string() } @@ -105,7 +105,7 @@ pub(crate) trait StateReadExt: StateRead { } #[instrument(skip_all)] - async fn is_ibc_relayer(&self, address: T) -> Result { + async fn is_ibc_relayer(&self, address: T) -> Result { Ok(self .get_raw(&ibc_relayer_key(&address)) .await @@ -147,22 +147,22 @@ pub(crate) trait StateWriteExt: StateWrite { } #[instrument(skip_all)] - fn put_ibc_sudo_address(&mut self, address: T) -> Result<()> { + fn put_ibc_sudo_address(&mut self, address: T) -> Result<()> { self.put_raw( IBC_SUDO_STORAGE_KEY.to_string(), - borsh::to_vec(&SudoAddress(address.get_address_bytes())) + borsh::to_vec(&SudoAddress(address.address_bytes())) .context("failed to convert sudo address to vec")?, ); Ok(()) } #[instrument(skip_all)] - fn put_ibc_relayer_address(&mut self, address: T) { + fn put_ibc_relayer_address(&mut self, address: T) { self.put_raw(ibc_relayer_key(&address), vec![]); } #[instrument(skip_all)] - fn delete_ibc_relayer_address(&mut self, address: T) { + fn delete_ibc_relayer_address(&mut self, address: T) { self.delete(ibc_relayer_key(&address)); } diff --git a/crates/astria-sequencer/src/sequencer.rs b/crates/astria-sequencer/src/sequencer.rs index 1e7ef52bc4..3468832bba 100644 --- a/crates/astria-sequencer/src/sequencer.rs +++ b/crates/astria-sequencer/src/sequencer.rs @@ -31,9 +31,7 @@ use tracing::{ }; use crate::{ - address::StateReadExt as _, app::App, - assets::StateReadExt as _, config::Config, grpc::sequencer::SequencerServer, ibc::host_interface::AstriaHost, @@ -84,21 +82,6 @@ impl Sequencer { .context("failed to load storage backing chain state")?; let snapshot = storage.latest_snapshot(); - // the native asset should be configurable only at genesis. - // the genesis state must include the native asset's base - // denomination, and it is set in storage during init_chain. - // on subsequent startups, we load the native asset from storage. - if storage.latest_version() != u64::MAX { - let _ = snapshot - .get_native_asset() - .await - .context("failed to query state for native asset")?; - let _ = snapshot - .get_base_prefix() - .await - .context("failed to query state for base prefix")?; - } - let mempool = Mempool::new(); let app = App::new(snapshot, mempool.clone(), metrics) .await From ac31db5909cadbdead2fea477e155a479e61fe1d Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 1 Aug 2024 22:35:29 +0200 Subject: [PATCH 06/23] clippy --- crates/astria-sequencer/src/app/tests_app.rs | 8 ++-- .../src/app/tests_breaking_changes.rs | 4 +- .../src/app/tests_execute_transaction.rs | 18 ++++----- .../src/bridge/bridge_lock_action.rs | 2 +- .../src/bridge/bridge_unlock_action.rs | 19 +++++---- .../src/bridge/init_bridge_account_action.rs | 25 +++++++----- crates/astria-sequencer/src/bridge/query.rs | 19 +++++---- .../astria-sequencer/src/bridge/state_ext.rs | 32 +++++++-------- .../src/ibc/ics20_transfer.rs | 26 ++++++------ .../src/ibc/ics20_withdrawal.rs | 23 ++++++----- crates/astria-sequencer/src/ibc/state_ext.rs | 10 ++--- crates/astria-sequencer/src/test_utils.rs | 9 +++-- .../src/transaction/checks.rs | 2 +- .../astria-sequencer/src/transaction/mod.rs | 40 +++++++++++-------- .../src/transaction/state_ext.rs | 2 +- 15 files changed, 130 insertions(+), 109 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_app.rs b/crates/astria-sequencer/src/app/tests_app.rs index 43c15058b2..9389ca19e5 100644 --- a/crates/astria-sequencer/src/app/tests_app.rs +++ b/crates/astria-sequencer/src/app/tests_app.rs @@ -292,9 +292,9 @@ async fn app_create_sequencer_block_with_sequenced_data_and_deposits() { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, nria()) + .put_bridge_account_ibc_asset(bridge_address, nria()) .unwrap(); app.apply(state_tx); app.prepare_commit(storage.clone()).await.unwrap(); @@ -382,9 +382,9 @@ async fn app_execution_results_match_proposal_vs_after_proposal() { let asset = nria().clone(); let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); app.apply(state_tx); app.prepare_commit(storage.clone()).await.unwrap(); diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index b7c11a8bf1..a65eafc25a 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -104,9 +104,9 @@ async fn app_finalize_block_snapshot() { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, nria()) + .put_bridge_account_ibc_asset(bridge_address, nria()) .unwrap(); app.apply(state_tx); diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 635c9bbac4..a41a60f82f 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -359,7 +359,7 @@ async fn app_execute_transaction_ibc_relayer_change_addition() { let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); - assert!(app.state.is_ibc_relayer(&alice_address).await.unwrap()); + assert!(app.state.is_ibc_relayer(alice_address).await.unwrap()); } #[tokio::test] @@ -386,7 +386,7 @@ async fn app_execute_transaction_ibc_relayer_change_deletion() { let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); - assert!(!app.state.is_ibc_relayer(&alice_address).await.unwrap()); + assert!(!app.state.is_ibc_relayer(alice_address).await.unwrap()); } #[tokio::test] @@ -602,7 +602,7 @@ async fn app_execute_transaction_init_bridge_account_ok() { assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); assert_eq!( app.state - .get_bridge_account_rollup_id(&alice_address) + .get_bridge_account_rollup_id(alice_address) .await .unwrap() .unwrap(), @@ -610,7 +610,7 @@ async fn app_execute_transaction_init_bridge_account_ok() { ); assert_eq!( app.state - .get_bridge_account_ibc_asset(&alice_address) + .get_bridge_account_ibc_asset(alice_address) .await .unwrap(), nria().to_ibc_prefixed(), @@ -680,9 +680,9 @@ async fn app_execute_transaction_bridge_lock_action_ok() { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, nria()) + .put_bridge_account_ibc_asset(bridge_address, nria()) .unwrap(); app.apply(state_tx); @@ -993,11 +993,11 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { .unwrap(); // create bridge account - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, nria()) + .put_bridge_account_ibc_asset(bridge_address, nria()) .unwrap(); - state_tx.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); + state_tx.put_bridge_account_withdrawer_address(bridge_address, bridge_address); app.apply(state_tx); let amount = 100; diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 840c0a4600..2bc87e1300 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -206,7 +206,7 @@ mod tests { .put_account_balance(from_address, &asset, 100 + transfer_fee) .unwrap(); assert_anyhow_error( - bridge_lock.check_and_execute(&mut state).await.unwrap_err(), + &bridge_lock.check_and_execute(&mut state).await.unwrap_err(), "insufficient funds for fee payment", ); diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index f6571204c9..e5d957cba1 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -4,9 +4,12 @@ use anyhow::{ Context as _, Result, }; -use astria_core::protocol::transaction::v1alpha1::action::{ - BridgeUnlockAction, - TransferAction, +use astria_core::{ + primitive::v1::Address, + protocol::transaction::v1alpha1::action::{ + BridgeUnlockAction, + TransferAction, + }, }; use cnidarium::StateWrite; @@ -47,7 +50,7 @@ impl ActionHandler for BridgeUnlockAction { // the bridge address to withdraw funds from // if unset, use the tx sender's address - let bridge_address = self.bridge_address.map_or(from, |addr| addr.bytes()); + let bridge_address = self.bridge_address.map_or(from, Address::bytes); let asset = state .get_bridge_account_ibc_asset(bridge_address) @@ -180,7 +183,7 @@ mod test { // invalid sender, doesn't match action's `from`, should fail assert_anyhow_error( - bridge_unlock.check_and_execute(state).await.unwrap_err(), + &bridge_unlock.check_and_execute(state).await.unwrap_err(), "bridge account does not have an associated withdrawer address", ); } @@ -217,7 +220,7 @@ mod test { // invalid sender, doesn't match action's bridge account's withdrawer, should fail assert_anyhow_error( - bridge_unlock.check_and_execute(state).await.unwrap_err(), + &bridge_unlock.check_and_execute(state).await.unwrap_err(), "unauthorized to unlock bridge account", ); } @@ -262,7 +265,7 @@ mod test { .put_account_balance(bridge_address, &asset, transfer_amount) .unwrap(); assert_anyhow_error( - bridge_unlock + &bridge_unlock .check_and_execute(&mut state) .await .unwrap_err(), @@ -316,7 +319,7 @@ mod test { .put_account_balance(bridge_address, &asset, transfer_amount) .unwrap(); assert_anyhow_error( - bridge_unlock + &bridge_unlock .check_and_execute(&mut state) .await .unwrap_err(), 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 3431ea4019..54b7122444 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -4,7 +4,10 @@ use anyhow::{ Context as _, Result, }; -use astria_core::protocol::transaction::v1alpha1::action::InitBridgeAccountAction; +use astria_core::{ + primitive::v1::Address, + protocol::transaction::v1alpha1::action::InitBridgeAccountAction, +}; use cnidarium::StateWrite; use crate::{ @@ -69,7 +72,12 @@ impl ActionHandler for InitBridgeAccountAction { // // after the account becomes a bridge account, it can no longer receive funds // via `TransferAction`, only via `BridgeLockAction`. - if state.get_bridge_account_rollup_id(&from).await?.is_some() { + if state + .get_bridge_account_rollup_id(from) + .await + .context("failed getting rollup ID of bridge account")? + .is_some() + { bail!("bridge account already exists"); } @@ -83,17 +91,14 @@ impl ActionHandler for InitBridgeAccountAction { "insufficient funds for bridge account initialization", ); - state.put_bridge_account_rollup_id(&from, &self.rollup_id); + state.put_bridge_account_rollup_id(from, &self.rollup_id); state - .put_bridge_account_ibc_asset(&from, &self.asset) + .put_bridge_account_ibc_asset(from, &self.asset) .context("failed to put asset ID")?; - state.put_bridge_account_sudo_address( - &from, - &self.sudo_address.map_or(from, |addr| addr.bytes()), - ); + 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, |addr| addr.bytes()), + from, + self.withdrawer_address.map_or(from, Address::bytes), ); state diff --git a/crates/astria-sequencer/src/bridge/query.rs b/crates/astria-sequencer/src/bridge/query.rs index 9ca0bee321..e0eb0e247b 100644 --- a/crates/astria-sequencer/src/bridge/query.rs +++ b/crates/astria-sequencer/src/bridge/query.rs @@ -37,11 +37,14 @@ fn error_query_response( } } +// allow / FIXME: there is a lot of code duplication due to `error_query_response`. +// this could be significantly shortened. +#[allow(clippy::too_many_lines)] async fn get_bridge_account_info( snapshot: cnidarium::Snapshot, address: Address, ) -> anyhow::Result, response::Query> { - let rollup_id = match snapshot.get_bridge_account_rollup_id(&address).await { + let rollup_id = match snapshot.get_bridge_account_rollup_id(address).await { Ok(Some(rollup_id)) => rollup_id, Ok(None) => { return Ok(None); @@ -55,7 +58,7 @@ async fn get_bridge_account_info( } }; - let ibc_asset = match snapshot.get_bridge_account_ibc_asset(&address).await { + let ibc_asset = match snapshot.get_bridge_account_ibc_asset(address).await { Ok(asset) => asset, Err(err) => { return Err(error_query_response( @@ -84,7 +87,7 @@ async fn get_bridge_account_info( } }; - let sudo_address_bytes = match snapshot.get_bridge_account_sudo_address(&address).await { + let sudo_address_bytes = match snapshot.get_bridge_account_sudo_address(address).await { Ok(Some(bytes)) => bytes, Ok(None) => { return Err(error_query_response( @@ -115,7 +118,7 @@ async fn get_bridge_account_info( }; let withdrawer_address_bytes = match snapshot - .get_bridge_account_withdrawer_address(&address) + .get_bridge_account_withdrawer_address(address) .await { Ok(Some(withdrawer_address)) => withdrawer_address, @@ -318,15 +321,15 @@ mod test { let sudo_address = astria_address(&[1u8; 20]); let withdrawer_address = astria_address(&[2u8; 20]); state.put_block_height(1); - state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state.put_bridge_account_rollup_id(bridge_address, &rollup_id); state .put_ibc_asset(asset.as_trace_prefixed().unwrap()) .unwrap(); state - .put_bridge_account_ibc_asset(&bridge_address, &asset) + .put_bridge_account_ibc_asset(bridge_address, &asset) .unwrap(); - state.put_bridge_account_sudo_address(&bridge_address, &sudo_address); - state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + state.put_bridge_account_sudo_address(bridge_address, sudo_address); + state.put_bridge_account_withdrawer_address(bridge_address, withdrawer_address); storage.commit(state).await.unwrap(); let query = request::Query { diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index a9852436d9..0316603c09 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -541,7 +541,7 @@ mod test { // uninitialized ok assert_eq!( - state.get_bridge_account_rollup_id(&address).await.expect( + state.get_bridge_account_rollup_id(address).await.expect( "call to get bridge account rollup id should not fail for uninitialized addresses" ), Option::None, @@ -559,10 +559,10 @@ mod test { let address = astria_address(&[42u8; 20]); // can write new - state.put_bridge_account_rollup_id(&address, &rollup_id); + state.put_bridge_account_rollup_id(address, &rollup_id); assert_eq!( state - .get_bridge_account_rollup_id(&address) + .get_bridge_account_rollup_id(address) .await .expect("a rollup ID was written and must exist inside the database") .expect("expecting return value"), @@ -572,10 +572,10 @@ mod test { // can rewrite with new value rollup_id = RollupId::new([2u8; 32]); - state.put_bridge_account_rollup_id(&address, &rollup_id); + state.put_bridge_account_rollup_id(address, &rollup_id); assert_eq!( state - .get_bridge_account_rollup_id(&address) + .get_bridge_account_rollup_id(address) .await .expect("a rollup ID was written and must exist inside the database") .expect("expecting return value"), @@ -586,10 +586,10 @@ mod test { // can write additional account and both valid let rollup_id_1 = RollupId::new([2u8; 32]); let address_1 = astria_address(&[41u8; 20]); - state.put_bridge_account_rollup_id(&address_1, &rollup_id_1); + state.put_bridge_account_rollup_id(address_1, &rollup_id_1); assert_eq!( state - .get_bridge_account_rollup_id(&address_1) + .get_bridge_account_rollup_id(address_1) .await .expect("a rollup ID was written and must exist inside the database") .expect("expecting return value"), @@ -599,7 +599,7 @@ mod test { assert_eq!( state - .get_bridge_account_rollup_id(&address) + .get_bridge_account_rollup_id(address) .await .expect("a rollup ID was written and must exist inside the database") .expect("expecting return value"), @@ -616,7 +616,7 @@ mod test { let address = astria_address(&[42u8; 20]); state - .get_bridge_account_ibc_asset(&address) + .get_bridge_account_ibc_asset(address) .await .expect_err("call to get bridge account asset ids should fail if no assets"); } @@ -632,10 +632,10 @@ mod test { // can write state - .put_bridge_account_ibc_asset(&address, &asset) + .put_bridge_account_ibc_asset(address, &asset) .expect("storing bridge account asset should not fail"); let mut result = state - .get_bridge_account_ibc_asset(&address) + .get_bridge_account_ibc_asset(address) .await .expect("bridge asset id was written and must exist inside the database"); assert_eq!( @@ -647,10 +647,10 @@ mod test { // can update asset = "asset_2".parse::().unwrap(); state - .put_bridge_account_ibc_asset(&address, &asset) + .put_bridge_account_ibc_asset(address, &asset) .expect("storing bridge account assets should not fail"); result = state - .get_bridge_account_ibc_asset(&address) + .get_bridge_account_ibc_asset(address) .await .expect("bridge asset id was written and must exist inside the database"); assert_eq!( @@ -663,18 +663,18 @@ mod test { let address_1 = astria_address(&[41u8; 20]); let asset_1 = asset_1(); state - .put_bridge_account_ibc_asset(&address_1, &asset_1) + .put_bridge_account_ibc_asset(address_1, &asset_1) .expect("storing bridge account assets should not fail"); assert_eq!( state - .get_bridge_account_ibc_asset(&address_1) + .get_bridge_account_ibc_asset(address_1) .await .expect("bridge asset id was written and must exist inside the database"), asset_1.into(), "second bridge account asset not what was expected" ); result = state - .get_bridge_account_ibc_asset(&address) + .get_bridge_account_ibc_asset(address) .await .expect("original bridge asset id was written and must exist inside the database"); assert_eq!( diff --git a/crates/astria-sequencer/src/ibc/ics20_transfer.rs b/crates/astria-sequencer/src/ibc/ics20_transfer.rs index 1b2dac46f8..3ae8eacfd5 100644 --- a/crates/astria-sequencer/src/ibc/ics20_transfer.rs +++ b/crates/astria-sequencer/src/ibc/ics20_transfer.rs @@ -556,7 +556,7 @@ async fn execute_ics20_transfer_bridge_lock( // check if the recipient is a bridge account; if so, // ensure that the packet memo field (`destination_address`) is set. let is_bridge_lock = state - .get_bridge_account_rollup_id(&recipient) + .get_bridge_account_rollup_id(recipient) .await .context("failed to get bridge account rollup ID from state")? .is_some(); @@ -612,7 +612,7 @@ async fn execute_deposit( // ensure that the asset ID being transferred // to it is allowed. let Some(rollup_id) = state - .get_bridge_account_rollup_id(&bridge_address) + .get_bridge_account_rollup_id(bridge_address) .await .context("failed to get bridge account rollup ID from state")? else { @@ -620,7 +620,7 @@ async fn execute_deposit( }; let allowed_asset = state - .get_bridge_account_ibc_asset(&bridge_address) + .get_bridge_account_ibc_asset(bridge_address) .await .context("failed to get bridge account asset ID")?; ensure!( @@ -765,9 +765,9 @@ mod test { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let denom = "dest_port/dest_channel/nootasset".parse::().unwrap(); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &denom) + .put_bridge_account_ibc_asset(bridge_address, &denom) .unwrap(); let memo = memos::v1alpha1::Ics20TransferDeposit { @@ -822,9 +822,9 @@ mod test { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let denom = "dest_port/dest_channel/nootasset".parse::().unwrap(); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &denom) + .put_bridge_account_ibc_asset(bridge_address, &denom) .unwrap(); // use invalid memo, which should fail @@ -860,9 +860,9 @@ mod test { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let denom = "dest_port/dest_channel/nootasset".parse::().unwrap(); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &denom) + .put_bridge_account_ibc_asset(bridge_address, &denom) .unwrap(); // use invalid asset, which should fail @@ -1000,9 +1000,9 @@ mod test { .parse::() .unwrap(); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &denom) + .put_bridge_account_ibc_asset(bridge_address, &denom) .unwrap(); let amount = 100; @@ -1041,9 +1041,9 @@ mod test { let denom = "nootasset".parse::().unwrap(); let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx - .put_bridge_account_ibc_asset(&bridge_address, &denom) + .put_bridge_account_ibc_asset(bridge_address, &denom) .unwrap(); let packet = FungibleTokenPacketData { diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index d08a367c44..34ca3fee80 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -6,7 +6,10 @@ use anyhow::{ Result, }; use astria_core::{ - primitive::v1::asset::Denom, + primitive::v1::{ + asset::Denom, + Address, + }, protocol::transaction::v1alpha1::action, }; use cnidarium::{ @@ -81,7 +84,7 @@ async fn ics20_withdrawal_check_stateful_bridge_account( // if `action.bridge_address` is Some, but it's not a valid bridge account, // the `get_bridge_account_withdrawer_address` step will fail. - let bridge_address = action.bridge_address.map_or(from, |addr| addr.bytes()); + let bridge_address = action.bridge_address.map_or(from, Address::bytes); let Some(withdrawer) = state .get_bridge_account_withdrawer_address(bridge_address) @@ -276,10 +279,10 @@ mod tests { // sender is a bridge address, which is also the withdrawer, so it's ok let bridge_address = [1u8; 20]; state.put_bridge_account_rollup_id( - &bridge_address, + bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), ); - state.put_bridge_account_withdrawer_address(&bridge_address, &bridge_address); + state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); let denom = "test".parse::().unwrap(); let action = action::Ics20Withdrawal { @@ -311,10 +314,10 @@ mod tests { // withdraw is *not* the bridge address, Ics20Withdrawal must be sent by the withdrawer let bridge_address = [1u8; 20]; state.put_bridge_account_rollup_id( - &bridge_address, + bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), ); - state.put_bridge_account_withdrawer_address(&bridge_address, &astria_address(&[2u8; 20])); + state.put_bridge_account_withdrawer_address(bridge_address, astria_address(&[2u8; 20])); let denom = "test".parse::().unwrap(); let action = action::Ics20Withdrawal { @@ -351,10 +354,10 @@ mod tests { let bridge_address = [1u8; 20]; let withdrawer_address = [2u8; 20]; state.put_bridge_account_rollup_id( - &bridge_address, + bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), ); - state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + state.put_bridge_account_withdrawer_address(bridge_address, withdrawer_address); let denom = "test".parse::().unwrap(); let action = action::Ics20Withdrawal { @@ -387,10 +390,10 @@ mod tests { let bridge_address = [1u8; 20]; let withdrawer_address = astria_address(&[2u8; 20]); state.put_bridge_account_rollup_id( - &bridge_address, + bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), ); - state.put_bridge_account_withdrawer_address(&bridge_address, &withdrawer_address); + state.put_bridge_account_withdrawer_address(bridge_address, withdrawer_address); let denom = "test".parse::().unwrap(); let action = action::Ics20Withdrawal { diff --git a/crates/astria-sequencer/src/ibc/state_ext.rs b/crates/astria-sequencer/src/ibc/state_ext.rs index 76427753b5..880f86f6ba 100644 --- a/crates/astria-sequencer/src/ibc/state_ext.rs +++ b/crates/astria-sequencer/src/ibc/state_ext.rs @@ -317,10 +317,10 @@ mod tests { // can write let address = astria_address(&[42u8; 20]); - state.put_ibc_relayer_address(&address); + state.put_ibc_relayer_address(address); assert!( state - .is_ibc_relayer(&address) + .is_ibc_relayer(address) .await .expect("a relayer address was written and must exist inside the database"), "stored relayer address could not be verified" @@ -328,17 +328,17 @@ mod tests { // can write multiple let address_1 = astria_address(&[41u8; 20]); - state.put_ibc_relayer_address(&address_1); + state.put_ibc_relayer_address(address_1); assert!( state - .is_ibc_relayer(&address_1) + .is_ibc_relayer(address_1) .await .expect("a relayer address was written and must exist inside the database"), "additional stored relayer address could not be verified" ); assert!( state - .is_ibc_relayer(&address) + .is_ibc_relayer(address) .await .expect("a relayer address was written and must exist inside the database"), "original stored relayer address could not be verified" diff --git a/crates/astria-sequencer/src/test_utils.rs b/crates/astria-sequencer/src/test_utils.rs index e9ba4268de..c25099ad52 100644 --- a/crates/astria-sequencer/src/test_utils.rs +++ b/crates/astria-sequencer/src/test_utils.rs @@ -40,9 +40,10 @@ pub(crate) fn verification_key(seed: u64) -> VerificationKey { } #[track_caller] -pub(crate) fn assert_anyhow_error(error: anyhow::Error, expected: &'static str) { +pub(crate) fn assert_anyhow_error(error: &anyhow::Error, expected: &'static str) { let msg = error.to_string(); - if !msg.contains(expected) { - panic!("error contained different message\n\texpected: {expected}\n\tfull_error: {msg}"); - }; + assert!( + msg.contains(expected), + "error contained different message\n\texpected: {expected}\n\tfull_error: {msg}", + ); } diff --git a/crates/astria-sequencer/src/transaction/checks.rs b/crates/astria-sequencer/src/transaction/checks.rs index 1d0f9965f3..5e0e31ea65 100644 --- a/crates/astria-sequencer/src/transaction/checks.rs +++ b/crates/astria-sequencer/src/transaction/checks.rs @@ -181,7 +181,7 @@ pub(crate) async fn check_balance_for_total_fees_and_transfers( } Action::BridgeUnlock(act) => { let asset = state - .get_bridge_account_ibc_asset(&tx) + .get_bridge_account_ibc_asset(tx) .await .context("failed to get bridge account asset id")?; cost_by_asset diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index 152eb9561b..ea338fbff6 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -146,6 +146,9 @@ impl ActionHandler for SignedTransaction { Ok(()) } + // allowed because most lines come from delegating (and error wrapping) to the individual + // actions. + #[allow(clippy::too_many_lines)] async fn check_and_execute(&self, mut state: S) -> anyhow::Result<()> { // Add the current signed transaction into the ephemeral state in case // downstream actions require access to it. @@ -187,7 +190,7 @@ impl ActionHandler for SignedTransaction { let from_nonce = state .get_account_nonce(self) .await - .context("failed getting `from` nonce")?; + .context("failed getting nonce of transaction signer")?; let next_nonce = from_nonce .checked_add(1) .context("overflow occurred incrementing stored nonce")?; @@ -200,26 +203,28 @@ impl ActionHandler for SignedTransaction { Action::Transfer(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for TransferAction")?, + .context("executing transfer action failed")?, Action::Sequence(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for SequenceAction")?, + .context("executing sequence action failed")?, Action::ValidatorUpdate(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for ValidatorUpdateAction")?, + .context("executing validor update")?, Action::SudoAddressChange(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for SudoAddressChangeAction")?, + .context("executing sudo address change failed")?, Action::FeeChange(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for FeeChangeAction")?, + .context("executing fee change failed")?, Action::Ibc(act) => { - // FIXME: this check could should be moved to check_and_execute, as it now has - // access to the state. + // FIXME: this check should be moved to check_and_execute, as it now has + // access to the the signer through state. However, what's the correct + // ibc AppHandler call to do it? Can we just update one of the trait methods + // of crate::ibc::ics20_transfer::Ics20Transfer? ensure!( state .is_ibc_relayer(self) @@ -233,40 +238,41 @@ impl ActionHandler for SignedTransaction { action .check_and_execute(&mut state) .await - .context("execution failed for IbcAction")?; + .context("failed executing ibc action")?; } Action::Ics20Withdrawal(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for Ics20WithdrawalAction")?, + .context("failed executing ics20 withdrawal")?, Action::IbcRelayerChange(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for IbcRelayerChangeAction")?, + .context("failed executing ibc relayer change")?, Action::FeeAssetChange(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for FeeAssetChangeAction")?, + .context("failed executing fee asseet change")?, Action::InitBridgeAccount(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for InitBridgeAccountAction")?, + .context("failed executing init bridge account")?, Action::BridgeLock(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for BridgeLockAction")?, + .context("failed executing bridge lock")?, Action::BridgeUnlock(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for BridgeUnlockAction")?, + .context("failed executing bridge unlock")?, Action::BridgeSudoChange(act) => act .check_and_execute(&mut state) .await - .context("stateful check failed for BridgeSudoChangeAction")?, + .context("failed executing bridge sudo change")?, } } - state.delete_current_source().await; + // XXX: Delete the current transaction data from the ephemeral state. + state.delete_current_source(); Ok(()) } } diff --git a/crates/astria-sequencer/src/transaction/state_ext.rs b/crates/astria-sequencer/src/transaction/state_ext.rs index 9a4e44e5d7..4304505b76 100644 --- a/crates/astria-sequencer/src/transaction/state_ext.rs +++ b/crates/astria-sequencer/src/transaction/state_ext.rs @@ -36,7 +36,7 @@ pub(crate) trait StateWriteExt: StateWrite { self.object_put(current_source(), context); } - async fn delete_current_source(&mut self) { + fn delete_current_source(&mut self) { self.object_delete(current_source()); } } From 1f90d29f0990ac51dc085023d3fa468d411200f9 Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 1 Aug 2024 22:46:21 +0200 Subject: [PATCH 07/23] comment out unused code --- .../astria-sequencer/src/app/action_handler.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/crates/astria-sequencer/src/app/action_handler.rs b/crates/astria-sequencer/src/app/action_handler.rs index 01daf1fb4b..86c3b77755 100644 --- a/crates/astria-sequencer/src/app/action_handler.rs +++ b/crates/astria-sequencer/src/app/action_handler.rs @@ -1,9 +1,6 @@ use std::sync::Arc; -use cnidarium::{ - StateRead, - StateWrite, -}; +use cnidarium::StateWrite; /// This trait is a verbatim copy of [`cnidarium_component::ActionHandler`]. /// @@ -15,8 +12,13 @@ pub(crate) trait ActionHandler { type CheckStatelessContext: Clone + Send + Sync + 'static; async fn check_stateless(&self, context: Self::CheckStatelessContext) -> anyhow::Result<()>; - async fn check_historical(&self, _state: Arc) -> anyhow::Result<()> { - Ok(()) - } + // Commenting out for the time being as CI flags this as not being used. Leaving this in + // for reference as this is copied from cnidarium_component. + // ``` + // async fn check_historical(&self, _state: Arc) -> anyhow::Result<()> { + // Ok(()) + // } + // ``` + async fn check_and_execute(&self, mut state: S) -> anyhow::Result<()>; } From 588cb96686d1a7d51566f597253b0db508d3e1a1 Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 1 Aug 2024 22:55:31 +0200 Subject: [PATCH 08/23] add fixmes --- crates/astria-sequencer/src/transaction/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index ea338fbff6..11870a62da 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -146,8 +146,9 @@ impl ActionHandler for SignedTransaction { Ok(()) } - // allowed because most lines come from delegating (and error wrapping) to the individual - // actions. + // allowed / FIXME: because most lines come from delegating (and error wrapping) to the + // individual actions. This could be tidied up by implementing `ActionHandler for Action` + // and letting it delegate. #[allow(clippy::too_many_lines)] async fn check_and_execute(&self, mut state: S) -> anyhow::Result<()> { // Add the current signed transaction into the ephemeral state in case @@ -198,6 +199,7 @@ impl ActionHandler for SignedTransaction { .put_account_nonce(self, next_nonce) .context("failed updating `from` nonce")?; + // FIXME: this should create one span per `check_and_execute` for action in self.actions() { match action { Action::Transfer(act) => act From 8326f0d21985d97355ce4afbdedbc321cda21b62 Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Thu, 1 Aug 2024 22:59:02 +0200 Subject: [PATCH 09/23] remove unused import --- crates/astria-sequencer/src/app/action_handler.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/astria-sequencer/src/app/action_handler.rs b/crates/astria-sequencer/src/app/action_handler.rs index 86c3b77755..73af0cefee 100644 --- a/crates/astria-sequencer/src/app/action_handler.rs +++ b/crates/astria-sequencer/src/app/action_handler.rs @@ -1,5 +1,3 @@ -use std::sync::Arc; - use cnidarium::StateWrite; /// This trait is a verbatim copy of [`cnidarium_component::ActionHandler`]. From 5334a9f6405b47093b814ba0bf53d9a326c7cd62 Mon Sep 17 00:00:00 2001 From: Ethan Oroshiba Date: Thu, 1 Aug 2024 14:11:07 -0500 Subject: [PATCH 10/23] chore(core): Implement Protobuf trait for tx actions (#1320) ## Summary Implemented the `Protobuf` trait for all TX `Action`s and variants. ## Background `Action` and variants are refined types from generated Protobuf code, but did not implement the `Protobuf` trait. This change is to standardize the types `Error` and `Raw` across `Action`, as well as the methods `to_raw()`, `into_raw()`, `try_from_raw()`, and `try_from_raw_ref()`. ## Changes - Implemented `Protobuf` trait, types, and necessary functions for `Action` and all of its variants. - Deleted redundant functions which took ownership, but kept those which prioritized cost over their `ref` counterparts. ## Testing Passing all tests. ## Related Issues closes #1307 --------- Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com> --- .../protocol/transaction/v1alpha1/action.rs | 316 ++++++++++++------ .../src/protocol/transaction/v1alpha1/mod.rs | 1 + crates/astria-sequencer/src/utils.rs | 1 + 3 files changed, 211 insertions(+), 107 deletions(-) diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index 81256f14e3..da22b1039e 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -45,32 +45,12 @@ pub enum Action { FeeChange(FeeChangeAction), } -impl Action { - #[must_use] - pub fn into_raw(self) -> raw::Action { - use raw::action::Value; - let kind = match self { - Action::Sequence(act) => Value::SequenceAction(act.into_raw()), - Action::Transfer(act) => Value::TransferAction(act.into_raw()), - Action::ValidatorUpdate(act) => Value::ValidatorUpdateAction(act.into_raw()), - Action::SudoAddressChange(act) => Value::SudoAddressChangeAction(act.into_raw()), - Action::Ibc(act) => Value::IbcAction(act.into()), - Action::Ics20Withdrawal(act) => Value::Ics20Withdrawal(act.into_raw()), - Action::IbcRelayerChange(act) => Value::IbcRelayerChangeAction(act.into_raw()), - Action::FeeAssetChange(act) => Value::FeeAssetChangeAction(act.into_raw()), - Action::InitBridgeAccount(act) => Value::InitBridgeAccountAction(act.into_raw()), - Action::BridgeLock(act) => Value::BridgeLockAction(act.into_raw()), - Action::BridgeUnlock(act) => Value::BridgeUnlockAction(act.into_raw()), - Action::BridgeSudoChange(act) => Value::BridgeSudoChangeAction(act.into_raw()), - Action::FeeChange(act) => Value::FeeChangeAction(act.into_raw()), - }; - raw::Action { - value: Some(kind), - } - } +impl Protobuf for Action { + type Error = ActionError; + type Raw = raw::Action; #[must_use] - pub fn to_raw(&self) -> raw::Action { + fn to_raw(&self) -> Self::Raw { use raw::action::Value; let kind = match self { Action::Sequence(act) => Value::SequenceAction(act.to_raw()), @@ -94,13 +74,23 @@ impl Action { } } + /// Attempt to convert from a reference to raw, unchecked protobuf [`raw::Action`]. + /// + /// # Errors + /// + /// Returns an error if conversion of one of the inner raw action variants + /// to a native action ([`SequenceAction`] or [`TransferAction`]) fails. + fn try_from_raw_ref(raw: &Self::Raw) -> Result { + Self::try_from_raw(raw.clone()) + } + /// Attempt to convert from a raw, unchecked protobuf [`raw::Action`]. /// /// # Errors /// /// Returns an error if conversion of one of the inner raw action variants /// to a native action ([`SequenceAction`] or [`TransferAction`]) fails. - pub fn try_from_raw(proto: raw::Action) -> Result { + fn try_from_raw(proto: raw::Action) -> Result { use raw::action::Value; let raw::Action { value, @@ -129,11 +119,12 @@ impl Action { Ics20Withdrawal::try_from_raw(act).map_err(ActionError::ics20_withdrawal)?, ), Value::IbcRelayerChangeAction(act) => Self::IbcRelayerChange( - IbcRelayerChangeAction::try_from_raw(&act) + IbcRelayerChangeAction::try_from_raw_ref(&act) .map_err(ActionError::ibc_relayer_change)?, ), Value::FeeAssetChangeAction(act) => Self::FeeAssetChange( - FeeAssetChangeAction::try_from_raw(&act).map_err(ActionError::fee_asset_change)?, + FeeAssetChangeAction::try_from_raw_ref(&act) + .map_err(ActionError::fee_asset_change)?, ), Value::InitBridgeAccountAction(act) => Self::InitBridgeAccount( InitBridgeAccountAction::try_from_raw(act) @@ -150,12 +141,14 @@ impl Action { .map_err(ActionError::bridge_sudo_change)?, ), Value::FeeChangeAction(act) => Self::FeeChange( - FeeChangeAction::try_from_raw(&act).map_err(ActionError::fee_change)?, + FeeChangeAction::try_from_raw_ref(&act).map_err(ActionError::fee_change)?, ), }; Ok(action) } +} +impl Action { #[must_use] pub fn as_sequence(&self) -> Option<&SequenceAction> { let Self::Sequence(sequence_action) = self else { @@ -580,7 +573,10 @@ pub struct ValidatorUpdate { pub verification_key: crate::crypto::VerificationKey, } -impl ValidatorUpdate { +impl Protobuf for ValidatorUpdate { + type Error = ValidatorUpdateError; + type Raw = crate::generated::astria_vendored::tendermint::abci::ValidatorUpdate; + /// Create a validator update by verifying a raw protobuf-decoded /// [`crate::generated::astria_vendored::tendermint::abci::ValidatorUpdate`]. /// @@ -589,7 +585,7 @@ impl ValidatorUpdate { /// is not set, or if `.pub_key` contains a non-ed25519 variant, or /// if the ed25519 has invalid bytes (that is, bytes from which an /// ed25519 public key cannot be constructed). - pub fn try_from_raw( + fn try_from_raw( value: crate::generated::astria_vendored::tendermint::abci::ValidatorUpdate, ) -> Result { use crate::generated::astria_vendored::tendermint::crypto::{ @@ -623,13 +619,20 @@ impl ValidatorUpdate { }) } - #[must_use] - pub fn into_raw(self) -> crate::generated::astria_vendored::tendermint::abci::ValidatorUpdate { - self.to_raw() + /// Create a validator update by verifying a reference to raw protobuf-decoded + /// [`crate::generated::astria_vendored::tendermint::abci::ValidatorUpdate`]. + /// + /// # Errors + /// Returns an error if the `.power` field is negative, if `.pub_key` + /// is not set, or if `.pub_key` contains a non-ed25519 variant, or + /// if the ed25519 has invalid bytes (that is, bytes from which an + /// ed25519 public key cannot be constructed). + fn try_from_raw_ref(raw: &Self::Raw) -> Result { + Self::try_from_raw(raw.clone()) } #[must_use] - pub fn to_raw(&self) -> crate::generated::astria_vendored::tendermint::abci::ValidatorUpdate { + fn to_raw(&self) -> crate::generated::astria_vendored::tendermint::abci::ValidatorUpdate { use crate::generated::astria_vendored::tendermint::crypto::{ public_key, PublicKey, @@ -676,9 +679,11 @@ pub struct SudoAddressChangeAction { pub new_address: Address, } -impl SudoAddressChangeAction { - #[must_use] - pub fn into_raw(self) -> raw::SudoAddressChangeAction { +impl Protobuf for SudoAddressChangeAction { + type Error = SudoAddressChangeActionError; + type Raw = raw::SudoAddressChangeAction; + + fn into_raw(self) -> raw::SudoAddressChangeAction { let Self { new_address, } = self; @@ -688,7 +693,7 @@ impl SudoAddressChangeAction { } #[must_use] - pub fn to_raw(&self) -> raw::SudoAddressChangeAction { + fn to_raw(&self) -> raw::SudoAddressChangeAction { let Self { new_address, } = self; @@ -697,15 +702,13 @@ impl SudoAddressChangeAction { } } - /// Convert from a raw, unchecked protobuf [`raw::SudoAddressChangeAction`]. + /// Convert from a reference to a raw, unchecked protobuf [`raw::SudoAddressChangeAction`]. /// /// # Errors /// /// Returns an error if the raw action's `new_address` did not have the expected /// length. - pub fn try_from_raw( - proto: raw::SudoAddressChangeAction, - ) -> Result { + fn try_from_raw_ref(proto: &Self::Raw) -> Result { let raw::SudoAddressChangeAction { new_address, } = proto; @@ -713,7 +716,7 @@ impl SudoAddressChangeAction { return Err(SudoAddressChangeActionError::field_not_set("new_address")); }; let new_address = - Address::try_from_raw(&new_address).map_err(SudoAddressChangeActionError::address)?; + Address::try_from_raw(new_address).map_err(SudoAddressChangeActionError::address)?; Ok(Self { new_address, }) @@ -841,9 +844,14 @@ impl Ics20Withdrawal { memo: self.memo.clone(), } } +} + +impl Protobuf for Ics20Withdrawal { + type Error = Ics20WithdrawalError; + type Raw = raw::Ics20Withdrawal; #[must_use] - pub fn to_raw(&self) -> raw::Ics20Withdrawal { + fn to_raw(&self) -> raw::Ics20Withdrawal { raw::Ics20Withdrawal { amount: Some(self.amount.into()), denom: self.denom.to_string(), @@ -859,7 +867,7 @@ impl Ics20Withdrawal { } #[must_use] - pub fn into_raw(self) -> raw::Ics20Withdrawal { + fn into_raw(self) -> raw::Ics20Withdrawal { raw::Ics20Withdrawal { amount: Some(self.amount.into()), denom: self.denom.to_string(), @@ -883,7 +891,7 @@ impl Ics20Withdrawal { /// - if the `return_address` field is invalid or missing /// - if the `timeout_height` field is missing /// - if the `source_channel` field is invalid - pub fn try_from_raw(proto: raw::Ics20Withdrawal) -> Result { + fn try_from_raw(proto: raw::Ics20Withdrawal) -> Result { let raw::Ics20Withdrawal { amount, denom, @@ -928,6 +936,64 @@ impl Ics20Withdrawal { bridge_address, }) } + + /// Convert from a reference to raw, unchecked protobuf [`raw::Ics20Withdrawal`]. + /// + /// # Errors + /// + /// - if the `amount` field is missing + /// - if the `denom` field is invalid + /// - if the `return_address` field is invalid or missing + /// - if the `timeout_height` field is missing + /// - if the `source_channel` field is invalid + fn try_from_raw_ref(proto: &raw::Ics20Withdrawal) -> Result { + let raw::Ics20Withdrawal { + amount, + denom, + destination_chain_address, + return_address, + timeout_height, + timeout_time, + source_channel, + fee_asset, + memo, + bridge_address, + } = proto; + let amount = amount.ok_or(Ics20WithdrawalError::field_not_set("amount"))?; + let return_address = Address::try_from_raw( + return_address + .as_ref() + .ok_or(Ics20WithdrawalError::field_not_set("return_address"))?, + ) + .map_err(Ics20WithdrawalError::return_address)?; + + let timeout_height = timeout_height + .clone() + .ok_or(Ics20WithdrawalError::field_not_set("timeout_height"))? + .into(); + let bridge_address = bridge_address + .as_ref() + .map(Address::try_from_raw) + .transpose() + .map_err(Ics20WithdrawalError::invalid_bridge_address)?; + + Ok(Self { + amount: amount.into(), + denom: denom.parse().map_err(Ics20WithdrawalError::invalid_denom)?, + destination_chain_address: destination_chain_address.clone(), + return_address, + timeout_height, + timeout_time: *timeout_time, + source_channel: source_channel + .parse() + .map_err(Ics20WithdrawalError::invalid_source_channel)?, + fee_asset: fee_asset + .parse() + .map_err(Ics20WithdrawalError::invalid_fee_asset)?, + memo: memo.clone(), + bridge_address, + }) + } } impl From for IbcHeight { @@ -1022,25 +1088,12 @@ pub enum IbcRelayerChangeAction { Removal(Address), } -impl IbcRelayerChangeAction { - #[must_use] - pub fn into_raw(self) -> raw::IbcRelayerChangeAction { - match self { - IbcRelayerChangeAction::Addition(address) => raw::IbcRelayerChangeAction { - value: Some(raw::ibc_relayer_change_action::Value::Addition( - address.to_raw(), - )), - }, - IbcRelayerChangeAction::Removal(address) => raw::IbcRelayerChangeAction { - value: Some(raw::ibc_relayer_change_action::Value::Removal( - address.to_raw(), - )), - }, - } - } +impl Protobuf for IbcRelayerChangeAction { + type Error = IbcRelayerChangeActionError; + type Raw = raw::IbcRelayerChangeAction; #[must_use] - pub fn to_raw(&self) -> raw::IbcRelayerChangeAction { + fn to_raw(&self) -> raw::IbcRelayerChangeAction { match self { IbcRelayerChangeAction::Addition(address) => raw::IbcRelayerChangeAction { value: Some(raw::ibc_relayer_change_action::Value::Addition( @@ -1060,7 +1113,7 @@ impl IbcRelayerChangeAction { /// # Errors /// /// - if the `address` field is invalid - pub fn try_from_raw( + fn try_from_raw_ref( raw: &raw::IbcRelayerChangeAction, ) -> Result { match raw { @@ -1116,25 +1169,12 @@ pub enum FeeAssetChangeAction { Removal(asset::Denom), } -impl FeeAssetChangeAction { - #[must_use] - pub fn into_raw(self) -> raw::FeeAssetChangeAction { - match self { - FeeAssetChangeAction::Addition(asset) => raw::FeeAssetChangeAction { - value: Some(raw::fee_asset_change_action::Value::Addition( - asset.to_string(), - )), - }, - FeeAssetChangeAction::Removal(asset) => raw::FeeAssetChangeAction { - value: Some(raw::fee_asset_change_action::Value::Removal( - asset.to_string(), - )), - }, - } - } +impl Protobuf for FeeAssetChangeAction { + type Error = FeeAssetChangeActionError; + type Raw = raw::FeeAssetChangeAction; #[must_use] - pub fn to_raw(&self) -> raw::FeeAssetChangeAction { + fn to_raw(&self) -> raw::FeeAssetChangeAction { match self { FeeAssetChangeAction::Addition(asset) => raw::FeeAssetChangeAction { value: Some(raw::fee_asset_change_action::Value::Addition( @@ -1149,12 +1189,12 @@ impl FeeAssetChangeAction { } } - /// Convert from a raw, unchecked protobuf [`raw::FeeAssetChangeAction`]. + /// Convert from a reference to a raw, unchecked protobuf [`raw::FeeAssetChangeAction`]. /// /// # Errors /// /// - if the `asset` field is invalid - pub fn try_from_raw( + fn try_from_raw_ref( raw: &raw::FeeAssetChangeAction, ) -> Result { match raw { @@ -1221,9 +1261,12 @@ pub struct InitBridgeAccountAction { pub withdrawer_address: Option
, } -impl InitBridgeAccountAction { +impl Protobuf for InitBridgeAccountAction { + type Error = InitBridgeAccountActionError; + type Raw = raw::InitBridgeAccountAction; + #[must_use] - pub fn into_raw(self) -> raw::InitBridgeAccountAction { + fn into_raw(self) -> raw::InitBridgeAccountAction { raw::InitBridgeAccountAction { rollup_id: Some(self.rollup_id.to_raw()), asset: self.asset.to_string(), @@ -1234,7 +1277,7 @@ impl InitBridgeAccountAction { } #[must_use] - pub fn to_raw(&self) -> raw::InitBridgeAccountAction { + fn to_raw(&self) -> raw::InitBridgeAccountAction { raw::InitBridgeAccountAction { rollup_id: Some(self.rollup_id.to_raw()), asset: self.asset.to_string(), @@ -1252,7 +1295,7 @@ impl InitBridgeAccountAction { /// - if the `rollup_id` field is invalid /// - if the `sudo_address` field is invalid /// - if the `withdrawer_address` field is invalid - pub fn try_from_raw( + fn try_from_raw( proto: raw::InitBridgeAccountAction, ) -> Result { let Some(rollup_id) = proto.rollup_id else { @@ -1289,6 +1332,18 @@ impl InitBridgeAccountAction { withdrawer_address, }) } + + /// Convert from a reference to a raw, unchecked protobuf [`raw::InitBridgeAccountAction`]. + /// + /// # Errors + /// + /// - if the `rollup_id` field is not set + /// - if the `rollup_id` field is invalid + /// - if the `sudo_address` field is invalid + /// - if the `withdrawer_address` field is invalid + fn try_from_raw_ref(proto: &Self::Raw) -> Result { + Self::try_from_raw(proto.clone()) + } } #[derive(Debug, thiserror::Error)] @@ -1362,9 +1417,12 @@ pub struct BridgeLockAction { pub destination_chain_address: String, } -impl BridgeLockAction { +impl Protobuf for BridgeLockAction { + type Error = BridgeLockActionError; + type Raw = raw::BridgeLockAction; + #[must_use] - pub fn into_raw(self) -> raw::BridgeLockAction { + fn into_raw(self) -> raw::BridgeLockAction { raw::BridgeLockAction { to: Some(self.to.to_raw()), amount: Some(self.amount.into()), @@ -1375,7 +1433,7 @@ impl BridgeLockAction { } #[must_use] - pub fn to_raw(&self) -> raw::BridgeLockAction { + fn to_raw(&self) -> raw::BridgeLockAction { raw::BridgeLockAction { to: Some(self.to.to_raw()), amount: Some(self.amount.into()), @@ -1393,7 +1451,7 @@ impl BridgeLockAction { /// - if the `to` field is invalid /// - if the `asset` field is invalid /// - if the `fee_asset` field is invalid - pub fn try_from_raw(proto: raw::BridgeLockAction) -> Result { + fn try_from_raw(proto: raw::BridgeLockAction) -> Result { let Some(to) = proto.to else { return Err(BridgeLockActionError::field_not_set("to")); }; @@ -1417,6 +1475,18 @@ impl BridgeLockAction { destination_chain_address: proto.destination_chain_address, }) } + + /// Convert from a reference to a raw, unchecked protobuf [`raw::BridgeLockAction`]. + /// + /// # Errors + /// + /// - if the `to` field is not set + /// - if the `to` field is invalid + /// - if the `asset` field is invalid + /// - if the `fee_asset` field is invalid + fn try_from_raw_ref(proto: &raw::BridgeLockAction) -> Result { + Self::try_from_raw(proto.clone()) + } } #[derive(Debug, thiserror::Error)] @@ -1481,9 +1551,12 @@ pub struct BridgeUnlockAction { pub bridge_address: Option
, } -impl BridgeUnlockAction { +impl Protobuf for BridgeUnlockAction { + type Error = BridgeUnlockActionError; + type Raw = raw::BridgeUnlockAction; + #[must_use] - pub fn into_raw(self) -> raw::BridgeUnlockAction { + fn into_raw(self) -> raw::BridgeUnlockAction { raw::BridgeUnlockAction { to: Some(self.to.to_raw()), amount: Some(self.amount.into()), @@ -1494,7 +1567,7 @@ impl BridgeUnlockAction { } #[must_use] - pub fn to_raw(&self) -> raw::BridgeUnlockAction { + fn to_raw(&self) -> raw::BridgeUnlockAction { raw::BridgeUnlockAction { to: Some(self.to.to_raw()), amount: Some(self.amount.into()), @@ -1513,7 +1586,7 @@ impl BridgeUnlockAction { /// - if the `amount` field is invalid /// - if the `fee_asset` field is invalid /// - if the `from` field is invalid - pub fn try_from_raw(proto: raw::BridgeUnlockAction) -> Result { + fn try_from_raw(proto: raw::BridgeUnlockAction) -> Result { let Some(to) = proto.to else { return Err(BridgeUnlockActionError::field_not_set("to")); }; @@ -1539,6 +1612,19 @@ impl BridgeUnlockAction { bridge_address, }) } + + /// Convert from a reference to a raw, unchecked protobuf [`raw::BridgeUnlockAction`]. + /// + /// # Errors + /// + /// - if the `to` field is not set + /// - if the `to` field is invalid + /// - if the `amount` field is invalid + /// - if the `fee_asset` field is invalid + /// - if the `from` field is invalid + fn try_from_raw_ref(proto: &raw::BridgeUnlockAction) -> Result { + Self::try_from_raw(proto.clone()) + } } #[derive(Debug, thiserror::Error)] @@ -1597,9 +1683,12 @@ pub struct BridgeSudoChangeAction { pub fee_asset: asset::Denom, } -impl BridgeSudoChangeAction { +impl Protobuf for BridgeSudoChangeAction { + type Error = BridgeSudoChangeActionError; + type Raw = raw::BridgeSudoChangeAction; + #[must_use] - pub fn into_raw(self) -> raw::BridgeSudoChangeAction { + fn into_raw(self) -> raw::BridgeSudoChangeAction { raw::BridgeSudoChangeAction { bridge_address: Some(self.bridge_address.to_raw()), new_sudo_address: self.new_sudo_address.map(Address::into_raw), @@ -1609,7 +1698,7 @@ impl BridgeSudoChangeAction { } #[must_use] - pub fn to_raw(&self) -> raw::BridgeSudoChangeAction { + fn to_raw(&self) -> raw::BridgeSudoChangeAction { raw::BridgeSudoChangeAction { bridge_address: Some(self.bridge_address.to_raw()), new_sudo_address: self.new_sudo_address.as_ref().map(Address::to_raw), @@ -1627,7 +1716,7 @@ impl BridgeSudoChangeAction { /// - if the `new_sudo_address` field is invalid /// - if the `new_withdrawer_address` field is invalid /// - if the `fee_asset` field is invalid - pub fn try_from_raw( + fn try_from_raw( proto: raw::BridgeSudoChangeAction, ) -> Result { let Some(bridge_address) = proto.bridge_address else { @@ -1659,6 +1748,21 @@ impl BridgeSudoChangeAction { fee_asset, }) } + + /// Convert from a reference to a raw, unchecked protobuf [`raw::BridgeSudoChangeAction`]. + /// + /// # Errors + /// + /// - if the `bridge_address` field is not set + /// - if the `bridge_address` field is invalid + /// - if the `new_sudo_address` field is invalid + /// - if the `new_withdrawer_address` field is invalid + /// - if the `fee_asset` field is invalid + fn try_from_raw_ref( + proto: &raw::BridgeSudoChangeAction, + ) -> Result { + Self::try_from_raw(proto.clone()) + } } #[derive(Debug, thiserror::Error)] @@ -1724,14 +1828,12 @@ pub struct FeeChangeAction { pub new_value: u128, } -impl FeeChangeAction { - #[must_use] - pub fn into_raw(self) -> raw::FeeChangeAction { - self.to_raw() - } +impl Protobuf for FeeChangeAction { + type Error = FeeChangeActionError; + type Raw = raw::FeeChangeAction; #[must_use] - pub fn to_raw(&self) -> raw::FeeChangeAction { + fn to_raw(&self) -> raw::FeeChangeAction { raw::FeeChangeAction { value: Some(match self.fee_change { FeeChange::TransferBaseFee => { @@ -1761,13 +1863,13 @@ impl FeeChangeAction { } } - /// Convert from a raw, unchecked protobuf [`raw::FeeChangeAction`]. + /// Convert from a reference to a raw, unchecked protobuf [`raw::FeeChangeAction`]. /// /// # Errors /// /// - if the fee change `value` field is missing /// - if the `new_value` field is missing - pub fn try_from_raw(proto: &raw::FeeChangeAction) -> Result { + fn try_from_raw_ref(proto: &raw::FeeChangeAction) -> Result { let (fee_change, new_value) = match proto.value { Some(raw::fee_change_action::Value::TransferBaseFee(new_value)) => { (FeeChange::TransferBaseFee, new_value) diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs index 77032792cd..d5e8543001 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs @@ -15,6 +15,7 @@ use crate::{ asset, ADDRESS_LEN, }, + Protobuf as _, }; pub mod action; diff --git a/crates/astria-sequencer/src/utils.rs b/crates/astria-sequencer/src/utils.rs index d2d2d3bf27..0ee4ffffa6 100644 --- a/crates/astria-sequencer/src/utils.rs +++ b/crates/astria-sequencer/src/utils.rs @@ -2,6 +2,7 @@ use anyhow::Context as _; use astria_core::{ generated::astria_vendored::tendermint::abci as raw, protocol::transaction::v1alpha1::action::ValidatorUpdate, + Protobuf as _, }; pub(crate) struct Hex<'a>(pub(crate) &'a [u8]); From 5302322e7c92518f564696b9937b171783e0642e Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Fri, 2 Aug 2024 10:40:09 -0500 Subject: [PATCH 11/23] Fix fee reporting --- ..._changes__app_finalize_block_snapshot.snap | 60 ++--- .../src/app/tests_execute_transaction.rs | 247 +++++++++++++++++- .../src/bridge/bridge_lock_action.rs | 8 +- .../src/bridge/bridge_sudo_change_action.rs | 15 +- .../src/bridge/init_bridge_account_action.rs | 10 +- .../src/ibc/ics20_withdrawal.rs | 5 + 6 files changed, 306 insertions(+), 39 deletions(-) diff --git a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_finalize_block_snapshot.snap b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_finalize_block_snapshot.snap index 7d4b7f970c..4647bcb04f 100644 --- a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_finalize_block_snapshot.snap +++ b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_finalize_block_snapshot.snap @@ -3,36 +3,36 @@ source: crates/astria-sequencer/src/app/tests_breaking_changes.rs expression: app.app_hash.as_bytes() --- [ - 72, - 87, - 29, - 138, - 91, - 221, - 71, + 36, 66, - 219, - 212, - 171, - 126, - 223, - 176, - 198, - 154, - 10, - 234, - 253, - 99, - 213, - 78, - 92, - 228, - 89, - 204, - 197, + 30, + 16, + 122, + 103, + 62, + 67, + 145, + 180, + 247, + 139, + 229, + 200, + 86, 193, - 90, - 57, - 205, - 2 + 28, + 71, + 235, + 32, + 134, + 143, + 239, + 229, + 8, + 86, + 190, + 164, + 81, + 230, + 24, + 149 ] diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index a41a60f82f..2f2218e70b 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -9,8 +9,10 @@ use astria_core::{ protocol::transaction::v1alpha1::{ action::{ BridgeLockAction, + BridgeSudoChangeAction, BridgeUnlockAction, IbcRelayerChangeAction, + InitBridgeAccountAction, SequenceAction, SudoAddressChangeAction, TransferAction, @@ -32,7 +34,10 @@ use penumbra_ibc::params::IBCParameters; use tendermint::abci::EventAttributeIndexExt as _; use crate::{ - accounts::StateReadExt as _, + accounts::{ + StateReadExt as _, + StateWriteExt, + }, app::{ test_utils::*, ActionHandler as _, @@ -40,11 +45,15 @@ use crate::{ assets::StateReadExt as _, authority::StateReadExt as _, bridge::{ + get_deposit_byte_len, StateReadExt as _, StateWriteExt as _, }, ibc::StateReadExt as _, - sequence::calculate_fee_from_state, + sequence::{ + calculate_fee_from_state, + StateWriteExt as _, + }, test_utils::{ astria_address, astria_address_from_hex_string, @@ -1100,3 +1109,237 @@ async fn transaction_execution_records_fee_event() { .into() ); } + +#[tokio::test] +async fn ensure_correct_block_fees_transfer() { + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + state_tx.put_transfer_base_fee(1).unwrap(); + app.apply(state_tx); + + let alice = get_alice_signing_key(); + let bob_address = astria_address_from_hex_string(BOB_ADDRESS); + let value = 333_333; + let actions = vec![ + TransferAction { + to: bob_address, + amount: value, + asset: nria().into(), + fee_asset: nria().into(), + } + .into(), + ]; + + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions, + }; + let signed_tx = Arc::new(tx.into_signed(&alice)); + app.execute_transaction(signed_tx).await.unwrap(); + + let total_block_fees: u128 = app + .state + .get_block_fees() + .await + .expect("failed to get block fees") + .into_iter() + .map(|(_, fee)| fee) + .sum(); + assert_eq!(total_block_fees, 1); +} + +#[tokio::test] +async fn ensure_correct_block_fees_sequence() { + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + state_tx.put_sequence_action_base_fee(1); + state_tx.put_sequence_action_byte_cost_multiplier(1); + app.apply(state_tx); + + let alice = get_alice_signing_key(); + let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); + let data = b"hello world".to_vec(); + + let actions = vec![ + SequenceAction { + rollup_id, + data: data.clone(), + fee_asset: nria().into(), + } + .into(), + ]; + + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions, + }; + let signed_tx = Arc::new(tx.into_signed(&alice)); + app.execute_transaction(signed_tx).await.unwrap(); + + let total_block_fees: u128 = app + .state + .get_block_fees() + .await + .expect("failed to get block fees") + .into_iter() + .map(|(_, fee)| fee) + .sum(); + let expected_fees = calculate_fee_from_state(&data, &app.state).await.unwrap(); + assert_eq!(total_block_fees, expected_fees); +} + +#[tokio::test] +async fn ensure_correct_block_fees_init_bridge_acct() { + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + state_tx.put_init_bridge_account_base_fee(1); + app.apply(state_tx); + + let alice = get_alice_signing_key(); + let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); + + let actions = vec![ + InitBridgeAccountAction { + rollup_id, + asset: nria().into(), + fee_asset: nria().into(), + sudo_address: None, + withdrawer_address: None, + } + .into(), + ]; + + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions, + }; + let signed_tx = Arc::new(tx.into_signed(&alice)); + app.execute_transaction(signed_tx).await.unwrap(); + + let total_block_fees: u128 = app + .state + .get_block_fees() + .await + .expect("failed to get block fees") + .into_iter() + .map(|(_, fee)| fee) + .sum(); + assert_eq!(total_block_fees, 1); +} + +#[tokio::test] +async fn ensure_correct_block_fees_bridge_lock() { + let alice = get_alice_signing_key(); + let bridge = get_bridge_signing_key(); + let bridge_address = astria_address(&bridge.address_bytes()); + let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); + + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + state_tx.put_transfer_base_fee(1).unwrap(); + state_tx.put_bridge_lock_byte_cost_multiplier(1); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); + state_tx + .put_bridge_account_ibc_asset(bridge_address, nria()) + .unwrap(); + app.apply(state_tx); + + let actions = vec![ + BridgeLockAction { + to: bridge_address, + amount: 1, + asset: nria().into(), + fee_asset: nria().into(), + destination_chain_address: rollup_id.to_string(), + } + .into(), + ]; + + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions, + }; + let signed_tx = Arc::new(tx.into_signed(&alice)); + app.execute_transaction(signed_tx).await.unwrap(); + + let test_deposit = Deposit::new( + bridge_address, + rollup_id, + 1, + nria().into(), + rollup_id.to_string(), + ); + + let total_block_fees: u128 = app + .state + .get_block_fees() + .await + .expect("failed to get block fees") + .into_iter() + .map(|(_, fee)| fee) + .sum(); + let expected_fees = 1 + get_deposit_byte_len(&test_deposit); + assert_eq!(total_block_fees, expected_fees); +} + +#[tokio::test] +async fn ensure_correct_block_fees_bridge_sudo_change() { + let alice = get_alice_signing_key(); + let alice_address = astria_address(&alice.address_bytes()); + let bridge = get_bridge_signing_key(); + let bridge_address = astria_address(&bridge.address_bytes()); + + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + state_tx.put_bridge_sudo_change_base_fee(1); + state_tx.put_bridge_account_sudo_address(bridge_address, alice_address); + state_tx + .increase_balance(bridge_address, nria(), 1) + .await + .unwrap(); + app.apply(state_tx); + + let actions = vec![ + BridgeSudoChangeAction { + bridge_address, + new_sudo_address: None, + new_withdrawer_address: None, + fee_asset: nria().into(), + } + .into(), + ]; + + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions, + }; + let signed_tx = Arc::new(tx.into_signed(&alice)); + app.execute_transaction(signed_tx).await.unwrap(); + + let total_block_fees: u128 = app + .state + .get_block_fees() + .await + .expect("failed to get block fees") + .into_iter() + .map(|(_, fee)| fee) + .sum(); + assert_eq!(total_block_fees, 1); +} + +// TODO: Add test to ensure correct block fees for ICS20 withdrawal diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 2bc87e1300..91d73a155c 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -9,6 +9,7 @@ use astria_core::{ TransferAction, }, sequencerblock::v1alpha1::block::Deposit, + Protobuf, }; use cnidarium::StateWrite; @@ -23,6 +24,7 @@ use crate::{ }, address::StateReadExt as _, app::ActionHandler, + assets::StateWriteExt as _, bridge::{ StateReadExt as _, StateWriteExt as _, @@ -120,7 +122,10 @@ impl ActionHandler for BridgeLockAction { .await .context("failed to get byte cost multiplier")?; let fee = byte_cost_multiplier.saturating_mul(get_deposit_byte_len(&deposit)); - + state + .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) + .await + .context("failed to add to block fees")?; state .decrease_balance(from, &self.fee_asset, fee) .await @@ -152,7 +157,6 @@ mod tests { use super::*; use crate::{ address::StateWriteExt, - assets::StateWriteExt as _, test_utils::{ assert_anyhow_error, astria_address, 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 cd835877c7..6318383698 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -3,14 +3,20 @@ use anyhow::{ Context as _, Result, }; -use astria_core::protocol::transaction::v1alpha1::action::BridgeSudoChangeAction; +use astria_core::{ + protocol::transaction::v1alpha1::action::BridgeSudoChangeAction, + Protobuf, +}; use cnidarium::StateWrite; use crate::{ accounts::StateWriteExt as _, address::StateReadExt as _, app::ActionHandler, - assets::StateReadExt as _, + assets::{ + StateReadExt as _, + StateWriteExt as _, + }, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -75,6 +81,10 @@ impl ActionHandler for BridgeSudoChangeAction { .get_bridge_sudo_change_base_fee() .await .context("failed to get bridge sudo change fee")?; + state + .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) + .await + .context("failed to add to block fees")?; state .decrease_balance(self.bridge_address, &self.fee_asset, fee) .await @@ -100,7 +110,6 @@ mod tests { use super::*; use crate::{ 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 54b7122444..214ef09962 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -7,6 +7,7 @@ use anyhow::{ use astria_core::{ primitive::v1::Address, protocol::transaction::v1alpha1::action::InitBridgeAccountAction, + Protobuf, }; use cnidarium::StateWrite; @@ -17,7 +18,10 @@ use crate::{ }, address::StateReadExt as _, app::ActionHandler, - assets::StateReadExt as _, + assets::{ + StateReadExt as _, + StateWriteExt as _, + }, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -100,7 +104,9 @@ impl ActionHandler for InitBridgeAccountAction { from, self.withdrawer_address.map_or(from, Address::bytes), ); - + state + .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) + .await?; state .decrease_balance(from, &self.fee_asset, fee) .await diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 34ca3fee80..f64aa25f33 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -11,6 +11,7 @@ use astria_core::{ Address, }, protocol::transaction::v1alpha1::action, + Protobuf, }; use cnidarium::{ StateRead, @@ -35,6 +36,7 @@ use crate::{ }, address::StateReadExt as _, app::ActionHandler, + assets::StateWriteExt as _, bridge::StateReadExt as _, ibc::{ StateReadExt as _, @@ -183,6 +185,9 @@ impl ActionHandler for action::Ics20Withdrawal { "insufficient funds for transfer" ); } + state + .get_and_increase_block_fees(self.fee_asset(), fee, Self::full_name()) + .await?; state .decrease_balance(from, self.denom(), self.amount()) From bead777b90cdabaf7d763c4315d69d207d3bbd7b Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Fri, 2 Aug 2024 10:49:49 -0500 Subject: [PATCH 12/23] rustfmt --- .../src/app/tests_execute_transaction.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 2f2218e70b..130c425795 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -44,25 +44,38 @@ use crate::{ }, assets::StateReadExt as _, authority::StateReadExt as _, + bridge::{ + }, + app::test_utils::*, + assets::StateReadExt as _, + authority::StateReadExt as _, bridge::{ get_deposit_byte_len, StateReadExt as _, StateWriteExt as _, }, ibc::StateReadExt as _, + sequence::{ + }, + ibc::StateReadExt as _, sequence::{ calculate_fee_from_state, StateWriteExt as _, }, + test_utils::{ + }, test_utils::{ astria_address, astria_address_from_hex_string, nria, }, + transaction::{ + }, transaction::{ InvalidChainId, InvalidNonce, }, + }, }; /// XXX: This should be expressed in terms of `crate::app::test_utils::unchecked_genesis_state` to From 1cd56092b37e1d8ab30bd2ec71a3daed8a786a18 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Fri, 2 Aug 2024 11:14:31 -0500 Subject: [PATCH 13/23] Minor changes --- .../src/app/tests_execute_transaction.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 130c425795..db75313a94 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -1132,11 +1132,10 @@ async fn ensure_correct_block_fees_transfer() { let alice = get_alice_signing_key(); let bob_address = astria_address_from_hex_string(BOB_ADDRESS); - let value = 333_333; let actions = vec![ TransferAction { to: bob_address, - amount: value, + amount: 1000, asset: nria().into(), fee_asset: nria().into(), } @@ -1173,12 +1172,11 @@ async fn ensure_correct_block_fees_sequence() { app.apply(state_tx); let alice = get_alice_signing_key(); - let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let data = b"hello world".to_vec(); let actions = vec![ SequenceAction { - rollup_id, + rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), data: data.clone(), fee_asset: nria().into(), } @@ -1215,11 +1213,10 @@ async fn ensure_correct_block_fees_init_bridge_acct() { app.apply(state_tx); let alice = get_alice_signing_key(); - let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let actions = vec![ InitBridgeAccountAction { - rollup_id, + rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), asset: nria().into(), fee_asset: nria().into(), sudo_address: None, From 62fdf13653907f95ed4c2d11556ec3e3f40954e9 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Fri, 2 Aug 2024 11:57:48 -0500 Subject: [PATCH 14/23] Changes after rebase --- .../src/app/tests_execute_transaction.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index db75313a94..9ff94cfb2f 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -44,38 +44,25 @@ use crate::{ }, assets::StateReadExt as _, authority::StateReadExt as _, - bridge::{ - }, - app::test_utils::*, - assets::StateReadExt as _, - authority::StateReadExt as _, bridge::{ get_deposit_byte_len, StateReadExt as _, StateWriteExt as _, }, ibc::StateReadExt as _, - sequence::{ - }, - ibc::StateReadExt as _, sequence::{ calculate_fee_from_state, StateWriteExt as _, }, - test_utils::{ - }, test_utils::{ astria_address, astria_address_from_hex_string, nria, }, - transaction::{ - }, transaction::{ InvalidChainId, InvalidNonce, }, - }, }; /// XXX: This should be expressed in terms of `crate::app::test_utils::unchecked_genesis_state` to From 28571e3c128a3fe7dc6113bec90ad4a088faaf94 Mon Sep 17 00:00:00 2001 From: Ethan Oroshiba Date: Mon, 19 Aug 2024 13:57:33 -0500 Subject: [PATCH 15/23] Apply suggestions from code review Co-authored-by: noot <36753753+noot@users.noreply.github.com> --- .../astria-sequencer/src/app/tests_execute_transaction.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 9ff94cfb2f..1c9204964a 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -1143,7 +1143,7 @@ async fn ensure_correct_block_fees_transfer() { .state .get_block_fees() .await - .expect("failed to get block fees") + .expect("can get block fees") .into_iter() .map(|(_, fee)| fee) .sum(); @@ -1242,7 +1242,8 @@ async fn ensure_correct_block_fees_bridge_lock() { let mut app = initialize_app(None, vec![]).await; let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_transfer_base_fee(1).unwrap(); + let transfer_base_fee = 1; + state_tx.put_transfer_base_fee(transfer_base_fee).unwrap(); state_tx.put_bridge_lock_byte_cost_multiplier(1); state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx @@ -1287,7 +1288,7 @@ async fn ensure_correct_block_fees_bridge_lock() { .into_iter() .map(|(_, fee)| fee) .sum(); - let expected_fees = 1 + get_deposit_byte_len(&test_deposit); + let expected_fees = transfer_base_fee + get_deposit_byte_len(&test_deposit); assert_eq!(total_block_fees, expected_fees); } From 6ca77b6ae9ffaee9fc675639b6e20dc5c77cd872 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Mon, 19 Aug 2024 14:06:12 -0500 Subject: [PATCH 16/23] minor updates --- .../src/app/tests_execute_transaction.rs | 27 +++++++++++-------- .../src/ibc/ics20_withdrawal.rs | 3 ++- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 1c9204964a..ac946275f7 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -45,9 +45,7 @@ use crate::{ assets::StateReadExt as _, authority::StateReadExt as _, bridge::{ - get_deposit_byte_len, - StateReadExt as _, - StateWriteExt as _, + get_deposit_byte_len, init_bridge_account_action, StateReadExt as _, StateWriteExt as _ }, ibc::StateReadExt as _, sequence::{ @@ -1114,7 +1112,8 @@ async fn transaction_execution_records_fee_event() { async fn ensure_correct_block_fees_transfer() { let mut app = initialize_app(None, vec![]).await; let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_transfer_base_fee(1).unwrap(); + let transfer_base_fee = 1; + state_tx.put_transfer_base_fee(transfer_base_fee).unwrap(); app.apply(state_tx); let alice = get_alice_signing_key(); @@ -1147,7 +1146,7 @@ async fn ensure_correct_block_fees_transfer() { .into_iter() .map(|(_, fee)| fee) .sum(); - assert_eq!(total_block_fees, 1); + assert_eq!(total_block_fees, transfer_base_fee); } #[tokio::test] @@ -1196,7 +1195,8 @@ async fn ensure_correct_block_fees_sequence() { async fn ensure_correct_block_fees_init_bridge_acct() { let mut app = initialize_app(None, vec![]).await; let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_init_bridge_account_base_fee(1); + let init_bridge_account_base_fee = 1; + state_tx.put_init_bridge_account_base_fee(init_bridge_account_base_fee); app.apply(state_tx); let alice = get_alice_signing_key(); @@ -1230,7 +1230,7 @@ async fn ensure_correct_block_fees_init_bridge_acct() { .into_iter() .map(|(_, fee)| fee) .sum(); - assert_eq!(total_block_fees, 1); + assert_eq!(total_block_fees, init_bridge_account_base_fee); } #[tokio::test] @@ -1242,9 +1242,12 @@ async fn ensure_correct_block_fees_bridge_lock() { let mut app = initialize_app(None, vec![]).await; let mut state_tx = StateDelta::new(app.state.clone()); + let transfer_base_fee = 1; + let bridge_lock_byte_cost_multiplier = 1; + state_tx.put_transfer_base_fee(transfer_base_fee).unwrap(); - state_tx.put_bridge_lock_byte_cost_multiplier(1); + state_tx.put_bridge_lock_byte_cost_multiplier(bridge_lock_byte_cost_multiplier); state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); state_tx .put_bridge_account_ibc_asset(bridge_address, nria()) @@ -1288,7 +1291,7 @@ async fn ensure_correct_block_fees_bridge_lock() { .into_iter() .map(|(_, fee)| fee) .sum(); - let expected_fees = transfer_base_fee + get_deposit_byte_len(&test_deposit); + let expected_fees = transfer_base_fee + (get_deposit_byte_len(&test_deposit) * bridge_lock_byte_cost_multiplier); assert_eq!(total_block_fees, expected_fees); } @@ -1301,7 +1304,9 @@ async fn ensure_correct_block_fees_bridge_sudo_change() { let mut app = initialize_app(None, vec![]).await; let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_sudo_change_base_fee(1); + + let sudo_change_base_fee = 1; + state_tx.put_bridge_sudo_change_base_fee(sudo_change_base_fee); state_tx.put_bridge_account_sudo_address(bridge_address, alice_address); state_tx .increase_balance(bridge_address, nria(), 1) @@ -1337,7 +1342,7 @@ async fn ensure_correct_block_fees_bridge_sudo_change() { .into_iter() .map(|(_, fee)| fee) .sum(); - assert_eq!(total_block_fees, 1); + assert_eq!(total_block_fees, sudo_change_base_fee); } // TODO: Add test to ensure correct block fees for ICS20 withdrawal diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index f64aa25f33..74d6b7bebc 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -187,7 +187,8 @@ impl ActionHandler for action::Ics20Withdrawal { } state .get_and_increase_block_fees(self.fee_asset(), fee, Self::full_name()) - .await?; + .await + .context("failed to get and increase block fees")?; state .decrease_balance(from, self.denom(), self.amount()) From aacf3565c65a834480a82fdf2f2e269f8415cab5 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Mon, 19 Aug 2024 14:18:49 -0500 Subject: [PATCH 17/23] Correct expect statements --- .../src/app/tests_execute_transaction.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index ac946275f7..4bcbf43805 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -1142,7 +1142,7 @@ async fn ensure_correct_block_fees_transfer() { .state .get_block_fees() .await - .expect("can get block fees") + .expect("get block fees should succeed") .into_iter() .map(|(_, fee)| fee) .sum(); @@ -1183,7 +1183,7 @@ async fn ensure_correct_block_fees_sequence() { .state .get_block_fees() .await - .expect("failed to get block fees") + .expect("get block fees should succeed") .into_iter() .map(|(_, fee)| fee) .sum(); @@ -1226,7 +1226,7 @@ async fn ensure_correct_block_fees_init_bridge_acct() { .state .get_block_fees() .await - .expect("failed to get block fees") + .expect("get block fees should succeed") .into_iter() .map(|(_, fee)| fee) .sum(); @@ -1287,7 +1287,7 @@ async fn ensure_correct_block_fees_bridge_lock() { .state .get_block_fees() .await - .expect("failed to get block fees") + .expect("get block fees should succeed") .into_iter() .map(|(_, fee)| fee) .sum(); @@ -1338,7 +1338,7 @@ async fn ensure_correct_block_fees_bridge_sudo_change() { .state .get_block_fees() .await - .expect("failed to get block fees") + .expect("get block fees should succeed") .into_iter() .map(|(_, fee)| fee) .sum(); From 943f0c7bbb144e6b8d429fd40afcdef744bbe982 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Mon, 19 Aug 2024 14:19:16 -0500 Subject: [PATCH 18/23] rust fmt --- .../astria-sequencer/src/app/tests_execute_transaction.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 4bcbf43805..25c77b774c 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -45,7 +45,10 @@ use crate::{ assets::StateReadExt as _, authority::StateReadExt as _, bridge::{ - get_deposit_byte_len, init_bridge_account_action, StateReadExt as _, StateWriteExt as _ + get_deposit_byte_len, + init_bridge_account_action, + StateReadExt as _, + StateWriteExt as _, }, ibc::StateReadExt as _, sequence::{ @@ -1291,7 +1294,8 @@ async fn ensure_correct_block_fees_bridge_lock() { .into_iter() .map(|(_, fee)| fee) .sum(); - let expected_fees = transfer_base_fee + (get_deposit_byte_len(&test_deposit) * bridge_lock_byte_cost_multiplier); + let expected_fees = transfer_base_fee + + (get_deposit_byte_len(&test_deposit) * bridge_lock_byte_cost_multiplier); assert_eq!(total_block_fees, expected_fees); } From ce0b542e458cb4b5f3b4becf66b986387afaa865 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Mon, 19 Aug 2024 14:22:30 -0500 Subject: [PATCH 19/23] unwrap instead of expect --- .../src/app/tests_execute_transaction.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 25c77b774c..0d62d54a20 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -1145,7 +1145,7 @@ async fn ensure_correct_block_fees_transfer() { .state .get_block_fees() .await - .expect("get block fees should succeed") + .unwrap() .into_iter() .map(|(_, fee)| fee) .sum(); @@ -1186,7 +1186,7 @@ async fn ensure_correct_block_fees_sequence() { .state .get_block_fees() .await - .expect("get block fees should succeed") + .unwrap() .into_iter() .map(|(_, fee)| fee) .sum(); @@ -1229,7 +1229,7 @@ async fn ensure_correct_block_fees_init_bridge_acct() { .state .get_block_fees() .await - .expect("get block fees should succeed") + .unwrap() .into_iter() .map(|(_, fee)| fee) .sum(); @@ -1290,7 +1290,7 @@ async fn ensure_correct_block_fees_bridge_lock() { .state .get_block_fees() .await - .expect("get block fees should succeed") + .unwrap() .into_iter() .map(|(_, fee)| fee) .sum(); @@ -1342,7 +1342,7 @@ async fn ensure_correct_block_fees_bridge_sudo_change() { .state .get_block_fees() .await - .expect("get block fees should succeed") + .unwrap() .into_iter() .map(|(_, fee)| fee) .sum(); From 329e46e5d5278b5bffdb3c56b116cf47a434ee00 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Mon, 19 Aug 2024 14:30:05 -0500 Subject: [PATCH 20/23] Fix clippy --- crates/astria-sequencer/src/app/tests_execute_transaction.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 0d62d54a20..78b04c5c8e 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -46,7 +46,6 @@ use crate::{ authority::StateReadExt as _, bridge::{ get_deposit_byte_len, - init_bridge_account_action, StateReadExt as _, StateWriteExt as _, }, From 5281050a088e992cd8a12c7eae1b244979384076 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Tue, 20 Aug 2024 09:17:57 -0500 Subject: [PATCH 21/23] Fix changes from merge --- .../astria-sequencer/src/app/tests_execute_transaction.rs | 2 +- crates/astria-sequencer/src/authority/action.rs | 2 +- crates/astria-sequencer/src/bridge/bridge_lock_action.rs | 3 ++- .../src/bridge/bridge_sudo_change_action.rs | 3 +-- crates/astria-sequencer/src/bridge/bridge_unlock_action.rs | 4 +--- .../src/bridge/init_bridge_account_action.rs | 7 +++++-- crates/astria-sequencer/src/ibc/ibc_relayer_change.rs | 4 +--- crates/astria-sequencer/src/ibc/ics20_withdrawal.rs | 1 - 8 files changed, 12 insertions(+), 14 deletions(-) diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 7aa0237b63..488315b75b 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -1166,7 +1166,7 @@ async fn ensure_correct_block_fees_sequence() { let actions = vec![ SequenceAction { rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), - data: data.clone(), + data: data.clone().into(), fee_asset: nria().into(), } .into(), diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index c7c8e28548..24e8ba7d20 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -77,7 +77,7 @@ impl ActionHandler for ValidatorUpdate { #[async_trait::async_trait] impl ActionHandler for SudoAddressChangeAction { - async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + async fn check_stateless(&self) -> Result<()> { Ok(()) } diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index b1e00747bb..e1376502b9 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -24,6 +24,7 @@ use crate::{ }, address::StateReadExt as _, app::ActionHandler, + assets::StateWriteExt, bridge::{ StateReadExt as _, StateWriteExt as _, @@ -33,7 +34,7 @@ use crate::{ #[async_trait::async_trait] impl ActionHandler for BridgeLockAction { - async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + async fn check_stateless(&self) -> Result<()> { Ok(()) } 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 6b50ec3ec0..f496b06cb3 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -25,7 +25,7 @@ use crate::{ }; #[async_trait::async_trait] impl ActionHandler for BridgeSudoChangeAction { - async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + async fn check_stateless(&self) -> Result<()> { Ok(()) } @@ -108,7 +108,6 @@ mod tests { use super::*; use crate::{ address::StateWriteExt as _, - assets::StateWriteExt as _, test_utils::{ astria_address, ASTRIA_PREFIX, diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index e5d957cba1..ab515171bd 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -26,9 +26,7 @@ use crate::{ #[async_trait::async_trait] impl ActionHandler for BridgeUnlockAction { - type CheckStatelessContext = (); - - async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + async fn check_stateless(&self) -> Result<()> { Ok(()) } 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 6da3095159..6ace1ad1b2 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -18,7 +18,10 @@ use crate::{ }, address::StateReadExt as _, app::ActionHandler, - assets::StateReadExt as _, + assets::{ + StateReadExt as _, + StateWriteExt, + }, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -28,7 +31,7 @@ use crate::{ #[async_trait::async_trait] impl ActionHandler for InitBridgeAccountAction { - async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + async fn check_stateless(&self) -> Result<()> { Ok(()) } diff --git a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs index fbf8ff58f8..e454e509ee 100644 --- a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs +++ b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs @@ -19,9 +19,7 @@ use crate::{ #[async_trait] impl ActionHandler for IbcRelayerChangeAction { - type CheckStatelessContext = (); - - async fn check_stateless(&self, _context: Self::CheckStatelessContext) -> Result<()> { + async fn check_stateless(&self) -> Result<()> { Ok(()) } diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index d993f1271f..c242a791b6 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -30,7 +30,6 @@ use penumbra_ibc::component::packet::{ use crate::{ accounts::{ AddressBytes, - StateReadExt as _, StateWriteExt as _, }, address::StateReadExt as _, From a65407c7e5406db1b1357b6745babb820312c458 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Tue, 20 Aug 2024 10:49:25 -0500 Subject: [PATCH 22/23] Requested changes --- crates/astria-sequencer/src/accounts/action.rs | 2 +- crates/astria-sequencer/src/bridge/bridge_lock_action.rs | 2 +- crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs | 2 +- .../astria-sequencer/src/bridge/init_bridge_account_action.rs | 2 +- crates/astria-sequencer/src/ibc/ics20_withdrawal.rs | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index 60cbf3c150..8d15d1a606 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -6,7 +6,7 @@ use anyhow::{ use astria_core::{ primitive::v1::ADDRESS_LEN, protocol::transaction::v1alpha1::action::TransferAction, - Protobuf, + Protobuf as _, }; use cnidarium::{ StateRead, diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index e1376502b9..a1b3e47f53 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -9,7 +9,7 @@ use astria_core::{ TransferAction, }, sequencerblock::v1alpha1::block::Deposit, - Protobuf, + Protobuf as _, }; use cnidarium::StateWrite; 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 f496b06cb3..1ebe568d53 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -5,7 +5,7 @@ use anyhow::{ }; use astria_core::{ protocol::transaction::v1alpha1::action::BridgeSudoChangeAction, - Protobuf, + Protobuf as _, }; use cnidarium::StateWrite; 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 6ace1ad1b2..40c8c88b36 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -7,7 +7,7 @@ use anyhow::{ use astria_core::{ primitive::v1::Address, protocol::transaction::v1alpha1::action::InitBridgeAccountAction, - Protobuf, + Protobuf as _, }; use cnidarium::StateWrite; diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index c242a791b6..314d44c60f 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -10,7 +10,7 @@ use astria_core::{ Address, }, protocol::transaction::v1alpha1::action, - Protobuf, + Protobuf as _, }; use cnidarium::{ StateRead, From fbc99f08915897b1237db585163173bd42986306 Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Tue, 20 Aug 2024 11:33:55 -0500 Subject: [PATCH 23/23] Move fee tests to new module --- crates/astria-sequencer/src/app/mod.rs | 2 + .../src/app/tests_block_fees.rs | 335 ++++++++++++++++++ .../src/app/tests_execute_transaction.rs | 303 +--------------- .../src/bridge/bridge_lock_action.rs | 5 +- .../src/bridge/init_bridge_account_action.rs | 5 +- 5 files changed, 344 insertions(+), 306 deletions(-) create mode 100644 crates/astria-sequencer/src/app/tests_block_fees.rs diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index 13ec8393c4..be42077ca1 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -3,6 +3,8 @@ pub(crate) mod test_utils; #[cfg(test)] mod tests_app; #[cfg(test)] +mod tests_block_fees; +#[cfg(test)] mod tests_breaking_changes; #[cfg(test)] mod tests_execute_transaction; diff --git a/crates/astria-sequencer/src/app/tests_block_fees.rs b/crates/astria-sequencer/src/app/tests_block_fees.rs new file mode 100644 index 0000000000..4801e661ea --- /dev/null +++ b/crates/astria-sequencer/src/app/tests_block_fees.rs @@ -0,0 +1,335 @@ +use std::sync::Arc; + +use astria_core::{ + primitive::v1::RollupId, + protocol::transaction::v1alpha1::{ + action::{ + BridgeLockAction, + BridgeSudoChangeAction, + InitBridgeAccountAction, + SequenceAction, + TransferAction, + }, + TransactionParams, + UnsignedTransaction, + }, + sequencerblock::v1alpha1::block::Deposit, +}; +use cnidarium::StateDelta; +use tendermint::abci::EventAttributeIndexExt as _; + +use crate::{ + accounts::{ + StateReadExt as _, + StateWriteExt as _, + }, + app::test_utils::{ + get_alice_signing_key, + get_bridge_signing_key, + initialize_app, + BOB_ADDRESS, + }, + assets::StateReadExt as _, + bridge::{ + get_deposit_byte_len, + StateWriteExt as _, + }, + sequence::{ + calculate_fee_from_state, + StateWriteExt as _, + }, + test_utils::{ + astria_address, + astria_address_from_hex_string, + nria, + }, +}; + +#[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 = get_alice_signing_key(); + let bob_address = astria_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: nria().into(), + fee_asset: nria().into(), + } + .into(), + ], + }; + + let signed_tx = Arc::new(tx.into_signed(&alice)); + + 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", nria().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() + ); +} + +#[tokio::test] +async fn ensure_correct_block_fees_transfer() { + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + let transfer_base_fee = 1; + state_tx.put_transfer_base_fee(transfer_base_fee).unwrap(); + app.apply(state_tx); + + let alice = get_alice_signing_key(); + let bob_address = astria_address_from_hex_string(BOB_ADDRESS); + let actions = vec![ + TransferAction { + to: bob_address, + amount: 1000, + asset: nria().into(), + fee_asset: nria().into(), + } + .into(), + ]; + + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions, + }; + let signed_tx = Arc::new(tx.into_signed(&alice)); + app.execute_transaction(signed_tx).await.unwrap(); + + let total_block_fees: u128 = app + .state + .get_block_fees() + .await + .unwrap() + .into_iter() + .map(|(_, fee)| fee) + .sum(); + assert_eq!(total_block_fees, transfer_base_fee); +} + +#[tokio::test] +async fn ensure_correct_block_fees_sequence() { + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + state_tx.put_sequence_action_base_fee(1); + state_tx.put_sequence_action_byte_cost_multiplier(1); + app.apply(state_tx); + + let alice = get_alice_signing_key(); + let data = b"hello world".to_vec(); + + let actions = vec![ + SequenceAction { + rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), + data: data.clone().into(), + fee_asset: nria().into(), + } + .into(), + ]; + + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions, + }; + let signed_tx = Arc::new(tx.into_signed(&alice)); + app.execute_transaction(signed_tx).await.unwrap(); + + let total_block_fees: u128 = app + .state + .get_block_fees() + .await + .unwrap() + .into_iter() + .map(|(_, fee)| fee) + .sum(); + let expected_fees = calculate_fee_from_state(&data, &app.state).await.unwrap(); + assert_eq!(total_block_fees, expected_fees); +} + +#[tokio::test] +async fn ensure_correct_block_fees_init_bridge_acct() { + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + let init_bridge_account_base_fee = 1; + state_tx.put_init_bridge_account_base_fee(init_bridge_account_base_fee); + app.apply(state_tx); + + let alice = get_alice_signing_key(); + + let actions = vec![ + InitBridgeAccountAction { + rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), + asset: nria().into(), + fee_asset: nria().into(), + sudo_address: None, + withdrawer_address: None, + } + .into(), + ]; + + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions, + }; + let signed_tx = Arc::new(tx.into_signed(&alice)); + app.execute_transaction(signed_tx).await.unwrap(); + + let total_block_fees: u128 = app + .state + .get_block_fees() + .await + .unwrap() + .into_iter() + .map(|(_, fee)| fee) + .sum(); + assert_eq!(total_block_fees, init_bridge_account_base_fee); +} + +#[tokio::test] +async fn ensure_correct_block_fees_bridge_lock() { + let alice = get_alice_signing_key(); + let bridge = get_bridge_signing_key(); + let bridge_address = astria_address(&bridge.address_bytes()); + let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); + + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + + let transfer_base_fee = 1; + let bridge_lock_byte_cost_multiplier = 1; + + state_tx.put_transfer_base_fee(transfer_base_fee).unwrap(); + state_tx.put_bridge_lock_byte_cost_multiplier(bridge_lock_byte_cost_multiplier); + state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); + state_tx + .put_bridge_account_ibc_asset(bridge_address, nria()) + .unwrap(); + app.apply(state_tx); + + let actions = vec![ + BridgeLockAction { + to: bridge_address, + amount: 1, + asset: nria().into(), + fee_asset: nria().into(), + destination_chain_address: rollup_id.to_string(), + } + .into(), + ]; + + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions, + }; + let signed_tx = Arc::new(tx.into_signed(&alice)); + app.execute_transaction(signed_tx).await.unwrap(); + + let test_deposit = Deposit::new( + bridge_address, + rollup_id, + 1, + nria().into(), + rollup_id.to_string(), + ); + + let total_block_fees: u128 = app + .state + .get_block_fees() + .await + .unwrap() + .into_iter() + .map(|(_, fee)| fee) + .sum(); + let expected_fees = transfer_base_fee + + (get_deposit_byte_len(&test_deposit) * bridge_lock_byte_cost_multiplier); + assert_eq!(total_block_fees, expected_fees); +} + +#[tokio::test] +async fn ensure_correct_block_fees_bridge_sudo_change() { + let alice = get_alice_signing_key(); + let alice_address = astria_address(&alice.address_bytes()); + let bridge = get_bridge_signing_key(); + let bridge_address = astria_address(&bridge.address_bytes()); + + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + + let sudo_change_base_fee = 1; + state_tx.put_bridge_sudo_change_base_fee(sudo_change_base_fee); + state_tx.put_bridge_account_sudo_address(bridge_address, alice_address); + state_tx + .increase_balance(bridge_address, nria(), 1) + .await + .unwrap(); + app.apply(state_tx); + + let actions = vec![ + BridgeSudoChangeAction { + bridge_address, + new_sudo_address: None, + new_withdrawer_address: None, + fee_asset: nria().into(), + } + .into(), + ]; + + let tx = UnsignedTransaction { + params: TransactionParams::builder() + .nonce(0) + .chain_id("test") + .build(), + actions, + }; + let signed_tx = Arc::new(tx.into_signed(&alice)); + app.execute_transaction(signed_tx).await.unwrap(); + + let total_block_fees: u128 = app + .state + .get_block_fees() + .await + .unwrap() + .into_iter() + .map(|(_, fee)| fee) + .sum(); + assert_eq!(total_block_fees, sudo_change_base_fee); +} + +// TODO(https://github.com/astriaorg/astria/issues/1382): Add test to ensure correct block fees for ICS20 withdrawal diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 488315b75b..42c2704b57 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -9,10 +9,8 @@ use astria_core::{ protocol::transaction::v1alpha1::{ action::{ BridgeLockAction, - BridgeSudoChangeAction, BridgeUnlockAction, IbcRelayerChangeAction, - InitBridgeAccountAction, SequenceAction, SudoAddressChangeAction, TransferAction, @@ -32,13 +30,9 @@ use astria_core::{ use bytes::Bytes; use cnidarium::StateDelta; use penumbra_ibc::params::IBCParameters; -use tendermint::abci::EventAttributeIndexExt as _; use crate::{ - accounts::{ - StateReadExt as _, - StateWriteExt, - }, + accounts::StateReadExt as _, app::{ test_utils::*, ActionHandler as _, @@ -46,15 +40,11 @@ use crate::{ assets::StateReadExt as _, authority::StateReadExt as _, bridge::{ - get_deposit_byte_len, StateReadExt as _, StateWriteExt as _, }, ibc::StateReadExt as _, - sequence::{ - calculate_fee_from_state, - StateWriteExt as _, - }, + sequence::calculate_fee_from_state, test_utils::{ astria_address, astria_address_from_hex_string, @@ -1061,292 +1051,3 @@ 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 = get_alice_signing_key(); - let bob_address = astria_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: nria().into(), - fee_asset: nria().into(), - } - .into(), - ], - }; - - let signed_tx = Arc::new(tx.into_signed(&alice)); - - 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", nria().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() - ); -} - -#[tokio::test] -async fn ensure_correct_block_fees_transfer() { - let mut app = initialize_app(None, vec![]).await; - let mut state_tx = StateDelta::new(app.state.clone()); - let transfer_base_fee = 1; - state_tx.put_transfer_base_fee(transfer_base_fee).unwrap(); - app.apply(state_tx); - - let alice = get_alice_signing_key(); - let bob_address = astria_address_from_hex_string(BOB_ADDRESS); - let actions = vec![ - TransferAction { - to: bob_address, - amount: 1000, - asset: nria().into(), - fee_asset: nria().into(), - } - .into(), - ]; - - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions, - }; - let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); - - let total_block_fees: u128 = app - .state - .get_block_fees() - .await - .unwrap() - .into_iter() - .map(|(_, fee)| fee) - .sum(); - assert_eq!(total_block_fees, transfer_base_fee); -} - -#[tokio::test] -async fn ensure_correct_block_fees_sequence() { - let mut app = initialize_app(None, vec![]).await; - let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_sequence_action_base_fee(1); - state_tx.put_sequence_action_byte_cost_multiplier(1); - app.apply(state_tx); - - let alice = get_alice_signing_key(); - let data = b"hello world".to_vec(); - - let actions = vec![ - SequenceAction { - rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), - data: data.clone().into(), - fee_asset: nria().into(), - } - .into(), - ]; - - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions, - }; - let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); - - let total_block_fees: u128 = app - .state - .get_block_fees() - .await - .unwrap() - .into_iter() - .map(|(_, fee)| fee) - .sum(); - let expected_fees = calculate_fee_from_state(&data, &app.state).await.unwrap(); - assert_eq!(total_block_fees, expected_fees); -} - -#[tokio::test] -async fn ensure_correct_block_fees_init_bridge_acct() { - let mut app = initialize_app(None, vec![]).await; - let mut state_tx = StateDelta::new(app.state.clone()); - let init_bridge_account_base_fee = 1; - state_tx.put_init_bridge_account_base_fee(init_bridge_account_base_fee); - app.apply(state_tx); - - let alice = get_alice_signing_key(); - - let actions = vec![ - InitBridgeAccountAction { - rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), - asset: nria().into(), - fee_asset: nria().into(), - sudo_address: None, - withdrawer_address: None, - } - .into(), - ]; - - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions, - }; - let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); - - let total_block_fees: u128 = app - .state - .get_block_fees() - .await - .unwrap() - .into_iter() - .map(|(_, fee)| fee) - .sum(); - assert_eq!(total_block_fees, init_bridge_account_base_fee); -} - -#[tokio::test] -async fn ensure_correct_block_fees_bridge_lock() { - let alice = get_alice_signing_key(); - let bridge = get_bridge_signing_key(); - let bridge_address = astria_address(&bridge.address_bytes()); - let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); - - let mut app = initialize_app(None, vec![]).await; - let mut state_tx = StateDelta::new(app.state.clone()); - - let transfer_base_fee = 1; - let bridge_lock_byte_cost_multiplier = 1; - - state_tx.put_transfer_base_fee(transfer_base_fee).unwrap(); - state_tx.put_bridge_lock_byte_cost_multiplier(bridge_lock_byte_cost_multiplier); - state_tx.put_bridge_account_rollup_id(bridge_address, &rollup_id); - state_tx - .put_bridge_account_ibc_asset(bridge_address, nria()) - .unwrap(); - app.apply(state_tx); - - let actions = vec![ - BridgeLockAction { - to: bridge_address, - amount: 1, - asset: nria().into(), - fee_asset: nria().into(), - destination_chain_address: rollup_id.to_string(), - } - .into(), - ]; - - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions, - }; - let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); - - let test_deposit = Deposit::new( - bridge_address, - rollup_id, - 1, - nria().into(), - rollup_id.to_string(), - ); - - let total_block_fees: u128 = app - .state - .get_block_fees() - .await - .unwrap() - .into_iter() - .map(|(_, fee)| fee) - .sum(); - let expected_fees = transfer_base_fee - + (get_deposit_byte_len(&test_deposit) * bridge_lock_byte_cost_multiplier); - assert_eq!(total_block_fees, expected_fees); -} - -#[tokio::test] -async fn ensure_correct_block_fees_bridge_sudo_change() { - let alice = get_alice_signing_key(); - let alice_address = astria_address(&alice.address_bytes()); - let bridge = get_bridge_signing_key(); - let bridge_address = astria_address(&bridge.address_bytes()); - - let mut app = initialize_app(None, vec![]).await; - let mut state_tx = StateDelta::new(app.state.clone()); - - let sudo_change_base_fee = 1; - state_tx.put_bridge_sudo_change_base_fee(sudo_change_base_fee); - state_tx.put_bridge_account_sudo_address(bridge_address, alice_address); - state_tx - .increase_balance(bridge_address, nria(), 1) - .await - .unwrap(); - app.apply(state_tx); - - let actions = vec![ - BridgeSudoChangeAction { - bridge_address, - new_sudo_address: None, - new_withdrawer_address: None, - fee_asset: nria().into(), - } - .into(), - ]; - - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions, - }; - let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); - - let total_block_fees: u128 = app - .state - .get_block_fees() - .await - .unwrap() - .into_iter() - .map(|(_, fee)| fee) - .sum(); - assert_eq!(total_block_fees, sudo_change_base_fee); -} - -// TODO: Add test to ensure correct block fees for ICS20 withdrawal diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index a1b3e47f53..b2375aae24 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -24,7 +24,7 @@ use crate::{ }, address::StateReadExt as _, app::ActionHandler, - assets::StateWriteExt, + assets::StateWriteExt as _, bridge::{ StateReadExt as _, StateWriteExt as _, @@ -161,8 +161,7 @@ mod tests { use super::*; use crate::{ - address::StateWriteExt, - assets::StateWriteExt as _, + address::StateWriteExt as _, test_utils::{ assert_anyhow_error, astria_address, 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 40c8c88b36..bb1ae84d25 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -20,7 +20,7 @@ use crate::{ app::ActionHandler, assets::{ StateReadExt as _, - StateWriteExt, + StateWriteExt as _, }, bridge::state_ext::{ StateReadExt as _, @@ -104,7 +104,8 @@ impl ActionHandler for InitBridgeAccountAction { ); state .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) - .await?; + .await + .context("failed to get and increase block fees")?; state .decrease_balance(from, &self.fee_asset, fee) .await