Skip to content

Commit

Permalink
refactor: finish removing unwraps (#2881)
Browse files Browse the repository at this point in the history
### Description

- Removes unwraps and expects from everywhere but `run-locally`
- Precomputes the `address` in `Signer` at construction time, to
propagate the error early and keep signatures ergonomic by not requiring
a `Result`. Couldn't also precompile `SigningKey` (the privkey type)
because it's not `Sync` 😢
- Defines a `hyperlane-cosmos`-specific error enum
(`HyperlaneCosmosError`), which can be converted to
`ChainCommunicationError` with the `?` operator. This is a pattern we'd
like to refactor towards in the future, to remove dependencies from
`hyperlane-core` as much as possible
- One inconvenience is that you need to `.map_err()` to
`HyperlaneCosmosError` first, to use `?`. I wish `?` had deref coercion
semantics where it'd keep covnerting until an error matches, but I
assume this isn't possible because while you can only have a single
`Deref` impl, you can have multiple `From<Err>` impls.
- Writing this I'm realising we could write a small macro to implement
`From<Err> for ChainCommunicationError` for all the sub-errors of
`HyperlaneCosmosError` et al (cc @tkporter)
  • Loading branch information
daniel-savu authored and yorhodes committed Nov 2, 2023
1 parent 35dcc7d commit 7289e0f
Show file tree
Hide file tree
Showing 14 changed files with 104 additions and 56 deletions.
5 changes: 3 additions & 2 deletions rust/chains/hyperlane-cosmos/src/aggregation_ism.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ impl AggregationIsm for CosmosAggregationIsm {
let data = self.provider.wasm_query(payload, None).await?;
let response: ModulesAndThresholdResponse = serde_json::from_slice(&data)?;

let modules: Vec<H256> = response.modules.into_iter().map(bech32_decode).collect();
let modules: ChainResult<Vec<H256>> =
response.modules.into_iter().map(bech32_decode).collect();

Ok((modules, response.threshold))
Ok((modules?, response.threshold))
}
}
21 changes: 21 additions & 0 deletions rust/chains/hyperlane-cosmos/src/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
use hyperlane_core::ChainCommunicationError;

