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

chore: decoupled {unit, starred, name} fields away from Account model #6796

Merged
merged 2 commits into from May 7, 2024

Conversation

gre
Copy link
Contributor

@gre gre commented May 2, 2024

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • see respective PRs for documented impacts

πŸ“ Description

This PR mashups many iterations on our Account model rework. some of these PRs were incrementally reviewed, so we now hope for a final review on this PR πŸ™ πŸ‘ NB: on this big mashup PR: it is easier to refactor the Account model in one batch and also with this one squashed commit, especially how it is complex to rebase with develop when changes are made there. see respective PRs for extended documentations:

user impact

For users, this PR impacts one thing: the unit settings (e.g. seeing amounts in satoshi instead of BTC) was moved to the currency settings.

Capture d’écran 2024-05-02 aΜ€ 10 26 25

Capture d’écran 2024-05-03 aΜ€ 10 52 28

per the PR comment below, this PR will however not address yet the usage of the currency unit in the "currency level features" like the account distributions or the assets screens.

Capture d’écran 2024-05-03 aΜ€ 10 51 55

That said, we removed the inconsistency behaviour of our app that was allowing to have different unit on different accounts πŸ˜†

Capture d’écran 2024-05-03 aΜ€ 10 56 05

the other user benefits is that, since we moved the unit from account unit to global currency unit, the global UX will be improved, notably this will fixed bugs related to unit lagging in the UI to be changed until the app is restarted (fixes https://ledgerhq.atlassian.net/browse/LIVE-12288 & https://ledgerhq.atlassian.net/browse/LIVE-11956 )

mini guide for developers

The other changes brought by this PR are only impacting the developers. The whole point was to completely remove all user's off chain config away from "Account", making this Account type entirely restorable from the blockchain data.

before this PR:

1

after this PR: we have a true decoupling of the data (between on chain and off chain data) while keeping the same AccountRaw serialized data

2

therefore, for developers, the impact is that you no longer have unit, starred, name fields directly available in Account. Instead you will get it on the end projects (LLD, LLM) with some accessors (selectors or hooks).
You will be able to easily switch from one approach to another with these:

- account.unit
- getAccountUnit(account)
+ useAccountUnit(account)
+ useMaybeAccountUnit(maybeAccount)
+ accountUnitSelector(state, account)
# ^ when we can't afford hooks, we use the redux store to lense the account's unit
+ account.currency.units[0]
# ^ if and only if we can afford to just fallback to the default's unit

- account.name
- getAccountName(account)
+ useAccountName(account)
+ useMaybeAccountName(maybeAccount)
+ accountNameWithDefaultSelector(walletState, account) 
# ^ when we can't afford hooks, we use the walletState store to lense the account's name
+ getDefaultAccountName(account)
# ^ if and only if we can afford to just fallback to the default and initial account's name

- account.starred
+ accountStarredSelector(state, { accountId: account.id })
# (nb: very few usage of this one, it is ok to only have a basic selector)

ℹ️ πŸͺ“ on the recent Account model and account management simplifications, see also:

❓ Context


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@gre gre requested review from a team as code owners May 2, 2024 08:22
Copy link

vercel bot commented May 2, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

5 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview May 6, 2024 1:04pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview May 6, 2024 1:04pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview May 6, 2024 1:04pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview May 6, 2024 1:04pm
web-tools ⬜️ Ignored (Inspect) Visit Preview May 6, 2024 1:04pm

Copy link
Contributor

@sprohaszka-ledger sprohaszka-ledger left a comment

Choose a reason for hiding this comment

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

I have still some files to review (all the coin families at least), but right now 2 things:

  • coin-framework must not have any live-wallet logic inside. It's purpose is UI-agnostic.
  • AccountRaw should reflect what Account is, not what Account+AccountUserData. We need to isolate backward compatibility issue in dedicated function, not in DataModel, as much as possible.

@gre
Copy link
Contributor Author

gre commented May 2, 2024

notes after testing LLM:

  • account distribution don't take unit into account
    • i see BTC under the pie chart where my accounts are in sats
    • currency asset screen don't take unit into account (e.g. the Bitcoin assets screen show the total balance in BTC where the account are correctly shown in sats)
    • same for the crypto home screen

now this seems to be purely intentional to not take the account unit into account, because before it wasn't even shared at the currency level, so we will not address this in this rework I think, but i'll ask product team.

apps/ledger-live-mobile/src/screens/WalletCentricAsset/AssetRow.tsx

Capture d’écran 2024-05-02 aΜ€ 16 58 15

Copy link
Contributor

@sprohaszka-ledger sprohaszka-ledger left a comment

Choose a reason for hiding this comment

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

πŸ‘ for Coin Integ team ownership.

Copy link
Contributor

@Justkant Justkant left a comment

Choose a reason for hiding this comment

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

LGTM tested mobile and desktop locally too

Copy link
Contributor

@ofreyssinet-ledger ofreyssinet-ledger left a comment

Choose a reason for hiding this comment

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

reviewed mainly the device experience team scope and it's all good πŸ‘

@gre gre merged commit 6552679 into develop May 7, 2024
58 checks passed
@gre gre deleted the support/account-model-decoupling branch May 7, 2024 09:09
Copy link

sentry-io bot commented May 10, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ TypeError: Cannot read properties of null (reading 'contract') fromHydrateValidator(libs/ledger-live-common/sr... View Issue
  • ‼️ TypeError: t.toFixed is not a function toOperationRaw(libs/coin-framework/src/serializ... View Issue
  • ‼️ TypeError: Cannot convert object to primitive value mergeOps(libs/coin-framework/src/bridge/jsHelpers) View Issue
  • ‼️ Error: currency with id "ethereum_goerli" not found getCryptoCurrencyById(libs/ledgerjs/packages/cr... View Issue
  • ‼️ RangeError: Invalid time value toOperationRaw(libs/coin-framework/src/serializ... View Issue

Did you find this useful? React with a πŸ‘ or πŸ‘Ž

@gre
Copy link
Contributor Author

gre commented May 10, 2024

can't be related

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli common Has changes in live-common desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs mobile Has changes in LLM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants