chore: move BLS Sigs import to Rust Dash Core #2252
Conversation
WalkthroughThe changes in this pull request involve updates to various Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (13)
packages/rs-dpp/src/lib.rs (1)
93-96: Summary of changes in lib.rsThe modifications in this file contribute to a more centralized and organized import structure:
- BLS signatures import is now routed through the dashcore crate.
- ed25519_dalek import is also centralized through dashcore, replacing the previous direct import.
- A new data_contracts export has been added under the system_contracts feature.
These changes align with the PR objective and appear to enhance the modularity of the codebase. However, it's important to ensure that these changes don't introduce any breaking changes in dependent modules.
Consider updating the module documentation to reflect these structural changes, particularly noting the new centralized import paths and the addition of the data_contracts export. This will help maintain clear and up-to-date documentation for developers working with this module.
packages/rs-dpp/Cargo.toml (1)
78-79: LGTM: Added new features for BLS and EdDSA signatures.The addition of "bls-signatures" and "ed25519-dalek" features, which depend on the corresponding features in
dashcore, aligns with the PR objective and the changes made to thedashcoredependency.Consider updating any relevant documentation to reflect these new features and their dependencies on
dashcore.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs (6)
Line range hint
1-124: LGTM! Consider enhancing version handling.The implementation of
StateTransitionActionTransformerV0forIdentityUpdateTransitionlooks good. It correctly handles version mismatches and delegates the actual transformation to a version-specific method.Consider extracting the version-specific method names and known versions into constants to improve maintainability. For example:
const TRANSFORM_METHOD_NAME: &str = "identity update transition: transform_into_action"; const KNOWN_VERSIONS: &[u64] = &[0]; // Then use these constants in the match statement and error handling
Line range hint
125-149: LGTM! Consider consistent error handling.The implementation of
StateTransitionBasicStructureValidationV0forIdentityUpdateTransitionis well-structured. It correctly handles version mismatches and inactive versions.For consistency with the
transform_into_actionmethod, consider using the same error handling approach:const VALIDATE_BASIC_STRUCTURE_METHOD_NAME: &str = "identity update transition: validate_basic_structure"; const KNOWN_VERSIONS: &[u64] = &[0]; match version { 0 => self.validate_basic_structure_v0(platform_version), version => Err(Error::Execution(ExecutionError::UnknownVersionMismatch { method: VALIDATE_BASIC_STRUCTURE_METHOD_NAME.to_string(), known_versions: KNOWN_VERSIONS.to_vec(), received: version, })), }This approach eliminates the need for the
Nonecase and makes the error handling more consistent across methods.
Line range hint
150-174: LGTM! Consider consistent error handling and method naming.The implementation of
StateTransitionStateValidationV0forIdentityUpdateTransitionis well-structured and correctly handles version mismatches.For consistency with the other methods, consider:
- Using the same error handling approach:
const VALIDATE_STATE_METHOD_NAME: &str = "identity update transition: validate_state"; const KNOWN_VERSIONS: &[u64] = &[0]; match version { 0 => self.validate_state_v0(platform, tx, platform_version), version => Err(Error::Execution(ExecutionError::UnknownVersionMismatch { method: VALIDATE_STATE_METHOD_NAME.to_string(), known_versions: KNOWN_VERSIONS.to_vec(), received: version, })), }
- Renaming the
validate_state_v0method tovalidate_state_structure_v0for consistency with thevalidate_basic_structure_v0method in the previous implementation.
Line range hint
175-249: LGTM! Consider enhancing test assertions.The test case
test_identity_update_that_disables_an_authentication_keyis well-structured and covers the scenario of disabling an authentication key.Consider adding more specific assertions to verify the state after the transition:
- Check that the disabled key is actually disabled in the updated identity:
let updated_identity = platform.drive.fetch_identity(identity.id(), &platform_version).expect("Identity should exist"); assert!(updated_identity.get_public_key_by_id(1).unwrap().is_disabled(), "Key should be disabled");
- Verify that the identity revision has been incremented:
assert_eq!(updated_identity.get_revision(), 1, "Identity revision should be incremented");These additional assertions will make the test more robust and explicitly verify the expected outcomes of the state transition.
Line range hint
250-373: LGTM! Consider enhancing test assertions and reducing duplication.The test case
test_identity_update_that_disables_an_encryption_keyis well-structured and covers the scenario of disabling an encryption key.
- Consider adding more specific assertions to verify the state after the transition, similar to the previous test:
let updated_identity = platform.drive.fetch_identity(identity.id(), &platform_version).expect("Identity should exist"); assert!(updated_identity.get_public_key_by_id(key.id()).unwrap().is_disabled(), "Encryption key should be disabled"); assert_eq!(updated_identity.get_revision(), 1, "Identity revision should be incremented");
- There's some duplication between this test and the previous one. Consider extracting common setup code into a helper function to reduce duplication:
fn setup_platform_and_identity() -> (TestPlatform, Identity, Signer, IdentityPublicKey) { // Common setup code here } // Use in tests: let (platform, identity, signer, master_key) = setup_platform_and_identity();
- The
issuesassertions are repeated. Consider extracting this into a helper function:fn assert_no_grove_issues(platform: &TestPlatform, platform_version: &PlatformVersion) { let issues = platform.drive.grove.visualize_verify_grovedb(None, true, false, &platform_version.drive.grove_version) .expect("expected to have no issues"); assert_eq!(issues.len(), 0, "issues are {}", issues.iter() .map(|(hash, (a, b, c))| format!("{}: {} {} {}", hash, a, b, c)) .collect::<Vec<_>>() .join(" | ")); } // Use in tests: assert_no_grove_issues(&platform, &platform_version);These improvements will make the tests more maintainable and reduce code duplication.
Line range hint
374-458: LGTM! Consider enhancing error checking and reducing duplication.The test case
test_identity_update_adding_owner_key_not_allowedis well-structured and correctly verifies that adding an owner key is not allowed.
- Consider making the error checking more specific:
assert_matches!( processing_result.execution_results().as_slice(), [StateTransitionExecutionResult::UnpaidConsensusError( ConsensusError::BasicError(error) )] if error.to_string().contains("Cannot add owner key") );This ensures that the error message is specific to the scenario being tested.
As mentioned in the previous review comment, consider extracting common setup code and the
assert_no_grove_issuescheck into helper functions to reduce duplication across tests.The
new_key_paircreation uses a fixed seed. Consider parameterizing this or using a more robust method for generating test keys:fn create_test_keypair() -> Keypair { let secp = Secp256k1::new(); let mut rng = rand::thread_rng(); Keypair::new(&secp, &mut rng) } // Use in test: let new_key_pair = create_test_keypair();These improvements will make the test more robust and easier to maintain.
packages/rs-dpp/src/signing.rs (3)
Line range hint
23-39: Remove commented-out debug statements to clean up the codeThere are several commented-out
dbg!statements within theKeyType::BLS12_381match arm. It's best practice to remove commented-out code to maintain code cleanliness and readability.Apply this diff to remove the commented-out debug statements:
let public_key = match bls_signatures::PublicKey::from_bytes(public_key_data) { Ok(public_key) => public_key, Err(e) => { - // dbg!(format!("bls public_key could not be recovered")); return SimpleConsensusValidationResult::new_with_error( SignatureError::BasicBLSError(BasicBLSError::new(e.to_string())).into(), ); } }; let signature = match bls_signatures::Signature::from_bytes(signature) { Ok(public_key) => public_key, Err(e) => { - // dbg!(format!("bls signature could not be recovered")); return SimpleConsensusValidationResult::new_with_error( SignatureError::BasicBLSError(BasicBLSError::new(e.to_string())).into(), ); } };
Line range hint
31-35: Simplify error handling by usingmap_errYou can simplify the creation of
public_keyandsignatureby usingmap_errto directly map the errors, reducing the amount of code and improving readability.Apply this diff to refactor the error handling:
let public_key = bls_signatures::PublicKey::from_bytes(public_key_data) - .map_err(|e| { - SimpleConsensusValidationResult::new_with_error( - SignatureError::BasicBLSError(BasicBLSError::new(e.to_string())).into(), - ) - })?; + .map_err(|e| SignatureError::BasicBLSError(BasicBLSError::new(e.to_string())).into())?; let signature = bls_signatures::Signature::from_bytes(signature) - .map_err(|e| { - SimpleConsensusValidationResult::new_with_error( - SignatureError::BasicBLSError(BasicBLSError::new(e.to_string())).into(), - ) - })?; + .map_err(|e| SignatureError::BasicBLSError(BasicBLSError::new(e.to_string())).into())?;
Line range hint
62-68: Avoid code duplication in error handling for unsupported key typesThe error handling for
KeyType::ECDSA_HASH160,KeyType::BIP13_SCRIPT_HASH, andKeyType::EDDSA_25519_HASH160is identical. Consider refactoring to reduce code duplication.Apply this diff to consolidate the match arms:
KeyType::ECDSA_HASH160 | KeyType::BIP13_SCRIPT_HASH | KeyType::EDDSA_25519_HASH160 => { if !signature.is_empty() { SimpleConsensusValidationResult::new_with_error( SignatureError::SignatureShouldNotBePresentError( SignatureShouldNotBePresentError::new( format!("{} keys should not have a signature as that would reveal sensitive information", key_type) ) ).into() ) } else { SimpleConsensusValidationResult::default() } }packages/rs-dpp/src/identity/identity_public_key/v0/methods/mod.rs (2)
Line range hint
52-59: Handle potential errors when creating Ed25519 signing keyIn the
KeyType::EDDSA_25519_HASH160match arm, the methoded25519_dalek::SigningKey::from_bytes(&secret_key)is used to create a signing key. However,from_bytesdoes not return aResultand may panic if the input slice is not of the correct length. To prevent panics and ensure robust error handling, consider usinged25519_dalek::SigningKey::try_fromwhich returns aResult.Apply this diff to handle potential errors:
let secret_key = match private_key_bytes.try_into() { Ok(secret_key) => secret_key, Err(_) => return Ok(false), }; - let key_pair = ed25519_dalek::SigningKey::from_bytes(&secret_key); + let key_pair = match ed25519_dalek::SigningKey::try_from(&secret_key) { + Ok(key_pair) => key_pair, + Err(_) => return Ok(false), + };
Line range hint
30-70: Consider refactoring conditional compilation blocks for maintainabilityThe extensive use of
#[cfg(feature = "...")]and#[cfg(not(feature = "..."))]within thevalidate_private_key_bytesfunction increases complexity. To enhance readability and maintainability, consider refactoring the key-type-specific logic into separate functions or modules, each conditioned on the relevant feature. This separation allows for cleaner code and easier future extensions.
📜 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 (11)
- packages/rs-dpp/Cargo.toml (2 hunks)
- packages/rs-dpp/src/bls/native_bls.rs (1 hunks)
- packages/rs-dpp/src/identity/identity_public_key/key_type.rs (1 hunks)
- packages/rs-dpp/src/identity/identity_public_key/v0/methods/mod.rs (1 hunks)
- packages/rs-dpp/src/lib.rs (1 hunks)
- packages/rs-dpp/src/signing.rs (1 hunks)
- packages/rs-drive-abci/Cargo.toml (1 hunks)
- packages/rs-drive-abci/src/execution/check_tx/v0/mod.rs (3 hunks)
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs (2 hunks)
- packages/rs-sdk/Cargo.toml (1 hunks)
- packages/simple-signer/Cargo.toml (1 hunks)
🧰 Additional context used
🔇 Additional comments (15)
packages/simple-signer/Cargo.toml (1)
11-11: Dependency version update looks good.The
dashcore-rpcdependency has been updated fromv0.15.7tov0.15.8. This change aligns with the PR objectives of moving BLS Sigs import to Rust Dash Core.To ensure this update doesn't introduce any breaking changes or conflicts, please run the following verification script:
If any of these tests fail or produce concerning output, please review the changes carefully and consider if any additional updates are needed in the project.
✅ Verification successful
Dependency version update is verified successfully.
The
dashcore-rpcdependency has been updated tov0.15.8. All verification checks passed, confirming that there are no breaking changes and no lingering references to the old version.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any breaking changes or conflicts in the updated dependency # Test 1: Check if the new version exists in the repository gh release view v0.15.8 --repo https://github.com/dashpay/rust-dashcore-rpc # Test 2: Look for any mentions of breaking changes in the release notes gh release view v0.15.8 --repo https://github.com/dashpay/rust-dashcore-rpc | grep -i "breaking change" # Test 3: Check if there are any other files that reference the old version rg "v0.15.7" --type tomlLength of output: 591
packages/rs-dpp/src/bls/native_bls.rs (2)
Line range hint
7-55: LGTM! The implementation remains consistent with the new import.The
NativeBlsModuleimplementation and its methods remain unchanged, which is correct given the nature of the import change. The usage ofPublicKey,PrivateKey, andbls_signatures::Signatureis consistent with the new import structure.
3-3: LGTM! Verify the new import path.The change in the import statement aligns with the PR objective of moving BLS Sigs import to Rust Dash Core. This modification should not affect the functionality of the
PrivateKeyandPublicKeytypes.To ensure the correctness of the new import path, please run the following verification script:
✅ Verification successful
Verified: The new import path for
bls_signaturesis correct.The change aligns with the PR objective of moving BLS signatures to Rust Dash Core and does not affect the functionality of the
PrivateKeyandPublicKeytypes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the bls_signatures module in the dashcore crate # Test 1: Check if the dashcore crate is available if ! rg --type rust "use dashcore" -g '!target/**'; then echo "Error: dashcore crate is not used in the project" exit 1 fi # Test 2: Verify the bls_signatures module in the dashcore crate if ! rg --type rust "use dashcore::bls_signatures" -g '!target/**'; then echo "Error: bls_signatures module is not found in the dashcore crate" exit 1 fi echo "Verification passed: The new import path for bls_signatures is correct"Length of output: 24213
packages/rs-dpp/src/lib.rs (2)
94-95: LGTM: ed25519_dalek import centralized through dashcore.This change centralizes the ed25519_dalek import through the dashcore crate while maintaining the existing feature flag condition. Note that the previous direct export of ed25519_dalek has been removed, as mentioned in the AI summary.
Let's verify the usage of this new import path and ensure the old import is no longer used:
#!/bin/bash # Description: Verify the usage of the new ed25519_dalek import path and absence of the old import # Test: Search for uses of ed25519_dalek from dashcore rg --type rust 'use (crate::)?dashcore::ed25519_dalek' # Test: Search for any remaining direct uses of ed25519_dalek rg --type rust 'use ed25519_dalek'
96-96: New data_contracts export added under system_contracts feature.A new export for
data_contractshas been added, conditional on thesystem_contractsfeature. While this change is not visible in the provided code snippet, it's mentioned in the AI summary.Could you provide more information about the purpose of this new export and how it's intended to be used? This would help in understanding the impact of this change on the overall system.
Let's verify the usage of this new export in the codebase:
#!/bin/bash # Description: Verify the usage of the new data_contracts export # Test: Search for uses of data_contracts rg --type rust 'use (crate::)?data_contracts' # Test: Search for the export of data_contracts rg --type rust 'pub use data_contracts'packages/rs-drive-abci/Cargo.toml (1)
31-31: Approved: dashcore-rpc dependency updateThe update of the dashcore-rpc dependency from v0.15.7 to v0.15.8 is a minor version increment, which typically includes backwards-compatible changes. This change aligns with the PR objective of moving BLS Sigs import to Rust Dash Core.
To ensure this update doesn't introduce any breaking changes or new features that need to be addressed, please run the following script:
Please review the output of this script to ensure there are no unexpected changes that might affect the project.
✅ Verification successful
Verification Successful: No Breaking Changes in dashcore-rpc v0.15.8
The update of the
dashcore-rpcdependency to versionv0.15.8does not introduce any breaking changes or major updates. TheCHANGELOG.mdremains unchanged betweenv0.15.7andv0.15.8, and there are no mentions of breaking or major changes in the commit messages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the changelog and any breaking changes in the dashcore-rpc update # Test: Check for a CHANGELOG.md file in the dashcore-rpc repository gh repo clone dashpay/rust-dashcore-rpc cd rust-dashcore-rpc git checkout v0.15.8 if [ -f CHANGELOG.md ]; then echo "CHANGELOG.md found. Here are the changes between v0.15.7 and v0.15.8:" git diff v0.15.7..v0.15.8 CHANGELOG.md else echo "CHANGELOG.md not found. Checking commit messages between v0.15.7 and v0.15.8:" git log --oneline v0.15.7..v0.15.8 fi # Test: Search for any mentions of breaking changes or major updates in the commit messages echo "Searching for mentions of breaking changes or major updates:" git log v0.15.7..v0.15.8 | grep -i "breaking\|major" cd .. rm -rf rust-dashcore-rpcLength of output: 1490
Script:
#!/bin/bash # Description: Correctly verify the changelog and any breaking changes in the dashcore-rpc update # Define the repository and tag REPO_URL="https://github.com/dashpay/rust-dashcore-rpc" TAG="v0.15.8" # Clone the correct repository gh repo clone dashpay/rust-dashcore-rpc cd rust-dashcore-rpc || exit # Checkout the specified tag git checkout "$TAG" # Verify the existence of CHANGELOG.md and display relevant changes if [ -f CHANGELOG.md ]; then echo "CHANGELOG.md found. Here are the changes between v0.15.7 and $TAG:" git diff v0.15.7.."$TAG" CHANGELOG.md else echo "CHANGELOG.md not found. Checking commit messages between v0.15.7 and $TAG:" git log --oneline v0.15.7.."$TAG" fi # Search for any mentions of breaking changes or major updates in commit messages echo "Searching for mentions of breaking changes or major updates:" git log v0.15.7.."$TAG" | grep -i "breaking\|major" # Clean up cd .. rm -rf rust-dashcore-rpcLength of output: 1560
Script:
#!/bin/bash # Description: Accurately verify the changelog and any breaking changes in the dashcore-rpc update # Define the repository and tag REPO_URL="https://github.com/dashpay/rust-dashcore-rpc.git" TAG="v0.15.8" # Verify if the repository exists by checking the GitHub API REPO_API="https://api.github.com/repos/dashpay/rust-dashcore-rpc" response=$(curl -s -o /dev/null -w "%{http_code}" "$REPO_API") if [ "$response" -ne 200 ]; then echo "Repository dashpay/rust-dashcore-rpc does not exist or is inaccessible." exit 1 fi # Clone the correct repository using the full URL git clone "$REPO_URL" cd rust-dashcore-rpc || { echo "Failed to enter repository directory."; exit 1; } # Checkout the specified tag git checkout "$TAG" || { echo "Tag $TAG does not exist."; cd ..; rm -rf rust-dashcore-rpc; exit 1; } # Verify the existence of CHANGELOG.md and display relevant changes if [ -f CHANGELOG.md ]; then echo "CHANGELOG.md found. Here are the changes between v0.15.7 and $TAG:" git diff v0.15.7.."$TAG" CHANGELOG.md else echo "CHANGELOG.md not found. Checking commit messages between v0.15.7 and $TAG:" git log --oneline v0.15.7.."$TAG" fi # Search for any mentions of breaking changes or major updates in commit messages echo "Searching for mentions of breaking changes or major updates:" git log v0.15.7.."$TAG" | grep -i "breaking\|major" # Clean up cd .. rm -rf rust-dashcore-rpcLength of output: 1545
packages/rs-sdk/Cargo.toml (1)
36-36: LGTM. Please verify testing and potential impacts.The update of the
dashcore-rpcdependency from v0.15.7 to v0.15.8 looks good. This minor version update aligns with the PR objective of moving BLS Sigs import to Rust Dash Core.To ensure the stability of the project, please confirm:
- Has this update been thoroughly tested?
- Are there any breaking changes or notable features in v0.15.8 that might affect the project?
You can use the following script to check the changelog or release notes for v0.15.8:
This script will help identify any significant changes or potential impacts of the update.
✅ Verification successful
Dependency update verified.
The
dashcore-rpcdependency has been successfully updated to v0.15.8. No significant changes or breaking updates were found in this minor version.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for changelog or release notes for dashcore-rpc v0.15.8 # Test: Look for CHANGELOG.md or RELEASES.md files echo "Searching for changelog or release notes..." fd -i -g 'CHANGELOG.md' -g 'RELEASES.md' --exec cat {} # If no changelog found, check commit messages if [ $? -ne 0 ]; then echo "No changelog found. Checking commit messages..." git clone --depth 100 https://github.com/dashpay/rust-dashcore-rpc.git temp_repo cd temp_repo git log --oneline v0.15.7..v0.15.8 cd .. rm -rf temp_repo fiLength of output: 693
packages/rs-dpp/Cargo.toml (4)
31-33: LGTM: Updated dashcore dependency with new features and version.The addition of "bls" and "eddsa" features to the
dashcoredependency aligns with the PR objective of moving BLS Sigs import to Rust Dash Core. The version update to 0.32.0 is also noted.Please ensure that the codebase is compatible with the new version of
dashcoreand that all functionality depending on these new features works as expected.
Line range hint
80-117: LGTM: Updated all_features list.The
all_featureslist has been correctly updated to include the newly added "bls-signatures" and "ed25519-dalek" features. This ensures consistency and allows these features to be enabled when all features are selected.
Line range hint
160-213: LGTM: Updated all_features_without_client list.The
all_features_without_clientlist has been correctly updated to include the newly added "bls-signatures" and "ed25519-dalek" features. This ensures consistency and allows these features to be enabled when all features except the client are selected.
Line range hint
1-365: Summary: Successfully moved BLS Sigs import to Rust Dash CoreThe changes in this file successfully accomplish the PR objective of moving the BLS Sigs import to Rust Dash Core. Key modifications include:
- Updated
dashcoredependency to include "bls" and "eddsa" features.- Added new features "bls-signatures" and "ed25519-dalek" that depend on the corresponding
dashcorefeatures.- Updated
all_featuresandall_features_without_clientlists to include the new features.These changes maintain consistency across the codebase and should improve the organization of cryptographic functionalities.
To ensure the changes have been implemented correctly and haven't introduced any regressions, please run the following verification script:
This script will help verify that the changes have been implemented correctly and that there are no remaining references to the old
bls-signaturescrate. It also checks for any potential build errors that might have been introduced by these changes.packages/rs-dpp/src/signing.rs (1)
11-11: Approved: Correctly importing required modulesThe addition of
bls_signaturesandsignerfromdashcoreis appropriate for handling BLS signature verification.packages/rs-dpp/src/identity/identity_public_key/v0/methods/mod.rs (2)
10-10: Imports correctly include necessary cryptographic dependenciesThe added imports for
bls_signaturesanded25519_dalekare appropriate for supporting BLS and EdDSA key types.
Line range hint
36-48: Ensure comprehensive error handling in BLS key conversionIn the
KeyType::BLS12_381match arm, errors frombls_signatures::PrivateKey::from_bytesandprivate_key.g1_element()are being converted intoOk(false). Confirm that this behavior is as intended and does not mask important errors. Additionally, ensure that all possible errors are appropriately handled to prevent potential issues during key validation.packages/rs-drive-abci/src/execution/check_tx/v0/mod.rs (1)
216-216: Imported modules are appropriately addedThe added imports of
Keypair,signer,Network, andPrivateKeyare necessary for key generation and cryptographic operations used in the test functions below.
| @@ -90,11 +90,11 @@ pub mod prelude { | |||
|
|
|||
| pub use bincode; | |||
| #[cfg(all(not(target_arch = "wasm32"), feature = "bls-signatures"))] | |||
| pub use bls_signatures; | |||
| pub use dashcore::bls_signatures; | |||
There was a problem hiding this comment.
💡 Codebase verification
Direct usage of bls_signatures detected outside of dashcore.
Several instances of bls_signatures are still being used directly in the codebase. To maintain consistency and align with the objective of centralizing imports through dashcore::bls_signatures, please ensure that all usages are routed accordingly.
Affected Files:
packages/rs-dpp/src/signing.rspackages/rs-dpp/src/identity/identity_public_key/key_type.rspackages/rs-drive-abci/src/platform_types/commit/mod.rspackages/rs-drive-proof-verifier/src/verify.rspackages/simple-signer/src/signer.rs- (and others as indicated by the search results)
🔗 Analysis chain
LGTM: BLS signatures import moved to dashcore.
This change aligns with the PR objective of moving the BLS Signatures import to Rust Dash Core. It centralizes the import through the dashcore crate while maintaining the existing feature flag and architecture conditions.
Let's verify the usage of this new import path in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new BLS signatures import path
# Test: Search for uses of bls_signatures from dashcore
rg --type rust 'use (crate::)?dashcore::bls_signatures'
# Test: Search for any remaining direct uses of bls_signatures
rg --type rust 'use bls_signatures'
Length of output: 153
Script:
#!/bin/bash
# Description: Comprehensive verification of bls_signatures usage in the codebase
# Test 1: Search for any occurrences of bls_signatures in the codebase
rg --type rust 'bls_signatures' --context 2
# Test 2: Ensure that all imports of bls_signatures are through dashcore
rg --type rust 'use\s+bls_signatures[^;]*;' | grep -v 'dashcore::bls_signatures'
# Test 3: Verify that there are no direct usages without the dashcore namespace
rg --type rust 'bls_signatures::' | grep -v 'dashcore::bls_signatures::'
Length of output: 30796
| @@ -8,7 +8,7 @@ use dashcore::secp256k1::rand::rngs::StdRng as EcdsaRng; | |||
| #[cfg(feature = "random-public-keys")] | |||
| use dashcore::secp256k1::rand::SeedableRng; | |||
| use dashcore::secp256k1::Secp256k1; | |||
| use dashcore::Network; | |||
| use dashcore::{bls_signatures, ed25519_dalek, Network}; | |||
There was a problem hiding this comment.
Make imports of bls_signatures and ed25519_dalek conditional on feature flags
The bls_signatures and ed25519_dalek modules are only used when the corresponding features (bls-signatures and ed25519-dalek) are enabled. Including these imports unconditionally can lead to compilation errors when these features are not enabled. To prevent this, wrap the imports with the appropriate #[cfg(feature = "...")] attributes.
Apply this diff to conditionally import the modules:
- use dashcore::{bls_signatures, ed25519_dalek, Network};
+ #[cfg(feature = "bls-signatures")]
+ use dashcore::bls_signatures;
+ #[cfg(feature = "ed25519-dalek")]
+ use dashcore::ed25519_dalek;
+ use dashcore::Network;📝 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.
| use dashcore::{bls_signatures, ed25519_dalek, Network}; | |
| #[cfg(feature = "bls-signatures")] | |
| use dashcore::bls_signatures; | |
| #[cfg(feature = "ed25519-dalek")] | |
| use dashcore::ed25519_dalek; | |
| use dashcore::Network; |
| @@ -2659,7 +2659,7 @@ mod tests { | |||
|
|
|||
| let secp = Secp256k1::new(); | |||
|
|
|||
| let new_key_pair = KeyPair::new(&secp, &mut rng); | |||
| let new_key_pair = Keypair::new(&secp, &mut rng); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated key pair generation code
The line let new_key_pair = Keypair::new(&secp, &mut rng); is duplicated in both the identity_update_with_non_master_key_check_tx and identity_update_with_encryption_key_check_tx test functions. Consider extracting this code into a helper function or a test utility to reduce duplication and improve maintainability.
Also applies to: 2763-2763
QuantumExplorer
added
the
cleanup
Issue being fixed or feature implemented
BLS Sigs are being used in different parts of the project, different versions could end up being build leading to longer compilation times.
What was done?
Moved BLS Sigs to Rust Dash Core.
How Has This Been Tested?
Passed tests.
Breaking Changes
Not breaking
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
dashcore-rpcdependency version across multiple packages tov0.15.8.