-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/sign cmd #595
Fix/sign cmd #595
Conversation
WalkthroughThe recent update enhances the digital signing capabilities in the project by integrating new cryptographic standards. Specifically, it broadens the scope of signature suites to include ECDSA secp256k1 and Ed25519. This is achieved by dynamically selecting the appropriate signature suite based on the public key type, ensuring that the signing process for verifiable credentials is both flexible and secure. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client/credential/sign.go (4 hunks)
Additional comments: 4
client/credential/sign.go (4)
- 14-15: The addition of imports for
ecdsasecp256k1signature2019
anded25519signature2020
from thearies-framework-go
package aligns with the PR's objective to support dynamic signature suite selection based on the public key type. This is a crucial step in enabling the functionality described in the PR objectives.- 25-26: The imports for
ed25519
andsecp256k1
from thecosmos-sdk/crypto/keys
package are necessary for the implementation of dynamic signature suite selection. These imports allow the code to perform type assertions on the public keys to determine the appropriate signature suite.- 214-214: The
expandPath
function has been modified to address the VC File Path Expansion issue mentioned in the PR objectives. This change ensures that file paths starting with the "~" symbol are correctly expanded to the user's home directory. This is a critical fix for the usability of thecredential sign
command, especially when dealing with file paths provided by the user.- 255-280: The
signVerifiableCredential
function now includes logic for dynamic signature suite selection based on the public key type, which is a significant improvement as outlined in the PR objectives. This change allows for the use of eitherEcdsaSecp256k1Signature2019
orEd25519Signature2020
depending on the type of the public key, enhancing the versatility and security of the credential signing process.However, it's important to ensure that the error message in lines 278-279 is clear and helpful for debugging purposes. The current message provides the expected public key types, which is good practice. Ensuring that error messages are informative aids in troubleshooting and improves the developer experience.
Consider adding tests to verify that the dynamic selection behaves as expected with different types of public keys. This could help catch any potential issues with the type assertions or the integration with the
aries-framework-go
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
🎉 This PR is included in version 7.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR aims to fix some issues related to the
credential sign
command.Details
VC file path
The provided credential file path to sign is now properly expanded.
Signature
The crypto suite used to sign the credential was systematically the
Ed25519Signature2018
independently of the public key type used to sign.We now support both
EcdsaSecp256k1Signature2019
&Ed25519Signature2020
depending on the public key type.Summary by CodeRabbit