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

Allow for 24 length seedphrase on import #1848

Merged
merged 6 commits into from
Sep 28, 2020
Merged

Allow for 24 length seedphrase on import #1848

merged 6 commits into from
Sep 28, 2020

Conversation

rickycodes
Copy link
Contributor

@rickycodes rickycodes commented Sep 24, 2020 •
edited
Loading

Description

This allows for the import of 24 length seed phrases!

comparing to the extension, they also do a bip39 validate, we should likely do the same? I noticed there was a bip39 patch file (patches/bip39+2.6.0.patch), but it looks like we no longer use that dependency? 🤔

this is now an exact replica of extension seed phrase import functionality

complete with bip39 validation courtesy ethers:

image

you can test this out by trying to import a 12 word seed phrase and tacking on extra chars to the end of it to break compliance

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #1838

@rickycodes rickycodes changed the title Allow for 24 length seedphrase on import, closes #1838 Allow for 24 length seedphrase on import Sep 24, 2020
@rickycodes rickycodes force-pushed the issue-1838 branch 3 times, most recently from 99f6c7f to 95f825b Compare September 24, 2020 17:22
@rickycodes rickycodes marked this pull request as ready for review September 24, 2020 17:24
@rickycodes rickycodes requested a review from a team as a code owner September 24, 2020 17:24
@rickycodes rickycodes added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Sep 24, 2020
@rickycodes rickycodes closed this Sep 24, 2020
@rickycodes rickycodes reopened this Sep 24, 2020
@rickycodes rickycodes force-pushed the issue-1838 branch 3 times, most recently from e9e5931 to d3ab579 Compare September 25, 2020 03:16
estebanmino
estebanmino approved these changes Sep 25, 2020
View reviewed changes
Copy link
Contributor

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

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

LGTM!

ibrahimtaveras00
ibrahimtaveras00 approved these changes Sep 28, 2020
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

