Single Asset Vault XLS-65d #814
Conversation
…g serialization of Number need to be solved;
WalkthroughThis pull request adds a new “SingleAssetVault” feature by updating configuration and changelog files. It introduces new integration and unit tests—including tests for the vault lifecycle and number serialization. Updates to transaction fee calculation integrate a new fee for vault creation. Additionally, the changes extend the binary codec definitions with vault-related fields and types while exporting a new Number codec. Several new vault-related transaction types and models are added throughout the codebase, alongside updates to account and ledger entry enumerations to accommodate vault operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant VaultOwner
participant Issuer
participant Ledger
Client->>VaultOwner: Initiate VaultCreate transaction
VaultOwner->>Ledger: Request vault creation
Ledger-->>VaultOwner: Return vault ID confirmation
Client->>VaultOwner: Send VaultDeposit transaction
VaultOwner->>Ledger: Process deposit
Ledger-->>VaultOwner: Confirm deposit
Client->>VaultOwner: Send VaultWithdraw transaction
VaultOwner->>Ledger: Process withdrawal
Ledger-->>VaultOwner: Confirm withdrawal
Issuer->>Ledger: Send VaultClawback transaction
Ledger-->>Issuer: Confirm clawback
Client->>VaultOwner: Send VaultDelete transaction
VaultOwner->>Ledger: Request vault deletion
Ledger-->>VaultOwner: Confirm deletion
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 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
Documentation and Community
|
…l; Update the changelog 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: 10
🧹 Nitpick comments (18)
xrpl/models/amounts/mpt_amount.py (1)
42-50: Consider makingMPTIssueconsistent with other data models.Currently, this class does not inherit from
BaseModelor use a dataclass. If you anticipate adding dynamic validation or JSON serialization logic, aligning it with the approach used forMPTAmountmay simplify future enhancements.xrpl/core/binarycodec/definitions/definitions.json (2)
2073-2082: New field “AssetTotal”.This addition follows the same pattern. Keep an eye on whether integrators might interpret it as a typical “UINT” or a decimal. Documentation should clarify that it’s a “Number” with potential decimal representation.
2702-2711: New field “WithdrawalPolicy”.Added as a
UInt8withnth=20. This is typically used for small enumerations. Confirm you’ve reserved or documented permissible values (e.g., 0,1,2).xrpl/models/transactions/vault_set.py (3)
1-3: Fix docstring formattingThe module docstring should fit on one line with quotes per linting rules.
-""" -Represents a VaultSet transaction on the XRP Ledger. -""" +"""Represents a VaultSet transaction on the XRP Ledger."""🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 1-1: D200 One-line docstring should fit on one line with quotes
16-19: Fix class docstring formattingThe class docstring should fit on one line with quotes per linting rules.
- """ - The VaultSet updates an existing Vault ledger object. - """ + """The VaultSet updates an existing Vault ledger object."""🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 17-17: D200 One-line docstring should fit on one line with quotes
30-33: Fix trailing whitespaceThere's a trailing whitespace at the end of line 31.
- """The maximum asset amount that can be held in a vault. The value cannot be lower + """The maximum asset amount that can be held in a vault. The value cannot be lower than the current AssetTotal unless the value is 0. """🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 31-31: W291 trailing whitespace
xrpl/models/transactions/vault_clawback.py (2)
1-3: Fix docstring formattingThe module docstring should fit on one line with quotes per linting rules.
-""" -Represents a VaultClawback transaction on the XRP Ledger. -""" +"""Represents a VaultClawback transaction on the XRP Ledger."""🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 1-1: D200 One-line docstring should fit on one line with quotes
35-38: Fix trailing whitespaceThere's a trailing whitespace at the end of line 36.
- """The asset amount to clawback. When Amount is 0 clawback all funds, up to the + """The asset amount to clawback. When Amount is 0 clawback all funds, up to the total shares the Holder owns. """🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 36-36: W291 trailing whitespace
xrpl/models/transactions/vault_create.py (6)
1-3: Fix docstring formattingThe module docstring should fit on one line with quotes per linting rules.
-""" -Represents a VaultCreate transaction on the XRP Ledger. -""" +"""Represents a VaultCreate transaction on the XRP Ledger."""🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 1-1: D200 One-line docstring should fit on one line with quotes
18-22: Add docstring to VaultCreateFlag enumThe enum class is missing a docstring, which is required for public classes.
class VaultCreateFlag(int, Enum): + """Flags that can be set on a VaultCreate transaction.""" TF_VAULT_PRIVATE = 0x0001 TF_VAULT_SHARE_NON_TRANSFERABLE = 0x0002🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 18-18: D101 Missing docstring in public class
24-28: Add docstring to VaultCreateFlagInterface classThe interface class is missing a docstring, which is required for public classes.
class VaultCreateFlagInterface(FlagInterface): + """Interface for VaultCreate transaction flags.""" TF_VAULT_PRIVATE: bool TF_VAULT_SHARE_NON_TRANSFERABLE: bool🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 24-24: D101 Missing docstring in public class
32-35: Fix class docstring formattingThe class docstring should fit on one line with quotes per linting rules.
- """ - The VaultCreate transaction creates a new Vault object. - """ + """The VaultCreate transaction creates a new Vault object."""🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 33-33: D200 One-line docstring should fit on one line with quotes
53-60: Fix blank line with whitespaceLine 54 contains whitespace on an otherwise blank line.
"""Indicates the withdrawal strategy used by the Vault. - + The below withdrawal policy is supported: Strategy Name Value Description🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 54-54: W293 blank line contains whitespace
37-61: Document flag meanings and consider adding more withdrawal policiesWhile the implementation is solid, it would be helpful to:
- Document what each flag means (TF_VAULT_PRIVATE and TF_VAULT_SHARE_NON_TRANSFERABLE)
- Consider using an enum for withdrawal_policy instead of an int with comments, to make the API more type-safe
🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 54-54: W293 blank line contains whitespace
tests/unit/core/binarycodec/types/test_number.py (1)
35-42: Consider adding validation for negative exponents in extreme limits testThe current test only checks extreme mantissa values with positive exponents. For comprehensive testing, consider adding test cases with negative exponents as well, which would test the handling of extremely small numbers.
tests/integration/transactions/test_sav.py (3)
78-78: Variable name should follow Python conventionUse lowercase for variable names according to PEP 8.
VAULT_IDshould be renamed tovault_id.- VAULT_ID = account_objects_response.result["account_objects"][0]["index"] + vault_id = account_objects_response.result["account_objects"][0]["index"]
114-123: Improve comment clarity for VaultClawback transactionThe comment describing the behavior is somewhat confusing. It would be clearer to explicitly state that the transaction will clawback the entire remaining balance (1 unit) regardless of the specified amount (9).
- # Note: Although the amount is specified as 9, 1 unit of the IOU will be - # clawed back, because that is the remaining balance in the vault + # Note: This will clawback only 1 unit of the IOU (the remaining balance), + # even though the amount is specified as 9. The clawback amount is limited + # to the available balance in the vault.
27-135: Add verification of vault state after each operationThe test validates that transactions are successful, but doesn't verify the actual state of the vault after each operation (e.g., checking balances). Adding assertions for vault state would strengthen the test.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.ci-config/rippled.cfg(1 hunks)CHANGELOG.md(1 hunks)tests/integration/transactions/test_sav.py(1 hunks)tests/unit/core/binarycodec/types/test_number.py(1 hunks)xrpl/asyncio/transaction/main.py(1 hunks)xrpl/core/binarycodec/definitions/definitions.json(6 hunks)xrpl/core/binarycodec/types/__init__.py(2 hunks)xrpl/core/binarycodec/types/number.py(1 hunks)xrpl/models/amounts/mpt_amount.py(1 hunks)xrpl/models/requests/account_objects.py(1 hunks)xrpl/models/requests/ledger_entry.py(3 hunks)xrpl/models/transactions/__init__.py(2 hunks)xrpl/models/transactions/types/transaction_type.py(1 hunks)xrpl/models/transactions/vault_clawback.py(1 hunks)xrpl/models/transactions/vault_create.py(1 hunks)xrpl/models/transactions/vault_delete.py(1 hunks)xrpl/models/transactions/vault_deposit.py(1 hunks)xrpl/models/transactions/vault_set.py(1 hunks)xrpl/models/transactions/vault_withdraw.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Unit test
xrpl/models/transactions/vault_delete.py
[warning] 1-1: D200 One-line docstring should fit on one line with quotes
[warning] 16-16: D200 One-line docstring should fit on one line with quotes
xrpl/models/transactions/vault_deposit.py
[warning] 1-1: D200 One-line docstring should fit on one line with quotes
[warning] 17-17: D200 One-line docstring should fit on one line with quotes
xrpl/models/transactions/vault_withdraw.py
[warning] 1-1: D200 One-line docstring should fit on one line with quotes
[warning] 18-18: D200 One-line docstring should fit on one line with quotes
xrpl/models/transactions/vault_set.py
[warning] 1-1: D200 One-line docstring should fit on one line with quotes
[warning] 17-17: D200 One-line docstring should fit on one line with quotes
[warning] 31-31: W291 trailing whitespace
xrpl/models/transactions/vault_create.py
[warning] 1-1: D200 One-line docstring should fit on one line with quotes
[warning] 18-18: D101 Missing docstring in public class
[warning] 24-24: D101 Missing docstring in public class
[warning] 33-33: D200 One-line docstring should fit on one line with quotes
[warning] 54-54: W293 blank line contains whitespace
tests/unit/core/binarycodec/types/test_number.py
[error] 8-8: Error in test_serialization_and_deserialization: TypeError occurred during test execution.
xrpl/models/transactions/vault_clawback.py
[warning] 1-1: D200 One-line docstring should fit on one line with quotes
[warning] 36-36: W291 trailing whitespace
xrpl/core/binarycodec/types/number.py
[error] 87-87: TypeError: to_bytes() missing required argument 'byteorder' (pos 2)
🔇 Additional comments (23)
.ci-config/rippled.cfg (1)
199-199: Ensure supported configuration for new feature.Adding
SingleAssetVaultunder[features]is straightforward. However, as noted in line 91, standalone mode may not honor these flags. If you plan on using this feature in standalone mode, please validate that the enabling logic is recognized by your rippled version.Would you like a shell script to cross-check whether the feature is supported on your targeted rippled build?
xrpl/core/binarycodec/definitions/definitions.json (8)
2043-2052: ”Number” field creation.The “Number” field is added as an alias to the newly introduced type with
nth=1. Ensure consistency regarding bit size constraints when using “Number” in practice, as it differs from standard “UInt32” or “UInt64.”
2053-2062: New field “AssetAvailable”.Marking this as a “Number” with
nth=2is fine. Please confirm in your usage or docs that the “Number” type properly handles large integer or floating-point values as intended for this field.
2063-2072: New field “AssetMaximum”.Same consideration as “AssetAvailable.” If “AssetMaximum” can be quite large, the “Number” type must handle big mantissa/exponent pairs without overflow or precision loss.
2083-2092: New field “LossUnrealized”.As a “Number,” ensure that negative values are supported if “loss” can indeed be negative. If not, consider validating to ensure non-negative input.
2959-2959: New Ledger Entry Type “Vault”.Assigned ID 131. Check that no collisions exist with previously assigned ledger entry type codes.
3197-3202: New Transaction Types for Vault operations.The new codes (64, 65, 66, 67, 68, 69) appear unique. Ensure that any transaction processing logic (like
TransactionTypeenum usage) recognizes these new values and is tested accordingly.
3226-3226: Number type assigned ID=9.Declaring “Number” in the
TYPESset is consistent with the newly introduced serialization logic. Confirm everything referencing type 9 is fully implemented and tested.
1263-1272:❓ Verification inconclusive
Addition of VaultID field.
Declaring a new Hash256 field with
nth=35in the[FIELDS]array is valid. Verify that no collisions occur with existing fields using the same index, especially if similar sets of indices are reused for different data types.
🏁 Script executed:
#!/bin/bash # Verify no index collisions for VaultID by scanning "nth": 35 usage across the file rg '"nth": 35' -A 5 definitions.jsonLength of output: 95
I'll perform an additional check to see if the file exists at the expected path. Below is a shell script to search for the file relative to the repository root:
#!/bin/bash # Locate the definitions.json file and verify its path. find . -type f -name definitions.json | grep "xrpl/core/binarycodec/definitions/definitions.json"If the above script finds the file, please re-run the previous scan using the correct full path:
#!/bin/bash # Verify no index collisions for VaultID by scanning "nth": 35 usage in the file: rg '"nth": 35' -A 5 xrpl/core/binarycodec/definitions/definitions.jsonVaultID Field Addition – Manual Verification Required
- The addition of the VaultID field (with
nth: 35and typeHash256) appears valid provided there are no conflicts.- The initial automated scan couldn’t locate
definitions.jsonusing the relative path. Please verify the file exists atxrpl/core/binarycodec/definitions/definitions.jsonand confirm that no other field uses thenthindex 35.Please double-check the file location and ensure the intended consistency across the field indices.
xrpl/models/requests/account_objects.py (1)
39-39: Addition of VAULT type looks goodThis new enumeration value for
AccountObjectTypeallows retrieving vault objects associated with an account, which aligns with the Single Asset Vault (XLS-65d) feature being introduced.xrpl/core/binarycodec/types/__init__.py (1)
13-13: Appropriate exposure of the Number typeThe addition of the Number type to the module exports is correctly implemented, following the existing pattern for exposing binary codec types. This new type will be essential for handling numerical values in the Single Asset Vault implementation.
Also applies to: 36-36
xrpl/models/transactions/vault_delete.py (2)
20-21: Implementation of the vault_id field looks goodThe required
vault_idfield is properly defined with appropriate documentation.
23-26: Transaction type definition looks goodThe transaction_type is correctly set to TransactionType.VAULT_DELETE with appropriate initialization parameters.
CHANGELOG.md (1)
15-15: Appropriate changelog entryThe addition of "Support for Single Asset Vault (XLS-65d)" to the changelog correctly documents this new feature following the project's changelog format.
xrpl/asyncio/transaction/main.py (1)
483-487: LGTM! Good addition of VAULT_CREATE transaction type fee calculation.The addition of
TransactionType.VAULT_CREATEto the list of transaction types that require owner reserve fee is consistent with the existing pattern for similar transaction types (ACCOUNT_DELETE and AMM_CREATE).xrpl/models/transactions/types/transaction_type.py (1)
55-60: LGTM! Properly added new transaction types.The six new vault-related transaction types have been added correctly to the TransactionType enumeration, following the established pattern.
xrpl/models/requests/ledger_entry.py (3)
44-44: New LedgerEntryType for Single Asset Vault looks goodThe addition of a new enumeration value for the Single Asset Vault is appropriate and follows the established naming convention in the class.
319-320: Good documentation for vault_id fieldThe comment and field definition for the vault_id are clear and follow the established pattern. The comment explaining that Single Asset Vault ledger objects can only be retrieved by index is helpful for API users.
355-355: Error handling update completeThe addition of
self.vault_idto the query parameters list in_get_errorsensures proper validation for the newly added field.xrpl/models/transactions/vault_set.py (1)
21-39: VaultSet implementation looks goodThe transaction definition follows the established pattern for XRPL transaction types with appropriate attributes, typing, and documentation. Good job implementing the VaultSet transaction.
🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 31-31: W291 trailing whitespace
xrpl/models/transactions/vault_clawback.py (1)
17-43: VaultClawback implementation is well documentedThe implementation of the VaultClawback transaction includes comprehensive documentation that clearly explains the transaction's purpose, behavior with insufficient funds, and how it handles future fees or penalties. The required and optional fields are properly defined following the established pattern.
🧰 Tools
🪛 GitHub Actions: Unit test
[warning] 36-36: W291 trailing whitespace
xrpl/models/transactions/__init__.py (2)
99-104: LGTM! Vault transaction imports added correctlyThe new vault-related transaction models are properly imported and will be available for library users.
202-207: LGTM! Vault transaction classes added to all listThe vault-related transaction classes are correctly added to the
__all__list, making them accessible when importing fromxrpl.models.transactions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/core/binarycodec/types/test_number.py (1)
6-42: Consider adding more test coverageThe current tests provide good coverage but could be enhanced by adding:
- Tests for zero in scientific notation (e.g., "0e0")
- Tests for negative exponents in the extreme limits test
- Tests for error handling with invalid inputs
🧰 Tools
🪛 GitHub Actions: Unit test
[error] 38-38: AttributeError: 'Number' object has no attribute 'hex'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/core/binarycodec/types/test_number.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Unit test
tests/unit/core/binarycodec/types/test_number.py
[error] 38-38: AttributeError: 'Number' object has no attribute 'hex'
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
🔇 Additional comments (2)
tests/unit/core/binarycodec/types/test_number.py (2)
1-6: Good job with the test organization.The imports and test class declaration look correct. The test class follows proper unittest conventions.
7-34: Good comprehensive test coverage for serialization.Your test cases cover a good range of scenarios including positive/negative integers, decimals, and scientific notation values. Each assertion is clear and properly formatted.
|
Note: The integration tests will not pass until the C++ code is merged into the rippled:develop branch. After this, a new docker container needs to be generated for the said branch. |
| @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ### Added | |||
| - Improved validation for models to also check param types | |||
| - Support for Single Asset Vault (XLS-65d) | |||
There was a problem hiding this comment.
| - Support for Single Asset Vault (XLS-65d) | |
| - Support for `Single Asset Vault` (XLS-65d) |
| @@ -196,6 +196,7 @@ fixEnforceNFTokenTrustline | |||
| fixReducedOffersV2 | |||
| DeepFreeze | |||
| PermissionedDomains | |||
| SingleAssetVault | |||
There was a problem hiding this comment.
Is this all still a part of 2.3.0 (no new releases)?
| @@ -315,6 +316,8 @@ class LedgerEntry(Request, LookupByLedgerRequest): | |||
| payment_channel: Optional[str] = None | |||
There was a problem hiding this comment.
how come there is no Vault class definition in this file?
| VAULT_DEPOSIT = "VaultDeposit" | ||
| VAULT_DELETE = "VaultDelete" |
There was a problem hiding this comment.
tiny nit: flip these two for alphabetical order
High Level Overview of Change
CPP implementation for SAV: XRPLF/rippled#5224
CPP implementation of the
STNumbertype: https://github.com/XRPLF/rippled/pull/5121/filesXLS Specification for this feature: XRPLF/XRPL-Standards#192
Context of Change
New Transactions added in this PR:
Vault <Create, Set, Deposit, Withdraw, Clawback, Delete>
Type of Change
Did you update CHANGELOG.md?
Test Plan
I don't see the value of writing unit tests for the transaction models. All the
temcodes are caught in thepreflightmethod of the relevantTransactorclass. Additional latency to an already failing transaction should not affect user-experience.Transaction-model unit tests add unnecessary code bloat and duplication. Certain transactions (like the ones related to
XChainamendment) do not have unit tests associated with them. Furthermore,requestsalso do not have unit tests associated with their models.