evm: Fix divide by zero error in eth_estimateGas
#2697
Conversation
lib/ain-grpc/src/call_request.rs
Outdated
| @@ -39,6 +39,25 @@ pub struct CallRequest { | |||
| } | |||
|
|
|||
| impl CallRequest { | |||
| pub fn guess_tx_type(&mut self) -> Result<(), Error> { | |||
There was a problem hiding this comment.
What's the context here ? Why do we want to re-assign to transaction_type ?
There was a problem hiding this comment.
@shohamc1 I guess the tradeoff in abstracting this out of th effective_gas_price calculation is that we cannot keep CallRequest immutable. Not sure if that is the best way although i think the idea is nice to always set a tx type on every call context. We could abstract this into our custom deserialize implementation actually which may keep it cleaner. Alternatively maybe keeping it inside effective_gas_price calculation is still the best way, since the tx type in the call context is really only relevant for gas price calculations, may be safer.
There was a problem hiding this comment.
Note that most libraries underlying implementation when sending estimate gas dont actually set a tx type - so we will find ourselves going into this method pretty often. Also, that will mean that CallRequest is required to guess_tx_type, which should be set on deserialization, since get_effective_gas_price will now expect an explicit tx type or result in an error. Might be better to keep the field optional and let effective_gas_price method handle it.
There was a problem hiding this comment.
Made guess_tx_type not mutate CallRequest in 506043e as a temporary fix to unblock RPCs. We can merge this and then explore what the best way to handle transaction_type will be.
lib/ain-grpc/src/rpc/eth.rs
Outdated
| } | ||
|
|
||
| let allowance = available | ||
| .checked_div(fee_cap) | ||
| .ok_or(RPCError::ValueOverflow)?; | ||
| .ok_or(RPCError::DivideByZero)?; |
There was a problem hiding this comment.
shouldn't validate the fee_cap is not zero before get divided for error DividedByZero??
There was a problem hiding this comment.
That is what checked_div does.
There was a problem hiding this comment.
hmm.. i mean that should have 2 validations:
i. fee_cap is not allowed to zero
ii. check_div for underoverflow checking
There was a problem hiding this comment.
Agree. And in relation to the other point, should be more generic.
fee_cap check, ideal to have it checked, but probably not necessary if it isn't used directly later, and can just rely on checked_div
There was a problem hiding this comment.
I think we can use checked_div for both for the moment. get_effective_gas_price already prevents fee_cap from being 0.
|
Going ahead and merging this to unblock a patch release today, but let's fix the error type in another PR to cleanup. DivideByZero is not the most accurate error to be returned. |
lib/ain-grpc/src/call_request.rs
Outdated
| @@ -39,6 +39,27 @@ pub struct CallRequest { | |||
| } | |||
|
|
|||
| impl CallRequest { | |||
| pub fn guess_tx_type(&self) -> Result<Self, Error> { | |||
There was a problem hiding this comment.
Let's rename this. The response does co-relate well with the guess name.
Perhaps clone_with_guessed_tx_type
There was a problem hiding this comment.
Or better yet - even better is guess_tx_type simply just returns the guess.
And then just do a clone internally in the method, and use the clone going forward?
There was a problem hiding this comment.
Renamed to clone_with_guessed_tx_type for now. We can decide on how to properly handle the mutation after we have unblocked the RPC users.
lib/ain-grpc/src/call_request.rs
Outdated
| @@ -39,6 +39,27 @@ pub struct CallRequest { | |||
| } | |||
|
|
|||
| impl CallRequest { | |||
| pub fn clone_with_guessed_tx_type(&self) -> Result<Self, Error> { | |||
There was a problem hiding this comment.
This is only used by get_effective_gas_price where we can switch on the tx type itself. We shouldn't need to clone nor re-assign
There was a problem hiding this comment.
transaction_type is optional and not included by certain clients (notably Metamask), which is why we need this function.
There was a problem hiding this comment.
Sure but this can be directly called by get_effective_gas_price and only return the tx_type itself
No description provided.