Skip to content

Commit

Permalink
refactor(sequencer): remove global state (#1317)
Browse files Browse the repository at this point in the history
## Summary
Removed all global from sequencer.

## Background
Upon init, sequencer stored its native asset and address base prefix at
a fixed global memory locations. This did not yet lead to issues during
runtime (because we don't mutate global after startup), it made testing
unnecessarily difficult: sequencer relies heavily on unit tests, yet the
tests were impure because they relied on the same global state.

The changes to the `mempool` module shows another issue: while the
mempool could be wholly agnostic to the specific global state, heavy use
of the global base prefix meant was less decoupled than it could be.

Because sequencer explicitly passes around state (in the form of state
deltas, the data inside which are made accessible via various
`StateReadExt` and `StateWriteExt` traits), we can read the configured
base prefix and native assets from there.

The downside is that this patch introduces more database reads. However,
given the amounts of data sequencer is dealing with this might not
matter and can be alleviated by using a rocksdb Cache (or other
mechanism) instead of relying on globals.


## Changes
- Remove global setters and writers in `crate::address` and
`crate::assets`
- Update all code to read the base address prefix and native asset
through `crate::address::StateReadExt` and
`crate::address::assets::StateReadExt` instead.
- Update the mempool to be agnostic of address prefixes and use address
bytes insteda: this change is in line with sequencer design
considerations in that the address prefix only matters at the boundary
while state reads are done via the address bytes underlying the bech32m
addresses.
- Require that the sequencer genesis contains a `TracePrefixed` asset.

## Testing
Updated all tests to either use hard coded prefixes, native assets (if
not performing reads or writes on state deltas), or write a base prefix
and native asset to state before performing tests (if accessing state).

## Breaking Changelist
While the change to `astria-core` is technically breaking (because a
field that was of type `asset::Denom` is now `asset::TracePrefixed`), in
practice this change is not breaking because Sequencer required native
assets to be non-ibc prefixed.

## Related Issues
Closes #1208
  • Loading branch information
SuperFluffy committed Aug 1, 2024
1 parent 8171eed commit c765408
Show file tree
Hide file tree
Showing 37 changed files with 1,010 additions and 867 deletions.
13 changes: 8 additions & 5 deletions crates/astria-core/src/sequencer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
pub use penumbra_ibc::params::IBCParameters;

use crate::primitive::v1::{
asset,
asset::{
self,
TracePrefixed,
},
Address,
};

Expand All @@ -26,7 +29,7 @@ pub struct GenesisState {
authority_sudo_address: Address,
ibc_sudo_address: Address,
ibc_relayer_addresses: Vec<Address>,
native_asset_base_denomination: String,
native_asset_base_denomination: TracePrefixed,
ibc_params: IBCParameters,
allowed_fee_assets: Vec<asset::Denom>,
fees: Fees,
Expand Down Expand Up @@ -59,7 +62,7 @@ impl GenesisState {
}

#[must_use]
pub fn native_asset_base_denomination(&self) -> &str {
pub fn native_asset_base_denomination(&self) -> &TracePrefixed {
&self.native_asset_base_denomination
}

Expand Down Expand Up @@ -140,7 +143,7 @@ pub struct UncheckedGenesisState {
pub authority_sudo_address: Address,
pub ibc_sudo_address: Address,
pub ibc_relayer_addresses: Vec<Address>,
pub native_asset_base_denomination: String,
pub native_asset_base_denomination: TracePrefixed,
pub ibc_params: IBCParameters,
pub allowed_fee_assets: Vec<asset::Denom>,
pub fees: Fees,
Expand Down Expand Up @@ -295,7 +298,7 @@ mod tests {
authority_sudo_address: alice(),
ibc_sudo_address: alice(),
ibc_relayer_addresses: vec![alice(), bob()],
native_asset_base_denomination: "nria".to_string(),
native_asset_base_denomination: "nria".parse().unwrap(),
ibc_params: IBCParameters {
ibc_enabled: true,
inbound_ics20_transfers_enabled: true,
Expand Down
2 changes: 1 addition & 1 deletion crates/astria-sequencer-utils/src/genesis_example.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ fn genesis_state() -> GenesisState {
authority_sudo_address: alice(),
ibc_sudo_address: alice(),
ibc_relayer_addresses: vec![alice(), bob()],
native_asset_base_denomination: "nria".to_string(),
native_asset_base_denomination: "nria".parse().unwrap(),
ibc_params: IBCParameters {
ibc_enabled: true,
inbound_ics20_transfers_enabled: true,
Expand Down
20 changes: 12 additions & 8 deletions crates/astria-sequencer/src/accounts/action.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@ use astria_core::{
use tracing::instrument;

use crate::{
accounts,
accounts::StateReadExt as _,
accounts::{
self,
StateReadExt as _,
},
address,
assets,
bridge::StateReadExt as _,
transaction::action_handler::ActionHandler,
Expand Down Expand Up @@ -81,15 +84,16 @@ where
#[async_trait::async_trait]
impl ActionHandler for TransferAction {
async fn check_stateless(&self) -> Result<()> {
crate::address::ensure_base_prefix(&self.to).context("destination address is invalid")?;
Ok(())
}

async fn check_stateful<S: accounts::StateReadExt + 'static>(
&self,
state: &S,
from: Address,
) -> Result<()> {
async fn check_stateful<S>(&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)
Expand Down
20 changes: 13 additions & 7 deletions crates/astria-sequencer/src/accounts/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ use tendermint::abci::request::{
};
use tracing::instrument;

use super::state_ext::StateWriteExt;
use crate::{
assets::get_native_asset,
accounts,
assets,
component::Component,
};

Expand All @@ -24,11 +24,17 @@ impl Component for AccountsComponent {
type AppState = astria_core::sequencer::GenesisState;

#[instrument(name = "AccountsComponent::init_chain", skip_all)]
async fn init_chain<S: StateWriteExt>(mut state: S, app_state: &Self::AppState) -> Result<()> {
let native_asset = get_native_asset();
async fn init_chain<S>(mut state: S, app_state: &Self::AppState) -> Result<()>
where
S: accounts::StateWriteExt + assets::StateReadExt,
{
let native_asset = state
.get_native_asset()
.await
.context("failed to read native asset from state")?;
for account in app_state.accounts() {
state
.put_account_balance(account.address, native_asset, account.balance)
.put_account_balance(account.address, &native_asset, account.balance)
.context("failed writing account balance to state")?;
}

Expand All @@ -39,15 +45,15 @@ impl Component for AccountsComponent {
}

#[instrument(name = "AccountsComponent::begin_block", skip_all)]
async fn begin_block<S: StateWriteExt + 'static>(
async fn begin_block<S: accounts::StateWriteExt + 'static>(
_state: &mut Arc<S>,
_begin_block: &BeginBlock,
) -> Result<()> {
Ok(())
}

#[instrument(name = "AccountsComponent::end_block", skip_all)]
async fn end_block<S: StateWriteExt + 'static>(
async fn end_block<S: accounts::StateWriteExt + 'static>(
_state: &mut Arc<S>,
_end_block: &EndBlock,
) -> Result<()> {
Expand Down
Loading

0 comments on commit c765408

Please sign in to comment.