QA Passed

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Sep 28, 2020
@rickycodes rickycodes merged commit ebe4745 into develop Sep 28, 2020
@rickycodes rickycodes deleted the issue-1838 branch September 28, 2020 22:15
rickycodes added a commit that referenced this pull request Jan 31, 2022
@rickycodes
Allow for 24 length seedphrase on import (#1848)
82f8f77 
* Allow for 24 length seedphrase on import, closes #1838

* call toggleHideSeedPhraseInput onScanError

* Update broken seed phrase translation keys

* Add proper logging for when seed phrase does fail

* Validate mnemonic with ethers

* Add tests for failedSeedPhraseRequirements
SamuelSalas added a commit that referenced this pull request Jul 6, 2024
@SamuelSalas
test: Refactor WalletView file (#10225)
b2cce87 
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

Refactor the WalletView screen to follow the page object model as well
as the current guidelines establish by the QA mobile team. As well to
unify all the testIDs into one single object.

## **Summary**
*
app/component-library/components/Pickers/PickerAccount/PickerAccount.tsx:
Remove previous `generateTestId ` with current testID
`WalletViewSelectorsIDs.ACCOUNT_NAME_LABEL_TEXT`

*
app/component-library/components/Pickers/PickerNetwork/PickerNetwork.tsx:
Remove previous `generateTestId ` with current testID
`WalletViewSelectorsIDs.NAVBAR_NETWORK_TEXT `

* app/component-library/components/Toast/Toast.tsx: Remove previous
`generateTestId ` with current testID `ToastSelectorsIDs.CONTAINER`

* app/components/UI/AccountOverview/index.js: Remove `currentCurrency `,
`newAssetTransaction `, `toggleReceiveModal ` props causing lint issues.
Remove previous `generateTestId ` with current testID
`WalletViewSelectorsIDs.ACCOUNT_ICON`, `WalletViewSelectorsIDs.
ACCOUNT_NAME_LABEL_INPUT `, `WalletViewSelectorsIDs.
ACCOUNT_NAME_LABEL_TEXT `

* app/components/UI/AddressCopy/AddressCopy.tsx: Remove previous
`generateTestId ` with current testID
`WalletViewSelectorsIDs.ACCOUNT_COPY_BUTTON`, `WalletViewSelectorsIDs.
ACCOUNT_ADDRESS `

* app/components/UI/CollectibleContracts/index.js: Remove previous
`generateTestId ` with current testID `WalletViewSelectorsIDs.
IMPORT_NFT_BUTTON `, `WalletViewSelectorsIDs. NFT_TAB_CONTAINER `

* app/components/UI/Navbar/index.js: Remove previous `generateTestId `
with current testID `AddContactViewSelectorsIDs.EDIT_BUTTON `,
`WalletViewSelectorsIDs.NAVBAR_NETWORK_BUTTON `

* app/components/UI/Tokens/index.test.tsx: Update `getByTestId ` with
current testID `AddContactViewSelectorsIDs. IMPORT_TOKEN_BUTTON `,
`WalletViewSelectorsIDs. TOKENS_CONTAINER `

* app/components/UI/Tokens/index.tsx: Remove previous `generateTestId `
with current testID `AddContactViewSelectorsIDs. IMPORT_TOKEN_BUTTON `,
`WalletViewSelectorsIDs. TOKENS_CONTAINER `

* app/components/UI/WalletAccount/WalletAccount.test.tsx: Update
`getByTestId ` with current testID
`WalletViewSelectorsIDs.ACCOUNT_ADDRESS `, `WalletViewSelectorsIDs.
ACCOUNT_COPY_BUTTON `, `WalletViewSelectorsIDs. ACCOUNT_ICON `

* app/components/UI/WalletAccount/WalletAccount.tsx: Remove previous
`generateTestId ` with current testID
`WalletViewSelectorsIDs.ACCOUNT_ADDRESS `, `WalletViewSelectorsIDs.
ACCOUNT_COPY_BUTTON `, `WalletViewSelectorsIDs. ACCOUNT_ICON `

* app/components/Views/AccountActions/AccountActions.constants.ts: File
removed

* app/components/Views/AccountActions/AccountActions.test.tsx: Update
`getByTestId ` with current testIDs from
`AccountActionsModalSelectorsIDs` object.

* app/components/Views/AccountActions/AccountActions.tsx: Remove
previous `generateTestId ` with current testIDs from
`AccountActionsModalSelectorsIDs ` object.

* app/components/Views/TransactionsView/index.js: Remove
`generateTestId`

* app/components/Views/Wallet/index.test.tsx: Update `getByTestId ` with
current testID `CommonSelectorsIDs.FOX_ICON `,
`WalletViewSelectorsIDs.WALLET_SCAN_BUTTON`

* app/components/Views/Wallet/index.tsx: Remove `generateTestId`

* e2e/pages/Send/TransactionConfirmView.js: Remove `await` inside
`Gestures.waitAndTap()`

* e2e/pages/WalletView.js: File Relocated

* e2e/pages/wallet/WalletView.js: File refactor to POM 

* e2e/selectors/Common.selectors.js: Add `ANDROID_PROGRESS_BAR` testId

* e2e/selectors/Modals/AccountActionsModal.selectors.js: File created
with already existing testIDs

* e2e/selectors/TransactionConfirmView.selectors.js: Remove `eslint`
exception line

* e2e/selectors/wallet/WalletView.selectors.js: Added testIDs located in
other files.

* e2e/specs/accounts/create-wallet-account.spec.js: Update
`pages/wallet/WalletView` file path.

* e2e/specs/accounts/import-wallet-account.spec.js: Update
`pages/wallet/WalletView` file path. Update `WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`.

* e2e/specs/assets/import-tokens.spec.js: Update
`pages/wallet/WalletView` file path. Update `WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`. Implement
`WalletView.tokenInWallet()`.

* e2e/specs/assets/nft-detection-modal.spec.js: Update
`pages/wallet/WalletView` file path. Update `WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`.

* e2e/specs/assets/token-detection-import-all.spec.js: Due to some
inconsistencies when importing the wallet not showing the import token
button, I decided to implement the fixture steps.

* e2e/specs/confirmations/advanced-gas-fees.spec.js: Update
`pages/wallet/WalletView` file path. Update `WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`. Remove await inside
`await Assertions.checkIfVisible()`

* e2e/specs/confirmations/approve-custom-erc20.spec.js: Update
`Assertions. checkIfHasText()` to `Assertions.
checkIfElementToHaveText()`.

* e2e/specs/confirmations/increase-allowance-erc20.spec.js: Update
`WalletView.checkIfHasText()` to `Assertions.
checkIfElementToHaveText()`.

* e2e/specs/networks/add-custom-rpc.spec.js: 

* e2e/specs/networks/connect-test-network.spec.js: Update
`WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`. Update `Assertions.
checkIfHasText()` to `Assertions. checkIfElementToHaveText()`.

* e2e/specs/onboarding/onboarding-wizard-opt-in.spec.js: Update
`pages/wallet/WalletView` file path. Update `WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`.

* e2e/specs/permission-systems/permission-system-delete-wallet.spec.js:
Update `pages/wallet/WalletView` file path. Update
`WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`.

* e2e/specs/quarantine/add-edit-custom-eth-mainnet.failing.js: Implement
`Assertions.checkIfVisible(WalletView.container)`.

* e2e/specs/quarantine/contract-nickname.failing.js: Update
`pages/wallet/WalletView` file path. Update `WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`. Implement
`Assertions.checkIfVisible(WalletView.container)`.

* e2e/specs/quarantine/deeplinks.failing.js:

* e2e/specs/quarantine/import-nft.failing.js: Implement
`Assertions.checkIfVisible()`.

*
e2e/specs/quarantine/permission-system-removing-imported-account.failing.js:
Update `pages/wallet/WalletView` file path. Update
`WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`.

* e2e/specs/settings/fiat-on-testnets.spec.js: Update
`pages/wallet/WalletView` file path. Create `WalletView.totalBalance`.
Update `Assertions. checkIfHasText()` to `Assertions.
checkIfElementToHaveText()`.

* e2e/specs/swaps/swap-token-chart.spec.js: Update
`pages/wallet/WalletView` file path. Update `WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`.

* e2e/specs/swaps/token-details.spec.js: Update
`pages/wallet/WalletView` file path.

* e2e/specs/wallet/portfolio-connect-account.spec.js: Update
`pages/wallet/WalletView` file path. Update `Assertions.
checkIfHasText()` to `Assertions. checkIfElementToHaveText()`.

* e2e/specs/wallet/request-token-flow.spec.js: Update
`pages/wallet/WalletView` file path. Update `WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`.

* e2e/specs/wallet/send-ERC-token.spec.js: Update
`pages/wallet/WalletView` file path. Update `WalletView.isVisible()` to
`Assertions.checkIfVisible(WalletView.container)`.

* e2e/specs/wallet/start-exploring.spec.js: Remove deprecated steps

* e2e/utils/Assertions.js: Remove `checkIfHasText ` method. 

* e2e/viewHelper.js: Implement `Assertions.checkIfVisible()` for
`WalletView` and `NetworkEducationModal `. Add `await
Assertions.checkIfVisible(ToastModal.container)` steps

* wdio/screen-objects/AddContact.js: Update testIDs to current
implementation.

* wdio/screen-objects/CommonScreen.js: Update testIDs to current
implementation.

* wdio/screen-objects/Modals/WalletAccountModal.js: Update testIDs to
current implementation.

* wdio/screen-objects/WalletMainScreen.js: Update testIDs to current
implementation, fix naming issues.

* wdio/screen-objects/testIDs/Common.testIds.js: File removed

* wdio/screen-objects/testIDs/Screens/WalletView.testIds.js: Walletview
testIDs moved to unify file


<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes: [#1848](MetaMask/mobile-planning#1848)

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->
Smoke test:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/558e5890-c45c-4d8f-bba4-f8c0456cb4d1
Regression test:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/01e5fdff-34cf-4956-b54f-1588c4066139

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [ ] 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).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] 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**

- [ ] 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibrahimtaveras00 ibrahimtaveras00 ibrahimtaveras00 approved these changes

@estebanmino estebanmino estebanmino approved these changes

Assignees
No one assigned
Labels
next release QA Passed A successful QA run through has been done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

24 word seed phrase
4 participants
@rickycodes @estebanmino @ibrahimtaveras00 @omnat