Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Adding in progress overlay/state for wallet recovery #13960

Merged
merged 1 commit into from
Apr 30, 2018

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Apr 27, 2018

Fixes #11790

With 1ef48b8 an overlay is added to the recovery dialogue while the wallet recovery is in progress. Included in the overlay is a 'Cancel' button which will cancel the current recovery while it is in progress.

Here is the PR in action:
http://recordit.co/OYC6OgUjMX

Note: An artificial delay was introduced in to the backup for display purposes, in many instances the recovery is quite fast

This PR is currently marked as WIP. @bradleyrichter had an idea for a pretty slick implementation of text animation with a minimum propagation time. Example:

The Recovery In Progress Overlay may include timed progress steps like:

Reading recovery file….
Processing wallet…
Wallet Recovered! Go spend it!

This will involve some refactoring/fail cases in the recovery state, and will involve some additional work from my end and perhaps some more UX oriented discussion. Further work will be pushed right to this PR.

We will be tackling this in a separate issue :)

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

For current in progress incarnation

  1. Start Brave with a clean profile, enable payments.
  2. Exit Brave and corrupt your wallet by removing seeds 20 and 21 from ledger-state.json
  3. Restart brave and go to about:preferences#payments
  4. Recover wallet with a bad recovery key
  5. Ensure that the in progress state is shown while the recovery attempts.
  6. Recover the wallet with vower obovoid menace tobogganist hoyle honoree pixel pestilently disconcertment sellable ruffing supervision zoroastrian based coparent slackened
  7. Ensure that the in progress state is shown while the recovery attempts.

Let me know if help is needed introducing a manual delay via the codebase for testing purposes.

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@ryanml ryanml changed the title Adding in progress overlay/state for wallet recovery Adding in progress overlay/state for wallet recovery [WIP] Apr 27, 2018
@ryanml ryanml self-assigned this Apr 27, 2018
@codecov-io
Copy link

codecov-io commented Apr 27, 2018

Codecov Report

Merging #13960 into master will increase coverage by <.01%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##           master   #13960      +/-   ##
==========================================
+ Coverage   56.52%   56.52%   +<.01%     
==========================================
  Files         284      284              
  Lines       29302    29280      -22     
  Branches     4864     4865       +1     
==========================================
- Hits        16562    16550      -12     
+ Misses      12740    12730      -10
Flag Coverage Δ
#unittest 56.52% <58.82%> (ø) ⬆️
Impacted Files Coverage Δ
app/common/state/ledgerState.js 85.28% <ø> (-0.26%) ⬇️
...r/components/preferences/payment/ledgerRecovery.js 24.41% <0%> (-0.59%) ⬇️
app/common/state/aboutPreferencesState.js 100% <100%> (ø) ⬆️
app/browser/reducers/ledgerReducer.js 46.68% <50%> (+1.9%) ⬆️
app/browser/api/ledger.js 63.15% <66.66%> (ø) ⬆️
js/stores/appStoreRenderer.js 91.66% <0%> (-8.34%) ⬇️
app/renderer/components/reduxComponent.js 57.75% <0%> (-3.45%) ⬇️
js/stores/windowStore.js 28.46% <0%> (-0.3%) ⬇️

@NejcZdovc NejcZdovc changed the title Adding in progress overlay/state for wallet recovery [WIP] Adding in progress overlay/state for wallet recovery Apr 27, 2018
Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

When recovering by a file, if I cancel the file dialog, the recovery overlay still says 'recovering...'

@@ -356,6 +356,7 @@ const ledgerState = {
setRecoveryStatus: (state, status) => {
state = validateState(state)
const date = new Date().getTime()
state = state.setIn(['about', 'preferences', 'recoveryInProgress'], false)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be state.setIn([....], !status) or will it always be false on setRecoveryStatus?

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 setRecoveryStatus method inside ledger state is called when the conclusion of the recovery has occurred, (success/fail) or when it is reset. In both cases recoveryInProgress should be set to false as the attempted recovery is over.

Copy link
Contributor

Choose a reason for hiding this comment

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

please use aboutPreferencesState.setPreferencesProp here as well

Copy link
Contributor

Choose a reason for hiding this comment

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

please move this function setRecoveryStatus into aboutPreferencesState and use setPreferencesProp for all 3 props. Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NejcZdovc refactoring pushed

@ryanml
Copy link
Contributor Author

ryanml commented Apr 28, 2018

@jasonrsadler I will see what's going on with the cancellation 👍

@@ -58,12 +58,18 @@ class LedgerRecoveryContent extends ImmutableComponent {
}
}

onCancelRecovery (status) {
this.onRecoveryOverlay(status)
this.clearRecoveryStatus(status)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this call is not needed, because we already have it in onRecoveryOverlay

Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

please squash commits

@NejcZdovc NejcZdovc self-requested a review April 30, 2018 05:33
@NejcZdovc NejcZdovc added this to the 0.22.x Release 3 (Beta channel) milestone Apr 30, 2018
@ryanml ryanml force-pushed the recovery-loading-indicator branch from 345f20f to 81523c7 Compare April 30, 2018 07:15
@ryanml
Copy link
Contributor Author

ryanml commented Apr 30, 2018

squashed

Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

++

@NejcZdovc NejcZdovc merged commit 417f3b7 into brave:master Apr 30, 2018
NejcZdovc added a commit that referenced this pull request Apr 30, 2018
Adding in progress overlay/state for wallet recovery
NejcZdovc added a commit that referenced this pull request Apr 30, 2018
Adding in progress overlay/state for wallet recovery
@NejcZdovc
Copy link
Contributor

master 417f3b7
0.23 61fedd8
0.22.x-release3 47a615e

@srirambv
Copy link
Collaborator

srirambv commented May 3, 2018

11790

@ryanml should the Recovery in progress text show right away when you click on import recovery keys?

@ryanml
Copy link
Contributor Author

ryanml commented May 3, 2018

@srirambv no it should not, it should only be showing when an actual recovery is in progress. Can you file issue? Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants