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

Commit

Permalink
initial check-in
Browse files Browse the repository at this point in the history 
1. Fixes #5788
2. Fixes #4114
3. Partially addresses #4720
  • Loading branch information
@mrose17
mrose17 committed Nov 30, 2016
1 parent bd60fe9 commit 9c33676
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 0 deletions.
10 changes: 10 additions & 0 deletions app/ledger.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ var backupKeys = (appState, action) => {

var recoverKeys = (appState, action) => {
client.recoverWallet(action.firstRecoveryKey, action.secondRecoveryKey, (err, body) => {
if (logError(err, 'recoveryWallet')) appActions.updateLedgerInfo(underscore.omit(ledgerInfo, [ '_internal' ]))

This comment has been minimized.

Sign in to view

Copy link
@willy-b

willy-b Dec 2, 2016

Contributor

Can this cause the current wallet to be wiped out if recovery fails?

This comment has been minimized.

Sign in to view

Copy link
@willy-b

willy-b Dec 2, 2016

Contributor

After testing, this line does appear to cause a "Status: Brave Wallet can't be reached" display on front page after failed wallet recoveries:
bravewalletcantbereached

(Doesn't happen without the appActions.updateLedgerInfo call.)

This comment has been minimized.

Sign in to view

Copy link
@mrose17

mrose17 Dec 3, 2016

Author Member

@Willly-B thanks for testing it. in looking at the ledger-client code, i'm pretty sure that recoveryWallet is "ACID" ... if the call fails, no state is changed...

This comment has been minimized.

Sign in to view

Copy link
@willy-b

willy-b Dec 3, 2016 •
edited
Loading

Contributor

Yes, I think you're right that client.recoverWallet does not change the state, but if recoverWallet fails, then L296 (which we are commenting on) calls appActions.updateLedgerInfo, which does update the state. The update performed here seems to invalidate the wallet.

This comment has been minimized.

Sign in to view

Copy link
@mrose17

mrose17 Dec 3, 2016

Author Member

how does it "invalidate" the wallet? just try again, right?

This comment has been minimized.

Sign in to view

Copy link
@bsclifton

bsclifton Dec 4, 2016

Member

@mrose17 @willy-b any action items needed for this?

This comment has been minimized.

Sign in to view

Copy link
@willy-b

willy-b Dec 4, 2016

Contributor

@mrose17 could you maybe explain why the _internal field needs to be cleared on ledgerInfo if LedgerClient.recoverWallet fails? (esp if recoverWallet doesn't change state.)

This comment has been minimized.

Sign in to view

Copy link
@mrose17

mrose17 Dec 5, 2016

Author Member

we never send _internal to the render process. it is just internal state for `ledger.js``

This comment has been minimized.

Sign in to view

Copy link
@willy-b

willy-b Dec 5, 2016 •
edited
Loading

Contributor

So why clear it here? It is internal state for the current wallet and is not modified by LedgerClient.recoverWallet.
When we recover, the balance of the recovered wallet is transferred to the current wallet (correct me if I'm wrong here) -- if this process fails, why delete the internal state of the current wallet?
I have observed this causing client to behave as if current wallet doesn't exist (see screenshot above for example)

This comment has been minimized.

Sign in to view

Copy link
@mrose17

mrose17 Dec 5, 2016

Author Member

just to be "clear", what is being cleared is not the value in the ledgerInfo structure, but what is getting sent to appActions.updateLedger. look at these calls and you'll see the pattern:

    grep -nH appActions.updateLedger ledger.js
    ledger.js:229:    appActions.updateLedgerInfo({ creating: true })
    ledger.js:233:      appActions.updateLedgerInfo({})
    ledger.js:290:    if (logError(err, 'recoveryWallet')) appActions.updateLedgerInfo(underscore.omit(ledgerInfo, [ '_internal' ]))
    ledger.js:320:      appActions.updateLedgerInfo(underscore.omit(ledgerInfo, [ '_internal' ]))
    ledger.js:526:    return appActions.updateLedgerInfo({})
    ledger.js:586:    appActions.updateLedgerInfo({})
    ledger.js:1063:  appActions.updateLedgerInfo(underscore.omit(ledgerInfo, [ '_internal' ]))

This comment has been minimized.

Sign in to view

Copy link
@willy-b

willy-b Dec 5, 2016 •
edited
Loading

Contributor

Ah, I see what you're saying about never sending _internal -- I misunderstood that response. Thanks for "clearing" that up ;-). Let me find what's behind the screenshot above and why I was getting it with this line enabled.

This comment has been minimized.

Sign in to view

Copy link
@willy-b

willy-b Dec 6, 2016 •
edited
Loading

Contributor

Hey, sorry it took a bit to get back to you on this.

tl;dr: if you merge this line, a failed wallet recovery will cause the payments panel to show errors until you either do a successful recovery, add funds, or restart browser.

Why? You're right that the _internal bit I mentioned earlier is irrelevant. It's the combination of the logError call and the updateLedgerInfo call on the same line that causes the wallet UI to show Brave wallet can't be reached (see screenshot above) if recovery fails, even if you had an existing wallet. This error message continues to show even if you switch tabs within preferences or reload the page (without closing browser).

In addition to logging, logError sets the error field on the ledgerInfo object:

var logError = (err, caller) => {
  if (err) {
    ledgerInfo.error = {
      caller: caller,
      error: err
    }

That is sent to the renderer via appActions.updateLedgerInfo, where various payment panel components in js/about/preferences.js will not render unless ledgerInfo.get('error') is falsey / null, e.g.:

  get walletStatus () {
    const ledgerData = this.props.ledgerData
    let status = {}
    if (ledgerData.get('error')) {
      status.id = 'statusOnError'
    } else if (ledgerData.get('created')) {
    // [...]

If this line is not merged, then your existing wallet still displays even if a recovery fails.
if (err) {
setImmediate(() => appActions.ledgerRecoveryFailed())
} else {
Expand All @@ -302,6 +303,15 @@ var recoverKeys = (appState, action) => {
*/

if (ipc) {
ipc.on(messages.LEDGER_PAYMENTS_PRESENT, (event, presentP) => {
if (presentP) {
if (!balanceTimeoutId) getBalance()
} else if (balanceTimeoutId) {
clearTimeout(balanceTimeoutId)
balanceTimeoutId = false
}
})

ipc.on(messages.CHECK_BITCOIN_HANDLER, (event, partition) => {
const protocolHandler = session.fromPartition(partition).protocol
// TODO: https://github.com/brave/browser-laptop/issues/3625
Expand Down
10 changes: 10 additions & 0 deletions js/components/tab.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ const ImmutableComponent = require('./immutableComponent')

const windowActions = require('../actions/windowActions')
const dragTypes = require('../constants/dragTypes')
const messages = require('../constants/messages')
const cx = require('../lib/classSet')
const {getTextColorForBackground} = require('../lib/color')
const {isIntermediateAboutPage} = require('../lib/appUrlUtil')

const contextMenus = require('../contextMenus')
const dnd = require('../dnd')
const windowStore = require('../stores/windowStore')
const ipc = global.require('electron').ipcRenderer

class Tab extends ImmutableComponent {
constructor () {
Expand Down Expand Up @@ -254,4 +256,12 @@ class Tab extends ImmutableComponent {
}
}

windowStore.addChangeListener(() => {
var presentP = false
const windowState = windowStore.getState().toJS()

windowState.tabs.forEach((tab) => { if (tab.location === 'about:preferences#payments') presentP = true })

ipc.send(messages.LEDGER_PAYMENTS_PRESENT, presentP)
})
module.exports = Tab
2 changes: 2 additions & 0 deletions js/constants/coinbaseCountries.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const coinbaseCountries = [
/* although coibnase operates in all these countries, the debit/credit payment service is, at present, only in the US
'AT',
'AU',
'BE',
Expand Down Expand Up @@ -35,6 +36,7 @@ const coinbaseCountries = [
'SI',
'SK',
'SM',
*/
'US'
]

Expand Down
1 change: 1 addition & 0 deletions js/constants/messages.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ const messages = {
// Debugging
DEBUG_REACT_PROFILE: _,
// Ledger
LEDGER_PAYMENTS_PRESENT: _,
LEDGER_PUBLISHER: _,
LEDGER_UPDATED: _,
LEDGER_CREATE_WALLET: _,
Expand Down

0 comments on commit 9c33676

Please sign in to comment.