ledger backup and recovery #4396

jkup · 2016-09-30T06:28:45Z

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).

Still remaining

Modals still need some custom CSS
Generate a new file with fs.writeStream for printing / saving
- Either ignore generated file or delete after printing or saving
Re-implement printing and saving with the new file
Send data from recovery form to ledger.js
- Send response back to UI and display message to user
Link through settings like minimum page time and minimum visits
At least write automated tests proving backup and recover flow

jkup · 2016-09-30T06:35:57Z

Right now I'm working on the recovery flow. The thing I need help with is figuring out a good approach for printing / saving the keys!

@ayumi for recovering keys do I just have an aboutAction that ipc's to ledger.js and then a method in ledger.js that calls client.recover? I'm still trying to get my head around the ledger.js -> ledger-client -> ledger workflow

@bradleyrichter on the recover modal we have "Recover" and "Done" but I think it might cause some confusion where entering your keys and clicking "Done" seems like it should do a recover. Maybe we can remove "Done" or change it to "Cancel"?

Also, we're gonna need a better approach for organizing all these modals. I read this thread (reactjs/react-modal#75) which pointed to an interesting approach: https://github.com/react-rally/www/blob/gh-pages/2015/app/screens/Schedule/index.js#L152-L180

bradleyrichter · 2016-09-30T16:57:24Z

@jkup good catch. This should fix the UX issue:

ayumi · 2016-09-30T19:52:15Z

@jkup yes, the appAction IPC -> ledger thing sounds legit

jkup · 2016-10-05T05:47:39Z

This one is getting close. Could I get some eyes on the communication from @bbondy or @bridiver.

