feat(RPC): estimate fee Error returns with transaction context #1551
Conversation
…re FieldElement is serialized as decimal in some context
|
@hhamud which one should be merged first? |
| ) -> jsonrpsee::core::Error { | ||
| jsonrpsee::core::Error::Call(CallError::Custom(ErrorObject::owned( | ||
| err as i32, | ||
| format!("{err}{msg}"), |
There was a problem hiding this comment.
no separator between err and msg?
There was a problem hiding this comment.
Thanks for the review!
In the latest commit, it is now possible to distinguish between revert_error and other errors within SimulationError. Therefore, I believe it may not be necessary to request this PR based on the current content, even if I modify it to reflect your review.
There was a problem hiding this comment.
What do you mean? Sorry I don't get your point
There was a problem hiding this comment.
I apologize if my previous comment was unclear. Since PR #1555 implements similar error handling, I suggest we close this PR. And if there are any further requirements, I would be happy to address them in the future in a new PR
| for (i, (overall_fee, gas_consumed)) in fee_estimates.iter().enumerate() { | ||
| match (*overall_fee, *gas_consumed) { | ||
| FEE_ESTIMATION_FAILED => { | ||
| failed_estimates.push_rejected(transactions[i].compute_hash::<H>(chain_id, false)) |
There was a problem hiding this comment.
you will have to rebase. The compute_hash does not take a generic anymore
| return Err(crate::errors::rpc_error_with_data( | ||
| StarknetRpcApiError::ContractError, | ||
| failed_estimates, | ||
| " with transaction hashes".to_string(), |
There was a problem hiding this comment.
Where does this comes from?
IT's the "msg" field. Did you add it yourself or is it part of the specs?
| match fee_res { | ||
| Ok(fee) => fees.push(fee?), | ||
| Err(Error::<T>::TransactionExecutionFailed) => fees.push(FEE_ESTIMATION_FAILED), | ||
| Err(Error::<T>::TransactionExecutionReverted) => fees.push(FEE_ESTIMATION_REVERTED), | ||
| Err(e) => return Err(e.into()), | ||
| } |
There was a problem hiding this comment.
For estimate fee, we abort as soon as the first estimate fail.
That is why the previous code was fees.push(fee_res??);, where ? immediately returns.
You should keep this behaviour of early exiting the loop at the first error encountered
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
estimate_feecalls, including specific transaction hash in error data for failed and reverted transactionsWhat is the current behavior?
When requesting a fee estimate, the call expects a list of transactions. However, if the call fails, it returns an error without context, making it difficult to identify which specific transaction caused the failure.
What is the new behavior?
Errorenum has been updated to distinguish between transactions thatfailedand those that werereverted.ContractError, distinguishing whether it isrevertedorrejected.Does this introduce a breaking change?
No
Other information