feat: enforce length constraints on destination_chain_address #1977
Conversation
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
varun-doshi
force-pushed
the
varun/dest-addr
branch
from
398a63c to
2f5d0ae
Compare
joroshiba
requested changes
Feb 20, 2025
| @@ -1557,6 +1539,8 @@ impl Protobuf for BridgeLock { | |||
| /// - if the `to` field is invalid | |||
| /// - if the `asset` field is invalid | |||
| /// - if the `fee_asset` field is invalid | |||
| /// /// - if `destination_chain_address` is not set | |||
| @@ -1988,6 +1989,9 @@ impl Protobuf for BridgeTransfer { | |||
| "destination_chain_address", | |||
| )); | |||
| } | |||
| if destination_chain_address.as_bytes().len() != 256 { | |||
| @@ -1638,6 +1636,8 @@ enum BridgeLockErrorKind { | |||
| InvalidAsset(#[source] asset::ParseDenomError), | |||
| #[error("the `fee_asset` field was invalid")] | |||
| InvalidFeeAsset(#[source] asset::ParseDenomError), | |||
| #[error("the `destination_chain_address` length is not 256")] | |||
There was a problem hiding this comment.
Re shouldn't be exactly update language here as well
There was a problem hiding this comment.
Would this be the correct error message?
the destination_chain_address length is 0 or greater than 256
| "destination_chain_address", | ||
| )); | ||
| } | ||
| if proto.destination_chain_address.as_bytes().len() != 256 { |
There was a problem hiding this comment.
I think 256 bytes is a reasonable upper limit, but we shouldn't restrict to an exact meassure.
| @@ -2059,6 +2068,8 @@ enum BridgeTransferErrorKind { | |||
| FeeAsset { source: asset::ParseDenomError }, | |||
| #[error("the `bridge_address` field was invalid")] | |||
| BridgeAddress { source: AddressError }, | |||
| #[error("the `destination_chain_address` length is not 256")] | |||
| channel::ChannelId, | ||
| client::Height as IbcHeight, | ||
| }, | ||
| core::{channel::ChannelId, client::Height as IbcHeight}, |
There was a problem hiding this comment.
general throughout here, please don't override formatting.
If you have just installed can run just fmt rust to format according to repo standards.
noot
reviewed
Feb 21, 2025
| @@ -1556,7 +1556,8 @@ impl Protobuf for BridgeLock { | |||
| /// - if the `to` field is not set | |||
| /// - if the `to` field is invalid | |||
| /// - if the `asset` field is invalid | |||
| /// - if the `fee_asset` field is invalid | |||
| /// - if the `fee_asset` field is invalid /// - if `destination_chain_address` is not set | |||
There was a problem hiding this comment.
Suggested change
| /// - if the `fee_asset` field is invalid /// - if `destination_chain_address` is not set | |
| /// - if the `fee_asset` field is invalid | |
| /// - if `destination_chain_address` is not set |
noot
reviewed
Feb 21, 2025
| @@ -1556,7 +1556,8 @@ impl Protobuf for BridgeLock { | |||
| /// - if the `to` field is not set | |||
| /// - if the `to` field is invalid | |||
| /// - if the `asset` field is invalid | |||
| /// - if the `fee_asset` field is invalid | |||
| /// - if the `fee_asset` field is invalid /// - if `destination_chain_address` is not set | |||
| /// - if the `destination_chain_address` is not 256 bytes long | |||
There was a problem hiding this comment.
Suggested change
| /// - if the `destination_chain_address` is not 256 bytes long | |
| /// - if the `destination_chain_address` is over 256 bytes long |
noot
reviewed
Feb 21, 2025
| @@ -1968,6 +1983,7 @@ impl Protobuf for BridgeTransfer { | |||
| /// - if the `fee_asset` field is invalid | |||
| /// - if the `from` field is invalid | |||
| /// - if `destination_chain_address` is not set | |||
| /// - if the `destination_chain_address` is not 256 bytes long | |||
There was a problem hiding this comment.
Suggested change
| /// - if the `destination_chain_address` is not 256 bytes long | |
| /// - if the `destination_chain_address` is over 256 bytes long |
noot
reviewed
Feb 21, 2025
Comment on lines
2007
to
2008
| if destination_chain_address.as_bytes().len() > 0 | ||
| && destination_chain_address.as_bytes().len() <= 256 |
There was a problem hiding this comment.
Suggested change
| if destination_chain_address.as_bytes().len() > 0 | |
| && destination_chain_address.as_bytes().len() <= 256 | |
| if destination_chain_address.as_bytes().is_empty() || destination_chain_address.as_bytes().len() > 256 |
ideally these will be two separate errors actually
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Enforces length constraints on
destination_chain_addressin BridgeTransfer and BridgeLockBackground
Invalid
destination_chain_addresscould have been passed before as there as no checkChanges
In `crates/astria-core/src/protocol/transaction/v1/mod.rs
Changelogs
Changelogs updated
Related Issues
#1961
closes #1961