refactor: delegate keypair encoding to cosmrs #2887
Conversation
| (_, Ok(v)) => ism_type_to_module_type(v.typ), | ||
| _ => ModuleType::Null, | ||
| }) | ||
| let module_type_response = |
| let account_id = AccountId::from_str(&addr)?; | ||
|
|
||
| // although `from_slice` can panic if the slice is not 32 bytes long, | ||
| // we know that we're passing in a value that is 32 bytes long because it was decoded from |
There was a problem hiding this comment.
I believe bech32 is named that because it's the encoding is done in base 32 and not because it's 32 bytes long. I think the pattern in Cosmos is that EOA addresses are 20 bytes and smart contracts are 32 bytes. So we should probably be more defensive here
Looks like account IDs can get up to 255 bytes https://docs.rs/cosmrs/latest/cosmrs/struct.AccountId.html#associatedconstant.MAX_LENGTH
There was a problem hiding this comment.
unit tests broke this, good point
| /// Wrapper around the cosmrs AccountId type that abstracts keypair conversions and | ||
| /// bech32 encoding | ||
| #[derive(new)] | ||
| pub struct CosmosAddress { |
There was a problem hiding this comment.
may be a good place to have some quick unit tests if we're uncertain about the encoding / decoding like iiuc you flag in the PR description
| @@ -1,28 +0,0 @@ | |||
| use hyperlane_core::{H160, H256, H512}; | |||
| @@ -1,132 +0,0 @@ | |||
| use std::cmp; | |||
| @@ -78,7 +77,7 @@ impl MultisigIsm for CosmosMultisigIsm { | |||
| let validators: ChainResult<Vec<H256>> = response | |||
| .validators | |||
| .iter() | |||
| .map(|v| H160::from_str(v).map(h160_to_h256).map_err(Into::into)) | |||
| .map(|v| H256::from_str(v).map_err(Into::into)) | |||
There was a problem hiding this comment.
I believe H256::from_str panics if the input length isn't 32 bytes, so we'll probably need to keep doing H160::from_str(v) and then .into() to H256
|
Fixed the address generation, just need to add a couple more unit tests |
| @@ -1,3 +1,7 @@ | |||
| // TODO: this file can be removed if `CosmosAddress` can be imported from `hyperlane-cosmos`. | |||
There was a problem hiding this comment.
I assume this is what you're currently looking into?
There was a problem hiding this comment.
actually no but I'll look into this as well
daniel-savu
merged commit 2a7d6d6
into
trevor/new-featv3-cosmos-oct-28
Removes cosmos signer unwraps by making the signer optional. Depends on #2887
Description
Delegates bech32 encoding, decoding, and cosmos address building from a private key to cosmrs/tendermint upstream crates. The only place where this wasn't doable was the tests, because of a cyclic dependency - left a TODO comment there.
As part of this, renamed
rust/chains/hyperlane-cosmos/src/libs/verify.rs->rust/chains/hyperlane-cosmos/src/libs/address.rsWarning: We should re-enable e2e and make sure everything still works locally, since the actualencode/decodefunctions that end up being called are differentTesting
Tested manually EVM <> Duality (both ways)