chore(core, sequencer): move stateless checks to domain type parsing

crates/astria-core/Cargo.toml

@@ -37,6 +37,7 @@ penumbra-proto = { workspace = true }
 prost = { workspace = true }
 rand = { workspace = true }
 serde = { workspace = true, features = ["derive"], optional = true }
+serde_json = { workspace = true }
 sha2 = { workspace = true }
 tendermint = { workspace = true }
 tendermint-proto = { workspace = true }

crates/astria-core/src/protocol/transaction/v1/action/mod.rs

@@ -21,22 +21,25 @@ use crate::{
         IncorrectRollupIdLength,
         RollupId,
     },
-    protocol::fees::v1::{
-        BridgeLockFeeComponents,
-        BridgeSudoChangeFeeComponents,
-        BridgeUnlockFeeComponents,
-        FeeAssetChangeFeeComponents,
-        FeeChangeFeeComponents,
-        FeeComponentError,
-        IbcRelayFeeComponents,
-        IbcRelayerChangeFeeComponents,
-        IbcSudoChangeFeeComponents,
-        Ics20WithdrawalFeeComponents,
-        InitBridgeAccountFeeComponents,
-        RollupDataSubmissionFeeComponents,
-        SudoAddressChangeFeeComponents,
-        TransferFeeComponents,
-        ValidatorUpdateFeeComponents,
+    protocol::{
+        fees::v1::{
+            BridgeLockFeeComponents,
+            BridgeSudoChangeFeeComponents,
+            BridgeUnlockFeeComponents,
+            FeeAssetChangeFeeComponents,
+            FeeChangeFeeComponents,
+            FeeComponentError,
+            IbcRelayFeeComponents,
+            IbcRelayerChangeFeeComponents,
+            IbcSudoChangeFeeComponents,
+            Ics20WithdrawalFeeComponents,
+            InitBridgeAccountFeeComponents,
+            RollupDataSubmissionFeeComponents,
+            SudoAddressChangeFeeComponents,
+            TransferFeeComponents,
+            ValidatorUpdateFeeComponents,
+        },
+        memos::v1::Ics20WithdrawalFromRollup,
     },
     Protobuf,
 };
@@ -1032,12 +1035,19 @@ impl Protobuf for Ics20Withdrawal {
             bridge_address,
             use_compat_address,
         } = proto;
-        let amount = amount.ok_or(Ics20WithdrawalError::field_not_set("amount"))?;
+        let amount = u128::from(amount.ok_or(Ics20WithdrawalError::field_not_set("amount"))?);
         let return_address = Address::try_from_raw(
             &return_address.ok_or(Ics20WithdrawalError::field_not_set("return_address"))?,
         )
         .map_err(Ics20WithdrawalError::return_address)?;
 
+        if timeout_time == 0 {
+            return Err(Ics20WithdrawalError::invalid_timeout_time());
+        }
+        if amount == 0 {
+            return Err(Ics20WithdrawalError::invalid_amount());
+        }
+
         let timeout_height = timeout_height
             .ok_or(Ics20WithdrawalError::field_not_set("timeout_height"))?
             .into();
@@ -1047,8 +1057,20 @@ impl Protobuf for Ics20Withdrawal {
             .transpose()
             .map_err(Ics20WithdrawalError::invalid_bridge_address)?;
 
+        if bridge_address.is_some() {
+            let parsed_withdrawal: Ics20WithdrawalFromRollup =
+                serde_json::from_str(&memo).map_err(Ics20WithdrawalError::parse_memo)?;
+            validate_rollup_return_address(&parsed_withdrawal.rollup_return_address)
+                .map_err(Ics20WithdrawalError::rollup_withdrawal)?;
+            validate_withdrawal_event_id_and_block_number(
+                &parsed_withdrawal.rollup_withdrawal_event_id,
+                parsed_withdrawal.rollup_block_number,
+            )
+            .map_err(Ics20WithdrawalError::rollup_withdrawal)?;
+        }
+
         Ok(Self {
-            amount: amount.into(),
+            amount,
             denom: denom.parse().map_err(Ics20WithdrawalError::invalid_denom)?,
             destination_chain_address,
             return_address,
@@ -1089,14 +1111,21 @@ impl Protobuf for Ics20Withdrawal {
             bridge_address,
             use_compat_address,
         } = proto;
-        let amount = amount.ok_or(Ics20WithdrawalError::field_not_set("amount"))?;
+        let amount = u128::from(amount.ok_or(Ics20WithdrawalError::field_not_set("amount"))?);
         let return_address = Address::try_from_raw(
             return_address
                 .as_ref()
                 .ok_or(Ics20WithdrawalError::field_not_set("return_address"))?,
         )
         .map_err(Ics20WithdrawalError::return_address)?;
 
+        if *timeout_time == 0 {
+            return Err(Ics20WithdrawalError::invalid_timeout_time());
+        }
+        if amount == 0 {
+            return Err(Ics20WithdrawalError::invalid_amount());
+        }
+
         let timeout_height = timeout_height
             .clone()
             .ok_or(Ics20WithdrawalError::field_not_set("timeout_height"))?
@@ -1107,8 +1136,20 @@ impl Protobuf for Ics20Withdrawal {
             .transpose()
             .map_err(Ics20WithdrawalError::invalid_bridge_address)?;
 
+        if bridge_address.is_some() {
+            let parsed_withdrawal: Ics20WithdrawalFromRollup =
+                serde_json::from_str(memo).map_err(Ics20WithdrawalError::parse_memo)?;
+            validate_rollup_return_address(&parsed_withdrawal.rollup_return_address)
+                .map_err(Ics20WithdrawalError::rollup_withdrawal)?;
+            validate_withdrawal_event_id_and_block_number(
+                &parsed_withdrawal.rollup_withdrawal_event_id,
+                parsed_withdrawal.rollup_block_number,
+            )
+            .map_err(Ics20WithdrawalError::rollup_withdrawal)?;
+        }
+
         Ok(Self {
-            amount: amount.into(),
+            amount,
             denom: denom.parse().map_err(Ics20WithdrawalError::invalid_denom)?,
             destination_chain_address: destination_chain_address.clone(),
             return_address,
@@ -1189,11 +1230,32 @@ impl Ics20WithdrawalError {
         Self(Ics20WithdrawalErrorKind::InvalidBridgeAddress(err))
     }
 
+    #[must_use]
     fn invalid_denom(source: asset::ParseDenomError) -> Self {
         Self(Ics20WithdrawalErrorKind::InvalidDenom {
             source,
         })
     }
+
+    #[must_use]
+    fn invalid_timeout_time() -> Self {
+        Self(Ics20WithdrawalErrorKind::InvalidTimeoutTime)
+    }
+
+    #[must_use]
+    fn invalid_amount() -> Self {
+        Self(Ics20WithdrawalErrorKind::InvalidAmount)
+    }
+
+    #[must_use]
+    fn parse_memo(err: serde_json::Error) -> Self {
+        Self(Ics20WithdrawalErrorKind::ParseMemo(err))
+    }
+
+    #[must_use]
+    fn rollup_withdrawal(err: RollupWithdrawalError) -> Self {
+        Self(Ics20WithdrawalErrorKind::RollupWithdrawal(err))
+    }
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -1210,6 +1272,14 @@ enum Ics20WithdrawalErrorKind {
     InvalidBridgeAddress(#[source] AddressError),
     #[error("`denom` field was invalid")]
     InvalidDenom { source: asset::ParseDenomError },
+    #[error("`timeout_time` must be non-zero")]
+    InvalidTimeoutTime,
+    #[error("`amount` must be greater than zero")]
+    InvalidAmount,
+    #[error("failed to parse memo for ICS bound bridge withdrawal")]
+    ParseMemo(#[source] serde_json::Error),
+    #[error("rollup withdrawal information was invalid")]
+    RollupWithdrawal(#[source] RollupWithdrawalError),
 }
 
 #[derive(Debug, Clone)]
@@ -1705,18 +1775,30 @@ impl Protobuf for BridgeUnlock {
         let to = to
             .ok_or_else(|| BridgeUnlockError::field_not_set("to"))
             .and_then(|to| Address::try_from_raw(&to).map_err(BridgeUnlockError::address))?;
-        let amount = amount.ok_or_else(|| BridgeUnlockError::field_not_set("amount"))?;
+        let amount = u128::from(amount.ok_or_else(|| BridgeUnlockError::field_not_set("amount"))?);
+        if amount == 0 {
+            return Err(BridgeUnlockError::invalid_amount());
+        }
+        if memo.len() > 64 {
+            return Err(BridgeUnlockError::invalid_memo());
+        }
         let fee_asset = fee_asset.parse().map_err(BridgeUnlockError::fee_asset)?;
 
+        validate_withdrawal_event_id_and_block_number(
+            &rollup_withdrawal_event_id,
+            rollup_block_number,
+        )
+        .map_err(BridgeUnlockError::rollup_withdrawal)?;
+
         let bridge_address = bridge_address
             .ok_or_else(|| BridgeUnlockError::field_not_set("bridge_address"))
             .and_then(|to| Address::try_from_raw(&to).map_err(BridgeUnlockError::bridge_address))?;
         Ok(Self {
             to,
-            amount: amount.into(),
+            amount,
             fee_asset,
-            memo,
             bridge_address,
+            memo,
             rollup_block_number,
             rollup_withdrawal_event_id,
         })
@@ -1766,6 +1848,21 @@ impl BridgeUnlockError {
             source,
         })
     }
+
+    #[must_use]
+    fn invalid_amount() -> Self {
+        Self(BridgeUnlockErrorKind::InvalidAmount)
+    }
+
+    #[must_use]
+    fn invalid_memo() -> Self {
+        Self(BridgeUnlockErrorKind::InvalidMemo)
+    }
+
+    #[must_use]
+    fn rollup_withdrawal(err: RollupWithdrawalError) -> Self {
+        Self(BridgeUnlockErrorKind::RollupWithdrawal(err))
+    }
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -1778,6 +1875,12 @@ enum BridgeUnlockErrorKind {
     FeeAsset { source: asset::ParseDenomError },
     #[error("the `bridge_address` field was invalid")]
     BridgeAddress { source: AddressError },
+    #[error("`amount` must be greater than zero")]
+    InvalidAmount,
+    #[error("`memo` muse be no longer than 64 bytes")]
+    InvalidMemo,
+    #[error("rollup withdrawal information was invalid")]
+    RollupWithdrawal(#[source] RollupWithdrawalError),
 }
 
 #[derive(Debug, Clone)]
@@ -2076,3 +2179,41 @@ impl Protobuf for FeeChange {
         })
     }
 }
+
+#[derive(Debug, thiserror::Error)]
+enum RollupWithdrawalError {
+    #[error("rollup return address must be non-empty")]
+    EmptyRollupReturnAddress,
+    #[error("rollup return address must be no more than 256 bytes")]
+    InvalidRollupReturnAddress,
+    #[error("rollup withdrawal event id must be non-empty")]
+    EmptyRollupEventId,
+    #[error("rollup withdrawal event id must be no more than 256 bytes")]
+    InvalidRollupEventId,
+    #[error("rollup block number must be non-zero")]
+    InvalidRollupBlockNumber,
+}
+
+fn validate_rollup_return_address(address: &str) -> Result<(), RollupWithdrawalError> {
+    if address.is_empty() {
+        return Err(RollupWithdrawalError::EmptyRollupReturnAddress);
+    } else if address.len() > 256 {
+        return Err(RollupWithdrawalError::InvalidRollupReturnAddress);
+    }
+    Ok(())
+}
+
+fn validate_withdrawal_event_id_and_block_number(
+    id: &str,
+    block: u64,
+) -> Result<(), RollupWithdrawalError> {
+    if id.is_empty() {
+        return Err(RollupWithdrawalError::EmptyRollupEventId);
+    } else if id.len() > 256 {
+        return Err(RollupWithdrawalError::InvalidRollupEventId);
+    }
+    if block == 0 {
+        return Err(RollupWithdrawalError::InvalidRollupBlockNumber);
+    }
+    Ok(())
+}

crates/astria-sequencer-client/Cargo.toml

@@ -39,3 +39,4 @@ serde_json = { workspace = true }
 tokio = { workspace = true }
 tokio-test = { workspace = true }
 wiremock = { workspace = true }
+astria-core = { path = "../astria-core", features = ["serde"] }

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

@@ -26,22 +26,7 @@ use crate::{
 
 #[async_trait::async_trait]
 impl ActionHandler for BridgeUnlock {
-    // 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() <= 256,
-            "rollup withdrawal event id must not be more than 256 bytes",
-        );
-        ensure!(
-            self.rollup_block_number > 0,
-            "rollup block number must be greater than zero",
-        );
         Ok(())
     }
 

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

@@ -143,36 +143,7 @@ async fn establish_withdrawal_target<'a, S: StateRead>(
 
 #[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() <= 256,
-                "rollup withdrawal event id must be no more than 256 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
         // use bech32 addresses.