From dbd3aec50e233653c7c10123985ead8af68e5cff Mon Sep 17 00:00:00 2001 From: Lily Johnson <35852084+Lilyjjo@users.noreply.github.com> Date: Tue, 14 May 2024 09:07:55 -0400 Subject: [PATCH] feat(sequencer)!: implement bridge unlock action and derestrict transfers (#1034) ## Summary Implements bridge accounts' `BridgeUnlock` action and allows bridge accounts to transfer assets like normal. ## Background We need a `BridgeUnlock` action to help with tracking/proving that withdraws for rollup specific events happened. This PR implements the ground work for that by adding a new action with a `memo:bytes` field that is currently unused. We also decided that it's unnecessary to have restrictions on the bridge account in terms of normal transfers in/out of the account. The bridge account is already a privileged actor and having the restrictions on transfer would make normal account maintenance (like gas funding) messy. ## Changes - implement `BridgeUnlockAction` for transferring out a bridge's bridged asset - derestricted the transfer function to let bridge account also act like normal accounts ## Testing unit tests ## Breaking Changelist - one new action is added: `BridgeUnlock` - bridge account now can transfer assets in/out like normal sequencer accounts ## Related Issues closes #983 --- .../astria.protocol.transactions.v1alpha1.rs | 35 ++- .../protocol/transaction/v1alpha1/action.rs | 115 +++++++++ .../astria-sequencer/src/accounts/action.rs | 12 - ...ransaction_with_every_action_snapshot.snap | 61 ++--- crates/astria-sequencer/src/app/test_utils.rs | 11 + .../src/app/tests_breaking_changes.rs | 31 ++- .../src/app/tests_execute_transaction.rs | 115 ++++++--- .../src/bridge/bridge_lock_action.rs | 6 +- .../src/bridge/bridge_unlock_action.rs | 219 ++++++++++++++++++ crates/astria-sequencer/src/bridge/mod.rs | 1 + .../astria-sequencer/src/bridge/state_ext.rs | 14 +- .../src/ibc/ics20_transfer.rs | 2 +- .../astria-sequencer/src/transaction/mod.rs | 27 +++ .../transactions/v1alpha1/types.proto | 19 +- 14 files changed, 574 insertions(+), 94 deletions(-) create mode 100644 crates/astria-sequencer/src/bridge/bridge_unlock_action.rs diff --git a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs index 658c0f5de..cc833ad12 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs @@ -57,7 +57,10 @@ impl ::prost::Name for TransactionParams { #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct Action { - #[prost(oneof = "action::Value", tags = "1, 2, 11, 12, 21, 22, 50, 51, 52, 53, 54")] + #[prost( + oneof = "action::Value", + tags = "1, 2, 11, 12, 13, 21, 22, 50, 51, 52, 53, 54" + )] pub value: ::core::option::Option, } /// Nested message and enum types in `Action`. @@ -75,6 +78,8 @@ pub mod action { InitBridgeAccountAction(super::InitBridgeAccountAction), #[prost(message, tag = "12")] BridgeLockAction(super::BridgeLockAction), + #[prost(message, tag = "13")] + BridgeUnlockAction(super::BridgeUnlockAction), /// IBC user actions are defined on 21-30 #[prost(message, tag = "21")] IbcAction(::penumbra_proto::core::component::ibc::v1::IbcRelay), @@ -353,3 +358,31 @@ impl ::prost::Name for BridgeLockAction { ::prost::alloc::format!("astria.protocol.transactions.v1alpha1.{}", Self::NAME) } } +/// `BridgeUnlockAction` represents a transaction that transfers +/// funds from a bridge account to a sequencer account. +/// +/// It's the same as a `TransferAction` but without the `asset_id` field +/// and with the `memo` field. +#[allow(clippy::derive_partial_eq_without_eq)] +#[derive(Clone, PartialEq, ::prost::Message)] +pub struct BridgeUnlockAction { + /// the to withdraw funds to + #[prost(message, optional, tag = "1")] + pub to: ::core::option::Option, + /// the amount to transfer + #[prost(message, optional, tag = "2")] + pub amount: ::core::option::Option, + /// the asset used to pay the transaction fee + #[prost(bytes = "vec", tag = "3")] + pub fee_asset_id: ::prost::alloc::vec::Vec, + /// memo for double spend prevention + #[prost(bytes = "vec", tag = "4")] + pub memo: ::prost::alloc::vec::Vec, +} +impl ::prost::Name for BridgeUnlockAction { + const NAME: &'static str = "BridgeUnlockAction"; + const PACKAGE: &'static str = "astria.protocol.transactions.v1alpha1"; + fn full_name() -> ::prost::alloc::string::String { + ::prost::alloc::format!("astria.protocol.transactions.v1alpha1.{}", Self::NAME) + } +} diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index dd6d350a6..a9072b62f 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -36,6 +36,7 @@ pub enum Action { FeeAssetChange(FeeAssetChangeAction), InitBridgeAccount(InitBridgeAccountAction), BridgeLock(BridgeLockAction), + BridgeUnlock(BridgeUnlockAction), } impl Action { @@ -54,6 +55,7 @@ impl Action { Action::FeeAssetChange(act) => Value::FeeAssetChangeAction(act.into_raw()), Action::InitBridgeAccount(act) => Value::InitBridgeAccountAction(act.into_raw()), Action::BridgeLock(act) => Value::BridgeLockAction(act.into_raw()), + Action::BridgeUnlock(act) => Value::BridgeUnlockAction(act.into_raw()), }; raw::Action { value: Some(kind), @@ -77,6 +79,7 @@ impl Action { Action::FeeAssetChange(act) => Value::FeeAssetChangeAction(act.to_raw()), Action::InitBridgeAccount(act) => Value::InitBridgeAccountAction(act.to_raw()), Action::BridgeLock(act) => Value::BridgeLockAction(act.to_raw()), + Action::BridgeUnlock(act) => Value::BridgeUnlockAction(act.to_raw()), }; raw::Action { value: Some(kind), @@ -134,6 +137,9 @@ impl Action { Value::BridgeLockAction(act) => Self::BridgeLock( BridgeLockAction::try_from_raw(act).map_err(ActionError::bridge_lock)?, ), + Value::BridgeUnlockAction(act) => Self::BridgeUnlock( + BridgeUnlockAction::try_from_raw(act).map_err(ActionError::bridge_unlock)?, + ), }; Ok(action) } @@ -215,6 +221,12 @@ impl From for Action { } } +impl From for Action { + fn from(value: BridgeUnlockAction) -> Self { + Self::BridgeUnlock(value) + } +} + #[allow(clippy::module_name_repetitions)] #[derive(Debug, thiserror::Error)] #[error(transparent)] @@ -268,6 +280,10 @@ impl ActionError { fn bridge_lock(inner: BridgeLockActionError) -> Self { Self(ActionErrorKind::BridgeLock(inner)) } + + fn bridge_unlock(inner: BridgeUnlockActionError) -> Self { + Self(ActionErrorKind::BridgeUnlock(inner)) + } } #[derive(Debug, thiserror::Error)] @@ -296,6 +312,8 @@ enum ActionErrorKind { InitBridgeAccount(#[source] InitBridgeAccountActionError), #[error("bridge lock action was not valid")] BridgeLock(#[source] BridgeLockActionError), + #[error("bridge unlock action was not valid")] + BridgeUnlock(#[source] BridgeUnlockActionError), } #[derive(Debug, thiserror::Error)] @@ -1269,3 +1287,100 @@ enum BridgeLockActionErrorKind { #[error("the `fee_asset_id` field was invalid")] InvalidFeeAssetId(#[source] asset::IncorrectAssetIdLength), } + +#[allow(clippy::module_name_repetitions)] +#[derive(Debug, Clone)] +pub struct BridgeUnlockAction { + pub to: Address, + pub amount: u128, + // asset to use for fee payment. + pub fee_asset_id: asset::Id, + // memo for double spend protection. + pub memo: Vec, +} + +impl BridgeUnlockAction { + #[must_use] + pub fn into_raw(self) -> raw::BridgeUnlockAction { + raw::BridgeUnlockAction { + to: Some(self.to.to_raw()), + amount: Some(self.amount.into()), + fee_asset_id: self.fee_asset_id.as_ref().to_vec(), + memo: self.memo, + } + } + + #[must_use] + pub fn to_raw(&self) -> raw::BridgeUnlockAction { + raw::BridgeUnlockAction { + to: Some(self.to.to_raw()), + amount: Some(self.amount.into()), + fee_asset_id: self.fee_asset_id.as_ref().to_vec(), + memo: self.memo.clone(), + } + } + + /// Convert from a raw, unchecked protobuf [`raw::BridgeUnlockAction`]. + /// + /// # Errors + /// + /// - if the `to` field is not set + /// - if the `to` field is invalid + /// - if the `amount` field is invalid + /// - if the `fee_asset_id` field is invalid + pub fn try_from_raw(proto: raw::BridgeUnlockAction) -> Result { + let Some(to) = proto.to else { + return Err(BridgeUnlockActionError::field_not_set("to")); + }; + let to = Address::try_from_raw(&to).map_err(BridgeUnlockActionError::invalid_address)?; + let amount = proto + .amount + .ok_or(BridgeUnlockActionError::missing_amount())?; + let fee_asset_id = asset::Id::try_from_slice(&proto.fee_asset_id) + .map_err(BridgeUnlockActionError::invalid_fee_asset_id)?; + Ok(Self { + to, + amount: amount.into(), + fee_asset_id, + memo: proto.memo, + }) + } +} + +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub struct BridgeUnlockActionError(BridgeUnlockActionErrorKind); + +impl BridgeUnlockActionError { + #[must_use] + fn field_not_set(field: &'static str) -> Self { + Self(BridgeUnlockActionErrorKind::FieldNotSet(field)) + } + + #[must_use] + fn invalid_address(err: IncorrectAddressLength) -> Self { + Self(BridgeUnlockActionErrorKind::InvalidAddress(err)) + } + + #[must_use] + fn missing_amount() -> Self { + Self(BridgeUnlockActionErrorKind::MissingAmount) + } + + #[must_use] + fn invalid_fee_asset_id(err: asset::IncorrectAssetIdLength) -> Self { + Self(BridgeUnlockActionErrorKind::InvalidFeeAssetId(err)) + } +} + +#[derive(Debug, thiserror::Error)] +enum BridgeUnlockActionErrorKind { + #[error("the expected field in the raw source type was not set: `{0}`")] + FieldNotSet(&'static str), + #[error("the `to` field was invalid")] + InvalidAddress(#[source] IncorrectAddressLength), + #[error("the `amount` field was not set")] + MissingAmount, + #[error("the `fee_asset_id` field was invalid")] + InvalidFeeAssetId(#[source] asset::IncorrectAssetIdLength), +} diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index f06ed1d17..1546add77 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -14,7 +14,6 @@ use crate::{ StateReadExt, StateWriteExt, }, - bridge::state_ext::StateReadExt as _, state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -86,17 +85,6 @@ impl ActionHandler for TransferAction { state: &S, from: Address, ) -> Result<()> { - // ensure the recipient is not a bridge account, - // as the explicit `BridgeLockAction` should be used to transfer to a bridge account. - ensure!( - state - .get_bridge_account_rollup_id(&self.to) - .await - .context("failed to get bridge account rollup ID from state")? - .is_none(), - "cannot send transfer to bridge account", - ); - transfer_check_stateful(self, state, from) .await .context("stateful transfer check failed") diff --git a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap index ad733fcd4..e06db4b71 100644 --- a/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap +++ b/crates/astria-sequencer/src/app/snapshots/astria_sequencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap @@ -1,38 +1,39 @@ --- source: crates/astria-sequencer/src/app/tests_breaking_changes.rs +assertion_line: 274 expression: app.app_hash.as_bytes() --- [ - 191, - 74, - 54, + 30, + 232, + 210, + 6, + 194, + 12, + 154, 179, - 5, - 152, - 196, - 250, - 125, - 255, - 173, - 93, - 58, - 245, - 0, - 43, - 100, - 173, - 142, - 245, - 56, + 145, + 105, + 40, + 101, + 30, + 224, + 10, + 195, + 119, + 124, + 207, + 182, + 132, + 175, + 9, 191, - 70, - 2, - 181, - 214, - 161, - 226, - 13, - 171, - 20, - 255 + 104, + 213, + 200, + 146, + 180, + 192, + 229, + 97 ] diff --git a/crates/astria-sequencer/src/app/test_utils.rs b/crates/astria-sequencer/src/app/test_utils.rs index 347c12370..22d2dba2d 100644 --- a/crates/astria-sequencer/src/app/test_utils.rs +++ b/crates/astria-sequencer/src/app/test_utils.rs @@ -49,6 +49,17 @@ pub(crate) fn get_alice_signing_key_and_address() -> (SigningKey, Address) { (alice_signing_key, alice) } +pub(crate) fn get_bridge_signing_key_and_address() -> (SigningKey, Address) { + let bridge_secret_bytes: [u8; 32] = + hex::decode("db4982e01f3eba9e74ac35422fcd49aa2b47c3c535345c7e7da5220fe3a0ce79") + .unwrap() + .try_into() + .unwrap(); + let bridge_signing_key = SigningKey::from(bridge_secret_bytes); + let bridge = Address::from_verification_key(bridge_signing_key.verification_key()); + (bridge_signing_key, bridge) +} + pub(crate) fn default_genesis_accounts() -> Vec { vec![ Account { diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 78463bcae..46434ce4d 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -19,6 +19,7 @@ use astria_core::{ protocol::transaction::v1alpha1::{ action::{ BridgeLockAction, + BridgeUnlockAction, IbcRelayerChangeAction, SequenceAction, TransferAction, @@ -46,6 +47,7 @@ use crate::{ default_fees, default_genesis_accounts, get_alice_signing_key_and_address, + get_bridge_signing_key_and_address, initialize_app, initialize_app_with_storage, BOB_ADDRESS, @@ -143,6 +145,7 @@ async fn app_finalize_block_snapshot() { // // If new actions are added to the app, they must be added to this test, // and the respective PR must be marked as breaking. +#[allow(clippy::too_many_lines)] #[tokio::test] async fn app_execute_transaction_with_every_action_snapshot() { use astria_core::{ @@ -155,6 +158,7 @@ async fn app_execute_transaction_with_every_action_snapshot() { }; let (alice_signing_key, alice_address) = get_alice_signing_key_and_address(); + let (bridge_signing_key, bridge_address) = get_bridge_signing_key_and_address(); let bob_address = address_from_hex_string(BOB_ADDRESS); let carol_address = address_from_hex_string(CAROL_ADDRESS); @@ -168,7 +172,7 @@ async fn app_execute_transaction_with_every_action_snapshot() { allowed_fee_assets: vec![DEFAULT_NATIVE_ASSET_DENOM.to_owned().into()], fees: default_fees(), }; - let mut app = initialize_app(Some(genesis_state), vec![]).await; + let (mut app, storage) = initialize_app_with_storage(Some(genesis_state), vec![]).await; // setup for ValidatorUpdate action let pub_key = tendermint::public_key::PublicKey::from_raw_ed25519(&[1u8; 32]).unwrap(); @@ -178,7 +182,6 @@ async fn app_execute_transaction_with_every_action_snapshot() { }; // setup for BridgeLockAction - let bridge_address = Address::from([99; 20]); let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let asset_id = get_native_asset().id(); let mut state_tx = StateDelta::new(app.state.clone()); @@ -238,5 +241,29 @@ async fn app_execute_transaction_with_every_action_snapshot() { let signed_tx = tx.into_signed(&alice_signing_key); app.execute_transaction(signed_tx).await.unwrap(); + + // execute BridgeUnlock action + let tx = UnsignedTransaction { + params: TransactionParams { + nonce: 0, + chain_id: "test".to_string(), + }, + actions: vec![ + BridgeUnlockAction { + to: bob_address, + amount: 10, + fee_asset_id: get_native_asset().id(), + memo: vec![0u8; 32], + } + .into(), + ], + }; + + let signed_tx = tx.into_signed(&bridge_signing_key); + app.execute_transaction(signed_tx).await.unwrap(); + + app.prepare_commit(storage.clone()).await.unwrap(); + app.commit(storage.clone()).await; + insta::assert_json_snapshot!(app.app_hash.as_bytes()); } diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index 0d2da2238..7bc739fdf 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -10,6 +10,7 @@ use astria_core::{ protocol::transaction::v1alpha1::{ action::{ BridgeLockAction, + BridgeUnlockAction, IbcRelayerChangeAction, SequenceAction, SudoAddressChangeAction, @@ -635,7 +636,7 @@ async fn app_execute_transaction_init_bridge_account_ok() { ); assert_eq!( app.state - .get_bridge_account_asset_ids(&alice_address) + .get_bridge_account_asset_id(&alice_address) .await .unwrap(), asset_id @@ -805,41 +806,6 @@ async fn app_execute_transaction_bridge_lock_action_invalid_for_eoa() { assert!(app.execute_transaction(signed_tx).await.is_err()); } -#[tokio::test] -async fn app_execute_transaction_transfer_invalid_to_bridge_account() { - let (alice_signing_key, _) = get_alice_signing_key_and_address(); - let mut app = initialize_app(None, vec![]).await; - - let bridge_address = Address::from([99; 20]); - let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); - let asset_id = get_native_asset().id(); - - let mut state_tx = StateDelta::new(app.state.clone()); - state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); - state_tx - .put_bridge_account_asset_id(&bridge_address, &asset_id) - .unwrap(); - app.apply(state_tx); - - let amount = 100; - let action = TransferAction { - to: bridge_address, - amount, - asset_id, - fee_asset_id: asset_id, - }; - let tx = UnsignedTransaction { - params: TransactionParams { - nonce: 0, - chain_id: "test".to_string(), - }, - actions: vec![action.into()], - }; - - let signed_tx = tx.into_signed(&alice_signing_key); - assert!(app.execute_transaction(signed_tx).await.is_err()); -} - #[cfg(feature = "mint")] #[tokio::test] async fn app_execute_transaction_mint() { @@ -1072,3 +1038,80 @@ async fn app_stateful_check_fails_insufficient_total_balance() { .await .expect("stateful check should pass since we transferred enough to cover fee"); } + +#[tokio::test] +async fn app_execute_transaction_bridge_lock_unlock_action_ok() { + use crate::accounts::state_ext::StateWriteExt as _; + + let (alice_signing_key, alice_address) = get_alice_signing_key_and_address(); + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + + let (bridge_signing_key, bridge_address) = get_bridge_signing_key_and_address(); + let rollup_id: RollupId = RollupId::from_unhashed_bytes(b"testchainid"); + let asset_id = get_native_asset().id(); + + // give bridge eoa funds so it can pay for the + // unlock transfer action + let transfer_fee = app.state.get_transfer_base_fee().await.unwrap(); + state_tx + .put_account_balance(bridge_address, asset_id, transfer_fee) + .unwrap(); + + // create bridge account + state_tx.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state_tx + .put_bridge_account_asset_id(&bridge_address, &asset_id) + .unwrap(); + app.apply(state_tx); + + let amount = 100; + let action = BridgeLockAction { + to: bridge_address, + amount, + asset_id, + fee_asset_id: asset_id, + destination_chain_address: "nootwashere".to_string(), + }; + let tx = UnsignedTransaction { + params: TransactionParams { + nonce: 0, + chain_id: "test".to_string(), + }, + actions: vec![action.into()], + }; + + let signed_tx = tx.into_signed(&alice_signing_key); + + app.execute_transaction(signed_tx).await.unwrap(); + assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); + + // see can unlock through bridge unlock + let action = BridgeUnlockAction { + to: alice_address, + amount, + fee_asset_id: asset_id, + memo: b"lilywashere".to_vec(), + }; + + let tx = UnsignedTransaction { + params: TransactionParams { + nonce: 0, + chain_id: "test".to_string(), + }, + actions: vec![action.into()], + }; + + let signed_tx = tx.into_signed(&bridge_signing_key); + app.execute_transaction(signed_tx) + .await + .expect("executing bridge unlock action should succeed"); + assert_eq!( + app.state + .get_account_balance(bridge_address, asset_id) + .await + .expect("executing bridge unlock action should succeed"), + 0, + "bridge should've transferred out whole balance" + ); +} diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 4be9cb3e5..dd356054b 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -54,7 +54,7 @@ impl ActionHandler for BridgeLockAction { .ok_or_else(|| anyhow::anyhow!("bridge lock must be sent to a bridge account"))?; let allowed_asset_id = state - .get_bridge_account_asset_ids(&self.to) + .get_bridge_account_asset_id(&self.to) .await .context("failed to get bridge account asset ID")?; ensure!( @@ -88,9 +88,7 @@ impl ActionHandler for BridgeLockAction { .saturating_add(transfer_fee); ensure!(from_balance >= fee, "insufficient funds for fee payment"); - // this performs the same checks as a normal `TransferAction`, - // but without the check that prevents transferring to a bridge account, - // as we are explicitly transferring to a bridge account here. + // this performs the same checks as a normal `TransferAction` transfer_check_stateful(&transfer_action, state, from).await } diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs new file mode 100644 index 000000000..ba6abcdfa --- /dev/null +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -0,0 +1,219 @@ +use anyhow::{ + Context as _, + Result, +}; +use astria_core::{ + primitive::v1::Address, + protocol::transaction::v1alpha1::action::{ + BridgeUnlockAction, + TransferAction, + }, +}; +use tracing::instrument; + +use crate::{ + accounts::action::transfer_check_stateful, + bridge::state_ext::StateReadExt as _, + state_ext::{ + StateReadExt, + StateWriteExt, + }, + transaction::action_handler::ActionHandler, +}; + +#[async_trait::async_trait] +impl ActionHandler for BridgeUnlockAction { + async fn check_stateful( + &self, + state: &S, + from: Address, + ) -> Result<()> { + // grab the bridge account's asset + let asset_id = state + .get_bridge_account_asset_id(&from) + .await + .context("failed to get bridge's asset id, must be a bridge account")?; + + let transfer_action = TransferAction { + to: self.to, + asset_id, + amount: self.amount, + fee_asset_id: self.fee_asset_id, + }; + + // TODO use the BridgeUnlock action's `memo` field. + + // this performs the same checks as a normal `TransferAction` + transfer_check_stateful(&transfer_action, state, from).await + } + + #[instrument(skip_all)] + async fn execute(&self, state: &mut S, from: Address) -> Result<()> { + let asset_id = state + .get_bridge_account_asset_id(&from) + .await + .context("failed to get bridge's asset id, must be a bridge account")?; + + let transfer_action = TransferAction { + to: self.to, + asset_id, + amount: self.amount, + fee_asset_id: self.fee_asset_id, + }; + + transfer_action + .execute(state, from) + .await + .context("failed to execute bridge unlock action as transfer action")?; + + Ok(()) + } +} + +#[cfg(test)] +mod test { + use astria_core::primitive::v1::{ + asset, + RollupId, + }; + use cnidarium::StateDelta; + + use super::*; + use crate::{ + accounts::state_ext::StateWriteExt as _, + bridge::state_ext::StateWriteExt, + state_ext::StateWriteExt as _, + }; + + #[tokio::test] + async fn bridge_unlock_fail_non_bridge_accounts() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let state = StateDelta::new(snapshot); + + let asset_id = asset::Id::from_denom("test"); + let transfer_amount = 100; + + let address = Address::from([1; 20]); + let to_address = Address::from([2; 20]); + + let bridge_unlock = BridgeUnlockAction { + to: to_address, + amount: transfer_amount, + fee_asset_id: asset_id, + memo: vec![0u8; 32], + }; + + // not a bridge account, should fail + assert!( + bridge_unlock + .check_stateful(&state, address) + .await + .unwrap_err() + .to_string() + .contains("failed to get bridge's asset id, must be a bridge account") + ); + } + + #[tokio::test] + async fn bridge_unlock_fee_check_stateful() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let asset_id = asset::Id::from_denom("test"); + let transfer_fee = 10; + let transfer_amount = 100; + state.put_transfer_base_fee(transfer_fee).unwrap(); + + let bridge_address = Address::from([1; 20]); + let to_address = Address::from([2; 20]); + let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); + + state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state + .put_bridge_account_asset_id(&bridge_address, &asset_id) + .unwrap(); + state.put_allowed_fee_asset(asset_id); + + let bridge_unlock = BridgeUnlockAction { + to: to_address, + amount: transfer_amount, + fee_asset_id: asset_id, + memo: vec![0u8; 32], + }; + + // not enough balance to transfer asset; should fail + state + .put_account_balance(bridge_address, asset_id, transfer_amount) + .unwrap(); + assert!( + bridge_unlock + .check_stateful(&state, bridge_address) + .await + .unwrap_err() + .to_string() + .contains("insufficient funds for transfer and fee payment") + ); + + // enough balance; should pass + state + .put_account_balance(bridge_address, asset_id, transfer_amount + transfer_fee) + .unwrap(); + bridge_unlock + .check_stateful(&state, bridge_address) + .await + .unwrap(); + } + + #[tokio::test] + async fn bridge_unlock_execute() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let asset_id = asset::Id::from_denom("test"); + let transfer_fee = 10; + let transfer_amount = 100; + state.put_transfer_base_fee(transfer_fee).unwrap(); + + let bridge_address = Address::from([1; 20]); + let to_address = Address::from([2; 20]); + let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); + + state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state + .put_bridge_account_asset_id(&bridge_address, &asset_id) + .unwrap(); + state.put_allowed_fee_asset(asset_id); + + let bridge_unlock = BridgeUnlockAction { + to: to_address, + amount: transfer_amount, + fee_asset_id: asset_id, + memo: vec![0u8; 32], + }; + + // not enough balance; should fail + state + .put_account_balance(bridge_address, asset_id, transfer_amount) + .unwrap(); + assert!( + bridge_unlock + .execute(&mut state, bridge_address) + .await + .unwrap_err() + .to_string() + .eq("failed to execute bridge unlock action as transfer action") + ); + + // enough balance; should pass + state + .put_account_balance(bridge_address, asset_id, transfer_amount + transfer_fee) + .unwrap(); + bridge_unlock + .execute(&mut state, bridge_address) + .await + .unwrap(); + } +} diff --git a/crates/astria-sequencer/src/bridge/mod.rs b/crates/astria-sequencer/src/bridge/mod.rs index 09ff1b295..b808bd84f 100644 --- a/crates/astria-sequencer/src/bridge/mod.rs +++ b/crates/astria-sequencer/src/bridge/mod.rs @@ -1,4 +1,5 @@ mod bridge_lock_action; +mod bridge_unlock_action; pub(crate) mod component; pub(crate) mod init_bridge_account_action; pub(crate) mod state_ext; diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index 903a7109d..16010fe9a 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -104,7 +104,7 @@ pub(crate) trait StateReadExt: StateRead { } #[instrument(skip(self))] - async fn get_bridge_account_asset_ids(&self, address: &Address) -> Result { + async fn get_bridge_account_asset_id(&self, address: &Address) -> Result { let bytes = self .get_raw(&asset_id_storage_key(address)) .await @@ -388,14 +388,14 @@ mod test { } #[tokio::test] - async fn get_bridge_account_asset_ids_none_should_fail() { + async fn get_bridge_account_asset_id_none_should_fail() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let state = StateDelta::new(snapshot); let address = Address::try_from_slice(&[42u8; 20]).unwrap(); state - .get_bridge_account_asset_ids(&address) + .get_bridge_account_asset_id(&address) .await .expect_err("call to get bridge account asset ids should fail if no assets"); } @@ -414,7 +414,7 @@ mod test { .put_bridge_account_asset_id(&address, &asset) .expect("storing bridge account asset should not fail"); let mut result = state - .get_bridge_account_asset_ids(&address) + .get_bridge_account_asset_id(&address) .await .expect("bridge asset id was written and must exist inside the database"); assert_eq!( @@ -428,7 +428,7 @@ mod test { .put_bridge_account_asset_id(&address, &asset) .expect("storing bridge account assets should not fail"); result = state - .get_bridge_account_asset_ids(&address) + .get_bridge_account_asset_id(&address) .await .expect("bridge asset id was written and must exist inside the database"); assert_eq!( @@ -444,14 +444,14 @@ mod test { .expect("storing bridge account assets should not fail"); assert_eq!( state - .get_bridge_account_asset_ids(&address_1) + .get_bridge_account_asset_id(&address_1) .await .expect("bridge asset id was written and must exist inside the database"), asset_1, "second bridge account asset not what was expected" ); result = state - .get_bridge_account_asset_ids(&address) + .get_bridge_account_asset_id(&address) .await .expect("original bridge asset id was written and must exist inside the database"); assert_eq!( diff --git a/crates/astria-sequencer/src/ibc/ics20_transfer.rs b/crates/astria-sequencer/src/ibc/ics20_transfer.rs index d84399189..5168a62f2 100644 --- a/crates/astria-sequencer/src/ibc/ics20_transfer.rs +++ b/crates/astria-sequencer/src/ibc/ics20_transfer.rs @@ -356,7 +356,7 @@ async fn execute_ics20_transfer_bridge_lock( ); let allowed_asset_id = state - .get_bridge_account_asset_ids(recipient) + .get_bridge_account_asset_id(recipient) .await .context("failed to get bridge account asset ID")?; ensure!( diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index 5c2898e09..c81034a9a 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -142,6 +142,20 @@ pub(crate) async fn check_balance_for_total_fees( .and_modify(|amt| *amt = amt.saturating_add(transfer_fee)) .or_insert(transfer_fee); } + Action::BridgeUnlock(act) => { + let asset_id = state + .get_bridge_account_asset_id(&from) + .await + .context("must be a bridge account for BridgeUnlock action")?; + fees_by_asset + .entry(asset_id) + .and_modify(|amt: &mut u128| *amt = amt.saturating_add(act.amount)) + .or_insert(act.amount); + fees_by_asset + .entry(act.fee_asset_id) + .and_modify(|amt| *amt = amt.saturating_add(transfer_fee)) + .or_insert(transfer_fee); + } Action::ValidatorUpdate(_) | Action::SudoAddressChange(_) | Action::Ibc(_) @@ -276,6 +290,10 @@ impl ActionHandler for UnsignedTransaction { .check_stateless() .await .context("stateless check failed for BridgeLockAction")?, + Action::BridgeUnlock(act) => act + .check_stateless() + .await + .context("stateless check failed for BridgeLockAction")?, #[cfg(feature = "mint")] Action::Mint(act) => act .check_stateless() @@ -358,6 +376,10 @@ impl ActionHandler for UnsignedTransaction { .check_stateful(state, from) .await .context("stateful check failed for BridgeLockAction")?, + Action::BridgeUnlock(act) => act + .check_stateful(state, from) + .await + .context("stateful check failed for BridgeUnlockAction")?, #[cfg(feature = "mint")] Action::Mint(act) => act .check_stateful(state, from) @@ -446,6 +468,11 @@ impl ActionHandler for UnsignedTransaction { .await .context("execution failed for BridgeLockAction")?; } + Action::BridgeUnlock(act) => { + act.execute(state, from) + .await + .context("execution failed for BridgeUnlockAction")?; + } #[cfg(feature = "mint")] Action::Mint(act) => { act.execute(state, from) diff --git a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto index 984d3edf8..39b19d60e 100644 --- a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto @@ -41,6 +41,7 @@ message Action { // Bridge actions are defined on 11-20 InitBridgeAccountAction init_bridge_account_action = 11; BridgeLockAction bridge_lock_action = 12; + BridgeUnlockAction bridge_unlock_action = 13; // IBC user actions are defined on 21-30 astria_vendored.penumbra.core.component.ibc.v1.IbcRelay ibc_action = 21; @@ -54,7 +55,7 @@ message Action { MintAction mint_action = 54; } reserved 3 to 10; - reserved 13 to 20; + reserved 14 to 20; reserved 23 to 30; reserved 55 to 60; } @@ -184,3 +185,19 @@ message BridgeLockAction { // will receive the bridged funds string destination_chain_address = 5; } + +// `BridgeUnlockAction` represents a transaction that transfers +// funds from a bridge account to a sequencer account. +// +// It's the same as a `TransferAction` but without the `asset_id` field +// and with the `memo` field. +message BridgeUnlockAction { + // the to withdraw funds to + astria.primitive.v1.Address to = 1; + // the amount to transfer + astria.primitive.v1.Uint128 amount = 2; + // the asset used to pay the transaction fee + bytes fee_asset_id = 3; + // memo for double spend prevention + bytes memo = 4; +}