-
Notifications
You must be signed in to change notification settings - Fork 107
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(CometBFTService): simplified cometBFTService Info #2028
Conversation
WalkthroughThe changes introduce updates to 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 (
|
ctx, err := s.CreateQueryContext(lastCommitID.Version, false) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed creating query context: %w", err) | ||
} |
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.
dropping this is the main change of this PR
const InitialAppVersion uint64 = 0 | ||
const ( | ||
initialAppVersion uint64 = 0 | ||
appName string = "beacond" |
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 prefer the name here since it's constant. Happy to revert the change if we don't agree with it
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2028 +/- ##
=======================================
Coverage 22.38% 22.38%
=======================================
Files 358 358
Lines 16045 16042 -3
Branches 12 12
=======================================
Hits 3591 3591
+ Misses 12305 12302 -3
Partials 149 149
|
@@ -75,8 +76,6 @@ type Service[ | |||
initialHeight int64 | |||
minRetainBlocks uint64 | |||
|
|||
// application's version string | |||
version string |
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.
does not seem necessary to make it an attribute of node since it's value won't change upon execution
return s.appVersion() | ||
} | ||
|
||
func (s *Service[_]) appVersion() (uint64, 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.
allows reusing this in Info
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- mod/consensus/pkg/cometbft/service/abci.go (3 hunks)
- mod/consensus/pkg/cometbft/service/service.go (3 hunks)
🔇 Additional comments (4)
mod/consensus/pkg/cometbft/service/service.go (1)
174-174
: LGTM! Simplified Name method.The change to use the
appName
constant in theName
method improves consistency and simplifies the code.mod/consensus/pkg/cometbft/service/abci.go (3)
531-531
: Minor update to error message inCreateQueryContext
The change in the error message to use
appName
instead ofs.name
is consistent with the changes made in theInfo
function. This modification doesn't affect the functionality of theCreateQueryContext
function and appears to be a part of a broader effort to standardize naming conventions.
Line range hint
1-651
: Overall assessment of changes inabci.go
The modifications in this file successfully achieve the PR's objective of simplifying the
Service.Info
function. The changes are consistent and well-implemented, focusing on:
- Simplifying version retrieval in the
Info
function.- Standardizing naming conventions (e.g., using
appName
instead ofs.name
).- Updating version reporting to use
sdkversion.Version
.These changes should result in a simpler and slightly faster implementation, as intended. However, it's crucial to ensure that these modifications don't introduce any unintended side effects, particularly in systems that may depend on the previous implementation details.
To ensure the changes don't introduce any regressions, please run comprehensive tests, paying special attention to:
- Version reporting functionality
- Any systems or processes that interact with the
Info
function- Error handling in the
CreateQueryContext
functionAdditionally, update any relevant documentation to reflect these changes, especially regarding version retrieval and reporting.
191-202
: Simplification ofInfo
function aligns with PR objectivesThe changes to the
Info
function successfully simplify the implementation by eliminating the need forCreateQueryContext
. This aligns with the PR objectives and should result in marginal performance improvements. However, there are a few points to consider:
The use of
initialAppVersion
instead of querying the context is a significant change. Ensure this doesn't affect any functionality that relies on dynamically updated app versions.The
Version
field now usessdkversion.Version
instead ofs.version
. Verify that this change doesn't impact any systems or processes that depend on the previously reported version.The
Data
field now usesappName
instead ofs.name
. Confirm that this change is intentional and doesn't break any existing integrations.To ensure these changes don't introduce unintended side effects, please run the following verification:
✅ Verification successful
Verification Successful: Changes are Safe and Consistent
All references to the old version retrieval method (
s.version
) are confined toabci.go
, and the switch tosdkversion.Version
is consistent throughout the codebase. Additionally, the removal ofCreateQueryContext
does not impact any functional areas.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any references to the old version retrieval method rg --type go 's\.version' --glob '!mod/consensus/pkg/cometbft/service/abci.go' # Check for any uses of CreateQueryContext for version retrieval rg --type go 'CreateQueryContext.*version' # Verify the usage of sdkversion.Version rg --type go 'sdkversion\.Version'Length of output: 496
const ( | ||
initialAppVersion uint64 = 0 | ||
appName string = "beacond" | ||
) |
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.
🧹 Nitpick (assertive)
LGTM! Consider using iota for initialAppVersion.
The introduction of these constants improves code maintainability and aligns with the PR's simplification objective.
Consider using iota
for initialAppVersion
:
const (
initialAppVersion = iota
appName = "beacond"
)
This would make it easier to introduce new versions in the future if needed.
func (s *Service[_]) appVersion() (uint64, error) { | ||
cp := s.paramStore.Get() | ||
return cp.Version.App, nil | ||
} |
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.
🧹 Nitpick (assertive)
LGTM! Consider adding error handling.
The new appVersion
method effectively encapsulates the version retrieval logic, simplifying the AppVersion
method and potentially improving performance.
Consider adding error handling for potential issues with paramStore.Get()
:
func (s *Service[_]) appVersion() (uint64, error) {
cp, err := s.paramStore.Get()
if err != nil {
return 0, fmt.Errorf("failed to get consensus params: %w", err)
}
return cp.Version.App, nil
}
This would make the function more robust and consistent with Go error handling practices.
return s.appVersion() | ||
} |
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.
💡 Codebase verification
CreateQueryContext Still Present in Multiple Files
The CreateQueryContext
function is still used in the following locations:
mod/node-core/pkg/components/api.go
mod/node-api/backend/backend.go
mod/node-api/backend/types.go
mod/node-api/backend/mocks/node.mock.go
mod/consensus/pkg/cometbft/service/abci.go
Please ensure that all instances of CreateQueryContext
are addressed to fully meet the PR objectives.
🔗 Analysis chain
LGTM! Verify elimination of CreateQueryContext.
The change to use a private appVersion
method simplifies the AppVersion
method and improves code organization.
Please confirm that this change eliminates the need for CreateQueryContext
as mentioned in the PR objectives. Run the following script to verify:
Further verification needed: Ensure CreateQueryContext
is fully eliminated from the codebase.
Please run the following script to search for any remaining usage across the repository:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of CreateQueryContext in the file
rg "CreateQueryContext" mod/consensus/pkg/cometbft/service/service.go
Length of output: 69
Script:
#!/bin/bash
# Search for any remaining usage of CreateQueryContext in the entire codebase
rg "CreateQueryContext"
Length of output: 2203
Simplified
Service.Info
to avoidCreateQueryContext
call.The proposed implementation of
Service.Info
differs more from cosmos-sdk Info implementation, but it is simpler and marginally fasterSummary by CodeRabbit
New Features
Bug Fixes
Refactor