respond to comment by @SuperFluffy

crates/astria-composer/src/executor/bundle_factory/mod.rs

@@ -129,12 +129,6 @@ impl SizedBundle {
         self.curr_size
     }
 
-    /// Consume self and return the underlying buffer of actions.
-    #[cfg(test)]
-    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()

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);
@@ -236,7 +236,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);
@@ -261,7 +261,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);
@@ -296,7 +296,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);
@@ -306,7 +306,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);
@@ -335,7 +335,7 @@ mod bundle_factory {
     }
 
     #[test]
-    fn unsigned_transaction_construction() {
+    fn transaction_construction_does_not_panic() {
         let mut bundle_factory = BundleFactory::new(1000, 10);
 
         bundle_factory

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> {

crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs

@@ -81,7 +81,7 @@ impl Action {
 }
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
-pub enum ActionGroup {
+pub(super) enum ActionGroup {
     General,
     UnbundleableGeneral,
     Sudo,
@@ -114,11 +114,6 @@ impl fmt::Display for ActionGroup {
 pub struct Error(ErrorKind);
 
 impl Error {
-    #[must_use]
-    pub fn kind(&self) -> &ErrorKind {
-        &self.0
-    }
-
     fn mixed(
         original_group: ActionGroup,
         additional_group: ActionGroup,
@@ -139,7 +134,7 @@ impl Error {
 }
 
 #[derive(Debug, thiserror::Error)]
-pub enum ErrorKind {
+enum ErrorKind {
     #[error(
         "input contains mixed `ActionGroup` types. original group: {original_group}, additional \
          group: {additional_group}, triggering action: {action}"

crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs

@@ -417,7 +417,7 @@ enum UnsignedTransactionErrorKind {
         raw::UnsignedTransaction::type_url()
     )]
     DecodeAny(#[source] prost::DecodeError),
-    #[error("`actions` field does not make a valid `ActionGroup`")]
+    #[error("`actions` field does not form a valid group of actions")]
     ActionGroup(#[source] action_group::Error),
 }
 

crates/astria-sequencer/src/app/tests_breaking_changes.rs

@@ -23,16 +23,12 @@ use astria_core::{
                 BridgeLockAction,
                 BridgeSudoChangeAction,
                 BridgeUnlockAction,
-                FeeChange,
-                FeeChangeAction,
                 IbcRelayerChangeAction,
                 IbcSudoChangeAction,
-                Ics20Withdrawal,
                 SequenceAction,
                 TransferAction,
                 ValidatorUpdate,
             },
-            action_group,
             Action,
             UnsignedTransaction,
         },
@@ -41,7 +37,6 @@ use astria_core::{
     Protobuf,
 };
 use cnidarium::StateDelta;
-use ibc_types::core::client::Height;
 use prost::{
     bytes::Bytes,
     Message as _,
@@ -352,178 +347,3 @@ async fn app_execute_transaction_with_every_action_snapshot() {
 
     insta::assert_json_snapshot!(app.app_hash.as_bytes());
 }
-
-// Note: this test ensures that all actions are in their expected
-// bundleable vs non-bundleable category. Tests all actions except
-// IbcRelay.
-//
-// If any PR changes the bundleable status of an action, the PR must
-// be marked as breaking.
-#[allow(clippy::too_many_lines)]
-#[tokio::test]
-async fn app_transaction_bundle_categories() {
-    use astria_core::protocol::transaction::v1alpha1::action::{
-        FeeAssetChangeAction,
-        InitBridgeAccountAction,
-        SudoAddressChangeAction,
-    };
-
-    let bridge = get_bridge_signing_key();
-    let bridge_address = astria_address(&bridge.address_bytes());
-    let bob_address = astria_address_from_hex_string(BOB_ADDRESS);
-    let rollup_id = RollupId::from_unhashed_bytes(b"testchainid");
-
-    assert!(
-        UnsignedTransaction::builder()
-            .actions(vec![
-                TransferAction {
-                    to: bob_address,
-                    amount: 333_333,
-                    asset: nria().into(),
-                    fee_asset: nria().into(),
-                }
-                .into(),
-                SequenceAction {
-                    rollup_id: RollupId::from_unhashed_bytes(b"testchainid"),
-                    data: Bytes::from_static(b"hello world"),
-                    fee_asset: nria().into(),
-                }
-                .into(),
-                Action::ValidatorUpdate(ValidatorUpdate {
-                    power: 100,
-                    verification_key: crate::test_utils::verification_key(1),
-                }),
-                BridgeLockAction {
-                    to: bridge_address,
-                    amount: 100,
-                    asset: nria().into(),
-                    fee_asset: nria().into(),
-                    destination_chain_address: "nootwashere".to_string(),
-                }
-                .into(),
-                BridgeUnlockAction {
-                    to: bob_address,
-                    amount: 10,
-                    fee_asset: nria().into(),
-                    memo: String::new(),
-                    bridge_address: astria_address(&bridge.address_bytes()),
-                    rollup_block_number: 1,
-                    rollup_withdrawal_event_id: "a-rollup-defined-hash".to_string(),
-                }
-                .into(),
-                Ics20Withdrawal {
-                    amount: 1,
-                    denom: nria().into(),
-                    bridge_address: None,
-                    destination_chain_address: "test".to_string(),
-                    return_address: bob_address,
-                    timeout_height: Height::new(1, 1).unwrap(),
-                    timeout_time: 1,
-                    source_channel: "channel-0".to_string().parse().unwrap(),
-                    fee_asset: nria().into(),
-                    memo: String::new(),
-                    use_compat_address: false,
-                }
-                .into(),
-            ])
-            .chain_id("test")
-            .try_build()
-            .is_ok(),
-        "should be able to construct general bundle"
-    );
-
-    assert!(
-        UnsignedTransaction::builder()
-            .actions(vec![
-                IbcRelayerChangeAction::Addition(bob_address).into(),
-                FeeAssetChangeAction::Removal("test-0".parse().unwrap()).into(),
-                FeeChangeAction {
-                    fee_change: FeeChange::TransferBaseFee,
-                    new_value: 10,
-                }
-                .into(),
-            ])
-            .nonce(1)
-            .chain_id("test")
-            .try_build()
-            .is_ok(),
-        "should be able to construct sudo bundle"
-    );
-
-    let error = UnsignedTransaction::builder()
-        .actions(vec![
-            SudoAddressChangeAction {
-                new_address: bob_address,
-            }
-            .into(),
-            SudoAddressChangeAction {
-                new_address: bob_address,
-            }
-            .into(),
-        ])
-        .nonce(2)
-        .chain_id("test")
-        .try_build()
-        .unwrap_err();
-    assert!(
-        matches!(error.kind(), action_group::ErrorKind::NotBundleable { .. }),
-        "expected ErrorKind::NotBundleable, got {:?}",
-        error.kind()
-    );
-
-    let error = UnsignedTransaction::builder()
-        .actions(vec![
-            InitBridgeAccountAction {
-                rollup_id,
-                asset: nria().into(),
-                fee_asset: nria().into(),
-                sudo_address: None,
-                withdrawer_address: None,
-            }
-            .into(),
-            InitBridgeAccountAction {
-                rollup_id,
-                asset: nria().into(),
-                fee_asset: nria().into(),
-                sudo_address: None,
-                withdrawer_address: None,
-            }
-            .into(),
-        ])
-        .nonce(2)
-        .chain_id("test")
-        .try_build()
-        .unwrap_err();
-    assert!(
-        matches!(error.kind(), action_group::ErrorKind::NotBundleable { .. }),
-        "expected ErrorKind::NotBundleable, got {:?}",
-        error.kind()
-    );
-
-    let error = UnsignedTransaction::builder()
-        .actions(vec![
-            BridgeSudoChangeAction {
-                bridge_address,
-                new_sudo_address: Some(bob_address),
-                new_withdrawer_address: Some(bob_address),
-                fee_asset: nria().into(),
-            }
-            .into(),
-            BridgeSudoChangeAction {
-                bridge_address,
-                new_sudo_address: Some(bob_address),
-                new_withdrawer_address: Some(bob_address),
-                fee_asset: nria().into(),
-            }
-            .into(),
-        ])
-        .nonce(2)
-        .chain_id("test")
-        .try_build()
-        .unwrap_err();
-    assert!(
-        matches!(error.kind(), action_group::ErrorKind::NotBundleable { .. }),
-        "expected ErrorKind::NotBundleable, got {:?}",
-        error.kind()
-    );
-}