docs: clarify what is available in ctx.BlockHeader() #23394
Conversation
📝 WalkthroughWalkthroughThe pull request introduces two distinct changes: first, enhancing documentation for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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
Documentation and Community
|
| func (c Context) HeaderInfo() header.Info { return c.headerInfo } | ||
|
|
||
| // BlockHeader returns the header by value. | ||
| // Note, only ChainID, Time and Height are always filled by baseapp. | ||
| // In finalize block, the proposer address, validator hash and app hash are also filled. | ||
| // Use CometInfo service and/or the Consensus Keeper for getting more information. | ||
| func (c Context) BlockHeader() cmtproto.Header { |
There was a problem hiding this comment.
Change potentially affects state.
Call sequence:
(github.com/cosmos/cosmos-sdk/types.Context).BlockHeader (types/context.go:95)
(github.com/cosmos/cosmos-sdk/types.Context).WithProposer (types/context.go:183)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).PrepareProposal (types/context.go:408)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
types/context.go(1 hunks)x/auth/ante/expected_keepers.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- types/context.go
🧰 Additional context used
📓 Path-based instructions (1)
x/auth/ante/expected_keepers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 GitHub Actions: Tests / Code Coverage
x/auth/ante/expected_keepers.go
[warning] No test coverage (0.0% of statements covered)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
| @@ -28,5 +28,6 @@ type FeegrantKeeper interface { | |||
| } | |||
|
|
|||
| type ConsensusKeeper interface { | |||
| AppVersion(ctx context.Context) (uint64, error) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add godoc comments for the new AppVersion method.
Following Go best practices and the Uber style guide, please add documentation explaining:
- The purpose of this method
- What the returned version number represents
- When errors might be returned
Example:
+// AppVersion returns the current application version.
+// It returns an error if the version cannot be retrieved.
AppVersion(ctx context.Context) (uint64, error)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AppVersion(ctx context.Context) (uint64, error) | |
| // AppVersion returns the current application version. | |
| // It returns an error if the version cannot be retrieved. | |
| AppVersion(ctx context.Context) (uint64, error) |
🧰 Tools
🪛 GitHub Actions: Tests / Code Coverage
[warning] No test coverage (0.0% of statements covered)
| AppVersion(ctx context.Context) (uint64, error) | ||
| BlockParams(context.Context) (uint64, uint64, error) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for ConsensusKeeper interface methods
The ConsensusKeeper interface lacks test coverage:
- No tests found for the new
AppVersionmethod - No actual usage of
BlockParamsmethod found in tests - Mock implementation in
testutil/expected_keepers_mocks.gois outdated and missing theAppVersionmethod
Please:
- Update the mock implementation to include the
AppVersionmethod - Add test coverage for both
AppVersionandBlockParamsmethods - Ensure the mock is properly used in integration tests
🔗 Analysis chain
Add test coverage for the ConsensusKeeper interface.
The pipeline indicates 0% test coverage. Please add tests to verify the behavior of implementations of this interface, particularly the new AppVersion method.
Let me help verify the current test coverage:
Would you like me to help create a test suite for this interface?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing test files and their coverage
echo "Checking for existing test files..."
fd --type f "expected_keepers_test.go" .
echo "Checking test coverage for ConsensusKeeper implementations..."
rg -l "type \w+ struct.*implements.*ConsensusKeeper"
Length of output: 309
Script:
#!/bin/bash
# Find all test files in auth/ante directory
echo "=== Test files in auth/ante ==="
fd -e go -e test.go . x/auth/ante/
# Search for ConsensusKeeper interface implementations
echo -e "\n=== ConsensusKeeper implementations ==="
ast-grep --pattern 'type $_ struct {
$$$
}'
# Search for ConsensusKeeper usage in test files
echo -e "\n=== ConsensusKeeper usage in tests ==="
rg "ConsensusKeeper" -A 5 -B 5 --type go --glob "*_test.go"
# Look for mock implementations
echo -e "\n=== Mock implementations ==="
rg "mock.*ConsensusKeeper" --type go
Length of output: 68209
Script:
#!/bin/bash
# Search for ConsensusKeeper interface usage and mocks in test files
echo "=== ConsensusKeeper in test files ==="
rg -B 5 -A 5 "ConsensusKeeper" x/auth/ante/testutil/expected_keepers_mocks.go
# Search for AppVersion and BlockParams method usage in tests
echo -e "\n=== AppVersion and BlockParams usage in tests ==="
rg -l "AppVersion|BlockParams" x/auth/ante/**/*_test.go
# Check mock implementations
echo -e "\n=== Mock implementations content ==="
cat x/auth/ante/testutil/expected_keepers_mocks.go
Length of output: 10079
Script:
#!/bin/bash
# Check for actual usage of BlockParams in tests
echo "=== BlockParams usage in tests ==="
rg -B 5 -A 5 "BlockParams" x/auth/ante/unorderedtx/manager_test.go x/auth/ante/unorderedtx/snapshotter_test.go
# Check if AppVersion is used anywhere in tests
echo -e "\n=== AppVersion usage in tests ==="
rg -l "AppVersion" x/auth/ante/**/*_test.go
Length of output: 375
🧰 Tools
🪛 GitHub Actions: Tests / Code Coverage
[warning] No test coverage (0.0% of statements covered)
|
LGTM but lint failing |
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Documentation
BlockHeader()method to clarify block finalization detailsNew Features
AppVersion()method toConsensusKeeperinterface, enabling application version retrieval