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
astriaorg · Sep 30, 2024 · 17e6711 · 17e6711
1 parent a9f5197
commit 17e6711
Show file tree

Hide file tree

Showing 25 changed files with 1,148 additions and 709 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<Action> {
-        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
@@ -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
@@ -17,8 +17,6 @@ use astria_core::{
         transaction::v1alpha1::{
             action::SequenceAction,
             SignedTransaction,
-            TransactionParams,
-            UnsignedTransaction,
         },
     },
 };
@@ -670,23 +668,17 @@ impl Future for SubmitFut {
     type Output = eyre::Result<u32>;
 
     // 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<Self::Output> {
         const INVALID_NONCE: Code = Code::Err(AbciErrorCode::INVALID_NONCE.value());
         loop {
             let this = self.as_mut().project();
 
             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
@@ -7,18 +7,15 @@ 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,
     primitive::v1::{
         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<RollupId, Vec<Deposit>> = 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
@@ -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<SequenceAction> for Action {
@@ -263,6 +272,33 @@ impl TryFrom<raw::Action> 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"