Skip to content

feat(keyring-eth-hd): allow passing native implementations of cryptography#102

Merged
ccharly merged 6 commits intomainfrom
fb/hd-keyring-native-implementation
Nov 26, 2024
Merged

feat(keyring-eth-hd): allow passing native implementations of cryptography#102
ccharly merged 6 commits intomainfrom
fb/hd-keyring-native-implementation

Conversation

@FrederikBolding
Copy link
Copy Markdown
Member

@FrederikBolding FrederikBolding commented Nov 25, 2024

Uses the latest version of @metamask/keytree, which allows passing cryptographicFunctions for derivation of the mnemonic seed. Also reintroduces constructor arguments to allow passing of cryptographicFunctions to HdKeyring.

Follow-up PR to #100

Base automatically changed from fb/hd-keyring-constructor-refactor to main November 25, 2024 19:49
@ccharly ccharly changed the title feat(keyring-eth-hd): Allow passing native implementations of cryptography feat(keyring-eth-hd): allow passing native implementations of cryptography Nov 26, 2024
@FrederikBolding FrederikBolding force-pushed the fb/hd-keyring-native-implementation branch from 207b446 to 638501b Compare November 26, 2024 10:32
@FrederikBolding FrederikBolding marked this pull request as ready for review November 26, 2024 14:20
@FrederikBolding FrederikBolding requested a review from a team as a code owner November 26, 2024 14:20
Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
@ccharly ccharly added this pull request to the merge queue Nov 26, 2024
Merged via the queue into main with commit 4ab73b5 Nov 26, 2024
@ccharly ccharly deleted the fb/hd-keyring-native-implementation branch November 26, 2024 15:05
github-merge-queue bot pushed a commit to MetaMask/metamask-mobile that referenced this pull request Dec 3, 2024
## **Description**

This PR is a draft and it still needs to

* ~Remove key-tree patch and update to the new release (still pending)~
* ~Remove eth-hd-keyring patch and install the latest release that
includes this [PR](MetaMask/accounts#102
* Updated patch version of Keyring Controller to 17.2.2 (Added a patch
to the Keyring Controller awaiting for the generation of random
mnemonic, that will be removed once this work is done, in this PR or
worst scenario, following up work)

App launch times pipeline:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/5b450f2a-91a3-4a94-8479-729655a2cf0b?tab=workflows

Follow up work:
* Remove patch of keyring controller added
* Decrease baseline of performance e2e pipeline (`app_launch_times`) for
android and ios (Android less than 2.9 seconds cold app start for wallet
view)

## **Related issues**

Fixes:

## **Manual testing steps**
Build used for testing:
https://app.bitrise.io/build/6ed111dd-6bbb-4492-a73e-a11347335e8e?tab=log

1. create new acount generating an SRP, ✅
2. create a new account when the SRP account is created ✅

https://github.com/user-attachments/assets/71a45633-6b2a-446f-ac7a-6aed99c97b54
3. import with SRP, ✅
4. add with private key,✅

https://github.com/user-attachments/assets/5ed51268-7b2a-47c1-abb7-1f1da19bda05
6. update the app (from v4) with 1 account imported via SRP ,✅

https://github.com/user-attachments/assets/2364461d-869f-40c5-85a4-ef2db3433ceb
7. update the app with 3 accounts (imported via SRP and private key and
added) ,✅

https://github.com/user-attachments/assets/fa452f36-48d8-4573-8c1f-d97a1938baf5


## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Charly Chevalier <charlyy.chevalier@gmail.com>
Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>
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