-
Notifications
You must be signed in to change notification settings - Fork 10
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
extract trace block common logic #608
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
api/debug.go (4)
57-59
: Unusedcfg *tracers.TraceConfig
parameter inTraceBlockByNumber
The
cfg
parameter is accepted but not used inTraceBlockByNumber
. If the trace configuration isn't required at this time, consider removing the parameter to avoid confusion. Alternatively, if you plan to use it in the future, consider adding a comment to explain its current unused state.
70-72
: Unusedcfg *tracers.TraceConfig
parameter inTraceBlockByHash
Similar to
TraceBlockByNumber
, thecfg
parameter inTraceBlockByHash
is currently unused. Consider utilizing the configuration parameter or removing it if it's not needed at this time.
80-84
: Unusedcfg *tracers.TraceConfig
parameter intraceBlock
In the
traceBlock
function, thecfg
parameter is unused (denoted by_ *tracers.TraceConfig
). If you don't need this parameter, consider removing it. If it's intended for future use, adding a comment to explain its purpose might be helpful.
87-89
: Consider passingcfg
toTraceTransaction
In
traceBlock
, when callingTraceTransaction
, you're passingnil
as the*tracers.TraceConfig
parameter. If you plan to support trace configurations inTraceTransaction
later, consider passingcfg
instead ofnil
to maintain consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- api/debug.go (3 hunks)
🔇 Additional comments (3)
api/debug.go (3)
13-13
: Importedmodels
package is appropriateThe addition of the
models
import is necessary since thetraceBlock
function now accepts a*models.Block
parameter.
64-64
: Good use of the newtraceBlock
functionRefactoring to use the
traceBlock
function improves code reusability and reduces duplication. This enhances maintainability.
77-78
: Consistent refactoring inTraceBlockByHash
Using the
traceBlock
function here ensures consistency withTraceBlockByNumber
and reduces code duplication.
_ context.Context, | ||
hash gethCommon.Hash, | ||
_ *tracers.TraceConfig, | ||
) (json.RawMessage, 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.
Utilize the context.Context
parameter in TraceTransaction
Currently, TraceTransaction
ignores the context.Context
parameter by naming it _ context.Context
. However, in the traceBlock
function, you pass ctx
to TraceTransaction
, indicating that the context should be propagated. To ensure proper context handling, consider changing the parameter back to ctx context.Context
and using it within TraceTransaction
.
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.
LGTM!
Description
Just a quick logic de-duplication
For contributor use:
master
branchFiles changed
in the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes