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

chore: remove unused Account#endpointConfig field #6521

Merged
merged 1 commit into from
Mar 26, 2024
Merged

chore: remove unused Account#endpointConfig field #6521

merged 1 commit into from
Mar 26, 2024

Conversation

gre
Copy link
Contributor

@gre gre commented Mar 22, 2024

βœ… Checklist

  • npx changeset was attached. (no need)
  • Covered by automatic tests. this was not used, it's covered by ability to still compile our libs.
  • Impact of the changes: no impact

πŸ“ Description

the .endpointConfig on Account type is unused since around 2020, we used it in the past to workaround problem on the XRP synchronisations that had unstable backends back then and we used to have a way accessible in the UI to configure the server. it's long gone so we can remove this unused tech debt.

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

@gre gre requested review from a team as code owners March 22, 2024 13:22
Copy link

vercel bot commented Mar 22, 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 Mar 22, 2024 1:25pm
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Mar 22, 2024 1:25pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Mar 22, 2024 1:25pm
native-ui-storybook ⬜️ Ignored (Inspect) Mar 22, 2024 1:25pm
react-ui-storybook ⬜️ Ignored (Inspect) Mar 22, 2024 1:25pm

@live-github-bot live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs translations Translation files have been touched labels Mar 22, 2024
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.

@gre I don't know if we want to clean up in libs/ledger-live-common/src/families/ripple/api/index.ts @getServerInfo usage of endpointConfig, is it intentional to keep it?

@gre
Copy link
Contributor Author

gre commented Mar 25, 2024

@KVNLS indeed the function still allows to configure the endpoint, but I did not want to be too intrusively touching the coin implementation in these sunset, only disconnecting them from the UI. because maybe @LedgerHQ/live-blockchain-support will have better ways to change this in future too (like connect it to something else like an env or a live-config dynamics')

Copy link
Contributor

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

@gre gre merged commit b9dfd66 into develop Mar 26, 2024
59 checks passed
@gre gre deleted the live-11771 branch March 26, 2024 16:11
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 mobile Has changes in LLM translations Translation files have been touched
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants