-
-
Notifications
You must be signed in to change notification settings - Fork 76
Added extended signing key support for cip8 #273
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
Added extended signing key support for cip8 #273
Conversation
I didn't see the Should be good now. |
After some testing, I'm not certain this is the appropriate solution. However, I don't know what is. I think ideally we could extract the non extended payment/staking signing and verification keys from the extended keys. That would simplify this and make it consistent. If someone could provide some guidance, I can work on implementing. |
Actually, this does work! When I was doing testing I used the wrong test wallet to validate. My validation test was to sign a message to get a validation key for Iagon, then perform a bunch of API calls to create/delete files and folders. Then I logged into the dashboard, which requires a CIP8 signed message (signed with Eternl). When I signed the message for Iagon, it successfully validated and let me perform API calls for my address. I think details about the hackiness of the solution may need to be worked out. I basically pulled apart the internals of the COSE library to achieve this. Maybe there is a better solution here. |
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #273 +/- ##
==========================================
+ Coverage 85.14% 85.29% +0.15%
==========================================
Files 26 26
Lines 2995 3006 +11
Branches 719 722 +3
==========================================
+ Hits 2550 2564 +14
+ Misses 335 332 -3
Partials 110 110
|
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.
Looks good to me. I don't think the solution is hacky. 😁
I guess one final question I would have before merging is whether we can autodetect whether the signing was done with an extended key. Right now I added an If this is a needed parameter, lets send it! I'm happy I've been able to contribute back. |
vk = BIP32ED25519PublicKey( | ||
public_key=verification_key[:32], chain_code=verification_key[32:] | ||
) |
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.
You are right, we can probably determine whether the key is an extended key by simply checking its length. 64 bytes means it is an extended key, while 32 bytes means non-extended key.
Just saw this PR-- awesome work @theeldermillenial :) |
Sorry that took a few days. Should be good to go now. What kind of a release schedule do you have with this? I have two different packages that are dependent on the different fixes I've implemented :) No rush, just curious. |
Fixes #270
This PR is an attempt at implementing extended signing key support for cip8 message signing and verification.
I am not confident that this is the correct approach, but it has the right pieces in place.
It wasn't clear to me whether I should include the public key plus the chain code for the extended verification key. It seemed obvious to me that you would, but I haven't played around with this. I did created a unit test, but it's not in depth.
I do have an application I can test against, but I thought I would open for comment.