feat(ripple): show XRP trust-line token balances in the asset list - #5387
Conversation
XRPL trust-line tokens have no on-chain decimals field and no contract address, so both had to be defined before any of them could be read. Amounts travel as decimal strings with 16 significant digits, so the fixed-point scale is a wallet-side choice: pin it at 15 and truncate down, so a displayed balance never rounds up past what the ledger holds. Identity is the (currency, issuer) pair stored as "<currency>.<issuer>" — the raw on-chain currency code, so it round-trips into account_lines comparisons unchanged, with the display ticker derived separately. Coin.id has to carry that pair. A currency code is only unique per issuer, and several independent issuers each mint their own USD; on the plain ticker-chainId id they would collide and REPLACE-overwrite each other's persisted row, exactly the case THORChain secured assets are already qualified for. account_lines returns every line for an account in one response while the balance layer asks per token, so N held currencies would fire N identical paginated request chains at the public cluster. Coalesce them behind a per-address mutex and a short TTL, mirroring CosmosBalanceCache. Co-Authored-By: aminsato <Amin.saradar@yahoo.com>
Route Chain.Ripple through the same token-discovery seam the EVM and Cosmos-bank finders already use, so the chain-detail screen and the background refresh worker pick these up with no bespoke states. Only a positive balance is a holding. A zero-balance line is an opt-in with nothing in it — holding one is how an account subscribes to a currency before ever receiving it — and a negative balance means the account issues the currency rather than holds it, so neither belongs in the asset list. Both still count toward OwnerCount, which the existing native-XRP reserve subtraction already folds into spendable XRP; no separate trust-line reserve accounting is needed. A transient account_lines failure yields no tokens rather than propagating, matching the sibling finders: a network blip must not wipe tokens the vault already holds, and the next refresh retries. Closes #5210 Co-Authored-By: aminsato <Amin.saradar@yahoo.com>
Ripple.OperationPayment carries a drops-denominated amount, so the payment builder can only move native XRP. An issued-currency coin reaching it would sign away toAmount *drops of XRP* instead of the token the user picked — a silent loss of unrelated funds, and newly reachable now that these tokens appear in the asset list. Moving one needs a Payment carrying a CurrencyAmount (currency, issuer, value), which is separate work. Until then they are read-only: gate the token-detail send and swap buttons and both asset pickers so the flows cannot be entered, and refuse in the helper itself so no future entry point can reach the drops path unnoticed. The guard sits after the dApp raw-JSON branch, which signs verbatim and stays free to submit TrustSet and issued-currency payments. Co-Authored-By: aminsato <Amin.saradar@yahoo.com>
|
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:
📝 WalkthroughWalkthroughAdds XRPL issued-token identity, trust-line balance retrieval, token discovery, curated RLUSD metadata, issuer-qualified coin IDs, and read-only asset handling across data, signing, and wallet UI flows. ChangesXRPL issued assets
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WalletUI
participant TokenRepository
participant RippleTokenFinder
participant RippleApi
participant XRPL
WalletUI->>TokenRepository: request Ripple tokens
TokenRepository->>RippleTokenFinder: find(address)
RippleTokenFinder->>RippleApi: fetchAccountLines(address)
RippleApi->>XRPL: request paginated account_lines
XRPL-->>RippleApi: return trust lines and markers
RippleApi-->>RippleTokenFinder: return curated Ripple coins
RippleTokenFinder-->>TokenRepository: return issuer-qualified read-only coins
TokenRepository-->>WalletUI: return token list and balances
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
data/src/test/kotlin/com/vultisig/wallet/data/api/RippleAccountLinesTest.kt (1)
41-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
runTestinstead ofrunBlockingin these tests.Replace the test wrappers with
kotlinx.coroutines.test.runTestto follow the coroutine testing guideline.Proposed change
-import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.runTest - fun `fetchAccountLines parses currency issuer and balance`() = runBlocking { + fun `fetchAccountLines parses currency issuer and balance`() = runTest {As per coding guidelines, “Avoid runBlocking — use suspend functions and proper coroutines with viewModelScope.”
🤖 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 `@data/src/test/kotlin/com/vultisig/wallet/data/api/RippleAccountLinesTest.kt` around lines 41 - 178, Replace the runBlocking wrappers in the RippleAccountLinesTest test methods with kotlinx.coroutines.test.runTest, adding the appropriate import. Keep each test’s existing coroutine body and assertions unchanged while using the coroutine test framework consistently.Source: Coding guidelines
🤖 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/TokenDetailViewModel.kt`:
- Line 42: Initialize canSend to false in TokenDetailViewModel so SEND remains
unavailable until a successful matching account load confirms the token is
writable; preserve the existing logic that enables it only after the read-only
check completes.
In `@data/src/main/kotlin/com/vultisig/wallet/data/api/RippleApi.kt`:
- Around line 213-218: Update the account-lines response handling in RippleApi
so `result.error` is propagated instead of returning and caching an empty list,
while preserving `actNotFound` as the legitimate empty-account case. Add a
regression test covering a successful HTTP response with an RPC error and no
lines, asserting that the error is thrown rather than cached.
In `@data/src/main/kotlin/com/vultisig/wallet/data/models/RippleToken.kt`:
- Around line 58-64: Update parseRippleTokenIdentity to reject contractAddress
values containing more than one RIPPLE_TOKEN_SEPARATOR, while preserving the
existing checks for missing currency or issuer; only addresses with exactly one
separator should produce RippleTokenIdentity, and add a parser test covering a
multiple-separator value such as USD.rIssuer.extra.
In `@data/src/main/kotlin/com/vultisig/wallet/data/usecases/RippleTokenFinder.kt`:
- Around line 58-60: Remove the broad Exception catch in the RippleTokenFinder
account_lines flow so unexpected failures from malformed balances,
deserialization, or programming errors propagate instead of returning
emptyList(). Preserve only the existing explicit timeout and NetworkException
handling branches, including their intended tolerant behavior.
- Around line 67-87: Update resolveCoin and its callers to accept the vault
address, set that address on generated Ripple token Coin instances, and copy it
onto curated coins returned by preferCurated. Preserve the existing token
identity and metadata while ensuring every discovered coin carries the vault
address used by RippleApi.getTokenBalance().
---
Nitpick comments:
In `@data/src/test/kotlin/com/vultisig/wallet/data/api/RippleAccountLinesTest.kt`:
- Around line 41-178: Replace the runBlocking wrappers in the
RippleAccountLinesTest test methods with kotlinx.coroutines.test.runTest, adding
the appropriate import. Keep each test’s existing coroutine body and assertions
unchanged while using the coroutine test framework consistently.
🪄 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: 172e7fd9-7be1-4661-bccd-9fda82361ec4
📒 Files selected for processing (17)
app/src/main/java/com/vultisig/wallet/ui/components/v2/fastselection/FastSelectionPopupSharedViewModel.ktapp/src/main/java/com/vultisig/wallet/ui/models/TokenDetailViewModel.ktapp/src/main/java/com/vultisig/wallet/ui/screens/TokenDetailScreen.ktapp/src/main/java/com/vultisig/wallet/ui/screens/select/SelectAssetViewModel.ktapp/src/test/java/com/vultisig/wallet/ui/models/send/ChainValidationServiceTest.ktdata/src/main/kotlin/com/vultisig/wallet/data/api/RippleApi.ktdata/src/main/kotlin/com/vultisig/wallet/data/chains/helpers/RippleHelpter.ktdata/src/main/kotlin/com/vultisig/wallet/data/models/Coin.ktdata/src/main/kotlin/com/vultisig/wallet/data/models/RippleToken.ktdata/src/main/kotlin/com/vultisig/wallet/data/repositories/BalanceRepository.ktdata/src/main/kotlin/com/vultisig/wallet/data/repositories/TokenRepository.ktdata/src/main/kotlin/com/vultisig/wallet/data/usecases/DataUsecasesModule.ktdata/src/main/kotlin/com/vultisig/wallet/data/usecases/RippleTokenFinder.ktdata/src/test/kotlin/com/vultisig/wallet/data/api/RippleAccountLinesTest.ktdata/src/test/kotlin/com/vultisig/wallet/data/models/RippleTokenTest.ktdata/src/test/kotlin/com/vultisig/wallet/data/repositories/TokenRepositoryImplTest.ktdata/src/test/kotlin/com/vultisig/wallet/data/usecases/RippleTokenFinderTest.kt
A curated entry lets the token be enabled before any balance exists and gives the discovered trust line an icon and a fiat price: account_lines discovery prefers the catalog match over the generic coin it would otherwise synthesize, which carries no logo and no priceProviderID. The currency code is the 40-hex form XRPL uses for names longer than three characters, paired with the issuing account in the same "<currency>.<issuer>" notation the balance lookup matches on, and scaled to RIPPLE_TOKEN_DECIMALS so its balance converts like any other trust line. Co-Authored-By: aminsato <Amin.saradar@yahoo.com>
An XRPL node answers a rate-limited or unsynced read with HTTP 200 and a result.error, which bodyOrThrow cannot see. Reading that as "no trust lines" dropped every issued currency from the asset list and cached the emptiness for the coalescing TTL, so throw on any error other than the actNotFound an unfunded account legitimately returns. Reject contract addresses carrying more than one separator: neither a currency code nor a base58 issuer can contain it, so splitting on the first yielded an issuer that no trust line could ever match. Default the token-detail SEND flag to closed like every other action flag there, since the read-only check only runs once a matching account resolves and the button has no disabled state. Co-Authored-By: aminsato <Amin.saradar@yahoo.com>
There was a problem hiding this comment.
I think we should do the full support for tokens, not just discovery. This is incomplete feature and needs more work. Few things in mind:
- Sending XRPL tokens doesn't work
- We need to control the list of tokens as anyone can create a trustline, call it USDT and you don't know where's correct one. XRP doesn't have many tokens, we could add them all
- No pricing for XRPL tokens
- We cache balances keys on
(chain, address, ticker), not the full issuer-qualified id, so two same-ticker issuers can collide (REPLACE-on-conflict)
…cache key Gate XRPL trust-line discovery to the curated Coins catalog: anyone can open a trust line under any currency label, so an arbitrary held line is not evidence of a genuine asset. The finder now surfaces a line only when the catalog lists its currency/issuer pair and drops the rest; new tokens are onboarded by adding them to Coins.Ripple.all. Fix the balance cache-key collision: TokenValueEntity.tokenId only qualified THORChain secured assets, so for Ripple it collapsed to the plain ticker-chain id and no longer mirrored the issuer-qualified Coin.id. Two issuers of the same currency then resolved to one row and BalanceRepository lookups by id missed (decimals -> 0). tokenId now mirrors Coin.id for issued currencies. Co-Authored-By: aminsato <Amin.saradar@yahoo.com>
|
Thanks Roman — agree the full token experience is the goal. I've split it so this PR lands correct, safe discovery now, with sending and pricing as the next steps. Addressed in this PR (749eb14): 2. Controlled token list. Discovery is now gated to the curated 4. Cache-key collision. Root cause was Follow-ups (separate PRs under the XRP token support set): 1. Sending XRPL tokens. Needs a 3. Pricing. I'll wire XRPL issued currencies into the price provider (keyed per issuer via each catalog entry's Keeping this PR to correct, curated, read-only discovery keeps it reviewable; happy to track 1 and 3 as linked issues if you'd like. |
Address review on #5387: - Skip read-only assets (XRPL issued currencies) in send preselection so a crafted vultisig://send deep link can't pin one as the send source until keysign throws. - Pin the first account_lines page's resolved ledger index on every later page instead of re-sending the "validated" shorthand, which could drop a line straddling a page boundary and read it as zero. - Add ignore_default:true so default zero-balance trust lines are filtered server-side, bounding the paged walk against trust-line spam. - Scale getTokenBalance by the coin's own decimal rather than a fixed constant, matching the scale the balance is rendered at. - Remove the dead rippleCurrencyTicker helper (and its hex constants / isHexDigit) that had no production caller and carried an inverted 0x00 doc. Co-Authored-By: aminsato <Amin.saradar@yahoo.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
data/src/main/kotlin/com/vultisig/wallet/data/models/RippleToken.kt (1)
54-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject native XRP in the identity parser.
Coin.isRippleIssuedTokendelegates toparseRippleTokenIdentity, but the parser’s structural checks allowXRP.<issuer>. A constructible non-nativeCoincan therefore be classified as an issued/read-only token even though XRP cannot be a trust-line currency. Reject native currency codes here and add a regression test forparseRippleTokenIdentity("XRP.$ISSUER").Proposed fix
+ if (isRippleNativeCurrency(currency)) return null🤖 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 `@data/src/main/kotlin/com/vultisig/wallet/data/models/RippleToken.kt` around lines 54 - 65, Update parseRippleTokenIdentity to return null when the parsed currency is the native XRP code, while preserving existing separator and issuer validation. Add a regression test covering parseRippleTokenIdentity("XRP.$ISSUER") and assert it is rejected.
🤖 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.
Outside diff comments:
In `@data/src/main/kotlin/com/vultisig/wallet/data/models/RippleToken.kt`:
- Around line 54-65: Update parseRippleTokenIdentity to return null when the
parsed currency is the native XRP code, while preserving existing separator and
issuer validation. Add a regression test covering
parseRippleTokenIdentity("XRP.$ISSUER") and assert it is rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9caae5fc-8d5b-491e-b174-e3427334bbb9
📒 Files selected for processing (5)
app/src/main/java/com/vultisig/wallet/ui/models/send/TokenPreselectionService.ktdata/src/main/kotlin/com/vultisig/wallet/data/api/RippleApi.ktdata/src/main/kotlin/com/vultisig/wallet/data/models/RippleToken.ktdata/src/test/kotlin/com/vultisig/wallet/data/api/RippleAccountLinesTest.ktdata/src/test/kotlin/com/vultisig/wallet/data/models/RippleTokenTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- data/src/test/kotlin/com/vultisig/wallet/data/api/RippleAccountLinesTest.kt
- data/src/main/kotlin/com/vultisig/wallet/data/api/RippleApi.kt
|
@Vaulty-bot pick-up |
|
I'm currently working on another task in this repo. I'll pick this up in the next cycle (~5 min). |
|
@Vaulty-bot pick-up |
|
Picked up. Reading feedback and working on it. |
… response (#5387) rippled counts every ledger object toward `limit` and pins the marker before the `ignore_default` filter runs, so the flag reduces each response body but leaves the page walk unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 3 — one comment, one commitCommit: `db65fbb49` — Fixed
Kept the flag itself: smaller bodies still mean less transfer and less parsing per page. Left Comment-only change, 4 insertions / 3 deletions, no behaviour touched. DeclinedNone this round. Verification
|
4 participants
What
XRP issued-currency (trust-line) tokens now appear in the asset list with their balances, discovered from
account_lines.Discovery routes through the existing
TokenRepository.getTokensWithBalanceseam that the EVM and Cosmos-bank finders already use, so the chain-detail screen and the background refresh worker pick these up with no bespoke UI states.Closes #5210 · SDK counterpart: vultisig/vultisig-sdk#997
Decisions worth reviewing
Android is the first platform to implement this — iOS and the extension have nothing here yet — so the token model was defined in this PR rather than matched to an existing one.
Identity is
"<currency>.<issuer>"incontractAddress. An XRPL currency code is only unique per issuer, and the raw on-chain code is stored (3-char ASCII or 40-char hex) so it round-trips intoaccount_linescomparisons unchanged; the display ticker is derived separately.Coin.idis contract-qualified for these. Several independent issuers each mint their ownUSD. On the plainticker-chainIdid they would collide, and Room's REPLACE-on-conflict insert means enabling the second would silently overwrite the first's persisted row — the same hazard THORChain secured assets are already qualified for. Native XRP is untouched.Decimals are a wallet-side choice. Unlike ERC-20/SPL, an XRPL issued currency has no on-chain decimals field: amounts travel as decimal strings with 16 significant digits. Pinned to 15, truncating down, so a displayed balance never rounds up past what the ledger holds.
Zero and negative lines are dropped. A zero-balance line is an opt-in with nothing in it (holding one is how an account subscribes to a currency before ever receiving it); a negative balance means the account issues the currency rather than holds it.
No separate owner-reserve work was needed for the SDK issue's second acceptance item: XRPL's
OwnerCountalready counts trust lines, so the existing native-XRP reserve subtraction inRippleApi.getBalancealready folds them into spendable XRP.account_linesreads are coalesced. One response carries every line for an account, but the balance layer asks per token — N held currencies would otherwise fire N identical paginated request chains at the public cluster.RippleAccountLinesCacheuses a per-address mutex + 10s TTL, mirroring the existingCosmosBalanceCache.Slightly beyond the ticket: these assets are read-only
Ripple.OperationPaymentcarries a drops-denominated amount, so the payment builder can only move native XRP. Now that these tokens are reachable in the asset list, routing one into send would have signed awaytoAmountdrops of XRP instead of the token the user picked — a silent loss of unrelated funds.Moving an issued currency needs a
Paymentcarrying aCurrencyAmount(currency + issuer + value), which is separate work. Until then they are gated: the token-detail send/swap buttons and both asset pickers are closed viaCoin.isReadOnlyAsset, andRippleHelperrefuses outright so no future entry point can reach the drops path unnoticed. That guard sits after the dApp raw-JSON branch, which signs verbatim and stays free to submitTrustSetand issued-currency payments.Shipping the display without this would have been unsafe, so it is included rather than deferred.
Test plan
:data:testDebugUnitTestand:app:testDebugUnitTest— full suites green:app:lintDebuggreen; ktfmt appliedRippleAccountLinesTest(11 — parsing, marker pagination, repeated-marker bail-out, per-token lookup, issuer disambiguation, read coalescing),RippleTokenTest(17 — identity round-trip, id collisions, hex currency decoding in both shapes, fixed-point scaling and truncation),RippleTokenFinderTest(8 — zero/negative/XRP filtering, two issuers of one currency, failure tolerance), plus a Ripple-delegation case inTokenRepositoryImplTest:data+:app)Not verified on device or mainnet. Reaching this state needs a vault whose XRP account holds a trust line with a positive balance, which requires a
TrustSetthe app has no screen for — it has to come through the extension's XRPL dApp path. Reviewers with such an account, the two things to eyeball are the balance precision on a fractional holding and two issuers of the same currency code rendering as separate rows.Summary by CodeRabbit
New Features
Bug Fixes
Tests