fix and create test for address generator #250
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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: 1
🧹 Outside diff range and nitpick comments (1)
packages/injective-testing/src/multi_test/address_generator.rs (1)
101-130: Consider enhancing test coverageWhile the current tests are good, consider these improvements:
- Add test cases for edge cases (e.g., error conditions)
- Extract the regex pattern length (39) into a constant to match ADDRESS_LENGTH
- Add more uniqueness test iterations
Example enhancement:
mod tests { use super::*; use regex::Regex; + const BECH32_ADDRESS_LENGTH: usize = 39; + const UNIQUENESS_TEST_ITERATIONS: usize = 100; #[test] fn test_generate_inj_address() { // ... existing tests ... + // Test multiple addresses for uniqueness + let mut addresses = std::collections::HashSet::new(); + for _ in 0..UNIQUENESS_TEST_ITERATIONS { + let addr = generate_inj_address(); + assert!(addresses.insert(addr.to_string()), "Generated duplicate address"); + } } + #[test] + fn test_address_length_constants() { + // Verify our constants are correctly related + assert_eq!(ADDRESS_LENGTH / 2, ADDRESS_BYTES); + assert_eq!(KECCAK_OUTPUT_BYTES - ADDRESS_BYTES, ADDRESS_BYTE_INDEX); + } }
📜 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 (3)
Cargo.toml(1 hunks)packages/injective-testing/Cargo.toml(2 hunks)packages/injective-testing/src/multi_test/address_generator.rs(3 hunks)
🔇 Additional comments (3)
packages/injective-testing/Cargo.toml (1)
8-8: LGTM! Dependencies align with implementation changes
The version bump and dependency changes appropriately support the address generator improvements:
- regex addition supports the new test validations
- secp256k1 rand feature enables the new key generation approach
Also applies to: 21-22
Cargo.toml (1)
36-36: Verify impact of major secp256k1 version upgrade
The upgrade from secp256k1 v0.6.2 to v0.30.0 is a significant version jump that may introduce breaking changes.
Also applies to: 38-38
✅ Verification successful
Let me gather more information about the actual usage of secp256k1 in the codebase.
Let me check the actual implementation to see how secp256k1 is being used.
secp256k1 upgrade is safe to proceed
The secp256k1 usage is limited to a test utility in packages/injective-testing for generating Injective addresses. The implementation only uses basic functionality (key generation, public key derivation) that has remained stable across versions. The code:
- Creates a new Secp256k1 context
- Generates a random SecretKey
- Derives PublicKey from SecretKey
- Uses the serialized public key for address generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all secp256k1 usages to verify compatibility
rg --type rust 'use.*secp256k1' -A 5
Length of output: 590
Script:
#!/bin/bash
# Search for actual secp256k1 function/type usages
rg --type rust "Secp256k1|SecretKey|PublicKey" -A 5
# Also check for any potential test files using secp256k1
fd -e rs "test" --exec grep -l "secp256k1"
Length of output: 1574
Script:
#!/bin/bash
# Get the full content of the file using secp256k1
cat packages/injective-testing/src/multi_test/address_generator.rs
Length of output: 4434
packages/injective-testing/src/multi_test/address_generator.rs (1)
82-85: LGTM! Address generation follows standard practices
The implementation correctly follows Ethereum-style address generation with proper Injective bech32 encoding.
| let secret_key = SecretKey::new(&mut rand::thread_rng()); | ||
|
|
||
| let public_key = PublicKey::from_secret_key(&secp256k1, &secret_key); | ||
|
|
||
| let public_key_array = &public_key.serialize()[1..]; | ||
|
|
There was a problem hiding this comment.
Security concern: Use OsRng instead of thread_rng for cryptographic operations
Using thread_rng() for cryptographic key generation is less secure than OsRng. For cryptographic operations, it's recommended to use the system's secure random number generator.
Apply this change:
- let secret_key = SecretKey::new(&mut rand::thread_rng());
+ let secret_key = SecretKey::new(&mut rand::OsRng);📝 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.
| let secret_key = SecretKey::new(&mut rand::thread_rng()); | |
| let public_key = PublicKey::from_secret_key(&secp256k1, &secret_key); | |
| let public_key_array = &public_key.serialize()[1..]; | |
| let secret_key = SecretKey::new(&mut rand::OsRng); | |
| let public_key = PublicKey::from_secret_key(&secp256k1, &secret_key); | |
| let public_key_array = &public_key.serialize()[1..]; |
| @@ -22,7 +22,7 @@ ethereum-types = { version = "0.5.2" } | |||
| hex = { version = "0.4.3", features = [ "serde" ] } | |||
| injective-cosmwasm = { version = "0.3.0", path = "./packages/injective-cosmwasm" } | |||
| injective-math = { version = "0.3.0", path = "./packages/injective-math" } | |||
| injective-std = { version = "1.13.2-auction", path = "./packages/injective-std" } | |||
| injective-std = { version = "1.13.2", path = "./packages/injective-std" } | |||
There was a problem hiding this comment.
Did 1.13.2 did not exist already ?
Summary by CodeRabbit
New Features
Bug Fixes