fix(bridge-contracts): fix memo transaction hash encoding (#1428)

## Summary
Use the correct encoding functions to populate memo field in the
bridge's conversion logic from rollup withdrawal events to sequencer
actions.

## Background
We were using `ethers::H256::to_string()` to get the string for the
`rollup_transaction_hash` value, which returns a shortened version of
the hash. the string `0x1234...0x1234` is instead of the full hash (i.e.
you would expect it to give you something like
`0x1234567890123456789012345678901234567890123456789012345678901234`).

## Changes
- Use the correct 

## Breaking Changelist
- The previously invalid memo data will now be populated with the
correct information. This doesn't actually break anything because we
also aren't actually using these hashes anywhere.

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

closes #1427, #1471

---------

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>

Author: itamarreif

crates/astria-bridge-contracts/src/lib.rs

@@ -25,6 +25,8 @@ use astria_withdrawer::{
     SequencerWithdrawalFilter,
 };
 use ethers::{
+    self,
+    abi::AbiEncode,
     contract::EthEvent,
     providers::Middleware,
     types::{
@@ -378,10 +380,16 @@ where
             .ok_or_else(|| GetWithdrawalActionsError::log_without_block_number(&log))?
             .as_u64();
 
-        let rollup_withdrawal_event_id = log
+        let transaction_hash = log
             .transaction_hash
             .ok_or_else(|| GetWithdrawalActionsError::log_without_transaction_hash(&log))?
-            .to_string();
+            .encode_hex();
+        let event_index = log
+            .log_index
+            .ok_or_else(|| GetWithdrawalActionsError::log_without_log_index(&log))?
+            .encode_hex();
+
+        let rollup_withdrawal_event_id = format!("{transaction_hash}.{event_index}");
 
         let event = decode_log::<Ics20WithdrawalFilter>(log)
             .map_err(GetWithdrawalActionsError::decode_log)?;
@@ -436,10 +444,16 @@ where
             .ok_or_else(|| GetWithdrawalActionsError::log_without_block_number(&log))?
             .as_u64();
 
-        let rollup_withdrawal_event_id = log
+        let transaction_hash = log
             .transaction_hash
             .ok_or_else(|| GetWithdrawalActionsError::log_without_transaction_hash(&log))?
-            .to_string();
+            .encode_hex();
+        let event_index = log
+            .log_index
+            .ok_or_else(|| GetWithdrawalActionsError::log_without_log_index(&log))?
+            .encode_hex();
+
+        let rollup_withdrawal_event_id = format!("{transaction_hash}.{event_index}");
 
         let event = decode_log::<SequencerWithdrawalFilter>(log)
             .map_err(GetWithdrawalActionsError::decode_log)?;
@@ -502,6 +516,11 @@ impl GetWithdrawalActionsError {
     fn log_without_transaction_hash(_log: &Log) -> Self {
         Self(GetWithdrawalActionsErrorKind::LogWithoutTransactionHash)
     }
+
+    // XXX: Somehow identify the log?
+    fn log_without_log_index(_log: &Log) -> Self {
+        Self(GetWithdrawalActionsErrorKind::LogWithoutLogIndex)
+    }
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -518,6 +537,8 @@ enum GetWithdrawalActionsErrorKind {
     LogWithoutBlockNumber,
     #[error("log did not contain a transaction hash")]
     LogWithoutTransactionHash,
+    #[error("log did not contain a log index")]
+    LogWithoutLogIndex,
     #[error(transparent)]
     CalculateWithdrawalAmount(CalculateWithdrawalAmountError),
 }

crates/astria-bridge-withdrawer/tests/blackbox/helpers/mod.rs

@@ -13,8 +13,10 @@ pub use self::{
     test_bridge_withdrawer::{
         assert_actions_eq,
         default_sequencer_address,
-        make_bridge_unlock_action,
-        make_ics20_withdrawal_action,
+        make_erc20_bridge_unlock_action,
+        make_erc20_ics20_withdrawal_action,
+        make_native_bridge_unlock_action,
+        make_native_ics20_withdrawal_action,
         TestBridgeWithdrawerConfig,
     },
 };

crates/astria-bridge-withdrawer/tests/blackbox/helpers/test_bridge_withdrawer.rs

@@ -28,7 +28,10 @@ use astria_core::{
         },
     },
 };
-use ethers::types::TransactionReceipt;
+use ethers::{
+    abi::AbiEncode,
+    types::TransactionReceipt,
+};
 use futures::Future;
 use ibc_types::core::{
     channel::ChannelId,
@@ -427,13 +430,66 @@ impl From<Ics20Withdrawal> for SubsetOfIcs20Withdrawal {
 }
 
 #[must_use]
-pub fn make_bridge_unlock_action(receipt: &TransactionReceipt) -> Action {
+pub fn make_native_bridge_unlock_action(receipt: &TransactionReceipt) -> Action {
+    let denom = default_native_asset();
+    let rollup_transaction_hash = receipt.transaction_hash.encode_hex();
+    let event_index = receipt.logs[0].log_index.unwrap().encode_hex();
+
+    let inner = BridgeUnlockAction {
+        to: default_sequencer_address(),
+        amount: 1_000_000u128,
+        rollup_block_number: receipt.block_number.unwrap().as_u64(),
+        rollup_withdrawal_event_id: format!("{rollup_transaction_hash}.{event_index}"),
+        memo: String::new(),
+        fee_asset: denom,
+        bridge_address: default_bridge_address(),
+    };
+    Action::BridgeUnlock(inner)
+}
+
+#[must_use]
+pub fn make_native_ics20_withdrawal_action(receipt: &TransactionReceipt) -> Action {
+    let timeout_height = IbcHeight::new(u64::MAX, u64::MAX).unwrap();
+    let timeout_time = make_ibc_timeout_time();
+    let denom = default_ibc_asset();
+    let rollup_transaction_hash = receipt.transaction_hash.encode_hex();
+    let event_index = receipt.logs[0].log_index.unwrap().encode_hex();
+
+    let inner = Ics20Withdrawal {
+        denom: denom.clone(),
+        destination_chain_address: default_sequencer_address().to_string(),
+        return_address: default_bridge_address(),
+        amount: 1_000_000u128,
+        memo: serde_json::to_string(&Ics20WithdrawalFromRollup {
+            memo: "nootwashere".to_string(),
+            rollup_return_address: receipt.from.to_string(),
+            rollup_block_number: receipt.block_number.unwrap().as_u64(),
+            rollup_withdrawal_event_id: format!("{rollup_transaction_hash}.{event_index}"),
+        })
+        .unwrap(),
+        fee_asset: denom,
+        timeout_height,
+        timeout_time,
+        source_channel: "channel-0".parse().unwrap(),
+        bridge_address: Some(default_bridge_address()),
+        use_compat_address: false,
+    };
+
+    Action::Ics20Withdrawal(inner)
+}
+
+#[must_use]
+pub fn make_erc20_bridge_unlock_action(receipt: &TransactionReceipt) -> Action {
     let denom = default_native_asset();
+    let rollup_transaction_hash = receipt.transaction_hash.encode_hex();
+    // use the second event because the erc20 transfer also emits an event
+    let event_index = receipt.logs[1].log_index.unwrap().encode_hex();
+
     let inner = BridgeUnlockAction {
         to: default_sequencer_address(),
         amount: 1_000_000u128,
         rollup_block_number: receipt.block_number.unwrap().as_u64(),
-        rollup_withdrawal_event_id: receipt.transaction_hash.to_string(),
+        rollup_withdrawal_event_id: format!("{rollup_transaction_hash}.{event_index}"),
         memo: String::new(),
         fee_asset: denom,
         bridge_address: default_bridge_address(),
@@ -442,10 +498,14 @@ pub fn make_bridge_unlock_action(receipt: &TransactionReceipt) -> Action {
 }
 
 #[must_use]
-pub fn make_ics20_withdrawal_action(receipt: &TransactionReceipt) -> Action {
+pub fn make_erc20_ics20_withdrawal_action(receipt: &TransactionReceipt) -> Action {
     let timeout_height = IbcHeight::new(u64::MAX, u64::MAX).unwrap();
     let timeout_time = make_ibc_timeout_time();
     let denom = default_ibc_asset();
+    let rollup_transaction_hash = receipt.transaction_hash.encode_hex();
+    // use the second event because the erc20 transfer also emits an event
+    let event_index = receipt.logs[1].log_index.unwrap().encode_hex();
+
     let inner = Ics20Withdrawal {
         denom: denom.clone(),
         destination_chain_address: default_sequencer_address().to_string(),
@@ -455,7 +515,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_withdrawal_event_id: receipt.transaction_hash.to_string(),
+            rollup_withdrawal_event_id: format!("{rollup_transaction_hash}.{event_index}"),
         })
         .unwrap(),
         fee_asset: denom,

crates/astria-bridge-withdrawer/tests/blackbox/main.rs

@@ -2,8 +2,10 @@ use astria_core::protocol::transaction::v1alpha1::Action;
 use helpers::{
     assert_actions_eq,
     default_sequencer_address,
-    make_bridge_unlock_action,
-    make_ics20_withdrawal_action,
+    make_erc20_bridge_unlock_action,
+    make_erc20_ics20_withdrawal_action,
+    make_native_bridge_unlock_action,
+    make_native_ics20_withdrawal_action,
     signed_tx_from_request,
     TestBridgeWithdrawerConfig,
 };
@@ -39,7 +41,7 @@ async fn native_sequencer_withdraw_success() {
         )
         .await;
 
-    assert_contract_receipt_action_matches_broadcast_action::<BridgeUnlock>(
+    assert_contract_receipt_action_matches_broadcast_action::<BridgeUnlock, NativeAsset>(
         &broadcast_guard.received_requests().await,
         &receipt,
     );
@@ -76,7 +78,7 @@ async fn native_ics20_withdraw_success() {
         )
         .await;
 
-    assert_contract_receipt_action_matches_broadcast_action::<Ics20>(
+    assert_contract_receipt_action_matches_broadcast_action::<Ics20, NativeAsset>(
         &broadcast_guard.received_requests().await,
         &receipt,
     );
@@ -119,7 +121,7 @@ async fn erc20_sequencer_withdraw_success() {
         )
         .await;
 
-    assert_contract_receipt_action_matches_broadcast_action::<BridgeUnlock>(
+    assert_contract_receipt_action_matches_broadcast_action::<BridgeUnlock, Erc20>(
         &broadcast_guard.received_requests().await,
         &receipt,
     );
@@ -162,34 +164,54 @@ async fn erc20_ics20_withdraw_success() {
         )
         .await;
 
-    assert_contract_receipt_action_matches_broadcast_action::<Ics20>(
+    assert_contract_receipt_action_matches_broadcast_action::<Ics20, Erc20>(
         &broadcast_guard.received_requests().await,
         &receipt,
     );
 }
 
-trait ActionFromReceipt {
+trait ActionFromReceipt<TAsset> {
     fn action_from_receipt(receipt: &ethers::types::TransactionReceipt) -> Action;
 }
 
+struct NativeAsset;
+struct Erc20;
+
 struct BridgeUnlock;
-impl ActionFromReceipt for BridgeUnlock {
+impl ActionFromReceipt<NativeAsset> for BridgeUnlock {
+    #[track_caller]
+    fn action_from_receipt(receipt: &ethers::types::TransactionReceipt) -> Action {
+        make_native_bridge_unlock_action(receipt)
+    }
+}
+
+impl ActionFromReceipt<Erc20> for BridgeUnlock {
     #[track_caller]
     fn action_from_receipt(receipt: &ethers::types::TransactionReceipt) -> Action {
-        make_bridge_unlock_action(receipt)
+        make_erc20_bridge_unlock_action(receipt)
     }
 }
 
 struct Ics20;
-impl ActionFromReceipt for Ics20 {
+impl ActionFromReceipt<NativeAsset> for Ics20 {
+    #[track_caller]
+    fn action_from_receipt(receipt: &ethers::types::TransactionReceipt) -> Action {
+        make_native_ics20_withdrawal_action(receipt)
+    }
+}
+
+impl ActionFromReceipt<Erc20> for Ics20 {
     #[track_caller]
     fn action_from_receipt(receipt: &ethers::types::TransactionReceipt) -> Action {
-        make_ics20_withdrawal_action(receipt)
+        make_erc20_ics20_withdrawal_action(receipt)
     }
 }
 
 #[track_caller]
-fn assert_contract_receipt_action_matches_broadcast_action<T: ActionFromReceipt>(
+fn assert_contract_receipt_action_matches_broadcast_action<
+    TAction: ActionFromReceipt<TAsset>,
+    TAsset,
+>(
     received_broadcasts: &[wiremock::Request],
     receipt: &ethers::types::TransactionReceipt,
 ) {
@@ -201,6 +223,6 @@ fn assert_contract_receipt_action_matches_broadcast_action<T: ActionFromReceipt>
         .first()
         .expect("the signed transaction should contain at least one action");
 
-    let expected = T::action_from_receipt(receipt);
+    let expected = TAction::action_from_receipt(receipt);
     assert_actions_eq(&expected, actual);
 }

crates/astria-sequencer/src/bridge/bridge_unlock_action.rs

@@ -35,8 +35,8 @@ impl ActionHandler for BridgeUnlockAction {
             "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",
+            self.rollup_withdrawal_event_id.len() <= 256,
+            "rollup withdrawal event id must not be more than 256 bytes",
         );
         ensure!(
             self.rollup_block_number > 0,

crates/astria-sequencer/src/ibc/ics20_withdrawal.rs

@@ -164,8 +164,8 @@ impl ActionHandler for action::Ics20Withdrawal {
                 "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",
+                parsed_bridge_memo.rollup_withdrawal_event_id.len() <= 256,
+                "rollup withdrawal event id must be no more than 256 bytes",
             );
             ensure!(
                 parsed_bridge_memo.rollup_block_number != 0,