-
Notifications
You must be signed in to change notification settings - Fork 318
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
LIVE-13528 Chore/remove pivx code #7563
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
4 Skipped Deployments
|
593815a
to
da9f9d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to update the README for wallet-api too now
@@ -2088,42 +2088,6 @@ export const cryptocurrenciesById: Record<CryptoCurrencyId, CryptoCurrency> = { | |||
}, | |||
], | |||
}, | |||
pivx: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove the pivx here?
AFAIK, we remove pivx support from Ledger-live. but the pivx nano app is still there.
i.e. we should give users a way to download the pivx nano app from Ledger Live manager.
@@ -81,7 +81,6 @@ export type CryptoCurrencyId = | |||
| "particl" | |||
| "persistence" | |||
| "pirl" | |||
| "pivx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering whether we should remove pivx in the ledgerjs package. because this package is used by other projects outside Ledger. This is why we see some LL unsupported currencies here. (e.g. monero)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI they were removed for peercoin, stealthcoin, vertcoin, viacoin
https://github.com/LedgerHQ/ledger-live/pull/7138/files#diff-8d5212f656c0023f496e2e16d624bfc345c426aa7f881b7b11b8dea43cf047ff
Was this a mistake also and do you think we should put them back in? Didn't seem to cause much trouble as no one complained but Im not 100% sure. In any case I'll put pivx back up
You make a good point about monero and the package usage outside of ledger live.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is true that peercoin, stealthcoin, vertcoin, viacoin have done the same thing. Maybe we can sync with product to make a decision.
Moreover, we can remove the "libs/ledger-live-common/explorers-config.md" file completely after my discussion with @gre |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look at my comments
@hzheng-ledger Do you have a link to the conv? Im assuming it's a legacy file that was automatically generated in an old process. |
0bdb30e
5f7707a
to
65a49b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
β Checklist
npx changeset
was attached.π Description
Replace this text by a clear and concise description of what this pull request is about and why it is needed. Be sure to explain the problem you're addressing and the solution you're proposing.
For libraries, you can add a code sample of how to use it.
For bugfixes, you can explain the previous behavior and how it was fixed.
In case of visual features, please attach screenshots or video recordings to demonstrate the changes.
β Context
π§ Checklist for the PR Reviewers