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

LLD: Fully sunset "internal commands" + remove tech debt & dead code #2859

Merged
merged 6 commits into from
Mar 16, 2023

Conversation

gre
Copy link
Contributor

@gre gre commented Mar 6, 2023

📝 Description

This PR ships another step in simplifying main/internal scripts and making internal process even more minimal (sunset of commands, only remains the transport).

Clean up related to internal process

  • all commands are sunset. internal process is now exclusively used for hardware wallet transport to HID. (We will later look at possibility to do this in a web worker)
    • transportListen was added in the transport channel.
    • all the renderer usage of commands has been moved to using the JS code directly. this removes a lot of remaining serialisation/deserialisation usage.
  • logger is entirely moving to renderer.
  • serialization.ts is entirely removed because it is unused since Move to coin execution on renderer side #2721
    • AccountBridge#applyReconciliation is therefore dropped.
  • main.bundle.js was greatly optimized. Moving from 20MB to 1MB. This should optimize the app load time.
    • This was made possible by moving more logic to renderer side. Notably the logger logic.
    • add a test that guarantee we stay < 5mb in future.
    • renderer.bundle.js on the other side, is growing a bit (49.2 -> 53.7 mb), taking a bit more of the work of internal commands. managed to reduce a bit the bundle with the other code clean up documented here. but this will be a major focus in a near future.

introduce Web Workers

  • @elbywan added the possibility to create web worker scripts.
  • secp256k1 Bitcoin elliptic curve is executed in a web worker (publicKeyTweakAdd). with some parallel web worker to allow even faster parallel derivations.

general libraries and other code clean up

  • minor Electron upgrade 23.1.0 -> 23.1.3 to benefit latest chromium.
  • remove 16 libraries that weren't even used!
  • remove the HTTP 404 fetch effect on LLD which was the only direct dependent of axios + also was useless. we will schedule bigger rework of that remote config system
  • remove EXPERIMENTAL_WS_EXPORT which was depending on "ip"
  • remove HSMStatusBanner system which was never used
  • remove EXPERIMENTAL_MARKET_INDICATOR_SETTINGS

❓ Context

  • Impacted projects: LLD
  • Linked resource(s): LIVE-5697 + LIVE-5698

✅ Checklist

  • Test coverage
  • Atomic delivery
  • No breaking changes

📸 Demo

no demo. negative coding 😆

🚀 Expectations to reach

Please make sure you follow these Important Steps.

Pull Requests must pass the CI and be internally validated in order to be merged.

@changeset-bot
Copy link

changeset-bot bot commented Mar 6, 2023

🦋 Changeset detected

Latest commit: 3ee4e3b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
ledger-live-desktop Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Mar 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
live-common-tools ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 16, 2023 at 9:16AM (UTC)
3 Ignored Deployments
Name Status Preview Comments Updated
ledger-live-github-bot ⬜️ Ignored (Inspect) Mar 16, 2023 at 9:16AM (UTC)
native-ui-storybook ⬜️ Ignored (Inspect) Mar 16, 2023 at 9:16AM (UTC)
react-ui-storybook ⬜️ Ignored (Inspect) Mar 16, 2023 at 9:16AM (UTC)

@github-actions github-actions bot added common Has changes in live-common desktop Has changes in LLD translations Translation files have been touched labels Mar 6, 2023
@gre gre force-pushed the support/sunset-internal-commands branch from d73c0f4 to a3e1342 Compare March 6, 2023 12:16
@gre gre force-pushed the support/sunset-internal-commands branch from a3e1342 to 977f492 Compare March 6, 2023 17:21
@gre gre force-pushed the support/sunset-internal-commands branch from 977f492 to 25f5138 Compare March 6, 2023 17:53
@github-actions github-actions bot added the ledgerjs Has changes in the ledgerjs open source libs label Mar 6, 2023
@gre gre force-pushed the support/sunset-internal-commands branch from 25f5138 to 1ae7fe7 Compare March 7, 2023 09:48
@gre gre force-pushed the support/sunset-internal-commands branch from 1ae7fe7 to 999ee29 Compare March 7, 2023 13:26
@gre gre force-pushed the support/sunset-internal-commands branch from 999ee29 to 1b3db3a Compare March 7, 2023 15:09
@gre gre force-pushed the support/sunset-internal-commands branch from 1b3db3a to 28f6045 Compare March 7, 2023 15:19
@gre gre force-pushed the support/sunset-internal-commands branch from 28f6045 to 7e01d1b Compare March 7, 2023 17:01
@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Patch coverage has no change and project coverage change: -24.58 ⚠️

Comparison is base (0ca89a8) 89.91% compared to head (d12df9b) 65.34%.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #2859       +/-   ##
============================================
- Coverage    89.91%   65.34%   -24.58%     
============================================
  Files           26       85       +59     
  Lines         1289     5041     +3752     
  Branches       259      973      +714     
============================================
+ Hits          1159     3294     +2135     
- Misses         125     1635     +1510     
- Partials         5      112      +107     
Flag Coverage Δ
test 65.34% <ø> (-24.58%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 59 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Small comment/question, I still need to look at the rest of the files

Copy link
Member

@hedi-edelbloute hedi-edelbloute left a comment

Choose a reason for hiding this comment

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

LGTM

Looks like it's just removing applyReconciliation on our part

Copy link
Contributor

@mcayuelas-ledger mcayuelas-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

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.

some comments/questions but LGTM

@tatijana

This comment was marked as spam.

@tatijana

This comment was marked as spam.

Copy link
Contributor

@sarneijim sarneijim left a comment

Choose a reason for hiding this comment

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

Amazing!!!!!!
Have we implemented something to prevent import in the "global" way? If not, could we implemented it?
I think no-restricted-import could help...
import-js/eslint-plugin-import#810

Alt Text

@gre
Copy link
Contributor Author

gre commented Mar 16, 2023

@sarneijim i'm going to note this as part of the bundle size optim we need to schedule on

Copy link
Contributor

@juan-cortes juan-cortes left a comment

Choose a reason for hiding this comment

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

lgtm

@gre gre merged commit dd642a4 into develop Mar 16, 2023
@gre gre deleted the support/sunset-internal-commands branch March 16, 2023 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Has changes in live-common desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants