feat(ripple): dedicated XRP Destination Tag field (+ X-addr autofill, RequireDest gate) - #5247
Merged
Merged
Conversation
… RequireDest gate) Adds first-class XRPL destination-tag support to the XRP send flow, mirroring the iOS combo design (vultisig-ios#4749). - Sync commondata submodule to include RippleSpecific.destination_tag; carry the tag in that first-class proto field (independent of the free-text memo). - Send form: collapsible "Destination Tag" field shown below Memo for XRP, numeric, optional; auto-expands + locks when an X-address paste autofills it. - Hand-rolled XLS-5d X-address codec (WalletCore exposes none on JNI): paste an X-address to normalize to the classic r-address and autofill the embedded tag. - RequireDest gate: block a tagless send to an account with lsfRequireDestTag (fails closed on lookup error, matching the reserve check). - RippleHelper prefers the tag field; supports tag+memo via rawJSON DestinationTag+Memos; legacy numeric-memo and swap-memo paths preserved. - Verify (initiator + join-device) shows a Destination Tag row from the proto field, alongside the Memo row. - Localized new strings across all 10 locales. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds dedicated XRP destination-tag state, X-address decoding and autofill, RequireDestTag validation, Ripple payload and payment propagation, fee simplification, localized UI strings, and separate destination-tag display during transaction verification. ChangesXRP destination tag flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SendForm
participant AddressManager
participant DefaultSendStrategy
participant RippleApi
participant RippleHelpter
User->>SendForm: Paste X-address or enter tag
SendForm->>AddressManager: Handle destination input
AddressManager->>AddressManager: Decode X-address and sync tag state
User->>DefaultSendStrategy: Submit XRP transaction
DefaultSendStrategy->>RippleApi: Check destination account flags
RippleApi-->>DefaultSendStrategy: Return RequireDestTag status
DefaultSendStrategy->>RippleHelpter: Build signed Payment
RippleHelpter-->>DefaultSendStrategy: Payment with DestinationTag
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
When both fields carry the same value (memo == the tag's canonical decimal), the memo is not a distinct memo — sign it as tag-only instead of a rawJSON DestinationTag+Memos combo. This keeps a co-signer that predates the destination_tag field byte-identical: it reads the numeric memo and rebuilds the same DestinationTag, so the pre-image matches. The echoed memo is also hidden from the Verify Memo row (it shows in the Destination Tag row). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
app/src/test/java/com/vultisig/wallet/ui/models/send/submit/DefaultSendStrategyTest.kt (1)
69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the new destination-tag validation paths.
The fixture now wires
destinationTagFieldState, but no test in this file exercises the non-canonical-tag rejection (send_error_xrp_invalid_destination_tag) or thevalidateRippleDestinationTagRequireDestTag gate added insubmit(). The existingsubmit blocks XRP send to an unfunded destination below the reservetest provides a ready template (same WalletCore-JNI skip pattern) to adapt for these new checks.Also applies to: 499-499
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/vultisig/wallet/ui/models/send/submit/DefaultSendStrategyTest.kt` at line 69, The test fixture includes destinationTagFieldState but lacks coverage for the new destination-tag validation paths. In DefaultSendStrategyTest, add tests based on the existing unfunded XRP destination test, preserving its WalletCore-JNI skip pattern, to verify non-canonical tags return send_error_xrp_invalid_destination_tag and RequireDestTag from validateRippleDestinationTag blocks submission; configure destinationTagFieldState and assert the expected failures.app/src/main/java/com/vultisig/wallet/ui/models/send/submit/DefaultSendStrategy.kt (1)
274-285: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate
fetchAccountsInfocall for the same address.
validateRippleDestinationReserveandvalidateRippleDestinationTag(ChainValidationService.kt, Lines 213-283) each independently callrippleApi.fetchAccountsInfo(dstAddress). Calling both here means every native XRP send now performs the same RPC twice before submission.Consider fetching the account info once in
DefaultSendStrategyand passing the result into both validators (or merging the two checks into a single validator call) to halve this network round-trip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/vultisig/wallet/ui/models/send/submit/DefaultSendStrategy.kt` around lines 274 - 285, The XRP validation flow in DefaultSendStrategy currently triggers duplicate account-info requests through validateRippleDestinationReserve and validateRippleDestinationTag. Fetch rippleApi account information once and pass the shared result into both validators, or introduce a combined validator that performs both checks from one response; update the corresponding ChainValidationService method signatures and callers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/vultisig/wallet/ui/models/send/AddressManager.kt`:
- Around line 164-181: Clear destinationTagFieldState when decoded.tag is null
before unlocking it, while preserving the existing behavior of populating and
locking the field when a tag is present. Update applyXAddress so a tag-less
X-address cannot retain a stale tag from a previously processed address.
- Around line 122-140: Release the stale destination-tag lock when a new invalid
X-address is entered. In the Ripple handling within AddressManager, ensure the
failed RippleDestinationTag.decodeXAddress path applies the same lock-clearing
and lockedClassicAddress reset logic currently used for changed non-X addresses,
while preserving normal invalid-address fallthrough.
In `@app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormGraph.kt`:
- Line 195: Update AddressManager.applyXAddress() so that when decoded.tag is
null, it both unlocks and clears destinationTagFieldState, preventing a prior
tag from persisting; add a regression test covering replacement of a tagged
X-address with an untagged X-address and verifying the field is empty.
In
`@data/src/main/kotlin/com/vultisig/wallet/data/chains/helpers/RippleHelpter.kt`:
- Around line 103-147: In buildPaymentRawJson, convert the ULong values sequence
and lastLedgerSequence to Long before inserting them into txJson, so JSONObject
serializes Sequence and LastLedgerSequence as numeric JSON scalars. Keep the
remaining payload construction unchanged.
In
`@data/src/test/kotlin/com/vultisig/wallet/data/chains/helpers/RippleDestinationTagTest.kt`:
- Around line 27-46: Replace the non-null assertions in the `decode X-address
canonical vectors` test with `requireNotNull(...)` or nullable-safe assertions.
Apply this to each `RippleDestinationTag.decodeXAddress` result before accessing
`classicAddress` and `tag`, so malformed fixtures produce a clear test failure
instead of an NPE.
---
Nitpick comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/models/send/submit/DefaultSendStrategy.kt`:
- Around line 274-285: The XRP validation flow in DefaultSendStrategy currently
triggers duplicate account-info requests through
validateRippleDestinationReserve and validateRippleDestinationTag. Fetch
rippleApi account information once and pass the shared result into both
validators, or introduce a combined validator that performs both checks from one
response; update the corresponding ChainValidationService method signatures and
callers accordingly.
In
`@app/src/test/java/com/vultisig/wallet/ui/models/send/submit/DefaultSendStrategyTest.kt`:
- Line 69: The test fixture includes destinationTagFieldState but lacks coverage
for the new destination-tag validation paths. In DefaultSendStrategyTest, add
tests based on the existing unfunded XRP destination test, preserving its
WalletCore-JNI skip pattern, to verify non-canonical tags return
send_error_xrp_invalid_destination_tag and RequireDestTag from
validateRippleDestinationTag blocks submission; configure
destinationTagFieldState and assert the expected failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2daa4e1-4ff3-4643-92ef-e4482b0e95d7
📒 Files selected for processing (33)
app/src/main/java/com/vultisig/wallet/ui/models/VerifyTransactionViewModel.ktapp/src/main/java/com/vultisig/wallet/ui/models/mappers/TransactionToUiModelMapper.ktapp/src/main/java/com/vultisig/wallet/ui/models/send/AddressManager.ktapp/src/main/java/com/vultisig/wallet/ui/models/send/ChainValidationService.ktapp/src/main/java/com/vultisig/wallet/ui/models/send/SendFormGraph.ktapp/src/main/java/com/vultisig/wallet/ui/models/send/SendFormUiModel.ktapp/src/main/java/com/vultisig/wallet/ui/models/send/SendFormViewModel.ktapp/src/main/java/com/vultisig/wallet/ui/models/send/submit/DefaultSendStrategy.ktapp/src/main/java/com/vultisig/wallet/ui/models/send/submit/SendStrategyFactory.ktapp/src/main/java/com/vultisig/wallet/ui/screens/send/SendFormAmountSection.ktapp/src/main/java/com/vultisig/wallet/ui/screens/send/SendFormScreen.ktapp/src/main/java/com/vultisig/wallet/ui/screens/send/VerifySendScreen.ktapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-hr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-ko/strings.xmlapp/src/main/res/values-nl/strings.xmlapp/src/main/res/values-pt/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values/strings.xmlapp/src/test/java/com/vultisig/wallet/ui/models/send/AddressManagerTest.ktapp/src/test/java/com/vultisig/wallet/ui/models/send/submit/DefaultSendStrategyTest.ktcommondatadata/src/main/kotlin/com/vultisig/wallet/data/api/RippleApi.ktdata/src/main/kotlin/com/vultisig/wallet/data/chains/helpers/RippleDestinationTag.ktdata/src/main/kotlin/com/vultisig/wallet/data/chains/helpers/RippleHelpter.ktdata/src/main/kotlin/com/vultisig/wallet/data/mappers/KeysignPayloadProtoMapper.ktdata/src/main/kotlin/com/vultisig/wallet/data/mappers/PayloadToProtoMapper.ktdata/src/main/kotlin/com/vultisig/wallet/data/models/payload/BlockChainSpecfic.ktdata/src/main/kotlin/com/vultisig/wallet/data/repositories/BlockChainSpecificRepository.ktdata/src/test/kotlin/com/vultisig/wallet/data/chains/helpers/RippleDestinationTagTest.kt
The XRP fee service folded the destination account-activation reserve (~1 XRP) into the fee amount, so an unactivated destination showed a "network fee" of 1.00002 XRP — and worse, that inflated amount flowed into the XRPL Fee field (burning the reserve) and the max-amount math. The reserve is not a fee: it is part of the amount sent to the destination and is already enforced as an amount constraint by validateRippleDestinationReserve. The fee now reports the network fee only; the dead accountActivationFee field and its redundant account probe are removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- AddressManager: drop a stale X-address-derived destination tag when an invalid X-address is entered or a tagged X-address is replaced with an untagged one, so a tag can't ride onto a different destination (a hand-typed tag is kept). - RippleHelper: serialize Sequence/LastLedgerSequence as Long — org.json can't wrap a Kotlin ULong value class, which would drop them from the rawJSON. - Tests: requireNotNull instead of !! ; add tagged->untagged X-address regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 10, 2026
rkokhatskyi
requested changes
Jul 10, 2026
- RippleHelper: parse the legacy memo-as-tag carrier with the canonical parser (rejects 0, leading zeros, out-of-uint32) instead of memo.toLongOrNull(), so a stray numeric memo can't be silently reinterpreted or overflow a tag (#6). - DefaultSendStrategy: dual-write the tag into the memo when a tag is set and no distinct memo, so a not-yet-updated co-signer rebuilds the same DestinationTag (byte-identical sighash), mirroring iOS dualWritingRippleTag (#8). RequireDest validation also treats a canonical numeric memo as a present tag (#10). - TransactionToUiModelMapper: surface the tag whether it rides in the proto field or the canonical-numeric memo, and suppress that memo when it merely echoes the tag so it isn't shown twice on Verify (#7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rkokhatskyi
requested changes
Jul 11, 2026
- DefaultSendStrategy: treat whitespace-only memo as absent (isNotBlank), aligning the dual-write gate with RippleHelper so a blank memo can't suppress the tag dual-write and leave an old co-signer untagged. - RippleHelper: correct the comment — iOS rejects a bare "0" memo, we intentionally keep it as free text rather than claim exact iOS parity. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 23, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Adds first-class XRPL Destination Tag support to the XRP send flow, mirroring the iOS combo design (vultisig-ios#4749).
commondatasubmodule to includeRippleSpecific.destination_tag(already merged on commondatamain, PR Added error dialog when transaction info is incorrect on SendScreen #95). The tag rides that proto field, independent of the free-text memo.X…address to normalize to the classicr…address and autofill the embedded tag; a tag-0 flag-1 X-address is rejected outright.lsfRequireDestTagset (fails closed on lookup error, matching the existing reserve check).RippleHelper): prefers the tag field; supports tag+memo via rawJSONDestinationTag+Memos; legacy numeric-memo→tag and swap text-memo→Memos paths preserved.TransactionToUiModelMapper+VerifySendScreen): shows a Destination Tag row from the proto field alongside Memo, when non-empty.Issue
Closes #5184
Design note / tradeoff
The tag is carried only in the
destination_tagproto field (no memo dual-write — impossible now that memo carries independent free-text). A keysign ceremony with a device on a pre-destination_tagbuild will fail to sign a tagged XRP send rather than silently drop the tag — fail-safe (no fund loss, no wrong tx), but all devices must update. The tag+memo combo path likewise requires all co-signers on this build.Test Plan
RippleDestinationTagTest— canonical parser + X-address codec vectors (with/without tag, uint32-max, testnet reject, tag-0 reject, corrupt checksum)AddressManagerTest— X-address autofill/lock, no-tag leaves editable, clearing releases lock and drops derived tag./gradlew assembleDebugsucceeds./gradlew lint(running)DestinationTagon-chain; paste X-address → autofill/lock; tagless send to a RequireDest account → blocked; verify tag shows on both initiator and joining-device Verify screensCo-Authored-By: Claude Opus 4.8 noreply@anthropic.com
On chain test tx link:
https://xrpscan.com/tx/5624D7D4CDC445492C84DF38384196CA67AD4261CB93499B19CEAD408D7950C9
https://xrpscan.com/tx/7A0E125D5BFA1885019E861A3AB4BF7B95EFA9AAABE0B1A9D81B2CAC86D8E94D
https://xrpscan.com/tx/72DD773D9216731A204BA97AF5E6C348030354711A41CDE09BFF919A1193DAF1
Summary by CodeRabbit