feat: add onchain delegation #24
Conversation
WalkthroughThe pull request introduces several changes to the Changes
Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (17)
crates/api/src/delegation/error.rs (1)
3-6: Consider using more specific error types.Instead of using String for error details, consider creating more specific error types or using underlying error types directly. This would provide better type safety and more structured error handling.
Example improvement:
pub enum Error { - #[error("error getting preconfer election: {0}")] - GetPreconferElection(String), + #[error("error getting preconfer election: {0}")] + GetPreconferElection(#[from] ContractError), - #[error("error parsing bls public key: {0}")] - BlsPublicKey(String), + #[error("error parsing bls public key: {0}")] + BlsPublicKey(#[from] BlsError), }crates/api/src/delegation/traits.rs (2)
7-11: Add documentation for the trait and method.While the implementation looks solid, it would benefit from documentation explaining:
- The purpose and responsibility of the
DelegationTrait- The expected behavior of
get_preconfer_election- The meaning of a None result vs Some result
- The conditions under which errors might occur
Consider adding documentation like this:
#[async_trait] #[auto_impl::auto_impl(Arc)] +/// Trait for handling delegation-related operations, particularly pre-conference election data. pub trait DelegationTrait: Send + Sync + Clone { + /// Retrieves the pre-conference election data for a validator at a specific slot. + /// + /// # Arguments + /// * `validator_pubkey` - The BLS public key of the validator + /// * `slot` - The slot number to query + /// + /// # Returns + /// * `Ok(Some(election))` - If election data exists for the validator at the slot + /// * `Ok(None)` - If no election data exists + /// * `Err(Error)` - If there was an error fetching the data async fn get_preconfer_election(&self, validator_pubkey: &BlsPublicKey, slot: u64) -> Result<Option<SignedPreconferElection>, Error>;
7-11: Consider caching for performance optimization.Since this trait will be used for blockchain interactions:
- Consider implementing a caching layer to reduce blockchain calls
- Think about cache invalidation strategy based on slot numbers
- Consider adding a bulk query method if multiple validators need to be checked frequently
This would help optimize performance, especially if the same validator/slot combinations are queried repeatedly.
crates/api/src/delegation/mock_delegation.rs (2)
7-8: Add documentation comments to explain the mock's purpose.Consider adding documentation comments to explain that this is a mock implementation for testing purposes and how it should be used.
+/// A mock implementation of the DelegationTrait for testing purposes. +/// This implementation always returns None for get_preconfer_election queries. #[derive(Clone, Default)] pub struct MockDelegation {}
10-15: Consider enhancing the mock implementation for better testing capabilities.While the current implementation is sufficient for basic testing, consider these improvements:
- Add test coverage for the mock itself
- Make the mock configurable to return different results for different test scenarios
Here's a suggested enhancement:
#[derive(Clone, Default)] -pub struct MockDelegation {} +pub struct MockDelegation { + // Allow configuring responses for specific validator keys and slots + responses: std::collections::HashMap<(BlsPublicKey, u64), SignedPreconferElection>, +} #[async_trait] impl DelegationTrait for MockDelegation { async fn get_preconfer_election(&self, _validator_pubkey: &BlsPublicKey, _slot: u64) -> Result<Option<SignedPreconferElection>, Error> { - Ok(None) + Ok(self.responses.get(&(_validator_pubkey.clone(), _slot)).cloned()) } } + +impl MockDelegation { + /// Sets the response for a specific validator and slot + pub fn set_response(&mut self, validator: BlsPublicKey, slot: u64, election: SignedPreconferElection) { + self.responses.insert((validator, slot), election); + } +}This enhancement would allow tests to configure specific responses for different scenarios.
crates/api/Cargo.toml (2)
43-46: Consider using version ranges for Alloy dependencies.The dependencies are correctly added and use recent versions. However, consider using version ranges to allow for non-breaking updates:
-alloy-sol-types = "0.8.8" -alloy-contract ="0.5.2" -alloy-provider = { version = "0.5.2", features = ["reqwest"], default-features = false } -alloy-transport = "0.5.2" +alloy-sol-types = "^0.8.8" +alloy-contract ="^0.5.2" +alloy-provider = { version = "^0.5.2", features = ["reqwest"], default-features = false } +alloy-transport = "^0.5.2"
45-45: Document the rationale for feature flag choices.For
alloy-provider, you've disabled default features and only enabledreqwest. Consider adding a comment explaining why this specific configuration was chosen to help future maintainers understand the decision.-alloy-provider = { version = "0.5.2", features = ["reqwest"], default-features = false } +# Only reqwest transport is needed for our use case, excluding other transport options +alloy-provider = { version = "0.5.2", features = ["reqwest"], default-features = false }crates/utils/src/signing.rs (3)
117-117: Remove commented-out code.Instead of commenting out the unused constant, it should be removed entirely. Commented-out code can lead to confusion and maintenance overhead.
- // const SLOT_NUMBER: u64 = 836343;
130-134: LGTM! Consider adding more test cases.The test initialization is well structured with proper field assignments and real-world test data. However, consider adding more test cases to cover:
- Invalid signatures
- Different chain IDs
- Edge cases for gas limits
Line range hint
1-142: Consider adding documentation for delegation-related security implications.Since this code handles cryptographic signing for the new onchain delegation feature:
- Consider adding documentation explaining the security model and trust assumptions for delegation message signing.
- Document how the domain computation relates to the delegation mechanism to help future maintainers understand the security boundaries.
crates/api/src/service.rs (1)
Line range hint
145-163: Consider operational aspects of execution client dependency.The addition of an execution client dependency introduces a new critical component to the system. Consider:
- Adding metrics/monitoring for execution client connectivity
- Implementing retry logic for temporary connection issues
- Documenting fallback procedures for execution client outages
- Adding health checks for the delegation contract
crates/common/src/api/constraints_api.rs (1)
48-49: LGTM! Consider adding a getter method for consistency.The addition of
proposer_pubkeyfield is well-documented and aligns with the onchain delegation feature. However, for consistency with other fields that have getter methods (preconfer_public_key(),slot(), etc.), consider adding a getter method forproposer_pubkey.Add this getter method to the
PreconferElectionimpl block:+ pub fn proposer_public_key(&self) -> &BlsPublicKey { + &self.proposer_pubkey + }crates/api/src/delegation/contract_delegation.rs (4)
56-56: Optimize conversion by avoiding unnecessary allocationThe current code converts
validator_pubkeyto aVec<u8>, which may incur an unnecessary allocation. Consider passing the slice directly toBytes::fromto improve performance.Apply this diff to optimize the conversion:
-let pubkey = Bytes::from(validator_pubkey.as_ref().to_vec()); +let pubkey = Bytes::from(validator_pubkey.as_ref());
57-57: Remove unnecessary use ofArcwhen creating the contract instanceSince
taiyi_core_contractis only used within this function and not shared across threads, wrapping it in anArcis unnecessary. RemovingArcsimplifies the code without affecting functionality.Apply this diff to remove
Arc:-let taiyi_core_contract = Arc::new(TaiyiCoreInstance::new(self.delegation_contract_address.clone(), self.provider.clone())); +let taiyi_core_contract = TaiyiCoreInstance::new(self.delegation_contract_address.clone(), self.provider.clone());
59-70: Improve readability by destructuring the contract call resultCurrently, you're accessing fields using
preconfer_election._0, which can be less readable. Consider destructuring the result to access the fields directly, enhancing code clarity.Apply this diff to destructure the result:
-let preconfer_election = - taiyi_core_contract.getPreconferElection(pubkey).call().await.map_err(|e| Error::GetPreconferElection(e.to_string()))?; -if preconfer_election._0.preconferAddress.is_zero() { +let election_res = + taiyi_core_contract + .getPreconferElection(pubkey) + .call() + .await + .map_err(|e| Error::GetPreconferElection(e.to_string()))?; +if election_res.preconferAddress.is_zero() {This allows you to access the fields directly without the
_0prefix.
67-69: Provide more descriptive error context when parsing BLS public keysWhen converting bytes to
BlsPublicKey, adding context to the error messages can aid in debugging.Apply this diff to enhance error messages:
-.map_err(|e| Error::BlsPublicKey(e.to_string()))?, +.map_err(|e| Error::BlsPublicKey(format!("Failed to parse preconfer_pubkey: {}", e)))?,Repeat a similar change for
proposer_pubkey.crates/api/src/proposer/api.rs (1)
794-830: Consider removing the commented out code.The
elect_preconferfunction has been commented out, indicating that it is no longer in use. Consider removing the commented out code to improve code clarity and maintainability.Apply this diff to remove the commented out code:
- // pub async fn elect_preconfer( - // Extension(proposer_api): Extension<Arc<ProposerApi<A, DB, M, G>>>, - // req: Request<Body>, - // ) -> Result<StatusCode, ProposerApiError> { - // let request_id = Uuid::new_v4(); - // let mut trace = ElectGatewayTrace { receive: get_nanos_timestamp()?, ..Default::default() }; - - // // Deserialise request - // let mut election_req: SignedPreconferElection = deserialize_json_request_bytes(req, MAX_GATEWAY_ELECTION_SIZE).await?; - // trace.deserialize = get_nanos_timestamp()?; - - // let slot_info = proposer_api.curr_slot_info.read().await; - // info!( - // request_id = %request_id, - // event = "elect_gateway", - // head_slot = slot_info.slot, - // request_ts = trace.receive, - // slot = %election_req.slot(), - // preconfer_public_key = ?election_req.preconfer_public_key(), - // gas_limit = election_req.gas_limit(), - // chain_id = election_req.chain_id(), - // ); - - // if let Err(err) = proposer_api.validate_election_request(&mut election_req, slot_info.slot).await { - // warn!(request_id = %request_id, ?err, "validation failed"); - // return Err(err); - // } - // trace.validation_complete = get_nanos_timestamp()?; - - // // Save to constraints datastore - // // TODO: database - // proposer_api.auctioneer.save_new_gateway_election(&election_req, election_req.slot()).await?; - // trace.gateway_election_saved = get_nanos_timestamp()?; - - // info!(%request_id, ?trace, "gateway elected"); - // Ok(StatusCode::OK) - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
crates/api/Cargo.toml(1 hunks)crates/api/src/constraints/api.rs(1 hunks)crates/api/src/delegation/contract_delegation.rs(1 hunks)crates/api/src/delegation/error.rs(1 hunks)crates/api/src/delegation/mock_delegation.rs(1 hunks)crates/api/src/delegation/mod.rs(1 hunks)crates/api/src/delegation/traits.rs(1 hunks)crates/api/src/lib.rs(1 hunks)crates/api/src/proposer/api.rs(15 hunks)crates/api/src/proposer/tests.rs(4 hunks)crates/api/src/router.rs(3 hunks)crates/api/src/service.rs(4 hunks)crates/api/src/test_utils.rs(4 hunks)crates/common/src/api/constraints_api.rs(2 hunks)crates/common/src/api/mod.rs(1 hunks)crates/common/src/config.rs(7 hunks)crates/utils/src/signing.rs(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- crates/api/src/constraints/api.rs
- crates/api/src/delegation/mod.rs
- crates/common/src/api/mod.rs
🔇 Additional comments (33)
crates/api/src/delegation/error.rs (2)
1-7: LGTM! The error handling structure is well-organized.
The error enum is well-designed with clear error messages and appropriate trait derivations.
1-7: Review error message content for security.
Ensure that the string error messages passed to these variants don't contain sensitive information like private keys, internal states, or implementation details that could be exploited.
✅ Verification successful
Let me analyze the usage of these error variants to verify if any sensitive information is being exposed.
Error handling appears secure - no sensitive data exposure
The error handling implementation is secure:
GetPreconferElectiononly wraps contract call errors usinge.to_string(), which provides standard error messages from the contract interactionBlsPublicKeyerrors come from public key parsing/validation failures usingtry_from, containing only validation error messages- Neither error variant exposes private keys, internal states or implementation details that could be exploited
- The error messages are appropriately generic while still being useful for debugging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential sensitive information in error messages
rg -g '!target/' -g '!*.lock' -A 3 'GetPreconferElection\(|BlsPublicKey\('
Length of output: 1452
Script:
#!/bin/bash
# Look for the actual error sources being passed to these variants
ast-grep --pattern 'Error::GetPreconferElection($_)'
ast-grep --pattern 'Error::BlsPublicKey($_)'
# Also check the contract call to see full context
ast-grep --pattern 'getPreconferElection($_)'
Length of output: 579
crates/api/src/lib.rs (1)
5-5: LGTM! Module declaration follows good practices.
The new delegation module is properly declared and follows the existing module organization pattern.
Let's verify the module structure is complete:
✅ Verification successful
Module structure is complete and well-organized
All expected files are present in the delegation module:
contract_delegation.rs: Main contract delegation implementationerror.rs: Error handlingmock_delegation.rs: Mock implementationstraits.rs: Trait definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the delegation module structure matches the AI summary
# Expected files: contract_delegation.rs, error.rs, mock_delegation.rs, traits.rs
# Check if all expected files exist in the delegation module
echo "Verifying delegation module structure..."
fd --type f "^(contract_delegation|error|mock_delegation|traits)\.rs$" "crates/api/src/delegation"
Length of output: 354
crates/api/src/delegation/traits.rs (1)
1-5: LGTM! Well-organized imports with clear dependencies.
The imports are properly organized and show good separation of concerns, particularly with error handling in a separate module.
crates/api/src/delegation/mock_delegation.rs (2)
1-5: LGTM! Imports are well-organized and necessary.
The imports are properly organized following Rust conventions, with external crates first followed by local imports.
1-15: Verify test coverage using this mock implementation.
Let's verify how this mock is being used in tests across the codebase.
✅ Verification successful
MockDelegation is properly integrated and used in tests
The mock implementation is well integrated into the codebase:
- Used in test utilities (
test_utils.rs) for setting up test API servers - Properly integrated in proposer API tests (
proposer/tests.rs) - Returns
Noneby default which is a safe mock behavior for testing - Implements the required
DelegationTraitcorrectly
The mock is used consistently across test scenarios, particularly in:
- API server initialization for testing
- Proposer API validation tests
- Integration with other mock components (MockAuctioneer, MockDatabaseService, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find test files using MockDelegation
# Expected: Test files showing usage of this mock
# Search for test modules using MockDelegation
rg -l "use.*MockDelegation" --type rust | grep "_test\|test_\|tests"
# Search for direct usage in test functions
rg "fn.*test.*MockDelegation" --type rust
Length of output: 114
Script:
#!/bin/bash
# Let's try a broader search to find any references to MockDelegation
rg "MockDelegation" -A 5 -B 5 --type rust
# Also search for test files in the delegation module
fd "test.*\.rs$" -p "crates/api/src/delegation/"
# Look for any test modules in the same file
rg "mod test" "crates/api/src/delegation/mock_delegation.rs"
Length of output: 12514
crates/api/src/router.rs (2)
34-35: LGTM: Clean integration of ContractDelegation
The addition of ContractDelegation as a type parameter to ProposerApiProd aligns with the PR's objective of implementing on-chain delegation.
96-98: Verify the impact of removing ElectPreconfer route
The ElectPreconfer route has been commented out. This could potentially break API consumers who depend on this endpoint.
Let's verify if this endpoint is still in use:
Please ensure:
- API consumers are notified of this breaking change
- Documentation is updated to reflect the removal
- Consider adding a deprecation notice if this needs to be phased out gradually
crates/api/src/test_utils.rs (6)
24-24: LGTM: Import added correctly.
The MockDelegation import follows the established pattern for mock implementations.
56-68: LGTM: Route handlers updated consistently.
All ProposerApi route handlers have been properly updated to include the MockDelegation type parameter while maintaining the original functionality.
117-117: LGTM: ProposerApi type and instantiation updated correctly in proposer_api_app().
The changes maintain consistency with the updates in the app() function:
- Return type properly includes
MockDelegation - All parameters are correctly wrapped in
Arc - Parameter ordering and initialization remain consistent
Also applies to: 124-137
142-151: LGTM: Route handlers in proposer_api_app() updated consistently.
The route handlers are properly updated to include MockDelegation while maintaining consistency with the changes in the app() function.
Line range hint 1-179: Verify test coverage for delegation functionality.
While the mock setup is complete, it would be beneficial to verify that the test coverage adequately exercises the new delegation functionality.
Let's check the test coverage:
#!/bin/bash
# Description: Find and analyze test cases for delegation functionality
# Expected: Locate test files and verify coverage of delegation features
# Search for test files that might contain delegation tests
rg -l "test.*delegation" --type rust
# Search for delegation-related test cases
rg "fn.*test.*delegation" -A 5 --type rustLine range hint 37-48: LGTM: ProposerApi instantiation updated correctly.
The MockDelegation is properly integrated with thread-safe Arc wrapper and follows the pattern of other mock objects.
Let's verify the MockDelegation implementation:
✅ Verification successful
MockDelegation implementation is suitable for testing purposes
The implementation is correct and appropriate for testing:
- It implements the required
DelegationTraitwith the single methodget_preconfer_election - The mock returns
Ok(None)which is a valid default behavior for testing scenarios where delegation is not needed - The implementation follows Rust's async trait patterns with proper error handling
- It maintains consistency with other mock objects in the codebase by implementing
default()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MockDelegation's default implementation
# Expected: Find the MockDelegation implementation to ensure it provides necessary test functionality
# Search for MockDelegation implementation
rg -A 10 "impl.*MockDelegation"
Length of output: 83
Script:
#!/bin/bash
# Let's try different search patterns to locate MockDelegation
# First, find the direct definition
rg "struct MockDelegation" --type rust -A 5
# Also search for the mock module where it might be defined
rg "mod mock" --type rust -A 10
# Search for any delegation-related mocks
rg "Mock.*Delegation" --type rust -A 5
# Try finding test files that might contain the mock
fd "test" --type f --extension rs --exec rg "MockDelegation" {}
Length of output: 14309
Script:
#!/bin/bash
# Let's examine the DelegationTrait to understand what the mock should implement
rg "trait DelegationTrait" -A 10 --type rust
# Also check if there are any tests using MockDelegation to verify its behavior
rg "fn.*test.*delegation" -A 10 --type rust
Length of output: 389
crates/api/src/service.rs (2)
3-3: LGTM: Import changes align with onchain delegation feature.
The new imports for ProviderBuilder and ContractDelegation are appropriately placed and support the onchain delegation functionality.
Also applies to: 22-22
163-163: LGTM: ProposerApi correctly initialized with delegation contract.
The delegation contract is properly passed to the ProposerApi constructor.
crates/common/src/config.rs (2)
181-181: Consider cleaning up the commented ElectPreconfer route.
The ElectPreconfer route is commented out in multiple places. If this route is being permanently removed:
- We should remove the commented code rather than leaving it
- This change might affect existing functionality
If this is a temporary change, please add a TODO comment explaining why it's commented out and when it will be re-enabled.
Let's check for any remaining references to this route:
#!/bin/bash
# Description: Check for any remaining references to ElectPreconfer
# Test: Look for any usage of ElectPreconfer that might be affected
rg "ElectPreconfer|elect_preconfer" --type rustAlso applies to: 190-190, 261-261, 280-280
109-118:
Review the safety of using Address::default() for delegation contract.
The OnchainDelegationConfig derives Default, which means delegation_contract_address will be initialized to the zero address (0x0000...0000). This could be dangerous if the code doesn't properly handle this case.
Consider:
- Explicitly implementing
Defaultwith a more appropriate default value - Adding runtime validation to ensure a non-zero address is provided
Let's verify how the contract address is used:
#!/bin/bash
# Description: Check how the delegation contract address is used
# Test: Look for usage of delegation_contract_address to ensure proper validation
rg "delegation_contract_address" --type rust -A 5crates/api/src/delegation/contract_delegation.rs (1)
74-74: Confirm if using a default signature is appropriate
The signature field is set to BlsSignature::default(). Verify whether this is intentional or if the signature should be obtained from a valid source.
crates/api/src/proposer/tests.rs (3)
73-73: Addition of MockDelegation Import
The import delegation::mock_delegation::MockDelegation is correctly added to support mock delegation in tests.
Line range hint 208-212: Update Return Type to Include MockDelegation
The return type of start_api_server now incorporates MockDelegation within the ProposerApi generic parameters. This change seamlessly integrates delegation functionality into the test API server.
Line range hint 829-840: Initialization of ProposerApi with MockDelegation
ProposerApi is instantiated with MockDelegation, ensuring that delegation logic is included in the tests. The initialization appears correct and follows the expected pattern.
crates/api/src/proposer/api.rs (11)
34-34: LGTM!
The import statement for traces::constraints_api::SetConstraintsTrace is valid and necessary for the set_constraints function.
42-42: LGTM!
The import statement for signing::{verify_signed_builder_message, verify_signed_consensus_message} is valid and necessary for signature verification.
54-55: LGTM!
The import statements for constraints::{api::MAX_SET_CONSTRAINTS_SIZE, SET_CONSTRAINTS_CUTOFF_NS} and delegation::traits::DelegationTrait are valid and necessary for the set_constraints function and the new DelegationTrait generic parameter.
Line range hint 156-173: LGTM!
The changes to the status, register_validators, get_header, get_payload, and set_constraints functions to include the new generic parameter D in the Extension type are valid and consistent with the updated ProposerApi struct.
455-455: LGTM!
The change to the get_payload function to include the new generic parameter D in the Extension type is valid and consistent with the updated ProposerApi struct.
734-734: LGTM!
The change to the set_constraints function to include the new generic parameter D in the Extension type is valid and consistent with the updated ProposerApi struct.
878-884: LGTM!
The changes to the helper functions to include the new generic parameter D constrained by the DelegationTrait are valid and consistent with the updated ProposerApi struct.
1236-1242: LGTM!
The changes to the state sync functions to include the new generic parameter D constrained by the DelegationTrait are valid and consistent with the updated ProposerApi struct.
1276-1298: Verify the integration of the DelegationTrait in the codebase.
The handle_new_slot function has been updated to integrate the DelegationTrait for retrieving preconfer elections. This change is valid and allows for the integration of delegation functionality.
Run the following script to verify the usage of the DelegationTrait:
#!/bin/bash
# Description: Verify the usage of the `DelegationTrait` in the `handle_new_slot` function.
# Test: Search for the `get_preconfer_election` function usage.
# Expect: The function should be called on the `delegation_contract` field to retrieve preconfer elections.
rg -A 10 $'self.delegation_contract.get_preconfer_election'74-80: Verify the new generic parameter D in the codebase.
The ProposerApi struct has been updated to include a new generic parameter D constrained by the DelegationTrait. This change is valid and allows for the integration of delegation functionality.
Run the following script to verify the usage of the new generic parameter:
Also applies to: 97-97
✅ Verification successful
The new generic parameter D is properly integrated throughout the codebase
The verification shows that the new generic parameter D with DelegationTrait constraint is:
- Correctly used in the production code with
ContractDelegationimplementation (crates/api/src/router.rs) - Properly handled in test code with
MockDelegationimplementation (crates/api/src/test_utils.rs,crates/api/src/proposer/tests.rs) - Consistently used across all
ProposerApiimplementations and method signatures incrates/api/src/proposer/api.rs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new generic parameter `D` in the `ProposerApi` struct.
# Test: Search for the `ProposerApi` struct definition and instantiation.
# Expect: The struct definition and instantiation should include the new generic parameter `D`.
rg -A 5 $'pub struct ProposerApi'
rg -A 5 $'ProposerApi::new'
Length of output: 427
Script:
#!/bin/bash
# Let's search for more comprehensive information about ProposerApi usage
# Search for any implementation blocks for ProposerApi
ast-grep --pattern 'impl$$$ProposerApi<$$$>$$$'
# Search for any struct fields or method parameters using ProposerApi
rg -l "ProposerApi"
# Search for where ProposerApi is instantiated or type parameters are specified
rg "ProposerApi\s*<" -A 3
Length of output: 4323
100-106: Verify the new delegation_contract field in the codebase.
The ProposerApi struct has been updated to include a new field delegation_contract of type Arc<D>. This change is valid and allows for the integration of delegation functionality.
Run the following script to verify the usage of the new field:
Also applies to: 119-119, 132-132
✅ Verification successful
Let me gather more information about the delegation contract field to verify its complete implementation.
Let me search for more specific implementation details of the delegation contract usage.
The delegation_contract field is properly integrated and used in the codebase
The verification shows that:
- The field is correctly declared in the
ProposerApistruct - It's properly initialized in the
newconstructor method - It's actively used in the codebase to get preconfer election data via
get_preconfer_electionmethod - The implementation follows the expected pattern with proper error handling and logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new `delegation_contract` field in the `ProposerApi` struct.
# Test: Search for the `delegation_contract` field usage.
# Expect: The field should be initialized in the `ProposerApi::new` function and used in other parts of the codebase.
rg -A 5 $'delegation_contract'
Length of output: 1137
Script:
#!/bin/bash
# Search for the ProposerApi struct definition and its new method
ast-grep --pattern 'struct ProposerApi {
$$$
}'
# Search for the initialization of delegation_contract in new method
ast-grep --pattern 'impl$$ ProposerApi$$ {
$$$
pub fn new($$$) -> Self {
$$$
}
$$$
}'
# Search for any other usages of delegation_contract
rg -A 3 "delegation_contract:"
Length of output: 632
Script:
#!/bin/bash
# Search for the DelegationTrait implementation and interface
ast-grep --pattern 'trait DelegationTrait {
$$$
}'
# Search for any async functions using delegation_contract
rg -A 5 "self.delegation_contract" --type rust
# Search for the complete new method implementation
rg -A 15 "pub fn new\(" --type rust crates/api/src/proposer/api.rs
Length of output: 2107
zsluedem
force-pushed
the
onchain-delegation
branch
2 times, most recently
from
bf84d3f to
d39396f
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
crates/api/src/test_utils.rs (2)
117-137: Consider extracting common test configurationThe ProposerApi initialization contains a lot of mock setup code that's duplicated from the app() function. Consider extracting this into a helper function to reduce duplication and improve maintainability.
+fn create_test_proposer_api( + auctioneer: Arc<MockAuctioneer>, + slot_update_sender: Sender<Sender<ChainUpdate>>, + gossip_receiver: Receiver<GossipedMessage>, +) -> Arc<ProposerApi<MockAuctioneer, MockDatabaseService, MockMultiBeaconClient, MockGossiper, MockDelegation>> { + Arc::new(ProposerApi::new( + auctioneer, + Arc::new(MockDatabaseService::default()), + Arc::new(MockGossiper::new().unwrap()), + vec![Arc::new(BlockBroadcaster::Mock(MockBlockBroadcaster::default()))], + Arc::new(MockMultiBeaconClient::default()), + Arc::new(ChainInfo::for_mainnet()), + slot_update_sender, + Arc::new(ValidatorPreferences::default()), + 0, + gossip_receiver, + Arc::new(MockDelegation::default()), + )) +}
Missing test coverage for MockDelegation implementation
The codebase analysis reveals:
- MockDelegation is a mock implementation of DelegationTrait with a single method
get_preconfer_election- There are no dedicated test modules or test cases in the delegation directory
- The mock is only used as a dependency in ProposerApi tests without verifying its behavior
The following areas need test coverage:
- Test module for mock_delegation.rs to verify the default behavior
- Test cases for
get_preconfer_electionmethod returning None- Integration tests verifying MockDelegation's interaction with ProposerApi
🔗 Analysis chain
Line range hint
1-179: Verify test coverage for delegation functionalitySince this is a test utilities file, we should ensure that the mock delegation functionality is properly tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for test modules that exercise the delegation functionality rg -p "mod tests" -A 10 "use.*MockDelegation" # Look for actual test cases rg -p "#\[test\]" -A 20 "MockDelegation"Length of output: 203
Script:
#!/bin/bash # First, let's find where MockDelegation is defined rg -p "struct MockDelegation" # Then look for any tests involving MockDelegation rg -p "MockDelegation" --type rust -C 5 # Look for any test files that might use MockDelegation fd -e rs "test" --exec grep -l "MockDelegation" {}Length of output: 8232
Script:
#!/bin/bash # Let's examine the DelegationTrait implementation and its test coverage rg -p "impl.*DelegationTrait" -A 10 # Look for any test modules in the delegation directory rg -p "mod tests" --type rust ./crates/api/src/delegation/ # Check if there are any test functions that verify the delegation functionality ast-grep --pattern 'async fn test_$_() { $$$ MockDelegation $$$ }'Length of output: 1285
crates/common/src/config.rs (1)
103-113: Consider extracting the default URL into a constantThe default URL
http://localhost:8545should be extracted into a constant for better maintainability and reusability.+const DEFAULT_EXECUTION_CLIENT_URL: &str = "http://localhost:8545"; impl Default for ExecutionClientConfig { fn default() -> Self { - Self { url: Url::parse("http://localhost:8545").unwrap() } + Self { url: Url::parse(DEFAULT_EXECUTION_CLIENT_URL).unwrap() } } }crates/api/src/proposer/tests.rs (1)
Line range hint
829-840: Consider adding test coverage for delegation functionality.While the
MockDelegationis properly integrated into theProposerApiconstructor, there are no specific test cases validating the delegation behavior.Would you like me to help generate test cases to cover the delegation functionality?
crates/api/src/delegation/contract_delegation.rs (4)
56-56: Optimize conversion ofvalidator_pubkeytoBytesThe conversion of
validator_pubkeytoBytescan be simplified to avoid unnecessary allocations.Apply this diff to optimize the conversion:
-let pubkey = Bytes::from(validator_pubkey.as_ref().to_vec()); +let pubkey = Bytes::copy_from_slice(validator_pubkey.as_ref());This change uses
Bytes::copy_from_slice, which is more efficient than creating an intermediateVec<u8>.
44-44: Avoid unnecessary cloning ofproviderCloning the
providermay be unnecessary if it's already reference-counted or cheaply clonable.Consider passing the
providerby reference or using anArcto avoid unnecessary clones.- provider: RecommendProvider, + provider: Arc<RecommendProvider>,Update the constructor accordingly:
-pub fn new(delegation_contract_address: Address, provider: RecommendProvider) -> Self { +pub fn new(delegation_contract_address: Address, provider: Arc<RecommendProvider>) -> Self {
64-73: Improve error context intry_fromconversionsWhen handling errors from
BlsPublicKey::try_from, include contextual information to aid debugging.Apply this diff to enhance error messages:
.map_err(|e| Error::BlsPublicKey(format!("Failed to parse preconfer_pubkey: {}", e)))?,Repeat for the
proposer_pubkey.
17-17: Consider makingDEFAULT_GAS_LIMITa configurable parameterMaking
DEFAULT_GAS_LIMITconfigurable enhances flexibility and allows adjustment without code changes.crates/api/src/proposer/api.rs (2)
1276-1298: Enhance error handling when fetching preconfer electionsIn the
handle_new_slotmethod, while fetching preconfer elections from the delegation contract, consider the following improvements:
- Retry Mechanism: Implement a retry mechanism for transient errors when calling
get_preconfer_electionandsave_new_gateway_electionto handle temporary network issues or timeouts.- Error Classification: Differentiate between recoverable and unrecoverable errors to decide when to retry or fail gracefully.
- Logging Enhancements: Include additional context in log messages, such as the error type or code, to facilitate easier debugging and monitoring.
794-830: Remove or document commented-out code in elect_preconfer functionThe
elect_preconferfunction is commented out entirely. If this code is deprecated or no longer needed, consider removing it to clean up the codebase. If it's intended for future implementation, add a comment explaining its purpose and any pending dependencies or reasons for its current state.
🛑 Comments failed to post (4)
crates/common/src/config.rs (2)
181-181: 🛠️ Refactor suggestion
Consider removing commented-out route instead of commenting
The
ElectPreconferroute is commented out in multiple places. If this route is no longer needed, it should be removed entirely rather than commented out. If it's temporary, please add a comment explaining why it's disabled and when it might be re-enabled.- // Route::ElectPreconfer,If this is truly temporary, add a TODO comment explaining why:
+ // TODO: ElectPreconfer route temporarily disabled due to [reason] + // Expected to be re-enabled in [timeline/condition] // Route::ElectPreconfer,Also applies to: 190-190, 261-261, 280-280
115-118:
⚠️ Potential issueVerify the safety of Address default value
The
#[derive(Default)]forOnchainDelegationConfigwill useAddress::default()(likely 0x0) fordelegation_contract_address. This might not be a valid contract address in practice.Consider either:
- Removing
Defaultand requiring explicit configuration- Implementing a custom
Defaultthat panics with a clear message-#[derive(Serialize, Deserialize, Clone, Debug, Default)] +#[derive(Serialize, Deserialize, Clone, Debug)] pub struct OnchainDelegationConfig { pub delegation_contract_address: Address, } + +impl Default for OnchainDelegationConfig { + fn default() -> Self { + panic!("OnchainDelegationConfig requires explicit configuration of delegation_contract_address") + } +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.#[derive(Serialize, Deserialize, Clone, Debug)] pub struct OnchainDelegationConfig { pub delegation_contract_address: Address, } impl Default for OnchainDelegationConfig { fn default() -> Self { panic!("OnchainDelegationConfig requires explicit configuration of delegation_contract_address") } }crates/api/src/delegation/contract_delegation.rs (2)
60-62: 🛠️ Refactor suggestion
Clarify the field access of
preconfer_electionAccessing fields using
._0suggests thatpreconfer_electioncontains unnamed fields. Ensure that the fields are being accessed correctly and consider naming the fields for clarity.Modify the Solidity struct to name the return parameter, which allows for direct field access:
#[sol(rpc)] contract TaiyiCore { #[derive(Debug)] - function getPreconferElection(bytes calldata validatorPubKey) external view returns (PreconferElectionRes memory); + function getPreconferElection(bytes calldata validatorPubKey) external view returns (PreconferElectionRes memory result); }Then, you can access the fields directly:
-if preconfer_election._0.preconferAddress.is_zero() { +if preconfer_election.preconferAddress.is_zero() {Committable suggestion skipped: line range outside the PR's diff.
70-70:
⚠️ Potential issueProperly convert
chainIdto the expected integer typeThe method
.to()onpreconfer_election.chainIdmay not perform the desired conversion. Ensure thatchainIdis converted to the correct Rust integer type, handling potential overflows.Apply this diff to explicitly convert
chainId:-chain_id: preconfer_election.chainId.to(), +chain_id: preconfer_election.chainId.as_u64(),If
chainIdexceedsu64::MAX, handle the error appropriately.Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
crates/api/src/delegation/contract_delegation.rs (4)
17-17: Consider making the gas limit configurable.The hardcoded
DEFAULT_GAS_LIMITmight not be suitable for all network conditions or contract interactions. Consider making it configurable through environment variables or configuration files.-const DEFAULT_GAS_LIMIT: u64 = 100_000; +const DEFAULT_GAS_LIMIT: u64 = std::env::var("DELEGATION_GAS_LIMIT") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(100_000);
19-24: Consider adding documentation for the RecommendProvider type.The complex provider type combines multiple fillers for transaction handling. Adding documentation would help explain its purpose and components.
+/// Provider type that combines multiple fillers for complete transaction handling: +/// - Identity: For authentication +/// - GasFiller: For gas price estimation +/// - BlobGasFiller: For blob gas estimation +/// - NonceFiller: For transaction nonce management +/// - ChainIdFiller: For network identification type RecommendProvider = FillProvider< JoinFill<Identity, JoinFill<GasFiller, JoinFill<BlobGasFiller, JoinFill<NonceFiller, ChainIdFiller>>>>, RootProvider<BoxTransport>, BoxTransport, Ethereum, >;
25-39: Add documentation for the contract interfaces.The smart contract definitions would benefit from documentation explaining their purpose and field meanings.
sol! { + /// Represents the result of a preconfer election query from the TaiyiCore contract #[derive(Debug)] struct PreconferElectionRes { + /// The public key of the validator bytes validatorPubkey; + /// The public key of the preconfer bytes preconferPubkey; + /// The chain ID where this election is valid uint256 chainId; + /// The Ethereum address of the preconfer address preconferAddress; } + /// TaiyiCore contract interface for delegation management #[sol(rpc)] contract TaiyiCore { #[derive(Debug)] + /// Retrieves preconfer election data for a given validator + /// @param validatorPubKey The public key of the validator + /// @return PreconferElectionRes containing the election details function getPreconferElection(bytes calldata validatorPubKey) external view returns (PreconferElectionRes memory); } }
57-57: Unnecessary Arc wrapping.The
Arc::new()wrapping seems unnecessary as the contract instance is only used for a single async call and not shared between threads.- let taiyi_core_contract = Arc::new(TaiyiCoreInstance::new(self.delegation_contract_address, self.provider.clone())); + let taiyi_core_contract = TaiyiCoreInstance::new(self.delegation_contract_address, self.provider.clone());crates/api/src/router.rs (1)
96-98: Consider removing commented code if the route is permanently disabled.Instead of commenting out the
ElectPreconferroute, consider removing it entirely if it's no longer needed. If this is a temporary change, please add a TODO comment explaining the reason and when it might be re-enabled.- // Route::ElectPreconfer => { - // router = router.route(&route.path(), post(ProposerApiProd::elect_preconfer)); - // }crates/api/src/proposer/api.rs (2)
35-35: Nitpick: Consider renamingconstraints_apitoconstraints_tracefor clarity.The module name
constraints_apiis a bit misleading since it contains a trace struct. Renaming it toconstraints_tracewould better convey its purpose.-use helix_common::traces::constraints_api::SetConstraintsTrace; +use helix_common::traces::constraints_trace::SetConstraintsTrace;
793-831: Refactor: Remove the commented outelect_preconferfunction.The
elect_preconferfunction has been commented out, indicating that it is no longer in use. To improve code clarity and maintainability, it is recommended to remove the commented out code entirely.-// /// Elects a gateway to perform pre-confirmations for a validator. The request must be signed by the validator -// /// and must be for the next epoch. -// pub async fn elect_preconfer( -// Extension(proposer_api): Extension<Arc<ProposerApi<A, DB, M, G>>>, -// req: Request<Body>, -// ) -> Result<StatusCode, ProposerApiError> { -// let request_id = Uuid::new_v4(); -// let mut trace = ElectGatewayTrace { receive: get_nanos_timestamp()?, ..Default::default() }; -// -// // Deserialise request -// let mut election_req: SignedPreconferElection = deserialize_json_request_bytes(req, MAX_GATEWAY_ELECTION_SIZE).await?; -// trace.deserialize = get_nanos_timestamp()?; -// -// let slot_info = proposer_api.curr_slot_info.read().await; -// info!( -// request_id = %request_id, -// event = "elect_gateway", -// head_slot = slot_info.slot, -// request_ts = trace.receive, -// slot = %election_req.slot(), -// preconfer_public_key = ?election_req.preconfer_public_key(), -// gas_limit = election_req.gas_limit(), -// chain_id = election_req.chain_id(), -// ); -// -// if let Err(err) = proposer_api.validate_election_request(&mut election_req, slot_info.slot).await { -// warn!(request_id = %request_id, ?err, "validation failed"); -// return Err(err); -// } -// trace.validation_complete = get_nanos_timestamp()?; -// -// // Save to constraints datastore -// // TODO: database -// proposer_api.auctioneer.save_new_gateway_election(&election_req, election_req.slot()).await?; -// trace.gateway_election_saved = get_nanos_timestamp()?; -// -// info!(%request_id, ?trace, "gateway elected"); -// Ok(StatusCode::OK) -// }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
crates/api/Cargo.toml(1 hunks)crates/api/src/constraints/api.rs(1 hunks)crates/api/src/delegation/contract_delegation.rs(1 hunks)crates/api/src/delegation/error.rs(1 hunks)crates/api/src/delegation/mock_delegation.rs(1 hunks)crates/api/src/delegation/mod.rs(1 hunks)crates/api/src/delegation/traits.rs(1 hunks)crates/api/src/lib.rs(1 hunks)crates/api/src/proposer/api.rs(16 hunks)crates/api/src/proposer/tests.rs(4 hunks)crates/api/src/router.rs(3 hunks)crates/api/src/service.rs(4 hunks)crates/api/src/test_utils.rs(4 hunks)crates/common/src/api/constraints_api.rs(2 hunks)crates/common/src/api/mod.rs(1 hunks)crates/common/src/config.rs(7 hunks)crates/utils/src/signing.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/api/src/delegation/error.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/api/Cargo.toml
- crates/api/src/constraints/api.rs
- crates/api/src/delegation/mock_delegation.rs
- crates/api/src/delegation/mod.rs
- crates/api/src/delegation/traits.rs
- crates/api/src/lib.rs
- crates/api/src/proposer/tests.rs
- crates/api/src/test_utils.rs
- crates/common/src/api/constraints_api.rs
- crates/common/src/api/mod.rs
- crates/utils/src/signing.rs
🔇 Additional comments (23)
crates/api/src/router.rs (2)
23-23: LGTM!
The import is correctly placed and necessary for the type definition changes.
34-35: Verify type definition usage across the codebase.
The addition of ContractDelegation as a new type parameter is correct, but we should verify that all instantiations of ProposerApiProd have been updated accordingly.
✅ Verification successful
Let me check the content of service.rs to verify the instantiation is correct.
Let me check the actual implementation of ProposerApi::new to verify the parameter order matches the type definition.
Let me try a different approach to find the ProposerApi implementation.
Let me check the complete constructor implementation to verify all parameters.
The type definition and its usage are correctly aligned
Based on the verification:
- The
ProposerApiProdtype definition inrouter.rscorrectly includesContractDelegationas the fifth type parameter - The only instantiation in
service.rsshowsdelegation_contractis properly created and passed as the last parameter toProposerApiProd::new - The constructor implementation in
proposer/api.rsconfirms the parameter order matches the type definition, withdelegation_contract: Arc<D>as the last parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ProposerApiProd instantiations to ensure they've been updated
rg -l "ProposerApiProd::new|: ProposerApiProd = " --type rust
# Search for any direct uses of ProposerApi that might need the new parameter
ast-grep --pattern 'ProposerApi<$$$>'
Length of output: 129
Script:
#!/bin/bash
# Check the content of service.rs focusing on ProposerApiProd usage
rg "ProposerApiProd" -A 5 -B 5 crates/api/src/service.rs
Length of output: 1090
Script:
#!/bin/bash
# Search for ProposerApi implementation to verify constructor parameters
ast-grep --pattern 'impl$_ ProposerApi<$$$> {
$$$
fn new($$$) {
$$$
}
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Search for ProposerApi implementation with ripgrep
rg "impl.*ProposerApi" -A 10 --type rust
Length of output: 6170
Script:
#!/bin/bash
# Get the complete new function implementation
rg "pub fn new" -A 15 crates/api/src/proposer/api.rs
Length of output: 616
crates/api/src/service.rs (3)
145-149: Previous review comment about provider initialization is still applicable.
The current implementation using expect for error handling needs improvement. Please refer to the previous review comment that suggested adding proper error handling, timeout configuration, and connection testing.
Line range hint 152-163: Verify ProposerApiProd constructor usage.
The addition of the delegation_contract parameter looks correct. Let's ensure all call sites are updated.
#!/bin/bash
# Description: Verify all ProposerApiProd constructor calls are updated
# Test: Search for ProposerApiProd::new calls
ast-grep --pattern 'ProposerApiProd::new($$$)'150-151:
Add validation and error handling for delegation contract initialization.
The current implementation should include:
- Validation of the contract address format
- Verification that the contract exists at the given address
- Error handling for contract initialization failures
Consider this approach:
-let delegation_contract = Arc::new(ContractDelegation::new(config.onchain_delegation.delegation_contract_address, provider));
+let delegation_contract = match ContractDelegation::new(config.onchain_delegation.delegation_contract_address, provider.clone()).await {
+ Ok(contract) => {
+ // Verify contract exists at the address
+ if provider.get_code(config.onchain_delegation.delegation_contract_address, None).await?.is_empty() {
+ error!("No contract found at configured address");
+ return;
+ }
+ Arc::new(contract)
+ },
+ Err(e) => {
+ error!("Failed to initialize delegation contract: {}", e);
+ return;
+ }
+};crates/common/src/config.rs (2)
25-28: LGTM! New configuration fields are properly structured.
The new fields follow good practices with proper #[serde(default)] attributes for backward compatibility.
103-113: LGTM! ExecutionClientConfig is well-implemented.
The implementation follows best practices:
- Proper URL serialization/deserialization
- Sensible default implementation using the standard Ethereum node port
crates/api/src/proposer/api.rs (16)
1-1: LGTM!
The #![allow(clippy::type_complexity)] attribute is appropriate here to suppress the Clippy warning about the complexity of the type definitions in this file.
43-43: LGTM!
The usage of helix_utils::signing functions for verifying signed builder and consensus messages is appropriate.
55-56: LGTM!
The usage of constants MAX_SET_CONSTRAINTS_SIZE and the new DelegationTrait import are appropriate.
97-98: LGTM!
The addition of the delegation_contract field of type Arc<D> to the ProposerApi struct is appropriate, given the new generic parameter D constrained by DelegationTrait.
120-120: LGTM!
The addition of the delegation_contract parameter of type Arc<D> to the ProposerApi::new function is appropriate and consistent with the new generic parameter D and the delegation_contract field in the struct.
133-133: LGTM!
The initialization of the delegation_contract field with the provided delegation_contract parameter in the ProposerApi::new function is correct.
157-157: LGTM!
The addition of the new generic parameter D to the ProposerApi type in the status function signature is appropriate and consistent with the changes made to the ProposerApi struct.
174-174: LGTM!
The addition of the new generic parameter D to the ProposerApi type in the register_validators function signature is appropriate and consistent with the changes made to the ProposerApi struct.
375-375: LGTM!
The addition of the new generic parameter D to the ProposerApi type in the get_header function signature is appropriate and consistent with the changes made to the ProposerApi struct.
456-456: LGTM!
The addition of the new generic parameter D to the ProposerApi type in the get_payload function signature is appropriate and consistent with the changes made to the ProposerApi struct.
735-735: LGTM!
The addition of the new generic parameter D to the ProposerApi type in the set_constraints function signature is appropriate and consistent with the changes made to the ProposerApi struct.
879-885: LGTM!
The addition of the new generic parameter D constrained by DelegationTrait to the impl block for the helper functions of ProposerApi is appropriate and consistent with the changes made to the ProposerApi struct.
1237-1243: LGTM!
The addition of the new generic parameter D constrained by DelegationTrait to the impl block for the state sync functions of ProposerApi is appropriate and consistent with the changes made to the ProposerApi struct.
1277-1299: Verify the usage of delegation_contract in the handle_new_slot function.
The handle_new_slot function now includes logic to retrieve the preconfer election using the delegation_contract and save it to the auctioneer. This change looks good, but please ensure that:
- The
get_preconfer_electionmethod ofdelegation_contractreturns the expected result. - The error handling for
get_preconfer_electionandsave_new_gateway_electionis appropriate. - The logging statements provide sufficient information for debugging and monitoring.
#!/bin/bash
# Description: Verify the usage of `delegation_contract` in `handle_new_slot`.
# Test 1: Check if `get_preconfer_election` method exists in `DelegationTrait`.
rg 'fn get_preconfer_election' $(fd DelegationTrait)
# Test 2: Check if `get_preconfer_election` returns a `Result<Option<_>, _>`.
rg 'get_preconfer_election(.+)\.await' -A 1 | rg 'Ok\(Some\(.+\)\)'
rg 'get_preconfer_election(.+)\.await' -A 1 | rg 'Ok\(None\)'
rg 'get_preconfer_election(.+)\.await' -A 1 | rg 'Err\(.+\)'
# Test 3: Check if the error handling for `get_preconfer_election` and `save_new_gateway_election` includes logging.
rg 'get_preconfer_election(.+)\.await' -A 5 | rg 'error!'
rg 'save_new_gateway_election(.+)\.await' -A 5 | rg 'error!'101-107: Verify the new generic parameter D is properly constrained.
The new generic parameter D has been added to the impl block for ProposerApi, which is good. However, please ensure that the DelegationTrait bound is sufficient and appropriate for all the use cases of D within the ProposerApi implementation.
✅ Verification successful
The DelegationTrait bound is appropriate and sufficient for the generic parameter D
The verification shows that:
DelegationTraitis properly defined with the requiredget_preconfer_electionmethod- Within
ProposerApi, the generic parameterDis used correctly throughself.delegation_contract.get_preconfer_election()call - The trait includes necessary bounds (
Send + Sync + Clone) for concurrent usage - No additional trait bounds are required based on its usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the generic parameter `D` within `ProposerApi` implementation.
# Test: Check if `D` is used with any other traits besides `DelegationTrait`.
rg 'impl<.+> ProposerApi<.+, D>' -A 10 | rg 'D: (?!DelegationTrait)'
Length of output: 1091
Script:
#!/bin/bash
# Let's check how D is used within ProposerApi implementation
ast-grep --pattern 'impl<$_> ProposerApi<$_> {
$$$
}'
# Also check for any direct usage of D or DelegationTrait
rg "D::" crates/api/src/proposer/api.rs
rg "self\.delegation" crates/api/src/proposer/api.rs
# Check the trait definition to understand required functionality
cat crates/api/src/delegation/traits.rs
Length of output: 733
75-81: Verify the new generic parameter D is properly constrained and used.
The new generic parameter D constrained by DelegationTrait has been added to the ProposerApi struct. This change looks good, but please ensure that:
- The
DelegationTraitis properly defined and implemented. - The
delegation_contractfield of typeArc<D>is properly initialized and used throughout the code. - All the necessary changes have been made in the codebase to accommodate this new generic parameter.
✅ Verification successful
Let me gather more information about the DelegationTrait definition and its implementations to ensure it's properly constrained and used.
Let me check one more thing to see the complete initialization of ProposerApi and its fields.
The new generic parameter D is properly constrained and used
The verification shows that:
DelegationTraitis properly defined intraits.rswith required bounds (Send + Sync + Clone) and a clear async method signature- The trait is correctly implemented by both
MockDelegationandContractDelegation delegation_contractfield is properly initialized inProposerApiasArc<D>- The field is actively used in the codebase, specifically in duty updates to get preconfer elections
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new generic parameter `D` and `DelegationTrait`.
# Test 1: Check if `DelegationTrait` is defined.
rg 'pub trait DelegationTrait'
# Test 2: Check if `DelegationTrait` is implemented.
rg 'impl DelegationTrait for'
# Test 3: Check if `delegation_contract` field is initialized in `ProposerApi::new`.
rg 'delegation_contract: Arc<D>'
# Test 4: Check if `delegation_contract` field is used.
rg 'self\.delegation_contract'
Length of output: 870
Script:
#!/bin/bash
# Check the definition of DelegationTrait and its methods
rg -A 10 'pub trait DelegationTrait' crates/api/src/delegation/traits.rs
# Check the implementation in MockDelegation
rg -A 10 'impl DelegationTrait for MockDelegation' crates/api/src/delegation/mock_delegation.rs
# Check the implementation in ContractDelegation
rg -A 10 'impl DelegationTrait for ContractDelegation' crates/api/src/delegation/contract_delegation.rs
# Check the initialization of ProposerApi with the delegation_contract
rg -A 5 'pub fn new\(' crates/api/src/proposer/api.rs
Length of output: 1662
Script:
#!/bin/bash
# Search for ProposerApi struct definition and initialization
ast-grep --pattern 'struct ProposerApi {
$$$
}'
# Check for any other usages of delegation_contract in ProposerApi
rg -B 5 -A 5 'delegation_contract' crates/api/src/proposer/api.rs
Length of output: 1884
|
We need to create a new branch offchain-delegation to backup the offchain version |
|
We may use a indexer to index delegation event on the contract to reduce the need to query contract on every slot. |
zsluedem
force-pushed
the
onchain-delegation
branch
from
d39396f to
ca7daf2
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
Cargo.toml (1)
70-74: Consider adding documentation for new dependencies.To improve maintainability, consider adding comments explaining why these Alloy dependencies are needed and their role in the onchain delegation feature. This will help future contributors understand the purpose of these dependencies.
Example documentation:
# Alloy rust types +# The following dependencies are required for onchain delegation: +# - alloy-sol-types: Solidity type definitions for contract interactions +# - alloy-contract: Smart contract interaction utilities +# - alloy-provider: Blockchain provider interface with HTTP support +# - alloy-transport: Network transport layer for blockchain communication alloy-sol-types = "0.8.8" alloy-contract ="0.5.2" alloy-provider = { version = "0.5.2", features = ["reqwest"], default-features = false } alloy-transport = "0.5.2"crates/api/src/delegation/contract_delegation.rs (3)
29-34: Add documentation for PreconferElectionRes fields.While the struct matches the Solidity contract, it would be helpful to document the purpose of each field, especially since this is a public interface.
59-61: Improve error context for contract call failures.The error handling for contract calls could provide more context about what failed:
- let taiyi_core_contract = Arc::new(TaiyiCoreInstance::new(self.delegation_contract_address, self.provider.clone())); - let preconfer_election = - taiyi_core_contract.getPreconferElection(pubkey).call().await.map_err(|e| Error::GetPreconferElection(e.to_string()))?; + let taiyi_core_contract = TaiyiCoreInstance::new(self.delegation_contract_address, self.provider.clone()); + let preconfer_election = taiyi_core_contract + .getPreconferElection(pubkey) + .call() + .await + .map_err(|e| Error::GetPreconferElection( + format!("Failed to get preconfer election for validator {:?}: {}", validator_pubkey, e) + ))?;
59-59: Remove unnecessary Arc wrapping.The
Arcwrapping of the contract instance appears unnecessary as it's only used locally within the function.crates/api/src/service.rs (1)
150-150: Consider implementing an indexer for delegation events.The current implementation might need to query the contract frequently, which could impact performance. As suggested in the PR comments, implementing an indexer for delegation events would:
- Reduce the load on the execution client
- Improve response times by caching delegation relationships
- Minimize RPC calls by only tracking changes through events
Consider these steps:
- Create an indexer service that subscribes to delegation events
- Store indexed data in your existing PostgreSQL database
- Update
ContractDelegationto first check the indexed data before falling back to direct contract queriesWould you like help designing the indexer service architecture?
Also applies to: 163-163
crates/api/src/proposer/tests.rs (1)
Line range hint
829-840: Consider adding specific test cases for delegation functionality.While the MockDelegation integration looks correct, there should be dedicated test cases to verify the delegation behavior. This would help ensure the new functionality works as expected.
Would you like me to help generate test cases for the delegation functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.toml(1 hunks)crates/api/Cargo.toml(1 hunks)crates/api/src/constraints/api.rs(1 hunks)crates/api/src/delegation/contract_delegation.rs(1 hunks)crates/api/src/delegation/error.rs(1 hunks)crates/api/src/delegation/mock_delegation.rs(1 hunks)crates/api/src/delegation/mod.rs(1 hunks)crates/api/src/delegation/traits.rs(1 hunks)crates/api/src/lib.rs(1 hunks)crates/api/src/proposer/api.rs(16 hunks)crates/api/src/proposer/tests.rs(4 hunks)crates/api/src/router.rs(3 hunks)crates/api/src/service.rs(4 hunks)crates/api/src/test_utils.rs(4 hunks)crates/common/src/api/constraints_api.rs(2 hunks)crates/common/src/api/mod.rs(1 hunks)crates/common/src/config.rs(7 hunks)crates/utils/src/signing.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/api/Cargo.toml
- crates/api/src/constraints/api.rs
- crates/api/src/delegation/error.rs
- crates/api/src/delegation/mock_delegation.rs
- crates/api/src/delegation/mod.rs
- crates/api/src/delegation/traits.rs
- crates/api/src/lib.rs
- crates/api/src/router.rs
- crates/api/src/test_utils.rs
- crates/common/src/api/constraints_api.rs
- crates/common/src/api/mod.rs
- crates/common/src/config.rs
- crates/utils/src/signing.rs
🔇 Additional comments (6)
crates/api/src/service.rs (2)
3-3: LGTM: Imports are correctly placed.
The new imports for ProviderBuilder and ContractDelegation are appropriately placed and necessary for the onchain delegation feature.
Also applies to: 22-22
145-149: Refer to previous review comment about provider initialization robustness.
crates/api/src/proposer/tests.rs (2)
73-73: LGTM: Mock delegation import added correctly.
The import is properly placed and follows the existing import structure.
208-208: LGTM: Function signature updated to support delegation.
The return type has been properly updated to include the new MockDelegation type parameter.
crates/api/src/proposer/api.rs (2)
75-81: LGTM: Generic type parameter and field addition.
The addition of the generic type parameter D with DelegationTrait constraint and the corresponding delegation_contract field is well-structured and properly thread-safe using Arc.
Also applies to: 97-98
1277-1299: Consider potential race conditions in preconfer election handling.
While the code properly handles error cases and includes appropriate logging, there's a potential race condition when saving preconfer elections for multiple duties concurrently. Consider:
- Multiple duties could attempt to save the same preconfer election simultaneously
- The
save_new_gateway_electioncall might need synchronization or deduplication
Note: A related issue about checking if the elected_preconfer has changed is being tracked in #25.
Consider implementing one of these solutions:
- Add a mutex/lock around the save operation
- Implement optimistic concurrency control
- Use a queue to process elections sequentially
|
close it since onchain delegation is not our goal now |
Summary by CodeRabbit
New Features
Bug Fixes
MAX_GATEWAY_ELECTION_SIZEconstant and the/elect_preconferAPI endpoint.Documentation