From fe21bcb4bcec36bfcc1a18e3f0fdc6cafe448fb2 Mon Sep 17 00:00:00 2001 From: Fraser Hutchison Date: Mon, 30 Sep 2024 23:04:21 +0100 Subject: [PATCH] add context to errors --- crates/astria-sequencer/src/app/mod.rs | 15 ++++--- .../astria-sequencer/src/authority/action.rs | 40 ++++++++++--------- .../src/bridge/bridge_sudo_change_action.rs | 9 +++-- .../astria-sequencer/src/bridge/component.rs | 20 ++++++---- .../src/bridge/init_bridge_account_action.rs | 24 +++++------ crates/astria-sequencer/src/grpc/state_ext.rs | 19 ++++++--- crates/astria-sequencer/src/ibc/component.rs | 5 ++- .../src/ibc/ibc_relayer_change.rs | 6 +-- .../src/sequence/component.rs | 18 ++++++--- .../astria-sequencer/src/transaction/mod.rs | 11 ++--- 10 files changed, 98 insertions(+), 69 deletions(-) diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index 6481f984e..b1372d92c 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -945,8 +945,9 @@ impl App { .get_block_height() .await .expect("block height must be set, as `put_block_height` was already called"); - // No need to add context as this method already reports sufficient context on error. - state.put_storage_version_by_height(height, new_version)?; + state + .put_storage_version_by_height(height, new_version) + .wrap_err("failed to put storage version by height")?; debug!( height, version = new_version, @@ -975,10 +976,12 @@ impl App { ) -> Result> { let mut state_tx = StateDelta::new(self.state.clone()); - // No need to add context as this method already reports sufficient context on error. - state_tx.put_block_height(begin_block.header.height.into())?; - // No need to add context as this method already reports sufficient context on error. - state_tx.put_block_timestamp(begin_block.header.time)?; + state_tx + .put_block_height(begin_block.header.height.into()) + .wrap_err("failed to put block height")?; + state_tx + .put_block_timestamp(begin_block.header.time) + .wrap_err("failed to put block timestamp")?; // call begin_block on all components let mut arc_state_tx = Arc::new(state_tx); diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index 53f37d17e..ac044f601 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -126,26 +126,28 @@ impl ActionHandler for FeeChangeAction { .wrap_err("failed to get sudo address from state")?; ensure!(sudo_address == from, "signer is not the sudo key"); - // No need to add context to any of these `put` calls, as they already report sufficient - // context on error. match self.fee_change { - FeeChange::TransferBaseFee => state.put_transfer_base_fee(self.new_value), - FeeChange::SequenceBaseFee => state.put_sequence_action_base_fee(self.new_value), - FeeChange::SequenceByteCostMultiplier => { - state.put_sequence_action_byte_cost_multiplier(self.new_value) - } - FeeChange::InitBridgeAccountBaseFee => { - state.put_init_bridge_account_base_fee(self.new_value) - } - FeeChange::BridgeLockByteCostMultiplier => { - state.put_bridge_lock_byte_cost_multiplier(self.new_value) - } - FeeChange::BridgeSudoChangeBaseFee => { - state.put_bridge_sudo_change_base_fee(self.new_value) - } - FeeChange::Ics20WithdrawalBaseFee => { - state.put_ics20_withdrawal_base_fee(self.new_value) - } + FeeChange::TransferBaseFee => state + .put_transfer_base_fee(self.new_value) + .wrap_err("failed to put transfer base fee"), + FeeChange::SequenceBaseFee => state + .put_sequence_action_base_fee(self.new_value) + .wrap_err("failed to put sequence action base fee"), + FeeChange::SequenceByteCostMultiplier => state + .put_sequence_action_byte_cost_multiplier(self.new_value) + .wrap_err("failed to put sequence action byte cost multiplier"), + FeeChange::InitBridgeAccountBaseFee => state + .put_init_bridge_account_base_fee(self.new_value) + .wrap_err("failed to put init bridge account base fee"), + FeeChange::BridgeLockByteCostMultiplier => state + .put_bridge_lock_byte_cost_multiplier(self.new_value) + .wrap_err("failed to put bridge lock byte cost multiplier"), + FeeChange::BridgeSudoChangeBaseFee => state + .put_bridge_sudo_change_base_fee(self.new_value) + .wrap_err("failed to put bridge sudo change base fee"), + FeeChange::Ics20WithdrawalBaseFee => state + .put_ics20_withdrawal_base_fee(self.new_value) + .wrap_err("failed to put ics20 withdrawal base fee"), } } } 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 52af80fbb..4d626ce69 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -90,14 +90,15 @@ impl ActionHandler for BridgeSudoChangeAction { .wrap_err("failed to decrease balance for bridge sudo change fee")?; if let Some(sudo_address) = self.new_sudo_address { - // No need to add context as this method already reports sufficient context on error. - state.put_bridge_account_sudo_address(&self.bridge_address, sudo_address)?; + state + .put_bridge_account_sudo_address(&self.bridge_address, sudo_address) + .wrap_err("failed to put bridge account sudo address")?; } if let Some(withdrawer_address) = self.new_withdrawer_address { - // No need to add context as this method already reports sufficient context on error. state - .put_bridge_account_withdrawer_address(&self.bridge_address, withdrawer_address)?; + .put_bridge_account_withdrawer_address(&self.bridge_address, withdrawer_address) + .wrap_err("failed to put bridge account withdrawer address")?; } Ok(()) diff --git a/crates/astria-sequencer/src/bridge/component.rs b/crates/astria-sequencer/src/bridge/component.rs index 8fcaff1a7..e844de463 100644 --- a/crates/astria-sequencer/src/bridge/component.rs +++ b/crates/astria-sequencer/src/bridge/component.rs @@ -1,7 +1,10 @@ use std::sync::Arc; use astria_core::protocol::genesis::v1alpha1::GenesisAppState; -use astria_eyre::eyre::Result; +use astria_eyre::eyre::{ + Result, + WrapErr, +}; use tendermint::abci::request::{ BeginBlock, EndBlock, @@ -20,12 +23,15 @@ impl Component for BridgeComponent { #[instrument(name = "BridgeComponent::init_chain", skip_all)] async fn init_chain(mut state: S, app_state: &Self::AppState) -> Result<()> { - // No need to add context as these `put` methods already report sufficient context on error. - state.put_init_bridge_account_base_fee(app_state.fees().init_bridge_account_base_fee)?; - state.put_bridge_lock_byte_cost_multiplier( - app_state.fees().bridge_lock_byte_cost_multiplier, - )?; - state.put_bridge_sudo_change_base_fee(app_state.fees().bridge_sudo_change_fee) + state + .put_init_bridge_account_base_fee(app_state.fees().init_bridge_account_base_fee) + .wrap_err("failed to put init bridge account base fee")?; + state + .put_bridge_lock_byte_cost_multiplier(app_state.fees().bridge_lock_byte_cost_multiplier) + .wrap_err("failed to put bridge lock byte cost multiplier")?; + state + .put_bridge_sudo_change_base_fee(app_state.fees().bridge_sudo_change_fee) + .wrap_err("failed to put bridge sudo change base fee") } #[instrument(name = "BridgeComponent::begin_block", skip_all)] 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 d3637becc..0cf23d677 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -93,21 +93,21 @@ impl ActionHandler for InitBridgeAccountAction { "insufficient funds for bridge account initialization", ); - // No need to add context as this method already reports sufficient context on error. - state.put_bridge_account_rollup_id(&from, self.rollup_id)?; + state + .put_bridge_account_rollup_id(&from, self.rollup_id) + .wrap_err("failed to put bridge account rollup id")?; state .put_bridge_account_ibc_asset(&from, &self.asset) .wrap_err("failed to put asset ID")?; - // No need to add context as this method already reports sufficient context on error. - state.put_bridge_account_sudo_address( - &from, - self.sudo_address.map_or(from, Address::bytes), - )?; - // No need to add context as this method already reports sufficient context on error. - state.put_bridge_account_withdrawer_address( - &from, - self.withdrawer_address.map_or(from, Address::bytes), - )?; + state + .put_bridge_account_sudo_address(&from, self.sudo_address.map_or(from, Address::bytes)) + .wrap_err("failed to put bridge account sudo address")?; + state + .put_bridge_account_withdrawer_address( + &from, + self.withdrawer_address.map_or(from, Address::bytes), + ) + .wrap_err("failed to put bridge account withdrawer address")?; state .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name()) .await diff --git a/crates/astria-sequencer/src/grpc/state_ext.rs b/crates/astria-sequencer/src/grpc/state_ext.rs index a9316ca90..e380d2f5a 100644 --- a/crates/astria-sequencer/src/grpc/state_ext.rs +++ b/crates/astria-sequencer/src/grpc/state_ext.rs @@ -204,13 +204,22 @@ async fn get_sequencer_block_by_hash( state: &S, hash: &[u8; 32], ) -> Result { - // No need to add context as these `get` methods already report sufficient context on error. - let header = state.get_sequencer_block_header_by_hash(hash).await?; - let rollup_ids = state.get_rollup_ids_by_block_hash(hash).await?; + let header = state + .get_sequencer_block_header_by_hash(hash) + .await + .wrap_err("failed to get sequencer block header by hash")?; + let rollup_ids = state + .get_rollup_ids_by_block_hash(hash) + .await + .wrap_err("failed to get rollup ids by block hash")?; let rollup_transactions_proof = state .get_rollup_transactions_proof_by_block_hash(hash) - .await?; - let rollup_ids_proof = state.get_rollup_ids_proof_by_block_hash(hash).await?; + .await + .wrap_err("failed to get rollup transactions proof by block hash")?; + let rollup_ids_proof = state + .get_rollup_ids_proof_by_block_hash(hash) + .await + .wrap_err("failed to get rollup ids proof by block hash")?; // allow: want to avoid explicitly importing `index_map` crate to sequencer crate. #[allow(clippy::default_trait_access)] diff --git a/crates/astria-sequencer/src/ibc/component.rs b/crates/astria-sequencer/src/ibc/component.rs index 1897f79ac..946ce7f13 100644 --- a/crates/astria-sequencer/src/ibc/component.rs +++ b/crates/astria-sequencer/src/ibc/component.rs @@ -45,8 +45,9 @@ impl Component for IbcComponent { .wrap_err("failed to set IBC sudo key")?; for address in app_state.ibc_relayer_addresses() { - // No need to add context as this method already reports sufficient context on error. - state.put_ibc_relayer_address(address)?; + state + .put_ibc_relayer_address(address) + .wrap_err("failed to put IBC relayer address")?; } state diff --git a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs index 7c4ca986f..24491b9b5 100644 --- a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs +++ b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs @@ -47,9 +47,9 @@ impl ActionHandler for IbcRelayerChangeAction { match self { IbcRelayerChangeAction::Addition(address) => { - // No need to add context as this method already reports sufficient context on - // error. - state.put_ibc_relayer_address(address)?; + state + .put_ibc_relayer_address(address) + .wrap_err("failed to put IBC relayer address")?; } IbcRelayerChangeAction::Removal(address) => { state.delete_ibc_relayer_address(address); diff --git a/crates/astria-sequencer/src/sequence/component.rs b/crates/astria-sequencer/src/sequence/component.rs index 8998a0c6d..50d35516b 100644 --- a/crates/astria-sequencer/src/sequence/component.rs +++ b/crates/astria-sequencer/src/sequence/component.rs @@ -1,7 +1,10 @@ use std::sync::Arc; use astria_core::protocol::genesis::v1alpha1::GenesisAppState; -use astria_eyre::eyre::Result; +use astria_eyre::eyre::{ + Result, + WrapErr, +}; use tendermint::abci::request::{ BeginBlock, EndBlock, @@ -20,11 +23,14 @@ impl Component for SequenceComponent { #[instrument(name = "SequenceComponent::init_chain", skip_all)] async fn init_chain(mut state: S, app_state: &Self::AppState) -> Result<()> { - // No need to add context as these `put` methods already report sufficient context on error. - state.put_sequence_action_base_fee(app_state.fees().sequence_base_fee)?; - state.put_sequence_action_byte_cost_multiplier( - app_state.fees().sequence_byte_cost_multiplier, - ) + state + .put_sequence_action_base_fee(app_state.fees().sequence_base_fee) + .wrap_err("failed to put sequence action base fee")?; + state + .put_sequence_action_byte_cost_multiplier( + app_state.fees().sequence_byte_cost_multiplier, + ) + .wrap_err("failed to put sequence action byte cost multiplier") } #[instrument(name = "SequenceComponent::begin_block", skip_all)] diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index fd9b2332b..9dc680a14 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -192,11 +192,12 @@ impl ActionHandler for SignedTransaction { .wrap_err("failed to check account rollup id")? .is_some() { - // No need to add context as this method already reports sufficient context on error. - state.put_last_transaction_id_for_bridge_account( - &self, - transaction_context.transaction_id, - )?; + state + .put_last_transaction_id_for_bridge_account( + &self, + transaction_context.transaction_id, + ) + .wrap_err("failed to put last transaction id for bridge account")?; } let from_nonce = state