-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: make validator key injectable by application developers #21608
Conversation
WalkthroughWalkthroughThe changes introduce a modular approach to private key generation within the application, allowing developers to specify custom key generation functions. This is achieved through the addition of a Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
@tac0turtle your pull request is missing a changelog! |
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, just need to resolve the conflict and go mod tidy
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: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (7)
- runtime/app.go (3 hunks)
- schema/diff/diff_test.go (1 hunks)
- server/start.go (1 hunks)
- server/v2/cometbft/options.go (2 hunks)
- server/v2/cometbft/server.go (1 hunks)
- simapp/app.go (2 hunks)
- types/module/simulation.go (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- runtime/app.go
- schema/diff/diff_test.go
- server/start.go
- server/v2/cometbft/options.go
- server/v2/cometbft/server.go
- simapp/app.go
Additional context used
Path-based instructions (1)
types/module/simulation.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
GitHub Check: tests (03)
types/module/simulation.go
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 43-43:
syntax error: unexpected [ after top level declaration
[failure] 51-51:
syntax error: non-declaration statement outside function body
[failure] 63-63:
syntax error: non-declaration statement outside function body
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 167-167:
syntax error: unexpected <<, expected field name or embedded type
[failure] 171-171:
syntax error: unexpected ||, expected field name or embedded type
[failure] 175-175:
syntax error: unexpected ==, expected field name or embedded type
GitHub Check: tests (02)
types/module/simulation.go
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 43-43:
syntax error: unexpected [ after top level declaration
[failure] 51-51:
syntax error: non-declaration statement outside function body
[failure] 63-63:
syntax error: non-declaration statement outside function body
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 167-167:
syntax error: unexpected <<, expected field name or embedded type
[failure] 171-171:
syntax error: unexpected ||, expected field name or embedded type
[failure] 175-175:
syntax error: unexpected ==, expected field name or embedded type
GitHub Check: tests (01)
types/module/simulation.go
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 43-43:
syntax error: unexpected [ after top level declaration
[failure] 51-51:
syntax error: non-declaration statement outside function body
[failure] 63-63:
syntax error: non-declaration statement outside function body
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 167-167:
syntax error: unexpected <<, expected field name or embedded type
[failure] 171-171:
syntax error: unexpected ||, expected field name or embedded type
[failure] 175-175:
syntax error: unexpected ==, expected field name or embedded type
GitHub Check: tests (00)
types/module/simulation.go
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 43-43:
syntax error: unexpected [ after top level declaration
[failure] 51-51:
syntax error: non-declaration statement outside function body
[failure] 63-63:
syntax error: non-declaration statement outside function body
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 167-167:
syntax error: unexpected <<, expected field name or embedded type
[failure] 171-171:
syntax error: unexpected ||, expected field name or embedded type
[failure] 175-175:
syntax error: unexpected ==, expected field name or embedded type
GitHub Check: dependency-review
types/module/simulation.go
[failure] 39-39:
expected 'IDENT', found '<<'
[failure] 39-39:
expected type, found '<<'
[failure] 41-41:
expected 'IDENT', found 'type'
[failure] 41-41:
expected ';', found 'interface'
[failure] 47-47:
expected 'IDENT', found 'type'
[failure] 47-47:
expected ';', found 'interface'
[failure] 53-53:
expected 'IDENT', found 'type'
[failure] 53-53:
expected ';', found 'interface'
[failure] 59-59:
expected 'IDENT', found 'type'
[failure] 59-59:
expected ';', found 'interface'
GitHub Check: golangci-lint
types/module/simulation.go
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 43-43:
syntax error: unexpected [ after top level declaration
[failure] 51-51:
syntax error: non-declaration statement outside function body
[failure] 63-63:
syntax error: non-declaration statement outside function body
[failure] 167-167:
syntax error: unexpected <<, expected field name or embedded type
[failure] 171-171:
syntax error: unexpected ||, expected field name or embedded type
[failure] 175-175:
syntax error: unexpected ==, expected field name or embedded type) (typecheck)
Additional comments not posted (2)
types/module/simulation.go (2)
49-50
: LGTM!The
ProposalContents
method is well-defined in theHasProposalContents
interface, with a clear comment indicating its purpose for simulating legacy governance proposals. The explicit definition ensures backward compatibility.
169-170
: LGTM!The addition of the
LegacyProposalContents
andProposalMsgs
fields to theSimulationState
struct enhances its structure and clarity. The field names are descriptive, and their types are appropriate for storing the respective generator functions and their metadata.
types/module/simulation.go
Outdated
<<<<<<< HEAD | ||
// HasProposalMsgs defines the messages that can be used to simulate governance (v1) proposals | ||
type HasProposalMsgs interface { | ||
// msg functions used to simulate governance proposals | ||
ProposalMsgs(simState SimulationState) []simulation.WeightedProposalMsg | ||
} | ||
|
||
// HasProposalContents defines the contents that can be used to simulate legacy governance (v1beta1) proposals | ||
type HasProposalContents interface { | ||
// content functions used to simulate governance proposals | ||
ProposalContents(simState SimulationState) []simulation.WeightedProposalContent | ||
} |
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.
Resolve the merge conflict.
The code segment contains merge conflict markers, indicating an unresolved merge conflict. Please resolve the conflict and ensure the code is valid before proceeding with the review.
Tools
GitHub Check: tests (03)
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 43-43:
syntax error: unexpected [ after top level declaration
[failure] 39-39:
syntax error: unexpected <<, expected name
GitHub Check: tests (02)
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 43-43:
syntax error: unexpected [ after top level declaration
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
GitHub Check: tests (01)
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 43-43:
syntax error: unexpected [ after top level declaration
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
GitHub Check: tests (00)
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 43-43:
syntax error: unexpected [ after top level declaration
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
GitHub Check: dependency-review
[failure] 39-39:
expected 'IDENT', found '<<'
[failure] 39-39:
expected type, found '<<'
[failure] 41-41:
expected 'IDENT', found 'type'
[failure] 41-41:
expected ';', found 'interface'
[failure] 47-47:
expected 'IDENT', found 'type'
[failure] 47-47:
expected ';', found 'interface'
GitHub Check: golangci-lint
[failure] 39-39:
syntax error: unexpected <<, expected name
[failure] 41-41:
syntax error: unexpected type, expected name
[failure] 43-43:
syntax error: unexpected [ after top level declaration
types/module/simulation.go
Outdated
<<<<<<< HEAD | ||
|
||
LegacyProposalContents []simulation.WeightedProposalContent // proposal content generator functions with their default weight and app sim key | ||
ProposalMsgs []simulation.WeightedProposalMsg // proposal msg generator functions with their default weight and app sim key | ||
||||||| c0eced8d7e |
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.
Resolve the merge conflict.
The code segment contains merge conflict markers, indicating an unresolved merge conflict. Please resolve the conflict and ensure the code is valid before proceeding with the review.
Tools
GitHub Check: tests (03)
[failure] 167-167:
syntax error: unexpected <<, expected field name or embedded type
[failure] 171-171:
syntax error: unexpected ||, expected field name or embedded type
GitHub Check: tests (02)
[failure] 167-167:
syntax error: unexpected <<, expected field name or embedded type
[failure] 171-171:
syntax error: unexpected ||, expected field name or embedded type
GitHub Check: tests (01)
[failure] 167-167:
syntax error: unexpected <<, expected field name or embedded type
[failure] 171-171:
syntax error: unexpected ||, expected field name or embedded type
GitHub Check: tests (00)
[failure] 167-167:
syntax error: unexpected <<, expected field name or embedded type
[failure] 171-171:
syntax error: unexpected ||, expected field name or embedded type
GitHub Check: golangci-lint
[failure] 167-167:
syntax error: unexpected <<, expected field name or embedded type
[failure] 171-171:
syntax error: unexpected ||, expected field name or embedded type
66d1fa9
to
317ddcb
Compare
317ddcb
to
23cf89c
Compare
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! just missing go docs.
@@ -30,6 +32,8 @@ import ( | |||
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx" | |||
) | |||
|
|||
type KeyGenF = func() (cmtcrypto.PrivKey, 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.
can we have a godoc here?
// ServerOptions defines the options for the CometBFT server. | ||
// When an option takes a map[string]any, it can access the app.tom's cometbft section and the config.toml config. | ||
type ServerOptions[T transaction.Tx] struct { | ||
PrepareProposalHandler handlers.PrepareHandler[T] | ||
ProcessProposalHandler handlers.ProcessHandler[T] | ||
VerifyVoteExtensionHandler handlers.VerifyVoteExtensionhandler | ||
ExtendVoteHandler handlers.ExtendVoteHandler | ||
KeygenF keyGenF |
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 really like that it doesn't leak in the app in v2 🙏🏾
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.
Can we have a go doc here too? It's less descriptive than the above.
x/simulation/params.go
Outdated
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.
you will probably need to revert this file to make the linter happy
@@ -308,3 +312,9 @@ var _ servertypes.Application = &App{} | |||
type hasServicesV1 interface { | |||
RegisterServices(grpc.ServiceRegistrar) error | |||
} | |||
|
|||
func (a *App) ValidatorKeyProvider() KeyGenF { |
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.
go doc here too please
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 (1)
x/simulation/params.go (1)
163-163
: Deprecation notice acknowledged. Consider updating the codebase.The comment correctly indicates that the
WeightedProposalContent
struct is deprecated. As confirmed by the static analysis hints,simulation.ContentSimulatorFn
should be replaced withsimulation.MsgSimulatorFn
throughout the codebase.Consider updating the codebase to use
simulation.MsgSimulatorFn
instead ofsimulation.ContentSimulatorFn
to align with the deprecation notice.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/simulation/params.go (2 hunks)
Additional context used
Path-based instructions (1)
x/simulation/params.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
GitHub Check: golangci-lint
x/simulation/params.go
[failure] 167-167:
SA1019: simulation.ContentSimulatorFn is deprecated: Use MsgSimulatorFn instead. (staticcheck)
[failure] 170-170:
SA1019: simulation.ContentSimulatorFn is deprecated: Use MsgSimulatorFn instead. (staticcheck)
[failure] 182-182:
SA1019: simulation.ContentSimulatorFn is deprecated: Use MsgSimulatorFn instead. (staticcheck)
x/simulation/params.go
Outdated
type WeightedProposalContent struct { | ||
appParamsKey string // key used to retrieve the value of the weight from the simulation application params | ||
defaultWeight int // default weight | ||
contentSimulatorFn simulation.ContentSimulatorFn // content simulator function | ||
} | ||
|
||
func NewWeightedProposalContent(appParamsKey string, defaultWeight int, contentSimulatorFn simulation.ContentSimulatorFn) simulation.WeightedProposalContent { //nolint:staticcheck // used for legacy testing | ||
func NewWeightedProposalContent(appParamsKey string, defaultWeight int, contentSimulatorFn simulation.ContentSimulatorFn) simulation.WeightedProposalContent { |
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.
Update the function signatures to use simulation.MsgSimulatorFn
.
The function signatures of NewWeightedProposalContent
and ContentSimulatorFn
use the deprecated simulation.ContentSimulatorFn
type. As confirmed by the static analysis hints, simulation.ContentSimulatorFn
should be replaced with simulation.MsgSimulatorFn
.
Please update the function signatures to use simulation.MsgSimulatorFn
instead of simulation.ContentSimulatorFn
. This will also require updating the corresponding function implementations and any code that invokes these functions.
Do you want me to open a GitHub issue to track the refactoring task or provide a diff with the necessary changes?
Also applies to: 182-182
Tools
GitHub Check: golangci-lint
[failure] 170-170:
SA1019: simulation.ContentSimulatorFn is deprecated: Use MsgSimulatorFn instead. (staticcheck)
(cherry picked from commit f1dd03f) # Conflicts: # schema/diff/diff_test.go
#21608) (#21802) Co-authored-by: Marko <[email protected]> Co-authored-by: marbar3778 <[email protected]>
Description
Closes: #21587
allow application developers to pick their validator key
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.md
Reviewers 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
New Features
Bug Fixes
Refactor
Chores