Skip to content
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

feat: expose API for EIP2333 key derivation #110

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bochaco
Copy link

@bochaco bochaco commented Dec 18, 2023

No description provided.

@bochaco bochaco marked this pull request as ready for review December 18, 2023 18:38
@bochaco bochaco marked this pull request as draft December 18, 2023 18:38
@bochaco bochaco force-pushed the feat-apis-for-eip2333-key-derivation branch 3 times, most recently from 8f06b9a to ac1f52f Compare December 18, 2023 21:28
/// Wrapper for 'os_perso_derive_eip2333'
pub fn eip2333_derive(path: &[u32], key: &mut [u8]) -> Result<(), CxError> {
unsafe {
os_perso_derive_eip2333(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's what we have done already, but shouldn't we change it to use the no_throw version of the syscalls / cx_calls?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had tried to use os_derive_eip2333_no_throw, but it's not finding it in scope, the same happens with bip32 one, is there a flag/define/env var missing to have these versions exported by https://github.com/LedgerHQ/ledger-secure-sdk or they are just not being exported?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, they are static inline function defines in os_perso.h @yhql maybe it can't be used to generate bindings?

@@ -495,6 +508,19 @@ impl SeedDerive for Stark256 {
}
}

impl SeedDerive for Bls12381G1 {
type Target = ECPrivateKey<32, 'E'>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bls12381G1 is a Weierstrass curve, so should be ECPrivateKey<32, 'W'> ideally

@@ -530,6 +556,7 @@ impl_curve!(BrainpoolP512R1, 64, 'W');
impl_curve!(BrainpoolP512T1, 64, 'W');
impl_curve!(Stark256, 32, 'W');
impl_curve!(Ed25519, 32, 'E');
impl_curve!(Bls12381G1, 32, 'E'); // TODO: review type of curve (...and sigining impl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should not do this and expose the usual signing APIs yet. EdDSA or ECDSA are typically not used on BLS12381G1. We need to find a way to expose signing on BLS12381G2 though, which is a BLS signature, not ECDSA or EdDSA, and whose keys must be derived with eip2333 using G1

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, apart from impl. details w.r.t. being able to use the impl_curve! or not that I'll try to work out, I've found there is ox_bls12381_sign, is that supposed to be good enough for BLS12381G2 sig?

@bochaco bochaco force-pushed the feat-apis-for-eip2333-key-derivation branch 5 times, most recently from fa2250a to 026d5a8 Compare December 27, 2023 16:18
@bochaco bochaco force-pushed the feat-apis-for-eip2333-key-derivation branch from 026d5a8 to 48a7ceb Compare January 22, 2024 08:59
@yogh333
Copy link
Contributor

yogh333 commented Mar 11, 2024

Could we rename this PR into "BLS12-381 support" ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants