[LIVE-8174] Feature - coin-xrp module #6822
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 5 Ignored Deployments
|
live-github-bot
bot
added
common
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
lambertkevin
force-pushed
the
feature/coin-xrp
branch
3 times, most recently
from
463b5e4
to
08b0b9e
Compare
lambertkevin
force-pushed
the
feature/coin-xrp
branch
from
08b0b9e
to
49707b2
Compare
lambertkevin
force-pushed
the
feature/coin-xrp
branch
3 times, most recently
from
b069291
to
90e3c97
Compare
lambertkevin
force-pushed
the
feature/coin-xrp
branch
4 times, most recently
from
b64e69e
to
67cd3d1
Compare
[Bot] Testing with 'Nitrogen' ✅ 1 txs ❌ 1 txs ($22.82) ⏲ 5min 53s
❌ 1 mutation errorsDetails of the 2 mutationsSpec XRP (4)Portfolio ($22.82) – Details of the 1 currencies
Performance ⏲ 5min 53sTime spent for each spec: (total across mutations)
|
lambertkevin
force-pushed
the
feature/coin-xrp
branch
from
67cd3d1
to
1b1f144
Compare
lvndry
reviewed
May 13, 2024
| import { getServerInfos, parseAPIValue } from "./api"; | ||
| import { NetworkInfo, Transaction } from "./types"; | ||
|
|
||
| // FIXME this could be cleaner |
There was a problem hiding this comment.
Is this comment still relevant ?
There was a problem hiding this comment.
I have no idea, the work of this PR was just to modularize the existing code base and adding a few types, we would need to have a proper look at how the coin integration is done to understand what should/could be done better 🤷♂️
| networkInfo = { | ||
| family: "xrp", | ||
| serverFee, | ||
| baseReserve: new BigNumber(0), // NOT USED. will refactor later. |
There was a problem hiding this comment.
should we delete this property ?
There was a problem hiding this comment.
My understanding for now is that the whole networkInfo is useless, but as I don't want to introduce any breaking change for the scope of this PR, I think we should keep it as is, and do some proper cleaning work as a separate PR
| @@ -16,6 +16,7 @@ describe("getWalletAPITransactionSignFlowInfos", () => { | |||
|
|
|||
| const expectedLiveTx: Partial<Transaction> = { | |||
| ...xrpPlatformTx, | |||
| family: "xrp", | |||
There was a problem hiding this comment.
Maybe we should have somewhere in Live common an object
export const defaultXRPTransaction = {
family: "xrp"
}And then do
const expectedLiveTx: Partial<Transaction> = {
...xrpPlatformTx,
...defaultXRPTransaction,
}That way we can easily extends the default values everywhere its needed
There was a problem hiding this comment.
Don't think it's necessary for now, as this is just a compatibilty layer while we wait for the wallet-api to stop using the "ripple" family string and rather use the xrp one. As soon as it's done, we can remove that
| @@ -1,6 +1,5 @@ | |||
| import type { CryptoCurrency } from "@ledgerhq/types-cryptoassets"; | |||
| import Transport from "@ledgerhq/hw-transport"; | |||
| import ripple from "./ripple"; | |||
| import tron from "./tron"; | |||
| // TODO deprecate this approach | |||
There was a problem hiding this comment.
Just out of curiosity do we have a better approach in mind already ?
There was a problem hiding this comment.
Tbh I discovered that file while doing the migration, and I have no idea why it's here and what's the reason to have it. My guess is that it's just a forgotten tech debt of us trying to have a "generated" like file to have a single entrypoint for all "signTransaction", but that's not the idea anymore, so it's most probably useless now and the last coin-integration (tron) using it should be removed as well. But that's not the scope of this PR 👍
lambertkevin
force-pushed
the
feature/coin-xrp
branch
from
1b1f144
to
f882655
Compare
[Bot] Testing with 'Nitrogen' ✅ 2 txs ($22.29) ⏲ 71.2s
Details of the 2 mutationsSpec XRP (4)Portfolio ($22.29) – Details of the 1 currencies
Performance ⏲ 71.2sTime spent for each spec: (total across mutations)
|
[Bot] Testing with 'Phosphore' ✅ 2 txs ($31.40) ⏲ 76.9s
Details of the 2 mutationsSpec XRP (5)Portfolio ($31.40) – Details of the 1 currencies
Performance ⏲ 76.9sTime spent for each spec: (total across mutations)
|
libs/ledger-live-common/src/families/xrp/bridge.integration.test.ts
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,11 @@ | |||
| /* istanbul ignore file: untested because empty */ | |||
There was a problem hiding this comment.
Why didn't you keep the content of the old cli-transaction file?
There was a problem hiding this comment.
It's just me doing a bad copy/paste of the EVM coin-module... (And I'm wondering now why this one is empty...) 🤦♂️
There was a problem hiding this comment.
Added the file back with some modifications for typing and removal of the lodash dep 👍
lambertkevin
force-pushed
the
feature/coin-xrp
branch
3 times, most recently
from
2c154e4
to
dedd719
Compare
lambertkevin
force-pushed
the
feature/coin-xrp
branch
from
dedd719
to
0bd2889
Compare
[Bot] Testing with 'Nitrogen' ✅ 2 txs ($22.31) ⏲ 71.5s
Details of the 2 mutationsSpec XRP (4)Portfolio ($22.31) – Details of the 1 currencies
Performance ⏲ 71.5sTime spent for each spec: (total across mutations)
|
[Bot] Testing with 'Phosphore' ✅ 1 txs ❌ 1 txs ($31.38) ⏲ 5min 58s
❌ 1 mutation errorsDetails of the 2 mutationsSpec XRP (5)Portfolio ($31.38) – Details of the 1 currencies
Performance ⏲ 5min 58sTime spent for each spec: (total across mutations)
|
gre
previously approved these changes
May 14, 2024
ofreyssinet-ledger
previously approved these changes
May 14, 2024
Justkant
previously approved these changes
May 14, 2024
|
|
||
| const CAN_EDIT_FEES = true; | ||
|
|
||
| const areFeesProvided = (tx: PlatformTransaction): boolean => !!tx.fee; | ||
|
|
||
| const convertToLiveTransaction = (tx: PlatformTransaction): Partial<Transaction> => { | ||
| return tx; | ||
| return { ...tx, family: "xrp" }; |
There was a problem hiding this comment.
Not sure why we need to add family here as I would expect it to already be in the tx
I suppose it's for making sure you always have the correct one
There was a problem hiding this comment.
@Justkant This change is only here because I'm renaming the "ripple" family into "xrp" family, and since I can't control the Wallet API in the same PR as this change, I need to have a compatibility layer for now ensuring that a tx with a "ripple" family from the Wallet API would still work correctly once reaching the live.
Next step is to ask your team to rename the family to xrp as well and to remove this compatibility thingy 👍
There was a problem hiding this comment.
Ok got it makes sense, thx for the details
lambertkevin
dismissed stale reviews from Justkant, ofreyssinet-ledger, and gre
via
d55dea2
lambertkevin
force-pushed
the
feature/coin-xrp
branch
from
0bd2889
to
d55dea2
Compare
lambertkevin
force-pushed
the
feature/coin-xrp
branch
from
d55dea2
to
9ddfbe1
Compare
Justkant
approved these changes
May 15, 2024
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✅ Checklist
npx changesetwas attached.📝 Description
Creation of the
coin-xrpmodule, with support for better spendable balance (monitoring base reserve & trustlines), better typing, signer injection & removal of unmaintained libs likeripple-bs58check❓ Context
🧐 Checklist for the PR Reviewers