-
Notifications
You must be signed in to change notification settings - Fork 94
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
[DRAFT] Fix refund calculation #268
base: main
Are you sure you want to change the base?
[DRAFT] Fix refund calculation #268
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.
great, all of these look good so far, the more challenging part is tracking the total gas refunds per call which I think we need to do when we create a new call object
but maybe there are some things we also need to consider on call_end @rakita ?
gas_used, | ||
decoded: None, | ||
immediate_bytes, | ||
|
||
// fields will be populated end of call | ||
gas_cost: 0, | ||
gas_refund_counter: 0, |
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 makes sense because we always need to init this with 0
/// The total gas refunded in the call. | ||
pub gas_refunded: u64 |
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 think we don't necessarily need this here because we could derive this from the sum of all refunds in the steps?
@@ -518,6 +518,8 @@ impl TracingInspector { | |||
// TODO: Figure out why this can overflow. https://github.com/paradigmxyz/revm-inspectors/pull/38 | |||
step.gas_cost = step.gas_remaining.saturating_sub(interp.control.gas().remaining()); | |||
|
|||
step.gas_refund_counter = interp.control.gas().refunded(); |
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.
makes sense
@@ -707,7 +709,7 @@ impl CallTraceStep { | |||
gas_cost: self.gas_cost, | |||
op: self.op.to_string(), | |||
pc: self.pc as u64, | |||
refund_counter: (self.gas_refund_counter > 0).then_some(self.gas_refund_counter), | |||
refund_counter: (self.gas_refund_counter > 0).then_some(self.gas_refund_counter as u64), |
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 think we can do it like this but only if we update the refund counters accordingly, because we're now only tracking the refund on a per call level, so what we need to di is one post processing step that updates the counter when a call ended.
I believe we could do this when we initialize the first calltrace using the sum of all gas refunds up until this point.
Yeah, there should be a And traced refund should be a |
Fix refund calculation
Fixes
Fixes issue: #267
Description
Should allow refund value in a call to be a negative value, since EVM supports negative values