-
Notifications
You must be signed in to change notification settings - Fork 1
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
Make tracing work for fee currency txs #211
Conversation
If we want to execute fee currency txs in tracing tests, we need a FeeCurrencyContext. Rather then setting a fixed on in the test setup code, it is more flexible to read it from the test JSON file, like all other test input.
Without including it in that case, we would miss out on the fee currency code and state, which would make the prestate useless for executing the tx.
We need to look up the fee currency address before using it, like it is done with the to, from and coinbase.
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.
Looks great, awesome that we can use the test infrastructure.
Left two questions, but feel free to ignore them.
@@ -93,6 +93,8 @@ func newPrestateTracer(ctx *tracers.Context, cfg json.RawMessage) (*tracers.Trac | |||
OnTxStart: t.OnTxStart, | |||
OnTxEnd: t.OnTxEnd, | |||
OnOpcode: t.OnOpcode, | |||
// Celo | |||
TraceDebitCredit: true, |
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.
Maybe add an explicit TraceDebitCredit: false
to the call tracer as well?
@@ -158,6 +160,9 @@ func (t *prestateTracer) OnTxStart(env *tracing.VMContext, tx *types.Transaction | |||
t.lookupAccount(from) | |||
t.lookupAccount(t.to) | |||
t.lookupAccount(env.Coinbase) |
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.
When this is added here, should we add accounts which receive fees here as well, like the FeeHandler
?
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 only thought about the fee currency txs in this PR, where the FeeHandler
is part of the fee currency's storage. But for a normal tx and enables Cel2, that might be necessary. I'll take a look at that 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.
Followup issue is at #216.
Todo: should the FeeHandler address show up in the diff mode tracing? EDIT: tipReceiver and baseFeeReceiver are the same in the test, therefore only a single address (plus tx sender) is touched. |
Closes #69
Added tests for the following tracers: