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

fix(llm): improve typing of ReceiveFunds/02-SelectAccount and fix crash case #7148

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

gre
Copy link
Contributor

@gre gre commented Jun 20, 2024

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
    • no tests yet but covered by type safety that wasn't correctly made before my change, on the whole SelectAccount file.
  • Impact of the changes:
    • Receive funds flow when selecting a token

📝 Description

Root cause: the rework of #6534

Capture d’écran 2024-06-21 à 13 38 13

as seen here, we did a mistake on LLM ReceiveFunds/02-SelectAccount file where we passed a Currency instead of an Account on the getDefaultAccountName function.
unfortunately the bad typing was hiding the issue: bad casting using item.parentAccount as Account which hides the problem. because actually .parentAccount is a Account | undefined. since we didn't have the | undefined part, TypeScript will infer the code as a Account and not as an Account | Currency that is actually the type of the previously bad code here.

the fix:

Capture d’écran 2024-06-21 à 13 36 08

❓ 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 Jun 20, 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 Jun 20, 2024 1:18pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Jun 20, 2024 1:18pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jun 20, 2024 1:18pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jun 20, 2024 1:18pm
web-tools ⬜️ Ignored (Inspect) Visit Preview Jun 20, 2024 1:18pm

@live-github-bot live-github-bot bot added the mobile Has changes in LLM label Jun 20, 2024
@gre gre requested a review from KVNLS June 20, 2024 12:56
@gre gre force-pushed the fix/receiveselectaccount branch from 4e4e6a5 to 1749a31 Compare June 20, 2024 13:15
@gre gre marked this pull request as ready for review June 20, 2024 14:05
@gre gre requested a review from a team as a code owner June 20, 2024 14:05
Copy link
Member

@KVNLS KVNLS left a comment

Choose a reason for hiding this comment

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

When I look at the Sentry error, the error happens in live-wallet/lib/accountName.js
Could make the function resilient, please?

const getDefaultAccountName = (account) => {
    if (account.type === "Account") {
        return (0, exports.getDefaultAccountNameForCurrencyIndex)(account);
    }
    return account.token.name;
};

Here we are not sure that we can access account.token.name.
Could you add the unit test to simulate the issue we had, please?

@gre
Copy link
Contributor Author

gre commented Jun 21, 2024

@KVNLS the error is not in live-wallet, it's a misusage of the getDefaultAccountName function. Instead of giving an Account we have previously given a CryptoCurrency 😨 , so we didn't respected the type. and the error basically is like if you would given a bad object to any function.
the overusage of as casting has prevented the problem to be detected with type safety.

the getDefaultAccountName function

(account: AccountLike) => {
  if (account.type === "Account") {
    return getDefaultAccountNameForCurrencyIndex(account);
  }
  return account.token.name;
};

if type properly respected, should not fail.

Sorry, the rootcause wasn't fully clear, so I have updated the PR description with all details of why the error happened and the fact it was hidden by as Account which should have been a as Account | undefined. Now that the TypeScript is fixed, we can't have this situation where we accept again a currency instead of an account to the function, and indeed that function should not accept a Currency.

I didn't had enough time to make a proper end to end test on mobile and I suggest we proceed to taking this fix in the hotfix first, because properly testing the receive flow will take some time, there are many different cases I could see in the code (at least we had two cases to cover the two sides of this || , in this PR fix, I actually converged to just use the item's name)

@gre gre requested a review from KVNLS June 24, 2024 08:16
@gre gre merged commit 1ae0a50 into develop Jun 24, 2024
45 of 51 checks passed
@gre gre deleted the fix/receiveselectaccount branch June 24, 2024 12:17
gre added a commit that referenced this pull request Jun 24, 2024
hzheng-ledger added a commit that referenced this pull request Jun 24, 2024
* chore(hotfix) 🚀 entering hotfix mode

* Polkadot generic app support (#6326)

* getAddress support

* fix: add metadata support

* fix: get polkadot app version

* fix: refactoring

* fix: refactoring

* fix: refactoring

* fix: refactoring

* fix: small refactoring

* fix: update doc

* feat: add Polkadot coin configuration

Signed-off-by: Stéphane Prohaszka <[email protected]>

* polkadot config location

* fix: polkadot transaction craft

* remove useless code

* fix: refactoring

* fix: runtime upgrade transaction

* fix: update doc

* fix: unit tests

* fix: update changelog

* fix: update pnpm-lock

* fix: update polkadot integration test APDU

---------

Signed-off-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: hzheng-ledger <[email protected]>
Co-authored-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: lvndry <[email protected]>

* chore(hotfix): 🔥 hotfix prerelease [LLD(2.82.1-hotfix.0), LLM(3.45.1-hotfix.0)]

* fix(llm): improve typing of ReceiveFunds/02-SelectAccount and fix crash case (#7148)

* chore(hotfix): 🔥 hotfix prerelease [LLD(2.82.1-hotfix.0), LLM(3.45.1-hotfix.1)]

* chore(hotfix): 🔥 hotfix release [skip ci]

* fix: update pnpm-lock

---------

Signed-off-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: live-github-bot[bot] <105061298+live-github-bot[bot]@users.noreply.github.com>
Co-authored-by: Hedi EDELBLOUTE <[email protected]>
Co-authored-by: hzheng-ledger <[email protected]>
Co-authored-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: lvndry <[email protected]>
Co-authored-by: @greweb <[email protected]>
kallen-ledger added a commit that referenced this pull request Jun 26, 2024
* chore(hotfix) 🚀 entering hotfix mode

* Polkadot generic app support (#6326)

* getAddress support

* fix: add metadata support

* fix: get polkadot app version

* fix: refactoring

* fix: refactoring

* fix: refactoring

* fix: refactoring

* fix: small refactoring

* fix: update doc

* feat: add Polkadot coin configuration

Signed-off-by: Stéphane Prohaszka <[email protected]>

* polkadot config location

* fix: polkadot transaction craft

* remove useless code

* fix: refactoring

* fix: runtime upgrade transaction

* fix: update doc

* fix: unit tests

* fix: update changelog

* fix: update pnpm-lock

* fix: update polkadot integration test APDU

---------

Signed-off-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: hzheng-ledger <[email protected]>
Co-authored-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: lvndry <[email protected]>

* chore(hotfix): 🔥 hotfix prerelease [LLD(2.82.1-hotfix.0), LLM(3.45.1-hotfix.0)]

* fix(llm): improve typing of ReceiveFunds/02-SelectAccount and fix crash case (#7148)

* chore(hotfix): 🔥 hotfix prerelease [LLD(2.82.1-hotfix.0), LLM(3.45.1-hotfix.1)]

* chore(hotfix): 🔥 hotfix release [skip ci]

* chore: add changeset

* fix: move polkdot getcrypto call outside of currency config fn

---------

Signed-off-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: live-github-bot[bot] <105061298+live-github-bot[bot]@users.noreply.github.com>
Co-authored-by: Hedi EDELBLOUTE <[email protected]>
Co-authored-by: hzheng-ledger <[email protected]>
Co-authored-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: lvndry <[email protected]>
Co-authored-by: @greweb <[email protected]>
kallen-ledger added a commit that referenced this pull request Jun 26, 2024
* chore(hotfix) 🚀 entering hotfix mode

* Polkadot generic app support (#6326)

* getAddress support

* fix: add metadata support

* fix: get polkadot app version

* fix: refactoring

* fix: refactoring

* fix: refactoring

* fix: refactoring

* fix: small refactoring

* fix: update doc

* feat: add Polkadot coin configuration

Signed-off-by: Stéphane Prohaszka <[email protected]>

* polkadot config location

* fix: polkadot transaction craft

* remove useless code

* fix: refactoring

* fix: runtime upgrade transaction

* fix: update doc

* fix: unit tests

* fix: update changelog

* fix: update pnpm-lock

* fix: update polkadot integration test APDU

---------

Signed-off-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: hzheng-ledger <[email protected]>
Co-authored-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: lvndry <[email protected]>

* chore(hotfix): 🔥 hotfix prerelease [LLD(2.82.1-hotfix.0), LLM(3.45.1-hotfix.0)]

* fix(llm): improve typing of ReceiveFunds/02-SelectAccount and fix crash case (#7148)

* chore(hotfix): 🔥 hotfix prerelease [LLD(2.82.1-hotfix.0), LLM(3.45.1-hotfix.1)]

* chore(hotfix): 🔥 hotfix release [skip ci]

* chore: add changeset

* fix: move polkdot getcrypto call outside of currency config fn

---------

Signed-off-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: live-github-bot[bot] <105061298+live-github-bot[bot]@users.noreply.github.com>
Co-authored-by: Hedi EDELBLOUTE <[email protected]>
Co-authored-by: hzheng-ledger <[email protected]>
Co-authored-by: Stéphane Prohaszka <[email protected]>
Co-authored-by: lvndry <[email protected]>
Co-authored-by: @greweb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Has changes in LLM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants