chore(core, sequencer): move stateless checks to domain type parsing #1770
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
| }, | ||
| memos::v1::Ics20WithdrawalFromRollup, | ||
| }, | ||
| Protobuf, | ||
| }; | ||
|
|
@@ -1032,12 +1035,19 @@ impl Protobuf for Ics20Withdrawal { | |
| bridge_address, | ||
| use_compat_address, | ||
| } = proto; | ||
| let amount = u128::from(amount.ok_or(Ics20WithdrawalError::field_not_set("amount"))?); | ||
ethanoroshiba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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 { | ||
ethanoroshiba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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)?; | ||
|
Comment on lines
+1123
to
+1124
There was a problem hiding this comment. Should we edit our domain type There was a problem hiding this comment. Yes, absolutely. Easiest is probably to just make You should also lean on a There was a problem hiding this comment. Split into enum in 84b00ef. I wanted to ask further before continuing with your second suggestion: in order to do this we would need to implement a new
|
||
| 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, | ||
| 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 = 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() { | ||
|
There was a problem hiding this comment. I am pretty unhappy that we now have a JSON formatted payload in here. I wonder if we can make a breaking change to these transactions and add another variant to the protobuf that makes this a protobuf. There was a problem hiding this comment. Considering this would be network breaking, I'm not sure what we can really do about it now, barring a hard fork :/ We could make a new action which uses a protobuf-formatted memo instead and follow the same upgrade path as the new |
||
| 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, | ||
| 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, | ||
ethanoroshiba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #[error("`amount` must be greater than zero")] | ||
| InvalidAmount, | ||
ethanoroshiba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #[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 = 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, | ||
| fee_asset, | ||
| 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(()) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this change force us to always activate serde for all of
astria-core? After all, the memo serde impls are feature gated I think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to address this in 84b00ef. Let me know if this is what you had in mind!