Skip to content

Commit

Permalink
feat(sequencer)!: make empty transactions invalid (#1609)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
Lilyjjo authored Oct 2, 2024
1 parent 6fdf4ea commit ac68b01
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,10 @@ impl Error {
group,
})
}

fn empty() -> Self {
Self(ErrorKind::Empty)
}
}

#[derive(Debug, thiserror::Error)]
Expand All @@ -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>,
}

Expand All @@ -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> {
Expand All @@ -176,7 +183,7 @@ impl Actions {
Some(action) => action.group(),
None => {
// empty `actions`
return Ok(Self::default());
return Err(Error::empty());
}
};

Expand All @@ -193,6 +200,7 @@ impl Actions {
}

Ok(Self {
group,
inner: actions,
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
));
}

Expand All @@ -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
));
}

Expand All @@ -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 {
Expand All @@ -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![
Expand Down Expand Up @@ -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![
Expand Down Expand Up @@ -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:?}"
);
}
6 changes: 1 addition & 5 deletions crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down

0 comments on commit ac68b01

Please sign in to comment.