-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Add AccessList result type to eth_createAccessList #9811
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ty, some suggestions
/// Optional error message if the transaction failed. | ||
pub error: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be skipped if none
#[serde(default, skip_serializing_if = "Option::is_none")]
let (result, env) = match self.inspect(&mut db, env, &mut inspector) { | ||
Ok((res, env)) => (res, env), | ||
Err(e) => { | ||
return Err(e.into()); | ||
} | ||
ExecutionResult::Success { .. } => Ok(()), | ||
}?; | ||
}; | ||
|
||
let error = match result.result { | ||
ExecutionResult::Halt { reason, .. } => Some(format!("{:?}", reason)), | ||
ExecutionResult::Revert { output, .. } => Some(format!("{:?}", output)), | ||
ExecutionResult::Success { .. } => None, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can keep the code on main and use RevertError::new(output) for the revert case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the code regarding (result, env)
from the main branch as is.
Also, I made some changes to the Revert part and left a commit for it.
|
||
/// `AccessListWithGasUsedAndError` for handling error from `eth_createAccessList` | ||
#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] | ||
pub struct AccessListWithGasUsedAndError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's also name this AccessListResult
|
||
/// `AccessListWithGasUsedAndError` for handling error from `eth_createAccessList` | ||
#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] | ||
pub struct AccessListWithGasUsedAndError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs camelCase rename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise lgtm
use revm_primitives::U256; | ||
use serde::{Deserialize, Serialize}; | ||
|
||
/// `AccessListResult ` for handling error from `eth_createAccessList` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// `AccessListResult ` for handling error from `eth_createAccessList` | |
/// `AccessListResult` for handling errors from `eth_createAccessList` |
nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last nits
return Err(e.into()); | ||
} | ||
}; | ||
let (result, env) = self.inspect(&mut db, env, &mut inspector)?; | ||
|
||
let error = match result.result { | ||
ExecutionResult::Halt { reason, .. } => Some(format!("{:?}", reason)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should use + to_string
reth/crates/rpc/rpc-eth-types/src/error.rs
Line 410 in ef84899
pub const fn halt(reason: HaltReason, gas_limit: u64) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reth/crates/rpc/rpc-eth-api/src/helpers/call.rs
Lines 245 to 249 in d711292
let error = match result.result { | |
ExecutionResult::Halt { reason, .. } => Some(format!("{:?}", reason).to_string()), | |
ExecutionResult::Revert { output, .. } => Some(RevertError::new(output).to_string()), | |
ExecutionResult::Success { .. } => None, | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still need to use
reth/crates/rpc/rpc-eth-types/src/error.rs
Line 410 in ef84899
pub const fn halt(reason: HaltReason, gas_limit: u64) -> Self { |
to create the appropriate error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to bother you. I fix it.
reth/crates/rpc/rpc-eth-api/src/helpers/call.rs
Lines 245 to 251 in 49ef679
let error = match result.result { | |
ExecutionResult::Halt { reason, gas_used } => { | |
Some(RpcInvalidTransactionError::halt(reason, gas_used).to_string()) | |
} | |
ExecutionResult::Revert { output, .. } => Some(RevertError::new(output).to_string()), | |
ExecutionResult::Success { .. } => None, | |
}; |
|
||
let error = match result.result { | ||
ExecutionResult::Halt { reason, .. } => Some(format!("{:?}", reason)), | ||
ExecutionResult::Revert { output, .. } => Some(format!("{:?}", output)), | ||
ExecutionResult::Revert { output, .. } => { | ||
Some(format!("{:?}", RevertError::new(output))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some(format!("{:?}", RevertError::new(output))) | |
Some(RevertError::new(output).to_string()) |
b6547b4
to
1f1b634
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ty, I changed it slightly to exit right away if the tx failed with the correct gas used and error message
close #9746.
@mattsse I made changes based on the conversation we had on the issue #9746 .
I wonder is this the right way to handle the issue. Thank you.