Single Asset Vault XLS-65d #814
Conversation
…g serialization of Number need to be solved;
WalkthroughThis update introduces comprehensive support for Single Asset Vaults (XLS-65d) in the XRPL Python codebase. It adds new transaction models, request types, enums, and serialization logic for vault operations, along with corresponding integration and unit tests. Configuration, changelog, and binary codec definitions are updated to reflect the new feature. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant XRPL_Python
participant XRPL_Node
Client->>XRPL_Python: Submit VaultCreate transaction
XRPL_Python->>XRPL_Node: Broadcast VaultCreate
XRPL_Node-->>XRPL_Python: Confirm transaction
XRPL_Python-->>Client: Return transaction result
Client->>XRPL_Python: Query VaultInfo (by vault_id or owner+seq)
XRPL_Python->>XRPL_Node: Send vault_info request
XRPL_Node-->>XRPL_Python: Return vault details
XRPL_Python-->>Client: Return vault info
Client->>XRPL_Python: Submit VaultDeposit / VaultWithdraw / VaultSet / VaultClawback / VaultDelete
XRPL_Python->>XRPL_Node: Broadcast transaction
XRPL_Node-->>XRPL_Python: Confirm transaction
XRPL_Python-->>Client: Return transaction result
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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. |
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
Co-authored-by: Omar Khan <[email protected]>
|
@khancode I have addressed your comments. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
xrpl/models/transactions/vault_create.py (1)
17-19: Fix swapped constant values for domain_id and mptoken_metadata lengths.The constant values are swapped. Based on the spec and past review comments:
domain_idis a Hash256 field requiring exactly 64 hex characters (32 bytes)mptoken_metadatacan be up to 1024 bytes (2048 hex characters)Also, remove the leading underscore from
_VAULT_MAX_MPTOKEN_METADATA_LENGTHsince it's used in other modules.VAULT_MAX_DATA_LENGTH = 256 * 2 -VAULT_MAX_DOMAIN_ID_LENGTH = 1024 * 2 -_VAULT_MAX_MPTOKEN_METADATA_LENGTH = 32 * 2 +VAULT_MAX_DOMAIN_ID_LENGTH = 32 * 2 +VAULT_MAX_MPTOKEN_METADATA_LENGTH = 1024 * 2
🧹 Nitpick comments (3)
tests/integration/transactions/test_single_asset_vault.py (2)
34-39: Consider adding assertions after wallet funding.While the current implementation allows natural test failures per the learning about xrpl-py test patterns, adding simple assertions after funding would make test failures more explicit and easier to debug.
vault_owner = Wallet.create() await fund_wallet_async(vault_owner) +self.assertIsNotNone(vault_owner.address) issuer_wallet = Wallet.create() await fund_wallet_async(issuer_wallet) +self.assertIsNotNone(issuer_wallet.address)
168-168: Add newline at end of file.Python files should end with a newline character.
self.assertEqual(response.result["engine_result"], "tesSUCCESS") +xrpl/models/transactions/vault_create.py (1)
108-110: Simplify the domain_id error message.The error message is confusing by mentioning both "32 characters" and "64 hex characters". Since this is a hex-encoded Hash256 field, just mention the hex character requirement.
errors["domain_id"] = ( - "Invalid domain ID: Length must be 32 characters (64 hex characters)." + "Invalid domain ID: Length must be exactly 64 hex characters." )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tests/integration/transactions/test_single_asset_vault.py(1 hunks)xrpl/core/binarycodec/types/number.py(1 hunks)xrpl/models/amounts/__init__.py(2 hunks)xrpl/models/amounts/clawback_amount.py(1 hunks)xrpl/models/requests/vault_info.py(1 hunks)xrpl/models/transactions/clawback.py(2 hunks)xrpl/models/transactions/vault_clawback.py(1 hunks)xrpl/models/transactions/vault_create.py(1 hunks)xrpl/models/transactions/vault_set.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- xrpl/models/amounts/init.py
- xrpl/models/amounts/clawback_amount.py
🚧 Files skipped from review as they are similar to previous changes (3)
- xrpl/models/transactions/vault_set.py
- xrpl/models/requests/vault_info.py
- xrpl/models/transactions/vault_clawback.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
Learnt from: mvadari
PR: XRPLF/xrpl-py#765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/models/requests/vault_info.py:16-31
Timestamp: 2025-06-04T22:17:47.822Z
Learning: In the xrpl-py library, Request classes do not perform client-side validation on their contents. The library relies on the rippled node to return appropriate error codes for malformed requests, maintaining a consistent pattern across all Request models.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/core/binarycodec/types/number.py:77-97
Timestamp: 2025-03-06T22:45:59.640Z
Learning: The `to_bytes()` method in Python requires the `byteorder` parameter even for single-byte operations, despite endianness not functionally affecting single-byte representation. The XRPL Python library implementation of serialization methods like `add32` and `add64` need to specify this parameter.
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
xrpl/models/transactions/clawback.py (8)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/models/amounts/mpt_amount.py:54-64
Timestamp: 2025-06-04T22:17:47.164Z
Learning: For MPTIssue class in xrpl-py, the user ckeshava confirmed that a simple class implementation without BaseModel inheritance works correctly for their current requirements, even though other currency types in the same Union (IssuedCurrency, MPTCurrency, XRP) extend BaseModel.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Learnt from: mvadari
PR: XRPLF/xrpl-py#765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:36-38
Timestamp: 2024-10-30T20:36:02.813Z
Learning: In `xrpl/models/transactions/credential_delete.py`, changing required field declarations from `credential_type: str = REQUIRED # type: ignore` to `credential_type: str = field(default=REQUIRED)` can cause mypy errors due to type incompatibility. It's acceptable to use `# type: ignore` in such cases in the `xrpl-py` codebase.
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_accept.py:27-39
Timestamp: 2024-10-30T20:32:03.246Z
Learning: In the `xrpl` codebase, when defining required fields in dataclasses (e.g., `account: str = REQUIRED`), it's necessary to include `# type: ignore` to prevent `mypy` errors.
tests/integration/transactions/test_single_asset_vault.py (10)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:61-63
Timestamp: 2024-11-01T16:20:50.192Z
Learning: In integration tests for xrpl-py, tests should only be testing the library, not rippled functionalities.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#773
File: tests/integration/reusable_values.py:112-119
Timestamp: 2025-01-15T00:41:02.631Z
Learning: In xrpl-py integration tests, errors during test setup should be allowed to terminate the test suite naturally to aid debugging, rather than being caught and wrapped in custom error handlers. This applies to functions like `sign_and_reliable_submission_async` which already have robust error handling.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:13-22
Timestamp: 2024-11-01T20:41:08.207Z
Learning: In Python test files in the xrpl-py codebase (e.g., `tests/unit/models/transactions/test_credential_create.py`), docstrings are not required for test classes and methods if their names are self-explanatory enough.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:37-108
Timestamp: 2024-10-30T20:28:45.649Z
Learning: In integration tests, setup and teardown are performed using the `CredentialCreate` and `CredentialDelete` transactions, and we prefer not to abstract these transactions into separate utility functions.
Learnt from: mvadari
PR: XRPLF/xrpl-py#765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#758
File: xrpl/transaction/__init__.py:8-8
Timestamp: 2024-10-17T17:45:46.828Z
Learning: In the xrpl-py project, importing private functions (indicated by a leading underscore) for integration testing purposes is acceptable when the methods are used internally.
xrpl/models/transactions/vault_create.py (27)
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: mvadari
PR: XRPLF/xrpl-py#765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_create.py:61-71
Timestamp: 2024-10-30T20:02:17.987Z
Learning: In `xrpl/models/transactions/credential_create.py`, the `uri` field in `CredentialCreate` is stored as a `Blob` in `rippled` and must be base-16 encoded as per the serialization requirements.
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Learnt from: mvadari
PR: XRPLF/xrpl-py#773
File: xrpl/models/transactions/permissioned_domain_set.py:22-22
Timestamp: 2024-11-20T21:16:43.491Z
Learning: In the `xrpl.models.transactions.PermissionedDomainSet` class, the `domain_id` field is optional in the `PermissionedDomainSet` transaction.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:130-136
Timestamp: 2025-02-06T18:41:24.232Z
Learning: In Python, use the bitwise AND operator (`&`) to check if a specific flag is set in a flags value, not the bitwise OR operator (`|`). For example, to check if `TF_SET_DEEP_FREEZE` flag is set: `flags & TrustSetFlag.TF_SET_DEEP_FREEZE`.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:11:19.326Z
Learning: In XRPL's binary codec format, large unsigned integers (UINT128, UINT160, UINT192, UINT256) should be consistently treated as Hash types, while smaller UINTs are translated to UInt types. This is reflected in generate_tx_models.py where they are mapped to "str" type.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl-py#840
File: xrpl/core/binarycodec/definitions/definitions.json:2194-2202
Timestamp: 2025-05-12T04:26:03.639Z
Learning: Serialization of the `Permission` STObject in XRPL binary codec works properly as verified by integration tests with actual transaction submission, even though it may not follow the typical pattern of explicitly updating OBJECT_END_MARKER mappings in st_object.py.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/models/requests/vault_info.py:16-31
Timestamp: 2025-06-04T22:17:47.822Z
Learning: In the xrpl-py library, Request classes do not perform client-side validation on their contents. The library relies on the rippled node to return appropriate error codes for malformed requests, maintaining a consistent pattern across all Request models.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/core/binarycodec/types/number.py:77-97
Timestamp: 2025-03-06T22:45:59.640Z
Learning: The `to_bytes()` method in Python requires the `byteorder` parameter even for single-byte operations, despite endianness not functionally affecting single-byte representation. The XRPL Python library implementation of serialization methods like `add32` and `add64` need to specify this parameter.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#758
File: xrpl/transaction/__init__.py:8-8
Timestamp: 2024-10-17T17:45:46.828Z
Learning: In the xrpl-py project, importing private functions (indicated by a leading underscore) for integration testing purposes is acceptable when the methods are used internally.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl-py#803
File: tests/integration/transactions/test_trust_set.py:0-0
Timestamp: 2025-02-05T21:42:42.425Z
Learning: Failure cases for deep freeze functionality (e.g., setting deep freeze without regular freeze, clearing deep freeze while regular freeze is set) are handled by rippled server logic and don't need to be tested in the xrpl-py library.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: tests/integration/transactions/test_sav.py:71-77
Timestamp: 2025-03-06T22:47:16.581Z
Learning: For test files in the xrpl-py repository, exception handling is considered not critical, and the preference is to let tests fail with natural errors rather than adding explicit error handling.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:89-93
Timestamp: 2024-10-30T20:44:27.753Z
Learning: In the Python file `xrpl/models/transactions/deposit_preauth.py`, within the `_validate_credentials_length` function, when validating `authorize_credentials` and `unauthorize_credentials`, it's acceptable to assign error messages to the same key in the `errors` dictionary if the conditions are mutually exclusive, as the error messages won't overwrite each other.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:11:19.326Z
Learning: In XRPL's binary codec format, the UINT192 type from the MPT amendment is translated to UInt192, following the same pattern as other non-hash UINT types.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_accept.py:27-39
Timestamp: 2024-10-30T20:32:03.246Z
Learning: In the `xrpl` codebase, when defining required fields in dataclasses (e.g., `account: str = REQUIRED`), it's necessary to include `# type: ignore` to prevent `mypy` errors.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:36-38
Timestamp: 2024-10-30T20:36:02.813Z
Learning: In `xrpl/models/transactions/credential_delete.py`, changing required field declarations from `credential_type: str = REQUIRED # type: ignore` to `credential_type: str = field(default=REQUIRED)` can cause mypy errors due to type incompatibility. It's acceptable to use `# type: ignore` in such cases in the `xrpl-py` codebase.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/payment_channel_claim.py:0-0
Timestamp: 2024-11-01T18:32:18.324Z
Learning: The client library does not validate the expiration time of credentials, as this requires access to the latest validated ledger and is handled in `rippled`.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_accept.py:56-60
Timestamp: 2024-10-30T20:36:47.418Z
Learning: It's acceptable to refer to length in bytes when measuring ASCII strings since each ASCII character is 1 byte.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl-py#840
File: xrpl/core/binarycodec/definitions/definitions.json:2594-2602
Timestamp: 2025-05-13T18:43:34.412Z
Learning: The `definitions.json` file in XRPL is a generated file and should not be directly edited. Constraints like maximum array lengths are enforced at the model level (e.g., in transaction models) rather than in the binary codec layer.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#824
File: tools/fetch_rippled_amendments.py:0-0
Timestamp: 2025-04-07T23:32:27.184Z
Learning: For CI/CD scripts in the xrpl-py repository, ckeshava prefers concise code over comprehensive error handling since developers can check the logs for error details.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/requests/test_ledger_entry.py:29-36
Timestamp: 2024-12-12T00:36:13.410Z
Learning: For basic dataclass testing in xrpl-py, simple initialization with sample values is sufficient when the main goal is to verify the validation logic works. Extensive property verification and negative test cases are unnecessary.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/models/amounts/mpt_amount.py:54-64
Timestamp: 2025-06-04T22:17:47.164Z
Learning: For MPTIssue class in xrpl-py, the user ckeshava confirmed that a simple class implementation without BaseModel inheritance works correctly for their current requirements, even though other currency types in the same Union (IssuedCurrency, MPTCurrency, XRP) extend BaseModel.
xrpl/core/binarycodec/types/number.py (12)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:11:19.326Z
Learning: In XRPL's binary codec format, large unsigned integers (UINT128, UINT160, UINT192, UINT256) should be consistently treated as Hash types, while smaller UINTs are translated to UInt types. This is reflected in generate_tx_models.py where they are mapped to "str" type.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/core/binarycodec/types/number.py:77-97
Timestamp: 2025-03-06T22:45:59.640Z
Learning: The `to_bytes()` method in Python requires the `byteorder` parameter even for single-byte operations, despite endianness not functionally affecting single-byte representation. The XRPL Python library implementation of serialization methods like `add32` and `add64` need to specify this parameter.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#772
File: tools/generate_definitions.py:0-0
Timestamp: 2025-02-11T21:11:19.326Z
Learning: In XRPL's binary codec format, the UINT192 type from the MPT amendment is translated to UInt192, following the same pattern as other non-hash UINT types.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/core/binarycodec/types/number.py:77-97
Timestamp: 2025-03-06T22:51:08.981Z
Learning: Python's to_bytes() method officially requires the byteorder parameter even for single-byte operations where endianness doesn't functionally matter. Some Python environments might accept code without this parameter, but for maximum compatibility across all environments including CI pipelines, the byteorder parameter should always be included.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/core/binarycodec/types/number.py:77-97
Timestamp: 2025-03-06T22:51:08.981Z
Learning: In Python 3.11.2, the `to_bytes()` method has 'big' as a default value for the `byteorder` parameter, making it optional. However, in some Python versions and environments (like certain CI pipelines), this parameter might still be strictly required. For maximum compatibility, it's recommended to always explicitly include the `byteorder` parameter in `to_bytes()` calls.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/core/binarycodec/types/number.py:77-97
Timestamp: 2025-03-06T22:51:08.981Z
Learning: Python's to_bytes() method behavior may be inconsistent across different environments, with some allowing omission of the byteorder parameter for single-byte operations, but the official specification requires it as a mandatory positional parameter.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#814
File: xrpl/core/binarycodec/types/number.py:59-75
Timestamp: 2025-03-06T22:45:41.790Z
Learning: In the XRP Ledger Python implementation, when serializing single bytes with `.to_bytes(1)`, specifying the byteorder parameter is not necessary as endianness only matters for multiple bytes. The manual bit shifting and appending individual bytes already implements the correct byte ordering.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: mvadari
PR: XRPLF/xrpl-py#765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#824
File: tools/fetch_rippled_amendments.py:0-0
Timestamp: 2025-04-07T23:32:27.184Z
Learning: For CI/CD scripts in the xrpl-py repository, ckeshava prefers concise code over comprehensive error handling since developers can check the logs for error details.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:36-38
Timestamp: 2024-10-30T20:36:02.813Z
Learning: In `xrpl/models/transactions/credential_delete.py`, changing required field declarations from `credential_type: str = REQUIRED # type: ignore` to `credential_type: str = field(default=REQUIRED)` can cause mypy errors due to type incompatibility. It's acceptable to use `# type: ignore` in such cases in the `xrpl-py` codebase.
🧬 Code Graph Analysis (2)
xrpl/models/transactions/vault_create.py (4)
xrpl/models/flags.py (1)
FlagInterface(13-16)xrpl/models/transactions/transaction.py (1)
Transaction(184-551)xrpl/models/transactions/types/transaction_type.py (1)
TransactionType(6-70)xrpl/models/utils.py (1)
require_kwargs_on_init(110-163)
xrpl/core/binarycodec/types/number.py (3)
xrpl/core/binarycodec/binary_wrappers/binary_parser.py (2)
BinaryParser(31-261)read(69-81)xrpl/core/binarycodec/exceptions.py (1)
XRPLBinaryCodecException(6-9)xrpl/core/binarycodec/types/serialized_type.py (1)
SerializedType(15-100)
🪛 Pylint (3.3.7)
xrpl/core/binarycodec/types/number.py
[refactor] 99-99: Too few public methods (0/2)
(R0903)
🔇 Additional comments (9)
xrpl/core/binarycodec/types/number.py (8)
1-14: LGTM! Clean imports and documentation.The file header provides good context about the rippled C++ implementation reference, and the imports are appropriate for the functionality.
16-24: LGTM! Well-defined constants.The constants are appropriately named and documented. The special
_DEFAULT_VALUE_EXPONENTvalue for zero representation aligns with the rippled implementation.
26-56: LGTM! Robust normalization logic.The normalize function correctly handles:
- Mantissa scaling within defined bounds
- Proper error handling for values that exceed limits
- Sign preservation
- Towards-zero rounding behavior (as documented)
The implementation matches the described behavior from the rippled reference.
59-96: LGTM! Serialization helpers properly implemented.Both
add32andadd64correctly serialize integers to big-endian byte representation. The byteorder parameter has been properly added to allto_bytes()calls, addressing the previous review feedback.
99-113: LGTM! Simple and effective data class.The
NumberPartsclass serves its purpose as a data container. The static analysis warning about too few public methods is a false positive since this is intentionally a simple data class.
115-171: LGTM! Comprehensive number parsing with good error handling.The regex pattern correctly handles:
- Optional signs
- Integer and fractional parts
- Scientific notation with optional exponent signs
- Proper validation of number format
The error message includes the input string (addressing previous feedback), and the parsing logic correctly extracts mantissa, exponent, and sign components.
174-233: LGTM! Well-structured Number class with proper serialization.The
from_valueandfrom_parsermethods correctly implement the SerializedType interface. The special handling for zero values with_DEFAULT_VALUE_EXPONENTmaintains compatibility with the rippled implementation. The 12-byte format (8 bytes mantissa + 4 bytes exponent) is properly validated.
234-285: LGTM! Comprehensive JSON conversion logic.The
to_jsonmethod handles all the different number representation cases:
- Direct integer representation when exponent is 0
- Special zero value handling
- Scientific notation for very large/small numbers
- Proper decimal formatting for the middle range
- Correct sign handling and padding logic
The implementation faithfully follows the rippled reference for API compatibility.
xrpl/models/transactions/clawback.py (1)
6-10: Clean refactoring to use the new ClawbackAmount type.The change simplifies the imports and type annotation by using the new
ClawbackAmounttype alias, which properly encapsulates the allowed amount types for clawback operations (IssuedCurrencyAmount and MPTAmount, excluding XRP).Also applies to: 23-23
| ) | ||
| self.assertEqual(len(account_objects_response.result["account_objects"]), 1) | ||
|
|
||
| VAULT_ID = account_objects_response.result["account_objects"][0]["index"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for vault existence before extracting VAULT_ID.
The code assumes that the vault exists in the account objects without validation.
-VAULT_ID = account_objects_response.result["account_objects"][0]["index"]
+account_objects = account_objects_response.result.get("account_objects", [])
+self.assertGreater(len(account_objects), 0, "No vault found in account objects")
+VAULT_ID = account_objects[0]["index"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| VAULT_ID = account_objects_response.result["account_objects"][0]["index"] | |
| account_objects = account_objects_response.result.get("account_objects", []) | |
| self.assertGreater(len(account_objects), 0, "No vault found in account objects") | |
| VAULT_ID = account_objects[0]["index"] |
🤖 Prompt for AI Agents
In tests/integration/transactions/test_single_asset_vault.py at line 104, the
code directly accesses the first account object to extract VAULT_ID without
checking if any account objects exist. Add a validation step to confirm that the
"account_objects" list is not empty before accessing its first element. If the
list is empty, handle the case appropriately, such as raising an error or
skipping the test, to prevent runtime exceptions.
There was a problem hiding this comment.
The existence is guaranteed due to a successful account_objects API response. The success is verified based on the length of the array in the response.result.
There was a problem hiding this comment.
@ckeshava You're absolutely right! I apologize for overlooking the existing validation. The assertion self.assertEqual(len(account_objects_response.result["account_objects"]), 1) already ensures that exactly one vault object exists in the response. If this assertion passes, the subsequent access to account_objects[0]["index"] is guaranteed to be safe.
The existing validation is indeed sufficient for this test case, and my original suggestion was unnecessary. Thank you for the clarification!
🧠 Learnings used
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/requests/test_deposit_authorized.py:7-15
Timestamp: 2024-11-01T18:53:01.394Z
Learning: In `tests/unit/models/requests/test_deposit_authorized.py`, additional tests for invalid credentials are unnecessary because `rippled` handles those checks, and the `xrpl-py` library does not include such checks.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:31-37
Timestamp: 2024-11-04T19:41:04.808Z
Learning: The `is_cred_object_present` function in `tests/integration/transactions/test_credential.py` assumes that the fields `'Issuer'`, `'Subject'`, and `'CredentialType'` are always present in the account object dictionaries within `account_objects`.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#773
File: tests/integration/reusable_values.py:112-119
Timestamp: 2025-01-15T00:41:02.631Z
Learning: In xrpl-py integration tests, errors during test setup should be allowed to terminate the test suite naturally to aid debugging, rather than being caught and wrapped in custom error handlers. This applies to functions like `sign_and_reliable_submission_async` which already have robust error handling.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:61-63
Timestamp: 2024-11-01T16:20:50.192Z
Learning: In integration tests for xrpl-py, tests should only be testing the library, not rippled functionalities.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/integration/transactions/test_credential.py:42-44
Timestamp: 2024-10-30T21:01:15.823Z
Learning: In this codebase, extensive unit tests are located in `rippled`, and additional tests are not required in this library.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#758
File: xrpl/transaction/__init__.py:8-8
Timestamp: 2024-10-17T17:45:46.828Z
Learning: In the xrpl-py project, importing private functions (indicated by a leading underscore) for integration testing purposes is acceptable when the methods are used internally.
Learnt from: mvadari
PR: XRPLF/xrpl-py#765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:13-22
Timestamp: 2024-11-01T20:41:08.207Z
Learning: In Python test files in the xrpl-py codebase (e.g., `tests/unit/models/transactions/test_credential_create.py`), docstrings are not required for test classes and methods if their names are self-explanatory enough.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
| if self.mptoken_metadata is not None and len(self.mptoken_metadata) > ( | ||
| _VAULT_MAX_MPTOKEN_METADATA_LENGTH | ||
| ): |
There was a problem hiding this comment.
Update validation to use the corrected constant name.
After fixing the constant name, update the validation code.
- if self.mptoken_metadata is not None and len(self.mptoken_metadata) > (
- _VAULT_MAX_MPTOKEN_METADATA_LENGTH
- ):
+ if self.mptoken_metadata is not None and len(self.mptoken_metadata) > VAULT_MAX_MPTOKEN_METADATA_LENGTH:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.mptoken_metadata is not None and len(self.mptoken_metadata) > ( | |
| _VAULT_MAX_MPTOKEN_METADATA_LENGTH | |
| ): | |
| if self.mptoken_metadata is not None and len(self.mptoken_metadata) > VAULT_MAX_MPTOKEN_METADATA_LENGTH: |
🤖 Prompt for AI Agents
In xrpl/models/transactions/vault_create.py around lines 97 to 99, the
validation uses an incorrect constant name _VAULT_MAX_MPTOKEN_METADATA_LENGTH.
Update this to use the corrected constant name as defined elsewhere in the
codebase to ensure proper validation of mptoken_metadata length.
There was a problem hiding this comment.
The existing validation is correct. _VAULT_MAX_MPTOKEN_METADATA_LENGTH is the correct name. The Github CI has not reported any errors.
There was a problem hiding this comment.
@ckeshava You're absolutely right, and I apologize for the confusion. The constant _VAULT_MAX_MPTOKEN_METADATA_LENGTH is correctly defined and used in the validation code. The underscore prefix appropriately indicates it's a private constant, and the validation logic is working as intended. Thank you for the correction!
(\ /)
( ._. )
o_(")(")
bows humbly
🧠 Learnings used
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:70-76
Timestamp: 2024-12-10T20:35:09.762Z
Learning: In the `xrpl/models/transactions/mptoken_issuance_create.py` file, the `maximum_amount` field in the `MPTokenIssuanceCreate` class is a non-negative integer string and should be typed as `Optional[str]`.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Learnt from: mvadari
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_account_delete.py:52-60
Timestamp: 2024-12-12T00:48:38.042Z
Learning: The credential ID format validation for xrpl-py is being tracked in issue #766 and should not be duplicated in other PRs.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/deposit_preauth.py:89-93
Timestamp: 2024-10-30T20:44:27.753Z
Learning: In the Python file `xrpl/models/transactions/deposit_preauth.py`, within the `_validate_credentials_length` function, when validating `authorize_credentials` and `unauthorize_credentials`, it's acceptable to assign error messages to the same key in the `errors` dictionary if the conditions are mutually exclusive, as the error messages won't overwrite each other.
Learnt from: mvadari
PR: XRPLF/xrpl-py#765
File: xrpl/models/transactions/xchain_add_claim_attestation.py:82-88
Timestamp: 2024-11-01T20:24:17.202Z
Learning: Changing the type of `was_locking_chain_send` in `xrpl/models/transactions/xchain_add_claim_attestation.py` to `bool` is out of scope for the current PR.
Learnt from: khancode
PR: XRPLF/xrpl-py#732
File: xrpl/models/transactions/mptoken_issuance_create.py:78-84
Timestamp: 2024-12-11T01:47:28.074Z
Learning: In the `xrpl-py` codebase, within the models (e.g., in `xrpl/models/transactions/`), flag validation checks are not performed; flag checks are handled elsewhere outside of the models.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:36-38
Timestamp: 2024-10-30T20:36:02.813Z
Learning: In `xrpl/models/transactions/credential_delete.py`, changing required field declarations from `credential_type: str = REQUIRED # type: ignore` to `credential_type: str = field(default=REQUIRED)` can cause mypy errors due to type incompatibility. It's acceptable to use `# type: ignore` in such cases in the `xrpl-py` codebase.
Learnt from: Patel-Raj11
PR: XRPLF/xrpl-py#840
File: xrpl/core/binarycodec/definitions/definitions.json:2594-2602
Timestamp: 2025-05-13T18:43:34.412Z
Learning: The `definitions.json` file in XRPL is a generated file and should not be directly edited. Constraints like maximum array lengths are enforced at the model level (e.g., in transaction models) rather than in the binary codec layer.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: tests/unit/models/transactions/test_credential_create.py:107-114
Timestamp: 2024-10-30T20:26:32.925Z
Learning: In `tests/unit/models/transactions/test_credential_create.py`, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.
Learnt from: ckeshava
PR: XRPLF/xrpl-py#759
File: xrpl/models/transactions/credential_delete.py:57-68
Timestamp: 2024-10-30T20:34:35.451Z
Learning: Consistent implementation patterns are preferred in the `xrpl-py` codebase, especially in transaction models under `xrpl/models/transactions/`. When suggesting refactoring that affects multiple transactions, consider the impact on overall consistency and propose comprehensive changes when appropriate.
|
thank you for the reviews _/ \_ @khancode @Patel-Raj11 |
5 participants
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
Please view the updates to the specification in this PR: https://github.com/XRPLF/XRPL-Standards/pull/275/files
Context of Change
This PR adds the following transactions:
Vault<Create, Set, Deposit, Withdraw, Clawback, Delete>This PR also adds the models and the unit tests for the
VaultInfoRPC. This is the reference commit: (a1073b3).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.