Repository navigation
Support LendingProtocolV1_1 - #1341
cybele-ripple wants to merge 8 commits into
Conversation
…elds Adds Vault ledger flag decoding (lsfVaultDepositBlocked, lsfVaultOwnerCanBlockDeposit) and a "Deposits Allowed" indicator on the Vault page, new tf* flag entries for VaultCreate/VaultSet/VaultDeposit, and rendering for VaultDelete's new MemoData field, matching the upstream xrpl4j changes for DGE-7974. Also fixes long Vault ID/Owner values being truncated or cut off on the Vault page and across the Vault transaction Simple views, since there's normally enough room to show them in full. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![ripple-code-reviewer[bot]](https://avatars.githubusercontent.com/u/3165523?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a clean, well-tested change. The new Vault deposit-blocking flags are decoded and gated correctly (the "Deposits Allowed" row only renders when lsfVaultOwnerCanBlockDeposit is set, and the row's inverse-of-blocked logic matches the tests), the new tf* transaction flag entries live in their own per-transaction-type namespaces with no bit collisions, and the truncation fixes (full Vault ID/Owner display, CSS wrapping via overflow-wrap: anywhere, and the new .vault-id selector) look consistent with the stated goal and are backed by updated tests. I did not find correctness or security issues in the diff. One minor doc nit below.
Covers the LendingProtocolV1_1 additions that were missing alongside the deposit-blocking work already on this branch: - VaultKind, SubscriptionDate and RedemptionDate on the Vault page and the VaultCreate simple view. Dates are rendered with the same convertRippleDate + localizeDate + DATE_OPTIONS treatment Escrow uses, so they read as timestamps rather than raw Ripple-epoch seconds. - CredentialIDs on VaultWithdraw, reusing the shared CredentialIDs row component already used by EscrowFinish and PaymentChannelClaim. VaultKind values live in a new shared vaultUtils so the Vault page and the transaction view agree on the labels. Fields absent from the xrpl package's types are read through a cast, matching the existing MemoData pattern. LEVersion is intentionally not surfaced: it selects legacy vs. cash-basis accounting for libraries decoding the ledger entry, and is noise for a human reading the explorer.
There was a problem hiding this comment.
This is a well-structured, well-tested MR that adds decoding for the new Vault deposit-blocking flags/fields, new tf* flag entries, VaultDelete MemoData rendering, and fixes truncation of long Vault IDs/Owner addresses. The logic for the new lsfVaultDepositBlocked/lsfVaultOwnerCanBlockDeposit flags is internally consistent (the 'Deposits Allowed' row is correctly gated on canBlockDeposit, and defaults sensibly when flags is undefined). The as-any casts for fields not yet in the xrpl package's types match the documented existing pattern (CredentialIDs). Test coverage for the new behavior (flag combinations, VaultKind branches, MemoData) is thorough. I did not find high-confidence correctness or security issues in the changed lines; only minor consistency/dead-code observations worth a second look.
… single-asset-vault-updates
There was a problem hiding this comment.
The change is clean overall: it correctly decodes the new Vault deposit-blocking flags, wires up new tf* flag entries consistently with the existing TX_FLAGS table, renders VaultDelete's MemoData using the same decodeHex/hexMatch pattern used elsewhere, and fixes the Vault ID/Owner truncation with reasonable CSS changes backed by before/after screenshots. Tests were updated/added for the new behavior and look thorough. I didn't find any correctness, security, or logic bugs in the added code — the as any casts are consistent with the documented pattern for fields ahead of the xrpl package's types. The one thing worth tidying is a duplicated date-formatting helper.
Collapse the multi-line union type casts flagged by prettier/prettier in the lint CI job.
There was a problem hiding this comment.
Overall a well-scoped feature addition (Vault deposit-blocking flags/fields) with good test coverage. No clear correctness, security, or data-handling bugs found in the changed lines. The main point worth double-checking is a className inconsistency on the new VaultDelete MemoData row, given this same MR is specifically fixing text-wrapping/truncation issues elsewhere.
|
Hi @cybele-ripple, I think this PR is based on an outdated issue: |
…oData/CredentialIDs xrpl@5.3.0 already ships types for these fields (from xrpl.js#3456), so the `as any` casts and "not yet part of the xrpl package" comments are stale.
There was a problem hiding this comment.
Solid, well-tested feature addition for Vault deposit-blocking flags/fields and the truncation fixes. The flag bit values, new tf* entries, and MemoData rendering all line up with their tests and mock data, and I didn't find correctness/security issues in the added code. One thing worth double-checking: the new .col2 CSS override in VaultHeader/styles.scss is scoped to the whole .token-table rather than just the Vault ID/Owner rows the comment describes, so it also applies to every other row in the table (Vault Kind, dates, the new Deposits Allowed toggle, etc.) — worth confirming that doesn't change their layout.
Applied fixes mentioned in the latest commit |
There was a problem hiding this comment.
Overall the vault deposit-blocking decode logic, flag mappings, and truncation/wrapping CSS fixes look correct and are well covered by tests. The main thing worth double-checking before merge is that newly introduced fields (VaultCreate's VaultKind/SubscriptionDate/RedemptionDate, VaultDelete's MemoData) are destructured directly from objects typed against the real xrpl package types, but the PR description says these fields aren't in the npm package yet and that the established pattern for this (as used for CredentialIDs in EscrowFinish/PaymentChannelClaim) is an as any cast with a comment — that cast appears to be missing here, which could be a TypeScript compile error unless there's a type augmentation elsewhere not shown in this diff.
|
@cybele-ripple could you also make sure to update the screenshots? Thanks! |
Done! |
|
Can you change the PR title to Support LendingProtocolV1_1 or something similar? |
|
The Credential IDs Simple Row on VaultWithdraw formatting looks buggy. Can you look at other transactions with Credential IDs and apply the same css, or create some formatting on its own here? |
|
CredentialIDs on LoanBrokerCoverWithdraw is missing |
- Remove outdated Vault/VaultCreate/VaultDeposit/VaultSet flags not present in rippled (lsfVaultDepositBlocked, lsfVaultOwnerCanBlockDeposit, tfVaultDonate, tfVaultDepositBlock/Unblock) along with the deposit-blocked UI built on them - Add word-break styling to CredentialIDs rows to match other long-hex fields - Add missing CredentialIDs field/rendering to LoanBrokerCoverWithdraw
cybele-ripple
changed the title
There was a problem hiding this comment.
Reviewed the diff for the Single Asset Vault / VaultCreate / VaultDelete / VaultWithdraw changes (closed-ended vault kind/dates, CredentialIDs rendering, tfInnerBatchTxn flag, and the Vault ID/Owner wrapping fixes). The code changes shown are internally consistent and the new tests match the new behavior. One notable gap: the MR title/description centers on decoding the new deposit-blocking flags (lsfVaultDepositBlocked, lsfVaultOwnerCanBlockDeposit, tfVaultOwnerCanBlockDeposit, tfVaultDepositBlock/Unblock, tfVaultDonate) and showing a 'Deposits Allowed' indicator, but none of that logic appears anywhere in the diff provided — only VaultKind/SubscriptionDate/RedemptionDate (closed-ended vault) and CredentialIDs support are implemented. This may simply be outside the diff scope shown, but it's worth confirming the deposit-blocking rendering code actually landed, since the new 'deposits_allowed' translation string currently has no consumer in this diff. No other correctness, security, or infra issues stood out in the changed lines.
Changed |
CredentialIDs didn’t have any wrapping CSS for its row values, so long hex IDs overflowed instead of breaking like other long-hex fields (vault-id, tx, channel, etc). Added a credential-id class and included it in that same word break: |
Added |
Can you update the screenshot so I can see what it looks like now? |
| "check_vault_id": "Please check the vault ID", | ||
| "get_vault_failed": "Unable to load vault information at this time. Please try again later.", | ||
| "private_vault": "Private Vault", | ||
| "deposits_allowed": "Deposits Allowed", |
| "assets_maximum": "Assets Maximum", | ||
| "mptoken_metadata": "MPToken Metadata", | ||
| "withdrawal_policy": "Withdrawal Policy", | ||
| "vault_kind": "Vault Kind", |
There was a problem hiding this comment.
Please add translation (placeholder with null values) for other languages
| <Account account={Destination} /> | ||
| </SimpleRow> | ||
| )} | ||
| {credentialIds && credentialIds.length > 0 && ( |
There was a problem hiding this comment.
Please update the Simple test for this tx
High Level Overview of Change
Context of Change
Tracks upstream additions to Single Asset Vault deposit blocking (XLS-469, XLS-470, gated behind
LendingProtocolV1_1), mirrored in xrpl4j#832. The xrpl npm package doesn't have these fields yet, so access uses as any casts with a comment, matching the existing pattern for fields ahead of the package's types (e.g. CredentialIDs in EscrowFinish/PaymentChannelClaim).The truncation fixes were found while manually previewing this work — Vault IDs and owner addresses were either shortened unnecessarily or overflowing instead of wrapping, despite there being enough room to show them in full.
Type of Change
Codebase Modernization
Before / After
Note: The data in these screenshots is mock data that was created locally.
Vault page with deposits allowed
Vault page with deposits blocked
Vault delete - simple view
Vault delete - detailed view
Vault delete - with memo data
Vault deposit - simple view
Vault create - simple view
Vault create - detailed view
Vault Withdraw - with credentials
Test Plan
All tests pass including new/updated tests for
VaultHeaderandVaultCreate