feat: add support for Batch amendment #757
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes involve multiple updates across various files in the XRPL framework. Key modifications include the addition of new transaction types and models for batch processing and ledger state fixes, enhancements to testing capabilities for batch transactions, and updates to the spell-checking configuration. Import paths have been restructured for clarity, and several methods have been added or modified to improve functionality and type safety. Overall, these changes expand the framework's capabilities and improve its robustness. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration 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: 0
🧹 Outside diff range and nitpick comments (13)
xrpl/models/transactions/ledger_state_fix.py (1)
19-25: LGTM: Attributes are well-defined. Consider adding more documentation.The class attributes are appropriately defined with correct types and default values. The use of
REQUIREDforledger_fix_typeand the optionalownerattribute provide necessary flexibility.Consider adding a comment or expanding the class docstring to explain the possible values and significance of the
ledger_fix_typeattribute. This would enhance the usability of the class for developers.tools/generate_tx_models.py (1)
188-189: Good addition of argument handling. Consider a minor improvement.The addition of argument checking and a usage message is a valuable improvement to the script's usability and error prevention. It aligns well with best practices for command-line tools.
Consider updating the usage message to be more explicit about the required argument:
print("Usage: poetry run python generate_tx_models.py <path_to_rippled_source>")This minor change makes it even clearer what the expected argument represents.
tests/unit/models/transactions/test_transaction.py (1)
162-171: LGTM: New test for multisigned transactions with X-addresses.This new test method effectively verifies the functionality of multisigning transactions using X-addresses, which aligns with the PR objectives.
Suggestion for improvement:
Consider adding assertions to verify the content of the multisigned transaction, not just that it's signed. For example, you could check that theAccountandAuthorizefields in the final transaction contain the correct X-addresses.xrpl/core/binarycodec/definitions/definitions.json (7)
383-392: LGTM: New LedgerFixType field addedThe addition of the "LedgerFixType" field of type UInt16 is appropriate for supporting the new LedgerStateFix transaction type. This allows for a wide range of potential ledger fix types (up to 65,535).
Consider adding documentation or comments explaining the purpose of this field and any predefined LedgerFixType values, if applicable.
2003-2012: LGTM: New InnerResult field addedThe addition of the "InnerResult" field of type Blob is appropriate for storing results of individual transactions within a Batch transaction. The Blob type provides flexibility for various result data.
Consider adding documentation or comments explaining the purpose and expected content of this field, especially in the context of Batch transactions.
2173-2182: LGTM: New OuterAccount field addedThe addition of the "OuterAccount" field of type AccountID is appropriate for identifying the account initiating a Batch transaction.
Consider adding documentation or comments explaining the specific role and usage of the OuterAccount field in the context of Batch transactions.
2223-2232: LGTM: New TxIDs field addedThe addition of the "TxIDs" field of type Vector256 is appropriate for storing transaction IDs within a Batch transaction. This allows for efficient storage and retrieval of multiple transaction hashes.
Consider adding documentation or comments explaining the purpose and usage of the TxIDs field, particularly its relationship to Batch transactions and any limitations on the number of transaction IDs that can be stored.
2931-2931: LGTM: New transaction types and results addedThe additions of new transaction types (LedgerStateFix and Batch) and transaction results (temINVALID_BATCH and tecBATCH_FAILURE) are consistent with the PR objectives. The numbering follows the existing pattern and doesn't conflict with other types/results.
Consider adding documentation or comments explaining:
- The purpose and use cases for the LedgerStateFix transaction type.
- The conditions under which temINVALID_BATCH and tecBATCH_FAILURE results would occur.
- Any specific requirements or limitations for Batch transactions.
Also applies to: 2954-2954, 3048-3049, 3100-3101
2603-2642: LGTM: New STObject and STArray fields added for Batch transactionsThe addition of new STObject fields (RawTransaction, BatchExecution, BatchTxn, BatchSigner) and STArray fields (BatchExecutions, RawTransactions, BatchSigners) is consistent with the PR objectives for adding Batch transaction support. The naming is clear and follows existing conventions.
Consider adding documentation or comments explaining:
- The purpose and contents of each new field.
- The relationships between these fields (e.g., how BatchExecutions relates to BatchExecution).
- Any constraints or requirements for these fields in the context of Batch transactions.
Also applies to: 2833-2861
Line range hint
1-3101: Overall changes look good, but documentation is neededThe additions and modifications to the definitions.json file are consistent with the PR objectives of adding support for Batch transactions and LedgerStateFix functionality. The new fields, transaction types, and results are appropriately structured and follow existing patterns.
However, there's a general lack of documentation for these new elements. To improve maintainability and ease of use, consider the following suggestions:
- Add a comment section at the beginning of the file explaining the overall changes introduced in this update.
- For each new field, transaction type, and result, add inline comments explaining their purpose, usage, and any constraints or requirements.
- If there are any relationships or dependencies between the new elements (e.g., how BatchExecutions relates to BatchExecution), document these connections.
- Consider creating or updating separate documentation that provides a more detailed explanation of the Batch transaction feature and the LedgerStateFix functionality.
xrpl/models/transactions/batch.py (2)
20-26: Consider adding docstrings for class attributesAdding docstrings to the
BatchSignerclass attributes can improve code readability and maintainability by providing clear descriptions of each field.
34-36: Consider adding docstrings for class attributesAdding docstrings to the
Batchclass attributes can enhance clarity by explaining the purpose and usage of each field.tests/integration/sugar/test_transaction.py (1)
266-267: Use constants for account addresses to improve maintainabilityHardcoding account addresses directly in the test code reduces maintainability and can lead to potential issues if the addresses need to be updated. Consider defining these addresses as constants at the top of the test module or using existing test account variables (e.g.,
ACCOUNTorDESTINATION). This will make the code cleaner and more consistent.Also applies to: 270-271, 274-275, 286-287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- .vscode/settings.json (1 hunks)
- tests/integration/sugar/test_transaction.py (2 hunks)
- tests/unit/models/transactions/test_transaction.py (5 hunks)
- tools/generate_tx_models.py (5 hunks)
- xrpl/asyncio/transaction/main.py (8 hunks)
- xrpl/core/binarycodec/definitions/definitions.json (11 hunks)
- xrpl/models/transactions/init.py (4 hunks)
- xrpl/models/transactions/batch.py (1 hunks)
- xrpl/models/transactions/ledger_state_fix.py (1 hunks)
- xrpl/models/transactions/transaction.py (3 hunks)
- xrpl/models/transactions/types/transaction_type.py (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🧰 Additional context used
🪛 Gitleaks
tests/integration/sugar/test_transaction.py
270-270: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
274-274: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (30)
xrpl/models/transactions/ledger_state_fix.py (3)
1-11: LGTM: File structure and imports are well-organized.The file structure is clean, with a clear module-level docstring and appropriate imports. The use of
from __future__ import annotationsis a good practice for forward compatibility with type annotations.
14-17: LGTM: Class definition and decorators are well-implemented.The
LedgerStateFixclass is properly defined as a frozen dataclass, inheriting fromTransaction. The use of@require_kwargs_on_initandKW_ONLY_DATACLASSensures consistency with other XRPL models and enforces clear instantiation practices.
1-25: LGTM: Overall implementation is solid and consistent with XRPL patterns.The
LedgerStateFixtransaction type is well-implemented, following XRPL conventions and Python best practices. The file is focused, concise, and maintains consistency with other transaction types in the XRPL framework.xrpl/models/transactions/types/transaction_type.py (2)
17-17: LGTM: Addition of BATCH transaction typeThe addition of
BATCH = "Batch"to theTransactionTypeenum is consistent with the PR objective of introducing support for Batch transactions. The naming convention and placement within the enum are appropriate.
28-28: Approved: Addition of LEDGER_STATE_FIX transaction type, but clarification neededThe addition of
LEDGER_STATE_FIX = "LedgerStateFix"to theTransactionTypeenum is consistent with the naming conventions and placement within the enum. However, this transaction type was not explicitly mentioned in the PR objectives.Could you please provide some context on the purpose of this new transaction type and how it relates to the overall objectives of this PR? This information would be helpful for understanding the full scope of the changes.
xrpl/models/transactions/__init__.py (2)
114-114: Updated all list with new transaction typesThe
__all__list has been correctly updated to includeBatchandLedgerStateFix, making these new classes part of the module's public API. This change is consistent with the existing pattern in the file and ensures that the new transaction types are accessible when importing from this module.Also applies to: 125-125
27-27: New transaction types added: Batch and LedgerStateFixThe addition of
BatchandLedgerStateFiximports aligns with the PR objectives of adding support for Batch transactions. TheLedgerStateFixaddition, while not explicitly mentioned in the PR objectives, appears to be a related enhancement to the framework's functionality.Let's verify the consistency of these additions with other files in the codebase:
Also applies to: 38-38
✅ Verification successful
Verification Successful: The additions of
BatchandLedgerStateFiximports inxrpl/models/transactions/__init__.pyare consistent with the project's structure and intended API enhancements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and usage of Batch and LedgerStateFix classes # Test 1: Check if the Batch and LedgerStateFix files exist echo "Checking for Batch and LedgerStateFix files:" fd -e py "batch|ledger_state_fix" xrpl/models/transactions # Test 2: Look for usage of Batch and LedgerStateFix in other files echo "Checking for usage of Batch and LedgerStateFix:" rg -t py "from xrpl\.models\.transactions import .*(?:Batch|LedgerStateFix)" --glob '!xrpl/models/transactions/__init__.py'Length of output: 469
tools/generate_tx_models.py (3)
134-134: Great addition of the @DataClass decorator!The addition of the
@dataclass(frozen=True, **KW_ONLY_DATACLASS)decorator is a valuable improvement. It simplifies class creation, enhances immutability, and likely enforces keyword-only arguments. These changes contribute to improved type safety, code robustness, and clarity, aligning well with the PR objectives.
40-40: LGTM. Verify the new file path.The update to the file path for
TxFormats.cppis consistent with the previous change, further confirming the restructuring of the source code to separate the XRPL protocol into its own library. This change aligns with the PR objectives.Please run the following script to verify the existence of the new file path:
#!/bin/bash # Verify the existence of the new TxFormats.cpp file fd -t f "TxFormats.cpp" | grep "src/libxrpl/protocol/TxFormats.cpp"
29-29: LGTM. Verify the new file path.The update to the file path for
SField.cppreflects a restructuring of the source code, which is in line with the PR objectives. This change suggests a more modular approach by separating the XRPL protocol into its own library.Please run the following script to verify the existence of the new file path:
tests/unit/models/transactions/test_transaction.py (4)
4-4: LGTM: New import for address conversion.The addition of
classic_address_to_xaddressimport is appropriate for the new test method that will be using X-addresses.
6-6: LGTM: Updated import for DepositPreauth transaction type.The inclusion of
DepositPreauthin the import statement is appropriate for the new test method that will be using this transaction type.
174-174: LGTM: Renamed test methods to follow Python naming conventions.The renaming of these test methods from camelCase to snake_case improves consistency with Python naming conventions (PEP 8). This change enhances code readability and maintainability.
Also applies to: 190-190, 206-206, 222-222
Line range hint
1-238: Summary: Valuable additions and improvements to transaction tests.The changes in this file effectively support the PR objectives:
- Added support for X-addresses in multisigned transactions.
- Expanded test coverage for Batch transactions.
- Improved code quality through consistent naming conventions.
These changes enhance the robustness of the XRPL Python library without breaking existing functionality. The new test for multisigned transactions with X-addresses is particularly valuable for ensuring the correct handling of these complex scenarios.
xrpl/core/binarycodec/definitions/definitions.json (1)
263-272: LGTM: New BatchIndex field addedThe addition of the "BatchIndex" field of type UInt8 is appropriate for supporting Batch transactions. This allows for indexing up to 255 items in a batch, which should be sufficient for most use cases.
xrpl/models/transactions/batch.py (2)
15-27: Well-definedBatchSignerclassThe
BatchSignerclass is correctly implemented with required and optional fields, following the codebase conventions.
29-41: Correct implementation of theBatchtransaction classThe
Batchclass extendsTransactionand appropriately defines required and optional fields, including setting thetransaction_typetoBATCH.tests/integration/sugar/test_transaction.py (1)
264-291: New test for batch autofill is comprehensive and correctThe
test_batch_autofillmethod effectively tests the autofilling of a batch transaction with multipleDepositPreauthtransactions. The assertions verify that the transaction IDs are correctly generated and that the batch transaction properties are as expected. This improves the test coverage for batch transactions.🧰 Tools
🪛 Gitleaks
270-270: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
274-274: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
xrpl/models/transactions/transaction.py (3)
430-434: Review the Updated Condition inget_hashMethod for CorrectnessThe
get_hashmethod now includesand self.batch_txn is Nonein its conditional check. This change implies that a transaction with abatch_txnis considered hashable even iftxn_signatureandsignersareNone. Please confirm that this aligns with the intended behavior for batch transactions and does not introduce security concerns by allowing unsigned transactions to be hashed.Run the following script to find all usages of
get_hashand ensure that the updated logic is compatible with how the method is utilized elsewhere in the codebase.#!/bin/bash # Description: Find all usages of `get_hash` method in the codebase. rg --type py '\.get_hash\(' -A 2 -B 2
267-268: Ensure Backward Compatibility with Addedbatch_txnFieldThe addition of the optional
batch_txnfield to theTransactionclass could affect serialization and deserialization processes. Please verify that transactions without this field are still correctly handled, maintaining backward compatibility with existing transactions.Run the following script to identify how
Transactionserialization and deserialization are handled across the codebase and ensure they account for the newbatch_txnfield appropriately.#!/bin/bash # Description: Find all instances of Transaction serialization/deserialization methods. rg --type py 'Transaction\.(from_dict|to_dict|from_xrpl|to_xrpl|from_blob|blob)\(' -A 2 -B 2
156-168: Question About the Necessity of# type: ignoreCommentsThe fields
outer_accountandbatch_indexin theBatchTxnclass are marked with# type: ignore. Could you verify if these comments are necessary? If they suppress type-checking errors due to the use ofREQUIRED, there might be a more elegant solution to handle the type annotations without ignoring type checks.Run the following script to find all instances where
REQUIREDfields are annotated with# type: ignoreto assess if this pattern is used consistently and determine if it's essential.✅ Verification successful
# type: ignoreComments forREQUIREDFields are NecessaryThe
# type: ignorecomments used withREQUIREDfields are essential to suppress type-checking errors caused by theREQUIREDsentinel value not matching the declared field types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances of REQUIRED fields with `# type: ignore`. rg --type py '=\s*REQUIRED\s*#\s*type:\s*ignore'Length of output: 1978
Script:
#!/bin/bash # Description: Find all instances of REQUIRED fields annotated with `# type: ignore`. rg --type py '=\s*REQUIRED\s*#\s*type:\s*ignore'Length of output: 17279
xrpl/asyncio/transaction/main.py (9)
200-202: Prevent direct signing of batch inner transactionsThe added check correctly raises an exception when attempting to sign a batch inner transaction directly, ensuring that only appropriate transactions are signed.
221-223: Enhance type safety with TypeVarTIntroducing the type variable
Tbounded toTransactionimproves type annotations and ensures that functions return the same subtype ofTransactionthat is passed in.
225-226: Updateautofillfunction signatureThe
autofillfunction now utilizes the type variableT, allowing it to return the same specific transaction type as the input. This enhances type safety and consistency.
256-261: Handle batch transactions inautofillThe added code correctly checks if the transaction is of type
BATCHand processes it using the_autofill_batchfunction. This ensures that batch transactions are properly autofilled.
392-392: Convert X-Addresses to classic addressesThe code ensures that if the specified field contains an X-Address, it is converted to a classic address. This maintains consistency in address formats throughout the transaction.
501-533: Implement_autofill_batchfunction for batch transactionsThe new
_autofill_batchfunction properly handles the autofilling of batch transactions. It assigns sequence numbers, manages batch indices, and computes transaction IDs for each inner transaction, ensuring correct processing of batch transactions.
516-523: Ensure correct sequence number assignment for multiple accountsThe sequence number assignment logic handles multiple transactions from potentially different accounts. Verify that the sequence number incrementation accurately reflects the state of each account, preventing sequence number collisions or gaps.
To verify that sequence numbers are correctly assigned per account, consider the following script:
#!/bin/bash # Description: Check for sequence number consistency across accounts in batch transactions. # List accounts and their assigned sequences in batch transactions rg --type python 'account_sequences\[\s*raw_txn\.account\s*\]' -A 5This script helps ensure that the
account_sequencesdictionary correctly tracks and increments sequence numbers for each account.
527-529: Confirm transaction reconstruction with updatedbatch_txnAfter adding the
batch_txnfield toraw_txn_dict, a newTransactionobject is created usingTransaction.from_dict(). Verify that this method accurately reconstructs the transaction with all necessary fields intact.You can run the following script to check the integrity of the reconstructed transactions:
#!/bin/bash # Description: Ensure all required fields are present after reconstructing transactions with `batch_txn`. # Search for usages of `Transaction.from_dict` and check fields rg --type python 'Transaction\.from_dict\(' -A 10This script helps confirm that
from_dict()properly includes all essential fields when creating the newTransactioninstances.
510-512: Verify handling of raw transactions with existingbatch_txnThe code skips raw transactions that already have a
batch_txnattribute. Please verify that these transactions have been adequately processed prior to this point and that skipping them here won't lead to missing necessary autofill steps.To confirm that all raw transactions with existing
batch_txnfields have been properly processed, you can search for their initialization in the codebase:This script looks for places where
batch_txnis assigned, helping ensure that such transactions are correctly handled before reaching this point in the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
xrpl/asyncio/transaction/main.py (3)
253-258: LGTM: Added support for batch transactions inautofill.The addition of batch transaction handling in the
autofillfunction is a significant feature. It correctly differentiates between regular and batch transactions, calling the appropriate autofill method.Consider adding comprehensive unit tests for this new batch transaction handling to ensure all edge cases are covered.
498-530: LGTM: Well-implemented batch transaction autofill logic.The
_autofill_batchfunction effectively handles the complexities of batch transactions, including sequence management for multiple accounts and transaction ID generation. The logic appears sound and well-structured.Consider adding error handling for potential issues, such as network errors during sequence retrieval. Also, it would be beneficial to add comprehensive unit tests for this function to cover various scenarios, including edge cases with multiple accounts and different transaction types within a batch.
Line range hint
1-530: Overall: Well-implemented batch transaction support with improved type safety.The changes in this file successfully introduce support for batch transactions while also improving type safety throughout. Key points:
- The new batch transaction handling is well-integrated into existing functions like
autofillandsign.- Type safety improvements, especially in the
autofillfunction, enhance the robustness of the code.- The new
_autofill_batchfunction effectively manages the complexities of batch transactions.These changes significantly enhance the capabilities of the XRPL Python library without introducing breaking changes to existing functionality.
To ensure the reliability and correctness of these new features:
- Add comprehensive unit tests for batch transaction handling, covering various scenarios and edge cases.
- Consider adding more detailed error handling, especially in the
_autofill_batchfunction.- Update the documentation to clearly explain the new batch transaction support and any considerations for developers using this feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- xrpl/asyncio/transaction/main.py (10 hunks)
🧰 Additional context used
🔇 Additional comments (4)
xrpl/asyncio/transaction/main.py (4)
2-6: LGTM: Import statements and type definitions updated appropriately.The new imports and type definitions are well-aligned with the introduction of batch transaction support and improved type safety. These changes lay a good foundation for the subsequent modifications in the file.
Also applies to: 18-19
Line range hint
180-200: LGTM: Consistent check for batch inner transactions.The addition of the check for batch inner transactions in
_prepare_transactionis consistent with the changes in thesignfunction. This double-check approach enhances the security and correctness of transaction handling.
218-223: LGTM: Improved type safety inautofillfunction.The introduction of the generic type
Tand its use in theautofillfunction signature enhances type safety. This change allows for more precise type checking and better IDE support when working with different transaction types.
Line range hint
90-99: Approve change, but verify impact on existing functionality.The addition of
_prepare_transactioncall and the check for batch inner transactions improves the security of the signing process. However, we should ensure this change doesn't break any existing use cases.Please run the following script to check for any existing direct uses of the
signfunction with batch inner transactions:✅ Verification successful
Action Required: Correct the file type identifier in the search script.
Please run the following script to check for any existing direct uses of the
signfunction with batch inner transactions:
Action Required: Refine the search pattern to ensure all relevant
signfunction calls withBatchare captured.Please run the following script to check for any existing direct uses of the
signfunction with batch inner transactions:
Change Verified: No direct calls to
signwithBatchfound.The addition of
_prepare_transactionand the check for batch inner transactions do not appear to break any existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct calls to sign() function with potential batch inner transactions rg -t python 'sign\s*\([^)]*Batch' --glob '!xrpl/asyncio/transaction/main.py'Length of output: 110
Script:
# #!/bin/bash # Search for direct calls to sign() function with potential batch inner transactions rg -t py 'sign\s*\([^)]*Batch' --glob '!xrpl/asyncio/transaction/main.py'Length of output: 75
Script:
# #!/bin/bash # Search for direct calls to sign() function with any usage of Batch in arguments rg -t py 'sign\s*\(.*Batch.*\)' --glob '!xrpl/asyncio/transaction/main.py'Length of output: 76
High Level Overview of Change
This PR:
BatchtransactionsBatchtransactionsBatchtransactionsautofillfor filling in theBatchTxnandTxIDsfieldsContext of Change
XRPLF/rippled#5060
Type of Change
Did you update CHANGELOG.md?
Test Plan
Added tests for the new features.