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

Fix QR codes not displaying for new wallets #11496

Merged
merged 2 commits into from
Oct 13, 2017
Merged

Fix QR codes not displaying for new wallets #11496

merged 2 commits into from
Oct 13, 2017

Conversation

bsclifton
Copy link
Member

Fix unconfirmed funds check which prevented code from setting payment data

Fixes #11492

Auditors: @NejcZdovc

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.

Test Plan:

Reviewer Checklist:

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

@bsclifton bsclifton added this to the 0.19.x Release 2 (Beta Channel) milestone Oct 13, 2017
@bsclifton bsclifton self-assigned this Oct 13, 2017
@bsclifton bsclifton requested review from NejcZdovc and bbondy October 13, 2017 00:01
@bsclifton
Copy link
Member Author

Problem was introduced with c965002#diff-3cdfdb124f718787d584e9e2b82fcedcR1720 via #11457

When reviewing, we need to make sure this issue is still solved:
#11449

@codecov-io
Copy link

codecov-io commented Oct 13, 2017

Codecov Report

Merging #11496 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11496      +/-   ##
==========================================
+ Coverage    52.5%   52.53%   +0.03%     
==========================================
  Files         268      268              
  Lines       25241    25218      -23     
  Branches     4028     4023       -5     
==========================================
- Hits        13252    13248       -4     
+ Misses      11989    11970      -19
Flag Coverage Δ
#unittest 52.53% <ø> (+0.03%) ⬆️
Impacted Files Coverage Δ
app/browser/api/ledger.js 31.78% <ø> (+0.14%) ⬆️
app/filtering.js 18.59% <0%> (-0.07%) ⬇️
js/about/aboutActions.js 9.32% <0%> (ø) ⬆️
js/constants/appConfig.js 100% <0%> (ø) ⬆️

@@ -1717,7 +1717,7 @@ const onWalletProperties = (state, body) => {

// unconfirmed amount
const unconfirmed = parseFloat(body.get('unconfirmed'))
if (unconfirmed >= 0) {
if (unconfirmed > 0) {
Copy link
Contributor

@NejcZdovc NejcZdovc Oct 13, 2017

Choose a reason for hiding this comment

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

I think this change is not correct, because you could have wallet that have unconfirmed balance, let's say 10$. Then you import wallet from the backup, which have unconfirmed balance 0. New value will not be saved.

Another use case is when your unconfirmed balance is handled, you can't hide that message, because 0 will not be saved.

This return in the if block is more or less from the old code. What I would do is to change it to

const unconfirmed = parseFloat(body.get('unconfirmed'))
if (unconfirmed >= 0) {
    state = ledgerState.setInfoProp(state, 'unconfirmed', unconfirmed)
    if (clientOptions.verboseP) {
      console.log('\ngetBalance refreshes ledger info: ' + ledgerState.getInfoProp(state, 'unconfirmed'))
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

want to just PR an alt fix @NejcZdovc ?

Copy link
Contributor

Choose a reason for hiding this comment

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

will push to this PR

@NejcZdovc
Copy link
Contributor

@bbondy @bsclifton pushed a fix that removes return statements. Please check if everything is ok.

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.

change pushed

@bsclifton
Copy link
Member Author

bsclifton commented Oct 13, 2017

Very nice- thanks, @NejcZdovc 😄

  • QR code loads properly with this
  • recovering wallet should also work great since flow is unchanged

@bsclifton bsclifton merged commit 0541557 into brave:master Oct 13, 2017
@bsclifton bsclifton deleted the fix-qr-code branch October 13, 2017 04:34
bsclifton added a commit that referenced this pull request Oct 13, 2017
Fix QR codes not displaying for new wallets
bsclifton added a commit that referenced this pull request Oct 13, 2017
Fix QR codes not displaying for new wallets
bsclifton added a commit that referenced this pull request Oct 13, 2017
Fix QR codes not displaying for new wallets
@bsclifton
Copy link
Member Author

master 0541557
0.21.x 4403ea4
0.20.x 4d5b422
0.19.x 9bed2cf

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