feat: cosmos ADR-36 and EIP-191 signatures can be verified #256
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThis pull request expands the cryptographic capabilities of the project by adding new dependencies and introducing support for additional signing and verification algorithms. It adds new Cargo dependency declarations across several packages, introduces new enum variants and modules (including Cosmos-related signing and Bech32 conversion), and updates function implementations to handle the added algorithms. The changes also adjust signature verification calls to work directly with hash digest types. Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
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: 1
🔭 Outside diff range comments (2)
crates/keys/src/signatures.rs (1)
76-82:⚠️ Potential issueUpdate algorithm() method to handle new variants.
The
algorithm()method needs to be updated to handle the newEip191andCosmosAdr36variants, even if they're not implemented yet. This ensures the method remains exhaustive for all variants.pub fn algorithm(&self) -> CryptoAlgorithm { match self { Signature::Ed25519(_) => CryptoAlgorithm::Ed25519, Signature::Secp256k1(_) => CryptoAlgorithm::Secp256k1, Signature::Secp256r1(_) => CryptoAlgorithm::Secp256r1, + // These variants are not yet implemented + _ => unreachable!("Eip191 and CosmosAdr36 signatures are not implemented"), } }crates/keys/src/signing_keys.rs (1)
144-153:⚠️ Potential issueUpdate PartialEq implementation to handle new key types
The
PartialEqimplementation doesn't handle the newEip191andCosmosAdr36variants, which means equality checks for these types will always returnfalse. This could lead to subtle bugs when comparing keys.Consider updating the implementation to include the new variants:
impl PartialEq for SigningKey { fn eq(&self, other: &Self) -> bool { match (self, other) { (SigningKey::Ed25519(a), SigningKey::Ed25519(b)) => a.as_bytes() == b.as_bytes(), (SigningKey::Secp256k1(a), SigningKey::Secp256k1(b)) => a == b, (SigningKey::Secp256r1(a), SigningKey::Secp256r1(b)) => a == b, + (SigningKey::Eip191(a), SigningKey::Eip191(b)) => a == b, + (SigningKey::CosmosAdr36(a), SigningKey::CosmosAdr36(b)) => a == b, _ => false, } } }
🧹 Nitpick comments (10)
crates/keys/src/signatures.rs (1)
69-73: Improve error messages to be more descriptive.The error messages could be more informative about why these signatures are not implemented and what alternatives are available.
- CryptoAlgorithm::Eip191 => bail!("No EIP-191 specific signatures implemented"), - CryptoAlgorithm::CosmosAdr36 => { - bail!("No cosmos ADR-36 specific signatures implemented") - } + CryptoAlgorithm::Eip191 => bail!("EIP-191 specific signatures are not yet implemented. Use Secp256k1 signatures instead."), + CryptoAlgorithm::CosmosAdr36 => { + bail!("Cosmos ADR-36 specific signatures are not yet implemented. Use Secp256k1 signatures instead.") + }crates/keys/src/algorithm.rs (1)
14-17: Consider adding references to official EIP-191/ADR-36 documentation.
It can be helpful to mention or link to the official specs in the doc comments to guide maintainers and users.crates/serde/src/bech32.rs (1)
3-18: Add trait usage examples.
Providing usage samples or doc tests for theToBech32trait would help new contributors understand how to integrate it.crates/keys/src/cosmos.rs (4)
8-16: Document each field for clarity.
Adding doc comments that explain each field inCosmosSignDocwould make the structure more maintainable.
38-54: Allow setting chain ID in the constructor.
Currently, it defaults to an empty string. If chain ID is vital, consider a constructor parameter or builder pattern.
57-71: Replace or remove the debug print.
println!("Signer: {}", signer);can clutter logs in production or libraries. Consider a structured logging macro or removing it.- println!("Signer: {}", signer);
83-90: Support for other prefixes.
The TODO indicates future prefix customization. If a variable prefix is necessary soon, consider a wrapper or parameter forsigner_from_key.crates/keys/src/verifying_keys.rs (3)
92-95: Unsupported DER format.
Explicitly failing early for EIP-191 and Cosmos ADR-36 is fine, but consider clarifying error messages or implementing these later.
129-132: DER parsing unimplemented.
Like before, these placeholders are acceptable, but plan to either remove or implement them if needed.
146-191: Extended verification logic.
EIP-191 and Cosmos ADR-36 verification uses prehash flows. Consider additional signature-length checks before verifying.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.toml(2 hunks)crates/keys/Cargo.toml(2 hunks)crates/keys/src/algorithm.rs(2 hunks)crates/keys/src/cosmos.rs(1 hunks)crates/keys/src/lib.rs(1 hunks)crates/keys/src/signatures.rs(2 hunks)crates/keys/src/signing_keys.rs(7 hunks)crates/keys/src/verifying_keys.rs(11 hunks)crates/serde/Cargo.toml(1 hunks)crates/serde/src/bech32.rs(1 hunks)crates/serde/src/lib.rs(1 hunks)crates/tree/src/proofs.rs(1 hunks)crates/tree/src/snarkable_tree.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unused dependencies
- GitHub Check: unit-test
- GitHub Check: clippy
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
🔇 Additional comments (23)
crates/keys/src/lib.rs (1)
2-2: LGTM! Clean addition of the Cosmos module.The new
cosmosmodule is well-placed among other cryptographic modules and follows Rust's module visibility best practices.crates/serde/src/lib.rs (1)
2-2: LGTM! Clean addition of the Bech32 module.The new
bech32module is appropriately made public and maintains the alphabetical ordering of modules.crates/serde/Cargo.toml (1)
14-14: LGTM! Clean addition of the Bech32 dependency.The
bech32dependency is correctly specified using workspace version and appropriately placed in the serde section.crates/keys/src/signatures.rs (1)
53-57: Inconsistency with PR objectives.The PR title suggests that Cosmos ADR-36 and EIP-191 signatures can be verified, but the implementation returns "not implemented" errors. Please clarify if this is intentional or if the implementation is missing.
Likely an incorrect or invalid review comment.
crates/tree/src/snarkable_tree.rs (1)
142-142: Signature verification now using Digest directlyThe signature verification now accepts the
hashparameter directly as aDigesttype instead of converting it to bytes first with&hash.to_bytes(). This change aligns with the PR objective to support additional signature verification algorithms.crates/keys/Cargo.toml (2)
13-13: Added serde_json dependencyAdding serde_json as a workspace dependency is appropriate for JSON serialization/deserialization needs.
23-25: Added new signature dependenciesThe added dependencies (
alloy-primitivesandripemd) are necessary for implementing the new signature verification algorithms (Cosmos ADR-36 and EIP-191) mentioned in the PR objectives.Cargo.toml (2)
59-59: Added bech32 dependencyThe bech32 dependency is necessary for Cosmos address encoding/decoding, which supports the Cosmos ADR-36 signature verification mentioned in the PR objectives.
109-114: Added new signature verification dependenciesThe added dependencies (
alloy-primitiveswith k256 feature andripemd) provide the necessary functionality for implementing the EIP-191 and Cosmos ADR-36 signature verification algorithms.It's good that you've specified a restricted feature set for
alloy-primitiveswith just thek256feature to minimize dependencies.crates/tree/src/proofs.rs (1)
144-144: Signature verification now using Digest directlySimilar to the change in
snarkable_tree.rs, the signature verification now accepts thehashparameter directly as aDigesttype instead of converting it to bytes first. This creates a consistent approach to signature verification across the codebase.crates/keys/src/algorithm.rs (1)
28-29: New match arms look correct.
The new variants for EIP-191 and Cosmos ADR-36 are properly handled in thefrom_strimplementation.crates/serde/src/bech32.rs (1)
20-34: Implementation looks solid.
Decoding logic usingbech32::decodeis straightforward and consistent with the trait design.crates/keys/src/cosmos.rs (4)
18-22: Confirm string-based amounts.
UsingStringfor numeric fields like gas and amount is common in Cosmos code, but ensure you handle potential parsing or localization issues.
24-29: No immediate concerns.
TheCosmosMessagestructure is succinct and follows standard naming.
31-36: Field-level serializing logic appears correct.
The usage ofraw_or_b64is a neat approach to handle raw/b64-encoded data.
73-81: Double-check special-character escaping.
Replacing<,>, and&in JSON is sometimes done for HTML safety, but verify if it’s a required step in ADR-36.crates/keys/src/verifying_keys.rs (6)
40-45: Additional variants are well integrated.
EIP-191 and Cosmos ADR-36 variants align with the existing scheme.
63-70: Proper hashing approach for new variants.
Including unique hash identifiers (3 and 4) for EIP-191 and Cosmos ADR-36 ensures correctness in hashing.
82-83: Consistent byte retrieval.
Returning sec1 bytes for EIP-191 and Cosmos ADR-36 keeps the approach uniform.
111-116: Variant handling for from_algorithm_and_bytes is correct.
The logic matches the newly introduced algorithms without any obvious issues.
141-142: Enum to algorithm mapping is straightforward.
The new variants map directly to the correctCryptoAlgorithm.
255-256: Consistent with otherFromimpls.
Automatically converting aSigningKeyvariant to the correspondingVerifyingKeyvariant remains consistent.crates/keys/src/signing_keys.rs (1)
36-37: Implementation of new signing algorithms looks goodThe implementation approach for the new signing algorithms is well-structured. Both new variants reuse the
Secp256k1SigningKeytype since they use the same underlying key format but with different signature algorithms.The construction, serialization, and deserialization methods are properly implemented to support the new key types.
Also applies to: 53-59, 66-67, 80-81, 96-101, 110-111
|
This is good. For the next iteration we should add EIP-712, which is more common and UX friendly in the eth ecosystem. Will show users even more transparently what they are signing! |
jns-ps
force-pushed
the
wallet-signatures
branch
from
79fd8a1 to
609e2a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/keys/src/signing_keys.rs (1)
130-139:⚠️ Potential issueImprove error handling in signature generation
The current implementation uses
unwrap()which will panic if an error occurs during signing. This doesn't follow the error handling pattern used elsewhere in the code.Consider using proper error handling instead of
unwrap():SigningKey::Eip191(sk) => { let message = eip191_hash_message(message); - let sig: Secp256k1Signature = sk.sign_prehash(message.as_slice()).unwrap(); + let sig: Secp256k1Signature = sk.sign_prehash(message.as_slice()) + .map_err(|e| anyhow::anyhow!("Failed to sign EIP-191 message: {}", e))?; Signature::Secp256k1(sig) } SigningKey::CosmosAdr36(sk) => { - let message = cosmos_adr36_hash_message(message, sk.verifying_key()).unwrap(); - let sig: Secp256k1Signature = sk.sign_prehash(message.as_slice()).unwrap(); + let message = cosmos_adr36_hash_message(message, sk.verifying_key()) + .map_err(|e| anyhow::anyhow!("Failed to hash Cosmos ADR-36 message: {}", e))?; + let sig: Secp256k1Signature = sk.sign_prehash(message.as_slice()) + .map_err(|e| anyhow::anyhow!("Failed to sign Cosmos ADR-36 message: {}", e))?; Signature::Secp256k1(sig) }Note that this would require changing the return type of the
signmethod toResult<Signature, anyhow::Error>.
🧹 Nitpick comments (5)
crates/keys/src/tests.rs (2)
54-54: Skipping DER for select algorithms
A comment clarifies why EIP-191 and Cosmos ADR-36 lack DER support. If future requirements change, adding DER handling would be beneficial.
144-144: Note on omitted EIP-191 and Cosmos signatures
Suggests these signatures rely on secp256k1 logic. Consider adding explicit tests if advanced handling is needed later.crates/keys/src/cosmos.rs (2)
18-22: CosmosFee type
Representing fee values as strings might invite parsing overhead elsewhere—still acceptable if usage remains minimal.
38-54: Constructor for CosmosSignDoc
new()supplies default chain, account, and sequence values. If future expansions require them, consider making them configurable.crates/keys/src/signing_keys.rs (1)
5-6: Consider importing K256DigestSigner for consistency.You're using the PrehashSigner trait but might want to also import K256DigestSigner for a more complete and consistent import set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(2 hunks)crates/keys/Cargo.toml(2 hunks)crates/keys/src/algorithm.rs(2 hunks)crates/keys/src/cosmos.rs(1 hunks)crates/keys/src/lib.rs(1 hunks)crates/keys/src/signatures.rs(2 hunks)crates/keys/src/signing_keys.rs(8 hunks)crates/keys/src/tests.rs(6 hunks)crates/keys/src/verifying_keys.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/keys/src/lib.rs
- crates/keys/src/signatures.rs
- crates/keys/Cargo.toml
- Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: unused dependencies
- GitHub Check: build-and-push-image
- GitHub Check: integration-test
🔇 Additional comments (26)
crates/keys/src/algorithm.rs (3)
14-18: Well-structured new enum variants
No issues stand out; introducingEip191andCosmosAdr36cleanly expands the supported algorithms.
20-30: Comprehensive method for enumerating all algorithms
Providing a dedicatedall()method to return every variant is a neat approach that helps ensure future expansions remain testable.
41-42: Consistent string matching
Matching"eip191"and"cosmos_adr36"aligns well with the existing pattern, maintaining clarity and consistency.crates/keys/src/tests.rs (10)
4-4: New imports for cryptographic structures
ImportingCryptoAlgorithm,Signature,SigningKey, andVerifyingKeyis appropriately aligned with the extended test coverage.
6-6: Base64 conversion utility
AdoptingFromBase64andToBase64is a straightforward solution for handling the signature key material in a test environment.
34-49: Extended coverage for verifying keys
Verifying that EIP-191 and Cosmos ADR-36 keys round-trip correctly viafrom_algorithm_and_bytesstrengthens confidence in the new features.
64-70: Secp256k1 re-parsing from DER
Good consistency with other curves; ensuring test coverage for DER path fosters interoperability.
99-105: EIP-191 signing key round-trip
Adding a re-parsing test for EIP-191 helps ensure serialization and deserialization correctness.
107-113: Cosmos ADR-36 signing key test
Clear demonstration of round-tripping Cosmos ADR-36 signing keys. Strengthens confidence in the approach.
167-168: Duplicate comment
Repeated note about EIP-191 and Cosmos ADR-36 using secp256k1 signatures.
170-199: Robust multi-algorithm verification
Iterating overCryptoAlgorithm::all()enhances coverage, ensuring each variant (old or new) undergoes end-to-end testing.
201-232: Real-world EIP-191 interoperability test
Using an actual Metamask-generated signature validates cross-environment compatibility for EIP-191. Nicely handled.
234-258: Integration test for Cosmos ADR-36
Ensures Keplr wallet signatures are recognized, confirming practical support for ADR-36.crates/keys/src/cosmos.rs (7)
1-7: Appropriate usage of cryptographic crates
Usingk256for secp256k1,Ripemd160, andsha2is standard and ensures reliable Cosmos address derivation.
8-16: Structured CosmosSignDoc
Cleanly captures all relevant fields for ADR-36. This design is straightforward and easy to extend.
24-29: CosmosMessage structure
Renamingtypetomsg_typeavoids collisions with Rust keywords and clarifies the intended field.
31-36: CosmosMessageValue handling
Good use ofraw_or_b64for binary data. This preserves flexibility in how payloads are provided.
57-81: cosmos_adr36_hash_message function
Follows the ADR-36 flow: build sign doc, serialize, then hash with SHA-256. Straightforward and secure.
83-103: Escaping JSON
Using string replacements for<,>, and&is a known approach for mitigating potential JSON injection. Verify no partial overlap or double-escaping occurs in edge cases.
105-127: Bech32 address derivation
Adheres to the classic 4-step process (SHA-256 → RIPEMD160 → bech32). Implementation looks correct and robust.crates/keys/src/verifying_keys.rs (4)
43-45: Clean implementation of new verification key variants.The addition of
Eip191andCosmosAdr36variants to theVerifyingKeyenum is well-designed and clearly documented with appropriate comments that explain their purpose.
146-146: Improved method signature for better flexibility.Changing the signature from
&[u8]toimpl AsRef<[u8]>is a good improvement that allows for more flexible usage without any performance impact.
176-183: Well-implemented EIP-191 verification logic.The implementation correctly uses the
eip191_hash_messagefunction to prehash the message before verification and includes proper error handling and informative error messages.
184-191: Comprehensive Cosmos ADR-36 verification with proper error handling.The implementation correctly uses the
cosmos_adr36_hash_messagefunction with appropriate error propagation and informative error messages.crates/keys/src/signing_keys.rs (2)
36-37: Well-structured implementation of new signing key variants.The addition of
Eip191andCosmosAdr36variants to theSigningKeyenum maintains consistency with the correspondingVerifyingKeyvariants.
53-59: Well-implemented constructor methods.The implementation of
new_eip191andnew_cosmos_adr36follows the pattern established by other constructors in the codebase.
This can now occur, because different verifying keys are using the same backing type.
jns-ps
force-pushed
the
wallet-signatures
branch
from
609e2a5 to
abcfa54
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/common/src/builder.rs (1)
108-109: Consider preserving the underlying error context.Currently, the
map_errcall discards the underlying signing error details. It might be beneficial to include the original error message or context to aid with debugging. For example:-service_signing_key.sign(hash).map_err(|_| TransactionError::SigningFailed)?; +service_signing_key.sign(hash).map_err(|e| { + TransactionError::SigningFailed(format!("Signing failed: {}", e)) +})?;crates/keys/src/tests.rs (1)
54-54: Consider adding DER coverage for additional algorithms.Currently, EIP-191 and Cosmos ADR-36 verification steps are skipped for DER-based testing. While they may not rely on DER encoding, any partial coverage or explicit tests could further strengthen reliability.
crates/keys/src/cosmos.rs (1)
83-103: Ensuring ADR-36 compliance by escaping HTML characters.The escaping of
<,>, and&is useful for reducing injection risks. For broader coverage, consider escaping additional characters if the environment is untrusted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
crates/common/src/builder.rs(1 hunks)crates/common/src/test_transaction_builder.rs(8 hunks)crates/common/src/transaction.rs(1 hunks)crates/da/src/lib.rs(1 hunks)crates/keys/src/algorithm.rs(2 hunks)crates/keys/src/cosmos.rs(1 hunks)crates/keys/src/signing_keys.rs(7 hunks)crates/keys/src/tests.rs(4 hunks)crates/keys/src/verifying_keys.rs(11 hunks)crates/node_types/prover/src/prover/mod.rs(1 hunks)crates/tree/src/tests.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/keys/src/algorithm.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit-test
- GitHub Check: unused dependencies
- GitHub Check: build-and-push-image
- GitHub Check: integration-test
🔇 Additional comments (47)
crates/tree/src/tests.rs (1)
231-231: Improved error handling by explicitly unwrapping the sign result.This change makes the error handling more explicit by unwrapping the
Resultreturned by thesignmethod. This is appropriate in a test context where we want immediate failure if the signing operation fails.crates/common/src/transaction.rs (1)
27-27: Improved error handling for signing operation.The change now maps any error from the signing operation to a specific
TransactionError::SigningFailedtype, providing clearer error information to callers.crates/common/src/test_transaction_builder.rs (2)
190-190: Now explicitly handling sign operation errors.This change ensures that the code will fail fast with a clear error message if signing fails.
344-344: Consistent error handling added for all sign operations.All sign method calls now use
unwrap()to handle potential errors, which aligns with the changes in the signing API. This ensures that any signing failures will be immediately apparent.Also applies to: 357-357, 389-390, 406-407, 489-490, 504-505, 517-518
crates/da/src/lib.rs (1)
40-44: Enhanced error handling in insert_signature method.The method signature has been updated to return a
Result<()>instead of void, and now properly propagates errors from the signing operation. This change improves the robustness of the cryptographic operations by allowing callers to handle signing failures.crates/node_types/prover/src/prover/mod.rs (1)
433-433: Now propagating errors from signature insertion.This change properly handles potential errors during signature insertion by using the
?operator, which aligns with the updatedinsert_signaturemethod signature that now returns aResult.crates/keys/src/tests.rs (10)
3-4: Good addition of new cryptographic enums and types.The newly added import references
CryptoAlgorithm, which is essential for handling multiple cryptographic variants in the tests. This integration looks appropriate.
6-6: Nice usage of base64 conversions.This ensures that we can parse base64-encoded crypto material in test scenarios without reinventing the wheel.
35-49: Expanded coverage for EIP-191 and Cosmos ADR-36 verifying keys.These lines ensure that verifying keys for EIP-191 and ADR-36 are correctly re-parsed from raw bytes. This coverage is crucial for guaranteeing compatibility.
64-70: Verifying secp256k1 DER parsing.Ensuring that DER-encoded secp256k1 verifying keys match their non-DER equivalents is essential for seamless interoperability with external tools.
99-113: Excellent coverage for EIP-191 and Cosmos ADR-36 signing keys.These lines confirm that the new signing keys can be serialized and re-parsed accurately, retaining their cryptographic properties.
120-144: Comprehensive signature re-parsing for multiple algorithms.This test covers Ed25519, secp256k1, and secp256r1. A note explains that EIP-191 and Cosmos ADR-36 also use secp256k1-based signatures, hence no separate coverage is provided here.
151-168: DER-based signature tests for secp256k1 and secp256r1.These blocks confirm that DER decoding for secp256k1 and secp256r1 signatures is consistent. EIP-191 and Cosmos ADR-36 are excluded due to raw signature usage.
170-199: Broad coverage for signature creation and verification.Verifying all supported algorithms in a loop is an effective strategy for ensuring consistent behavior across different cryptographic backends.
201-232: Integration test with EIP-191 wallet signatures.These lines confirm real-world compatibility by verifying a wallet-generated signature. This is critical for production readiness within Ethereum-based workflows.
234-258: Integration test with Cosmos ADR-36 wallet signatures.Testing with Keplr-generated signatures ensures correctness in actual Cosmos environments. This helps validate end-to-end functionality.
crates/keys/src/cosmos.rs (8)
1-7: Sensible crates for Cosmos-based signing logic.
sha2andripemdare standard for generating Cosmos addresses, and the included bech32 utility further ensures overall compatibility.
8-16: Well-structured sign doc.
CosmosSignDocincludes essential fields (msgs,fee,chain_id, etc.), aligning with ADR-36 expectations for message signing on Cosmos.
18-22: Flexible design for fees.Representing fee amounts as
Vec<String>accommodates various token denominations. Numeric types could be considered for stricter type safety, but this is a solid approach.
24-29: Correct usage of#[serde(rename = "type"].This ensures JSON compatibility with the typical Cosmos message structure, preserving the
typefield naming convention.
31-36: Using#[serde(with = "raw_or_b64")]for message data.Handling binary data with either raw bytes or base64 encoding helps interoperate smoothly with standard Cosmos message formats.
38-55: Constructor with sensible defaults.Users can quickly create a
CosmosSignDocby providing just the signer and data, while other fields default to typical ADR-36 placeholders.
57-81: Well-documented ADR-36 hashing function.Returning a
Result<Vec<u8>>is a good practice, allowing upper layers to handle any cryptographic or serialization errors gracefully.
105-126: Standard Cosmos address derivation procedure.Sequential SHA-256 followed by RIPEMD160, plus bech32 encoding, matches Cosmos norms. This ensures compatibility with major Cosmos wallets.
crates/keys/src/verifying_keys.rs (12)
1-1: LGTM: Appropriate imports added for the new signature verification schemesThe imports for
eip191_hash_messagefrom alloy_primitives and the updated imports from the crate includingcosmos_adr36_hash_messageare correctly added to support the new functionality.Also applies to: 26-29
42-45: LGTM: New VerifyingKey variants properly documentedThe two new variants
Eip191andCosmosAdr36are added with appropriate documentation comments explaining their purpose. This makes it clear that these variants are used specifically for verifying signatures according to EIP-191 and Cosmos ADR-36 standards.
63-70: LGTM: Hash implementation properly updated for new variantsThe Hash implementation is correctly extended to include the new VerifyingKey variants, with unique identifiers (3 and 4) assigned to each variant.
82-83: LGTM: Byte representation for new variants implementedThe to_bytes method is properly extended to handle the new variants, correctly reusing the SEC1 byte representation of the Secp256k1 key.
92-95: LGTM: Proper error handling for unsupported DER formatThe to_der method is appropriately updated to handle the new variants with clear error messages stating that DER format is not supported for these verification key types.
111-116: LGTM: Key creation from bytes implemented for new variantsThe from_algorithm_and_bytes method is correctly updated to support the new variants, properly creating the appropriate VerifyingKey type based on the algorithm and byte data.
129-132: LGTM: Error handling for unsupported DER format in key creationThe from_algorithm_and_der method is properly updated with clear error messages for the new algorithms when attempting to create keys from DER format.
141-142: LGTM: Algorithm mapping for new variantsThe algorithm method is properly updated to return the correct CryptoAlgorithm for the new variants.
146-146: LGTM: Improved function signature for verify_signature methodThe signature verification method now accepts
impl AsRef<[u8]>instead of&[u8], which provides more flexibility in the types of message inputs that can be verified.
176-183: LGTM: EIP-191 signature verification properly implementedThe verification logic for EIP-191 signatures is implemented correctly:
- It ensures the signature type is compatible (Secp256k1)
- It properly hashes the message using
eip191_hash_message- It verifies the signature against the prehashed message
- It provides clear error messages if verification fails
184-191: LGTM: Cosmos ADR-36 signature verification properly implementedThe verification logic for Cosmos ADR-36 signatures is implemented correctly:
- It ensures the signature type is compatible (Secp256k1)
- It properly hashes the message using
cosmos_adr36_hash_message- It verifies the signature against the prehashed message
- It provides clear error messages if verification fails
219-220: LGTM: SigningKey to VerifyingKey conversion updated for new variantsThe From implementation is correctly updated to support converting the new SigningKey variants to their corresponding VerifyingKey variants.
crates/keys/src/signing_keys.rs (11)
1-1: LGTM: Appropriate imports added for the new signing schemesThe imports for
eip191_hash_message, signature-related traits, and crate modules includingcosmos_adr36_hash_messageare correctly added to support the new signing functionality.Also applies to: 5-6, 12-15
36-37: LGTM: New SigningKey variants addedThe two new variants
Eip191andCosmosAdr36are properly added to the SigningKey enum, both using Secp256k1SigningKey as their underlying implementation.
53-59: LGTM: Constructor methods added for new key typesThe new methods
new_eip191()andnew_cosmos_adr36()are properly implemented to create random signing keys for the new variants using secure random number generation.
66-67: LGTM: Algorithm-based key creation updatedThe
new_with_algorithmmethod is correctly updated to handle the new CryptoAlgorithm variants.
80-81: LGTM: Byte representation for new variants implementedThe to_bytes method is properly extended to handle the new variants, correctly reusing the byte representation of the Secp256k1 key.
96-101: LGTM: Key creation from bytes implemented for new variantsThe from_algorithm_and_bytes method is correctly updated to support the new variants, properly creating the appropriate SigningKey type based on the algorithm and byte data.
110-111: LGTM: Algorithm mapping for new variantsThe algorithm method is properly updated to return the correct CryptoAlgorithm for the new variants.
115-115: LGTM: Improved function signature and return type for sign methodThe signing method now:
- Returns
Result<Signature>to properly handle errors- Accepts
impl AsRef<[u8]>instead of&[u8], which provides more flexibility
130-134: LGTM: EIP-191 signing properly implementedThe signing logic for EIP-191 is implemented correctly:
- It properly hashes the message using
eip191_hash_message- It signs the prehashed message using the Secp256k1 key
- It correctly handles errors through the Result type
135-139: LGTM: Cosmos ADR-36 signing properly implementedThe signing logic for Cosmos ADR-36 is implemented correctly:
- It properly hashes the message using
cosmos_adr36_hash_messagewith the verifying key- It signs the prehashed message using the Secp256k1 key
- It correctly handles errors through the Result type
The implementation properly addresses the error handling concerns raised in previous reviews by using
?instead ofunwrap().
150-151: LGTM: PartialEq implementation updated for new variantsThe PartialEq implementation is correctly extended to include equality comparison for the new SigningKey variants.
Summary by CodeRabbit
New Features
Chores