Add more review comments

README.md

@@ -8,5 +8,9 @@
 - [x] Mailbox
 - [x] MerkleTree
 - [x] MultisigIsm
-- [ ] InterchainGasPaymaster
-- [ ] ValidatorAnnounce
+- [x] RoutingIsm
+- [x] InterchainGasPaymaster
+- [x] Router
+- [x] CW20Token
+- [x] NativeToken
+- [x] ValidatorAnnounce

contracts/ism-multisig/src/lib.rs

@@ -19,6 +19,7 @@ pub const CONTRACT_VERSION: &str = env!("CARGO_PKG_VERSION");
 const PREFIX: &str = "\x19Ethereum Signed Message:\n";
 
 // can we reuse hyperlane-monorepo/rust/hyperlane-core/src/types/checkpoint ?
+// this should be possible following https://github.com/hyperlane-xyz/hyperlane-monorepo/pull/2788
 
 pub fn domain_hash(local_domain: u32, address: Binary) -> StdResult<Binary> {
     let mut bz = vec![];

contracts/ism-routing/src/contract.rs

@@ -52,6 +52,7 @@ pub fn execute(
 
     match msg {
         Ownership(msg) => Ok(hpl_ownable::handle(deps, env, info, msg)?),
+        // can we add an explicit removal action? see https://github.com/hyperlane-xyz/hyperlane-monorepo/pull/2760
         Set { ism } => {
             ensure_eq!(
                 info.sender,

contracts/mailbox/src/core.rs

@@ -96,6 +96,7 @@ pub fn process(
 
     let config = CONFIG.load(deps.storage)?;
     // FIXME: use hrp fetched from hub
+    // created https://github.com/many-things/cw-hyperlane/issues/40 to track
     let recipient = decoded_msg.recipient_addr("osmo")?;
     let origin_domain = fetch_origin_domain(&deps.querier, &config.factory)?;
 

contracts/mailbox/src/merkle.rs

@@ -5,6 +5,7 @@ use hpl_interface::types::keccak256_hash;
 use crate::state::assert_full_merkle_tree;
 
 // can we reuse hyperlane-monorepo/rust/hyperlane-core/src/accumulator ?
+// this should be possible following https://github.com/hyperlane-xyz/hyperlane-monorepo/pull/2788
 
 pub const HASH_LENGTH: usize = 32;
 pub const TREE_DEPTH: usize = 32;
@@ -46,6 +47,7 @@ pub const ZERO_HASHES: [&str; HASH_LENGTH] = [
     "8448818bb4ae4562849e949e17ac16e0be16688e156b5cf15e098c627c0056a9",
 ];
 
+// default trait implementation should be inherited https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/401b32ddddc6faa281beb050f641afb4f024d905/rust/hyperlane-core/src/accumulator/incremental.rs#L17
 #[cw_serde]
 #[derive(Default)]
 pub struct MerkleTree {

contracts/token-cw20/src/contract.rs

@@ -47,7 +47,7 @@ pub fn instantiate(
                         code_id,
                         msg: to_binary(&init_msg)?,
                         funds: vec![],
-                        label: "created by hpl-toen-cw20".to_string(),
+                        label: "created by hpl-token-cw20".to_string(),
                     },
                     REPLY_ID_CREATE_DENOM,
                 ));
@@ -104,6 +104,8 @@ pub fn execute(
             );
 
             let token_msg: token::Message = msg.body.into();
+            // can we make this agnostic to osmosis?
+            // maybe a feature flag or some trait we implement separately for osmo and neutron?
             let recipient = bech32_encode("osmo", &token_msg.recipient)?;
 
             let token = TOKEN.load(deps.storage)?;
@@ -154,7 +156,6 @@ pub fn execute(
                     dest_domain,
                     recipient,
                 } => {
-                    let token = TOKEN.load(deps.storage)?;
                     let mode = MODE.load(deps.storage)?;
                     let mailbox = MAILBOX.load(deps.storage)?;
 
@@ -182,6 +183,7 @@ pub fn execute(
                             .into(),
                         );
                     }
+                    // else if mode is collateral, how do we ensure `amount` was deposited by `sender`?
 
                     let dispatch_payload = token::Message {
                         recipient: recipient.clone(),

contracts/token-native/src/contract.rs

@@ -37,6 +37,8 @@ pub fn instantiate(
     let mut resp = Response::new();
 
     // create native denom if token is bridged
+    // in EVM we use Native on collateral side and HypERC20 on bridged side, meaning native is only "collateral mode"
+    // does creating a "native denom" mean creating a CW20 on bridged side?
     if msg.mode == TokenMode::Bridged {
         resp = resp.add_submessage(SubMsg::reply_on_success(
             MsgCreateDenom {
@@ -68,7 +70,7 @@ pub fn instantiate(
             });
         }
     } else {
-        // use denom directly if token is native
+        // use denom directly if token is if mode is collateral ?
         TOKEN.save(deps.storage, &msg.denom)?;
     }
 
@@ -137,6 +139,7 @@ pub fn execute(
                 );
             }
 
+            // if mode is bridged, we still push this? I am struggling to understand the "bridged mode" here
             // push token send msg
             msgs.push(
                 BankMsg::Send {

contracts/va/src/contract.rs

@@ -23,6 +23,7 @@ use crate::{
     CONTRACT_NAME, CONTRACT_VERSION,
 };
 
+// let's reuse hyperlane-core here too
 pub fn domain_hash(local_domain: u32, mailbox: &str) -> StdResult<Binary> {
     let mut bz = vec![];
     bz.append(&mut local_domain.to_be_bytes().to_vec());

contracts/va/src/lib.rs

@@ -18,6 +18,7 @@ const PREFIX: &str = "\x19Ethereum Signed Message:\n";
 pub const CONTRACT_NAME: &str = env!("CARGO_PKG_NAME");
 pub const CONTRACT_VERSION: &str = env!("CARGO_PKG_VERSION");
 
+// let's reuse hyperlane-core here too
 pub fn eth_hash(message: Binary) -> Result<Binary, ContractError> {
     let mut eth_message = format!("{PREFIX}{}", message.len()).into_bytes();
     eth_message.extend_from_slice(&message);

packages/router/src/handler.rs

@@ -4,6 +4,7 @@ use cosmwasm_std::{
 };
 use hpl_interface::router::{DomainsResponse, RouterMsg, RouterQuery, RouterResponse, RouterSet};
 
+// can we move owner to Router state and add owner enforcement to relevant `RouterMsg` shapes
 use crate::state::ROUTES;
 
 pub fn handle(deps: DepsMut, _env: Env, info: MessageInfo, msg: RouterMsg) -> StdResult<Response> {