Skip to content

Commit

Permalink
chore(sequencer): consolidate all action handling to one module (#1759)
Browse files Browse the repository at this point in the history
## Summary
Moved all action and transaction handling to single module.

## Background
Previously, all implementations of `ActionHandler` were done separately
in components which corresponded to their given actions (or groups of
actions, such as in `bridge`). While this may make sense from an
action-centered point of view, it is confusing to an outsider to have to
look many different places to find where stateless/stateful checks and
execution occur. This PR consolidates all of the implementations as well
as their testing.

## Changes
- Created new `action_handler` module and moved the corresponding
`ActionHandler` trait as well as all of its `impl`s into this module.
- Renamed some moved tests for clarity.
- Categorized tests by action in submodule `tests`.
- Categorized impls by action in submodule `impls`.

## Testing
Passing all tests.

## Changelogs
Changelogs updated

## Related Issues
closes #1657
  • Loading branch information
ethanoroshiba authored Dec 2, 2024
1 parent ab87d35 commit d5b9a3d
Show file tree
Hide file tree
Showing 34 changed files with 861 additions and 780 deletions.
1 change: 1 addition & 0 deletions crates/astria-sequencer/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed

- Index all event attributes [#1786](https://github.com/astriaorg/astria/pull/1786).
- Consolidate action handling to single module [#1759](https://github.com/astriaorg/astria/pull/1759).
- Ensure all deposit assets are trace prefixed [#1807](https://github.com/astriaorg/astria/pull/1807).

## [1.0.0] - 2024-10-25
Expand Down
99 changes: 0 additions & 99 deletions crates/astria-sequencer/src/accounts/action.rs

This file was deleted.

1 change: 0 additions & 1 deletion crates/astria-sequencer/src/accounts/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
pub(crate) mod action;
pub(crate) mod component;
pub(crate) mod query;
mod state_ext;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,26 @@ use astria_eyre::eyre::{
Result,
WrapErr as _,
};
use async_trait::async_trait;
use cnidarium::StateWrite;

use crate::{
accounts::action::{
action_handler::{
check_transfer,
execute_transfer,
ActionHandler,
},
address::StateReadExt as _,
app::ActionHandler,
assets::StateReadExt as _,
bridge::{
StateReadExt as _,
StateWriteExt as _,
StateWriteExt,
},
transaction::StateReadExt as _,
utils::create_deposit_event,
};

#[async_trait::async_trait]
#[async_trait]
impl ActionHandler for BridgeLock {
async fn check_stateless(&self) -> Result<()> {
Ok(())
Expand Down Expand Up @@ -122,8 +123,8 @@ mod tests {
AddressBytes,
StateWriteExt as _,
},
action_handler::ActionHandler as _,
address::StateWriteExt as _,
app::ActionHandler as _,
assets::StateWriteExt as _,
benchmark_and_test_utils::{
astria_address,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,20 @@ use astria_eyre::eyre::{
Result,
WrapErr as _,
};
use async_trait::async_trait;
use cnidarium::StateWrite;

use crate::{
action_handler::ActionHandler,
address::StateReadExt as _,
app::ActionHandler,
bridge::state_ext::{
bridge::{
StateReadExt as _,
StateWriteExt as _,
StateWriteExt,
},
transaction::StateReadExt as _,
};
#[async_trait::async_trait]

#[async_trait]
impl ActionHandler for BridgeSudoChange {
async fn check_stateless(&self) -> Result<()> {
Ok(())
Expand Down Expand Up @@ -79,35 +81,38 @@ impl ActionHandler for BridgeSudoChange {
#[cfg(test)]
mod tests {
use astria_core::{
primitive::v1::{
asset,
TransactionId,
primitive::v1::TransactionId,
protocol::{
fees::v1::BridgeSudoChangeFeeComponents,
transaction::v1::action::BridgeSudoChange,
},
protocol::fees::v1::BridgeSudoChangeFeeComponents,
};
use cnidarium::StateDelta;

use super::*;
use crate::{
accounts::StateWriteExt as _,
action_handler::{
impls::test_utils::test_asset,
ActionHandler as _,
},
address::StateWriteExt as _,
benchmark_and_test_utils::{
astria_address,
ASTRIA_PREFIX,
},
bridge::{
StateReadExt as _,
StateWriteExt as _,
},
fees::StateWriteExt as _,
transaction::{
StateWriteExt as _,
TransactionContext,
},
};

fn test_asset() -> asset::Denom {
"test".parse().unwrap()
}

#[tokio::test]
async fn fails_with_unauthorized_if_signer_is_not_sudo_address() {
async fn bridge_sudo_change_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);
Expand Down Expand Up @@ -146,7 +151,7 @@ mod tests {
}

#[tokio::test]
async fn executes() {
async fn bridge_sudo_change_executes_as_expected() {
let storage = cnidarium::TempStorage::new().await.unwrap();
let snapshot = storage.latest_snapshot();
let mut state = StateDelta::new(snapshot);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,24 @@ use astria_eyre::eyre::{
Result,
WrapErr as _,
};
use async_trait::async_trait;
use cnidarium::StateWrite;

use crate::{
accounts::action::{
action_handler::{
check_transfer,
execute_transfer,
ActionHandler,
},
address::StateReadExt as _,
app::ActionHandler,
bridge::{
StateReadExt as _,
StateWriteExt as _,
StateWriteExt,
},
transaction::StateReadExt as _,
};

#[async_trait::async_trait]
#[async_trait]
impl ActionHandler for BridgeUnlock {
// TODO(https://github.com/astriaorg/astria/issues/1430): move checks to the `BridgeUnlock` parsing.
async fn check_stateless(&self) -> Result<()> {
Expand Down Expand Up @@ -104,7 +105,6 @@ impl ActionHandler for BridgeUnlock {
mod tests {
use astria_core::{
primitive::v1::{
asset,
RollupId,
TransactionId,
},
Expand All @@ -117,8 +117,11 @@ mod tests {
use crate::{
accounts::StateWriteExt as _,
action_handler::{
impls::test_utils::test_asset,
ActionHandler as _,
},
address::StateWriteExt as _,
app::ActionHandler as _,
benchmark_and_test_utils::{
assert_eyre_error,
astria_address,
Expand All @@ -132,12 +135,8 @@ mod tests {
},
};
fn test_asset() -> asset::Denom {
"test".parse().unwrap()
}

#[tokio::test]
async fn fails_if_bridge_account_has_no_withdrawer_address() {
async fn bridge_unlock_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);
Expand Down Expand Up @@ -176,7 +175,7 @@ mod tests {
}

#[tokio::test]
async fn fails_if_withdrawer_is_not_signer() {
async fn bridge_unlock_fails_if_withdrawer_is_not_signer() {
let storage = cnidarium::TempStorage::new().await.unwrap();
let snapshot = storage.latest_snapshot();
let mut state = StateDelta::new(snapshot);
Expand Down Expand Up @@ -219,7 +218,7 @@ mod tests {
}

#[tokio::test]
async fn execute_with_duplicated_withdrawal_event_id() {
async fn bridge_unlock_executes_with_duplicated_withdrawal_event_id() {
let storage = cnidarium::TempStorage::new().await.unwrap();
let snapshot = storage.latest_snapshot();
let mut state = StateDelta::new(snapshot);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
use astria_core::protocol::transaction::v1::action::FeeAssetChange;
use astria_eyre::eyre::{
self,
ensure,
WrapErr as _,
};
use async_trait::async_trait;
use cnidarium::StateWrite;
use futures::StreamExt as _;
use tokio::pin;

use crate::{
action_handler::ActionHandler,
authority::StateReadExt as _,
fees::{
StateReadExt as _,
StateWriteExt as _,
},
transaction::StateReadExt as _,
};

#[async_trait]
impl ActionHandler for FeeAssetChange {
async fn check_stateless(&self) -> eyre::Result<()> {
Ok(())
}

async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> eyre::Result<()> {
let from = state
.get_transaction_context()
.expect("transaction source must be present in state when executing an action")
.address_bytes();
let authority_sudo_address = state
.get_sudo_address()
.await
.wrap_err("failed to get authority sudo address")?;
ensure!(
authority_sudo_address == from,
"unauthorized address for fee asset change"
);
match self {
FeeAssetChange::Addition(asset) => {
state
.put_allowed_fee_asset(asset)
.context("failed to write allowed fee asset to state")?;
}
FeeAssetChange::Removal(asset) => {
state.delete_allowed_fee_asset(asset);

pin!(
let assets = state.allowed_fee_assets();
);
ensure!(
assets
.filter_map(|item| std::future::ready(item.ok()))
.next()
.await
.is_some(),
"cannot remove last allowed fee asset",
);
}
}
Ok(())
}
}
Loading

0 comments on commit d5b9a3d

Please sign in to comment.