Right now preferences.js triggers and aboutAction to appStore to ledger.js (which is all working but maybe not the best approach.

Then (the last part I'm struggling with is getting the data back to preferences.js from ledger.js. I tried (as you can see in this branch) just emitting an ipc event but that doesn't seem to be working.

I can see that the recover task is working correctly, I just need to get that info back to the UI.

bridiver · 2016-10-05T05:52:08Z

@jkup you communicate back to preferences by updating the state

bridiver · 2016-10-05T05:53:50Z

js/stores/appStore.js

@@ -583,6 +586,27 @@ const handleAppAction = (action) => {
    case AppConstants.APP_SET_DICTIONARY:
      appState = appState.setIn(['dictionary', 'locale'], action.locale)
      break
+    case AppConstants.APP_BACKUP_KEYS:
+      const paymentId = appState.get('ledgerInfo').get('paymentId')


I think you want to encapsulate this in a ledger.js method

bridiver · 2016-10-05T06:01:28Z

js/stores/appStore.js

+      })
+      break
+    case AppConstants.APP_RECOVER_WALLET:
+      ipcMain.emit(messages.LEDGER_RECOVER_WALLET, action.firstRecoveryKey, action.secondRecoveryKey)


same as above. ledger.js runs in the same process as appStore.js so emitting an ipc event doesn't really make sense. Just call a method on ledger.js - see https://github.com/brave/browser-laptop/compare/tabs-api?expand=1#diff-23ca389e2bcb77191b5a9c10900eb3a3R717 for an example. The methods should always take the state and the action (reducer) as parameters and return the state (may or may not be modified). The flow should almost always be the same: call an action -> reducer functions update state in appStore -> ui updates to reflect the new state

bridiver · 2016-10-05T06:04:02Z

js/about/aboutActions.js

@@ -88,6 +88,38 @@ const aboutActions = {
  },

  /**
+   * Generates a file with the users backup keys
+   */
+  generateKeyFile: function (backupAction) {


it would probably be helpful to give these actions/constants names that include ledger. Eventually we'll probably break them out into ledger actions

diracdeltas · 2016-10-05T16:03:39Z

js/about/preferences.js

@@ -722,7 +722,17 @@ class TabsTab extends ImmutableComponent {
 class PaymentsTab extends ImmutableComponent {
  constructor () {
    super()
+    this.state = {


please put the state in AboutPreferences and pass it to PaymentTab as props.

Can you help me understand why? Will I also need to create and pass down actions for updating those keys as well? Thanks!

yes. @bbondy can explain why

diracdeltas · 2016-10-05T16:06:33Z

js/stores/appStore.js

+    case AppConstants.APP_BACKUP_KEYS:
+      const paymentId = appState.get('ledgerInfo').get('paymentId')
+      const passphrase = appState.get('ledgerInfo').get('passphrase')
+      const message = 'Your ledger keys are ' + paymentId + ' and ' + passphrase


this should be localized

diracdeltas · 2016-10-05T16:07:40Z

js/stores/appStore.js

+      const message = 'Your ledger keys are ' + paymentId + ' and ' + passphrase
+      const filePath = app.getPath('userData') + '/backup_keys.html'
+
+      fs.writeFile(filePath, message)


i think this should be writeFileSync

writeFileSync will block the entire browser process. Is there a particular reason for sync over a callback?

@bridiver sorry, i meant either writeFileSync or put the rest of the block in the callback

diracdeltas · 2016-10-05T16:13:49Z

docs/state.md

@@ -478,8 +478,12 @@ WindowStore
      percentage: number,        // i.e., 0, 1, ... 100
      publisherURL: string,      // publisher site, e.g., "https://wikipedia.org/"
      faviconURL: string         // i.e., "data:image/...;base64,..."
+    } ],
+    options: {


it would be useful to give this a more descriptive name, like synopsisOptions, because this property gets merged into ledgerData in js/components/frame.js

diracdeltas · 2016-10-05T16:15:36Z

js/stores/appStore.js

@@ -583,6 +586,27 @@ const handleAppAction = (action) => {
    case AppConstants.APP_SET_DICTIONARY:
      appState = appState.setIn(['dictionary', 'locale'], action.locale)
      break
+    case AppConstants.APP_BACKUP_KEYS:
+      const paymentId = appState.get('ledgerInfo').get('paymentId')
+      const passphrase = appState.get('ledgerInfo').get('passphrase')


use getIn here and in the line above to avoid chaining gets

fyi - getIn is helpful because you'll never get an undefined method on null when accessing nested props

diracdeltas · 2016-10-05T16:16:21Z

please add a test or two for this in test/components/braveryPanelTest.js

diracdeltas · 2016-10-11T05:24:06Z

pushing a fix for unicode thing now

@aekeus

address #4396 (comment) Auditors: @aekeus Test Plan: 1. go to ledger panel, save recovery keys 2. verify that opening the saved file in the browser does not show non-ascii characters

bridiver · 2016-10-11T18:23:22Z

js/stores/appStore.js

@@ -585,6 +587,21 @@ const handleAppAction = (action) => {
    case AppConstants.APP_SET_DICTIONARY:
      appState = appState.setIn(['dictionary', 'locale'], action.locale)
      break
+    case AppConstants.APP_BACKUP_KEYS:
+      ledger.backupKeys(appState, action)


this should be appState = ledger.backupKeys(appState, action) to capture any state updates that this method might make. I know it doesn't directly make any now, but the pattern should always be the same to avoid confusion

bridiver · 2016-10-11T18:23:29Z

js/stores/appStore.js

+      ledger.backupKeys(appState, action)
+      break
+    case AppConstants.APP_RECOVER_WALLET:
+      ledger.recoverKeys(appState, action)


same as above

bridiver · 2016-10-11T18:29:52Z

app/ledger.js

+ */
+
+var recoverKeys = (appState, action) => {
+  client.recoverWallet(action.firstRecoveryKey, action.secondRecoveryKey, (err, body) => {


this callback may or may not happen asynchronously so the appActions should be called inside setImmediate to prevent any potential race conditions with state updates. The issue was discussed in slack, but this is how it happens:

For simplicity, all actions increment the current count by 1, but the same problem exists even if the actions change different parts of the state 1. action1 is called with the current appState = {count: 0} 2. action1 calls a method with a callback 3. callback returns synchronously (does not perform any io or other operations that would make it async) 4. action2 is called inside callback with the current appState = {count: 0} 5. action2 returns appState = {count: 1} 6. appState is emitted 7. action1 returns appState = {count: 1} 8 app state is emitted

bridiver · 2016-10-11T19:21:00Z

++

jkup added the design A design change, especially one which needs input from the design team. label Sep 30, 2016

jkup added this to the 0.12.4dev milestone Sep 30, 2016

luixxiul added the feature/rewards label Sep 30, 2016

jkup force-pushed the ledger-redesign branch from df5b17b to acec001 Compare September 30, 2016 07:50

jkup force-pushed the ledger-redesign branch from acec001 to 3d9d3c2 Compare September 30, 2016 17:36

jkup force-pushed the ledger-redesign branch 6 times, most recently from 5dac2e3 to 85e9db8 Compare October 4, 2016 03:02

bbondy modified the milestones: 0.12.5dev, 0.12.4dev Oct 4, 2016

jkup force-pushed the ledger-redesign branch 3 times, most recently from fc02edc to a637fc7 Compare October 5, 2016 05:45

bridiver reviewed Oct 5, 2016

View reviewed changes

diracdeltas reviewed Oct 5, 2016

View reviewed changes

bridiver reviewed Oct 11, 2016

View reviewed changes

jkup force-pushed the ledger-redesign branch from 7fb5ca0 to 9fca556 Compare October 11, 2016 18:40

ledger backup and recovery

b0ffd72

jkup force-pushed the ledger-redesign branch from 9fca556 to b0ffd72 Compare October 11, 2016 19:13

bridiver merged commit 43c0100 into master Oct 11, 2016

This was referenced Oct 12, 2016

Manual tests for OS X 0.12.5 Preview 1 #4707

Closed

Manual tests for Ubuntu 0.12.5 Preview 1 #4706

Closed

Manual tests for Windows x64 0.12.5 Preview 1 #4705

Closed

Manual tests for Windows ia-32 0.12.5 Preview 1 #4704

Closed

alexwykoff added the QA/checked-macOS label Oct 13, 2016

This was referenced Oct 13, 2016

Manual tests for Ubuntu 0.12.5 Preview 2 #4734

Closed

Manual tests for OS X 0.12.5 Preview 2 #4735

Closed

Manual tests for Windows x64 0.12.5 Preview 2 #4736

Closed

alexwykoff added the QA/checked-Win64 label Oct 13, 2016

jkup deleted the ledger-redesign branch October 13, 2016 23:12

This was referenced Oct 14, 2016

Manual tests for Ubuntu 0.12.5 RC1 #4772

Closed

Manual tests for OS X 0.12.5 RC1 #4773

Closed

Manual tests for Windows ia-32 0.12.5 RC 1 #4774

Closed

Manual tests for Windows x64 0.12.5 RC 1 #4775

Closed

luixxiul added the QA/checked-Win32 label Oct 15, 2016

This was referenced Oct 16, 2016

Manual tests for Windows x64 0.12.5 RC 3 #4838

Closed

Manual tests for OS X 0.12.5 RC3 #4839

Closed

Manual tests for Windows ia-32 0.12.5 RC3 #4840

Closed

Manual tests for Ubuntu 0.12.5 RC3 #4841

Closed

luixxiul added the feature/printing label Mar 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ledger backup and recovery #4396

ledger backup and recovery #4396

jkup commented Sep 30, 2016 •

edited

Loading

jkup commented Sep 30, 2016 •

edited

Loading

bradleyrichter commented Sep 30, 2016

ayumi commented Sep 30, 2016

jkup commented Oct 5, 2016

bridiver commented Oct 5, 2016

bridiver Oct 5, 2016

bridiver Oct 5, 2016

bridiver Oct 5, 2016

diracdeltas Oct 5, 2016

jkup Oct 5, 2016

diracdeltas Oct 5, 2016

diracdeltas Oct 5, 2016

diracdeltas Oct 5, 2016

bridiver Oct 5, 2016

diracdeltas Oct 5, 2016

diracdeltas Oct 5, 2016

diracdeltas Oct 5, 2016

bridiver Oct 5, 2016

diracdeltas commented Oct 5, 2016

diracdeltas commented Oct 11, 2016

bridiver Oct 11, 2016

bridiver Oct 11, 2016

bridiver Oct 11, 2016

bridiver commented Oct 11, 2016

ledger backup and recovery #4396

ledger backup and recovery #4396

Conversation

jkup commented Sep 30, 2016 • edited Loading

jkup commented Sep 30, 2016 • edited Loading

bradleyrichter commented Sep 30, 2016

ayumi commented Sep 30, 2016

jkup commented Oct 5, 2016

bridiver commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Oct 5, 2016

diracdeltas commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver commented Oct 11, 2016

jkup commented Sep 30, 2016 •

edited

Loading

jkup commented Sep 30, 2016 •

edited

Loading