Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed AccountId validation to check for maximum number of bytes #197

Merged
merged 3 commits into from
Apr 19, 2022
Merged

Changed AccountId validation to check for maximum number of bytes #197

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11e627b
Changed AccountId validation to check for minimum number of bytes
jstuczyn Apr 5, 2022
b8f0479
Updated AccountId parsing to more closely follow cosmos-sdk logic
jstuczyn Apr 7, 2022
2695ceb
Applying suggestion to disallow empty addresses
jstuczyn Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cosmrs/src/base.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ impl FromStr for AccountId {
fn from_str(s: &str) -> Result<Self> {
let (hrp, bytes) = bech32::decode(s).wrap_err("failed to decode bech32")?;

if bytes.len() == tendermint::account::LENGTH {
if bytes.len() >= tendermint::account::LENGTH {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this should perhaps check inclusion within a set of allowed lengths?

Suggested change
if bytes.len() >= tendermint::account::LENGTH {
if matches!(bytes.len(), tendermint::account::LENGTH | 32) {

(seems like perhaps there should be a new upstream constant for 32-byte lengths, but we can cross that bridge when we get there)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I guess the decision is ultimately up to you. I was inspired by the associated error message, i.e. account ID should be at least {} bytes long, but was {} bytes long implying that the check should be performed against a range rather than against a set of allowed values. Let me know which one you prefer and I'll update the PR accordingly!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, I just looked at cosmos-sdk code directly and it looks like for their AccAddress (which I think the AccountId corresponds to?) they only length check they perform is whether the bytes decoded from the bech32 representation are shorter than 255 (and obviously whether they're not empty) (https://github.com/cosmos/cosmos-sdk/blob/f133317cfe8b4723ccb5e3687ffc9ab71b52ea75/types/address.go#L148-L150)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Looks like that was changed in cosmos/cosmos-sdk#8363.

The changelog includes the following comment:

Addresses no longer have a fixed 20-byte length. From the SDK modules' point of view, any 1-255 bytes-long byte array is a valid address.

...so I guess we should match that logic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR accordingly. Presumably new and to_bytes would also need to be changed, however, those would be incompatible breaking changes and thus I didn't want to touch them now : )

Ok(Self {
bech32: s.to_owned(),
hrp_length: hrp.len(),
Expand Down