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.

Sign up for GitHub

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

Jump to bottom

chore(CometBFTService): simplified cometBFTService Info #2028

Merged
merged 1 commit into from
Sep 30, 2024
Merged

chore(CometBFTService): simplified cometBFTService Info #2028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions mod/consensus/pkg/cometbft/service/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
cmtproto "github.com/cometbft/cometbft/api/cometbft/types/v1"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
sdkversion "github.com/cosmos/cosmos-sdk/version"
itsdevbear marked this conversation as resolved.
Show resolved Hide resolved
"github.com/sourcegraph/conc/iter"
)

Expand Down Expand Up @@ -187,21 +188,18 @@ func (s *Service[LoggerT]) Info(
*cmtabci.InfoRequest,
) (*cmtabci.InfoResponse, error) {
lastCommitID := s.sm.CommitMultiStore().LastCommitID()
appVersion := InitialAppVersion
appVersion := initialAppVersion
if lastCommitID.Version > 0 {
ctx, err := s.CreateQueryContext(lastCommitID.Version, false)
if err != nil {
return nil, fmt.Errorf("failed creating query context: %w", err)
}
Comment on lines -192 to -195
Copy link
Contributor Author

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

appVersion, err = s.AppVersion(ctx)
var err error
appVersion, err = s.appVersion()
if err != nil {
return nil, fmt.Errorf("failed getting app version: %w", err)
}
}

return &cmtabci.InfoResponse{
Data: s.name,
Version: s.version,
Data: appName,
Version: sdkversion.Version,
AppVersion: appVersion,
LastBlockHeight: lastCommitID.Version,
LastBlockAppHash: lastCommitID.Hash,
Expand Down Expand Up @@ -530,7 +528,7 @@ func (s *Service[LoggerT]) CreateQueryContext(
return sdk.Context{}, errorsmod.Wrapf(
sdkerrors.ErrInvalidHeight,
"%s is not ready; please wait for first block",
s.name,
appName,
)
}

Expand Down
17 changes: 9 additions & 8 deletions mod/consensus/pkg/cometbft/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import (
"github.com/cometbft/cometbft/proxy"
dbm "github.com/cosmos/cosmos-db"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/version"
)

type (
Expand All @@ -54,7 +53,10 @@ const (
execModeFinalize
)

const InitialAppVersion uint64 = 0
const (
initialAppVersion uint64 = 0
appName string = "beacond"
Copy link
Contributor Author

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

)
Comment on lines +56 to +59
Copy link
Contributor

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.


type Service[
LoggerT log.AdvancedLogger[LoggerT],
Expand All @@ -63,7 +65,6 @@ type Service[
cmtCfg *cmtcfg.Config

logger LoggerT
name string
sm *statem.Manager
Middleware MiddlewareI

Expand All @@ -75,8 +76,6 @@ type Service[
initialHeight int64
minRetainBlocks uint64

// application's version string
version string
Copy link
Contributor Author

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

chainID string
}

Expand All @@ -93,15 +92,13 @@ func NewService[
) *Service[LoggerT] {
s := &Service[LoggerT]{
logger: logger,
name: "beacond",
sm: statem.NewManager(
db,
servercmtlog.WrapSDKLogger(logger),
),
Middleware: middleware,
cmtCfg: cmtCfg,
paramStore: params.NewConsensusParamsStore(cs),
version: version.Version,
}

s.MountStore(storeKey, storetypes.StoreTypeIAVL)
Expand Down Expand Up @@ -174,7 +171,7 @@ func (s *Service[_]) Close() error {

// Name returns the name of the cometbft.
func (s *Service[_]) Name() string {
return s.name
return appName
}

// CommitMultiStore returns the CommitMultiStore of the cometbft.
Expand All @@ -184,6 +181,10 @@ func (s *Service[_]) CommitMultiStore() storetypes.CommitMultiStore {

// AppVersion returns the application's protocol version.
func (s *Service[_]) AppVersion(_ context.Context) (uint64, error) {
return s.appVersion()
}
Comment on lines +184 to +185
Copy link
Contributor

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


func (s *Service[_]) appVersion() (uint64, error) {
Copy link
Contributor Author

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

cp := s.paramStore.Get()
return cp.Version.App, nil
}
Comment on lines +187 to 190
Copy link
Contributor

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.

Expand Down
Loading