/// Errors from the crates specific to the hyperlane-cosmos
/// implementation.
/// This error can then be converted into the broader error type
/// in hyperlane-core using the `From` trait impl
#[derive(Debug, thiserror::Error)]
pub enum HyperlaneCosmosError {
/// bech32 error
#[error("{0}")]
Bech32(#[from] bech32::Error),
/// gRPC error
#[error("{0}")]
GrpcError(#[from] tonic::Status),
}

impl From<HyperlaneCosmosError> for ChainCommunicationError {
fn from(value: HyperlaneCosmosError) -> Self {
ChainCommunicationError::from_other(value)
}
}
8 changes: 3 additions & 5 deletions rust/chains/hyperlane-cosmos/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#![allow(unused_variables)]

mod aggregation_ism;
mod error;
mod interchain_gas;
mod interchain_security_module;
mod libs;
Expand All @@ -21,10 +22,7 @@ mod utils;
mod validator_announce;

pub use self::{
aggregation_ism::*, interchain_gas::*, interchain_security_module::*, libs::*, mailbox::*,
merkle_tree_hook::*, multisig_ism::*, providers::*, routing_ism::*, signers::*,
aggregation_ism::*, error::*, interchain_gas::*, interchain_security_module::*, libs::*,
mailbox::*, merkle_tree_hook::*, multisig_ism::*, providers::*, routing_ism::*, signers::*,
trait_builder::*, trait_builder::*, validator_announce::*, validator_announce::*,
};

/// Safe default imports of commonly used traits/types.
pub mod prelude {}
17 changes: 9 additions & 8 deletions rust/chains/hyperlane-cosmos/src/libs/verify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,20 @@ use hyperlane_core::{ChainCommunicationError, ChainResult, H160, H256};
use ripemd::Ripemd160;
use sha2::{Digest, Sha256};

use crate::HyperlaneCosmosError;

/// decode bech32 address to H256
pub fn bech32_decode(addr: String) -> H256 {
let (_hrp, data, _variant) = bech32::decode(addr.as_str()).unwrap();
pub fn bech32_decode(addr: String) -> ChainResult<H256> {
let (_hrp, data, _variant) =
bech32::decode(addr.as_str()).map_err(Into::<HyperlaneCosmosError>::into)?;

let value = Vec::<u8>::from_base32(&data).unwrap();
let value = Vec::<u8>::from_base32(&data).map_err(Into::<HyperlaneCosmosError>::into)?;
let mut result: [u8; 32] = [0; 32];

let start_point = cmp::max(0, 32 - value.len());
result[start_point..32].copy_from_slice(value.as_slice());

H256::from(result)
Ok(H256::from(result))
}

/// encode H256 to bech32 address
Expand Down Expand Up @@ -81,8 +84,7 @@ pub fn pub_to_addr(pub_key: Vec<u8>, prefix: &str) -> ChainResult<String> {
/// encode H256 to bech32 address
pub fn priv_to_binary_addr(priv_key: Vec<u8>) -> ChainResult<H160> {
let sha_hash = sha256_digest(
SigningKey::from_slice(priv_key.as_slice())
.unwrap()
SigningKey::from_slice(priv_key.as_slice())?
.public_key()
.to_bytes(),
)?;
Expand All @@ -94,8 +96,7 @@ pub fn priv_to_binary_addr(priv_key: Vec<u8>) -> ChainResult<H160> {
/// encode H256 to bech32 address
pub fn priv_to_addr_string(prefix: String, priv_key: Vec<u8>) -> ChainResult<String> {
let sha_hash = sha256_digest(
SigningKey::from_slice(priv_key.as_slice())
.unwrap()
SigningKey::from_slice(priv_key.as_slice())?
.public_key()
.to_bytes(),
)?;
Expand Down
2 changes: 1 addition & 1 deletion rust/chains/hyperlane-cosmos/src/mailbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ impl Mailbox for CosmosMailbox {
let response: mailbox::RecipientIsmResponse = serde_json::from_slice(&data)?;

// convert Hex to H256
let ism = verify::bech32_decode(response.ism);
let ism = verify::bech32_decode(response.ism)?;
Ok(ism)
}

Expand Down
17 changes: 8 additions & 9 deletions rust/chains/hyperlane-cosmos/src/merkle_tree_hook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use async_trait::async_trait;
use base64::Engine;
use cosmrs::tendermint::abci::EventAttribute;
use hyperlane_core::{
accumulator::incremental::IncrementalMerkle, ChainResult, Checkpoint, ContractLocator,
HyperlaneChain, HyperlaneContract, HyperlaneDomain, HyperlaneProvider, Indexer, LogMeta,
MerkleTreeHook, MerkleTreeInsertion, SequenceIndexer, H256,
accumulator::incremental::IncrementalMerkle, ChainCommunicationError, ChainResult, Checkpoint,
ContractLocator, HyperlaneChain, HyperlaneContract, HyperlaneDomain, HyperlaneProvider,
Indexer, LogMeta, MerkleTreeHook, MerkleTreeInsertion, SequenceIndexer, H256,
};
use tracing::instrument;

Expand Down Expand Up @@ -94,12 +94,11 @@ impl MerkleTreeHook for CosmosMerkleTreeHook {
.iter()
.map(|s| s.as_str())
.map(H256::from_str)
.collect::<Result<Vec<H256>, _>>()
.expect("fail to parse tree branch");
.collect::<Result<Vec<H256>, _>>()?;

let branch_res: [H256; 32] = branch
.try_into()
.expect("fail to convert tree branch to array");
let branch_res: [H256; 32] = branch.try_into().map_err(|_| {
ChainCommunicationError::from_other_str("Failed to build merkle branch array")
})?;

Ok(IncrementalMerkle::new(branch_res, response.count as usize))
}
Expand Down Expand Up @@ -149,7 +148,7 @@ impl MerkleTreeHook for CosmosMerkleTreeHook {
Ok(Checkpoint {
merkle_tree_hook_address: self.address,
mailbox_domain: self.domain.id(),
root: response.root.parse().unwrap(),
root: response.root.parse()?,
index: response.count,
})
}
Expand Down
6 changes: 3 additions & 3 deletions rust/chains/hyperlane-cosmos/src/multisig_ism.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,12 @@ impl MultisigIsm for CosmosMultisigIsm {
.await?;
let response: multisig_ism::VerifyInfoResponse = serde_json::from_slice(&data)?;

let validators: Vec<H256> = response
let validators: ChainResult<Vec<H256>> = response
.validators
.iter()
.map(|v| h160_to_h256(H160::from_str(v).unwrap()))
.map(|v| H160::from_str(v).map(h160_to_h256).map_err(Into::into))
.collect();

Ok((validators, response.threshold))
Ok((validators?, response.threshold))
}
}
16 changes: 8 additions & 8 deletions rust/chains/hyperlane-cosmos/src/providers/grpc.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use async_trait::async_trait;
use cosmrs::crypto::secp256k1::SigningKey;
use cosmrs::proto::cosmos::auth::v1beta1::BaseAccount;
use cosmrs::proto::cosmos::auth::v1beta1::{
query_client::QueryClient as QueryAccountClient, QueryAccountRequest,
Expand All @@ -25,8 +24,8 @@ use hyperlane_core::{
};
use serde::Serialize;

use crate::verify;
use crate::{signers::Signer, ConnectionConf};
use crate::{verify, HyperlaneCosmosError};

const DEFAULT_GAS_PRICE: f32 = 0.05;
const DEFAULT_GAS_ADJUSTMENT: f32 = 1.25;
Expand Down Expand Up @@ -218,13 +217,14 @@ impl WasmProvider for WasmGrpcProvider {
where
I: IntoIterator<Item = cosmrs::Any> + Send + Sync,
{
let account_info = self.account_query(self.signer.address()).await?;
let account_info = self.account_query(self.signer.address.clone()).await?;

let private_key = SigningKey::from_slice(&self.signer.private_key).unwrap();
let private_key = self.signer.signing_key()?;
let public_key = private_key.public_key();

let tx_body = tx::Body::new(msgs, "", 9000000u32);
let signer_info = SignerInfo::single_direct(Some(public_key), account_info.sequence);
let signer_info =
SignerInfo::single_direct(Some(self.signer.public_key), account_info.sequence);

let gas_limit: u64 = gas_limit.unwrap_or(U256::from(300000u64)).as_u64();

Expand Down Expand Up @@ -256,7 +256,7 @@ impl WasmProvider for WasmGrpcProvider {
let mut client = TxServiceClient::connect(self.get_conn_url()?).await?;

let msgs = vec![MsgExecuteContract {
sender: self.signer.address(),
sender: self.signer.address.clone(),
contract: self.get_contract_addr()?,
msg: serde_json::to_string(&payload)?.as_bytes().to_vec(),
funds: vec![],
Expand All @@ -272,7 +272,7 @@ impl WasmProvider for WasmGrpcProvider {
let tx_res = client
.broadcast_tx(tx_req)
.await
.unwrap()
.map_err(Into::<HyperlaneCosmosError>::into)?
.into_inner()
.tx_response
.ok_or_else(|| ChainCommunicationError::from_other_str("Empty tx_response"))?;
Expand All @@ -288,7 +288,7 @@ impl WasmProvider for WasmGrpcProvider {
T: Serialize + Send + Sync,
{
let msg = MsgExecuteContract {
sender: self.signer.address(),
sender: self.signer.address.clone(),
contract: self.get_contract_addr()?,
msg: serde_json::to_string(&payload)?.as_bytes().to_vec(),
funds: vec![],
Expand Down
4 changes: 2 additions & 2 deletions rust/chains/hyperlane-cosmos/src/providers/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl WasmIndexer for CosmosWasmIndexer {
let target_type = format!("{}-{}", Self::WASM_TYPE, self.event_type);

// Get BlockHash from block_search
let client = self.get_client().unwrap();
let client = self.get_client()?;

for tx in txs {
if tx.tx_result.code.is_err() {
Expand All @@ -139,7 +139,7 @@ impl WasmIndexer for CosmosWasmIndexer {
if event.kind.as_str() == target_type {
if let Some(msg) = parser(event.attributes.clone())? {
let meta = LogMeta {
address: bech32_decode(contract_address.clone()),
address: bech32_decode(contract_address.clone())?,
block_number: tx.height.value(),
// FIXME: block_hash is not available in tx_search
block_hash: H256::zero(),
Expand Down
2 changes: 1 addition & 1 deletion rust/chains/hyperlane-cosmos/src/routing_ism.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,6 @@ impl RoutingIsm for CosmosRoutingIsm {
.await?;
let response: IsmRouteRespnose = serde_json::from_slice(&data)?;

Ok(bech32_decode(response.ism))
Ok(bech32_decode(response.ism)?)
}
}
52 changes: 39 additions & 13 deletions rust/chains/hyperlane-cosmos/src/signers.rs
Original file line number Diff line number Diff line change
@@ -1,33 +1,59 @@
use cosmrs::crypto::secp256k1::SigningKey;
use cosmrs::crypto::{secp256k1::SigningKey, PublicKey};
use hyperlane_core::ChainResult;

use crate::verify;

#[derive(Clone, Debug)]
/// Signer for cosmos chain
pub struct Signer {
/// prefix for signer address
/// public key
pub public_key: PublicKey,
/// precomputed address, because computing it is a fallible operation
/// and we want to avoid returning `Result`
pub address: String,
/// address prefix
pub prefix: String,
pub(crate) private_key: Vec<u8>,
private_key: Vec<u8>,
}

impl Signer {
/// create new signer
pub fn new(private_key: Vec<u8>, prefix: String) -> Self {
Self {
///
/// # Arguments
/// * `private_key` - private key for signer
/// * `prefix` - prefix for signer address
pub fn new(private_key: Vec<u8>, prefix: String) -> ChainResult<Self> {
let address = Self::address(&private_key, &prefix)?;

let signing_key = Self::build_signing_key(&private_key)?;
SigningKey::from_slice(&private_key)?;
let public_key = signing_key.public_key();
Ok(Self {
public_key,
private_key,
address,
prefix,
}
})
}

/// get bech32 address
pub fn address(&self) -> String {
verify::pub_to_addr(
SigningKey::from_slice(self.private_key.as_slice())
.unwrap()
fn address(private_key: &Vec<u8>, prefix: &str) -> ChainResult<String> {
let address = verify::pub_to_addr(
SigningKey::from_slice(private_key.as_slice())?
.public_key()
.to_bytes(),
self.prefix.as_str(),
)
.unwrap()
prefix,
)?;
Ok(address)
}

/// Build a SigningKey from a private key. This cannot be
/// precompiled and stored in `Signer`, because `SigningKey` is not `Sync`.
pub fn signing_key(&self) -> ChainResult<SigningKey> {
Self::build_signing_key(&self.private_key)
}

fn build_signing_key(private_key: &Vec<u8>) -> ChainResult<SigningKey> {
Ok(SigningKey::from_slice(private_key.as_slice())?)
}
}
3 changes: 1 addition & 2 deletions rust/chains/hyperlane-cosmos/src/validator_announce.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,7 @@ impl ValidatorAnnounce for CosmosValidatorAnnounce {
.await?;

Ok(TxOutcome {
transaction_id: H256::from_slice(hex::decode(response.txhash).unwrap().as_slice())
.into(),
transaction_id: H256::from_slice(hex::decode(response.txhash)?.as_slice()).into(),
executed: response.code == 0,
gas_used: U256::from(response.gas_used),
gas_price: U256::from(response.gas_wanted),
Expand Down
4 changes: 2 additions & 2 deletions rust/hyperlane-base/src/settings/signers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ impl BuildableWithSignerConf for hyperlane_cosmos::Signer {
SignerConf::HexKey { .. } => bail!("HexKey signer is not supported by cosmos"),
SignerConf::Aws { .. } => bail!("Aws signer is not supported by cosmos"),
SignerConf::CosmosKey { key, prefix } => {
hyperlane_cosmos::Signer::new(key.as_bytes().to_vec(), prefix.clone())
hyperlane_cosmos::Signer::new(key.as_bytes().to_vec(), prefix.clone())?
}
SignerConf::Node => bail!("Node signer is not supported by cosmos"),
})
Expand All @@ -161,6 +161,6 @@ impl BuildableWithSignerConf for hyperlane_cosmos::Signer {

impl ChainSigner for hyperlane_cosmos::Signer {
fn address_string(&self) -> String {
self.address()
self.address.clone()
}
}
3 changes: 3 additions & 0 deletions rust/hyperlane-core/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ pub enum ChainCommunicationError {
/// Int string parsing error
#[error("{0}")]
ParseIntError(#[from] std::num::ParseIntError),
/// Hash string parsing error
#[error("{0}")]
HashParsingError(#[from] fixed_hash::rustc_hex::FromHexError),
/// Invalid Request
#[error("Invalid Request: {msg:?}")]
InvalidRequest {
Expand Down

0 comments on commit 7289e0f

Please sign in to comment.