Hyperlane contracts feedback

hyperlane_starknet/contracts/src/contracts/mailbox.cairo

Line 37 in 24f4e31

local_domain: u32,

- local_domain should be immutable

hyperlane_starknet/contracts/src/contracts/mailbox.cairo

Line 279 in 24f4e31

required_hook.post_dispatch(hook_metadata, message.clone());

- flip the order of requiredHook and hook

hyperlane_starknet/contracts/src/contracts/mailbox.cairo

Line 275 in 24f4e31

if (hook != contract_address_const::<0>()) {

- required checks for enough gas payment for the hooks (not sure how you'll do it if it's not in the native token like in protocol_fee)

hyperlane_starknet/contracts/src/contracts/hooks/merkle_tree_hook.cairo

Line 49 in 24f4e31

ownable: OwnableComponent::Storage,

- safe to remove ownable upgradeable from merkle_tree hook if mailbox is immutable

hyperlane_starknet/contracts/src/contracts/hooks/merkle_tree_hook.cairo

Line 152 in 24f4e31

    
           fn post_dispatch(ref self: ContractState, _metadata: Bytes, _message: Message) {

- can you use a separate AbstractPostDispatchHook so you can dedupe some of the public functions definitions and supports_metadata calls

hyperlane_starknet/contracts/src/contracts/client/mailboxclient.cairo

Line 2 in 24f4e31

mod mailboxclient {

- should this be called mailboxClientProxy?

hyperlane_starknet/contracts/src/contracts/isms/aggregation/aggregation.cairo

Line 141 in 24f4e31

fn set_modules(ref self: ContractState, _modules: Span<ContractAddress>) {

- the solidity impl has the modules as static whereas the cairo impl can be modified and ownable. This is a slightly different behavior-wise because the ism owner which doesn't need to be mailbox owner can change the modules before a message is verified. I'm nitpicking here, so it shouldn't block anything but just pointing it out. Same with the multisigISMs.

Local Domain Immutable : got it, we cannot specify directly on the storage that the variable is immutable, but the only way to change the value of the local domain is through the line self.local_domain.write(). I will remove the possibility to do for the owner
Ownable Upgradeable removal : I can remove the upgradeable, but it will be trickier to remove the ownable. From what I have understood, when deploying the merkle_tree_hook, we specify a mailbox and initiate the merkle_tree_hook contract with an instance mailbox client associated to the mailbox provided. Even if the mailbox is immutable, the mailbox client associated is mutable, since we can set the hooks or the ism. If it is really necessary to remove the ownable part, I can but it will slightly modify the trait definition and architecture.
separate AbstractPostDispatchHook I am not sure I understood what you meant here, can you detail please ?
set_modules I see what you mean, maybe a solution here could be to define and set the modules in the constructor and disable the set_module function. However, we loose some flexibility, what do you think @aroralanuk