fix: return signed delegations #6
Conversation
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes involve significant modifications across multiple files, primarily focusing on the management of validator delegations. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Auctioneer
participant DatabaseService
participant RedisCache
Client->>Auctioneer: Request to save validator delegation
Auctioneer->>DatabaseService: Save delegation
DatabaseService-->>Auctioneer: Confirmation
Auctioneer->>RedisCache: Save delegation
RedisCache-->>Auctioneer: Confirmation
Auctioneer-->>Client: Delegation saved
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 (
|
![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
🧹 Outside diff range and nitpick comments (14)
crates/datastore/src/auctioneer/mock_auctioneer.rs (3)
39-44: LGTM: Newget_validator_delegationsmethod implemented correctly.The method signature and implementation are appropriate for a mock object. However, consider adding a comment explaining that this is a mock implementation returning an empty vector.
Consider adding a comment like this:
// Mock implementation: always returns an empty vector Ok(vec![])
46-58: LGTM: New delegation methods implemented correctly.Both
save_validator_delegationandrevoke_validator_delegationare implemented appropriately for mock objects. However, consider adding comments to explain that these are mock implementations.Consider adding comments like this to both methods:
// Mock implementation: no-op, always returns Ok Ok(())
Line range hint
114-120: LGTM: Test case added toget_best_bidmethod.The addition of a test case for a specific bid value (9999) is implemented correctly. It enhances the mock object's ability to simulate error conditions. Consider adding a more descriptive comment to explain the purpose of this test case.
Consider updating the comment to be more descriptive:
// Test case: If the bid value is 9999, return an UnexpectedValueType error // This allows simulating specific error conditions in tests if let Some(bid) = self.best_bid.lock().unwrap().clone() { if bid.value() == U256::from(9999) { return Err(AuctioneerError::UnexpectedValueType); } }crates/api/src/proposer/api.rs (2)
Line range hint
454-556: LGTM! Consider standardizing error handling for consistency.The implementation of
get_header_with_proofslooks good. It correctly extends the functionality ofget_headerby including the fetching of inclusion proofs. The error handling and logging are consistent with other methods in the file.For consistency with other error handling in the file, consider wrapping the
get_inclusion_proofcall in amatchstatement to handle potential errors explicitly:let proofs = match self.get_inclusion_proof( slot, &bid_request.public_key, bid.block_hash(), &request_id ).await { Some(proofs) => proofs, None => { warn!(request_id = %request_id, "inclusion proof not found"); return Err(ProposerApiError::InclusionProofNotFound); } };This change would require adding a new variant to the
ProposerApiErrorenum.
Line range hint
557-561: LGTM! Consider standardizing error message format.The error handling improvements in the
get_payloadmethod are good. They provide more context for deserialization and broadcasting errors, which will be helpful for debugging.For consistency, consider standardizing the error message format across all error logs in this method. For example:
warn!( request_id = %request_id, event = "get_payload", error = %err, "Failed to deserialize signed block", ); error!( request_id = %request_id, event = "get_payload", error = %e, "Failed to broadcast get payload", );This format aligns with other error logs in the file and makes it easier to parse and analyze logs consistently.
Also applies to: 562-566
crates/datastore/src/auctioneer/traits.rs (3)
18-21: Add documentation comments forget_validator_delegationsAs this is a public method in a trait, including documentation comments helps explain its purpose, parameters, and return value to other developers.
Would you like assistance in drafting the documentation comments for this method?
23-27: Add documentation comments forsave_validator_delegationIncluding documentation comments for this public method will improve code clarity and help others understand how to use it correctly.
Would you like assistance in drafting the documentation comments for this method?
28-31: Add documentation comments forrevoke_validator_delegationAdding documentation comments to this public method will assist developers in understanding its functionality and any side effects.
Would you like assistance in drafting the documentation comments for this method?
crates/common/src/config.rs (3)
Line range hint
102-104: Optimizecontainsmethod to avoid unnecessaryHashSetcreationIn the
containsmethod, aHashSetis created every time the method is called, which can impact performance, especially when called frequently. Consider refactoring the method to avoid building aHashSeton each call. You could iterate overself.enabled_routesand check for the route directly, or storeenabled_routesas aHashSetinstead of aVec.[performance]
Line range hint
106-110: Enhanceextendmethod performance by optimizingcontainsusageThe
extendmethod callscontainsin a loop for each route. Sincecontainscurrently creates aHashSeton each call, this can lead to performance issues when adding multiple routes. After optimizing thecontainsmethod, ensure theextendmethod benefits from this improvement.[performance]
Line range hint
112-117: Reviewremovemethod for efficiencyThe
removemethod usesretainto filter out the specified route, which iterates overself.enabled_routes. Ifenabled_routesis changed to aHashSet, this method could be optimized by directly removing the route without iteration.[performance]
crates/api/src/constraints/api.rs (2)
Line range hint
184-192: Potential issue with unhandled errors in asynchronous taskThe
save_validator_delegationoperation is being executed in a spawned asynchronous task without awaiting its result. If an error occurs during the delegation saving process, it will only be logged, and the main function will proceed without confirming whether the operation was successful. This could lead to inconsistencies if the save operation fails.Consider awaiting the result of
save_validator_delegationto ensure that any errors are propagated back to the caller, allowing for proper error handling and client notification.Apply this diff to handle errors appropriately:
-tokio::spawn( async move { - if let Err(err) = api.auctioneer.save_validator_delegation(signed_delegation).await { - error!( - error = %err, - "Failed to save delegation", - ) - } -}); +if let Err(err) = api.auctioneer.save_validator_delegation(signed_delegation).await { + error!( + error = %err, + "Failed to save delegation", + ); + return Err(ConstraintsApiError::AuctioneerError(err)); +}
Line range hint
250-258: Potential issue with unhandled errors in asynchronous taskThe
revoke_validator_delegationoperation is being executed in a spawned asynchronous task without awaiting its result. If an error occurs during the revocation process, it will only be logged, and the main function will proceed without confirming whether the operation was successful. This could lead to inconsistencies if the revocation fails.Consider awaiting the result of
revoke_validator_delegationto ensure that any errors are propagated back to the caller, allowing for proper error handling and client notification.Apply this diff to handle errors appropriately:
-tokio::spawn( async move { - if let Err(err) = api.auctioneer.revoke_validator_delegation(signed_revocation).await { - error!( - error = %err, - "Failed to do revocation", - ) - } -}); +if let Err(err) = api.auctioneer.revoke_validator_delegation(signed_revocation).await { + error!( + error = %err, + "Failed to revoke delegation", + ); + return Err(ConstraintsApiError::AuctioneerError(err)); +}crates/datastore/src/redis/redis_cache.rs (1)
541-556: Consider returning an empty vector when no delegations are foundIn the
get_validator_delegationsmethod, if no delegations are found for the given validator, returning an empty vector instead of an error might be more user-friendly. This allows callers to handle cases with zero delegations without needing to catch exceptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
- crates/api/src/builder/api.rs (6 hunks)
- crates/api/src/constraints/api.rs (5 hunks)
- crates/api/src/proposer/api.rs (1 hunks)
- crates/common/src/bid_submission/submission.rs (1 hunks)
- crates/common/src/config.rs (1 hunks)
- crates/database/src/error.rs (0 hunks)
- crates/database/src/mock_database_service.rs (0 hunks)
- crates/database/src/postgres/postgres_db_service.rs (0 hunks)
- crates/database/src/traits.rs (0 hunks)
- crates/datastore/src/auctioneer/mock_auctioneer.rs (2 hunks)
- crates/datastore/src/auctioneer/traits.rs (2 hunks)
- crates/datastore/src/error.rs (2 hunks)
- crates/datastore/src/redis/redis_cache.rs (3 hunks)
- crates/datastore/src/redis/utils.rs (1 hunks)
- crates/datastore/src/types/keys.rs (1 hunks)
💤 Files with no reviewable changes (4)
- crates/database/src/error.rs
- crates/database/src/mock_database_service.rs
- crates/database/src/postgres/postgres_db_service.rs
- crates/database/src/traits.rs
✅ Files skipped from review due to trivial changes (1)
- crates/common/src/bid_submission/submission.rs
🔇 Additional comments (15)
crates/datastore/src/types/keys.rs (1)
2-2: LGTM! Verify usage across the codebase.The addition of
DELEGATIONS_KEYis consistent with the existing naming conventions and visibility. It aligns with the PR objective of fixing the return of signed delegations.To ensure consistent usage of this new constant, please run the following script:
This script will help identify any places where the hardcoded string "delegations" is used instead of the new constant, and also show where the constant is properly imported and used.
✅ Verification successful
Verified usage of
DELEGATIONS_KEYacross the codebase. No hardcoded "delegations" strings found outsidekeys.rs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of DELEGATIONS_KEY across the codebase # Test 1: Check for hardcoded "delegations" strings that should use the constant echo "Checking for hardcoded 'delegations' strings:" rg --type rust '"delegations"' -g '!**/keys.rs' # Test 2: Verify the import and usage of DELEGATIONS_KEY echo "Checking for imports and usage of DELEGATIONS_KEY:" rg --type rust 'use .*DELEGATIONS_KEY|DELEGATIONS_KEY'Length of output: 855
crates/datastore/src/redis/utils.rs (2)
4-4: LGTM: Import statement updated correctly.The addition of
DELEGATIONS_KEYto the import list is consistent with its usage in the newget_delegations_keyfunction. The import statement maintains the existing style and formatting.
7-9: LGTM: Newget_delegations_keyfunction implemented correctly.The function is well-implemented and consistent with other key generation functions in the file. It correctly uses the
DELEGATIONS_KEYconstant and follows the established pattern for key formatting.Regarding the past review comment about the trailing comma in the parameters:
This is now considered good practice in Rust, as it allows for easier future expansion of parameters and is consistent with Rust's formatting guidelines. The trailing comma can be kept as is.crates/datastore/src/error.rs (1)
51-52:⚠️ Potential issueConsider returning an empty list instead of an error
The addition of
ValidatorDelegationNotFoundas an error variant is syntactically correct. However, as suggested in a previous review comment, returning an error when a delegation is not found might not be the most appropriate approach.Consider refactoring this to return an empty list instead of an error when a validator delegation is not found. This approach can simplify error handling in the calling code and align with the principle of "fall soft" in API design.
If you decide to keep this as an error, please provide a justification for why this scenario should be treated as an error condition rather than a normal, empty result.
crates/datastore/src/auctioneer/mock_auctioneer.rs (1)
7-7: LGTM: Import changes are consistent with new functionality.The addition of
SignedDelegationandSignedRevocationimports aligns with the new validator delegation methods being implemented.crates/api/src/proposer/api.rs (1)
Line range hint
1-1337: Overall, the changes look good and improve the codebase.The implementation of
get_header_with_proofsand the error handling improvements inget_payloadare well-done. They add new functionality and enhance the existing error logging, which will be beneficial for debugging and maintenance.The suggestions provided are minor and aimed at improving consistency across the codebase. These changes can be addressed in this PR or as follow-up tasks, depending on the team's preference.
crates/api/src/builder/api.rs (6)
38-38: Update to error handling: AuctioneerError replaces DatabaseErrorThe import statement has been modified to use
AuctioneerErrorfrom thehelix_datastorecrate instead ofDatabaseError. This change suggests a shift in error handling from database-specific errors to more general auctioneer-related errors.
Line range hint
162-196: Improved error handling and API consistency inconstraintsmethodThe
constraintsmethod has been updated with the following improvements:
- The API documentation link has been updated to reflect the current documentation.
- Error handling now uses
AuctioneerError, which is more appropriate for this context.- The method now returns an empty vector instead of an error when no constraints are found, which is a more graceful way to handle this case.
- The use of
debug!log for no constraints found is a good practice for debugging purposes.These changes enhance the method's robustness and align it better with the overall error handling strategy of the system.
Line range hint
196-226: Updated API documentation link forconstraints_streammethodThe
constraints_streammethod has been updated with a new API documentation link. This change ensures that developers can access the most current and relevant documentation for this method.
Line range hint
226-260: Enhanced error handling and API consistency indelegationsmethodThe
delegationsmethod has been significantly improved:
- The API documentation link has been updated to the latest version.
- Error handling now uses
AuctioneerError, which is more appropriate for this context.- The method now returns an empty vector instead of an error when no delegations are found, providing a more graceful handling of this case.
- The use of
debug!log for no delegations found is good for debugging purposes.- The error handling structure is more detailed, allowing for specific handling of the
ValidatorDelegationNotFoundcase.These changes make the method more robust and consistent with the overall error handling strategy of the system.
493-493: Updated API documentation link forsubmit_blockmethodThe
submit_blockmethod has been updated with a new API documentation link. This change ensures that developers can access the most current and relevant documentation for this critical method in the block submission process.
493-493: Updated API documentation link forsubmit_block_with_proofsmethodThe
submit_block_with_proofsmethod has been updated with a new API documentation link. This change ensures that developers can access the most current and relevant documentation for this method, which is crucial for understanding the block submission process with inclusion proofs.crates/api/src/constraints/api.rs (3)
63-63: Documentation URL updated correctlyThe updated documentation URL
<https://docs.boltprotocol.xyz/api/builder#constraints>appears correct and points to the appropriate API documentation for thesubmit_constraintsmethod.
141-141: Documentation URL updated correctlyThe updated documentation URL
<https://docs.boltprotocol.xyz/api/builder#delegate>appears correct and points to the appropriate API documentation for thedelegatemethod.
207-207: Documentation URL updated correctlyThe updated documentation URL
<https://docs.boltprotocol.xyz/api/builder#revoke>appears correct and points to the appropriate API documentation for therevokemethod.
crates/datastore/src/error.rs
Outdated
| AuctioneerError::ValidatorDelegationNotFound => { | ||
| (StatusCode::BAD_REQUEST, "Validator delegation not found".to_string()).into_response() | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Handling of ValidatorDelegationNotFound Error
The ValidatorDelegationNotFound error is handled differently across the codebase:
- Error Response: In
crates/datastore/src/error.rs, it returns a 400 BAD_REQUEST status. - Empty List Response: In
crates/api/src/builder/api.rs, it results in an empty JSON array.
This inconsistency can lead to unpredictable behavior and confusion for API consumers. Please standardize the handling of this error by either:
-
Always returning a 400 BAD_REQUEST error when delegations are not found.
OR
-
Consistently returning an empty list instead of an error, and removing the
ValidatorDelegationNotFounderror variant if opting for this approach.
🔗 Analysis chain
Review the necessity of this error response
The implementation of IntoResponse for ValidatorDelegationNotFound is consistent with other error variants. However, this implementation directly relates to the previous comment about reconsidering the error design.
If you decide to return an empty list instead of an error for not found delegations, this entire match arm should be removed. Otherwise, consider whether a 400 BAD_REQUEST is the most appropriate status code for this scenario. A 404 NOT_FOUND might be more suitable if you maintain this as an error condition.
To help verify the usage and impact of this error, you can run the following script:
This will help identify where this error is being used and assist in determining the best approach for handling not found delegations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of ValidatorDelegationNotFound in the codebase
rg "ValidatorDelegationNotFound" --type rust
Length of output: 412
Script:
#!/bin/bash
# Extract 5 lines of context around each usage of ValidatorDelegationNotFound in Rust files
rg "ValidatorDelegationNotFound" --type rust -C 5
Length of output: 3236
| async fn get_validator_delegations( | ||
| &self, | ||
| pub_key: BlsPublicKey, | ||
| ) -> Result<Vec<SignedDelegation>, AuctioneerError>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing pagination for get_validator_delegations
If the number of delegations can be large, returning all delegations at once may impact performance and memory usage. Consider implementing pagination or streaming results to handle large datasets efficiently.
| GetBuilderConstraints, | ||
| /// Reference: <https://chainbound.github.io/bolt-docs/api/relay#constraints_stream> | ||
| /// Reference: <https://docs.boltprotocol.xyz/api/relay#constraints_stream> | ||
| GetBuilderConstraintsStream, |
There was a problem hiding this comment.
Add missing documentation comment for GetBuilderDelegations
The GetBuilderDelegations route lacks a documentation comment with a reference link. For consistency and clarity, please add a /// Reference: comment pointing to the appropriate API documentation for this route.
| async fn save_validator_delegation( | ||
| &self, | ||
| signed_delegation: SignedDelegation, | ||
| ) -> Result<(), AuctioneerError> { | ||
| let key = get_delegations_key(&signed_delegation.message.validator_pubkey); | ||
|
|
||
| // Attempt to get the existing delegations from the cache. | ||
| let delegations: Option<Vec<SignedDelegation>> = self | ||
| .get(&key) | ||
| .await | ||
| .map_err(AuctioneerError::RedisError)?; | ||
|
|
||
| // Append the new delegation to the existing delegations or create a new Vec if none exist. | ||
| let mut all_delegations = match delegations { | ||
| Some(mut delegations) => { | ||
| delegations.push(signed_delegation); | ||
| delegations | ||
| } | ||
| None => Vec::from([signed_delegation]), | ||
| }; | ||
|
|
||
| // Save the updated delegations back to the cache. | ||
| self.set(&key, &all_delegations, None) | ||
| .await | ||
| .map_err(AuctioneerError::RedisError) | ||
| } |
There was a problem hiding this comment.
Ensure atomicity to prevent race conditions when saving delegations
The save_validator_delegation method performs a read-modify-write sequence without synchronization. This can lead to race conditions if multiple concurrent operations modify delegations for the same validator. Consider using Redis transactions or Lua scripts to perform these operations atomically and maintain data consistency.
🛠️ Refactor suggestion
Optimize delegation storage using Redis Sets or Hashes
Storing delegations as serialized vectors under a single key may lead to performance bottlenecks and complicate concurrent modifications, especially as the number of delegations grows. Consider using Redis Sets or Hashes for more efficient storage and to leverage atomic addition and removal operations provided by Redis.
Also applies to: 585-610
| async fn revoke_validator_delegation( | ||
| &self, | ||
| signed_revocation: SignedRevocation, | ||
| ) -> Result<(), AuctioneerError> { | ||
| let key = get_delegations_key(&signed_revocation.message.validator_pubkey); | ||
|
|
||
| // Attempt to get the existing delegations from the cache. | ||
| let delegations: Option<Vec<SignedDelegation>> = self | ||
| .get(&key) | ||
| .await | ||
| .map_err(AuctioneerError::RedisError)?; | ||
|
|
||
| // Filter out the revoked delegation. | ||
| let updated_delegations = match delegations { | ||
| Some(mut delegations) => { | ||
| delegations.retain(|delegation| delegation.message.delegatee_pubkey != signed_revocation.message.delegatee_pubkey); | ||
| delegations | ||
| } | ||
| None => Vec::new(), | ||
| }; | ||
|
|
||
| // Save the updated delegations back to the cache. | ||
| self.set(&key, &updated_delegations, None) | ||
| .await | ||
| .map_err(AuctioneerError::RedisError) | ||
| } |
There was a problem hiding this comment.
Ensure atomicity to prevent race conditions when revoking delegations
Similar to save_validator_delegation, the revoke_validator_delegation method may encounter race conditions due to concurrent access without synchronization. Utilizing atomic operations or Redis transactions can prevent potential data inconsistency issues when multiple revocations occur simultaneously.
Summary by CodeRabbit
Release Notes
New Features
Documentation Updates
Bug Fixes
Chores