-
Notifications
You must be signed in to change notification settings - Fork 324
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
[api] web3 api debug_traceCall support javascript tracing #3931
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3931 +/- ##
==========================================
+ Coverage 75.38% 76.02% +0.64%
==========================================
Files 303 334 +31
Lines 25923 28431 +2508
==========================================
+ Hits 19541 21615 +2074
- Misses 5360 5716 +356
- Partials 1022 1100 +78 ☔ View full report in Codecov by Sentry. |
api/coreservice.go
Outdated
@@ -59,9 +60,18 @@ import ( | |||
"github.com/iotexproject/iotex-core/server/itx/nodestats" | |||
"github.com/iotexproject/iotex-core/state" | |||
"github.com/iotexproject/iotex-core/state/factory" | |||
|
|||
// Force-load the tracer engines to trigger registration | |||
_ "github.com/ethereum/go-ethereum/eth/tracers/js" |
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.
move to the line below L29
@@ -1731,3 +1733,45 @@ func (core *coreService) Track(ctx context.Context, start time.Time, method stri | |||
Success: success, | |||
}, size) | |||
} | |||
|
|||
func (core *coreService) traceTx(ctx context.Context, txctx *tracers.Context, config *tracers.TraceConfig, simulateFn func(ctx context.Context) ([]byte, *action.Receipt, error)) ([]byte, *action.Receipt, any, error) { |
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.
create a private func for context initialization instead of traceTx with simulateFn?
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.
create a private func for context initialization instead of traceTx with simulateFn?
traceTx (js tracing) has timeout settings, need to put simulateFn into tractTx.
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.
it seems no need to put simulateFn
as param of traceTx
, can just call core.SimulateExecution
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.
it seems no need to put
simulateFn
as param oftraceTx
, can just callcore.SimulateExecution
If do this, need to pass some core.SimulateExecution
required arguments
StructLogs: fromLoggerStructLogs(traces.StructLogs()), | ||
Gas: receipt.GasConsumed, | ||
}, nil | ||
switch tracer := tracer.(type) { |
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.
can tracer's type be deduced from the cfg on L928?
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.
yes, you can. but it is cleaner and safer to deduced from the result
SonarCloud Quality Gate failed. 0 Bugs No Coverage information Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
api/coreservice.go
Outdated
t.Stop(errors.New("execution timeout")) | ||
} | ||
}() | ||
defer cancel() |
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.
move to L1758
api/coreservice.go
Outdated
@@ -27,6 +28,10 @@ import ( | |||
"google.golang.org/protobuf/types/known/durationpb" | |||
"google.golang.org/protobuf/types/known/timestamppb" | |||
|
|||
// Force-load the tracer engines to trigger registration | |||
_ "github.com/ethereum/go-ethereum/eth/tracers/js" | |||
_ "github.com/ethereum/go-ethereum/eth/tracers/native" |
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.
move to L19
DisableStack: disableStack, | ||
DisableStorage: disableStorage, | ||
EnableReturnData: enableReturnData, | ||
cfg := &tracers.TraceConfig{ |
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.
Why not set Tracer
and Timeout
, compared with tracCall
?
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.
If users have this requirement, they can add.
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 mean, does it support tracer and timeout config in traceTransaction just like what we supporting in traceCall?
const ( | ||
// defaultTraceTimeout is the amount of time a single transaction can execute | ||
// by default before being forcefully aborted. | ||
defaultTraceTimeout = 5 * time.Second |
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.
Why defaultTraceTimeout
is only used in TraceTransaction
, but not in TraceCall
?
Quality Gate failedFailed conditions 12.3% Duplication on New Code (required ≤ 3%) |
Description
as title
Fixes #3930
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: