diff --git a/crates/astria-bridge-contracts/src/lib.rs b/crates/astria-bridge-contracts/src/lib.rs index 49b4b0161d..11195a2dae 100644 --- a/crates/astria-bridge-contracts/src/lib.rs +++ b/crates/astria-bridge-contracts/src/lib.rs @@ -378,7 +378,7 @@ where .ok_or_else(|| GetWithdrawalActionsError::log_without_block_number(&log))? .as_u64(); - let rollup_transaction_hash = log + let rollup_withdrawal_event_id = log .transaction_hash .ok_or_else(|| GetWithdrawalActionsError::log_without_transaction_hash(&log))? .to_string(); @@ -400,7 +400,7 @@ where memo: event.memo.clone(), rollup_block_number, rollup_return_address: event.sender.to_string(), - rollup_transaction_hash, + rollup_withdrawal_event_id, }) .map_err(GetWithdrawalActionsError::encode_memo)?; @@ -433,7 +433,7 @@ where .ok_or_else(|| GetWithdrawalActionsError::log_without_block_number(&log))? .as_u64(); - let rollup_transaction_hash = log + let rollup_withdrawal_event_id = log .transaction_hash .ok_or_else(|| GetWithdrawalActionsError::log_without_transaction_hash(&log))? .to_string(); @@ -441,12 +441,6 @@ where let event = decode_log::(log) .map_err(GetWithdrawalActionsError::decode_log)?; - let memo = memo_to_json(&memos::v1alpha1::BridgeUnlock { - rollup_block_number, - rollup_transaction_hash, - }) - .map_err(GetWithdrawalActionsError::encode_memo)?; - let amount = calculate_amount(&event, self.asset_withdrawal_divisor) .map_err(GetWithdrawalActionsError::calculate_withdrawal_amount)?; @@ -456,7 +450,9 @@ where let action = astria_core::protocol::transaction::v1alpha1::action::BridgeUnlockAction { to, amount, - memo, + rollup_block_number, + rollup_withdrawal_event_id, + memo: String::new(), fee_asset: self.fee_asset.clone(), bridge_address: self.bridge_address, }; diff --git a/crates/astria-bridge-withdrawer/src/bridge_withdrawer/startup.rs b/crates/astria-bridge-withdrawer/src/bridge_withdrawer/startup.rs index a040347ec9..cc17090dab 100644 --- a/crates/astria-bridge-withdrawer/src/bridge_withdrawer/startup.rs +++ b/crates/astria-bridge-withdrawer/src/bridge_withdrawer/startup.rs @@ -461,11 +461,7 @@ fn rollup_height_from_signed_transaction( .ok_or_eyre("last transaction by the bridge account did not contain a withdrawal action")?; let last_batch_rollup_height = match withdrawal_action { - Action::BridgeUnlock(action) => { - let memo: memos::v1alpha1::BridgeUnlock = serde_json::from_str(&action.memo) - .wrap_err("failed to parse memo from last transaction by the bridge account")?; - Some(memo.rollup_block_number) - } + Action::BridgeUnlock(action) => Some(action.rollup_block_number), Action::Ics20Withdrawal(action) => { let memo: memos::v1alpha1::Ics20WithdrawalFromRollup = serde_json::from_str(&action.memo) diff --git a/crates/astria-bridge-withdrawer/tests/blackbox/helpers/test_bridge_withdrawer.rs b/crates/astria-bridge-withdrawer/tests/blackbox/helpers/test_bridge_withdrawer.rs index 012988fef7..2405c4c04a 100644 --- a/crates/astria-bridge-withdrawer/tests/blackbox/helpers/test_bridge_withdrawer.rs +++ b/crates/astria-bridge-withdrawer/tests/blackbox/helpers/test_bridge_withdrawer.rs @@ -17,10 +17,7 @@ use astria_core::{ }, protocol::{ bridge::v1alpha1::BridgeAccountLastTxHashResponse, - memos::v1alpha1::{ - BridgeUnlock, - Ics20WithdrawalFromRollup, - }, + memos::v1alpha1::Ics20WithdrawalFromRollup, transaction::v1alpha1::{ action::{ BridgeUnlockAction, @@ -423,11 +420,9 @@ pub fn make_bridge_unlock_action(receipt: &TransactionReceipt) -> Action { let inner = BridgeUnlockAction { to: default_sequencer_address(), amount: 1_000_000u128, - memo: serde_json::to_string(&BridgeUnlock { - rollup_block_number: receipt.block_number.unwrap().as_u64(), - rollup_transaction_hash: receipt.transaction_hash.to_string(), - }) - .unwrap(), + rollup_block_number: receipt.block_number.unwrap().as_u64(), + rollup_withdrawal_event_id: receipt.transaction_hash.to_string(), + memo: String::new(), fee_asset: denom, bridge_address: default_bridge_address(), }; @@ -448,7 +443,7 @@ pub fn make_ics20_withdrawal_action(receipt: &TransactionReceipt) -> Action { memo: "nootwashere".to_string(), rollup_return_address: receipt.from.to_string(), rollup_block_number: receipt.block_number.unwrap().as_u64(), - rollup_transaction_hash: receipt.transaction_hash.to_string(), + rollup_withdrawal_event_id: receipt.transaction_hash.to_string(), }) .unwrap(), fee_asset: denom, diff --git a/crates/astria-core/src/generated/astria.protocol.memos.v1alpha1.rs b/crates/astria-core/src/generated/astria.protocol.memos.v1alpha1.rs index bd066f2abf..ead1e09197 100644 --- a/crates/astria-core/src/generated/astria.protocol.memos.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.memos.v1alpha1.rs @@ -1,25 +1,3 @@ -#[allow(clippy::derive_partial_eq_without_eq)] -#[derive(Clone, PartialEq, ::prost::Message)] -pub struct BridgeUnlock { - /// The block number on the rollup that triggered the transaction underlying - /// this bridge unlock memo. - #[prost(uint64, tag = "1")] - pub rollup_block_number: u64, - /// The hash of the original rollup transaction that triggered a bridge unlock - /// and that is underlying this bridge unlock memo. - /// - /// This field is of type `string` so that it can be formatted in the preferred - /// format of the rollup when targeting plain text encoding. - #[prost(string, tag = "2")] - pub rollup_transaction_hash: ::prost::alloc::string::String, -} -impl ::prost::Name for BridgeUnlock { - const NAME: &'static str = "BridgeUnlock"; - const PACKAGE: &'static str = "astria.protocol.memos.v1alpha1"; - fn full_name() -> ::prost::alloc::string::String { - ::prost::alloc::format!("astria.protocol.memos.v1alpha1.{}", Self::NAME) - } -} /// Memo for an ICS20 withdrawal from the rollup which is sent to /// an external IBC-enabled chain. #[allow(clippy::derive_partial_eq_without_eq)] @@ -29,13 +7,14 @@ pub struct Ics20WithdrawalFromRollup { /// this ics20 withdrawal memo. #[prost(uint64, tag = "1")] pub rollup_block_number: u64, - /// The hash of the original rollup transaction that triggered this ics20 - /// withdrawal and that is underlying this bridge unlock memo. + /// An identifier of the original rollup withdrawal event that triggered this ics20 + /// withdrawal and that is underlying this bridge unlock memo. For general EVM + /// this is typically a transaction hash. /// /// This field is of type `string` so that it can be formatted in the preferred /// format of the rollup when targeting plain text encoding. #[prost(string, tag = "2")] - pub rollup_transaction_hash: ::prost::alloc::string::String, + pub rollup_withdrawal_event_id: ::prost::alloc::string::String, /// The return address on the rollup to which funds should returned in case of /// failure. This field exists so that the rollup can identify which account /// the returned funds originated from. diff --git a/crates/astria-core/src/generated/astria.protocol.memos.v1alpha1.serde.rs b/crates/astria-core/src/generated/astria.protocol.memos.v1alpha1.serde.rs index 315161ec8d..a7efd8ee1b 100644 --- a/crates/astria-core/src/generated/astria.protocol.memos.v1alpha1.serde.rs +++ b/crates/astria-core/src/generated/astria.protocol.memos.v1alpha1.serde.rs @@ -1,116 +1,3 @@ -impl serde::Serialize for BridgeUnlock { - #[allow(deprecated)] - fn serialize(&self, serializer: S) -> std::result::Result - where - S: serde::Serializer, - { - use serde::ser::SerializeStruct; - let mut len = 0; - if self.rollup_block_number != 0 { - len += 1; - } - if !self.rollup_transaction_hash.is_empty() { - len += 1; - } - let mut struct_ser = serializer.serialize_struct("astria.protocol.memos.v1alpha1.BridgeUnlock", len)?; - if self.rollup_block_number != 0 { - #[allow(clippy::needless_borrow)] - struct_ser.serialize_field("rollupBlockNumber", ToString::to_string(&self.rollup_block_number).as_str())?; - } - if !self.rollup_transaction_hash.is_empty() { - struct_ser.serialize_field("rollupTransactionHash", &self.rollup_transaction_hash)?; - } - struct_ser.end() - } -} -impl<'de> serde::Deserialize<'de> for BridgeUnlock { - #[allow(deprecated)] - fn deserialize(deserializer: D) -> std::result::Result - where - D: serde::Deserializer<'de>, - { - const FIELDS: &[&str] = &[ - "rollup_block_number", - "rollupBlockNumber", - "rollup_transaction_hash", - "rollupTransactionHash", - ]; - - #[allow(clippy::enum_variant_names)] - enum GeneratedField { - RollupBlockNumber, - RollupTransactionHash, - } - impl<'de> serde::Deserialize<'de> for GeneratedField { - fn deserialize(deserializer: D) -> std::result::Result - where - D: serde::Deserializer<'de>, - { - struct GeneratedVisitor; - - impl<'de> serde::de::Visitor<'de> for GeneratedVisitor { - type Value = GeneratedField; - - fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(formatter, "expected one of: {:?}", &FIELDS) - } - - #[allow(unused_variables)] - fn visit_str(self, value: &str) -> std::result::Result - where - E: serde::de::Error, - { - match value { - "rollupBlockNumber" | "rollup_block_number" => Ok(GeneratedField::RollupBlockNumber), - "rollupTransactionHash" | "rollup_transaction_hash" => Ok(GeneratedField::RollupTransactionHash), - _ => Err(serde::de::Error::unknown_field(value, FIELDS)), - } - } - } - deserializer.deserialize_identifier(GeneratedVisitor) - } - } - struct GeneratedVisitor; - impl<'de> serde::de::Visitor<'de> for GeneratedVisitor { - type Value = BridgeUnlock; - - fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - formatter.write_str("struct astria.protocol.memos.v1alpha1.BridgeUnlock") - } - - fn visit_map(self, mut map_: V) -> std::result::Result - where - V: serde::de::MapAccess<'de>, - { - let mut rollup_block_number__ = None; - let mut rollup_transaction_hash__ = None; - while let Some(k) = map_.next_key()? { - match k { - GeneratedField::RollupBlockNumber => { - if rollup_block_number__.is_some() { - return Err(serde::de::Error::duplicate_field("rollupBlockNumber")); - } - rollup_block_number__ = - Some(map_.next_value::<::pbjson::private::NumberDeserialize<_>>()?.0) - ; - } - GeneratedField::RollupTransactionHash => { - if rollup_transaction_hash__.is_some() { - return Err(serde::de::Error::duplicate_field("rollupTransactionHash")); - } - rollup_transaction_hash__ = Some(map_.next_value()?); - } - } - } - Ok(BridgeUnlock { - rollup_block_number: rollup_block_number__.unwrap_or_default(), - rollup_transaction_hash: rollup_transaction_hash__.unwrap_or_default(), - }) - } - } - deserializer.deserialize_struct("astria.protocol.memos.v1alpha1.BridgeUnlock", FIELDS, GeneratedVisitor) - } -} impl serde::Serialize for Ics20TransferDeposit { #[allow(deprecated)] fn serialize(&self, serializer: S) -> std::result::Result @@ -214,7 +101,7 @@ impl serde::Serialize for Ics20WithdrawalFromRollup { if self.rollup_block_number != 0 { len += 1; } - if !self.rollup_transaction_hash.is_empty() { + if !self.rollup_withdrawal_event_id.is_empty() { len += 1; } if !self.rollup_return_address.is_empty() { @@ -228,8 +115,8 @@ impl serde::Serialize for Ics20WithdrawalFromRollup { #[allow(clippy::needless_borrow)] struct_ser.serialize_field("rollupBlockNumber", ToString::to_string(&self.rollup_block_number).as_str())?; } - if !self.rollup_transaction_hash.is_empty() { - struct_ser.serialize_field("rollupTransactionHash", &self.rollup_transaction_hash)?; + if !self.rollup_withdrawal_event_id.is_empty() { + struct_ser.serialize_field("rollupWithdrawalEventId", &self.rollup_withdrawal_event_id)?; } if !self.rollup_return_address.is_empty() { struct_ser.serialize_field("rollupReturnAddress", &self.rollup_return_address)?; @@ -249,8 +136,8 @@ impl<'de> serde::Deserialize<'de> for Ics20WithdrawalFromRollup { const FIELDS: &[&str] = &[ "rollup_block_number", "rollupBlockNumber", - "rollup_transaction_hash", - "rollupTransactionHash", + "rollup_withdrawal_event_id", + "rollupWithdrawalEventId", "rollup_return_address", "rollupReturnAddress", "memo", @@ -259,7 +146,7 @@ impl<'de> serde::Deserialize<'de> for Ics20WithdrawalFromRollup { #[allow(clippy::enum_variant_names)] enum GeneratedField { RollupBlockNumber, - RollupTransactionHash, + RollupWithdrawalEventId, RollupReturnAddress, Memo, } @@ -284,7 +171,7 @@ impl<'de> serde::Deserialize<'de> for Ics20WithdrawalFromRollup { { match value { "rollupBlockNumber" | "rollup_block_number" => Ok(GeneratedField::RollupBlockNumber), - "rollupTransactionHash" | "rollup_transaction_hash" => Ok(GeneratedField::RollupTransactionHash), + "rollupWithdrawalEventId" | "rollup_withdrawal_event_id" => Ok(GeneratedField::RollupWithdrawalEventId), "rollupReturnAddress" | "rollup_return_address" => Ok(GeneratedField::RollupReturnAddress), "memo" => Ok(GeneratedField::Memo), _ => Err(serde::de::Error::unknown_field(value, FIELDS)), @@ -307,7 +194,7 @@ impl<'de> serde::Deserialize<'de> for Ics20WithdrawalFromRollup { V: serde::de::MapAccess<'de>, { let mut rollup_block_number__ = None; - let mut rollup_transaction_hash__ = None; + let mut rollup_withdrawal_event_id__ = None; let mut rollup_return_address__ = None; let mut memo__ = None; while let Some(k) = map_.next_key()? { @@ -320,11 +207,11 @@ impl<'de> serde::Deserialize<'de> for Ics20WithdrawalFromRollup { Some(map_.next_value::<::pbjson::private::NumberDeserialize<_>>()?.0) ; } - GeneratedField::RollupTransactionHash => { - if rollup_transaction_hash__.is_some() { - return Err(serde::de::Error::duplicate_field("rollupTransactionHash")); + GeneratedField::RollupWithdrawalEventId => { + if rollup_withdrawal_event_id__.is_some() { + return Err(serde::de::Error::duplicate_field("rollupWithdrawalEventId")); } - rollup_transaction_hash__ = Some(map_.next_value()?); + rollup_withdrawal_event_id__ = Some(map_.next_value()?); } GeneratedField::RollupReturnAddress => { if rollup_return_address__.is_some() { @@ -342,7 +229,7 @@ impl<'de> serde::Deserialize<'de> for Ics20WithdrawalFromRollup { } Ok(Ics20WithdrawalFromRollup { rollup_block_number: rollup_block_number__.unwrap_or_default(), - rollup_transaction_hash: rollup_transaction_hash__.unwrap_or_default(), + rollup_withdrawal_event_id: rollup_withdrawal_event_id__.unwrap_or_default(), rollup_return_address: rollup_return_address__.unwrap_or_default(), memo: memo__.unwrap_or_default(), }) 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 fd8f2cf0a0..a65b8a8b28 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs @@ -386,7 +386,8 @@ pub struct BridgeUnlockAction { /// the asset used to pay the transaction fee #[prost(string, tag = "3")] pub fee_asset: ::prost::alloc::string::String, - /// memo for double spend prevention + /// The memo field can be used to provide unique identifying additional + /// information about the bridge unlock transaction. #[prost(string, tag = "4")] pub memo: ::prost::alloc::string::String, /// the address of the bridge account to transfer from @@ -394,6 +395,19 @@ pub struct BridgeUnlockAction { pub bridge_address: ::core::option::Option< super::super::super::primitive::v1::Address, >, + /// The block number on the rollup that triggered the transaction underlying + /// this bridge unlock memo. + #[prost(uint64, tag = "6")] + pub rollup_block_number: u64, + /// An identifier of the original rollup event, such as a transaction hash which + /// triggered a bridge unlock and is underlying event that led to this bridge + /// unlock. This can be utilized for tracing from the bridge back to + /// distinct rollup events. + /// + /// This field is of type `string` so that it can be formatted in the preferred + /// format of the rollup when targeting plain text encoding. + #[prost(string, tag = "7")] + pub rollup_withdrawal_event_id: ::prost::alloc::string::String, } impl ::prost::Name for BridgeUnlockAction { const NAME: &'static str = "BridgeUnlockAction"; diff --git a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.serde.rs b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.serde.rs index 9c89346b65..f6bad8ea54 100644 --- a/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.serde.rs +++ b/crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.serde.rs @@ -593,6 +593,12 @@ impl serde::Serialize for BridgeUnlockAction { if self.bridge_address.is_some() { len += 1; } + if self.rollup_block_number != 0 { + len += 1; + } + if !self.rollup_withdrawal_event_id.is_empty() { + len += 1; + } let mut struct_ser = serializer.serialize_struct("astria.protocol.transactions.v1alpha1.BridgeUnlockAction", len)?; if let Some(v) = self.to.as_ref() { struct_ser.serialize_field("to", v)?; @@ -609,6 +615,13 @@ impl serde::Serialize for BridgeUnlockAction { if let Some(v) = self.bridge_address.as_ref() { struct_ser.serialize_field("bridgeAddress", v)?; } + if self.rollup_block_number != 0 { + #[allow(clippy::needless_borrow)] + struct_ser.serialize_field("rollupBlockNumber", ToString::to_string(&self.rollup_block_number).as_str())?; + } + if !self.rollup_withdrawal_event_id.is_empty() { + struct_ser.serialize_field("rollupWithdrawalEventId", &self.rollup_withdrawal_event_id)?; + } struct_ser.end() } } @@ -626,6 +639,10 @@ impl<'de> serde::Deserialize<'de> for BridgeUnlockAction { "memo", "bridge_address", "bridgeAddress", + "rollup_block_number", + "rollupBlockNumber", + "rollup_withdrawal_event_id", + "rollupWithdrawalEventId", ]; #[allow(clippy::enum_variant_names)] @@ -635,6 +652,8 @@ impl<'de> serde::Deserialize<'de> for BridgeUnlockAction { FeeAsset, Memo, BridgeAddress, + RollupBlockNumber, + RollupWithdrawalEventId, } impl<'de> serde::Deserialize<'de> for GeneratedField { fn deserialize(deserializer: D) -> std::result::Result @@ -661,6 +680,8 @@ impl<'de> serde::Deserialize<'de> for BridgeUnlockAction { "feeAsset" | "fee_asset" => Ok(GeneratedField::FeeAsset), "memo" => Ok(GeneratedField::Memo), "bridgeAddress" | "bridge_address" => Ok(GeneratedField::BridgeAddress), + "rollupBlockNumber" | "rollup_block_number" => Ok(GeneratedField::RollupBlockNumber), + "rollupWithdrawalEventId" | "rollup_withdrawal_event_id" => Ok(GeneratedField::RollupWithdrawalEventId), _ => Err(serde::de::Error::unknown_field(value, FIELDS)), } } @@ -685,6 +706,8 @@ impl<'de> serde::Deserialize<'de> for BridgeUnlockAction { let mut fee_asset__ = None; let mut memo__ = None; let mut bridge_address__ = None; + let mut rollup_block_number__ = None; + let mut rollup_withdrawal_event_id__ = None; while let Some(k) = map_.next_key()? { match k { GeneratedField::To => { @@ -717,6 +740,20 @@ impl<'de> serde::Deserialize<'de> for BridgeUnlockAction { } bridge_address__ = map_.next_value()?; } + GeneratedField::RollupBlockNumber => { + if rollup_block_number__.is_some() { + return Err(serde::de::Error::duplicate_field("rollupBlockNumber")); + } + rollup_block_number__ = + Some(map_.next_value::<::pbjson::private::NumberDeserialize<_>>()?.0) + ; + } + GeneratedField::RollupWithdrawalEventId => { + if rollup_withdrawal_event_id__.is_some() { + return Err(serde::de::Error::duplicate_field("rollupWithdrawalEventId")); + } + rollup_withdrawal_event_id__ = Some(map_.next_value()?); + } } } Ok(BridgeUnlockAction { @@ -725,6 +762,8 @@ impl<'de> serde::Deserialize<'de> for BridgeUnlockAction { fee_asset: fee_asset__.unwrap_or_default(), memo: memo__.unwrap_or_default(), bridge_address: bridge_address__, + rollup_block_number: rollup_block_number__.unwrap_or_default(), + rollup_withdrawal_event_id: rollup_withdrawal_event_id__.unwrap_or_default(), }) } } diff --git a/crates/astria-core/src/protocol/memos/snapshots/astria_core__protocol__memos__v1alpha1__test__bridge_unlock_memo_snapshot.snap b/crates/astria-core/src/protocol/memos/snapshots/astria_core__protocol__memos__v1alpha1__test__bridge_unlock_memo_snapshot.snap deleted file mode 100644 index 03fd3880e5..0000000000 --- a/crates/astria-core/src/protocol/memos/snapshots/astria_core__protocol__memos__v1alpha1__test__bridge_unlock_memo_snapshot.snap +++ /dev/null @@ -1,8 +0,0 @@ ---- -source: crates/astria-core/src/protocol/memos/v1alpha1.rs -expression: memo ---- -{ - "rollupBlockNumber": "42", - "rollupTransactionHash": "a-rollup-defined-hash" -} diff --git a/crates/astria-core/src/protocol/memos/snapshots/astria_core__protocol__memos__v1alpha1__test__ics20_withdrawal_from_rollup_memo_snapshot.snap b/crates/astria-core/src/protocol/memos/snapshots/astria_core__protocol__memos__v1alpha1__test__ics20_withdrawal_from_rollup_memo_snapshot.snap index 8fdb7038ea..0f7388fcdd 100644 --- a/crates/astria-core/src/protocol/memos/snapshots/astria_core__protocol__memos__v1alpha1__test__ics20_withdrawal_from_rollup_memo_snapshot.snap +++ b/crates/astria-core/src/protocol/memos/snapshots/astria_core__protocol__memos__v1alpha1__test__ics20_withdrawal_from_rollup_memo_snapshot.snap @@ -4,7 +4,7 @@ expression: memo --- { "rollupBlockNumber": "1", - "rollupTransactionHash": "a-rollup-defined-hash", + "rollupWithdrawalEventId": "a-rollup-defined-hash", "rollupReturnAddress": "a-rollup-defined-address", "memo": "hello" } diff --git a/crates/astria-core/src/protocol/memos/v1alpha1.rs b/crates/astria-core/src/protocol/memos/v1alpha1.rs index 6de9472aaf..9bc1d794a0 100644 --- a/crates/astria-core/src/protocol/memos/v1alpha1.rs +++ b/crates/astria-core/src/protocol/memos/v1alpha1.rs @@ -1,5 +1,4 @@ pub use crate::generated::protocol::memos::v1alpha1::{ - BridgeUnlock, Ics20TransferDeposit, Ics20WithdrawalFromRollup, }; @@ -8,22 +7,12 @@ pub use crate::generated::protocol::memos::v1alpha1::{ mod test { use super::*; - #[test] - fn bridge_unlock_memo_snapshot() { - let memo = BridgeUnlock { - rollup_block_number: 42, - rollup_transaction_hash: "a-rollup-defined-hash".to_string(), - }; - - insta::assert_json_snapshot!(memo); - } - #[test] fn ics20_withdrawal_from_rollup_memo_snapshot() { let memo = Ics20WithdrawalFromRollup { rollup_block_number: 1, rollup_return_address: "a-rollup-defined-address".to_string(), - rollup_transaction_hash: "a-rollup-defined-hash".to_string(), + rollup_withdrawal_event_id: "a-rollup-defined-hash".to_string(), memo: "hello".to_string(), }; diff --git a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs index 6fb4f3d2cc..fe23ffd23a 100644 --- a/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs +++ b/crates/astria-core/src/protocol/transaction/v1alpha1/action.rs @@ -1544,10 +1544,14 @@ pub struct BridgeUnlockAction { pub amount: u128, // asset to use for fee payment. pub fee_asset: asset::Denom, - // memo for double spend protection. - pub memo: String, // the address of the bridge account to transfer from. pub bridge_address: Address, + // A field for users to additional identifying information + pub memo: String, + // The block number of the rollup block containing the withdrawal event. + pub rollup_block_number: u64, + // The identifier of the withdrawal event in the rollup block. + pub rollup_withdrawal_event_id: String, } impl Protobuf for BridgeUnlockAction { @@ -1562,6 +1566,8 @@ impl Protobuf for BridgeUnlockAction { fee_asset: self.fee_asset.to_string(), memo: self.memo, bridge_address: Some(self.bridge_address.into_raw()), + rollup_block_number: self.rollup_block_number, + rollup_withdrawal_event_id: self.rollup_withdrawal_event_id, } } @@ -1573,6 +1579,8 @@ impl Protobuf for BridgeUnlockAction { fee_asset: self.fee_asset.to_string(), memo: self.memo.clone(), bridge_address: Some(self.bridge_address.to_raw()), + rollup_block_number: self.rollup_block_number, + rollup_withdrawal_event_id: self.rollup_withdrawal_event_id.clone(), } } @@ -1592,6 +1600,8 @@ impl Protobuf for BridgeUnlockAction { fee_asset, memo, bridge_address, + rollup_block_number, + rollup_withdrawal_event_id, } = proto; let to = to .ok_or_else(|| BridgeUnlockActionError::field_not_set("to")) @@ -1612,6 +1622,8 @@ impl Protobuf for BridgeUnlockAction { fee_asset, memo, bridge_address, + rollup_block_number, + rollup_withdrawal_event_id, }) } 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 ce51df7d23..887dcec2b7 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 @@ -3,36 +3,36 @@ source: crates/astria-sequencer/src/app/tests_breaking_changes.rs expression: app.app_hash.as_bytes() --- [ - 126, - 205, - 191, - 0, + 241, + 61, + 69, + 211, 65, - 191, - 147, - 26, - 88, - 230, - 39, - 60, - 192, - 5, - 177, - 11, - 248, - 83, - 115, - 225, - 203, - 17, - 58, - 245, - 71, - 172, - 232, + 119, + 7, + 14, + 200, + 183, 75, - 203, - 163, - 32, - 75 + 232, + 68, + 126, + 56, + 5, + 8, + 220, + 199, + 173, + 193, + 135, + 169, + 26, + 36, + 119, + 144, + 174, + 76, + 108, + 221, + 225 ] diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 6909cc833d..f18739a5e5 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -271,8 +271,10 @@ async fn app_execute_transaction_with_every_action_snapshot() { to: bob_address, amount: 10, fee_asset: nria().into(), - memo: "{}".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(), BridgeSudoChangeAction { diff --git a/crates/astria-sequencer/src/app/tests_execute_transaction.rs b/crates/astria-sequencer/src/app/tests_execute_transaction.rs index c433c48fcf..2c60e06f83 100644 --- a/crates/astria-sequencer/src/app/tests_execute_transaction.rs +++ b/crates/astria-sequencer/src/app/tests_execute_transaction.rs @@ -1042,6 +1042,8 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() { fee_asset: nria().into(), memo: "{ \"msg\": \"lilywashere\" }".into(), bridge_address, + rollup_block_number: 1, + rollup_withdrawal_event_id: "id-from-rollup".to_string(), }; let tx = UnsignedTransaction { diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index a49e90c311..615f77b9e2 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -17,17 +17,35 @@ use crate::{ }, address::StateReadExt as _, app::ActionHandler, - bridge::StateReadExt as _, + bridge::{ + StateReadExt as _, + StateWriteExt as _, + }, transaction::StateReadExt as _, }; #[async_trait::async_trait] impl ActionHandler for BridgeUnlockAction { + // TODO(https://github.com/astriaorg/astria/issues/1430): move checks to the `BridgeUnlock` parsing. async fn check_stateless(&self) -> Result<()> { + ensure!(self.amount > 0, "amount must be greater than zero",); + ensure!(self.memo.len() <= 64, "memo must not be more than 64 bytes"); + ensure!( + !self.rollup_withdrawal_event_id.is_empty(), + "rollup withdrawal event id must be non-empty", + ); + ensure!( + self.rollup_withdrawal_event_id.len() <= 64, + "rollup withdrawal event id must not be more than 64 bytes", + ); + ensure!( + self.rollup_block_number > 0, + "rollup block number must be greater than zero", + ); Ok(()) } - async fn check_and_execute(&self, state: S) -> Result<()> { + async fn check_and_execute(&self, mut state: S) -> Result<()> { let from = state .get_current_source() .expect("transaction source must be present in state when executing an action") @@ -68,6 +86,14 @@ impl ActionHandler for BridgeUnlockAction { }; check_transfer(&transfer_action, self.bridge_address, &state).await?; + state + .check_and_set_withdrawal_event_block_for_bridge_account( + self.bridge_address, + &self.rollup_withdrawal_event_id, + self.rollup_block_number, + ) + .await + .context("withdrawal event already processed")?; execute_transfer(&transfer_action, self.bridge_address, state).await?; Ok(()) @@ -130,8 +156,10 @@ mod tests { to: to_address, amount: transfer_amount, fee_asset: asset.clone(), - memo: "{}".into(), + memo: String::new(), bridge_address, + rollup_block_number: 1, + rollup_withdrawal_event_id: "a-rollup-defined-hash".to_string(), }; // invalid sender, doesn't match action's `from`, should fail @@ -167,8 +195,10 @@ mod tests { to: to_address, amount: transfer_amount, fee_asset: asset, - memo: "{}".into(), + memo: String::new(), bridge_address, + rollup_block_number: 1, + rollup_withdrawal_event_id: "a-rollup-defined-hash".to_string(), }; // invalid sender, doesn't match action's bridge account's withdrawer, should fail @@ -179,7 +209,7 @@ mod tests { } #[tokio::test] - async fn execute_with_bridge_address_unset() { + async fn execute_with_bridge_address_set() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); @@ -209,8 +239,10 @@ mod tests { to: to_address, amount: transfer_amount, fee_asset: asset.clone(), - memo: "{}".into(), + memo: String::new(), bridge_address, + rollup_block_number: 1, + rollup_withdrawal_event_id: "a-rollup-defined-hash-3".to_string(), }; // not enough balance; should fail @@ -233,7 +265,7 @@ mod tests { } #[tokio::test] - async fn execute_with_bridge_address_set() { + async fn execute_with_duplicated_withdrawal_event_id() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); @@ -258,31 +290,36 @@ mod tests { .unwrap(); state.put_bridge_account_withdrawer_address(bridge_address, bridge_address); state.put_allowed_fee_asset(&asset); + // Put plenty of balance + state + .put_account_balance(bridge_address, &asset, 3 * transfer_amount) + .unwrap(); - let bridge_unlock = BridgeUnlockAction { + let bridge_unlock_first = BridgeUnlockAction { to: to_address, amount: transfer_amount, fee_asset: asset.clone(), - memo: "{}".into(), + memo: String::new(), bridge_address, + rollup_block_number: 1, + rollup_withdrawal_event_id: "a-rollup-defined-hash".to_string(), + }; + let bridge_unlock_second = BridgeUnlockAction { + rollup_block_number: 10, + ..bridge_unlock_first.clone() }; - // not enough balance; should fail - state - .put_account_balance(bridge_address, &asset, transfer_amount) + // first should succeed, next should fail due to duplicate event. + bridge_unlock_first + .check_and_execute(&mut state) + .await .unwrap(); assert_anyhow_error( - &bridge_unlock + &bridge_unlock_second .check_and_execute(&mut state) .await .unwrap_err(), - "insufficient funds for transfer and fee payment", + "withdrawal event already processed", ); - - // enough balance; should pass - state - .put_account_balance(bridge_address, &asset, transfer_amount + transfer_fee) - .unwrap(); - bridge_unlock.check_and_execute(&mut state).await.unwrap(); } } diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index 8e12ba6949..5da4f9bf8a 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -5,6 +5,7 @@ use std::collections::{ use anyhow::{ anyhow, + bail, Context, Result, }; @@ -135,6 +136,20 @@ fn bridge_account_withdrawer_address_storage_key(address: &T) - ) } +fn bridge_account_withdrawal_event_storage_key( + address: &T, + withdrawal_event_id: &str, +) -> String { + format!( + "{}/withdrawalevent/{}", + BridgeAccountKey { + prefix: BRIDGE_ACCOUNT_PREFIX, + address + }, + withdrawal_event_id + ) +} + fn last_transaction_hash_for_bridge_account_storage_key(address: &T) -> Vec { format!( "{}/lasttx", @@ -419,6 +434,37 @@ pub(crate) trait StateWriteExt: StateWrite { ); } + #[instrument(skip_all)] + async fn check_and_set_withdrawal_event_block_for_bridge_account( + &mut self, + address: T, + withdrawal_event_id: &str, + block_num: u64, + ) -> Result<()> { + let key = bridge_account_withdrawal_event_storage_key(&address, withdrawal_event_id); + + // Check if the withdrawal ID has already been used, if so return an error. + let bytes = self + .get_raw(&key) + .await + .context("failed reading raw withdrawal event from state")?; + if let Some(bytes) = bytes { + let existing_block_num = u64::from_be_bytes( + bytes + .try_into() + .expect("all block numbers stored should be 8 bytes; this is a bug"), + ); + + bail!( + "withdrawal event ID {withdrawal_event_id} used by block number \ + {existing_block_num}" + ); + } + + self.put_raw(key, block_num.to_be_bytes().to_vec()); + Ok(()) + } + // the deposit "nonce" for a given rollup ID during a given block. // this is only used to generate storage keys for each of the deposits within a block, // and is reset to 0 at the beginning of each block. diff --git a/crates/astria-sequencer/src/ibc/ics20_transfer.rs b/crates/astria-sequencer/src/ibc/ics20_transfer.rs index b632474bcb..9149859455 100644 --- a/crates/astria-sequencer/src/ibc/ics20_transfer.rs +++ b/crates/astria-sequencer/src/ibc/ics20_transfer.rs @@ -1052,7 +1052,7 @@ mod test { memo: String::new(), rollup_block_number: 1, rollup_return_address: "rollup-defined".to_string(), - rollup_transaction_hash: hex::encode([1u8; 32]), + rollup_withdrawal_event_id: "a-rollup-defined-id".into(), }) .unwrap(), }; diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index cf5d77221a..515befa6ba 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -9,7 +9,10 @@ use astria_core::{ asset::Denom, Address, }, - protocol::transaction::v1alpha1::action, + protocol::{ + memos::v1alpha1::Ics20WithdrawalFromRollup, + transaction::v1alpha1::action, + }, Protobuf as _, }; use cnidarium::{ @@ -35,7 +38,10 @@ use crate::{ address::StateReadExt as _, app::ActionHandler, assets::StateWriteExt as _, - bridge::StateReadExt as _, + bridge::{ + StateReadExt as _, + StateWriteExt as _, + }, ibc::{ StateReadExt as _, StateWriteExt as _, @@ -63,49 +69,81 @@ fn withdrawal_to_unchecked_ibc_packet( /// Establishes the withdrawal target. /// /// The function returns the following addresses under the following conditions: -/// 1. `from` if `action.bridge_address` is unset and `from` is *not* a bridge account; -/// 2. `from` if `action.bridge_address` is unset and `from` is a bridge account and `from` is its -/// stored withdrawer address. -/// 3. `action.bridge_address` if `action.bridge_address` is set and a bridge account and `from` is -/// its stored withdrawer address. +/// 1. `action.bridge_address` if `action.bridge_address` is set and `from` is its stored withdrawer +/// address. +/// 2. `from` if `action.bridge_address` is unset and `from` is *not* a bridge account. +/// +/// Errors if: +/// 1. Errors reading from DB +/// 2. `action.bridge_address` is set, but `from` is not the withdrawer address. +/// 3. `action.bridge_address` is unset, but `from` is a bridge account. async fn establish_withdrawal_target( action: &action::Ics20Withdrawal, state: &S, from: [u8; 20], ) -> Result<[u8; 20]> { - if action.bridge_address.is_none() - && !state - .is_a_bridge_account(from) + // If the bridge address is set, the withdrawer on that address must match + // the from address. + if let Some(bridge_address) = action.bridge_address { + let Some(withdrawer) = state + .get_bridge_account_withdrawer_address(bridge_address) .await - .context("failed to get bridge account rollup id")? - { - return Ok(from); - } + .context("failed to get bridge withdrawer")? + else { + bail!("bridge address must have a withdrawer address set"); + }; + + ensure!( + withdrawer == from.address_bytes(), + "sender does not match bridge withdrawer address; unauthorized" + ); - // if `action.bridge_address` is set, but it's not a valid bridge account, - // the `get_bridge_account_withdrawer_address` step will fail. - let bridge_address = action.bridge_address.map_or(from, Address::bytes); + return Ok(bridge_address.address_bytes()); + } - let Some(withdrawer) = state - .get_bridge_account_withdrawer_address(bridge_address) + // If the bridge address is not set, the sender must not be a bridge account. + if state + .is_a_bridge_account(from) .await - .context("failed to get bridge withdrawer")? - else { - bail!("bridge address must have a withdrawer address set"); - }; - - ensure!( - withdrawer == from.address_bytes(), - "sender does not match bridge withdrawer address; unauthorized" - ); + .context("failed to establish whether the sender is a bridge account")? + { + bail!("sender cannot be a bridge address if bridge address is not set"); + } - Ok(bridge_address) + Ok(from) } #[async_trait::async_trait] impl ActionHandler for action::Ics20Withdrawal { + // TODO(https://github.com/astriaorg/astria/issues/1430): move checks to the `Ics20Withdrawal` parsing. async fn check_stateless(&self) -> Result<()> { ensure!(self.timeout_time() != 0, "timeout time must be non-zero",); + ensure!(self.amount() > 0, "amount must be greater than zero",); + if self.bridge_address.is_some() { + let parsed_bridge_memo: Ics20WithdrawalFromRollup = serde_json::from_str(&self.memo) + .context("failed to parse memo for ICS bound bridge withdrawal")?; + + ensure!( + !parsed_bridge_memo.rollup_return_address.is_empty(), + "rollup return address must be non-empty", + ); + ensure!( + parsed_bridge_memo.rollup_return_address.len() <= 256, + "rollup return address must be no more than 256 bytes", + ); + ensure!( + !parsed_bridge_memo.rollup_withdrawal_event_id.is_empty(), + "rollup withdrawal event id must be non-empty", + ); + ensure!( + parsed_bridge_memo.rollup_withdrawal_event_id.len() <= 64, + "rollup withdrawal event id must be no more than 64 bytes", + ); + ensure!( + parsed_bridge_memo.rollup_block_number != 0, + "rollup block number must be non-zero", + ); + } // NOTE (from penumbra): we could validate the destination chain address as bech32 to // prevent mistyped addresses, but this would preclude sending to chains that don't @@ -128,6 +166,17 @@ impl ActionHandler for action::Ics20Withdrawal { state.ensure_base_prefix(bridge_address).await.context( "failed to verify that bridge address address has permitted base prefix", )?; + let parsed_bridge_memo: Ics20WithdrawalFromRollup = serde_json::from_str(&self.memo) + .context("failed to parse memo for ICS bound bridge withdrawal")?; + + state + .check_and_set_withdrawal_event_block_for_bridge_account( + self.bridge_address.map_or(from, Address::bytes), + &parsed_bridge_memo.rollup_withdrawal_event_id, + parsed_bridge_memo.rollup_block_number, + ) + .await + .context("withdrawal event already processed")?; } let withdrawal_target = establish_withdrawal_target(self, &state, from) @@ -207,7 +256,6 @@ mod tests { use super::*; use crate::{ address::StateWriteExt as _, - bridge::StateWriteExt as _, test_utils::{ assert_anyhow_error, astria_address, @@ -274,11 +322,11 @@ mod tests { memo: String::new(), }; - assert_eq!( - establish_withdrawal_target(&action, &state, bridge_address) + assert_anyhow_error( + &establish_withdrawal_target(&action, &state, bridge_address) .await - .unwrap(), - bridge_address, + .unwrap_err(), + "sender cannot be a bridge address if bridge address is not set", ); } @@ -333,13 +381,6 @@ mod tests { ); } - #[tokio::test] - async fn bridge_unset() { - let mut action = action(); - action.bridge_address = None; - run_test(action).await; - } - #[tokio::test] async fn bridge_set() { let mut action = action(); diff --git a/proto/protocolapis/astria/protocol/memos/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/memos/v1alpha1/types.proto index 4e8b2e4936..00435b6697 100644 --- a/proto/protocolapis/astria/protocol/memos/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/memos/v1alpha1/types.proto @@ -9,30 +9,19 @@ syntax = "proto3"; package astria.protocol.memos.v1alpha1; -message BridgeUnlock { - // The block number on the rollup that triggered the transaction underlying - // this bridge unlock memo. - uint64 rollup_block_number = 1; - // The hash of the original rollup transaction that triggered a bridge unlock - // and that is underlying this bridge unlock memo. - // - // This field is of type `string` so that it can be formatted in the preferred - // format of the rollup when targeting plain text encoding. - string rollup_transaction_hash = 2; -} - // Memo for an ICS20 withdrawal from the rollup which is sent to // an external IBC-enabled chain. message Ics20WithdrawalFromRollup { // The block number on the rollup that triggered the transaction underlying // this ics20 withdrawal memo. uint64 rollup_block_number = 1; - // The hash of the original rollup transaction that triggered this ics20 - // withdrawal and that is underlying this bridge unlock memo. + // An identifier of the original rollup withdrawal event that triggered this ics20 + // withdrawal and that is underlying this bridge unlock memo. For general EVM + // this is typically a transaction hash. // // This field is of type `string` so that it can be formatted in the preferred // format of the rollup when targeting plain text encoding. - string rollup_transaction_hash = 2; + string rollup_withdrawal_event_id = 2; // The return address on the rollup to which funds should returned in case of // failure. This field exists so that the rollup can identify which account // the returned funds originated from. diff --git a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto index 19069f44a5..05f40dc21b 100644 --- a/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto +++ b/proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto @@ -209,10 +209,22 @@ message BridgeUnlockAction { astria.primitive.v1.Uint128 amount = 2; // the asset used to pay the transaction fee string fee_asset = 3; - // memo for double spend prevention + // The memo field can be used to provide unique identifying additional + // information about the bridge unlock transaction. string memo = 4; // the address of the bridge account to transfer from astria.primitive.v1.Address bridge_address = 5; + // The block number on the rollup that triggered the transaction underlying + // this bridge unlock memo. + uint64 rollup_block_number = 6; + // An identifier of the original rollup event, such as a transaction hash which + // triggered a bridge unlock and is underlying event that led to this bridge + // unlock. This can be utilized for tracing from the bridge back to + // distinct rollup events. + // + // This field is of type `string` so that it can be formatted in the preferred + // format of the rollup when targeting plain text encoding. + string rollup_withdrawal_event_id = 7; } message BridgeSudoChangeAction {