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

Disable import wallet button when the backup phrase is not valid #1012

Conversation

Pedro-vk
Copy link
Contributor

@Pedro-vk Pedro-vk commented Sep 16, 2019

Description

Disable import wallet button when the backup phrase is not valid

Tested

Changes were tested on Android emulator, going to import wallet view and filling the textarea with different words, checking that adding the 12th word (or first letter of the word) enables the button

Related issues

Backwards compatibility

It doesn't affect compatibility

Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

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

Nice first pass, see comments below.
For the Tested section of the PR, please provide specific details about how you tested the change.

@@ -177,7 +177,7 @@ export class ImportWallet extends React.Component<Props, State> {
</View>
)}
<GethAwareButton
disabled={isSubmitting || !this.state.backupPhrase}
disabled={isSubmitting || this.state.backupPhrase.trim().split(/\s+/g).length < 12}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider creating an isBackupPhraseValid method and doing this check there

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, did you mean 24 instead of 12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue considers both: 12 and 24. Is it ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider creating an isBackupPhraseValid method and doing this check there

Sure!

@codecov
Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #1012 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1012      +/-   ##
==========================================
+ Coverage   66.92%   66.93%   +<.01%     
==========================================
  Files         252      252              
  Lines        7311     7312       +1     
  Branches      418      418              
==========================================
+ Hits         4893     4894       +1     
  Misses       2331     2331              
  Partials       87       87
Flag Coverage Δ
#mobile 66.93% <100%> (ø) ⬆️
Impacted Files Coverage Δ
packages/mobile/src/import/ImportWallet.tsx 92.3% <100%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 833a0be...414dd9d. Read the comment docs.

@@ -126,6 +126,10 @@ export class ImportWallet extends React.Component<Props, State> {
}
}

isBackupPhraseValid() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Guess this should be isBackupPhraseInvalid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! And added more details on tested section. Is it ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it fixed? Still shows as Valid for me

@Pedro-vk Pedro-vk force-pushed the feature/#802-enable-import-wallet-button-on-valid-phrase branch from 6256b26 to f0b1fc7 Compare September 16, 2019 17:17
@jmrossy jmrossy merged commit f2abdff into celo-org:master Sep 17, 2019
aaronmgdr added a commit that referenced this pull request Sep 17, 2019
* master: (132 commits)
  [Wallet] Differentiate dollars and gold transactions (#1021)
  [wip] LinkedList cycle fix (#941)
  Fix slither issues (#572)
  [Wallet] Disable import wallet button when the backup phrase is not valid (#1012)
  Fix fixed fraction division underflow (#952)
  [CircleCI]Run protocol coverage once a day (#1018)
  Expose and test validator set precompiles (#874)
  [celotool] Fund Faucet accounts on network init (#990)
  Add Nexmo as a text provider for MX and US numbers (#1002)
  [CircleCI]Run protocol test with and without coverage (#991)
  Increase memory requests for Alfajores nodes (#966)
  [wallet]Select blockchain url based on testnet (#936)
  Pilot app config and ability to not show testnet banner (#1009)
  Add Linux onboarding steps and a TOC to SETUP.md (#1004)
  Add README-dev.md (#989)
  [Wallet] Exchange flow formatting fixes (#961)
  [celotool] Cleanup genesis block generation (#988)
  [celotool] Some ❤️. Clean up (#948)
  CeloCLI sorted list of Validator Groups should not include groups with zero votes (#907)
  [contractkit] Fix tests (#963)
  ...
aaronmgdr added a commit that referenced this pull request Sep 18, 2019
* master: (72 commits)
  Remove unused Reserve.burnToken function (#984)
  Persistent disks for transaction nodes (#1016)
  [Wallet] Differentiate dollars and gold transactions (#1021)
  [wip] LinkedList cycle fix (#941)
  Fix slither issues (#572)
  [Wallet] Disable import wallet button when the backup phrase is not valid (#1012)
  Fix fixed fraction division underflow (#952)
  [CircleCI]Run protocol coverage once a day (#1018)
  Expose and test validator set precompiles (#874)
  [celotool] Fund Faucet accounts on network init (#990)
  Add Nexmo as a text provider for MX and US numbers (#1002)
  [CircleCI]Run protocol test with and without coverage (#991)
  Increase memory requests for Alfajores nodes (#966)
  [wallet]Select blockchain url based on testnet (#936)
  Pilot app config and ability to not show testnet banner (#1009)
  Add Linux onboarding steps and a TOC to SETUP.md (#1004)
  Add README-dev.md (#989)
  [Wallet] Exchange flow formatting fixes (#961)
  [celotool] Cleanup genesis block generation (#988)
  [celotool] Some ❤️. Clean up (#948)
  ...

# Conflicts:
#	packages/web/src/header/Header.3.tsx
aaronmgdr added a commit that referenced this pull request Sep 18, 2019
* master: (38 commits)
  Remove unused Reserve.burnToken function (#984)
  Persistent disks for transaction nodes (#1016)
  [Wallet] Differentiate dollars and gold transactions (#1021)
  [wip] LinkedList cycle fix (#941)
  Fix slither issues (#572)
  [Wallet] Disable import wallet button when the backup phrase is not valid (#1012)
  Fix fixed fraction division underflow (#952)
  [CircleCI]Run protocol coverage once a day (#1018)
  Expose and test validator set precompiles (#874)
  [celotool] Fund Faucet accounts on network init (#990)
  Add Nexmo as a text provider for MX and US numbers (#1002)
  [CircleCI]Run protocol test with and without coverage (#991)
  Increase memory requests for Alfajores nodes (#966)
  [wallet]Select blockchain url based on testnet (#936)
  Pilot app config and ability to not show testnet banner (#1009)
  Add Linux onboarding steps and a TOC to SETUP.md (#1004)
  Add README-dev.md (#989)
  [Wallet] Exchange flow formatting fixes (#961)
  [celotool] Cleanup genesis block generation (#988)
  [celotool] Some ❤️. Clean up (#948)
  ...
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.

Users SNBAT try to restore invalid backup phrases
2 participants