XRP send functionality implementation #555
Conversation
WalkthroughRemoves prior signedPayloads validation and multi-path submission for XRPL; uses a single submission flow that extracts tx_blob, submits via provider, and handles results by transaction-result-prefix (tesSUCCESS, tec*, tem*, ter*, tef*), with adjusted error messaging and catch behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/multichain/routines/executors/pay.ts (1)
203-227: Missing XRPL connection cleanup.The XRPL instance establishes a WebSocket connection at line 204 but is never disconnected after the transaction completes. This can lead to resource leaks, especially if this function is called repeatedly. Consider adding cleanup in a
finallyblock.🔎 Suggested fix with cleanup
try { const signedTx = operation.task.signedPayloads[0] // ... existing transaction logic ... } catch (error) { console.log("[XMScript Parser] Ripple Pay: error:", error) return { result: "error", error: error instanceof Error ? error.message : String(error), } + } finally { + try { + await xrplInstance.disconnect() + } catch (disconnectError) { + console.log("[XMScript Parser] Ripple Pay: disconnect error:", disconnectError) + } }Note: Verify that
xrplInstanceexposes adisconnect()method. The xrpl.js library typically provides this on the Client class.
🧹 Nitpick comments (3)
src/features/multichain/routines/executors/pay.ts (3)
230-237: Add defensive check fortx_blobproperty.If
signedPayloads[0]doesn't contain atx_blobproperty (e.g., different payload format, corrupted data), this will passundefinedtosubmitAndWait, potentially causing an unclear error. Consider validating the payload structure before submission.🔎 Suggested defensive check
const signedTx = operation.task.signedPayloads[0] + + if (!signedTx?.tx_blob) { + return { + result: "error", + error: "Invalid signed payload: missing tx_blob", + } + } // Submit transaction and wait for validation const res = await xrplInstance.provider.submitAndWait(signedTx.tx_blob)
292-297: Handle undefinedtxResultmore gracefully.If both
meta?.TransactionResultandengine_resultare undefined, the error message will read "Unknown transaction result: undefined". Consider providing a more informative message for this edge case.🔎 Suggested improvement
return { result: "error", - error: `Unknown transaction result: ${txResult} - ${resultMessage}`, + error: txResult + ? `Unknown transaction result: ${txResult} - ${resultMessage}` + : `Transaction result unavailable in response`, hash: txHash, extra: { code: txResult, validated: res.result.validated } }
298-304: Consider safer error stringification.The
error.toString()call could fail iferrorisnullorundefined. A safer pattern handles edge cases:🔎 Suggested improvement
} catch (error) { console.log("[XMScript Parser] Ripple Pay: error:", error) return { result: "error", - error: error.toString(), + error: error instanceof Error ? error.message : String(error), } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/features/multichain/routines/executors/pay.ts
🔇 Additional comments (2)
src/features/multichain/routines/executors/pay.ts (2)
239-245: LGTM!Correctly treating only
tesSUCCESSas a successful transaction is the right approach for XRPL.
247-290: No action needed. TheTransactionResponseinterface (fromsdk/localsdk/multichain/types/multichain.ts) explicitly supports theextra?: { [key: string]: any }field with documentation stating it's for "Extra optional data about the transaction extracted from the response. Currently available for XRPL transactions." The return statements in lines 247-290 are fully compatible with the type definition.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/features/multichain/routines/executors/pay.ts (1)
320-325: Add type guard for safer error handling.Calling
error.toString()directly on an untyped caught exception can fail iferrorisnullorundefined. Consider using a type guard orString(error)for robustness.♻️ Suggested fix
} catch (error) { console.log("[XMScript Parser] Ripple Pay: error:", error) return { result: "error", - error: error.toString(), + error: error instanceof Error ? error.message : String(error), } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/features/multichain/routines/executors/pay.ts
🔇 Additional comments (2)
src/features/multichain/routines/executors/pay.ts (2)
256-259: LGTM!The result extraction logic handles both validated transaction responses (
meta.TransactionResult) and pending/initial responses (engine_result) correctly. The optional chaining and fallback pattern is appropriate for XRPL's varying response structures.
269-319: Well-structured XRPL error categorization.The prefix-based error handling provides clear, actionable error messages for each XRPL result code category. Including
codeandvalidatedin theextrafield enables callers to implement custom handling logic if needed. The inline comments documenting each category are helpful.
| // ter* codes: Provisional/retryable result (not final) | ||
| // Example: terQUEUED (transaction queued for future ledger) | ||
| if (txResult?.startsWith('ter')) { | ||
| return { | ||
| result: "error", | ||
| error: `${xrplErrorMessages[errorPrefix]}: ${txResult} - ${resultMessage}`, | ||
| error: `Transaction provisional/queued: ${txResult} - ${resultMessage}`, | ||
| hash: txHash, | ||
| extra: { code: txResult, validated: res.result.validated }, | ||
| extra: { code: txResult, validated: res.result.validated } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider differentiating ter* provisional results from actual errors.
The comment correctly notes that ter* codes are "provisional/retryable" and "not final," yet the code returns result: "error". For terQUEUED or similar codes, the transaction may still succeed in a future ledger. Treating this as an error could mislead callers into thinking the transaction definitively failed.
Consider returning a distinct status (e.g., result: "pending") or documenting that callers should handle ter* codes specially.
PR Type
Enhancement
Description
Refactored XRP transaction error handling with explicit prefix checks
Removed complex payload format detection logic
Improved error messages with specific XRPL result code categories
Simplified transaction result extraction and validation logic
Diagram Walkthrough
File Walkthrough
pay.ts
Simplify XRP error handling with explicit code checkssrc/features/multichain/routines/executors/pay.ts
TransactionResultorengine_resultstartsWith()checks for each XRPL code categoryextrafield containingtransaction code and validation status
error.toString()insteadof conditional message extraction
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.