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

chore: Add eth hd keyring and key tree to decrease unlock time #12428

Merged
merged 31 commits into from
Dec 3, 2024

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Nov 26, 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
  • 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
  5. update the app (from v4) with 1 account imported via SRP ,✅
    https://github.com/user-attachments/assets/2364461d-869f-40c5-85a4-ef2db3433ceb
  6. 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

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • 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.

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise team-tiger Tiger team (for tech debt reduction + performance improvements) labels Nov 26, 2024
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Copy link
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 4ed8536
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/98aac5df-803c-45b9-bd5e-46cb8ba376a1

Note

  • This comment will auto-update when build completes
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link

socket-security bot commented Nov 26, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] None +2 1.16 MB metamaskbot

View full report↗︎

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 26, 2024
Copy link
Contributor

github-actions bot commented Nov 26, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 2834fba
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/5351aa81-0e8d-4484-ae5b-55fe6c12e115

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 26, 2024
Copy link
Contributor

github-actions bot commented Nov 26, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: e6e8c10
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/77897a67-becb-48b3-9c11-d48bd4809ce1

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 27, 2024
Copy link
Contributor

github-actions bot commented Nov 27, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: c1704e9
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/72e36820-36ed-449a-bbdf-2799ce676253

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

tommasini and others added 3 commits November 27, 2024 12:43
…cenarios. Added patch to keyring controller to make generateRandomMnemonic awaited since it is async in hd-keyring
@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 27, 2024
Copy link
Contributor

github-actions bot commented Nov 27, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 38a192b
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/6394a7c9-6458-4f25-b2b8-910206ec513c

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

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

I'm thinking that we should also test an update from a previous version, to check that the accounts derived from the mnemonic stay the same

app/core/Encryptor/pbkdf2.ts Outdated Show resolved Hide resolved
app/core/Encryptor/pbkdf2.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 57.00%. Comparing base (22a4989) to head (64ed517).
Report is 66 commits behind head on main.

Files with missing lines Patch % Lines
app/core/Engine/Engine.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12428      +/-   ##
==========================================
+ Coverage   56.41%   57.00%   +0.58%     
==========================================
  Files        1797     1815      +18     
  Lines       40586    40852     +266     
  Branches     5097     5161      +64     
==========================================
+ Hits        22896    23287     +391     
+ Misses      16134    15981     -153     
- Partials     1556     1584      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 29, 2024
Copy link
Contributor

github-actions bot commented Nov 29, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 0716d9f
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/17e0ec19-a9c7-4aed-b38b-e4044b384f3e

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Dec 3, 2024
Copy link
Contributor

github-actions bot commented Dec 3, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 1dba714
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/39ac67f8-a293-46c2-99c4-2e6400e1ca38

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link

sonarcloud bot commented Dec 3, 2024

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Dec 3, 2024
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

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

LGTM!

@tommasini tommasini added this pull request to the merge queue Dec 3, 2024
Merged via the queue into main with commit dc310af Dec 3, 2024
49 checks passed
@tommasini tommasini deleted the chore/add-eth-hd-keyring branch December 3, 2024 23:22
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Dec 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Dec 3, 2024
@metamaskbot metamaskbot added the release-7.38.0 Issue or pull request that will be included in release 7.38.0 label Dec 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.38.0 Issue or pull request that will be included in release 7.38.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform team-tiger Tiger team (for tech debt reduction + performance improvements)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants