hpl-54: quote gas #66
Conversation
HPL-54 Aggregate hook does not consider alternate denominations
The quote_dispatch function in contracts/hooks/aggregate/src/lib.rs:147 does not account for the possibility of having gas coins of different denominations. It incorrectly assumes that all gas coins are of the same denomination, which cannot be guaranteed. Ultimately this will cause an error further in the execution of the dispatch when the necessary funds are not present. Recommendation We recommend ensuring that the gas denomination is the same as the denomination as the gas_total that is being calculated. |
byeongsu-hong
force-pushed
the
develop
branch
2 times, most recently
from
50c0e67
to
49781b7
Compare
|
@byeongsu-hong I think the linked linear issue is incorrect? |
| MailboxHookQueryMsg::QuoteDispatch { sender, msg } => { | ||
| to_binary(quote_dispatch(deps, sender, msg)) |
There was a problem hiding this comment.
we do not want to special case the parameters here with a sender
in the solidity contracts we have a generic metadata param that the message sender can pass through
There was a problem hiding this comment.
thanks for your information, i'll update it after reviewing v3 solidity contract
There was a problem hiding this comment.
after reviewing, so the reason why we decided to add sender to QueryMsg is msg.sender is needed to build a dispatch message according to here
There was a problem hiding this comment.
isn't the sender here a subset of msg? we want to allow arbitrary metadata to be passed through
see https://docs.hyperlane.xyz/docs/reference/hooks/overview
There was a problem hiding this comment.
Of course yes. msg has the type DispatchMsg which is grouped representation of argument dispatch / quote_dispatch in solidity - it has the optional metadata field for custom use cases.
+ It's impossible to construct the mailbox message in case of quoting dispatch due to there's no way to track msg.sender of QueryMsg.
There was a problem hiding this comment.
this makes sense
let's make sure we are consistent everywhere we use info.sender in execute to pass through a sender in the corresponding query
packages/interface/src/hook/mod.rs
Outdated
| @@ -69,7 +69,7 @@ pub struct MailboxResponse { | |||
|
|
|||
| #[cw_serde] | |||
| pub struct QuoteDispatchResponse { | |||
| pub gas_amount: Option<Coin>, | |||
| pub gas_amount: Vec<Coin>, | |||
There was a problem hiding this comment.
can we rename this from gas_amount since it also includes the denomination? how about fees?
There was a problem hiding this comment.
yes i realized there's update on terms about gas fee to protocol fee. makes sense i'll change it
There was a problem hiding this comment.
- how about consider using
protocol_feeto make clear to understand
There was a problem hiding this comment.
nvm changed to fees
| let mut total_gas = required_gas.clone().into_iter().try_fold( | ||
| Coins::try_from(base_gas.clone())?, | ||
| |mut acc, gas| { | ||
| acc.add(gas)?; | ||
| StdResult::Ok(acc) | ||
| }, | ||
| )?; |
There was a problem hiding this comment.
if the denominations are not the same, we should return both rather than add them together
There was a problem hiding this comment.
cosmwasm_std::Coins's add method contains the logic that you mentioned.
|
@yorhodes i think this update fully covers that issue because there's no need to check single denomination if we use multi denom protocol fee, so i linked it |
This has API breaking changes in mailbox contract.
senderfield to QuoteDispatch query msgresolves HPL-54