From 613993fd16b71feab8706e8ad3001179020b0f9d Mon Sep 17 00:00:00 2001 From: lilyjjo Date: Mon, 30 Sep 2024 10:42:19 -0400 Subject: [PATCH] respond to comment by @SuperFluffy --- .../src/executor/bundle_factory/mod.rs | 6 - .../src/executor/bundle_factory/tests.rs | 14 +- .../protocol/transaction/v1alpha1/action.rs | 1 + .../transaction/v1alpha1/action_group/mod.rs | 9 +- .../src/protocol/transaction/v1alpha1/mod.rs | 2 +- .../src/app/tests_breaking_changes.rs | 180 ------------------ 6 files changed, 11 insertions(+), 201 deletions(-) diff --git a/crates/astria-composer/src/executor/bundle_factory/mod.rs b/crates/astria-composer/src/executor/bundle_factory/mod.rs index dbac5f48c9..de3e9ee5f2 100644 --- a/crates/astria-composer/src/executor/bundle_factory/mod.rs +++ b/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 { - 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 7930a13bc1..91b65a7fd7 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); @@ -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 diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index ef0c95c19d..5ef8e94934 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> { 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 87b39a67ab..1499b0409d 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 @@ -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}" diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs index 7c94d516ea..82832fe09d 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs +++ b/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), } diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 35e72c7d88..c6a8653d83 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/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() - ); -}