feat(blockchain-link): migrate ripple-lib to xrpl.js #17417
Conversation
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is network access?This module accesses the network. Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
|
🚀 Expo preview is ready!
|
izmy
force-pushed
the
feat/migrate-to-xrpl
branch
from
9f5e4dc to
6bf7fc9
Compare
WalkthroughThe changes update type definitions and library dependencies across multiple packages. In the blockchain-link-types package, the ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
packages/blockchain-link/src/workers/ripple/index.ts (9)
55-64: Consider adding error handling for unreachable servers.You're fetching server info via
client.request({ command: 'server_info' })without a try/catch. If the server is offline or misconfigured, you’ll rely on a higher-level error path. That’s fine as long as you’re confident the outer catch blocks handle it gracefully.
61-64: Double-check global state usage.Storing base and owner reserve in the global
RESERVEobject might work in a single-tenant or single-connection scenario, but in multi-tenant or parallel requests, concurrency issues can arise. If you expect multiple concurrent calls, consider storing these values in local state or context instead.
77-89: Improved approach to fetch mempool data.Using
queue: truewith theaccount_infocommand for current ledger data is a valid solution for mempool detection. Just ensure you handle cases where the queue might be absent or incomplete for certain ledger indexes.
186-190: Validate transaction fields inflatMap.Skipping items where
raw.tx_jsonis null is sensible. If partial transaction data might appear in any edge cases, consider logging or tracking them to avoid losing potentially important data.
203-214: Confirm partial transaction data handling.For
getTransaction, confirm thatrawTx.result.metaorrawTx.result.tx_jsonis always present on success. If the XRPL server returns incomplete data for certain in-flight transactions, handle it similarly toaccount_tx.
236-245: Clarify fee selection approach.Relying on
fee.result.drops.median_feeis valid, but be sure this matches your desired fee policy. Some use average, some minimal. The XRPL docs recommend the minimum open ledger fee in certain contexts. Consider documenting your choice.
269-302: Skipping non-Payment transactions is consistent but confirm completeness.If you intend to monitor only Payment transactions, this is correct. However, XRPL includes other transaction types (e.g., OfferCreate, EscrowCreate). Confirm no business logic depends on these.
317-319: Ensure robust error handling forsubscribecall.The approach is correct but lacks explicit error handling. Consider a try/catch or a fallback mechanism to handle edge cases like a node not supporting subscription or being unreachable.
333-345: Subscription for addresses mirrors the same pattern.Everything looks aligned with
subscribeAddresses. Confirm that adding a subsequent subscription does not overwrite or conflict with earlier ones, especially in multi-user or multi-address scenarios.packages/blockchain-link-utils/src/ripple.ts (2)
21-27: Returning 0 for invalid timestamps may cause confusion.While returning 0 effectively signals “no valid timestamp,” it could be mistaken for a valid early epoch. Consider using distinct handling (e.g., throwing, logging, or returning a clearly invalid placeholder) if you need stronger guarantees.
50-51: ts-expect-error usage could mask missing type definitions.While it unblocks compilation, consider refining the XRPL type definitions or actual usage to remove the need for
ts-expect-error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
packages/blockchain-link-types/src/common.ts(1 hunks)packages/blockchain-link-utils/package.json(1 hunks)packages/blockchain-link-utils/src/ripple.ts(1 hunks)packages/blockchain-link/package.json(1 hunks)packages/blockchain-link/src/workers/ripple/index.ts(15 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (32)
packages/blockchain-link-utils/package.json (1)
25-26:❓ Verification inconclusive
Add a verification step for the newly introduced
xrpldependency.You've added
"xrpl": "^4.2.0"alongside"@trezor/utils". While this aligns with the migration away from the deprecatedripple-lib, consider verifying that the specified version is compatible with other packages in the repository and that any known security advisories have been addressed.
🏁 Script executed:
#!/bin/bash # Description: Check for interplay between xrpl@^4.2.0 and other dependencies. # 1) Find references to ripple-lib to ensure no leftover references remain. # 2) List direct dependencies referencing xrpl to check for potential conflicts. fd ripple-lib | xargs cat rg xrpl -A 5Length of output: 9833
Verify XRPL Dependency Integration and Ripple-lib Residuals
The upgrade to
"xrpl": "^4.2.0"appears consistent across our codebase (e.g. in bothpackages/blockchain-linkandpackages/blockchain-link-utils). The yarn lock entries confirm that xrpl is resolved to version 4.2.0. However, the verification script still uncovered references toripple-libin the node_modules (including patch-like diffs) which suggests that some transitive dependencies may still bring in or rely onripple-lib. Please ensure that:
- Any residual usage of
ripple-libis intentional and does not conflict with the new xrpl-based implementation.- The xrpl version 4.2.0 is fully compatible with all dependent packages, especially if some parts of the code or their dependencies have yet to complete the migration.
- No known security vulnerabilities affect xrpl v4.2.0 or the transitive dependencies related to
ripple-lib.Keep a close eye on integration tests to catch any incompatibilities early.
packages/blockchain-link/package.json (1)
87-88:✅ Verification successful
Confirm the removal of
ripple-libin tandem with the newly introducedxrpl.You've replaced
ripple-libwithxrplhere as well, which is consistent with the overall XRPL migration goal. Ensure there are no leftover imports or references toripple-libin the codebase.
🏁 Script executed:
#!/bin/bash # Description: Verify complete removal of ripple-lib references across the repository. fd ripple-lib | xargs catLength of output: 2574
Ripple-lib has been successfully removed from our source code.
The verification shows that the only occurrences of “ripple-lib” appear within thenode_modulesfolder, which are not part of our own code. The dependency replacement inpackages/blockchain-link/package.jsonnow correctly references only"xrpl".packages/blockchain-link-types/src/common.ts (1)
64-65: Handle optional properties in downstream usage.Changing
blockHeightandblockHashtonumber | undefinedandstring | undefinedrespectively removes the guarantee that these fields are always present. Ensure all consumers ofServerInfoproperly handle the potentialundefinedvalues (e.g., via nullish checks or default values).packages/blockchain-link/src/workers/ripple/index.ts (17)
1-9: Top-level imports look good.Replacing
ripple-libwithxrplimports is consistent with the PR’s objective. No issues spotted here.
11-16: Synchronized type imports align with the new XRPL client usage.You're correctly importing types like
AccountInfoandResponsefrom@trezor/blockchain-link-types. Ensure all references toMessageTypesare fully migrated as well.
21-21: New utility import is consistent.Importing
getSuiteVersionfrom@trezor/env-utilshelps set a user-agent header. Looks fine.
27-27: Context adaptation.Switching
ContextTypetoClientis a necessary step in fully migrating toxrpl. Good move.
37-38: Ensure robust error transformation.Replacing
RippleErrorwithRippledErroris correct, but confirm thaterror.datacan safely be cast to{ error_message: string }. If some variants ofRippledErrorlackdataor contain a different structure, consider additional type checks.
116-146: Catch and handleactNotFoundgracefully.The catch logic for empty accounts is well-implemented. Make sure the user experience is consistent with your business logic—returning an account with
balance = '0'andempty = truecan help front-end clients display the correct status for new or non-existent accounts.
153-167: Graceful fallbacks for mempool retrieval failures.Skipping errors when fetching mempool data avoids breaking the entire flow. This is reasonable when certain mainnet or testnet configurations temporarily disallow mempool queries. Just remember to trace if repeated failures occur and alert users.
175-184: Leverageapi_version: 2with caution.The
account_txcommand usingapi_version: 2adds new fields to the response (liketx_json). Verify this remains stable long term and test older XRPL server versions that may not support this setting.
197-197: Safeguard type-casting forresponse.result.marker.Casting
response.result.markertoAccountInfoParams['marker']is fine if you’re certain they match. Ifmarkercan include different shapes in future versions, consider a more explicit check or fallback to handle unexpected data.
222-233: Restrict totesSUCCESScarefully.The
info.status === 'tesSUCCESS'check is correct for a successful transaction, but other success-like codes may exist (e.g.,terQUEUED). Decide if ignoring them is desired or if you should provide a more nuanced approach.
255-264: Block notifications look good.Reporting
blockHeightandblockHashon new ledgers is straightforward. This aligns with existing block subscription patterns in the codebase.
313-316: Subscription totransactionevents.Subscribing to the new XRPL event type is in line with the library’s approach. Be mindful that unsubscribing properly remains necessary to avoid memory leaks on reconnection or teardown.
353-354: Register ledger listener for block updates.Likewise, the block subscription approach is consistent with XRPL best practices. Fine as is.
461-461: Class definition updated toBaseWorker<Client>.This finalizes the pivot from the legacy
RippleAPIto the new XRPLClient. Great job on the transition.
474-476:isConnectedtype guard is correct.A type predicate ensures
client.isConnected()is used safely. Nicely done.
478-511: New XRPL client connection logic.Using
client.request({ command: 'subscribe', streams: ['ledger'] })is a solid approach. Make sure you handle disconnections gracefully, which yourdisconnectedevent listener does. Good work.
531-531: Helpful custom error responses.Transforming errors before sending them to the caller is a best practice. Your usage of
transformError(error)is well-structured.packages/blockchain-link-utils/src/ripple.ts (12)
1-3: Good job updating import statements to reference xrpl and blockchain-link-types.These changes align with the migration away from the deprecated ripple-lib.
5-5: Improved type safety for transformServerInfo.Switching from
anyto theServerInfoResponsetype correctly leverages XRPL’s definitions, reducing the risk of runtime errors.
10-13: Consider fallback behavior if validated_ledger is undefined.While optional chaining prevents runtime errors, you may want to confirm if any fallback or error-handling is needed when the ledger info is missing. This helps ensure consistent data across different XRPL nodes or edge cases.
16-18: Informative XRPL timestamp documentation comments.Providing references to official XRPL docs is helpful for future maintainers. This practice improves clarity and ensures correct offset usage.
30-32: Signature change for transformTransaction looks appropriate.The new parameter set cleanly expresses the separate concerns of transaction hash, JSON data, and metadata. This pattern aligns well with XRPL structures and clarifies usage.
36-36: DeliverMax type check is valid.This check helps distinguish token transactions from standard ones. No immediate concerns.
41-42: Good reference to XRPL transaction results.Commenting the “tesSUCCESS” logic clarifies XRPL-specific semantics and benefits future maintainers.
44-44: Verify condition flow for isTokenTransaction and descriptor checks.Ensure that non-Payment transactions or token-based payments are correctly handled here. You might want to add tests covering these flow branches to confirm they work as expected.
47-47: Conditional assignment for sent vs. recv is straightforward.Setting
txTypebased on the descriptor comparison is clear.
53-54: Conditional amount assignment based on token transaction.This logic properly refrains from setting
amountfor token transactions.
56-57: DestinationTag reference is acceptable.Reading
DestinationTagdirectly fromtx_jsonis aligned with XRPL transaction structures.
61-61: Empty string fallback for txid might cause ambiguity.If
hashis undefined, producing an empty string could hide transaction context. Validate whether an empty txid is acceptable or if you should propagate an error or use a distinct placeholder.
| const unsubscribeBlock = async ({ state, connect }: Context) => { | ||
| if (!state.getSubscription('ledger')) return; | ||
| if (!state.getSubscription('ledgerClos')) return; | ||
| const api = await connect(); | ||
| api.removeAllListeners('ledger'); | ||
| api.removeAllListeners('ledgerClosed'); | ||
| state.removeSubscription('ledger'); |
There was a problem hiding this comment.
Fix mismatch in block unsubscription keys.
You add the subscription under 'ledger', but here you are checking 'ledgerClos'. This mismatch prevents unsubscription from happening. Update the key to 'ledger' or rename the subscription key to match.
Proposed fix:
- if (!state.getSubscription('ledgerClos')) return;
+ if (!state.getSubscription('ledger')) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const unsubscribeBlock = async ({ state, connect }: Context) => { | |
| if (!state.getSubscription('ledger')) return; | |
| if (!state.getSubscription('ledgerClos')) return; | |
| const api = await connect(); | |
| api.removeAllListeners('ledger'); | |
| api.removeAllListeners('ledgerClosed'); | |
| state.removeSubscription('ledger'); | |
| const unsubscribeBlock = async ({ state, connect }: Context) => { | |
| if (!state.getSubscription('ledger')) return; | |
| const api = await connect(); | |
| api.removeAllListeners('ledgerClosed'); | |
| state.removeSubscription('ledger'); | |
| } |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| fee: tx_json.Fee ?? '0', | ||
| blockTime: getUnixTimestamp(tx_json.date), | ||
| blockHeight: tx_json.ledger_index, | ||
| blockHash: hash, |
There was a problem hiding this comment.
Potential mismatch between transaction hash and blockHash property.
Currently, blockHash is set to hash, which appears to be the transaction ID rather than the ledger hash. If you intend to store the ledger’s block hash, consider retrieving it from tx_json or meta if available.
Apply this diff to use the ledger hash instead of the transaction hash (assuming tx_json.ledger_hash is the correct property):
- blockHash: hash,
+ blockHash: tx_json.ledger_hash ?? '',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fee: tx_json.Fee ?? '0', | |
| blockTime: getUnixTimestamp(tx_json.date), | |
| blockHeight: tx_json.ledger_index, | |
| blockHash: hash, | |
| fee: tx_json.Fee ?? '0', | |
| blockTime: getUnixTimestamp(tx_json.date), | |
| blockHeight: tx_json.ledger_index, | |
| blockHash: tx_json.ledger_hash ?? '', |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| RESERVE.BASE = xrpToDrops(response.result.info.validated_ledger.reserve_base_xrp); | ||
| RESERVE.OWNER = xrpToDrops(response.result.info.validated_ledger.reserve_inc_xrp); |
Description
ripple-libhas been replaced byxrpl.jsas the former is deprecatedRelated Issue
Resolve #6980