Skip to content

✨ (dmk) [DSDK-1074]: Add language package management#1382

Open
pvautherin-ledger wants to merge 21 commits intodevelopfrom
feat/dsdk-1074-language-package-management
Open

✨ (dmk) [DSDK-1074]: Add language package management#1382
pvautherin-ledger wants to merge 21 commits intodevelopfrom
feat/dsdk-1074-language-package-management

Conversation

@pvautherin-ledger
Copy link
Copy Markdown
Contributor

@pvautherin-ledger pvautherin-ledger commented Mar 23, 2026

📝 New commands ✨

  • List Language Package command : already exists -> added to the sample App ✅
  • Delete : Created ✅
  • Create/Load/Commit command : uses a bulk APDU fetched Backend-side -> to implement on a device-action ↩️

New Device Action 🎉

  • InstallLanguagePackageDeviceAction

❓ Context

✅ Checklist

Pull Requests must pass CI checks and undergo code review. Set the PR as Draft if it is not yet ready for review.


🧐 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.

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 23, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
device-sdk-ts-sample Ready Ready Preview, Comment Apr 17, 2026 8:55am
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
doc-device-management-kit Ignored Ignored Apr 17, 2026 8:55am

Request Review

@pvautherin-ledger pvautherin-ledger force-pushed the feat/dsdk-1074-language-package-management branch from 4fd753d to 735aaca Compare March 23, 2026 16:31
@pvautherin-ledger pvautherin-ledger changed the title ✨ (feat) [DSK-1074] Add Language package management ✨ (feat) [DSK-1074]: Add Language package management Mar 24, 2026
@pvautherin-ledger pvautherin-ledger force-pushed the feat/dsdk-1074-language-package-management branch from 735aaca to 306576f Compare March 24, 2026 15:21
@pvautherin-ledger pvautherin-ledger changed the title ✨ (feat) [DSK-1074]: Add Language package management ✨ (dmk) [DSK-1074]: Add Language package management Mar 24, 2026
@pvautherin-ledger pvautherin-ledger force-pushed the feat/dsdk-1074-language-package-management branch from 306576f to 26b4375 Compare March 24, 2026 15:39
@LedgerHQ LedgerHQ deleted a comment from github-actions bot Mar 24, 2026
@pvautherin-ledger pvautherin-ledger changed the title ✨ (dmk) [DSK-1074]: Add Language package management ✨ (dmk) [DSDK-1074]: Add language package management Mar 24, 2026
@LedgerHQ LedgerHQ deleted a comment from github-actions bot Mar 24, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 24, 2026

Danger Check Results

Messages

Danger: All checks passed successfully! 🎉

Generated by 🚫 dangerJS against a909faa

@pvautherin-ledger pvautherin-ledger force-pushed the feat/dsdk-1074-language-package-management branch from 95e0f02 to 45b7bcf Compare March 26, 2026 07:54
@pvautherin-ledger pvautherin-ledger force-pushed the feat/dsdk-1074-language-package-management branch from b52e273 to 92610fd Compare March 26, 2026 09:24
@pvautherin-ledger pvautherin-ledger force-pushed the feat/dsdk-1074-language-package-management branch from 4b301c4 to c3c26a5 Compare April 3, 2026 08:54
@pvautherin-ledger pvautherin-ledger force-pushed the feat/dsdk-1074-language-package-management branch from c3c26a5 to e9e8e33 Compare April 3, 2026 09:39
@pvautherin-ledger pvautherin-ledger force-pushed the feat/dsdk-1074-language-package-management branch from e9e8e33 to dd4d8f3 Compare April 3, 2026 09:50
@pvautherin-ledger pvautherin-ledger force-pushed the feat/dsdk-1074-language-package-management branch from dd4d8f3 to 08879fd Compare April 9, 2026 15:02
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
22.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

const execute = async () => {
const { data: rawApdus } = await axios.get<string>(
this.args.apduInstallUrl,
);
Copy link
Copy Markdown
Contributor

@OlivierFreyssinet OlivierFreyssinet Apr 17, 2026

Choose a reason for hiding this comment

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

[SHOULD] If this throws, it should be wrapped in a InstallLanguagePackageTaskError, especially because the DeviceAction makes this type assertion, even though it's incorrect.


execute().then(
() => subscriber.complete(),
(err) => subscriber.error(err),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[SHOULD] I think we should avoid subscriber.error because then errors are of type unknown. What we could do instead is always emit those errors in the observable directly (it becomes Observable<ProgressEvent|ErrorEvent>), and like this it does not force the DeviceAction to assert unknown as *DAError

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[COULD] Overall I feel like the task could be much easier to follow if it was rewritten as an RxJS pipeline (with operators like defer, from, switchMap, catchError etc. instead of new Observable(() => {...}) wrapping the async logic in an inner function, throwing inside it and catching in then etc.

I think it's worth exploring the alternative to see if it would be a bit cleaner, but it's not a required change if you already tackle the [SHOULD] comments.

Copy link
Copy Markdown
Contributor

@OlivierFreyssinet OlivierFreyssinet left a comment

Choose a reason for hiding this comment

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

Nice 👍
Just some things to rework regarding error flow in the install task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants