fix(hyperlane-ethereum): only apply gas overrides when submitting #5062
Conversation
|
| @@ -315,6 +315,7 @@ where | |||
| &self, | |||
| message: &HyperlaneMessage, | |||
| metadata: &[u8], | |||
| apply_gas_overrides: bool, | |||
There was a problem hiding this comment.
nit: maybe we can have a ContractCallOptions struct that holds different parameters. I think this makes the code a bit easier to read compared to a boolean function parameter.
But I also see some places that don't have tx_gas_estimate, so not sure.
#[derive(Clone, Debug)]
pub struct ContractCallOptions {
pub apply_gas_overrides: bool,
pub tx_gas_estimate: Option<U256>,
}
async fn process_contract_call(
&self,
message: &HyperlaneMessage,
metadata: &[u8],
options: &ContractCallOptions,
) -> ChainResult<ContractCall<M, ()>> {
...
}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5062 +/- ##
=======================================
Coverage 77.53% 77.53%
=======================================
Files 103 103
Lines 2110 2110
Branches 190 190
=======================================
Hits 1636 1636
Misses 453 453
Partials 21 21
|
| @@ -355,7 +355,7 @@ impl PendingOperation for PendingMessage { | |||
| if self | |||
| .ctx | |||
| .destination_mailbox | |||
| .process_estimate_costs(&self.message, metadata) | |||
| .process_estimate_costs(&self.message, metadata, true) | |||
There was a problem hiding this comment.
any reason why this needs to be true?
iiuc this being true or false doesn't change the behavior we care about, which is just that an eth_estimateGas is successful. We don't care about the buffer being applied or anything here
I think if it doesn't matter, then we can keep the process_estimate_costs interface the same as before, and just have process_contract_call be the one with the interface change, and have process_estimate_costs always pass false as this param to process_contract_call. I think this would be kinda nice because it'd keep the Mailbox trait impl more self explanatory / simple, and the whole gas buffer application stuff feels like it's an EVM detail that maybe shouldn't be mentioned on the VM-agnostic interface
| ) -> ChainResult<TxCostEstimate> { | ||
| let contract_call = self.process_contract_call(message, metadata, None).await?; | ||
| let contract_call = self | ||
| .process_contract_call(message, metadata, apply_gas_overrides, None) |
There was a problem hiding this comment.
I think if apply_gas_overrides is false, when we call process_contract_call we end up never calling eth_estimateGas, and we'd hit the .ok_or(HyperlaneProtocolError::ProcessGasLimitRequired)?; - I assume this is why e2e is failing
Description
We want gas overrides (and the gas buffer) to be added to a tx just before submitting, to increase its chances of landing. However we want these to be excluded in the
preparestep when just checking that the sender's gas payment is enough to approximately cover the estimatedgas_limitreturned by the destination chain RPCDrive-by changes
Related issues
Backward compatibility
Yes
Testing
will rely on e2e and will first release this to the RC context