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

✨ (device-core): add new reinstallConfiguration consent use case #7771

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

valpinkman
Copy link
Member

@valpinkman valpinkman commented Sep 6, 2024

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • new PINNotSet in @ledgerhq/errors + error entry in translation file for LLM / LLD
    • new consent usecase reinstallConfiguration in @ledgerhq/device-core
    • implement new usecase in CLI

📝 Description

Implement new APDU to give consent after an OS update to reinstall the whole config of the device.
This allows us to skip all the user consent for each step (language pack, custom locksceen, apps, and now apps data) and only require one approval before the whole restore flow.

❓ 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 Sep 6, 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 Sep 9, 2024 2:15pm
4 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Sep 9, 2024 2:15pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Sep 9, 2024 2:15pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Sep 9, 2024 2:15pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Sep 9, 2024 2:15pm

Copy link
Contributor

Choose a reason for hiding this comment

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

nice 👍

* @param transport - The transport object used to communicate with the device.
* @returns A promise that resolves when the consent is granted.
*/
export async function reinstallConfiguration(
Copy link
Contributor

Choose a reason for hiding this comment

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

[COULD] I know in the firmware doc it's called REINSTALL_CONFIG but in my opinion it's better to have something more explicit that describes what it actually does, like reinstallConfigurationInit or requestReinstallConfigurationConsent

Copy link
Member Author

Choose a reason for hiding this comment

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

will do

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.

lgtm 👍

@valpinkman valpinkman merged commit 7f53095 into develop Sep 10, 2024
52 of 57 checks passed
@valpinkman valpinkman deleted the feat/new-super-consent branch September 10, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs mobile Has changes in LLM translations Translation files have been touched
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants