From 9f959f4fc492599ffc4a0bfa0d6e29d26b097b4e Mon Sep 17 00:00:00 2001 From: Richard Janis Goldschmidt Date: Tue, 20 Aug 2024 12:32:49 +0100 Subject: [PATCH] fix(sequencer)!: fix TOCTOU issues by merging check and execution (#1332) ## Summary Introduces `ActionHandler::check_and_execute`, replacing `ActionHandler::check_stateful` and `ActionHandler::execute`. ## Background Zelic found that separating execution into `check_stateful` and `execute` lead to time-of-check-vs-time-of-use risks: while `check_stateful` for two actions `A` and `B` might each pass, the execution of action `A` might change the state such that a subsequent execution of action `B` would now fail - or worse, lead to invalid state. This patch follows Penumbra (see issues linked at the bottom) in merging them into one atomic operation (atomic in the sense that `::check_and_execute` are run sequentially). There is also a `ActionHandler::check_historic` trait method, which however is currently not used. It is left for future work to go through the individual checks and move them to `check_historic`, where applicable (for example, checking address prefixes as long as changing these is not possible after chain init). ## Changes - change `ActionHandler` trait to merge `check_stateful` and `execute` into `check_and_execute`. - inject a transaction's signer into the ephemeral object store, setting before and after a transaction is executed. Necessary because this follows the `cnidarium_component::ActionHandler` convention, but also allows simplifying - remove the notion of bech32m addresses from many state writes and reads: the prefix only matters at the boundary, not inside the system ## Testing All tests were updated and pass. NOTE: a useful test would be to craft a problematic transaction that would be rejected with the newest change. However, crafting and executing such a transaction so that it is rejected by the current sequencer but leads to incorrect is left to a follow-up. ## Breaking Changelist While no snapshots guarding against breaking app state were triggered, this is still a breaking change: if supplied with a problematic payload, a sequencer node without this patch will reach a different (and invalid) state compared a node with the present patch. ## Related Issues Original Penumbra issues and fix: https://github.com/penumbra-zone/penumbra/issues/3960 https://github.com/penumbra-zone/penumbra/issues/3960 Closes #1318 --------- Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com> --- Cargo.lock | 1 - crates/astria-sequencer/Cargo.toml | 1 - .../astria-sequencer/src/accounts/action.rs | 193 +++++------ crates/astria-sequencer/src/accounts/mod.rs | 54 ++++ .../src/accounts/state_ext.rs | 88 ++--- .../src/app/action_handler.rs | 25 ++ crates/astria-sequencer/src/app/mod.rs | 40 +-- crates/astria-sequencer/src/app/tests_app.rs | 10 +- .../src/app/tests_breaking_changes.rs | 4 +- .../src/app/tests_execute_transaction.rs | 33 +- .../astria-sequencer/src/authority/action.rs | 150 ++++----- .../src/authority/state_ext.rs | 29 +- .../src/bridge/bridge_lock_action.rs | 151 +++------ .../src/bridge/bridge_sudo_change_action.rs | 99 +++--- .../src/bridge/bridge_unlock_action.rs | 292 +++++------------ .../src/bridge/init_bridge_account_action.rs | 50 ++- crates/astria-sequencer/src/bridge/query.rs | 48 ++- .../astria-sequencer/src/bridge/state_ext.rs | 165 +++++----- .../astria-sequencer/src/fee_asset_change.rs | 35 +- .../src/ibc/ibc_relayer_change.rs | 29 +- .../src/ibc/ics20_transfer.rs | 26 +- .../src/ibc/ics20_withdrawal.rs | 128 ++++---- crates/astria-sequencer/src/ibc/state_ext.rs | 60 ++-- crates/astria-sequencer/src/mempool/mod.rs | 25 +- .../astria-sequencer/src/sequence/action.rs | 66 ++-- crates/astria-sequencer/src/sequencer.rs | 17 - .../astria-sequencer/src/service/mempool.rs | 3 +- crates/astria-sequencer/src/test_utils.rs | 9 + .../src/transaction/action_handler.rs | 24 -- .../src/transaction/checks.rs | 73 ++--- .../astria-sequencer/src/transaction/mod.rs | 304 ++++++------------ .../src/transaction/state_ext.rs | 51 +++ 32 files changed, 992 insertions(+), 1291 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 74f5d556b8..d6bea69d74 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..60cbf3c150 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -4,31 +4,123 @@ 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::AddressBytes; 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( +#[async_trait::async_trait] +impl ActionHandler for TransferAction { + async fn check_stateless(&self) -> Result<()> { + Ok(()) + } + + 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(); + + 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 execute_transfer( + action: &TransferAction, + from: [u8; ADDRESS_LEN], + mut state: S, +) -> anyhow::Result<()> { + 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")?; + + // 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 decreasing `from` account balance")?; + state + .increase_balance(action.to, &action.asset, action.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, &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")?; + + // 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(()) +} + +pub(crate) async fn check_transfer( action: &TransferAction, + from: TAddress, state: &S, - from: Address, ) -> Result<()> where - S: accounts::StateReadExt + assets::StateReadExt + 'static, + S: StateRead, + TAddress: AddressBytes, { + 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) @@ -44,7 +136,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")?; @@ -80,84 +172,3 @@ where Ok(()) } - -#[async_trait::async_trait] -impl ActionHandler for TransferAction { - async fn check_stateless(&self) -> 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", - ); - - transfer_check_stateful(self, state, from) - .await - .context("stateful transfer check failed") - } - - #[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() - .await - .context("failed to get transfer base fee")?; - state - .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) - .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")?; - } - - Ok(()) - } -} diff --git a/crates/astria-sequencer/src/accounts/mod.rs b/crates/astria-sequencer/src/accounts/mod.rs index ae0b420808..ddb9fe68a9 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 AddressBytes: Send + Sync { + fn address_bytes(&self) -> [u8; ADDRESS_LEN]; +} + +impl AddressBytes for Address { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.bytes() + } +} + +impl AddressBytes for [u8; ADDRESS_LEN] { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + *self + } +} + +impl AddressBytes for SignedTransaction { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes() + } +} + +impl AddressBytes for SigningKey { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes() + } +} + +impl AddressBytes for VerificationKey { + fn address_bytes(&self) -> [u8; ADDRESS_LEN] { + self.address_bytes() + } +} + +impl<'a, T> AddressBytes for &'a T +where + T: AddressBytes, +{ + 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 fa0fe00a6c..e8a60f3d03 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::AddressBytes; + /// Newtype wrapper to read and write a u32 from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] struct Nonce(u32); @@ -41,57 +38,20 @@ 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> { +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>( - address: Address, +fn balance_storage_key>( + address: TAddress, asset: TAsset, ) -> String { format!( @@ -101,7 +61,7 @@ fn balance_storage_key>( ) } -fn nonce_storage_key(address: T) -> String { +fn nonce_storage_key(address: T) -> String { format!("{}/nonce", StorageKey(&address)) } @@ -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: AddressBytes, TAsset: Into + std::fmt::Display + Send, { let Some(bytes) = self @@ -177,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 @@ -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: AddressBytes, 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: AddressBytes, 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: AddressBytes, 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..180f592d39 --- /dev/null +++ b/crates/astria-sequencer/src/app/action_handler.rs @@ -0,0 +1,25 @@ +use cnidarium::StateWrite; + +/// This trait is a verbatim copy of `cnidarium_component::ActionHandler`. +/// +/// It's duplicated here because all actions are foreign types, forbidding +/// the implementation of [`cnidarium_component::ActionHandler`][1] for +/// these types due to Rust orphan rules. +/// +/// [1]: https://github.com/penumbra-zone/penumbra/blob/14959350abcb8cfbf33f9aedc7463fccfd8e3f9f/crates/cnidarium-component/src/action_handler.rs#L30 +#[async_trait::async_trait] +pub(crate) trait ActionHandler { + // Commenting out for the time being as this is currentl nonot being used. Leaving this in + // for reference as this is copied from cnidarium_component. + // ``` + // 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_stateless(&self) -> anyhow::Result<()>; + + 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 fc605cd717..13ec8393c4 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::{ @@ -59,7 +60,6 @@ use tracing::{ debug, info, instrument, - Instrument as _, }; use crate::{ @@ -106,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,46 +979,29 @@ impl App { &mut self, 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(), - ); - - stateless + signed_tx + .check_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 .state .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/app/tests_app.rs b/crates/astria-sequencer/src/app/tests_app.rs index 195b810999..4e6b6dbfe6 100644 --- a/crates/astria-sequencer/src/app/tests_app.rs +++ b/crates/astria-sequencer/src/app/tests_app.rs @@ -295,9 +295,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(); @@ -385,9 +385,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(); @@ -682,7 +682,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_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 8c1bb2d097..e135b55f0b 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -107,9 +107,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 e2d3449c78..6a39904305 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -34,7 +34,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::{ @@ -357,7 +360,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] @@ -384,7 +387,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] @@ -436,7 +439,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] @@ -600,7 +603,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(), @@ -608,7 +611,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(), @@ -678,9 +681,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); @@ -879,8 +882,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(); @@ -940,7 +941,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() @@ -964,7 +966,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"); } @@ -991,11 +994,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/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index baf6220db9..24e8ba7d20 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -4,30 +4,39 @@ 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<()> { + async fn check_stateless(&self) -> 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 +61,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() @@ -82,11 +83,11 @@ impl ActionHandler for SudoAddressChangeAction { /// 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 +98,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 +107,23 @@ impl ActionHandler for SudoAddressChangeAction { #[async_trait::async_trait] impl ActionHandler for FeeChangeAction { + async fn check_stateless(&self) -> 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 => { @@ -172,31 +161,28 @@ mod test { use super::*; use crate::{ - accounts::{ - StateReadExt as _, + accounts::StateReadExt as _, + bridge::StateReadExt as _, + ibc::StateReadExt as _, + sequence::StateReadExt as _, + transaction::{ StateWriteExt as _, + TransactionContext, }, - bridge::{ - StateReadExt as _, - StateWriteExt as _, - }, - ibc::{ - StateReadExt as _, - StateWriteExt as _, - }, - sequence::{ - StateReadExt as _, - StateWriteExt as _, - }, - test_utils::astria_address, }; #[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 { @@ -204,10 +190,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; @@ -218,10 +201,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; @@ -232,10 +212,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() @@ -252,10 +229,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; @@ -266,10 +240,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 @@ -285,10 +256,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 afdb4856a8..f98d18a78f 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,7 +18,7 @@ use cnidarium::{ use tracing::instrument; use super::ValidatorSet; -use crate::address; +use crate::accounts::AddressBytes; /// Newtype wrapper to read and write an address from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -32,9 +29,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 +42,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)] @@ -88,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.address_bytes())) .context("failed to convert sudo address to vec")?, ); Ok(()) @@ -126,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::{ @@ -137,7 +135,6 @@ mod tests { address::StateWriteExt as _, authority::ValidatorSet, test_utils::{ - astria_address, verification_key, ASTRIA_PREFIX, }, @@ -162,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"); @@ -176,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 2326593150..880298fb5f 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -4,31 +4,30 @@ 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, + action::{ + check_transfer, + execute_transfer, + }, 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] @@ -37,31 +36,24 @@ impl ActionHandler for BridgeLockAction { 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 +87,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 +94,15 @@ impl ActionHandler for BridgeLockAction { fee_asset: self.fee_asset.clone(), }; - transfer_action - .execute(state, from) - .await - .context("failed to execute bridge lock action as transfer action")?; + check_transfer(&transfer_action, from, &state).await?; + // Executes the transfer and deducts transfer feeds. + // FIXME: This is a very roundabout way of paying for fees. IMO it would be + // better to just duplicate this entire logic here so that we don't call out + // to the transfer-action logic. + execute_transfer(&transfer_action, from, &mut state).await?; 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"); @@ -127,8 +115,11 @@ impl ActionHandler for BridgeLockAction { self.destination_chain_address.clone(), ); - // the transfer fee is already deducted in `transfer_action.execute()`, + // the transfer fee is already deducted in `execute_transfer() above, // so we just deduct the bridge lock byte multiplier fee. + // FIXME: similar to what is mentioned there: this should be reworked so that + // the fee deducation logic for these actions are defined fully independently + // (even at the cost of duplicating code). let byte_cost_multiplier = state .get_bridge_lock_byte_cost_multiplier() .await @@ -157,7 +148,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, @@ -166,11 +156,17 @@ mod tests { use super::*; use crate::{ + address::StateWriteExt, assets::StateWriteExt as _, test_utils::{ + assert_anyhow_error, astria_address, ASTRIA_PREFIX, }, + transaction::{ + StateWriteExt as _, + TransactionContext, + }, }; fn test_asset() -> asset::Denom { @@ -178,73 +174,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_stateful(&state, from_address) - .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_stateful(&state, from_address) - .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); @@ -259,25 +200,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 - .execute(&mut state, from_address) - .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 @@ -292,6 +227,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 518a9417e9..483adf9af5 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,31 @@ 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<()> { 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 @@ -62,7 +55,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 { @@ -76,11 +69,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 @@ -91,11 +79,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(()) @@ -104,17 +92,21 @@ 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, ASTRIA_PREFIX, }, + transaction::{ + StateWriteExt as _, + TransactionContext, + }, }; fn test_asset() -> asset::Denom { @@ -122,11 +114,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(); @@ -134,32 +129,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_stateful(&state, sudo_address).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, @@ -170,7 +140,7 @@ mod tests { assert!( action - .check_stateful(&state, bridge_address) + .check_and_execute(state) .await .unwrap_err() .to_string() @@ -179,16 +149,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 @@ -202,21 +181,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) + .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) + .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 550cf5dce4..ab515171bd 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -11,17 +11,17 @@ use astria_core::{ TransferAction, }, }; -use tracing::instrument; +use cnidarium::StateWrite; use crate::{ - accounts::action::transfer_check_stateful, - address, - bridge::StateReadExt as _, - state_ext::{ - StateReadExt, - StateWriteExt, + accounts::action::{ + check_transfer, + execute_transfer, }, - transaction::action_handler::ActionHandler, + address::StateReadExt as _, + app::ActionHandler, + bridge::StateReadExt as _, + transaction::StateReadExt as _, }; #[async_trait::async_trait] @@ -30,11 +30,11 @@ impl ActionHandler for BridgeUnlockAction { Ok(()) } - async fn check_stateful( - &self, - state: &S, - from: Address, - ) -> 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") + .address_bytes(); state .ensure_base_prefix(&self.to) .await @@ -48,17 +48,16 @@ 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, Address::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 { @@ -77,31 +76,8 @@ 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")?; + check_transfer(&transfer_action, bridge_address, &state).await?; + execute_transfer(&transfer_action, bridge_address, state).await?; Ok(()) } @@ -109,22 +85,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 { @@ -132,17 +116,19 @@ 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(); let transfer_amount = 100; - let address = astria_address(&[1; 20]); let to_address = astria_address(&[2; 20]); let bridge_unlock = BridgeUnlockAction { @@ -156,7 +142,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() @@ -165,24 +151,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 sender_address = astria_address(&[1; 20]); 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) + .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, @@ -193,35 +180,32 @@ mod test { }; // invalid sender, doesn't match action's `from`, should fail - assert!( - bridge_unlock - .check_stateful(&state, sender_address) - .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 sender_address = astria_address(&[1; 20]); 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 { @@ -233,77 +217,22 @@ mod test { }; // invalid sender, doesn't match action's bridge account's withdrawer, should fail - assert!( - bridge_unlock - .check_stateful(&state, sender_address) - .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 fee_check_stateful_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); - 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_current_source(TransactionContext { + address_bytes: bridge_address.bytes(), + }); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); let asset = test_asset(); @@ -311,69 +240,14 @@ mod test { 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(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - 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 { @@ -388,44 +262,46 @@ mod test { state .put_account_balance(bridge_address, &asset, transfer_amount) .unwrap(); - assert!( - bridge_unlock - .execute(&mut state, bridge_address) + 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 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] - 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 { @@ -440,22 +316,18 @@ mod test { state .put_account_balance(bridge_address, &asset, transfer_amount) .unwrap(); - assert!( - bridge_unlock - .execute(&mut state, bridge_address) + 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 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/bridge/init_bridge_account_action.rs b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs index 986a314bfe..0dbbd07c78 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -8,24 +8,21 @@ use astria_core::{ primitive::v1::Address, protocol::transaction::v1alpha1::action::InitBridgeAccountAction, }; -use tracing::instrument; +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] @@ -34,11 +31,11 @@ impl ActionHandler for InitBridgeAccountAction { 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) @@ -73,7 +70,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"); } @@ -87,23 +89,15 @@ 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_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.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, Address::bytes)); + state.put_bridge_account_withdrawer_address( + from, + self.withdrawer_address.map_or(from, Address::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 60fa07c2a2..7807ebb188 100644 --- a/crates/astria-sequencer/src/bridge/query.rs +++ b/crates/astria-sequencer/src/bridge/query.rs @@ -15,6 +15,7 @@ use tendermint::abci::{ }; use crate::{ + address::StateReadExt, assets::StateReadExt as _, bridge::StateReadExt as _, state_ext::StateReadExt as _, @@ -37,11 +38,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 +59,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,8 +88,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, @@ -102,8 +106,20 @@ async fn get_bridge_account_info( } }; - let withdrawer_address = match snapshot - .get_bridge_account_withdrawer_address(&address) + 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 { Ok(Some(withdrawer_address)) => withdrawer_address, @@ -123,6 +139,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(), @@ -294,15 +322,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 3b8ba0b5f6..0316603c09 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::AddressBytes, + 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: 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.bytes() { + for byte in self.address.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,7 @@ 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 +150,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 +169,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 +184,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 +220,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 +361,55 @@ 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: AddressBytes, 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: AddressBytes, + TSudoAddress: AddressBytes, + { 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.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: AddressBytes, + TWithdrawerAddress: AddressBytes, + { 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.address_bytes().to_vec(), ); } @@ -465,13 +486,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(), ); } @@ -520,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, @@ -538,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"), @@ -551,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"), @@ -565,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"), @@ -578,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"), @@ -595,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"); } @@ -611,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!( @@ -626,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!( @@ -642,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/fee_asset_change.rs b/crates/astria-sequencer/src/fee_asset_change.rs index 7ccdfdf804..d496b6581d 100644 --- a/crates/astria-sequencer/src/fee_asset_change.rs +++ b/crates/astria-sequencer/src/fee_asset_change.rs @@ -4,26 +4,31 @@ 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<()> { + async fn check_stateless(&self) -> 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 +37,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..e454e509ee 100644 --- a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs +++ b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs @@ -3,16 +3,18 @@ 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] @@ -21,11 +23,11 @@ impl ActionHandler for IbcRelayerChangeAction { 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 +44,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_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 859ca1d920..3dcbb3bfb8 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -12,6 +12,10 @@ use astria_core::{ }, protocol::transaction::v1alpha1::action, }; +use cnidarium::{ + StateRead, + StateWrite, +}; use ibc_types::core::channel::{ ChannelId, PortId, @@ -22,17 +26,21 @@ use penumbra_ibc::component::packet::{ SendPacketWrite as _, Unchecked, }; -use tracing::instrument; use crate::{ - accounts, - address, + accounts::{ + AddressBytes, + 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 +59,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` @@ -65,7 +73,7 @@ async fn ics20_withdrawal_check_stateful_bridge_account Result<()> { ensure!(self.timeout_time() != 0, "timeout time must be non-zero",); @@ -106,12 +113,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 +130,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 +182,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 +194,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 +211,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(()) } } @@ -236,13 +226,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, @@ -257,13 +247,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(), @@ -285,12 +275,12 @@ 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, + 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 { @@ -298,7 +288,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(), @@ -320,12 +310,12 @@ 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, + 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 { @@ -333,7 +323,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(), @@ -359,21 +349,21 @@ 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, + 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 { 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(), @@ -395,21 +385,21 @@ 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, + 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 { 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(), @@ -434,15 +424,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 3abde83481..880f86f6ba 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; @@ -23,7 +22,7 @@ use tracing::{ instrument, }; -use crate::address; +use crate::accounts::AddressBytes; /// Newtype wrapper to read and write a u128 from rocksdb. #[derive(BorshSerialize, BorshDeserialize, Debug)] @@ -40,13 +39,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: 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.bytes() { + for byte in self.0.address_bytes() { f.write_fmt(format_args!("{byte:02x}"))?; } Ok(()) @@ -63,12 +62,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 +90,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 +101,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()) @@ -151,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.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)] @@ -234,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"); @@ -248,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"); @@ -274,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" @@ -291,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" @@ -321,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" @@ -332,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/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); diff --git a/crates/astria-sequencer/src/sequence/action.rs b/crates/astria-sequencer/src/sequence/action.rs index 5b52bfe760..835386f65e 100644 --- a/crates/astria-sequencer/src/sequence/action.rs +++ b/crates/astria-sequencer/src/sequence/action.rs @@ -4,49 +4,27 @@ 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", - ); - - 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<()> { // TODO: do we want to place a maximum on the size of the data? // https://github.com/astriaorg/astria/issues/222 @@ -57,14 +35,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/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 diff --git a/crates/astria-sequencer/src/service/mempool.rs b/crates/astria-sequencer/src/service/mempool.rs index 6efae965e8..838578cbd4 100644 --- a/crates/astria-sequencer/src/service/mempool.rs +++ b/crates/astria-sequencer/src/service/mempool.rs @@ -41,6 +41,7 @@ use tracing::{ use crate::{ accounts, address, + app::ActionHandler as _, mempool::{ Mempool as AppMempool, RemovalReason, @@ -171,7 +172,7 @@ async fn handle_check_tx 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(); + assert!( + msg.contains(expected), + "error contained different message\n\texpected: {expected}\n\tfull_error: {msg}", + ); +} diff --git a/crates/astria-sequencer/src/transaction/action_handler.rs b/crates/astria-sequencer/src/transaction/action_handler.rs deleted file mode 100644 index 17cfb7345b..0000000000 --- a/crates/astria-sequencer/src/transaction/action_handler.rs +++ /dev/null @@ -1,24 +0,0 @@ -use anyhow::Result; -use astria_core::primitive::v1::Address; -use async_trait::async_trait; -use cnidarium::{ - StateRead, - StateWrite, -}; - -#[async_trait] -pub(crate) trait ActionHandler { - async fn check_stateless(&self) -> 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 e73e18acf7..77aad88236 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")?; @@ -315,10 +295,6 @@ fn bridge_unlock_update_fees( #[cfg(test)] mod tests { - use address::{ - StateReadExt, - StateWriteExt, - }; use astria_core::{ primitive::v1::{ asset::Denom, @@ -338,8 +314,9 @@ mod tests { use super::*; use crate::{ - accounts::{ - StateReadExt as _, + accounts::StateWriteExt as _, + address::{ + StateReadExt, StateWriteExt as _, }, app::test_utils::*, diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index 1426a91804..0d89e68452 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,26 @@ pub(crate) use checks::{ check_chain_id_mempool, check_nonce_mempool, }; -use tracing::instrument; +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, +}; 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 +45,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,11 +76,11 @@ impl fmt::Display for InvalidNonce { impl std::error::Error for InvalidNonce {} #[async_trait::async_trait] -impl ActionHandler for UnsignedTransaction { +impl ActionHandler for SignedTransaction { async fn check_stateless(&self) -> anyhow::Result<()> { - ensure!(!self.actions.is_empty(), "must have at least one action"); + 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() @@ -193,10 +144,16 @@ impl ActionHandler for UnsignedTransaction { Ok(()) } - async fn check_stateful(&self, state: &S, from: Address) -> anyhow::Result<()> - where - S: accounts::StateReadExt + 'static, - { + // 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 + // 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 +163,116 @@ 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 nonce of transaction signer")?; + 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")?; + + // FIXME: this should create one span per `check_and_execute` + 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")?, + .context("executing transfer action failed")?, Action::Sequence(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for SequenceAction")?, + .context("executing sequence action failed")?, Action::ValidatorUpdate(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for ValidatorUpdateAction")?, + .context("executing validor update")?, Action::SudoAddressChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for SudoAddressChangeAction")?, + .context("executing sudo address change failed")?, Action::FeeChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for FeeChangeAction")?, - Action::Ibc(_) => { + .context("executing fee change failed")?, + Action::Ibc(act) => { + // 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(&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("failed executing ibc action")?; } Action::Ics20Withdrawal(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for Ics20WithdrawalAction")?, + .context("failed executing ics20 withdrawal")?, Action::IbcRelayerChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for IbcRelayerChangeAction")?, + .context("failed executing ibc relayer change")?, Action::FeeAssetChange(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for FeeAssetChangeAction")?, + .context("failed executing fee asseet change")?, Action::InitBridgeAccount(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for InitBridgeAccountAction")?, + .context("failed executing init bridge account")?, Action::BridgeLock(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for BridgeLockAction")?, + .context("failed executing bridge lock")?, Action::BridgeUnlock(act) => act - .check_stateful(state, from) + .check_and_execute(&mut state) .await - .context("stateful check failed for BridgeUnlockAction")?, + .context("failed executing bridge unlock")?, 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")?; - } + .context("failed executing bridge sudo change")?, } } + // 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 new file mode 100644 index 0000000000..4304505b76 --- /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 { + pub(crate) 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: impl Into) { + let context: TransactionContext = transaction.into(); + self.object_put(current_source(), context); + } + + 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 {}