fix: check the key byte length generically #245
Conversation
WalkthroughThe changes modify the key handling and trait implementations in two files. In Changes
Sequence Diagram(s)sequenceDiagram
participant Internal
participant CompressedKey
Note right of CompressedKey: `new()` is now private
Internal ->>+ CompressedKey: Request key creation
CompressedKey -->>- Internal: Returns CompressedKey instance
sequenceDiagram
participant Caller
participant RistrettoPublicKey
Caller ->> RistrettoPublicKey: Call from_canonical_bytes(bytes)
alt bytes length equals Self::KEY_LEN
RistrettoPublicKey -->> Caller: Return valid key object
else
RistrettoPublicKey -->> Caller: Return ByteArrayError::IncorrectLength
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
sdbondi
force-pushed
the
compressed-generic-key-len
branch
from
ce5e3e8 to
670eca1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
src/compressed_key.rs (3)
133-137:⚠️ Potential issueApply PublicKey trait bound to other implementations.
Several implementations are missing the
PublicKeytrait bound, causing pipeline failures. Methods liketo_hex(),as_bytes(), andfrom_canonical_bytes()are provided by traits that now requireT: PublicKey.-impl<T> Hashable for CompressedKey<T> { +impl<T: PublicKey> Hashable for CompressedKey<T> { -impl<T> Hash for CompressedKey<T> { +impl<T: PublicKey> Hash for CompressedKey<T> { -impl<T> fmt::Display for CompressedKey<T> { +impl<T: PublicKey> fmt::Display for CompressedKey<T> { -impl<T> ConstantTimeEq for CompressedKey<T> { +impl<T: PublicKey> ConstantTimeEq for CompressedKey<T> { -impl<T> fmt::LowerHex for CompressedKey<T> { +impl<T: PublicKey> fmt::LowerHex for CompressedKey<T> { -impl<T> fmt::UpperHex for CompressedKey<T> { +impl<T: PublicKey> fmt::UpperHex for CompressedKey<T> { -impl<T> fmt::Debug for CompressedKey<T> { +impl<T: PublicKey> fmt::Debug for CompressedKey<T> { -impl<T> PartialEq for CompressedKey<T> { +impl<T: PublicKey> PartialEq for CompressedKey<T> { -impl<T> Eq for CompressedKey<T> {} +impl<T: PublicKey> Eq for CompressedKey<T> {} -impl<T> PartialOrd for CompressedKey<T> { +impl<T: PublicKey> PartialOrd for CompressedKey<T> { -impl<T> Ord for CompressedKey<T> { +impl<T: PublicKey> Ord for CompressedKey<T> { -impl<T> Zeroize for CompressedKey<T> { +impl<T: PublicKey> Zeroize for CompressedKey<T> {Also applies to: 139-144, 153-157, 159-163, 165-175, 177-181, 183-189, 191-201, 224-230
232-264:⚠️ Potential issueAdd PublicKey trait bound to serde implementations.
The serde implementations also need the
PublicKeytrait bound to fix the pipeline failures related tofrom_hex(),to_hex(), andas_bytes()methods.-impl<'de, T> Deserialize<'de> for CompressedKey<T> { +impl<'de, T: PublicKey> Deserialize<'de> for CompressedKey<T> { -impl<T> Serialize for CompressedKey<T> { +impl<T: PublicKey> Serialize for CompressedKey<T> {Additionally, the visitor implementation might need adjustment:
- impl<T> Visitor<'_> for CompressedKeyVisitor<T> { + impl<T: PublicKey> Visitor<'_> for CompressedKeyVisitor<T> {Also applies to: 266-277
🧰 Tools
🪛 GitHub Actions: Test
[error] 259-259: the trait bound
T: PublicKeyis not satisfied🪛 GitHub Actions: Formatting, lints, and code checks
[error] 232-232: Formatting issue: Incorrect placement of 'where' clause. Expected 'where D: Deserializer<'de>' on the same line.
[error] 247-247: Formatting issue: Incorrect placement of 'where' clause. Expected 'where E: de::Error' on the same line.
114-130:⚠️ Potential issueAdd PublicKey trait bound to borsh implementations.
The borsh implementations also need the
PublicKeytrait bound as they useas_bytes()andfrom_canonical_bytes().-impl<T: borsh::BorshSerialize> borsh::BorshSerialize for CompressedKey<T> { +impl<T: borsh::BorshSerialize + PublicKey> borsh::BorshSerialize for CompressedKey<T> { -impl<T: borsh::BorshDeserialize> borsh::BorshDeserialize for CompressedKey<T> { +impl<T: borsh::BorshDeserialize + PublicKey> borsh::BorshDeserialize for CompressedKey<T> {🧰 Tools
🪛 GitHub Actions: Test
[error] 117-117: the method
as_bytesexists for reference&CompressedKey<T>, but its trait bounds were not satisfied
[error] 128-128: the function or associated item
from_canonical_bytesexists for structCompressedKey<T>, but its trait bounds were not satisfied🪛 GitHub Actions: Formatting, lints, and code checks
[error] 121-121: Formatting issue: Incorrect placement of 'where' clause. Expected 'where R: borsh::io::Read' on the same line.
🛑 Comments failed to post (2)
src/compressed_key.rs (2)
124-126:
⚠️ Potential issueFix the formatting of where clauses.
The pipeline failures show that these where clauses should be on the same line as the preceding line, not indented on separate lines.
- where - R: borsh::io::Read, - { + where R: borsh::io::Read {Apply similar fixes to all other where clauses to resolve the formatting errors.
Also applies to: 209-211, 235-237, 250-252, 269-271
203-213: 💡 Verification agent
🧩 Analysis chain
Good use of T::KEY_LEN and PublicKey trait bound.
Adding the
PublicKeytrait bound to theByteArrayimplementation forCompressedKey<T>ensures thatThas aKEY_LENconstant, and replacing the hardcoded key length check withT::KEY_LENmakes the code more adaptable to different key sizes.However, there are several pipeline failures showing trait bound issues. The
PublicKeytrait bound needs to be applied consistently across other implementations that use methods requiring it.
🏁 Script executed:
#!/bin/bash # Identify places where trait bounds are needed but missing # Find places where to_hex or as_bytes are used without proper trait bounds echo "Checking for missing trait bounds when using ByteArray methods:" rg -A 2 "to_hex|as_bytes" src/compressed_key.rs | grep -v "where.*PublicKey" # Find other implementations that might need the PublicKey trait bound echo "Checking other implementations that might need PublicKey trait bound:" rg "impl<T>" src/compressed_key.rs | grep -v "PublicKey"Length of output: 1758
Action Required: Consistent PublicKey Trait Bound Across Implementations
The use of
T::KEY_LENin theByteArrayimplementation is well done, ensuring that the key length check is flexible and adaptable. However, our pipeline failures indicate that several other implementations ofCompressedKey<T>(e.g., those invokingto_hexandas_bytes) lack thePublicKeytrait bound. This omission could lead to inconsistencies or errors in contexts where methods depend on a validKEY_LENfrom the trait.Recommendations:
- Review and update all implementations for
CompressedKey<T>(includingHashable,Hash,fmt::Display,fmt::LowerHex,fmt::UpperHex,fmt::Debug,PartialEq,PartialOrd,Ord,Zeroize,Visitor, andSerialize) to ensure they consistently include thePublicKeytrait bound.- Ensure that every method using functionalities like
to_hexandas_bytesis appropriately constrained by thePublicKeytrait so thatT::KEY_LENis always available.Please address these missing trait bounds to resolve the pipeline failures.
🧰 Tools
🪛 GitHub Actions: Formatting, lints, and code checks
[error] 206-206: Formatting issue: Incorrect placement of 'where' clause. Expected 'where Self: Sized' on the same line.
|
Looks like more work is needed if ByteArray impl has the PublicKey trait bound - closing for now |
CompressedKey assumes the key length is 32 in
from_canonical_bytes.Made
CompressedKey::newprivate since it is not used externally and needs to be used correctly, as it does not check byte length or if the compressed key bytes are validSummary by CodeRabbit