From 17e6711ce4032930519660f70a9e09af1dea90f7 Mon Sep 17 00:00:00 2001 From: Lily Johnson <35852084+Lilyjjo@users.noreply.github.com> Date: Mon, 30 Sep 2024 13:11:52 -0400 Subject: [PATCH] feat(sequencer)!: transaction categories on UnsignedTransaction (#1512) ## Note for Reviewers Most of the logic changes are in the files `crates/astria-core/src/protocol/transaction/v1alpha1/action_group.rs` and `crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs`. The rest of the changes are updating the construction of `UnsignedTransactions` and/or changing the access to the inner fields. ## Summary Adds restrictions on what type of `Actions` can be included in the same `UnsignedTransaction`. Implements the categories as described in #1412 but with slightly different names: - General: - TransferAction - SequenceAction - BridgeLockAction - BridgeUnlockAction - IbcRelay - Ics20Withdrawal - ValidatorUpdate - UnbundeableGeneral (Instead of Bridge Control): - InitBridgeAccountAction - BridgeSudoChangeAction - Sudo: - FeeAssetChangeAction - FeeChangeAction - IbcRelayerChangeAction - UnbundleableSudo (Instead of Sudo Control): - SudoAddressChangeAction - IbcSudoChangeAction The check is applied at the time of constructing the `UnsignedTransaction`. The `UnsignedTransaction` additionally had its struct fields changed to private and now uses a new constructor to prevent the contained actions from being modified. ## Background We want transactions that can affect the validity of other transactions to be ordered last in blocks to reduce the amount of failed transactions we process. These logic changes are the first code changes being made to realize this goal. ## Changes - Introduced the `Actions` struct to hold valid groupings of `Actions`. - Introduced `ActionGroup` enum to represent which `Actions` can be included together in a transaction and if more than one action can be included in a transaction. - Changed the `UnsignedTransaction` struct to have private fields and to use a new constructor. - Changed the `UnsignedTransaction`'s `action` to be a `Actions` type instead of just a vector of `Actions`. ## Testing Unit testing and ran locally. ## Breaking Changelist Transactions that contain invalid `ActionGroup` combos (due to mixed groups or multiple actions in non-bundleable group types) are now 'invalid' transactions. I had to update one of the snapshot tests due to having to unbundle some of the transactions, creating a new state. ## Related Issues Initial steps for #1412 closes #1414, #1416 --- .../src/bridge_withdrawer/submitter/mod.rs | 14 +- .../astria-cli/src/commands/bridge/submit.rs | 16 +- crates/astria-cli/src/commands/sequencer.rs | 16 +- .../src/executor/bundle_factory/mod.rs | 27 +- .../src/executor/bundle_factory/tests.rs | 34 +- crates/astria-composer/src/executor/mod.rs | 29 +- crates/astria-core/src/protocol/test_utils.rs | 24 +- .../protocol/transaction/v1alpha1/action.rs | 36 ++ .../transaction/v1alpha1/action_group/mod.rs | 199 ++++++++ .../v1alpha1/action_group/tests.rs | 241 ++++++++++ .../src/protocol/transaction/v1alpha1/mod.rs | 161 ++++--- .../src/sequencerblock/v1alpha1/block.rs | 2 +- .../astria-sequencer-client/src/tests/http.rs | 16 +- crates/astria-sequencer/src/app/mod.rs | 13 +- ...ransaction_with_every_action_snapshot.snap | 61 ++- crates/astria-sequencer/src/app/test_utils.rs | 16 +- .../src/app/tests_app/mempool.rs | 100 ++-- .../astria-sequencer/src/app/tests_app/mod.rs | 108 ++--- .../src/app/tests_block_fees.rs | 76 ++- .../src/app/tests_breaking_changes.rs | 112 +++-- .../src/app/tests_execute_transaction.rs | 434 ++++++++---------- .../astria-sequencer/src/benchmark_utils.rs | 36 +- .../src/proposal/commitment.rs | 38 +- .../astria-sequencer/src/service/consensus.rs | 15 +- .../src/transaction/checks.rs | 33 +- 25 files changed, 1148 insertions(+), 709 deletions(-) create mode 100644 crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs create mode 100644 crates/astria-core/src/protocol/transaction/v1alpha1/action_group/tests.rs diff --git a/crates/astria-bridge-withdrawer/src/bridge_withdrawer/submitter/mod.rs b/crates/astria-bridge-withdrawer/src/bridge_withdrawer/submitter/mod.rs index 952153b7b7..a768873c5e 100644 --- a/crates/astria-bridge-withdrawer/src/bridge_withdrawer/submitter/mod.rs +++ b/crates/astria-bridge-withdrawer/src/bridge_withdrawer/submitter/mod.rs @@ -13,7 +13,6 @@ use astria_core::{ }, protocol::transaction::v1alpha1::{ Action, - TransactionParams, UnsignedTransaction, }, }; @@ -154,13 +153,12 @@ impl Submitter { .wrap_err("failed to get nonce from sequencer")?; debug!(nonce, "fetched latest nonce"); - let unsigned = UnsignedTransaction { - actions, - params: TransactionParams::builder() - .nonce(nonce) - .chain_id(sequencer_chain_id) - .build(), - }; + let unsigned = UnsignedTransaction::builder() + .actions(actions) + .nonce(nonce) + .chain_id(sequencer_chain_id) + .try_build() + .wrap_err("failed to build unsigned transaction")?; // sign transaction let signed = unsigned.into_signed(signer.signing_key()); diff --git a/crates/astria-cli/src/commands/bridge/submit.rs b/crates/astria-cli/src/commands/bridge/submit.rs index c332a5a6d1..cd0085b54c 100644 --- a/crates/astria-cli/src/commands/bridge/submit.rs +++ b/crates/astria-cli/src/commands/bridge/submit.rs @@ -7,7 +7,6 @@ use astria_core::{ crypto::SigningKey, protocol::transaction::v1alpha1::{ Action, - TransactionParams, UnsignedTransaction, }, }; @@ -129,14 +128,13 @@ async fn submit_transaction( .await .wrap_err("failed to get nonce")?; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(nonce_res.nonce) - .chain_id(chain_id) - .build(), - actions, - } - .into_signed(signing_key); + let tx = UnsignedTransaction::builder() + .actions(actions) + .nonce(nonce_res.nonce) + .chain_id(chain_id) + .try_build() + .wrap_err("failed to build transaction from actions")? + .into_signed(signing_key); let res = client .submit_transaction_sync(tx) .await diff --git a/crates/astria-cli/src/commands/sequencer.rs b/crates/astria-cli/src/commands/sequencer.rs index c8f19bdb65..95c274f534 100644 --- a/crates/astria-cli/src/commands/sequencer.rs +++ b/crates/astria-cli/src/commands/sequencer.rs @@ -16,7 +16,6 @@ use astria_core::{ TransferAction, ValidatorUpdate, }, - TransactionParams, UnsignedTransaction, }, }; @@ -475,14 +474,13 @@ async fn submit_transaction( .await .wrap_err("failed to get nonce")?; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(nonce_res.nonce) - .chain_id(chain_id) - .build(), - actions: vec![action], - } - .into_signed(&sequencer_key); + let tx = UnsignedTransaction::builder() + .actions(vec![action]) + .nonce(nonce_res.nonce) + .chain_id(chain_id) + .try_build() + .wrap_err("failed to build transaction from actions")? + .into_signed(&sequencer_key); let res = sequencer_client .submit_transaction_sync(tx) .await diff --git a/crates/astria-composer/src/executor/bundle_factory/mod.rs b/crates/astria-composer/src/executor/bundle_factory/mod.rs index 58a3e5a9b2..f4fb47eaaa 100644 --- a/crates/astria-composer/src/executor/bundle_factory/mod.rs +++ b/crates/astria-composer/src/executor/bundle_factory/mod.rs @@ -13,6 +13,7 @@ use astria_core::{ protocol::transaction::v1alpha1::{ action::SequenceAction, Action, + UnsignedTransaction, }, Protobuf as _, }; @@ -74,6 +75,27 @@ impl SizedBundle { } } + /// Constructs an [`UnsignedTransaction`] from the actions contained in the bundle and `params`. + /// # Panics + /// Method is expected to never panic because only `SequenceActions` are added to the bundle, + /// which should produce a valid variant of the `ActionGroup` type. + pub(super) fn to_unsigned_transaction( + &self, + nonce: u32, + chain_id: &str, + ) -> UnsignedTransaction { + UnsignedTransaction::builder() + .actions(self.buffer.clone()) + .chain_id(chain_id) + .nonce(nonce) + .try_build() + .expect( + "method is expected to never panic because only `SequenceActions` are added to \ + the bundle, which should produce a valid variant of the `ActionGroup` type; this \ + is checked by `tests::transaction_construction_should_not_panic", + ) + } + /// Buffer `seq_action` into the bundle. /// # Errors /// - `seq_action` is beyond the max size allowed for the entire bundle @@ -111,11 +133,6 @@ impl SizedBundle { self.curr_size } - /// Consume self and return the underlying buffer of actions. - pub(super) fn into_actions(self) -> Vec { - self.buffer - } - /// Returns the number of sequence actions in the bundle. pub(super) fn actions_count(&self) -> usize { self.buffer.len() diff --git a/crates/astria-composer/src/executor/bundle_factory/tests.rs b/crates/astria-composer/src/executor/bundle_factory/tests.rs index 4dab9b056a..762934782e 100644 --- a/crates/astria-composer/src/executor/bundle_factory/tests.rs +++ b/crates/astria-composer/src/executor/bundle_factory/tests.rs @@ -73,7 +73,7 @@ mod sized_bundle { assert!(bundle.buffer.is_empty()); // assert that the flushed bundle has just the sequence action pushed earlier - let actions = flushed_bundle.into_actions(); + let actions = flushed_bundle.buffer; assert_eq!(actions.len(), 1); let actual_seq_action = actions[0].as_sequence().unwrap(); assert_eq!(actual_seq_action.rollup_id, seq_action.rollup_id); @@ -137,7 +137,7 @@ mod bundle_factory { assert_eq!(bundle_factory.finished.len(), 1); // assert `pop_finished()` will return `seq_action0` let next_actions = bundle_factory.next_finished(); - let actions = next_actions.unwrap().pop().into_actions(); + let actions = next_actions.unwrap().pop().buffer; let actual_seq_action = actions[0].as_sequence().unwrap(); assert_eq!(actual_seq_action.rollup_id, seq_action0.rollup_id); assert_eq!(actual_seq_action.data, seq_action0.data); @@ -240,7 +240,7 @@ mod bundle_factory { // assert that the finished queue is empty (curr wasn't flushed) assert_eq!(bundle_factory.finished.len(), 0); // assert `pop_now()` returns `seq_action` - let actions = bundle_factory.pop_now().into_actions(); + let actions = bundle_factory.pop_now().buffer; let actual_seq_action = actions[0].as_sequence().unwrap(); assert_eq!(actual_seq_action.rollup_id, seq_action.rollup_id); assert_eq!(actual_seq_action.data, seq_action.data); @@ -265,7 +265,7 @@ mod bundle_factory { // assert that the bundle factory has one bundle in the finished queue assert_eq!(bundle_factory.finished.len(), 1); // assert `pop_now()` will return `seq_action0` - let actions = bundle_factory.pop_now().into_actions(); + let actions = bundle_factory.pop_now().buffer; let actual_seq_action = actions[0].as_sequence().unwrap(); assert_eq!(actual_seq_action.rollup_id, seq_action0.rollup_id); assert_eq!(actual_seq_action.data, seq_action0.data); @@ -300,7 +300,7 @@ mod bundle_factory { assert_eq!(bundle_factory.finished.len(), 1); // assert `pop_now()` will return `seq_action0` on the first call - let actions_finished = bundle_factory.pop_now().into_actions(); + let actions_finished = bundle_factory.pop_now().buffer; assert_eq!(actions_finished.len(), 1); let actual_seq_action = actions_finished[0].as_sequence().unwrap(); assert_eq!(actual_seq_action.rollup_id, seq_action0.rollup_id); @@ -310,7 +310,7 @@ mod bundle_factory { assert_eq!(bundle_factory.finished.len(), 0); // assert `pop_now()` will return `seq_action1` on the second call (i.e. from curr) - let actions_curr = bundle_factory.pop_now().into_actions(); + let actions_curr = bundle_factory.pop_now().buffer; assert_eq!(actions_curr.len(), 1); let actual_seq_action = actions_curr[0].as_sequence().unwrap(); assert_eq!(actual_seq_action.rollup_id, seq_action1.rollup_id); @@ -337,4 +337,26 @@ mod bundle_factory { assert_eq!(bundle_factory.finished.len(), 0); assert!(!bundle_factory.is_full()); } + + #[test] + fn transaction_construction_does_not_panic() { + let mut bundle_factory = BundleFactory::new(1000, 10); + + bundle_factory + .try_push(sequence_action_with_n_bytes(50)) + .unwrap(); + bundle_factory + .try_push(sequence_action_with_n_bytes(50)) + .unwrap(); + bundle_factory + .try_push(sequence_action_with_n_bytes(50)) + .unwrap(); + + let bundle = bundle_factory.pop_now(); + + // construction of multiple sequence actions should not panic + let unsigned_tx = bundle.to_unsigned_transaction(0, "astria-testnet-1"); + + assert_eq!(unsigned_tx.actions().len(), 3); + } } diff --git a/crates/astria-composer/src/executor/mod.rs b/crates/astria-composer/src/executor/mod.rs index 5e88d5fbba..26260cbfaa 100644 --- a/crates/astria-composer/src/executor/mod.rs +++ b/crates/astria-composer/src/executor/mod.rs @@ -17,8 +17,6 @@ use astria_core::{ transaction::v1alpha1::{ action::SequenceAction, SignedTransaction, - TransactionParams, - UnsignedTransaction, }, }, }; @@ -670,7 +668,6 @@ impl Future for SubmitFut { type Output = eyre::Result; // FIXME (https://github.com/astriaorg/astria/issues/1572): This function is too long and should be refactored. - #[expect(clippy::too_many_lines, reason = "this may warrant a refactor")] fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll { const INVALID_NONCE: Code = Code::Err(AbciErrorCode::INVALID_NONCE.value()); loop { @@ -678,15 +675,10 @@ impl Future for SubmitFut { let new_state = match this.state.project() { SubmitStateProj::NotStarted => { - let params = TransactionParams::builder() - .nonce(*this.nonce) - .chain_id(&*this.chain_id) - .build(); - let tx = UnsignedTransaction { - actions: this.bundle.clone().into_actions(), - params, - } - .into_signed(this.signing_key); + let tx = this + .bundle + .to_unsigned_transaction(*this.nonce, &*this.chain_id) + .into_signed(this.signing_key); info!( nonce.actual = *this.nonce, bundle = %telemetry::display::json(&SizedBundleReport(this.bundle)), @@ -756,15 +748,10 @@ impl Future for SubmitFut { } => match ready!(fut.poll(cx)) { Ok(nonce) => { *this.nonce = nonce; - let params = TransactionParams::builder() - .nonce(*this.nonce) - .chain_id(&*this.chain_id) - .build(); - let tx = UnsignedTransaction { - actions: this.bundle.clone().into_actions(), - params, - } - .into_signed(this.signing_key); + let tx = this + .bundle + .to_unsigned_transaction(*this.nonce, &*this.chain_id) + .into_signed(this.signing_key); info!( nonce.resubmission = *this.nonce, bundle = %telemetry::display::json(&SizedBundleReport(this.bundle)), diff --git a/crates/astria-core/src/protocol/test_utils.rs b/crates/astria-core/src/protocol/test_utils.rs index bc39e94edf..c1edc8e92f 100644 --- a/crates/astria-core/src/protocol/test_utils.rs +++ b/crates/astria-core/src/protocol/test_utils.rs @@ -7,11 +7,7 @@ use prost::Message as _; use super::{ group_sequence_actions_in_signed_transaction_transactions_by_rollup_id, - transaction::v1alpha1::{ - action::SequenceAction, - TransactionParams, - UnsignedTransaction, - }, + transaction::v1alpha1::action::SequenceAction, }; use crate::{ crypto::SigningKey, @@ -19,6 +15,7 @@ use crate::{ derive_merkle_tree_from_rollup_txs, RollupId, }, + protocol::transaction::v1alpha1::UnsignedTransaction, sequencerblock::v1alpha1::{ block::Deposit, SequencerBlock, @@ -107,16 +104,17 @@ impl ConfigureSequencerBlock { let txs = if actions.is_empty() { vec![] } else { - let unsigned_transaction = UnsignedTransaction { - actions, - params: TransactionParams::builder() - .nonce(1) - .chain_id(chain_id.clone()) - .build(), - }; + let unsigned_transaction = UnsignedTransaction::builder() + .actions(actions) + .chain_id(chain_id.clone()) + .nonce(1) + .try_build() + .expect( + "should be able to build unsigned transaction since only sequence actions are \ + contained", + ); vec![unsigned_transaction.into_signed(&signing_key)] }; - let mut deposits_map: HashMap> = HashMap::new(); for deposit in deposits { if let Some(entry) = deposits_map.get_mut(&deposit.rollup_id) { diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index c6d8caf716..5a4ef5d88b 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -153,6 +153,7 @@ impl Protobuf for Action { } } +// TODO: add unit tests for these methods (https://github.com/astriaorg/astria/issues/1593) impl Action { #[must_use] pub fn as_sequence(&self) -> Option<&SequenceAction> { @@ -169,6 +170,14 @@ impl Action { }; Some(transfer_action) } + + pub fn is_fee_asset_change(&self) -> bool { + matches!(self, Self::FeeAssetChange(_)) + } + + pub fn is_fee_change(&self) -> bool { + matches!(self, Self::FeeChange(_)) + } } impl From for Action { @@ -263,6 +272,33 @@ impl TryFrom for Action { } } +// TODO: replace this trait with a Protobuf:FullName implementation. +// Issue tracked in #1567 +pub(super) trait ActionName { + fn name(&self) -> &'static str; +} + +impl ActionName for Action { + fn name(&self) -> &'static str { + match self { + Action::Sequence(_) => "Sequence", + Action::Transfer(_) => "Transfer", + Action::ValidatorUpdate(_) => "ValidatorUpdate", + Action::SudoAddressChange(_) => "SudoAddressChange", + Action::Ibc(_) => "Ibc", + Action::IbcSudoChange(_) => "IbcSudoChange", + Action::Ics20Withdrawal(_) => "Ics20Withdrawal", + Action::IbcRelayerChange(_) => "IbcRelayerChange", + Action::FeeAssetChange(_) => "FeeAssetChange", + Action::InitBridgeAccount(_) => "InitBridgeAccount", + Action::BridgeLock(_) => "BridgeLock", + Action::BridgeUnlock(_) => "BridgeUnlock", + Action::BridgeSudoChange(_) => "BridgeSudoChange", + Action::FeeChange(_) => "FeeChange", + } + } +} + #[expect( clippy::module_name_repetitions, reason = "for parity with the Protobuf spec" diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs new file mode 100644 index 0000000000..e39971d956 --- /dev/null +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs @@ -0,0 +1,199 @@ +#[cfg(test)] +mod tests; + +use std::fmt::{ + self, + Debug, +}; + +use penumbra_ibc::IbcRelay; + +use super::{ + action::{ + ActionName, + BridgeLockAction, + BridgeSudoChangeAction, + BridgeUnlockAction, + FeeAssetChangeAction, + FeeChangeAction, + IbcRelayerChangeAction, + IbcSudoChangeAction, + Ics20Withdrawal, + InitBridgeAccountAction, + SequenceAction, + SudoAddressChangeAction, + TransferAction, + ValidatorUpdate, + }, + Action, +}; + +trait BelongsToGroup { + const GROUP: ActionGroup; +} + +macro_rules! impl_belong_to_group { + ($(($act:ty, $group:expr)),*$(,)?) => { + $( + impl BelongsToGroup for $act { + const GROUP: ActionGroup = $group; + } + )* + } +} + +impl_belong_to_group!( + (SequenceAction, ActionGroup::BundleableGeneral), + (TransferAction, ActionGroup::BundleableGeneral), + (ValidatorUpdate, ActionGroup::BundleableGeneral), + (SudoAddressChangeAction, ActionGroup::UnbundleableSudo), + (IbcRelayerChangeAction, ActionGroup::BundleableSudo), + (Ics20Withdrawal, ActionGroup::BundleableGeneral), + (InitBridgeAccountAction, ActionGroup::UnbundleableGeneral), + (BridgeLockAction, ActionGroup::BundleableGeneral), + (BridgeUnlockAction, ActionGroup::BundleableGeneral), + (BridgeSudoChangeAction, ActionGroup::UnbundleableGeneral), + (FeeChangeAction, ActionGroup::BundleableSudo), + (FeeAssetChangeAction, ActionGroup::BundleableSudo), + (IbcRelay, ActionGroup::BundleableGeneral), + (IbcSudoChangeAction, ActionGroup::UnbundleableSudo), +); + +impl Action { + const fn group(&self) -> ActionGroup { + match self { + Action::Sequence(_) => SequenceAction::GROUP, + Action::Transfer(_) => TransferAction::GROUP, + Action::ValidatorUpdate(_) => ValidatorUpdate::GROUP, + Action::SudoAddressChange(_) => SudoAddressChangeAction::GROUP, + Action::IbcRelayerChange(_) => IbcRelayerChangeAction::GROUP, + Action::Ics20Withdrawal(_) => Ics20Withdrawal::GROUP, + Action::InitBridgeAccount(_) => InitBridgeAccountAction::GROUP, + Action::BridgeLock(_) => BridgeLockAction::GROUP, + Action::BridgeUnlock(_) => BridgeUnlockAction::GROUP, + Action::BridgeSudoChange(_) => BridgeSudoChangeAction::GROUP, + Action::FeeChange(_) => FeeChangeAction::GROUP, + Action::FeeAssetChange(_) => FeeAssetChangeAction::GROUP, + Action::Ibc(_) => IbcRelay::GROUP, + Action::IbcSudoChange(_) => IbcSudoChangeAction::GROUP, + } + } +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub(super) enum ActionGroup { + BundleableGeneral, + UnbundleableGeneral, + BundleableSudo, + UnbundleableSudo, +} + +impl ActionGroup { + pub(super) fn is_bundleable(self) -> bool { + matches!( + self, + ActionGroup::BundleableGeneral | ActionGroup::BundleableSudo + ) + } + + pub(super) fn is_bundleable_sudo(self) -> bool { + matches!(self, ActionGroup::BundleableSudo) + } +} + +impl fmt::Display for ActionGroup { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ActionGroup::BundleableGeneral => write!(f, "bundleable general"), + ActionGroup::UnbundleableGeneral => write!(f, "unbundleable general"), + ActionGroup::BundleableSudo => write!(f, "bundleable sudo"), + ActionGroup::UnbundleableSudo => write!(f, "unbundleable sudo"), + } + } +} + +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub struct Error(ErrorKind); + +impl Error { + fn mixed( + original_group: ActionGroup, + additional_group: ActionGroup, + action: &'static str, + ) -> Self { + Self(ErrorKind::Mixed { + original_group, + additional_group, + action, + }) + } + + fn not_bundleable(group: ActionGroup) -> Self { + Self(ErrorKind::NotBundleable { + group, + }) + } +} + +#[derive(Debug, thiserror::Error)] +enum ErrorKind { + #[error( + "input contains mixed `ActionGroup` types. original group: {original_group}, additional \ + group: {additional_group}, triggering action: {action}" + )] + Mixed { + original_group: ActionGroup, + additional_group: ActionGroup, + action: &'static str, + }, + #[error("attempted to create bundle with non bundleable `ActionGroup` type: {group}")] + NotBundleable { group: ActionGroup }, +} + +#[derive(Clone, Debug, Default)] +pub(super) struct Actions { + inner: Vec, +} + +impl Actions { + pub(super) fn actions(&self) -> &[Action] { + &self.inner + } + + #[must_use] + pub(super) fn into_actions(self) -> Vec { + self.inner + } + + pub(super) fn group(&self) -> Option { + self.inner.first().map(super::action::Action::group) + } + + pub(super) fn try_from_list_of_actions(actions: Vec) -> Result { + let mut actions_iter = actions.iter(); + let group = match actions_iter.next() { + Some(action) => action.group(), + None => { + // empty `actions` + return Ok(Self::default()); + } + }; + + // assert size constraints on non-bundleable action groups + if actions.len() > 1 && !group.is_bundleable() { + return Err(Error::not_bundleable(group)); + } + + // assert the rest of the actions have the same group as the first + for action in actions_iter { + if action.group() != group { + return Err(Error::mixed(group, action.group(), action.name())); + } + } + + Ok(Self { + inner: actions, + }) + } +} diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/tests.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/tests.rs new file mode 100644 index 0000000000..6aa2afb353 --- /dev/null +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/tests.rs @@ -0,0 +1,241 @@ +use ibc_types::core::client::Height; + +use crate::{ + crypto::VerificationKey, + primitive::v1::{ + asset::Denom, + Address, + RollupId, + }, + protocol::transaction::v1alpha1::{ + action::{ + Action, + BridgeLockAction, + BridgeSudoChangeAction, + BridgeUnlockAction, + FeeAssetChangeAction, + FeeChange, + FeeChangeAction, + IbcRelayerChangeAction, + IbcSudoChangeAction, + Ics20Withdrawal, + InitBridgeAccountAction, + SequenceAction, + SudoAddressChangeAction, + TransferAction, + ValidatorUpdate, + }, + action_group::{ + ActionGroup, + Actions, + ErrorKind, + }, + }, +}; +const ASTRIA_ADDRESS_PREFIX: &str = "astria"; + +#[test] +fn try_from_list_of_actions_bundleable_general() { + let address: Address<_> = Address::builder() + .array([0; 20]) + .prefix(ASTRIA_ADDRESS_PREFIX) + .try_build() + .unwrap(); + + let asset: Denom = "nria".parse().unwrap(); + let actions = vec![ + Action::Sequence(SequenceAction { + rollup_id: RollupId::from([8; 32]), + data: vec![].into(), + fee_asset: asset.clone(), + }), + Action::Transfer(TransferAction { + to: address, + amount: 100, + asset: asset.clone(), + fee_asset: asset.clone(), + }), + Action::BridgeLock(BridgeLockAction { + to: address, + amount: 100, + asset: asset.clone(), + fee_asset: asset.clone(), + destination_chain_address: String::new(), + }), + Action::BridgeUnlock(BridgeUnlockAction { + to: address, + amount: 100, + fee_asset: asset.clone(), + bridge_address: address, + memo: String::new(), + rollup_block_number: 0, + rollup_withdrawal_event_id: String::new(), + }), + Action::ValidatorUpdate(ValidatorUpdate { + power: 100, + verification_key: VerificationKey::try_from([0; 32]).unwrap(), + }), + Action::Ics20Withdrawal(Ics20Withdrawal { + denom: asset.clone(), + destination_chain_address: String::new(), + return_address: address, + amount: 1_000_000u128, + memo: String::new(), + fee_asset: asset.clone(), + timeout_height: Height::new(1, 1).unwrap(), + timeout_time: 0, + source_channel: "channel-0".parse().unwrap(), + bridge_address: Some(address), + use_compat_address: false, + }), + ]; + + assert!(matches!( + Actions::try_from_list_of_actions(actions).unwrap().group(), + Some(ActionGroup::BundleableGeneral) + )); +} + +#[test] +fn from_list_of_actions_bundleable_sudo() { + let address: Address<_> = Address::builder() + .array([0; 20]) + .prefix(ASTRIA_ADDRESS_PREFIX) + .try_build() + .unwrap(); + + let asset: Denom = "nria".parse().unwrap(); + let actions = vec![ + Action::FeeChange(FeeChangeAction { + fee_change: FeeChange::TransferBaseFee, + new_value: 100, + }), + Action::FeeAssetChange(FeeAssetChangeAction::Addition(asset)), + Action::IbcRelayerChange(IbcRelayerChangeAction::Addition(address)), + ]; + + assert!(matches!( + Actions::try_from_list_of_actions(actions).unwrap().group(), + Some(ActionGroup::BundleableSudo) + )); +} + +#[test] +fn from_list_of_actions_unbundleable_sudo() { + let address: Address<_> = Address::builder() + .array([0; 20]) + .prefix(ASTRIA_ADDRESS_PREFIX) + .try_build() + .unwrap(); + + let actions = vec![Action::SudoAddressChange(SudoAddressChangeAction { + new_address: address, + })]; + + assert!(matches!( + Actions::try_from_list_of_actions(actions).unwrap().group(), + Some(ActionGroup::UnbundleableSudo) + )); + + let actions = vec![Action::IbcSudoChange(IbcSudoChangeAction { + new_address: address, + })]; + + assert!(matches!( + Actions::try_from_list_of_actions(actions).unwrap().group(), + Some(ActionGroup::UnbundleableSudo) + )); + + let actions = vec![ + Action::SudoAddressChange(SudoAddressChangeAction { + new_address: address, + }), + Action::SudoAddressChange(SudoAddressChangeAction { + new_address: address, + }), + ]; + + let error_kind = Actions::try_from_list_of_actions(actions).unwrap_err().0; + assert!( + matches!(error_kind, ErrorKind::NotBundleable { .. }), + "expected ErrorKind::NotBundleable, got {error_kind:?}" + ); +} + +#[test] +fn from_list_of_actions_unbundleable_general() { + let address: Address<_> = Address::builder() + .array([0; 20]) + .prefix(ASTRIA_ADDRESS_PREFIX) + .try_build() + .unwrap(); + + let asset: Denom = "nria".parse().unwrap(); + + let init_bridge_account_action = InitBridgeAccountAction { + rollup_id: RollupId::from([8; 32]), + asset: asset.clone(), + fee_asset: asset.clone(), + sudo_address: Some(address), + withdrawer_address: Some(address), + }; + + let sudo_bridge_address_change_action = BridgeSudoChangeAction { + new_sudo_address: Some(address), + bridge_address: address, + new_withdrawer_address: Some(address), + fee_asset: asset.clone(), + }; + + let actions = vec![init_bridge_account_action.clone().into()]; + + assert!(matches!( + Actions::try_from_list_of_actions(actions).unwrap().group(), + Some(ActionGroup::UnbundleableGeneral) + )); + + let actions = vec![sudo_bridge_address_change_action.clone().into()]; + + assert!(matches!( + Actions::try_from_list_of_actions(actions).unwrap().group(), + Some(ActionGroup::UnbundleableGeneral) + )); + + let actions = vec![ + init_bridge_account_action.into(), + sudo_bridge_address_change_action.into(), + ]; + + let error_kind = Actions::try_from_list_of_actions(actions).unwrap_err().0; + assert!( + matches!(error_kind, ErrorKind::NotBundleable { .. }), + "expected ErrorKind::NotBundleable, got {error_kind:?}" + ); +} + +#[test] +fn from_list_of_actions_mixed() { + let address: Address<_> = Address::builder() + .array([0; 20]) + .prefix(ASTRIA_ADDRESS_PREFIX) + .try_build() + .unwrap(); + + let asset: Denom = "nria".parse().unwrap(); + let actions = vec![ + Action::Sequence(SequenceAction { + rollup_id: RollupId::from([8; 32]), + data: vec![].into(), + fee_asset: asset.clone(), + }), + Action::SudoAddressChange(SudoAddressChangeAction { + new_address: address, + }), + ]; + + let error_kind = Actions::try_from_list_of_actions(actions).unwrap_err().0; + assert!( + matches!(error_kind, ErrorKind::Mixed { .. }), + "expected ErrorKind::Mixed, got {error_kind:?}" + ); +} diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs index d7d926a6c1..67f86add44 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs @@ -1,3 +1,4 @@ +use action_group::Actions; use bytes::Bytes; use prost::{ Message as _, @@ -21,6 +22,7 @@ use crate::{ }; pub mod action; +pub mod action_group; pub use action::Action; #[derive(Debug, thiserror::Error)] @@ -197,7 +199,16 @@ impl SignedTransaction { #[must_use] pub fn actions(&self) -> &[Action] { - &self.transaction.actions + self.transaction.actions.actions() + } + + #[must_use] + pub fn is_bundleable_sudo_action_group(&self) -> bool { + if let Some(group) = self.transaction.actions.group() { + group.is_bundleable_sudo() + } else { + false + } } #[must_use] @@ -227,11 +238,26 @@ impl SignedTransaction { #[derive(Clone, Debug)] pub struct UnsignedTransaction { - pub actions: Vec, - pub params: TransactionParams, + actions: Actions, + params: TransactionParams, } impl UnsignedTransaction { + #[must_use] + pub fn builder() -> UnsignedTransactionBuilder { + UnsignedTransactionBuilder::new() + } + + #[must_use] + pub fn into_actions(self) -> Vec { + self.actions.into_actions() + } + + #[must_use] + pub fn actions(&self) -> &[Action] { + self.actions.actions() + } + #[must_use] pub fn nonce(&self) -> u32 { self.params.nonce @@ -260,7 +286,11 @@ impl UnsignedTransaction { actions, params, } = self; - let actions = actions.into_iter().map(Action::into_raw).collect(); + let actions = actions + .into_actions() + .into_iter() + .map(Action::into_raw) + .collect(); raw::UnsignedTransaction { actions, params: Some(params.into_raw()), @@ -281,7 +311,7 @@ impl UnsignedTransaction { actions, params, } = self; - let actions = actions.iter().map(Action::to_raw).collect(); + let actions = actions.actions().iter().map(Action::to_raw).collect(); let params = params.clone().into_raw(); raw::UnsignedTransaction { actions, @@ -315,10 +345,12 @@ impl UnsignedTransaction { .collect::>() .map_err(UnsignedTransactionError::action)?; - Ok(Self { - actions, - params, - }) + UnsignedTransaction::builder() + .actions(actions) + .chain_id(params.chain_id) + .nonce(params.nonce) + .try_build() + .map_err(UnsignedTransactionError::action_group) } /// Attempt to convert from a protobuf [`pbjson_types::Any`]. @@ -360,6 +392,10 @@ impl UnsignedTransactionError { fn decode_any(inner: prost::DecodeError) -> Self { Self(UnsignedTransactionErrorKind::DecodeAny(inner)) } + + fn action_group(inner: action_group::Error) -> Self { + Self(UnsignedTransactionErrorKind::ActionGroup(inner)) + } } #[derive(Debug, thiserror::Error)] @@ -379,59 +415,69 @@ enum UnsignedTransactionErrorKind { raw::UnsignedTransaction::type_url() )] DecodeAny(#[source] prost::DecodeError), + #[error("`actions` field does not form a valid group of actions")] + ActionGroup(#[source] action_group::Error), } -pub struct TransactionParamsBuilder> { +#[derive(Default)] +pub struct UnsignedTransactionBuilder { nonce: u32, - chain_id: TChainId, + chain_id: String, + actions: Vec, } -impl TransactionParamsBuilder { - fn new() -> Self { +impl UnsignedTransactionBuilder { + #[must_use] + pub fn new() -> Self { + Self::default() + } + + #[must_use] + pub fn actions(self, actions: Vec) -> Self { Self { - nonce: 0, - chain_id: "".into(), + actions, + ..self } } -} -impl TransactionParamsBuilder { - #[must_use = "the transaction params builder must be built to be useful"] - pub fn chain_id<'a, T: Into>>( - self, - chain_id: T, - ) -> TransactionParamsBuilder> { - TransactionParamsBuilder { + #[must_use] + pub fn chain_id>(self, chain_id: T) -> UnsignedTransactionBuilder { + UnsignedTransactionBuilder { chain_id: chain_id.into(), nonce: self.nonce, + actions: self.actions, } } - #[must_use = "the transaction params builder must be built to be useful"] + #[must_use] pub fn nonce(self, nonce: u32) -> Self { Self { nonce, ..self } } -} -impl<'a> TransactionParamsBuilder> { - /// Constructs a [`TransactionParams`] from the configured builder. + /// Constructs a [`UnsignedTransaction`] from the configured builder. /// /// # Errors + /// Returns an error if the actions do not make a valid `ActionGroup`. + /// /// Returns an error if the set chain ID does not contain a chain name that can be turned into /// a bech32 human readable prefix (everything before the first dash i.e. `-`). - #[must_use] - pub fn build(self) -> TransactionParams { + pub fn try_build(self) -> Result { let Self { nonce, chain_id, + actions, } = self; - TransactionParams { - nonce, - chain_id: chain_id.into(), - } + let actions = Actions::try_from_list_of_actions(actions)?; + Ok(UnsignedTransaction { + actions, + params: TransactionParams { + nonce, + chain_id, + }, + }) } } @@ -442,11 +488,6 @@ pub struct TransactionParams { } impl TransactionParams { - #[must_use = "the transaction params builder must be built to be useful"] - pub fn builder() -> TransactionParamsBuilder { - TransactionParamsBuilder::new() - } - #[must_use] pub fn into_raw(self) -> raw::TransactionParams { let Self { @@ -461,16 +502,17 @@ impl TransactionParams { } /// Convert from a raw protobuf [`raw::UnsignedTransaction`]. - /// - /// # Errors - /// See [`TransactionParamsBuilder::try_build`] for errors returned by this method. #[must_use] pub fn from_raw(proto: raw::TransactionParams) -> Self { let raw::TransactionParams { nonce, chain_id, } = proto; - Self::builder().nonce(nonce).chain_id(chain_id).build() + + Self { + nonce, + chain_id, + } } } @@ -555,10 +597,7 @@ enum TransactionFeeResponseErrorKind { mod tests { use super::*; use crate::{ - primitive::v1::{ - asset, - Address, - }, + primitive::v1::Address, protocol::transaction::v1alpha1::action::TransferAction, }; const ASTRIA_ADDRESS_PREFIX: &str = "astria"; @@ -592,14 +631,12 @@ mod tests { fee_asset: asset(), }; - let params = TransactionParams::from_raw(raw::TransactionParams { - nonce: 1, - chain_id: "test-1".to_string(), - }); - let unsigned = UnsignedTransaction { - actions: vec![transfer.into()], - params, - }; + let unsigned = UnsignedTransaction::builder() + .actions(vec![transfer.into()]) + .chain_id("test-1".to_string()) + .nonce(1) + .try_build() + .unwrap(); let tx = SignedTransaction { signature, @@ -629,16 +666,14 @@ mod tests { fee_asset: asset(), }; - let params = TransactionParams::from_raw(raw::TransactionParams { - nonce: 1, - chain_id: "test-1".to_string(), - }); - let unsigned = UnsignedTransaction { - actions: vec![transfer.into()], - params, - }; + let unsigned_tx = UnsignedTransaction::builder() + .actions(vec![transfer.into()]) + .chain_id("test-1".to_string()) + .nonce(1) + .try_build() + .unwrap(); - let signed_tx = unsigned.into_signed(&signing_key); + let signed_tx = unsigned_tx.into_signed(&signing_key); let raw = signed_tx.to_raw(); // `try_from_raw` verifies the signature diff --git a/crates/astria-core/src/sequencerblock/v1alpha1/block.rs b/crates/astria-core/src/sequencerblock/v1alpha1/block.rs index 8dee1fdd80..6df6428e0e 100644 --- a/crates/astria-core/src/sequencerblock/v1alpha1/block.rs +++ b/crates/astria-core/src/sequencerblock/v1alpha1/block.rs @@ -737,7 +737,7 @@ impl SequencerBlock { .map_err(SequencerBlockError::signed_transaction_protobuf_decode)?; let signed_tx = SignedTransaction::try_from_raw(raw_tx) .map_err(SequencerBlockError::raw_signed_transaction_conversion)?; - for action in signed_tx.into_unsigned().actions { + for action in signed_tx.into_unsigned().into_actions() { // XXX: The fee asset is dropped. We shjould explain why that's ok. if let action::Action::Sequence(action::SequenceAction { rollup_id, diff --git a/crates/astria-sequencer-client/src/tests/http.rs b/crates/astria-sequencer-client/src/tests/http.rs index 5bfbc89f00..c23f14b1d1 100644 --- a/crates/astria-sequencer-client/src/tests/http.rs +++ b/crates/astria-sequencer-client/src/tests/http.rs @@ -7,7 +7,6 @@ use astria_core::{ protocol::transaction::v1alpha1::{ action::TransferAction, SignedTransaction, - TransactionParams, UnsignedTransaction, }, }; @@ -157,14 +156,13 @@ fn create_signed_transaction() -> SignedTransaction { } .into(), ]; - UnsignedTransaction { - params: TransactionParams::builder() - .nonce(1) - .chain_id("test") - .build(), - actions, - } - .into_signed(&alice_key) + UnsignedTransaction::builder() + .actions(actions) + .chain_id("test") + .nonce(1) + .try_build() + .unwrap() + .into_signed(&alice_key) } #[tokio::test] diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index 921f0fe9ac..7982c3df06 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -530,7 +530,7 @@ impl App { // check if tx's sequence data will fit into sequence block let tx_sequence_data_bytes = tx .unsigned_transaction() - .actions + .actions() .iter() .filter_map(Action::as_sequence) .fold(0usize, |acc, seq| acc.saturating_add(seq.data.len())); @@ -650,7 +650,7 @@ impl App { // check if tx's sequence data will fit into sequence block let tx_sequence_data_bytes = tx .unsigned_transaction() - .actions + .actions() .iter() .filter_map(Action::as_sequence) .fold(0usize, |acc, seq| acc.saturating_add(seq.data.len())); @@ -1012,10 +1012,11 @@ impl App { // flag mempool for cleaning if we ran a fee change action self.recost_mempool = self.recost_mempool - || signed_tx - .actions() - .iter() - .any(|action| matches!(action, Action::FeeAssetChange(_) | Action::FeeChange(_))); + || signed_tx.is_bundleable_sudo_action_group() + && signed_tx + .actions() + .iter() + .any(|act| act.is_fee_asset_change() || act.is_fee_change()); Ok(state_tx.apply().1) } diff --git a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap index 52c47f2f6a..e02ff29153 100644 --- a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap +++ b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap @@ -1,39 +1,38 @@ --- source: crates/astria-sequencer/src/app/tests_breaking_changes.rs -assertion_line: 308 expression: app.app_hash.as_bytes() --- [ - 67, - 124, - 63, - 240, - 228, - 207, - 78, - 64, - 191, - 89, - 84, - 121, - 150, - 21, - 207, - 248, - 173, - 132, - 77, + 96, 126, - 148, - 252, - 239, - 104, - 130, - 55, - 201, - 32, - 57, - 167, + 64, + 175, + 139, + 186, + 101, + 145, + 115, + 19, + 45, + 255, + 124, + 232, + 128, + 181, + 193, + 25, + 192, + 26, + 185, + 10, + 113, + 193, + 151, + 38, + 1, 215, - 228 + 173, + 183, + 175, + 217 ] diff --git a/crates/astria-sequencer/src/app/test_utils.rs b/crates/astria-sequencer/src/app/test_utils.rs index be68c81fc3..508c6003c6 100644 --- a/crates/astria-sequencer/src/app/test_utils.rs +++ b/crates/astria-sequencer/src/app/test_utils.rs @@ -24,7 +24,6 @@ use astria_core::{ ValidatorUpdate, }, SignedTransaction, - TransactionParams, UnsignedTransaction, }, }, @@ -263,20 +262,19 @@ pub(crate) fn mock_tx( signer: &SigningKey, rollup_name: &str, ) -> Arc { - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(nonce) - .chain_id("test") - .build(), - actions: vec![ + let tx = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from_unhashed_bytes(rollup_name.as_bytes()), data: Bytes::from_static(&[0x99]), fee_asset: denom_0(), } .into(), - ], - }; + ]) + .chain_id("test") + .nonce(nonce) + .try_build() + .unwrap(); Arc::new(tx.into_signed(signer)) } diff --git a/crates/astria-sequencer/src/app/tests_app/mempool.rs b/crates/astria-sequencer/src/app/tests_app/mempool.rs index ec325f6e28..d90f969a0b 100644 --- a/crates/astria-sequencer/src/app/tests_app/mempool.rs +++ b/crates/astria-sequencer/src/app/tests_app/mempool.rs @@ -9,7 +9,6 @@ use astria_core::{ FeeChangeAction, TransferAction, }, - TransactionParams, UnsignedTransaction, }, }, @@ -47,23 +46,20 @@ async fn trigger_cleaning() { let (mut app, storage) = initialize_app_with_storage(None, vec![]).await; app.prepare_commit(storage.clone()).await.unwrap(); app.commit(storage.clone()).await; - let sudo = get_judy_signing_key(); // create tx which will cause mempool cleaning flag to be set - let tx_trigger = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx_trigger = UnsignedTransaction::builder() + .actions(vec![ FeeChangeAction { fee_change: FeeChange::TransferBaseFee, new_value: 10, } .into(), - ], - } - .into_signed(&sudo); + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&get_judy_signing_key()); app.mempool .insert( @@ -146,24 +142,20 @@ async fn do_not_trigger_cleaning() { app.prepare_commit(storage.clone()).await.unwrap(); app.commit(storage.clone()).await; - let alice = get_alice_signing_key(); - // create tx which will fail execution and not trigger flag // (wrong sudo signer) - let tx_fail = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx_fail = UnsignedTransaction::builder() + .actions(vec![ FeeChangeAction { fee_change: FeeChange::TransferBaseFee, new_value: 10, } .into(), - ], - } - .into_signed(&alice); + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&get_alice_signing_key()); app.mempool .insert( @@ -223,12 +215,8 @@ async fn maintenance_recosting_promotes() { // create tx which will not be included in block due to // having insufficient funds (transaction will be recosted to enable) - let tx_fail_recost_funds = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx_fail_recost_funds = UnsignedTransaction::builder() + .actions(vec![ TransferAction { to: astria_address_from_hex_string(CAROL_ADDRESS), amount: 1u128, @@ -236,9 +224,11 @@ async fn maintenance_recosting_promotes() { fee_asset: nria().into(), } .into(), - ], - } - .into_signed(&get_bob_signing_key()); + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&get_bob_signing_key()); let mut bob_funds = HashMap::new(); bob_funds.insert(nria().into(), 11); @@ -255,20 +245,18 @@ async fn maintenance_recosting_promotes() { .unwrap(); // create tx which will enable recost tx to pass - let tx_recost = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx_recost = UnsignedTransaction::builder() + .actions(vec![ FeeChangeAction { fee_change: FeeChange::TransferBaseFee, new_value: 10, // originally 12 } .into(), - ], - } - .into_signed(&get_judy_signing_key()); + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&get_judy_signing_key()); let mut judy_funds = HashMap::new(); judy_funds.insert(nria().into(), 0); @@ -396,12 +384,8 @@ async fn maintenance_funds_added_promotes() { // create tx that will not be included in block due to // having no funds (will be sent transfer to then enable) - let tx_fail_transfer_funds = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx_fail_transfer_funds = UnsignedTransaction::builder() + .actions(vec![ TransferAction { to: astria_address_from_hex_string(BOB_ADDRESS), amount: 10u128, @@ -409,9 +393,11 @@ async fn maintenance_funds_added_promotes() { fee_asset: nria().into(), } .into(), - ], - } - .into_signed(&get_carol_signing_key()); + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&get_carol_signing_key()); let mut carol_funds = HashMap::new(); carol_funds.insert(nria().into(), 0); @@ -428,12 +414,8 @@ async fn maintenance_funds_added_promotes() { .unwrap(); // create tx which will enable no funds to pass - let tx_fund = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx_fund = UnsignedTransaction::builder() + .actions(vec![ TransferAction { to: astria_address_from_hex_string(CAROL_ADDRESS), amount: 22u128, @@ -441,9 +423,11 @@ async fn maintenance_funds_added_promotes() { fee_asset: nria().into(), } .into(), - ], - } - .into_signed(&get_alice_signing_key()); + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&get_alice_signing_key()); let mut alice_funds = HashMap::new(); alice_funds.insert(nria().into(), 100); diff --git a/crates/astria-sequencer/src/app/tests_app/mod.rs b/crates/astria-sequencer/src/app/tests_app/mod.rs index f62c655bf9..7967e5a0d8 100644 --- a/crates/astria-sequencer/src/app/tests_app/mod.rs +++ b/crates/astria-sequencer/src/app/tests_app/mod.rs @@ -16,7 +16,6 @@ use astria_core::{ SequenceAction, TransferAction, }, - TransactionParams, UnsignedTransaction, }, }, @@ -229,12 +228,8 @@ async fn app_transfer_block_fees_to_sudo() { // transfer funds from Alice to Bob; use native token for fee payment let bob_address = astria_address_from_hex_string(BOB_ADDRESS); let amount = 333_333; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx = UnsignedTransaction::builder() + .actions(vec![ TransferAction { to: bob_address, amount, @@ -242,8 +237,10 @@ async fn app_transfer_block_fees_to_sudo() { fee_asset: nria().into(), } .into(), - ], - }; + ]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = tx.into_signed(&alice); @@ -336,13 +333,12 @@ async fn app_create_sequencer_block_with_sequenced_data_and_deposits() { data: Bytes::from_static(b"hello world"), fee_asset: nria().into(), }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![lock_action.into(), sequence_action.into()], - }; + + let tx = UnsignedTransaction::builder() + .actions(vec![lock_action.into(), sequence_action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = tx.into_signed(&alice); @@ -427,13 +423,12 @@ async fn app_execution_results_match_proposal_vs_after_proposal() { data: Bytes::from_static(b"hello world"), fee_asset: nria().into(), }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![lock_action.into(), sequence_action.into()], - }; + + let tx = UnsignedTransaction::builder() + .actions(vec![lock_action.into(), sequence_action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = tx.into_signed(&alice); @@ -564,36 +559,34 @@ async fn app_prepare_proposal_cometbft_max_bytes_overflow_ok() { // create txs which will cause cometBFT overflow let alice = get_alice_signing_key(); - let tx_pass = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx_pass = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from([1u8; 32]), data: Bytes::copy_from_slice(&[1u8; 100_000]), fee_asset: nria().into(), } .into(), - ], - } - .into_signed(&alice); - let tx_overflow = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(1) - .chain_id("test") - .build(), - actions: vec![ + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&alice); + + let tx_overflow = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from([1u8; 32]), data: Bytes::copy_from_slice(&[1u8; 100_000]), fee_asset: nria().into(), } .into(), - ], - } - .into_signed(&alice); + ]) + .chain_id("test") + .nonce(1) + .try_build() + .unwrap() + .into_signed(&alice); app.mempool .insert( @@ -656,36 +649,33 @@ async fn app_prepare_proposal_sequencer_max_bytes_overflow_ok() { // create txs which will cause sequencer overflow (max is currently 256_000 bytes) let alice = get_alice_signing_key(); - let tx_pass = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx_pass = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from([1u8; 32]), data: Bytes::copy_from_slice(&[1u8; 200_000]), fee_asset: nria().into(), } .into(), - ], - } - .into_signed(&alice); - let tx_overflow = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(1) - .chain_id("test") - .build(), - actions: vec![ + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&alice); + let tx_overflow = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from([1u8; 32]), data: Bytes::copy_from_slice(&[1u8; 100_000]), fee_asset: nria().into(), } .into(), - ], - } - .into_signed(&alice); + ]) + .nonce(1) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&alice); app.mempool .insert( diff --git a/crates/astria-sequencer/src/app/tests_block_fees.rs b/crates/astria-sequencer/src/app/tests_block_fees.rs index 63f6b92f3c..14efea59be 100644 --- a/crates/astria-sequencer/src/app/tests_block_fees.rs +++ b/crates/astria-sequencer/src/app/tests_block_fees.rs @@ -10,7 +10,6 @@ use astria_core::{ SequenceAction, TransferAction, }, - TransactionParams, UnsignedTransaction, }, sequencerblock::v1alpha1::block::Deposit, @@ -53,12 +52,8 @@ async fn transaction_execution_records_fee_event() { let alice = get_alice_signing_key(); let bob_address = astria_address_from_hex_string(BOB_ADDRESS); let value = 333_333; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx = UnsignedTransaction::builder() + .actions(vec![ TransferAction { to: bob_address, amount: value, @@ -66,9 +61,10 @@ async fn transaction_execution_records_fee_event() { fee_asset: nria().into(), } .into(), - ], - }; - + ]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); let events = app.execute_transaction(signed_tx).await.unwrap(); @@ -114,13 +110,11 @@ async fn ensure_correct_block_fees_transfer() { .into(), ]; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions, - }; + let tx = UnsignedTransaction::builder() + .actions(actions) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -155,13 +149,11 @@ async fn ensure_correct_block_fees_sequence() { .into(), ]; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions, - }; + let tx = UnsignedTransaction::builder() + .actions(actions) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -198,13 +190,11 @@ async fn ensure_correct_block_fees_init_bridge_acct() { .into(), ]; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions, - }; + let tx = UnsignedTransaction::builder() + .actions(actions) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -252,13 +242,11 @@ async fn ensure_correct_block_fees_bridge_lock() { .into(), ]; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions, - }; + let tx = UnsignedTransaction::builder() + .actions(actions) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx.clone()).await.unwrap(); @@ -314,13 +302,11 @@ async fn ensure_correct_block_fees_bridge_sudo_change() { .into(), ]; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions, - }; + let tx = UnsignedTransaction::builder() + .actions(actions) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 4fd6225973..ef578114ad 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -30,7 +30,6 @@ use astria_core::{ ValidatorUpdate, }, Action, - TransactionParams, UnsignedTransaction, }, }, @@ -112,13 +111,12 @@ async fn app_finalize_block_snapshot() { data: Bytes::from_static(b"hello world"), fee_asset: nria().into(), }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![lock_action.into(), sequence_action.into()], - }; + + let tx = UnsignedTransaction::builder() + .actions(vec![lock_action.into(), sequence_action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = tx.into_signed(&alice); @@ -202,12 +200,8 @@ async fn app_execute_transaction_with_every_action_snapshot() { let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx_bundleable_general = UnsignedTransaction::builder() + .actions(vec![ TransferAction { to: bob_address, amount: 333_333, @@ -222,33 +216,67 @@ async fn app_execute_transaction_with_every_action_snapshot() { } .into(), Action::ValidatorUpdate(update.clone()), + ]) + .chain_id("test") + .try_build() + .unwrap(); + + let tx_bundleable_sudo = UnsignedTransaction::builder() + .actions(vec![ IbcRelayerChangeAction::Addition(bob_address).into(), IbcRelayerChangeAction::Addition(carol_address).into(), IbcRelayerChangeAction::Removal(bob_address).into(), - // TODO: should fee assets be stored in state? FeeAssetChangeAction::Addition("test-0".parse().unwrap()).into(), FeeAssetChangeAction::Addition("test-1".parse().unwrap()).into(), FeeAssetChangeAction::Removal("test-0".parse().unwrap()).into(), + ]) + .nonce(1) + .chain_id("test") + .try_build() + .unwrap(); + + let tx_sudo_ibc = UnsignedTransaction::builder() + .actions(vec![ IbcSudoChangeAction { new_address: bob_address, } .into(), + ]) + .nonce(2) + .chain_id("test") + .try_build() + .unwrap(); + + let tx_sudo = UnsignedTransaction::builder() + .actions(vec![ SudoAddressChangeAction { new_address: bob_address, } .into(), - ], - }; + ]) + .nonce(3) + .chain_id("test") + .try_build() + .unwrap(); - let signed_tx = Arc::new(tx.into_signed(&alice)); - app.execute_transaction(signed_tx).await.unwrap(); + let signed_tx_general_bundleable = Arc::new(tx_bundleable_general.into_signed(&alice)); + app.execute_transaction(signed_tx_general_bundleable) + .await + .unwrap(); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let signed_tx_sudo_bundleable = Arc::new(tx_bundleable_sudo.into_signed(&alice)); + app.execute_transaction(signed_tx_sudo_bundleable) + .await + .unwrap(); + + let signed_tx_sudo_ibc = Arc::new(tx_sudo_ibc.into_signed(&alice)); + app.execute_transaction(signed_tx_sudo_ibc).await.unwrap(); + + let signed_tx_sudo = Arc::new(tx_sudo.into_signed(&alice)); + app.execute_transaction(signed_tx_sudo).await.unwrap(); + + let tx = UnsignedTransaction::builder() + .actions(vec![ InitBridgeAccountAction { rollup_id, asset: nria().into(), @@ -257,17 +285,15 @@ async fn app_execute_transaction_with_every_action_snapshot() { withdrawer_address: None, } .into(), - ], - }; + ]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&bridge)); app.execute_transaction(signed_tx).await.unwrap(); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .chain_id("test") - .nonce(1) - .build(), - actions: vec![ + let tx_bridge_bundleable = UnsignedTransaction::builder() + .actions(vec![ BridgeLockAction { to: bridge_address, amount: 100, @@ -286,6 +312,17 @@ async fn app_execute_transaction_with_every_action_snapshot() { rollup_withdrawal_event_id: "a-rollup-defined-hash".to_string(), } .into(), + ]) + .nonce(1) + .chain_id("test") + .try_build() + .unwrap(); + + let signed_tx = Arc::new(tx_bridge_bundleable.into_signed(&bridge)); + app.execute_transaction(signed_tx).await.unwrap(); + + let tx_bridge = UnsignedTransaction::builder() + .actions(vec![ BridgeSudoChangeAction { bridge_address, new_sudo_address: Some(bob_address), @@ -293,10 +330,13 @@ async fn app_execute_transaction_with_every_action_snapshot() { fee_asset: nria().into(), } .into(), - ], - }; + ]) + .nonce(2) + .chain_id("test") + .try_build() + .unwrap(); - let signed_tx = Arc::new(tx.into_signed(&bridge)); + let signed_tx = Arc::new(tx_bridge.into_signed(&bridge)); app.execute_transaction(signed_tx).await.unwrap(); let sudo_address = app.state.get_sudo_address().await.unwrap(); diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 30a6290ecf..6dd088face 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -20,7 +20,6 @@ use astria_core::{ ValidatorUpdate, }, Action, - TransactionParams, UnsignedTransaction, }, }, @@ -104,12 +103,8 @@ async fn app_execute_transaction_transfer() { let alice_address = astria_address(&alice.address_bytes()); let bob_address = astria_address_from_hex_string(BOB_ADDRESS); let value = 333_333; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx = UnsignedTransaction::builder() + .actions(vec![ TransferAction { to: bob_address, amount: value, @@ -117,8 +112,10 @@ async fn app_execute_transaction_transfer() { fee_asset: crate::test_utils::nria().into(), } .into(), - ], - }; + ]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -161,12 +158,8 @@ async fn app_execute_transaction_transfer_not_native_token() { // transfer funds from Alice to Bob; use native token for fee payment let bob_address = astria_address_from_hex_string(BOB_ADDRESS); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx = UnsignedTransaction::builder() + .actions(vec![ TransferAction { to: bob_address, amount: value, @@ -174,8 +167,10 @@ async fn app_execute_transaction_transfer_not_native_token() { fee_asset: nria().into(), } .into(), - ], - }; + ]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -226,12 +221,8 @@ async fn app_execute_transaction_transfer_balance_too_low_for_fee() { let bob = astria_address_from_hex_string(BOB_ADDRESS); // 0-value transfer; only fee is deducted from sender - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx = UnsignedTransaction::builder() + .actions(vec![ TransferAction { to: bob, amount: 0, @@ -239,8 +230,10 @@ async fn app_execute_transaction_transfer_balance_too_low_for_fee() { fee_asset: nria().into(), } .into(), - ], - }; + ]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&keypair)); let res = app @@ -267,20 +260,18 @@ async fn app_execute_transaction_sequence() { let data = Bytes::from_static(b"hello world"); let fee = calculate_fee_from_state(&data, &app.state).await.unwrap(); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), data, fee_asset: nria().into(), } .into(), - ], - }; + ]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -302,20 +293,18 @@ async fn app_execute_transaction_invalid_fee_asset() { let alice = get_alice_signing_key(); let data = Bytes::from_static(b"hello world"); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let tx = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), data, fee_asset: test_asset(), } .into(), - ], - }; + ]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); assert!(app.execute_transaction(signed_tx).await.is_err()); @@ -333,13 +322,11 @@ async fn app_execute_transaction_validator_update() { verification_key: crate::test_utils::verification_key(1), }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![Action::ValidatorUpdate(update.clone())], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![Action::ValidatorUpdate(update.clone())]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -360,13 +347,13 @@ async fn app_execute_transaction_ibc_relayer_change_addition() { let mut app = initialize_app(Some(genesis_state()), vec![]).await; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![IbcRelayerChangeAction::Addition(alice_address).into()], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![Action::IbcRelayerChange( + IbcRelayerChangeAction::Addition(alice_address), + )]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -388,14 +375,11 @@ async fn app_execute_transaction_ibc_relayer_change_deletion() { .unwrap(); let mut app = initialize_app(Some(genesis_state), vec![]).await; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![IbcRelayerChangeAction::Removal(alice_address).into()], - }; - + let tx = UnsignedTransaction::builder() + .actions(vec![IbcRelayerChangeAction::Removal(alice_address).into()]) + .chain_id("test") + .try_build() + .unwrap(); 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); @@ -418,13 +402,11 @@ async fn app_execute_transaction_ibc_relayer_change_invalid() { .unwrap(); let mut app = initialize_app(Some(genesis_state), vec![]).await; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![IbcRelayerChangeAction::Removal(alice_address).into()], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![IbcRelayerChangeAction::Removal(alice_address).into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); assert!(app.execute_transaction(signed_tx).await.is_err()); @@ -438,16 +420,13 @@ async fn app_execute_transaction_sudo_address_change() { let mut app = initialize_app(Some(genesis_state()), vec![]).await; let new_address = astria_address_from_hex_string(BOB_ADDRESS); - - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![Action::SudoAddressChange(SudoAddressChangeAction { + let tx = UnsignedTransaction::builder() + .actions(vec![Action::SudoAddressChange(SudoAddressChangeAction { new_address, - })], - }; + })]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -476,16 +455,13 @@ async fn app_execute_transaction_sudo_address_change_error() { .try_into() .unwrap(); let mut app = initialize_app(Some(genesis_state), vec![]).await; - - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![Action::SudoAddressChange(SudoAddressChangeAction { + let tx = UnsignedTransaction::builder() + .actions(vec![Action::SudoAddressChange(SudoAddressChangeAction { new_address: alice_address, - })], - }; + })]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); let res = app @@ -506,15 +482,13 @@ async fn app_execute_transaction_fee_asset_change_addition() { let mut app = initialize_app(Some(genesis_state()), vec![]).await; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![Action::FeeAssetChange(FeeAssetChangeAction::Addition( - test_asset(), - ))], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![Action::FeeAssetChange( + FeeAssetChangeAction::Addition(test_asset()), + )]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -539,15 +513,13 @@ async fn app_execute_transaction_fee_asset_change_removal() { .unwrap(); let mut app = initialize_app(Some(genesis_state), vec![]).await; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![Action::FeeAssetChange(FeeAssetChangeAction::Removal( + let tx = UnsignedTransaction::builder() + .actions(vec![Action::FeeAssetChange(FeeAssetChangeAction::Removal( test_asset(), - ))], - }; + ))]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -564,15 +536,13 @@ async fn app_execute_transaction_fee_asset_change_invalid() { let mut app = initialize_app(Some(genesis_state()), vec![]).await; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![Action::FeeAssetChange(FeeAssetChangeAction::Removal( + let tx = UnsignedTransaction::builder() + .actions(vec![Action::FeeAssetChange(FeeAssetChangeAction::Removal( nria().into(), - ))], - }; + ))]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); let res = app @@ -605,13 +575,12 @@ async fn app_execute_transaction_init_bridge_account_ok() { sudo_address: None, withdrawer_address: None, }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![action.into()], - }; + + let tx = UnsignedTransaction::builder() + .actions(vec![action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); @@ -661,14 +630,11 @@ async fn app_execute_transaction_init_bridge_account_account_already_registered( sudo_address: None, withdrawer_address: None, }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - - actions: vec![action.into()], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -680,13 +646,12 @@ async fn app_execute_transaction_init_bridge_account_account_already_registered( sudo_address: None, withdrawer_address: None, }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![action.into()], - }; + + let tx = UnsignedTransaction::builder() + .actions(vec![action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); assert!(app.execute_transaction(signed_tx).await.is_err()); @@ -717,13 +682,11 @@ async fn app_execute_transaction_bridge_lock_action_ok() { fee_asset: nria().into(), destination_chain_address: "nootwashere".to_string(), }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![action.into()], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); @@ -797,13 +760,11 @@ async fn app_execute_transaction_bridge_lock_action_invalid_for_eoa() { fee_asset: nria().into(), destination_chain_address: "nootwashere".to_string(), }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![action.into()], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); assert!(app.execute_transaction(signed_tx).await.is_err()); @@ -818,20 +779,20 @@ async fn app_execute_transaction_invalid_nonce() { // create tx with invalid nonce 1 let data = Bytes::from_static(b"hello world"); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(1) - .chain_id("test") - .build(), - actions: vec![ + + let tx = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), data, fee_asset: nria().into(), } .into(), - ], - }; + ]) + .nonce(1) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); let response = app.execute_transaction(signed_tx).await; @@ -865,21 +826,18 @@ async fn app_execute_transaction_invalid_chain_id() { // create tx with invalid nonce 1 let data = Bytes::from_static(b"hello world"); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("wrong-chain") - .build(), - actions: vec![ + let tx = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), data, fee_asset: nria().into(), } .into(), - ], - }; - + ]) + .chain_id("wrong-chain") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); let response = app.execute_transaction(signed_tx).await; @@ -922,12 +880,8 @@ async fn app_stateful_check_fails_insufficient_total_balance() { .unwrap(); // transfer just enough to cover single sequence fee with data - let signed_tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let signed_tx = UnsignedTransaction::builder() + .actions(vec![ TransferAction { to: keypair_address, amount: fee, @@ -935,20 +889,16 @@ async fn app_stateful_check_fails_insufficient_total_balance() { fee_asset: nria().into(), } .into(), - ], - } - .into_signed(&alice); - - // make transfer + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&alice); app.execute_transaction(Arc::new(signed_tx)).await.unwrap(); // build double transfer exceeding balance - let signed_tx_fail = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let signed_tx_fail = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), data: data.clone(), @@ -961,10 +911,11 @@ async fn app_stateful_check_fails_insufficient_total_balance() { fee_asset: nria().into(), } .into(), - ], - } - .into_signed(&keypair); - + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&keypair); // try double, see fails stateful check let res = signed_tx_fail .check_and_execute(Arc::get_mut(&mut app.state).unwrap()) @@ -975,21 +926,19 @@ async fn app_stateful_check_fails_insufficient_total_balance() { assert!(res.contains("insufficient funds for asset")); // build single transfer to see passes - let signed_tx_pass = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + let signed_tx_pass = UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), data, fee_asset: nria().into(), } .into(), - ], - } - .into_signed(&keypair); + ]) + .chain_id("test") + .try_build() + .unwrap() + .into_signed(&keypair); signed_tx_pass .check_and_execute(Arc::get_mut(&mut app.state).unwrap()) @@ -1034,13 +983,11 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { fee_asset: nria().into(), destination_chain_address: "nootwashere".to_string(), }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![action.into()], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); @@ -1058,13 +1005,11 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { rollup_withdrawal_event_id: "id-from-rollup".to_string(), }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![action.into()], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&bridge)); app.execute_transaction(signed_tx) @@ -1105,13 +1050,12 @@ async fn app_execute_transaction_action_index_correctly_increments() { fee_asset: nria().into(), destination_chain_address: "nootwashere".to_string(), }; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![action.clone().into(), action.into()], - }; + + let tx = UnsignedTransaction::builder() + .actions(vec![action.clone().into(), action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx.clone()).await.unwrap(); @@ -1142,23 +1086,19 @@ async fn transaction_execution_records_deposit_event() { state_tx .put_bridge_account_ibc_asset(bob_address, nria()) .unwrap(); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ - BridgeLockAction { - to: bob_address, - amount: 1, - asset: nria().into(), - fee_asset: nria().into(), - destination_chain_address: "test_chain_address".to_string(), - } - .into(), - ], - }; + let action = BridgeLockAction { + to: bob_address, + amount: 1, + asset: nria().into(), + fee_asset: nria().into(), + destination_chain_address: "test_chain_address".to_string(), + }; + let tx = UnsignedTransaction::builder() + .actions(vec![action.into()]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); let expected_deposit = Deposit { @@ -1189,15 +1129,13 @@ async fn app_execute_transaction_ibc_sudo_change() { let new_address = astria_address_from_hex_string(BOB_ADDRESS); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![Action::IbcSudoChange(IbcSudoChangeAction { + let tx = UnsignedTransaction::builder() + .actions(vec![Action::IbcSudoChange(IbcSudoChangeAction { new_address, - })], - }; + })]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); app.execute_transaction(signed_tx).await.unwrap(); @@ -1226,15 +1164,13 @@ async fn app_execute_transaction_ibc_sudo_change_error() { .unwrap(); let mut app = initialize_app(Some(genesis_state), vec![]).await; - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![Action::IbcSudoChange(IbcSudoChangeAction { + let tx = UnsignedTransaction::builder() + .actions(vec![Action::IbcSudoChange(IbcSudoChangeAction { new_address: alice_address, - })], - }; + })]) + .chain_id("test") + .try_build() + .unwrap(); let signed_tx = Arc::new(tx.into_signed(&alice)); let res = app diff --git a/crates/astria-sequencer/src/benchmark_utils.rs b/crates/astria-sequencer/src/benchmark_utils.rs index cbf9faafca..9cc901065f 100644 --- a/crates/astria-sequencer/src/benchmark_utils.rs +++ b/crates/astria-sequencer/src/benchmark_utils.rs @@ -22,7 +22,6 @@ use astria_core::{ TransferAction, }, SignedTransaction, - TransactionParams, UnsignedTransaction, }, }; @@ -86,21 +85,18 @@ fn sequence_actions() -> Vec> { let (nonce, chain_id) = nonces_and_chain_ids .entry(verification_key) .or_insert_with(|| (0_u32, format!("chain-{}", signing_key.verification_key()))); - let params = TransactionParams::builder() - .nonce(*nonce) - .chain_id(chain_id.as_str()) - .build(); - *nonce = (*nonce).wrapping_add(1); let sequence_action = SequenceAction { rollup_id: RollupId::new([1; 32]), data: vec![2; 1000].into(), fee_asset: Denom::IbcPrefixed(IbcPrefixed::new([3; 32])), }; - let tx = UnsignedTransaction { - actions: vec![Action::Sequence(sequence_action)], - params, - } - .into_signed(signing_key); + let tx = UnsignedTransaction::builder() + .actions(vec![Action::Sequence(sequence_action)]) + .nonce(*nonce) + .chain_id(chain_id.as_str()) + .try_build() + .expect("failed to build transaction from actions") + .into_signed(signing_key); Arc::new(tx) }) .take(SEQUENCE_ACTION_TX_COUNT) @@ -119,17 +115,17 @@ fn transfers() -> Vec> { }); (0..TRANSFERS_TX_COUNT) .map(|nonce| { - let params = TransactionParams::builder() + let tx = UnsignedTransaction::builder() + .actions( + std::iter::repeat(action.clone()) + .take(TRANSFERS_PER_TX) + .collect(), + ) .nonce(u32::try_from(nonce).unwrap()) .chain_id("test") - .build(); - let tx = UnsignedTransaction { - actions: std::iter::repeat(action.clone()) - .take(TRANSFERS_PER_TX) - .collect(), - params, - } - .into_signed(sender); + .try_build() + .expect("failed to build transaction from actions") + .into_signed(sender); Arc::new(tx) }) .collect() diff --git a/crates/astria-sequencer/src/proposal/commitment.rs b/crates/astria-sequencer/src/proposal/commitment.rs index bf902e0bdb..627cb1998f 100644 --- a/crates/astria-sequencer/src/proposal/commitment.rs +++ b/crates/astria-sequencer/src/proposal/commitment.rs @@ -98,7 +98,6 @@ mod tests { SequenceAction, TransferAction, }, - TransactionParams, UnsignedTransaction, }, }; @@ -121,13 +120,12 @@ mod tests { }; let signing_key = SigningKey::new(OsRng); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test-chain-1") - .build(), - actions: vec![sequence_action.clone().into(), transfer_action.into()], - }; + + let tx = UnsignedTransaction::builder() + .actions(vec![sequence_action.clone().into(), transfer_action.into()]) + .chain_id("test-chain-1") + .try_build() + .unwrap(); let signed_tx = tx.into_signed(&signing_key); let txs = vec![signed_tx]; @@ -137,13 +135,11 @@ mod tests { } = generate_rollup_datas_commitment(&txs, HashMap::new()); let signing_key = SigningKey::new(OsRng); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test-chain-1") - .build(), - actions: vec![sequence_action.into()], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![sequence_action.into()]) + .chain_id("test-chain-1") + .try_build() + .unwrap(); let signed_tx = tx.into_signed(&signing_key); let txs = vec![signed_tx]; @@ -175,13 +171,11 @@ mod tests { }; let signing_key = SigningKey::new(OsRng); - let tx = UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test-chain-1") - .build(), - actions: vec![sequence_action.into(), transfer_action.into()], - }; + let tx = UnsignedTransaction::builder() + .actions(vec![sequence_action.clone().into(), transfer_action.into()]) + .chain_id("test-chain-1") + .try_build() + .unwrap(); let signed_tx = tx.into_signed(&signing_key); let txs = vec![signed_tx]; diff --git a/crates/astria-sequencer/src/service/consensus.rs b/crates/astria-sequencer/src/service/consensus.rs index 001d7aceae..441e1f2268 100644 --- a/crates/astria-sequencer/src/service/consensus.rs +++ b/crates/astria-sequencer/src/service/consensus.rs @@ -211,7 +211,6 @@ mod tests { primitive::v1::RollupId, protocol::transaction::v1alpha1::{ action::SequenceAction, - TransactionParams, UnsignedTransaction, }, }; @@ -237,20 +236,18 @@ mod tests { }; fn make_unsigned_tx() -> UnsignedTransaction { - UnsignedTransaction { - params: TransactionParams::builder() - .nonce(0) - .chain_id("test") - .build(), - actions: vec![ + UnsignedTransaction::builder() + .actions(vec![ SequenceAction { rollup_id: RollupId::from_unhashed_bytes(b"testchainid"), data: Bytes::from_static(b"hello world"), fee_asset: crate::test_utils::nria().into(), } .into(), - ], - } + ]) + .chain_id("test") + .try_build() + .unwrap() } fn new_prepare_proposal_request() -> request::PrepareProposal { diff --git a/crates/astria-sequencer/src/transaction/checks.rs b/crates/astria-sequencer/src/transaction/checks.rs index b594b18c40..b4eedff56a 100644 --- a/crates/astria-sequencer/src/transaction/checks.rs +++ b/crates/astria-sequencer/src/transaction/checks.rs @@ -91,7 +91,7 @@ pub(crate) async fn get_fees_for_transaction( .wrap_err("failed to get bridge sudo change fee")?; let mut fees_by_asset = HashMap::new(); - for (i, action) in tx.actions.iter().enumerate() { + for (i, action) in tx.actions().iter().enumerate() { match action { Action::Transfer(act) => { transfer_update_fees(&act.fee_asset, &mut fees_by_asset, transfer_fee); @@ -316,12 +316,9 @@ mod tests { RollupId, ADDRESS_LEN, }, - protocol::transaction::v1alpha1::{ - action::{ - SequenceAction, - TransferAction, - }, - TransactionParams, + protocol::transaction::v1alpha1::action::{ + SequenceAction, + TransferAction, }, }; use bytes::Bytes; @@ -404,14 +401,11 @@ mod tests { }), ]; - let params = TransactionParams::builder() - .nonce(0) + let tx = UnsignedTransaction::builder() + .actions(actions) .chain_id("test-chain-id") - .build(); - let tx = UnsignedTransaction { - actions, - params, - }; + .try_build() + .unwrap(); let signed_tx = tx.into_signed(&alice); check_balance_for_total_fees_and_transfers(&signed_tx, &state_tx) @@ -470,14 +464,11 @@ mod tests { }), ]; - let params = TransactionParams::builder() - .nonce(0) + let tx = UnsignedTransaction::builder() + .actions(actions) .chain_id("test-chain-id") - .build(); - let tx = UnsignedTransaction { - actions, - params, - }; + .try_build() + .unwrap(); let signed_tx = tx.into_signed(&alice); let err = check_balance_for_total_fees_and_transfers(&signed_tx, &state_tx)