From ac68b011cd021f6ac5cda7a6be59d3b6f076a987 Mon Sep 17 00:00:00 2001
From: Lily Johnson <35852084+Lilyjjo@users.noreply.github.com>
Date: Wed, 2 Oct 2024 16:41:20 -0400
Subject: [PATCH] feat(sequencer)!: make empty transactions invalid  (#1609)

## Summary
This PR makes empty transactions (i.e. no actions) invalid.

## Background
Transactions with no actions cost nothing and could be used to clog the
mempool and blocks. In the future if we want a dummy transaction to aid
in removing actions via nonce replacement, we could do a noop action
like a zero balance transfer.

## Changes
Made empty transactions invalid during the `UnsignedTransaction`
construction process.

## Testing
Unit test was added.


## Breaking Changelist
Empty transactions are no longer valid.

## Related Issues
Link any issues that are related, prefer full github links.

closes #1607
---
 .../transaction/v1alpha1/action_group/mod.rs  | 16 ++++++++++----
 .../v1alpha1/action_group/tests.rs            | 21 +++++++++++++------
 .../src/protocol/transaction/v1alpha1/mod.rs  |  6 +-----
 3 files changed, 28 insertions(+), 15 deletions(-)

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
index e39971d956..fe526a1452 100644
--- a/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs
+++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/mod.rs
@@ -134,6 +134,10 @@ impl Error {
             group,
         })
     }
+
+    fn empty() -> Self {
+        Self(ErrorKind::Empty)
+    }
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -149,10 +153,13 @@ enum ErrorKind {
     },
     #[error("attempted to create bundle with non bundleable `ActionGroup` type: {group}")]
     NotBundleable { group: ActionGroup },
+    #[error("actions cannot be empty")]
+    Empty,
 }
 
-#[derive(Clone, Debug, Default)]
+#[derive(Clone, Debug)]
 pub(super) struct Actions {
+    group: ActionGroup,
     inner: Vec<Action>,
 }
 
@@ -166,8 +173,8 @@ impl Actions {
         self.inner
     }
 
-    pub(super) fn group(&self) -> Option<ActionGroup> {
-        self.inner.first().map(super::action::Action::group)
+    pub(super) fn group(&self) -> ActionGroup {
+        self.group
     }
 
     pub(super) fn try_from_list_of_actions(actions: Vec<Action>) -> Result<Self, Error> {
@@ -176,7 +183,7 @@ impl Actions {
             Some(action) => action.group(),
             None => {
                 // empty `actions`
-                return Ok(Self::default());
+                return Err(Error::empty());
             }
         };
 
@@ -193,6 +200,7 @@ impl Actions {
         }
 
         Ok(Self {
+            group,
             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
index 6aa2afb353..71169c6a67 100644
--- a/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/tests.rs
+++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action_group/tests.rs
@@ -92,7 +92,7 @@ fn try_from_list_of_actions_bundleable_general() {
 
     assert!(matches!(
         Actions::try_from_list_of_actions(actions).unwrap().group(),
-        Some(ActionGroup::BundleableGeneral)
+        ActionGroup::BundleableGeneral
     ));
 }
 
@@ -116,7 +116,7 @@ fn from_list_of_actions_bundleable_sudo() {
 
     assert!(matches!(
         Actions::try_from_list_of_actions(actions).unwrap().group(),
-        Some(ActionGroup::BundleableSudo)
+        ActionGroup::BundleableSudo
     ));
 }
 
@@ -134,7 +134,7 @@ fn from_list_of_actions_unbundleable_sudo() {
 
     assert!(matches!(
         Actions::try_from_list_of_actions(actions).unwrap().group(),
-        Some(ActionGroup::UnbundleableSudo)
+        ActionGroup::UnbundleableSudo
     ));
 
     let actions = vec![Action::IbcSudoChange(IbcSudoChangeAction {
@@ -143,7 +143,7 @@ fn from_list_of_actions_unbundleable_sudo() {
 
     assert!(matches!(
         Actions::try_from_list_of_actions(actions).unwrap().group(),
-        Some(ActionGroup::UnbundleableSudo)
+        ActionGroup::UnbundleableSudo
     ));
 
     let actions = vec![
@@ -191,14 +191,14 @@ fn from_list_of_actions_unbundleable_general() {
 
     assert!(matches!(
         Actions::try_from_list_of_actions(actions).unwrap().group(),
-        Some(ActionGroup::UnbundleableGeneral)
+        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)
+        ActionGroup::UnbundleableGeneral
     ));
 
     let actions = vec![
@@ -239,3 +239,12 @@ fn from_list_of_actions_mixed() {
         "expected ErrorKind::Mixed, got {error_kind:?}"
     );
 }
+
+#[test]
+fn from_list_of_actions_empty() {
+    let error_kind = Actions::try_from_list_of_actions(vec![]).unwrap_err().0;
+    assert!(
+        matches!(error_kind, ErrorKind::Empty { .. }),
+        "expected ErrorKind::Empty, 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 c47c6ac684..457c357c7b 100644
--- a/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs
+++ b/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs
@@ -180,11 +180,7 @@ impl SignedTransaction {
 
     #[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
-        }
+        self.transaction.actions.group().is_bundleable_sudo()
     }
 
     #[must_use]