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.

Sign up for GitHub

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

Jump to bottom

Wrap cryptolib functions #132

Merged
merged 8 commits into from
Feb 27, 2024
Merged

Wrap cryptolib functions #132

merged 8 commits into from
Feb 27, 2024

Conversation

yogh333
Copy link
Contributor

@yogh333 yogh333 commented Feb 8, 2024 •
edited
Loading

  • Added wrapper for hash functions
  • Return Chain Code (BIP32 derivation)

@yogh333 yogh333 force-pushed the jca/wrap_crypto_functions branch from 0fb5774 to 14739bf Compare February 20, 2024 07:31
@yogh333 yogh333 marked this pull request as ready for review February 20, 2024 07:32
xchapron-ledger
xchapron-ledger reviewed Feb 20, 2024
View reviewed changes
ledger_device_sdk/src/ecc.rs
path: &[u32],
key: &mut [u8],
cc: &mut [u8],
) -> Result<(), CxError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why for ed25519 the requested length is 96 few lines below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, looks to be ok with 64 🤔. Maybe a typo @yhql ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix in this commit, waiting feedback from @yhql if revert is needed

@yogh333 yogh333 mentioned this pull request Feb 22, 2024
Closed
niooss-ledger
niooss-ledger approved these changes Feb 22, 2024
View reviewed changes
Copy link

@niooss-ledger niooss-ledger left a 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.

@yogh333 yogh333 force-pushed the jca/wrap_crypto_functions branch from 3521a3e to 11d4dbc Compare February 27, 2024 10:43
yhql
yhql approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@yhql yhql left a comment

Choose a reason for hiding this comment

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

LGTM, would recommend using macro_rules! to declare HashInit for all variants though

@yogh333 yogh333 merged commit 4d0e7fd into master Feb 27, 2024
25 checks passed
@yogh333 yogh333 deleted the jca/wrap_crypto_functions branch February 27, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xchapron-ledger xchapron-ledger xchapron-ledger left review comments

@yhql yhql yhql approved these changes

@niooss-ledger niooss-ledger niooss-ledger approved these changes

@agrojean-ledger agrojean-ledger Awaiting requested review from agrojean-ledger

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yogh333 @yhql @niooss-ledger @xchapron-ledger