Integrate with the Bolt registry to check if pubkeys are registered #340
Conversation
thedevbirb
force-pushed
the
lore/feat/check-pubkeys-registry
branch
from
5470221 to
e455d3c
Compare
bolt-sidecar/src/onchain_registry.rs
Outdated
| } | ||
| } | ||
|
|
||
| sol! { |
There was a problem hiding this comment.
In the future we should probably do something to keep these interfaces up to date, @coderabbitai please draft an issue
There was a problem hiding this comment.
@mempirate, I've created an issue to track keeping the interfaces up to date: Issue #341.
🔗 Analysis chain
@mempirate, I've drafted an issue to keep these interfaces up to date.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to track keeping interfaces up to date
gh issue create \
--title "Maintain Up-to-Date Interfaces in Codebase" \
--body "As discussed in [this comment](https://github.com/chainbound/bolt/pull/340#discussion_r1821409808) by @mempirate on [PR #340](https://github.com/chainbound/bolt/pull/340), we should implement a process to ensure that interfaces remain up to date." \
--assignee mempirate
Length of output: 393
✏️ Learnings added
Learnt from: mempirate
PR: chainbound/bolt#340
File: bolt-sidecar/src/onchain_registry.rs:143-143
Timestamp: 2024-10-29T19:21:54.868Z
Learning: Ensure that interfaces in the codebase are kept up to date by implementing a process to maintain them.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@mempirate, happy to help!
(_/)
(^.^)
(")(")
|
@coderabbitai review |
WalkthroughThis pull request introduces several enhancements across multiple files, primarily focusing on the 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
- bolt-sidecar/src/crypto/ecdsa.rs (2 hunks)
- bolt-sidecar/src/driver.rs (2 hunks)
- bolt-sidecar/src/lib.rs (1 hunks)
- bolt-sidecar/src/onchain_registry.rs (1 hunks)
- bolt-sidecar/src/signer/commit_boost.rs (2 hunks)
- bolt-sidecar/src/signer/keystore.rs (2 hunks)
- bolt-sidecar/src/state/consensus.rs (4 hunks)
🔇 Additional comments (12)
bolt-sidecar/src/lib.rs (1)
51-52: Verify module documentation.
The module addition and export look good and align with the PR objectives for Bolt registry integration. However, ensure that the onchain_registry module has proper documentation.
✅ Verification successful
Module documentation is present and comprehensive
The onchain_registry module has proper documentation. The main BoltManager struct is well-documented with a clear description, and its methods have detailed doc comments explaining their functionality. The documentation covers the purpose and behavior of key components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the onchain_registry module has documentation
# Expect: Module documentation should be present
rg -U "(?s)/// .*\nmod onchain_registry" "bolt-sidecar/src/onchain_registry.rs"
Length of output: 79
Script:
#!/bin/bash
# Let's check the actual content of the onchain_registry.rs file
cat bolt-sidecar/src/onchain_registry.rs
Length of output: 6051
bolt-sidecar/src/crypto/ecdsa.rs (2)
3-6: LGTM: Clean import organization
The imports are well-organized and properly scoped for the new Address type requirement.
71-74: LGTM: Clean implementation with proper delegation
The implementation correctly delegates to the existing address() method. Let's verify the address format consistency.
bolt-sidecar/src/signer/commit_boost.rs (1)
3-3: LGTM: Import addition is appropriate.
The addition of Address from alloy::primitives aligns with the new functionality.
bolt-sidecar/src/state/consensus.rs (4)
120-121: LGTM: Improved readability of deadline check.
The condition is now more readable while maintaining the correct logic for checking if the request is within the commitment deadline.
204-206: LGTM: Improved readability of slot calculation.
The furthest slot calculation is now more readable with proper line breaks.
328-329: LGTM: Improved readability of epoch calculation.
The epoch calculation from slot is now more readable with proper line breaks.
164-165: 🧹 Nitpick (assertive)
Consider implementing the registry integration.
There's a TODO comment about registry integration in validate_request. Since this PR is about integrating with the Bolt registry, we should implement this check.
Let's verify if there are any other TODO comments about registry integration:
Would you like help implementing the registry integration for validator verification?
bolt-sidecar/src/signer/keystore.rs (2)
206-207: LGTM!
The comment reformatting improves readability while maintaining clarity.
305-306: LGTM!
The comment reformatting improves readability while maintaining clarity.
bolt-sidecar/src/driver.rs (2)
33-34: New dependencies are correctly imported
The added imports (BoltManager, BuilderProxyConfig, CommitBoostSigner, etc.) are appropriate and correspond to the new functionality introduced.
168-171: Verify the association between operator and validators
As previously noted, it's important to verify that the provided operator is active for the specified validators to ensure correct linkage and system integrity.
…alidators' authorized operator
Etherscan links usually does some checks like user agent for dos protection, to avoid that we should call the api with a proper API key
thedevbirb
force-pushed
the
lore/feat/check-pubkeys-registry
branch
from
2992eb5 to
5e8bb1f
Compare
ValidatorDoesNotExist now returns also the public key hash
…since getProposerStatuses does that already
thedevbirb
force-pushed
the
lore/feat/check-pubkeys-registry
branch
from
440775e to
4c0c9ee
Compare
thedevbirb
force-pushed
the
lore/feat/check-pubkeys-registry
branch
from
4c0c9ee to
abd3843
Compare
thedevbirb
force-pushed
the
lore/feat/check-pubkeys-registry
branch
from
5a89cf9 to
bc54408
Compare
thedevbirb
force-pushed
the
lore/feat/check-pubkeys-registry
branch
from
bc54408 to
c589453
Compare
Closes #332.
Introduces new checks for both the commitment signer key and the validator public keys which should correspond to an active operator and validator respectively in the
BoltManagercontract.The check is performed at startup, when creating the
SidecarDriver::from_components. This is a good spot because both signers (ECDSA, BLS) are abstracted via traits so the check can be safely done in a single pace.Summary by CodeRabbit
New Features
public_keymethod in theSignerECDSAtrait and its implementations, allowing retrieval of the public key as an address.BoltManagerfor enhanced key verification in theSidecarDriver.onchain_registrymodule, expanding on-chain management capabilities.BoltManagerfor address retrieval and operator/validator verification.Bug Fixes
Documentation
make_pathandtest_keystore_signerfunctions.