ghost commented 2 years ago

Context (issues, jira)

https://ledgerhq.atlassian.net/browse/LIVE-2241

Description / Usage

Cosmos accounts containing assets other than ATOM had an incorrect ATOM balance that was including the balance of all assets. This PR makes sure only ATOM is used for the balance computation.

Before

Cosmos legacy cosmos1c...na2l68hq: 111.45 ATOM (98ops) (cosmos1c8qxw4dsj37dy0s2dmsc34ahvkredgna2l68hq on 44'/118'/0'/0/0) #0 js:2:cosmos:cosmos1c8qxw4dsj37dy0s2dmsc34ahvkredgna2l68hq: (! sum of ops 646.386381 ATOM) 0.449751 ATOM spendable. 111.002554 ATOM delegated.

After

Cosmos legacy cosmos1c...na2l68hq: 111.35 ATOM (98ops) (cosmos1c8qxw4dsj37dy0s2dmsc34ahvkredgna2l68hq on 44'/118'/0'/0/0) #0 js:2:cosmos:cosmos1c8qxw4dsj37dy0s2dmsc34ahvkredgna2l68hq: (! sum of ops 646.386381 ATOM) 0.349751 ATOM spendable. 111.002554 ATOM delegated.

Expectations

[ ] Test coverage: The changes of this PR are covered by test. Unit test were added with mocks when depending on a backend/device.
[x] No impact: The changes of this PR have ZERO impact on the userland. Meaning, we can use these changes without modifying LLD/LLM at all. It will be a "noop" and the maintainers will be able to bump it without changing anything.

vercel[bot] commented 2 years ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
ledger-live-common	❌ Failed (Inspect)		May 6, 2022 at 4:52PM (UTC)

codecov[bot] commented 2 years ago

Codecov Report

Merging #1943 (bffd6e7) into develop (d9b83d8) will decrease coverage by 27.22%. The diff coverage is 84.61%.

@@             Coverage Diff              @@
##           develop    #1943       +/-   ##
============================================
- Coverage    56.42%   29.19%   -27.23%     
============================================
  Files          450      450               
  Lines        20822    20559      -263     
  Branches      5356     5280       -76     
============================================
- Hits         11749     6003     -5746     
- Misses        9045    14550     +5505     
+ Partials        28        6       -22

Impacted Files	Coverage Δ
src/families/cosmos/api/Cosmos.ts	`86.45% <81.81%> (-1.19%)`	:arrow_down:
src/families/cosmos/js-synchronisation.ts	`95.49% <100.00%> (ø)`
src/families/bitcoin/wallet-btc/crypto/factory.ts	`3.61% <0.00%> (-89.16%)`	:arrow_down:
src/families/bitcoin/wallet-btc/xpub.ts	`2.95% <0.00%> (-88.76%)`	:arrow_down:
...bitcoin/wallet-btc/pickingstrategies/CoinSelect.ts	`8.79% <0.00%> (-85.72%)`	:arrow_down:
src/families/bitcoin/js-synchronisation.ts	`13.14% <0.00%> (-83.43%)`	:arrow_down:
src/families/solana/api/queued.ts	`16.66% <0.00%> (-83.34%)`	:arrow_down:
src/families/bitcoin/wallet-btc/crypto/dash.ts	`16.66% <0.00%> (-83.34%)`	:arrow_down:
src/families/bitcoin/wallet-btc/explorer/index.ts	`5.75% <0.00%> (-82.74%)`	:arrow_down:
src/families/bitcoin/networks.ts	`5.88% <0.00%> (-82.36%)`	:arrow_down:
... and 228 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d9b83d8...bffd6e7. Read the comment docs.

ghost commented 2 years ago

Would be ideal to add a test for this into the datasets. I'm not sure how difficult it would be to regenerate them and clean up the libcore references.

You're definitely right about adding an account in datasets, we just don't have an appropriate one at the moment unfortunately... I've asked for one to our QA but they may not have bandwidth now, so I'll merge this now, but whenever an account is provided we'll have a task to add it to the datasets.

About the libcore accounts, I think these should stay as they actually allow to test migration for users with old builds.

LedgerHQ / ledger-live-common

LIVE-2241 Take care about currency for balance computation (#1941) #1943