refactor(core, sequencer)!: require that bridge unlock address always…

… be set (#1339)

## Summary
Requires that
`astria.protocol.v1alpha1.BridgeUnlockAction.bridge_address` is always.

## Background
Before this patch, the transaction signer containing the bridge unlock
action was used as the bridge address if the bridge address was not set.
This means that setting the bridge address to the signer or leaving it
unset resultes in the same behavior. This patch makes the bridge address
a requirement so that a bridge unlock is easier to understand at a first
glance (i.e. without needing to remember implementation or spec
details), and because two separate actions should not lead to the same
result.

## Changes
- Make `astria.protocol.v1alpha1.BridgeUnlockAction.bridge_address` a
required field.

## Testing
Tests have been updated to reflect this change: explicitly setteing the
bridge address to the signer makes those tests pass again that relied on
the implicit behaviour.

## Breaking Changelist
This is a protocl breaking change because newer sequencers will reject
bridge unlock actions without a bridge address.

## Related Issues
Closes #1338

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

@@ -458,7 +458,7 @@ where
             amount,
             memo,
             fee_asset: self.fee_asset.clone(),
-            bridge_address: Some(self.bridge_address),
+            bridge_address: self.bridge_address,
         };
 
         Ok(Action::BridgeUnlock(action))

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

@@ -429,7 +429,7 @@ pub fn make_bridge_unlock_action(receipt: &TransactionReceipt) -> Action {
         })
         .unwrap(),
         fee_asset: denom,
-        bridge_address: Some(default_bridge_address()),
+        bridge_address: default_bridge_address(),
     };
     Action::BridgeUnlock(inner)
 }

crates/astria-core/src/protocol/transaction/v1alpha1/action.rs

@@ -1546,10 +1546,8 @@ pub struct BridgeUnlockAction {
     pub fee_asset: asset::Denom,
     // memo for double spend protection.
     pub memo: String,
-    // the address of the bridge account to transfer from,
-    // if the bridge account's withdrawer address is not the same as the bridge address.
-    // if unset, the signer of the transaction is used.
-    pub bridge_address: Option<Address>,
+    // the address of the bridge account to transfer from.
+    pub bridge_address: Address,
 }
 
 impl Protobuf for BridgeUnlockAction {
@@ -1559,11 +1557,11 @@ impl Protobuf for BridgeUnlockAction {
     #[must_use]
     fn into_raw(self) -> raw::BridgeUnlockAction {
         raw::BridgeUnlockAction {
-            to: Some(self.to.to_raw()),
+            to: Some(self.to.into_raw()),
             amount: Some(self.amount.into()),
             fee_asset: self.fee_asset.to_string(),
             memo: self.memo,
-            bridge_address: self.bridge_address.map(Address::into_raw),
+            bridge_address: Some(self.bridge_address.into_raw()),
         }
     }
 
@@ -1574,7 +1572,7 @@ impl Protobuf for BridgeUnlockAction {
             amount: Some(self.amount.into()),
             fee_asset: self.fee_asset.to_string(),
             memo: self.memo.clone(),
-            bridge_address: self.bridge_address.as_ref().map(Address::to_raw),
+            bridge_address: Some(self.bridge_address.to_raw()),
         }
     }
 
@@ -1587,29 +1585,32 @@ impl Protobuf for BridgeUnlockAction {
     /// - if the `amount` field is invalid
     /// - if the `fee_asset` field is invalid
     /// - if the `from` field is invalid
-    fn try_from_raw(proto: raw::BridgeUnlockAction) -> Result<Self, BridgeUnlockActionError> {
-        let Some(to) = proto.to else {
-            return Err(BridgeUnlockActionError::field_not_set("to"));
-        };
-        let to = Address::try_from_raw(&to).map_err(BridgeUnlockActionError::address)?;
-        let amount = proto
-            .amount
-            .ok_or(BridgeUnlockActionError::missing_amount())?;
-        let fee_asset = proto
-            .fee_asset
+    fn try_from_raw(proto: raw::BridgeUnlockAction) -> Result<Self, Self::Error> {
+        let raw::BridgeUnlockAction {
+            to,
+            amount,
+            fee_asset,
+            memo,
+            bridge_address,
+        } = proto;
+        let to = to
+            .ok_or_else(|| BridgeUnlockActionError::field_not_set("to"))
+            .and_then(|to| Address::try_from_raw(&to).map_err(BridgeUnlockActionError::address))?;
+        let amount = amount.ok_or_else(|| BridgeUnlockActionError::field_not_set("amount"))?;
+        let fee_asset = fee_asset
             .parse()
-            .map_err(BridgeUnlockActionError::invalid_fee_asset)?;
-        let bridge_address = proto
-            .bridge_address
-            .as_ref()
-            .map(Address::try_from_raw)
-            .transpose()
-            .map_err(BridgeUnlockActionError::invalid_bridge_address)?;
+            .map_err(BridgeUnlockActionError::fee_asset)?;
+
+        let bridge_address = bridge_address
+            .ok_or_else(|| BridgeUnlockActionError::field_not_set("bridge_address"))
+            .and_then(|to| {
+                Address::try_from_raw(&to).map_err(BridgeUnlockActionError::bridge_address)
+            })?;
         Ok(Self {
             to,
             amount: amount.into(),
             fee_asset,
-            memo: proto.memo,
+            memo,
             bridge_address,
         })
     }
@@ -1646,18 +1647,17 @@ impl BridgeUnlockActionError {
     }
 
     #[must_use]
-    fn missing_amount() -> Self {
-        Self(BridgeUnlockActionErrorKind::MissingAmount)
-    }
-
-    #[must_use]
-    fn invalid_fee_asset(err: asset::ParseDenomError) -> Self {
-        Self(BridgeUnlockActionErrorKind::InvalidFeeAsset(err))
+    fn fee_asset(source: asset::ParseDenomError) -> Self {
+        Self(BridgeUnlockActionErrorKind::FeeAsset {
+            source,
+        })
     }
 
     #[must_use]
-    fn invalid_bridge_address(err: AddressError) -> Self {
-        Self(BridgeUnlockActionErrorKind::InvalidBridgeAddress(err))
+    fn bridge_address(source: AddressError) -> Self {
+        Self(BridgeUnlockActionErrorKind::BridgeAddress {
+            source,
+        })
     }
 }
 
@@ -1667,12 +1667,10 @@ enum BridgeUnlockActionErrorKind {
     FieldNotSet(&'static str),
     #[error("the `to` field was invalid")]
     Address { source: AddressError },
-    #[error("the `amount` field was not set")]
-    MissingAmount,
     #[error("the `fee_asset` field was invalid")]
-    InvalidFeeAsset(#[source] asset::ParseDenomError),
+    FeeAsset { source: asset::ParseDenomError },
     #[error("the `bridge_address` field was invalid")]
-    InvalidBridgeAddress(#[source] AddressError),
+    BridgeAddress { source: AddressError },
 }
 
 #[allow(clippy::module_name_repetitions)]

crates/astria-sequencer/src/accounts/action.rs

@@ -4,7 +4,6 @@ use anyhow::{
     Result,
 };
 use astria_core::{
-    primitive::v1::ADDRESS_LEN,
     protocol::transaction::v1alpha1::action::TransferAction,
     Protobuf,
 };
@@ -57,11 +56,17 @@ impl ActionHandler for TransferAction {
     }
 }
 
-pub(crate) async fn execute_transfer<S: StateWrite>(
+pub(crate) async fn execute_transfer<S, TAddress>(
     action: &TransferAction,
-    from: [u8; ADDRESS_LEN],
+    from: TAddress,
     mut state: S,
-) -> anyhow::Result<()> {
+) -> anyhow::Result<()>
+where
+    S: StateWrite,
+    TAddress: AddressBytes,
+{
+    let from = from.address_bytes();
+
     let fee = state
         .get_transfer_base_fee()
         .await

crates/astria-sequencer/src/app/tests_breaking_changes.rs

@@ -289,7 +289,7 @@ async fn app_execute_transaction_with_every_action_snapshot() {
                 amount: 10,
                 fee_asset: nria().into(),
                 memo: "{}".into(),
-                bridge_address: None,
+                bridge_address: astria_address(&bridge.address_bytes()),
             }
             .into(),
             BridgeSudoChangeAction {

crates/astria-sequencer/src/app/tests_execute_transaction.rs

@@ -1028,7 +1028,7 @@ async fn app_execute_transaction_bridge_lock_unlock_action_ok() {
         amount,
         fee_asset: nria().into(),
         memo: "{ \"msg\": \"lilywashere\" }".into(),
-        bridge_address: None,
+        bridge_address,
     };
 
     let tx = UnsignedTransaction {

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

@@ -4,12 +4,9 @@ use anyhow::{
     Context as _,
     Result,
 };
-use astria_core::{
-    primitive::v1::Address,
-    protocol::transaction::v1alpha1::action::{
-        BridgeUnlockAction,
-        TransferAction,
-    },
+use astria_core::protocol::transaction::v1alpha1::action::{
+    BridgeUnlockAction,
+    TransferAction,
 };
 use cnidarium::StateWrite;
 
@@ -39,25 +36,19 @@ impl ActionHandler for BridgeUnlockAction {
             .ensure_base_prefix(&self.to)
             .await
             .context("failed check for base prefix of destination address")?;
-        if let Some(bridge_address) = &self.bridge_address {
-            state
-                .ensure_base_prefix(bridge_address)
-                .await
-                .context("failed check for base prefix of bridge address")?;
-        }
-
-        // the bridge address to withdraw funds from
-        // if unset, use the tx sender's address
-        let bridge_address = self.bridge_address.map_or(from, Address::bytes);
+        state
+            .ensure_base_prefix(&self.bridge_address)
+            .await
+            .context("failed check for base prefix of bridge address")?;
 
         let asset = state
-            .get_bridge_account_ibc_asset(bridge_address)
+            .get_bridge_account_ibc_asset(self.bridge_address)
             .await
             .context("failed to get bridge's asset id, must be a bridge account")?;
 
         // check that the sender of this tx is the authorized withdrawer for the bridge account
         let Some(withdrawer_address) = state
-            .get_bridge_account_withdrawer_address(bridge_address)
+            .get_bridge_account_withdrawer_address(self.bridge_address)
             .await
             .context("failed to get bridge account withdrawer address")?
         else {
@@ -76,15 +67,15 @@ impl ActionHandler for BridgeUnlockAction {
             fee_asset: self.fee_asset.clone(),
         };
 
-        check_transfer(&transfer_action, bridge_address, &state).await?;
-        execute_transfer(&transfer_action, bridge_address, state).await?;
+        check_transfer(&transfer_action, self.bridge_address, &state).await?;
+        execute_transfer(&transfer_action, self.bridge_address, state).await?;
 
         Ok(())
     }
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
     use astria_core::{
         primitive::v1::{
             asset,
@@ -115,41 +106,6 @@ mod test {
         "test".parse().unwrap()
     }
 
-    #[tokio::test]
-    async fn fails_if_bridge_address_is_not_set_and_signer_is_not_bridge() {
-        let storage = cnidarium::TempStorage::new().await.unwrap();
-        let snapshot = storage.latest_snapshot();
-        let mut state = StateDelta::new(snapshot);
-
-        state.put_current_source(TransactionContext {
-            address_bytes: [1; 20],
-        });
-        state.put_base_prefix(ASTRIA_PREFIX).unwrap();
-
-        let asset = test_asset();
-        let transfer_amount = 100;
-
-        let to_address = astria_address(&[2; 20]);
-
-        let bridge_unlock = BridgeUnlockAction {
-            to: to_address,
-            amount: transfer_amount,
-            fee_asset: asset,
-            memo: "{}".into(),
-            bridge_address: None,
-        };
-
-        // not a bridge account, should fail
-        assert!(
-            bridge_unlock
-                .check_and_execute(state)
-                .await
-                .unwrap_err()
-                .to_string()
-                .contains("failed to get bridge's asset id, must be a bridge account")
-        );
-    }
-
     #[tokio::test]
     async fn fails_if_bridge_account_has_no_withdrawer_address() {
         let storage = cnidarium::TempStorage::new().await.unwrap();
@@ -166,7 +122,6 @@ mod test {
 
         let to_address = astria_address(&[2; 20]);
         let bridge_address = astria_address(&[3; 20]);
-        // state.put_bridge_account_withdrawer_address(bridge_address, bridge_address);
         state
             .put_bridge_account_ibc_asset(bridge_address, &asset)
             .unwrap();
@@ -176,7 +131,7 @@ mod test {
             amount: transfer_amount,
             fee_asset: asset.clone(),
             memo: "{}".into(),
-            bridge_address: Some(bridge_address),
+            bridge_address,
         };
 
         // invalid sender, doesn't match action's `from`, should fail
@@ -213,7 +168,7 @@ mod test {
             amount: transfer_amount,
             fee_asset: asset,
             memo: "{}".into(),
-            bridge_address: Some(bridge_address),
+            bridge_address,
         };
 
         // invalid sender, doesn't match action's bridge account's withdrawer, should fail
@@ -255,7 +210,7 @@ mod test {
             amount: transfer_amount,
             fee_asset: asset.clone(),
             memo: "{}".into(),
-            bridge_address: None,
+            bridge_address,
         };
 
         // not enough balance; should fail
@@ -309,7 +264,7 @@ mod test {
             amount: transfer_amount,
             fee_asset: asset.clone(),
             memo: "{}".into(),
-            bridge_address: Some(bridge_address),
+            bridge_address,
         };
 
         // not enough balance; should fail

proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto

@@ -211,9 +211,7 @@ message BridgeUnlockAction {
   string fee_asset = 3;
   // memo for double spend prevention
   string memo = 4;
-  // the address of the bridge account to transfer from,
-  // if the bridge account's withdrawer address is not the same as the bridge address.
-  // if unset, the signer of the transaction is used.
+  // the address of the bridge account to transfer from
   astria.primitive.v1.Address bridge_address = 5;
 }