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

Display correct balance from state when creating an account that already holds funds #1759

Closed
wants to merge 4 commits into from

Conversation

rickycodes
Copy link
Member

@rickycodes rickycodes commented Aug 10, 2020

Description

re: https://trello.com/c/NytEfYLU/191-after-importing-a-wallet-with-seedphrase-new-account-shows-0-balance-until-another-account-is-created

Previously when adding a new account that already has funds we'd see it displayed with zero in the drawer:

image

(you can see the fiat balance is correct in the background)

Now we check state to see if we have the amount and display it if there's a balance for the matching account:

image

Checklist

  • There is a related Trello card
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #???

@rickycodes rickycodes requested a review from a team as a code owner August 10, 2020 05:05
@rickycodes rickycodes added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Aug 10, 2020
const { accounts } = AccountTrackerController;
const selectedAccount = accounts[selectedAddress];
const selectedAccountHasBalance =
selectedAccount && Object.prototype.hasOwnProperty.call(selectedAccount, BALANCE_KEY);
Copy link
Member Author

@rickycodes rickycodes Aug 10, 2020

Choose a reason for hiding this comment

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

normally i would just use obj.hasOwnProperty() directly here, but I guess we don't allow that with this rule: https://eslint.org/docs/rules/no-prototype-builtins

@rickycodes rickycodes changed the title Display correct balance from state Display correct balance from state when creating an account that already holds funds Aug 10, 2020
Copy link
Contributor

@EtDu EtDu left a comment

Choose a reason for hiding this comment

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

Is it possible to move this logic up into getAccounts() in the AccountList parent component? That way we can be sure the item.balance passed down is always accurate and we can leave the child AccountElement unconnected/stateless.

@rickycodes
Copy link
Member Author

rickycodes commented Aug 18, 2020

@EtDu I just tried to move this logic into getAccounts() in the AccountList (as suggested) but it doesn't seem to be working as expected. I think the actual problem is that the selectedAddress prop isn't functioning as expected? While I agree this isn't an ideal fix, it does resolve the issue. feel free to take a stab if you'd like, it's possible I am missing something silly.

@omnat omnat added the next up prioritized to be picked up next label Aug 18, 2020
Copy link
Contributor

@EtDu EtDu left a comment

Choose a reason for hiding this comment

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

SOLID

@rickycodes rickycodes force-pushed the bugfix/display-correct-balance-from-state branch from 5c23554 to 4cb235f Compare August 20, 2020 14:18
@rickycodes
Copy link
Member Author

rickycodes commented Aug 26, 2020

grouping this in with #1797

@rickycodes rickycodes closed this Aug 26, 2020
@rickycodes rickycodes deleted the bugfix/display-correct-balance-from-state branch November 19, 2020 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. next up prioritized to be picked up next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants