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

refactor(encryptor): align Encryptor methods to match @metamask/browser-passworder #9203

Merged
merged 18 commits into from
May 8, 2024
Merged

refactor(encryptor): align Encryptor methods to match @metamask/browser-passworder #9203

merged 18 commits into from
May 8, 2024

Conversation

ccharly
Copy link
Contributor

@ccharly ccharly commented Apr 11, 2024 •
edited by gantunesr
Loading

Description

Align Encryptor methods to match @metamask/browser-passworder as much as possible.

Related issues

Fixes https://github.com/MetaMask/accounts-planning/issues/379

Manual testing steps

Update app version and verify secrets

  1. Create a new wallet using the last version of the app
  2. Lock and unlock the app
  3. Update the wallet to the code in this branch
  4. Lock and unlock the app
  5. Add 2-3 accounts
  6. Lock and unlock the app
  7. Verify the private for a random account is still accessible
  8. Verify the SRP for the wallet is still accessible
  9. Connect a hardware wallet and import accounts
  10. Lock and unlock the app
  11. Verify the state of the app is consistent

Screenshots/Recordings

Not applicable

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

@github-actions GitHub Actions
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.

@socket-security Socket Security
Copy link

socket-security bot commented Apr 11, 2024 •
edited
Loading

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

Package New capabilities Transitives Size Publisher
npm/@metamask/browser-passworder@5.0.0 None 0 56.6 kB metamaskbot

View full report↗︎

@ccharly ccharly force-pushed the refactor/implements-generic-encryptor branch from e3f8949 to b12e139 Compare April 11, 2024 14:20
@gantunesr gantunesr mentioned this pull request Apr 12, 2024
Merged
11 tasks
@ccharly ccharly marked this pull request as ready for review April 15, 2024 08:36
@ccharly ccharly requested a review from a team as a code owner April 15, 2024 08:36
Base automatically changed from refactor/encryptor-class to main April 19, 2024 15:03
@gantunesr gantunesr requested a review from a team as a code owner April 19, 2024 15:03
@ccharly
refactor(encryptor): adapt types + use types from browser-passworder
49680c3
@ccharly
refactor(encryptor): *DERIVATION_PARAMS* -> *DERIVATION_OPTIONS*
ec2c924
@ccharly
refactor(encryptor): align Encryptor to match browser-passworder inte…
34edb69 
…rface
@ccharly
feat(encryptor): abstract underlying encryption library + fix key par…
6969f85 
…ameter

There was also an error in the way we were using the actual key value.
We were passing an EncryptionKey to the native library... This would
have probably crash with the real library.
@ccharly
refactor(encryptor): export generateSalt
d0cb1f0
@ccharly
fix(encryptor): prevent mocks from being discarded
94e91f9
@ccharly
feat(encryptor): checks for more preconditions in encryption lib layer
95f93b0
@ccharly
feat(encryptor): add exportable attribute for EncryptionKey + keyFrom…
d78d73f 
…Password
@ccharly
feat(encryptor): add {import,export}Key methods
2d91d00
@ccharly
refactor(encryptor): use same test style for it.each
634c5b6
@ccharly
chore: lint
ff818df
@ccharly ccharly force-pushed the refactor/implements-generic-encryptor branch from 42ed5bd to ff818df Compare April 24, 2024 16:57
@danroc danroc added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 26, 2024
ccharly and others added 2 commits April 29, 2024 17:25
@ccharly @gantunesr
docs: remove @throws on Encryptor's constructor
7528425 
Co-authored-by: Gustavo Antunes <17601467+gantunesr@users.noreply.github.com>
@ccharly
Merge branch 'main' into refactor/implements-generic-encryptor
0b623e4
gantunesr
gantunesr previously approved these changes Apr 30, 2024
View reviewed changes
@plasmacorral plasmacorral added DO-NOT-MERGE Pull requests that should not be merged QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels May 3, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 6, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@plasmacorral plasmacorral mentioned this pull request May 6, 2024
Closed
@plasmacorral plasmacorral added the Run Smoke E2E Triggers smoke e2e on Bitrise label May 6, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 6, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 5b232e0
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/9ec5d157-9a59-433d-9104-b07b9d5be721

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

@plasmacorral plasmacorral added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels May 7, 2024
@plasmacorral
Copy link
Contributor

Completed upgrade tests on Samsung a515f (Android 12) , Pixel 5a (Android 14), Redmi Note 8 on browserstack (Android 9) and iPhone Xs (iOS 17.4.1) from QA build of v7.20.1 to this branch with no issues.

I was able to create new wallet, import SRP, change password, toggle biometrics, add accounts, remove accounts, delete wallet, Import a PK, Add Ledger, Add QR address, lock/unlock wallet.

Only observation was confirmed present in production, so reported issue 9560.

@plasmacorral plasmacorral added QA Passed A successful QA run through has been done needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed QA in Progress QA has started on the feature. DO-NOT-MERGE Pull requests that should not be merged labels May 7, 2024
@gantunesr gantunesr removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 8, 2024
@gantunesr gantunesr merged commit fc79f40 into main May 8, 2024
43 of 47 checks passed
@gantunesr gantunesr deleted the refactor/implements-generic-encryptor branch May 8, 2024 15:59
@github-actions github-actions bot locked and limited conversation to collaborators May 8, 2024
@metamaskbot metamaskbot added the release-7.23.0 Issue or pull request that will be included in release 7.23.0 label May 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@danroc danroc danroc left review comments

@gantunesr gantunesr gantunesr approved these changes

Assignees

@ccharly ccharly

Labels
QA Passed A successful QA run through has been done release-7.23.0 Issue or pull request that will be included in release 7.23.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-accounts
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ccharly @plasmacorral @gantunesr @danroc @metamaskbot