Skip to content
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

chore(abci): Pass the context parameter #2576

Merged
merged 2 commits into from
Mar 9, 2025
Merged

Conversation

shotes
Copy link
Contributor

@shotes shotes commented Mar 6, 2025

Passing the beacond App's s.ctx context field as a parameter to the ABCI functions imitates the functionality of using the ctx from CometBFT. This makes it so that the arguments in each ABCI function call are easily swappable between s.ctx and ctx, should CometBFT ever fully implement their ctx.

@@ -174,7 +174,7 @@ func (s *Service) Start(
return err
}

s.ctx = ctx
s.ResetAppCtx(ctx)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just a nit. Might as well use the setter function.

Copy link

codecov bot commented Mar 6, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 57.94%. Comparing base (823ec52) to head (658ec44).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
consensus/cometbft/service/service.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2576      +/-   ##
==========================================
- Coverage   58.14%   57.94%   -0.21%     
==========================================
  Files         335      338       +3     
  Lines       15036    15122      +86     
  Branches        0       20      +20     
==========================================
+ Hits         8743     8762      +19     
- Misses       5632     5698      +66     
- Partials      661      662       +1     
Files with missing lines Coverage Δ
consensus/cometbft/service/abci.go 50.68% <100.00%> (ø)
consensus/cometbft/service/finalize_block.go 71.28% <100.00%> (ø)
consensus/cometbft/service/init_chain.go 55.88% <100.00%> (+0.65%) ⬆️
consensus/cometbft/service/process_proposal.go 83.72% <100.00%> (ø)
consensus/cometbft/service/service.go 37.43% <0.00%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -46,7 +46,7 @@ func (s *Service) InitChain(
return &cmtabci.InitChainResponse{}, s.ctx.Err()
}
//nolint:contextcheck // see s.ctx comment for more details
return s.initChain(req) // internally this uses s.ctx
return s.initChain(s.ctx, req)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this PR, I could easily interchange this s.ctx with the ctx that is provided in the function parameters.

@shotes
Copy link
Contributor Author

shotes commented Mar 6, 2025

Also note that on main, PrepareProposal is currently inconsistent with the other ABCI calls. It is currently implemented exactly like the changes in this PR. So this PR will make all ABCI calls consistent with each other.

@shotes shotes marked this pull request as ready for review March 6, 2025 21:06
@shotes shotes requested a review from a team as a code owner March 6, 2025 21:06
Copy link
Contributor

@rezbera rezbera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@abi87 abi87 merged commit 09b0252 into main Mar 9, 2025
19 checks passed
@abi87 abi87 deleted the pass_context_parameter_abci branch March 9, 2025 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants