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

HD derivation: add ed25519 support #9

Merged
merged 12 commits into from
Oct 29, 2024
Merged

HD derivation: add ed25519 support #9

merged 12 commits into from
Oct 29, 2024

Conversation

survived
Copy link
Contributor

@survived survived commented Oct 1, 2024

No description provided.

@survived
Copy link
Contributor Author

survived commented Oct 2, 2024

CI isn't working due to new org settings. Hopefully that will be soon fixed.

@survived survived marked this pull request as ready for review October 2, 2024 08:21
Signed-off-by: Denis Varlakov <[email protected]>
@survived
Copy link
Contributor Author

survived commented Oct 2, 2024

@maurges this is PR for using new hd_wallet in givre

@survived survived requested a review from maurges October 2, 2024 08:27
Signed-off-by: Denis Varlakov <[email protected]>
Signed-off-by: Denis Varlakov <[email protected]>
Signed-off-by: Denis Varlakov <[email protected]>
Signed-off-by: Denis Varlakov <[email protected]>
Signed-off-by: Denis Varlakov <[email protected]>
Comment on lines +177 to +182
/// Derives an HD child key using `HdAlgo` algorithm
///
/// It's not desirable to use this function. In the tests, we should rather use other libraries
/// from what we use in `givre` implementation itself. However, not all derivation methods have
/// other libraries than `hd_wallet`
pub fn derive_child_key<E: Curve, HdAlgo: givre::hd_wallet::HdWallet<E>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use libraries native derivations for those that support it? Also I'm not sure I understand this comment correctly: it means that some derivations methods (like Edwards) don't have alternative implementations other than our one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see we already do use external libraries's derivations where possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it means that some derivations methods (like Edwards) don't have alternative implementations other than our one?

Yes exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, there's one but in C

Comment on lines +132 to +144
#[cfg(feature = "hd-wallet")]
pub fn derive_additive_shift<E: generic_ec::Curve, Hd: hd_wallet::DeriveShift<E>, Index>(
mut epub: hd_wallet::ExtendedPublicKey<E>,
path: impl IntoIterator<Item = Index>,
) -> Result<Scalar<E>, <Index as TryInto<slip_10::NonHardenedIndex>>::Error>
) -> Result<Scalar<E>, <Index as TryInto<hd_wallet::NonHardenedIndex>>::Error>
where
slip_10::NonHardenedIndex: TryFrom<Index>,
hd_wallet::NonHardenedIndex: TryFrom<Index>,
{
let mut additive_shift = Scalar::<E>::zero();

for child_index in path {
let child_index: slip_10::NonHardenedIndex = child_index.try_into()?;
let shift = slip_10::derive_public_shift(&epub, child_index);
let child_index: hd_wallet::NonHardenedIndex = child_index.try_into()?;
let shift = Hd::derive_public_shift(&epub, child_index);
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the api of hd_wallet library looks good here. It's what we build on top of it that I want to complain about, with my usual song about <Hd,_ > signatures. =) But since it's completely internal, I'll let it be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no <Hd, _> though, is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah the functions that use this function might have underscores, right. It's a feature of Rust, difficult to avoid it

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no <Hd, _> though, is there?

Not here, but in usage of this function I mean

@maurges
Copy link
Contributor

maurges commented Oct 7, 2024

Looks good

Signed-off-by: Denis Varlakov <[email protected]>
Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Denis Varlakov <[email protected]>
Signed-off-by: Denis Varlakov <[email protected]>
@survived
Copy link
Contributor Author

@maurges I updated the deps

@survived survived requested a review from maurges October 23, 2024 16:03
@survived survived merged commit 3946a66 into m Oct 29, 2024
12 checks passed
@survived survived deleted the hd-ed25519 branch October 29, 2024 12:32
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.

2 participants