feat:add active field for validator #278
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested labels
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 (
|
trestinlsd
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
x/operator/keeper/consensus_keys.go (4)
4-4: Apply gofumpt formatting.Static analysis flags these lines for not being gofumpt-formatted. Consider running gofumpt or a compatible formatter to align with project guidelines and pass CI checks.
Also applies to: 7-7
🧰 Tools
🪛 golangci-lint (1.62.2)
4-4: File is not
gofumpt-ed(gofumpt)
🪛 GitHub Check: Run golangci-lint
[failure] 4-4:
File is notgofumpt-ed (gofumpt)
610-615: Leverage the existing context for remote queries.Here, a new call to
QueryCometbftValidators()is made, usingcontext.Background(). Consider passing down thectxfrom the caller or implementing a timeout/cancellation strategy for better control over potential network issues.
699-718: Parameterize RPC URL for better configuration.
QueryCometbftValidators()currently uses a hardcoded RPC URL ("http://localhost:26657"). Extracting this into a configurable parameter or environment variable can improve maintainability and accommodate different deployments.🧰 Tools
🪛 golangci-lint (1.62.2)
718-718: File is not
gofumpt-ed(gofumpt)
🪛 GitHub Check: Run golangci-lint
[failure] 718-718:
File is notgofumpt-ed (gofumpt)
719-726: Optimize for large validator sets if performance is a concern.
IsAddressInSlicedoes a linear lookup. For large validator sets, consider using a map to achieve O(1) lookups. Otherwise, if the list remains small, the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/operator/types/validator.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
proto/exocore/operator/v1/validator.proto(1 hunks)x/operator/keeper/consensus_keys.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
x/operator/keeper/consensus_keys.go
4-4: File is not gofumpt-ed
(gofumpt)
7-7: File is not gofumpt-ed
(gofumpt)
718-718: File is not gofumpt-ed
(gofumpt)
🪛 GitHub Check: Run golangci-lint
x/operator/keeper/consensus_keys.go
[failure] 4-4:
File is not gofumpt-ed (gofumpt)
[failure] 7-7:
File is not gofumpt-ed (gofumpt)
[failure] 718-718:
File is not gofumpt-ed (gofumpt)
🔇 Additional comments (1)
proto/exocore/operator/v1/validator.proto (1)
59-60: New field aligns with the intended functionality.Introducing the
activefield is straightforward and should default tofalsein Proto3, which makes sense for an opt-in setting. Ensure any client code handles the default correctly and updates this field only after verifying validator status in the Tendermint set.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
x/operator/keeper/consensus_keys.go (1)
722-729: Consider optimizing address lookup for large validator sets.The current implementation has O(n) time complexity. For large validator sets, consider using a map for O(1) lookup.
+func BuildAddressMap(addresses []sdk.ConsAddress) map[string]struct{} { + addrMap := make(map[string]struct{}, len(addresses)) + for _, addr := range addresses { + addrMap[addr.String()] = struct{}{} + } + return addrMap +} func IsAddressInSlice(address sdk.ConsAddress, addresses []sdk.ConsAddress) bool { + // For small slices (n < 50), keep the current implementation + if len(addresses) < 50 { for _, addr := range addresses { if addr.Equals(address) { return true } } return false + } + + // For larger slices, use a map for O(1) lookup + addrMap := BuildAddressMap(addresses) + _, exists := addrMap[address.String()] + return exists }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/operator/keeper/consensus_keys.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: goreleaser
trestinlsd
requested review from
magj2006,
bwhour,
adu-web3,
cloud8little,
MaxMustermann2,
mikebraver,
TimmyExogenous and
leonz789
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/dogfood/keeper/validators.go (1)
158-158: Update method comment to accurately describe its purpose.The current comment is copied from
GetExocoreValidatorand doesn't accurately describe what this method does. Consider updating it to:// IsExocoreValidator checks if a validator exists based on the pub key derived (consensus) address.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/app.go(1 hunks)x/dogfood/keeper/validators.go(1 hunks)x/operator/keeper/consensus_keys.go(2 hunks)x/operator/keeper/keeper.go(3 hunks)x/operator/types/expected_keepers.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/operator/keeper/consensus_keys.go
🧰 Additional context used
📓 Learnings (1)
x/operator/types/expected_keepers.go (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#220
File: x/avs/types/expected_keepers.go:47-48
Timestamp: 2024-11-12T10:03:10.791Z
Learning: Adding new methods to the `OperatorKeeper` interface in `x/avs/types/expected_keepers.go` is acceptable, even if there are other unmodifiable `OperatorKeeper` interfaces elsewhere, because interfaces can be extended without causing conflicts.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: goreleaser
🔇 Additional comments (5)
x/operator/keeper/keeper.go (2)
23-23: LGTM! Field placement follows existing pattern.The
stakingKeeperfield is appropriately placed with other keeper dependencies.
35-35: LGTM! Consistent initialization of stakingKeeper.The
stakingKeeperparameter and initialization follow the established pattern for keeper dependencies.Also applies to: 45-45
x/operator/types/expected_keepers.go (1)
144-146: LGTM! Clean interface definition.The
StakingKeeperinterface is well-defined with a single, focused responsibility. The method name and signature clearly indicate its purpose.x/dogfood/keeper/validators.go (1)
159-164: LGTM! Efficient implementation.The implementation efficiently checks for validator existence using
store.Has()instead of loading the full validator data.app/app.go (1)
701-701: LGTM! Proper dependency injection.The
StakingKeeperis correctly wired into theOperatorKeeperinitialization, maintaining the proper dependency graph.
It's the x/dogfood version of validator, containing only the consensus address, the consensus key and the vote power. |
thanks, I see, so this is only used for querying? |
No; it is important to x/dogfood. It wasn't introduced in this PR but has been present throughout. My rationale behind creating this type is that the x/dogfood module only needs to know the bare-minimum information (key and vote power) to pass to the consensus engine. Everything else can be handled by other modules; for example, x/operator handles the metadata of each operator and maps back from the consensus address to the account address. |
Description
Summary by CodeRabbit
Summary by CodeRabbit
New Features
activefield to the Validator message to indicate presence in the Tendermint validator set.Improvements