Changed AccountId validation to check for maximum number of bytes #197
Conversation
cosmrs/src/base.rs
Outdated
| @@ -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 { | |||
There was a problem hiding this comment.
Seems like this should perhaps check inclusion within a set of allowed lengths?
| 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)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 : )
jstuczyn
changed the title
cosmrs/src/base.rs
Outdated
| if bytes.len() == tendermint::account::LENGTH { | ||
| if bytes.len() <= MAX_ADDRESS_LENGTH { |
There was a problem hiding this comment.
It looks like this allows empty addresses. How about:
| if bytes.len() == tendermint::account::LENGTH { | |
| if bytes.len() <= MAX_ADDRESS_LENGTH { | |
| if matches!(bytes.len(), 1..=MAX_ADDRESS_LENGTH) { |
So that it would be compatible with the new 32-byte long addresses