frontend/account-summary: add coins total balance #2573
Conversation
strmci
force-pushed
the
add_coins_total_balance
branch
from
77d0103 to
1744a10
Compare
strmci
changed the title
strmci
force-pushed
the
add_coins_total_balance
branch
4 times, most recently
from
f6cf02a to
d2e7dfa
Compare
strmci
force-pushed
the
add_coins_total_balance
branch
from
d2e7dfa to
9b1d8d9
Compare
|
@thisconnect updated, PTAL |
|
I've only tested with with a BitBox and a remembered wallet. Looks good. Maybe a bit unrelated but when I unplug the device, the chart sometimes looks funny and goes into a bit a weird state. PR #2635 should properly mount and unmount the chart if there are changes. So I think we should merge #2635 first. |
|
@strmci is this the same backend code that got removed in an earlier commit? |
|
@strmci please rebase |
yes, it is the same code |
strmci
force-pushed
the
add_coins_total_balance
branch
from
9b1d8d9 to
84690cb
Compare
could you add the commit hash where it got removed in your commit message, so it is easier to review? |
strmci
force-pushed
the
add_coins_total_balance
branch
from
84690cb to
3c748d7
Compare
@thisconnect rebased and commit hash added, PTAL |
There was a problem hiding this comment.
frontend tested LGTM
defering to @Beerosagos to take a look at backend change
| @@ -749,6 +750,59 @@ func (handlers *Handlers) getAccountsBalance(*http.Request) (interface{}, error) | |||
| return totalAmount, nil | |||
| } | |||
|
|
|||
| // getCoinsTotalBalance returns the total balances grouped by coins. | |||
| func (handlers *Handlers) getCoinsTotalBalance(_ *http.Request) (interface{}, error) { | |||
There was a problem hiding this comment.
I just merged a PR that creates a new method backend.accountFiatBalance (https://github.com/digitalbitbox/bitbox-wallet-app/blob/master/backend/accounts.go#L230). It would be nice to refactor getCoinsTotalBalance to exploit the function and avoid code duplication.
There was a problem hiding this comment.
Also, it would be nice to add unit tests for getCoinsTotalBalance, but it's probably easier to do it after #2633 gets merged
There was a problem hiding this comment.
It would be nice to refactor getCoinsTotalBalance to exploit the function and avoid code duplication.
Never mind, I didn't realize that the function was providing the fiat conversions for each enabled fiat and that now it is possible to cycle across fiat units also in the account summary.
strmci
force-pushed
the
add_coins_total_balance
branch
from
3c748d7 to
ce1bb69
Compare
Before this update, the account summary didn't show total coin balances, especially when managing multiple wallets with the watch-only feature. This commit addresses that by adding a new coin balances table below the chart. It appears only when there are multiple wallets in the portfolio. Restored backend code previously removed in commit d8c6ba0.
strmci
force-pushed
the
add_coins_total_balance
branch
from
ce1bb69 to
8239c92
Compare
|
This was missing a CHANGELOG entry, see also: #2613 |
Before this update, the account summary didn't show total coin balances, especially when managing multiple wallets with the watch-only feature. This commit addresses that by adding a new coin balances table below the chart. It appears only when there are multiple wallets in the portfolio.
Restored backend code previously removed in commit d8c6ba0.