Skip to content
This repository has been archived by the owner on Jul 15, 2022. It is now read-only.

LIVE-1341: Remove xrp lib #1762

Merged
merged 4 commits into from
Mar 1, 2022
Merged

Conversation

henri-ly
Copy link
Contributor

Context (issues, jira)

https://ledgerhq.atlassian.net/browse/LIVE-1341

Description / Usage

Replace Websocket of XRP with JSON-RPC should be more stable cause the websocket was not handle properly.

We should probably retest XRP and all these use case properly

Expectations

  • Test coverage: The changes of this PR are covered by test. Unit test were added with mocks when depending on a backend/device.
  • No impact: The changes of this PR have ZERO impact on the userland. Meaning, we can use these changes without modifying LLD/LLM at all. It will be a "noop" and the maintainers will be able to bump it without changing anything.

@henri-ly henri-ly requested a review from a team as a code owner February 28, 2022 13:57
@vercel
Copy link

vercel bot commented Feb 28, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/ledgerhq/ledger-live-common/DhY9dqfbt2CaoLNGXzAaDv3RPpgi
✅ Preview: https://ledger-live-common-git-support-live-1341-remove-9baa76-ledgerhq.vercel.app

@henri-ly
Copy link
Contributor Author

Still have some work on CI to do

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This is largely a rework of the XRP impl, good job!
I'm thinking it will probably need some non-reg in addition to testing the linked issue...

Also maybe that rework is the occasion to split that huge js.ts bridge file into appropriate js-*.ts files. But not really mandatory. 😄

src/families/ripple/bridge/js.ts Show resolved Hide resolved
src/families/ripple/bridge/js.ts Outdated Show resolved Hide resolved
src/families/ripple/bridge/js.ts Outdated Show resolved Hide resolved
@henri-ly
Copy link
Contributor Author

Well I think the issues was related to websocket, since it doesn't open properly so I don't know if I can reproduce it with test right now :/
I'll try to split with js think right now, but will deliver build to QA right now since there's no functional modification by changing this

@gre
Copy link
Contributor

gre commented Mar 1, 2022

I think we need to set up a bot for it and do our own QA first. 🙏

@gre gre changed the base branch from develop to family/ripple March 1, 2022 08:41
@gre
Copy link
Contributor

gre commented Mar 1, 2022

@henrily-ledger @haammar-ledger I have changed the base to be family/ripple which have a XRP run-per-commit on Mere Denis bot. We can merge to it when you are ready.

@gre gre merged commit 5b6f300 into family/ripple Mar 1, 2022
@gre gre deleted the support/LIVE-1341-remove-xrp-lib branch March 1, 2022 09:46
henri-ly added a commit that referenced this pull request Mar 29, 2022
* LIVE-1341: Remove xrp lib

* update .lock

* remove other currency than XRP

* Remove String and use toString + typescript fix
This was referenced May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants