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

introduce wallet store, decoupling of account user's data (name, starred,..) from the account's blockchain data #6534

Closed
wants to merge 1 commit into from

Conversation

gre
Copy link
Contributor

@gre gre commented Mar 25, 2024

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • all projects (LLD, LLM, CLI)
    • Add Accounts flow
    • State management of accounts (load / save of accounts in the app.json / llm's storage)
    • ordering of accounts
    • starring an Account feature (LLD)
    • Account name edition
    • Wallet API and its ability to send the user's account names
    • export to mobile feature (LiveQR)
    • account search by name

πŸ“ Description

Goals

initial specification: https://ledgerhq.atlassian.net/wiki/spaces/WXP/pages/4574642221/move+Account+user+data+states+out+of+Account+model

The main goal is to make the type Account exclusively having "blockchain data" and no longer any "user data" fields. (example of user data field includes: starred, name, unit,... those are editable by the end user and typically can't be restored from the blockchain). By splitting these user data fields out, we simplify the logic of the Account[] lifecycle as well as allowing ourself a future where we can work on different ways to "export/import" user data (aka "wallet sync") more easily and no longer with data "nested" among the blockchain public/immutable data.

The fields that are dropped by this PR are:

  • Account#name
  • Account#starred and TokenAccount#starred

The secondary goal includes the modularisation of live-common by exporting all logic related to account name management as well as lifecycle of some account data that need the account.name to work (the UI logic for addAccounts, ordering of accounts, importAccounts). We have therefore created a library live-wallet to allow this modularization.

Backward compatible!

βœ… There are no migration involved!

  • We have preserved the same app.json data. Which means the AccountRaw has now been touched. Instead, we have made that AccountRaw will be split into two parts: Account + AccountUserData

this is why you can see in the code that we have moved from

DataModel<AccountRaw, Account>

to

DataModel<AccountRaw, [Account, AccountUserData]>

To make this work, the underlying function toAccountRaw has been reworked to include a userData as a second parameter, as seen in the implementation:

decode: (raw: AccountRaw) => [fromAccountRaw(raw), accountRawToAccountUserData(raw)],
encode: ([account, userData]: [Account, AccountUserData]): AccountRaw => toAccountRaw(account, userData),

live-wallet breakdown

The live-wallet library currently includes:

  • liveqr: all the logic around the "live qr" which is this animated qr code that we today uses to scan-import accounts from desktop to mobile app
    • cross: it's under "liveqr" because it's only needed for this. (historically was in live-common)
    • importAccounts: the high level logic (used by the UI) to do the import accounts (during the qr scan) (historically was in live-common)
  • ordering: manage the account sorting logic (historically was in coin-framework)
  • accountName: manage the logic around account name defaulting and validation (historically was in coin-framework)
  • addAccounts: manage the logic used by the Add Accounts flow to deduplicate, rename and import accounts in the model (historically was in live-common)
  • store: export a reducer, actions and other logic to create a store that contains the wallet data (currently have accountNames and starredAccountIds)

Main library changes

Each time we used to be able to do account.name or account.starred we will no longer be able to access it as a simple field. Instead, the data needs to be looked up from the "WalletState" store.

So code like

const name = account.name

will be replaced by one of these (depending on the context)

const name = accountNameWithDefaultSelector(walletState, account) // if you have walletState in context, (typically in the logic side)
const name = useAccountName(account) // if you can afford a React hook (typically in the UI side)

If you only need the "default account name", you can also use

import { getDefaultAccountName } from "@ledgerhq/live-wallet/accountName";
const name = getDefaultAccountName(account);

❓ 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)

Copy link

vercel bot commented Mar 25, 2024

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

Name Status Preview Comments Updated (UTC)
web-tools βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Apr 25, 2024 4:16pm
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Apr 25, 2024 4:16pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Apr 25, 2024 4:16pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Apr 25, 2024 4:16pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Apr 25, 2024 4:16pm

Copy link

socket-security bot commented Mar 28, 2024

No dependency changes detected. Learn more about Socket for GitHub β†—οΈŽ

πŸ‘ No dependency changes detected in pull request

@gre
Copy link
Contributor Author

gre commented May 2, 2024

closing in favor of #6796

@gre gre closed this May 2, 2024
@gre gre deleted the LIVE-11767 branch May 30, 2024 09:15
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

2 participants