feat: add support for Batch amendment #757
Conversation
WalkthroughThis update introduces comprehensive support for batch transactions (XLS-56d) in the XRPL Python library. It adds new transaction models, batch signing and combination utilities, batch encoding for signing, and test coverage. Numerous transaction flag interfaces are unified, and new type annotations are applied throughout transaction autofill and signing workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BatchTx
participant Wallet
participant Client
participant BatchSigners
User->>BatchTx: Create Batch with raw_transactions
User->>Client: autofill(BatchTx)
Client->>BatchTx: Assign sequence, set flags, validate inner txns
User->>Wallet: sign_multiaccount_batch(BatchTx, Wallet)
Wallet->>BatchSigners: Sign Batch, add to BatchSigners
User->>Client: submit(BatchTx with BatchSigners)
Client-->>User: Submission result
Possibly related PRs
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
|
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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
10-13: Unify bullet punctuation and improve terminology
The “Added” bullets are inconsistent: the 3rd entry ends with a period, while the others do not. Also, expand “param” to “parameter” for clarity.Apply this diff:
@@ -10-13 - ### Added - - Improved validation for models to also check param types - - Support for `Account Permission` and `Account Permission Delegation` (XLS-74d, XLS-75d) - - Support for the `Batch` amendment (XLS-56d). + ### Added + - Improved validation for models to also check parameter types + - Support for `Account Permission` and `Account Permission Delegation` (XLS-74d, XLS-75d) + - Support for the `Batch` amendment (XLS-56d)
16-22: Unify bullet punctuation in “Fixed” section
The first bullet under “Fixed” lacks a trailing period, while subsequent entries include one. Add a period to maintain consistency.Apply this diff:
@@ -16 - - Add `MPTCurrency` support in `Issue` (rippled internal type) + - Add `MPTCurrency` support in `Issue` (rippled internal type).🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ... extracting sequence number. - Increase default maximum payload size for websocket clie...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ... extracting sequence number. - Increase default maximum payload size for websocket clie...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Integration test (3.8)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
xrpl/transaction/batch_signers.py (1)
122-134: Consider adding a clarifying comment for the filtering logic.The function correctly aggregates signers and filters out the submitter's signature. However, the logic at line 128 that uses
transactions[0].accountas the submitter might benefit from a comment explaining this assumption.def _get_batch_with_all_signers(transactions: List[Batch]) -> Batch: + # Collect all batch signers except the submitter's own signature. + # The submitter is identified as the account field of the first transaction. batch_signers = [
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)xrpl/core/binarycodec/main.py(3 hunks)xrpl/transaction/batch_signers.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- xrpl/core/binarycodec/main.py
🧰 Additional context used
🪛 Pylint (3.3.7)
xrpl/transaction/batch_signers.py
[error] 106-106: Instance of 'BaseModel' has no 'to_xrpl' member
(E1101)
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~17-~17: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...current" ledger for extracting sequence number - Increase default maximum payload size...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...r extracting sequence number - Increase default maximum payload size for websocket clie...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (4)
xrpl/transaction/batch_signers.py (3)
1-13: LGTM!Well-structured module with clear purpose and appropriate imports.
15-62: Well-implemented batch signing function.The function correctly validates that the signing wallet is involved in the batch, properly normalizes flags using
_flags_to_int(), and handles both single-sign and multisign modes appropriately.
109-119: Validation logic looks good.The equivalence check correctly ensures all batch transactions have identical flags and raw transactions, which is essential for safe combining of batch signers.
CHANGELOG.md (1)
10-22: Changelog updates are well-organized.The changes properly document the new Batch amendment support and related fixes. The reorganization with "Added" section before "Fixed" follows changelog best practices.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...current" ledger for extracting sequence number - Increase default maximum payload size...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...r extracting sequence number - Increase default maximum payload size for websocket clie...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
xrpl/models/transactions/batch.py (1)
77-101: Consider defensive programming for flag handlingThe
__post_init__method assumes that when flags is a dict, it can be modified by setting["TF_INNER_BATCH_TXN"]. However, this could potentially overwrite existing flag values if there are naming conflicts.Consider validating that the flag isn't already set in dict form before adding it:
elif isinstance(tx_dict["flags"], dict): - tx_dict["flags"]["TF_INNER_BATCH_TXN"] = True + if "TF_INNER_BATCH_TXN" not in tx_dict["flags"]: + tx_dict["flags"]["TF_INNER_BATCH_TXN"] = Truetests/unit/models/transactions/test_batch.py (1)
30-41: Consider expanding test coverageWhile the minimum transaction count test is good, consider adding tests for:
- Edge cases with different flag formats (dict, list, None)
- Batch with pre-signed transactions (should fail)
- Nested batch transactions (should fail)
Would you like me to generate additional test cases to improve coverage of the edge cases mentioned above?
xrpl/asyncio/transaction/main.py (1)
319-319: Simplify nested conditional logicThe static analysis tool correctly identifies that the nested if statements can be simplified for better readability.
- if client.network_id is not None and client.network_id > _RESTRICTED_NETWORKS: + if (client.network_id is not None + and client.network_id > _RESTRICTED_NETWORKS):However, the current logic is actually correct as-is since we need to check both conditions. The static analysis suggestion might not be appropriate here given the specific business logic requirements.
🧰 Tools
🪛 Ruff (0.11.9)
319-322: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.ci-config/rippled.cfg(1 hunks)CHANGELOG.md(1 hunks)tests/unit/core/binarycodec/test_main.py(2 hunks)tests/unit/models/transactions/test_batch.py(1 hunks)xrpl/asyncio/transaction/main.py(13 hunks)xrpl/models/transactions/batch.py(1 hunks)xrpl/transaction/main.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/core/binarycodec/test_main.py
- .ci-config/rippled.cfg
- xrpl/transaction/main.py
🧰 Additional context used
🪛 Ruff (0.11.9)
xrpl/asyncio/transaction/main.py
319-322: Use a single if statement instead of nested if statements
(SIM102)
555-555: Function definition does not bind loop variable raw_txn_dict
(B023)
556-556: Function definition does not bind loop variable raw_txn_dict
(B023)
557-557: Function definition does not bind loop variable raw_txn_dict
(B023)
🪛 Pylint (3.3.7)
xrpl/asyncio/transaction/main.py
[error] 529-529: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 530-530: Instance of 'BaseModel' has no 'account' member
(E1101)
[error] 530-530: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 533-533: Instance of 'BaseModel' has no 'raw_transactions' member
(E1101)
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~17-~17: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...current" ledger for extracting sequence number - Increase default maximum payload size...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...r extracting sequence number - Increase default maximum payload size for websocket clie...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~21-~21: The preposition ‘to’ seems more likely in this position.
Context: ...est.from_dict- Improve error handling foramm_info` - Handle autofilling better ...
(AI_HYDRA_LEO_REPLACE_FOR_TO)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Integration test (3.9)
🔇 Additional comments (6)
xrpl/models/transactions/batch.py (1)
130-137: LGTM: Clean handling of nested transaction structureThe
from_dictmethod correctly handles the nestedraw_transactionstructure that comes from the XRPL format while maintaining backward compatibility with direct transaction objects.tests/unit/models/transactions/test_batch.py (1)
10-28: LGTM: Good coverage of basic batch functionalityThe test correctly verifies that the
TF_INNER_BATCH_TXNflag is automatically added to inner transactions and that the batch validates successfully.xrpl/asyncio/transaction/main.py (2)
413-413: LGTM: Correct fix for address conversionThe fix correctly extracts only the classic address from the tuple returned by
xaddress_to_classic_address, preventing potential issues with tag and test flag values.
525-585: Good implementation of batch autofill logicThe
_autofill_batchfunction properly handles the complex requirements for batch transactions:
- Prevents nested batches
- Manages sequence numbers per account
- Validates required field values
- Prevents pre-signed transactions
🧰 Tools
🪛 Ruff (0.11.9)
555-555: Function definition does not bind loop variable
raw_txn_dict(B023)
556-556: Function definition does not bind loop variable
raw_txn_dict(B023)
557-557: Function definition does not bind loop variable
raw_txn_dict(B023)
🪛 Pylint (3.3.7)
[error] 529-529: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 530-530: Instance of 'BaseModel' has no 'account' member
(E1101)
[error] 530-530: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 533-533: Instance of 'BaseModel' has no 'raw_transactions' member
(E1101)
CHANGELOG.md (2)
13-13: LGTM: Accurate documentation of new featureThe changelog entry correctly documents the addition of Batch amendment support (XLS-56d) which aligns with the comprehensive implementation across the codebase.
15-24: Good organization of changelog entriesThe reorganization of the Fixed section with clear bullet points improves readability and properly documents the various improvements made alongside the batch transaction feature.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...current" ledger for extracting sequence number - Increase default maximum payload size...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...r extracting sequence number - Increase default maximum payload size for websocket clie...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~21-~21: The preposition ‘to’ seems more likely in this position.
Context: ...est.from_dict- Improve error handling foramm_info` - Handle autofilling better ...(AI_HYDRA_LEO_REPLACE_FOR_TO)
| def _validate_field(field_name: str, expected_value: Union[str, None]) -> None: | ||
| if field_name not in raw_txn_dict: | ||
| raw_txn_dict[field_name] = expected_value | ||
| elif raw_txn_dict[field_name] != expected_value: | ||
| raise XRPLException( | ||
| f"Must have a `{field_name}` of {repr(expected_value)} in an " | ||
| "inner Batch transaction." | ||
| ) |
There was a problem hiding this comment.
Fix function closure issue with loop variable
The static analysis correctly identifies that the nested _validate_field function doesn't properly capture the loop variable raw_txn_dict. This could lead to unexpected behavior.
Move the function definition outside the loop or pass the dictionary as a parameter:
+ def _validate_field(txn_dict: Dict[str, Any], field_name: str, expected_value: Union[str, None]) -> None:
+ if field_name not in txn_dict:
+ txn_dict[field_name] = expected_value
+ elif txn_dict[field_name] != expected_value:
+ raise XRPLException(
+ f"Must have a `{field_name}` of {repr(expected_value)} in an "
+ "inner Batch transaction."
+ )
+
for raw_txn in transaction.raw_transactions:
raw_txn_dict = raw_txn.to_dict()
# ... existing validation logic ...
- def _validate_field(field_name: str, expected_value: Union[str, None]) -> None:
- if field_name not in raw_txn_dict:
- raw_txn_dict[field_name] = expected_value
- elif raw_txn_dict[field_name] != expected_value:
- raise XRPLException(
- f"Must have a `{field_name}` of {repr(expected_value)} in an "
- "inner Batch transaction."
- )
- _validate_field("fee", "0")
- _validate_field("signing_pub_key", "")
- _validate_field("txn_signature", None)
+ _validate_field(raw_txn_dict, "fee", "0")
+ _validate_field(raw_txn_dict, "signing_pub_key", "")
+ _validate_field(raw_txn_dict, "txn_signature", None)📝 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.
| def _validate_field(field_name: str, expected_value: Union[str, None]) -> None: | |
| if field_name not in raw_txn_dict: | |
| raw_txn_dict[field_name] = expected_value | |
| elif raw_txn_dict[field_name] != expected_value: | |
| raise XRPLException( | |
| f"Must have a `{field_name}` of {repr(expected_value)} in an " | |
| "inner Batch transaction." | |
| ) | |
| def _validate_field(txn_dict: Dict[str, Any], field_name: str, expected_value: Union[str, None]) -> None: | |
| if field_name not in txn_dict: | |
| txn_dict[field_name] = expected_value | |
| elif txn_dict[field_name] != expected_value: | |
| raise XRPLException( | |
| f"Must have a `{field_name}` of {repr(expected_value)} in an " | |
| "inner Batch transaction." | |
| ) | |
| for raw_txn in transaction.raw_transactions: | |
| raw_txn_dict = raw_txn.to_dict() | |
| # ... existing validation logic ... | |
| _validate_field(raw_txn_dict, "fee", "0") | |
| _validate_field(raw_txn_dict, "signing_pub_key", "") | |
| _validate_field(raw_txn_dict, "txn_signature", None) |
🧰 Tools
🪛 Ruff (0.11.9)
555-555: Function definition does not bind loop variable raw_txn_dict
(B023)
556-556: Function definition does not bind loop variable raw_txn_dict
(B023)
557-557: Function definition does not bind loop variable raw_txn_dict
(B023)
🤖 Prompt for AI Agents
In xrpl/asyncio/transaction/main.py around lines 554 to 561, the nested function
_validate_field improperly closes over the loop variable raw_txn_dict, which can
cause incorrect behavior. To fix this, refactor _validate_field to accept
raw_txn_dict as an explicit parameter instead of relying on closure, or move the
function definition outside the loop so it does not capture the loop variable.
This ensures the function uses the correct dictionary instance each time it is
called.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
xrpl/asyncio/transaction/main.py (1)
558-565:⚠️ Potential issueFix function closure issue with loop variable
The nested
_validate_fieldfunction capturesraw_txn_dictfrom the loop, which could lead to incorrect behavior. Move the function outside the loop or pass the dictionary as a parameter:- # validate fields that are supposed to be empty/zeroed - def _validate_field(field_name: str, expected_value: Union[str, None]) -> None: - if field_name not in raw_txn_dict: - raw_txn_dict[field_name] = expected_value - elif raw_txn_dict[field_name] != expected_value: - raise XRPLException( - f"Must have a `{field_name}` of {repr(expected_value)} in an " - "inner Batch transaction." - ) - - _validate_field("fee", "0") - _validate_field("signing_pub_key", "") - _validate_field("txn_signature", None) + # validate fields that are supposed to be empty/zeroed + def _validate_field( + txn_dict: Dict[str, Any], + field_name: str, + expected_value: Union[str, None] + ) -> None: + if field_name not in txn_dict: + txn_dict[field_name] = expected_value + elif txn_dict[field_name] != expected_value: + raise XRPLException( + f"Must have a `{field_name}` of {repr(expected_value)} in an " + "inner Batch transaction." + ) + + _validate_field(raw_txn_dict, "fee", "0") + _validate_field(raw_txn_dict, "signing_pub_key", "") + _validate_field(raw_txn_dict, "txn_signature", None)🧰 Tools
🪛 Ruff (0.11.9)
559-559: Function definition does not bind loop variable
raw_txn_dict(B023)
560-560: Function definition does not bind loop variable
raw_txn_dict(B023)
561-561: Function definition does not bind loop variable
raw_txn_dict(B023)
🧹 Nitpick comments (5)
xrpl/asyncio/transaction/main.py (2)
319-322: Simplify nested if statementsThe logic is correct, but can be simplified by combining the conditions:
- if client.network_id is not None and client.network_id > _RESTRICTED_NETWORKS: - if client.build_version and _is_not_later_rippled_version( - _REQUIRED_NETWORKID_VERSION, client.build_version - ): - return True + if (client.network_id is not None + and client.network_id > _RESTRICTED_NETWORKS + and client.build_version + and _is_not_later_rippled_version(_REQUIRED_NETWORKID_VERSION, client.build_version)): + return True🧰 Tools
🪛 Ruff (0.11.9)
319-322: Use a single
ifstatement instead of nestedifstatements(SIM102)
506-514: Optimize batch fee calculation with generator expressionThe fee calculation correctly follows the specification. Consider using a generator expression for better memory efficiency:
- base_fee = base_fee * 2 + sum( - [ - int(await _calculate_fee_per_transaction_type(raw_txn, client)) - for raw_txn in batch.raw_transactions - ] - ) + base_fee = base_fee * 2 + sum( + int(await _calculate_fee_per_transaction_type(raw_txn, client)) + for raw_txn in batch.raw_transactions + )🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 508-513: Consider using a generator instead 'sum(int(await _calculate_fee_per_transaction_type(raw_txn, client)) for raw_txn in batch.raw_transactions)'
(R1728)
xrpl/models/transactions/batch.py (3)
99-101: Include actual type in error messageThe error message could be more helpful by including the actual type that was encountered.
else: raise XRPLModelException( - "Invalid `flags` ty 2684 pe for raw transaction in Batch." + f"Invalid `flags` type for raw transaction in Batch: {type(tx_dict['flags'])}." )
112-117: Use enumerate for more Pythonic iterationConsider using
enumerateinstead ofrange(len())for cleaner iteration.- for i in range(len(self.raw_transactions)): - tx = self.raw_transactions[i] + for i, tx in enumerate(self.raw_transactions):
83-102: Consider extracting flag handling logicThe logic for ensuring the
TF_INNER_BATCH_TXNflag is complex and handles multiple cases. Consider extracting this into a helper method for better maintainability and reusability.def _ensure_inner_batch_flag(tx_dict: Dict[str, Any]) -> Dict[str, Any]: """Ensure the transaction has the TF_INNER_BATCH_TXN flag set.""" if "flags" not in tx_dict: tx_dict["flags"] = TransactionFlag.TF_INNER_BATCH_TXN elif isinstance(tx_dict["flags"], int): tx_dict["flags"] |= TransactionFlag.TF_INNER_BATCH_TXN elif isinstance(tx_dict["flags"], dict): tx_dict["flags"]["TF_INNER_BATCH_TXN"] = True elif isinstance(tx_dict["flags"], list): tx_dict["flags"].append(TransactionFlag.TF_INNER_BATCH_TXN) elif tx_dict["flags"] is None: tx_dict["flags"] = TransactionFlag.TF_INNER_BATCH_TXN else: raise XRPLModelException( f"Invalid `flags` type for raw transaction in Batch: {type(tx_dict['flags'])}." ) return tx_dict
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
xrpl/asyncio/transaction/main.py(14 hunks)xrpl/models/transactions/batch.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
xrpl/asyncio/transaction/main.py (1)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#757
File: xrpl/asyncio/transaction/main.py:505-509
Timestamp: 2025-06-05T20:52:31.065Z
Learning: The correct fee calculation for XRPL Batch transactions follows the C++ rippled implementation structure: (2 × base_fee) + sum of individual inner transaction fees + (signer_count × base_fee). Each inner transaction's fee should be calculated individually using the same fee calculation logic as standalone transactions, not assumed to be equal to base_fee.
🪛 Ruff (0.11.9)
xrpl/asyncio/transaction/main.py
319-322: Use a single if statement instead of nested if statements
(SIM102)
559-559: Function definition does not bind loop variable raw_txn_dict
(B023)
560-560: Function definition does not bind loop variable raw_txn_dict
(B023)
561-561: Function definition does not bind loop variable raw_txn_dict
(B023)
🪛 Pylint (3.3.7)
xrpl/asyncio/transaction/main.py
[refactor] 508-513: Consider using a generator instead 'sum(int(await _calculate_fee_per_transaction_type(raw_txn, client)) for raw_txn in batch.raw_transactions)'
(R1728)
[error] 533-533: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 534-534: Instance of 'BaseModel' has no 'account' member
(E1101)
[error] 534-534: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 537-537: Instance of 'BaseModel' has no 'raw_transactions' member
(E1101)
🔇 Additional comments (7)
xrpl/asyncio/transaction/main.py (5)
42-42: Good use of TypeVar for better type safetyThe generic type variable
Tproperly constrains toTransactionand provides type safety for functions that preserve the specific transaction type.
80-124: Improved sign() function with better type safety and structureThe refactoring properly:
- Uses generics to preserve transaction type
- Delegates transaction preparation to
_prepare_transaction- Fixes the multisign structure to use dictionaries
- Correctly assigns
SigningPubKeyfor non-multisigned transactions
223-242: Proper validation to prevent signing of inner batch transactionsThe function correctly prevents direct signing of inner batch transactions, which is essential for batch transaction security. The simplified signature without the wallet parameter is cleaner.
260-304: Well-structured autofill() with batch transaction supportThe function properly:
- Uses generics to preserve transaction type
- Correctly handles ticket sequences by setting sequence to 0
- Delegates batch transaction autofilling to a specialized helper function
413-413: Fixed extraction of classic address from tupleGood fix - correctly extracts only the classic address from the tuple returned by
xaddress_to_classic_address.xrpl/models/transactions/batch.py (2)
24-37: Well-designed flag enumerationThe
BatchFlagenum correctly uses distinct bit positions for each flag, enabling proper bitwise operations. The naming convention follows the established pattern.
122-159: Clean implementation of serialization/deserializationThe
from_dictandto_dictmethods correctly handle the wrapping/unwrapping of theraw_transactionfield, maintaining consistency with the expected wire format.
| The dictionary representation of a Batch. | ||
| """ | ||
| tx_dict = super().to_dict() | ||
| tx_dict["raw_transactions"] = [ |
There was a problem hiding this comment.
Help me understand this question: Why are you manually unpacking/packing the RawTransactions field? There are other "inner-object" (in rippled parlance)/ NestedModel fields which do not have this special code in their from_dict and to_dict method definitions. (AuthAccount, BatchSigner are two examples).
Aren't inner-objects handled by the JSON library automatically? Do we need to spell out arbitrary depth nesting of the JSON object structure?
(The below structure is my mental model of the RawTransactions STArray inner object)
RawTransactions: [
{
RawTransaction: {
TransactionType: "OfferCreate",
...
}
},
{
RawTransaction: {
TransactionType: "Payment",
...
}
},
]
There was a problem hiding this comment.
Note that RawTransactions is a list of Transactions, and Transaction does not inherit from NestedModel. We could theoretically have a model called RawTransaction that just has a Transaction field, but that's much less user-friendly IMO, so instead we have to do the nesting in the Batch class. If you take a look at the NestedModel class, you'll see the code handling it is pretty similar.
There was a problem hiding this comment.
You could have implemented this idea by making use of NestedModel.
class RawTransaction(NestedModel):
raw_transaction: Transaction = REQUIRED # type: ignore
class Batch(Transaction):
"""Represents a Batch transaction."""
raw_transactions: List[RawTransaction] = REQUIRED # type: ignore
# instead of the current design
# raw_transactions: List[Transaction] = REQUIRED # type: ignore
# rest of the code
I don't see how this lessens the user-experience. As a bonus, you wouldn't have had to unpack/pack the raw-transactions yourself.
There was a problem hiding this comment.
Yes, I could have, but now every Batch looks like this:
Batch(account="r...",
raw_transactions=[
RawTransaction(Payment(account="r...", ...)),
RawTransaction(AccountSet(account="r...", ...)),
]
)
This is much weirder/trickier to use, instead of just having a list of transactions, which is how I would want to use it. Users don't have to do any additional work with the existing solution anyways, it "magically" works because of the additional NestedModel-esque code.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
xrpl/asyncio/transaction/main.py (1)
565-572:⚠️ Potential issueFix function closure issue with loop variable.
The static analysis correctly identifies that the nested
_validate_fieldfunction doesn't properly capture the loop variableraw_txn_dict. This has been flagged in previous reviews as well.Move the function definition outside the loop or pass the dictionary as a parameter:
+ def _validate_field(txn_dict: Dict[str, Any], field_name: str, expected_value: Union[str, None]) -> None: + if field_name not in txn_dict: + txn_dict[field_name] = expected_value + elif txn_dict[field_name] != expected_value: + raise XRPLException( + f"Must have a `{field_name}` of {repr(expected_value)} in an " + "inner Batch transaction." + ) + for raw_txn in transaction.raw_transactions: raw_txn_dict = raw_txn.to_dict() # ... existing validation logic ... - def _validate_field(field_name: str, expected_value: Union[str, None]) -> None: - if field_name not in raw_txn_dict: - raw_txn_dict[field_name] = expected_value - elif raw_txn_dict[field_name] != expected_value: - raise XRPLException( - f"Must have a `{field_name}` of {repr(expected_value)} in an " - "inner Batch transaction." - ) - _validate_field("fee", "0") - _validate_field("signing_pub_key", "") - _validate_field("txn_signature", None) + _validate_field(raw_txn_dict, "fee", "0") + _validate_field(raw_txn_dict, "signing_pub_key", "") + _validate_field(raw_txn_dict, "txn_signature", None)🧰 Tools
🪛 Ruff (0.11.9)
566-566: Function definition does not bind loop variable
raw_txn_dict(B023)
567-567: Function definition does not bind loop variable
raw_txn_dict(B023)
568-568: Function definition does not bind loop variable
raw_txn_dict(B023)
🧹 Nitpick comments (1)
xrpl/asyncio/transaction/main.py (1)
319-319: Consider simplifying nested if statements.The static analysis correctly suggests combining the nested if statements for better readability.
- if client.network_id is not None and client.network_id > _RESTRICTED_NETWORKS: - if client.build_version and _is_not_later_rippled_version( - _REQUIRED_NETWORKID_VERSION, client.build_version - ): - return True + if (client.network_id is not None + and client.network_id > _RESTRICTED_NETWORKS + and client.build_version + and _is_not_later_rippled_version(_REQUIRED_NETWORKID_VERSION, client.build_version)): + return True🧰 Tools
🪛 Ruff (0.11.9)
319-322: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)tests/integration/transactions/test_batch.py(1 hunks)xrpl/asyncio/transaction/main.py(15 hunks)xrpl/models/transactions/batch.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/transactions/test_batch.py
- xrpl/models/transactions/batch.py
🧰 Additional context used
🧠 Learnings (1)
xrpl/asyncio/transaction/main.py (1)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#757
File: xrpl/asyncio/transaction/main.py:505-509
Timestamp: 2025-06-05T20:52:31.065Z
Learning: The correct fee calculation for XRPL Batch transactions follows the C++ rippled implementation structure: (2 × base_fee) + sum of individual inner transaction fees + (signer_count × base_fee). Each inner transaction's fee should be calculated individually using the same fee calculation logic as standalone transactions, not assumed to be equal to base_fee.
🪛 Ruff (0.11.9)
xrpl/asyncio/transaction/main.py
319-322: Use a single if statement instead of nested if statements
(SIM102)
566-566: Function definition does not bind loop variable raw_txn_dict
(B023)
567-567: Function definition does not bind loop variable raw_txn_dict
(B023)
568-568: Function definition does not bind loop variable raw_txn_dict
(B023)
🪛 Pylint (3.3.7)
xrpl/asyncio/transaction/main.py
[refactor] 512-517: Consider using a generator instead 'sum(int(await _calculate_fee_per_transaction_type(raw_txn, client)) for raw_txn in batch.raw_transactions)'
(R1728)
[error] 540-540: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 541-541: Instance of 'BaseModel' has no 'account' member
(E1101)
[error] 541-541: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 544-544: Instance of 'BaseModel' has no 'raw_transactions' member
(E1101)
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~17-~17: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...current" ledger for extracting sequence number - Increase default maximum payload size...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...r extracting sequence number - Increase default maximum payload size for websocket clie...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (7)
xrpl/asyncio/transaction/main.py (6)
4-6: Generic typing implementation looks good.The addition of TypeVar
Tbound toTransactionis well-implemented and will help preserve transaction subtype information through the transformation functions.Also applies to: 42-42
240-242: Good validation for preventing direct signing of batch inner transactions.The check for
TF_INNER_BATCH_TXNflag correctly prevents direct signing of batch inner transactions, which aligns with the batch transaction specification.
413-413: Correct fix for X-address conversion.The update to extract only the classic address from the tuple returned by
xaddress_to_classic_addressis correct and fixes a potential bug.
510-518: Verify batch fee calculation matches specification.Based on the retrieved learning, the fee calculation has been updated to follow the C++ rippled implementation structure. The current implementation calculates individual fees for each inner transaction and applies the correct base fee multiplier.
Note: This addresses the previously discussed fee calculation concerns by properly calculating fees for each inner transaction rather than assuming they equal the base fee.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 512-517: Consider using a generator instead 'sum(int(await _calculate_fee_per_transaction_type(raw_txn, client)) for raw_txn in batch.raw_transactions)'
(R1728)
536-596: Comprehensive batch autofill implementation.The
_autofill_batchfunction provides thorough handling of inner transaction autofilling with proper validation for:
- Nested batch transaction prevention
- Sequence number management per account
- Signature validation
- Required field enforcement
The implementation correctly enforces batch transaction constraints as specified in XLS-56d.
🧰 Tools
🪛 Ruff (0.11.9)
566-566: Function definition does not bind loop variable
raw_txn_dict(B023)
567-567: Function definition does not bind loop variable
raw_txn_dict(B023)
568-568: Function definition does not bind loop variable
raw_txn_dict(B023)
🪛 Pylint (3.3.7)
[error] 540-540: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 541-541: Instance of 'BaseModel' has no 'account' member
(E1101)
[error] 541-541: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 544-544: Instance of 'BaseModel' has no 'raw_transactions' member
(E1101)
96-123:❓ Verification inconclusive
Verify the multisign dictionary structure matches expected format.
The multisign logic has been updated to use a dictionary structure instead of the previous
Signermodel. Please ensure this new structure matches the expected XRPL transaction format for multisigned transactions.
🏁 Script executed:
#!/bin/bash # Description: Check if the new multisign dictionary structure is used consistently across the codebase # Search for other multisign implementations to verify consistency rg -A 5 -B 5 "Signers.*Signer.*Account" --type pyLength of output: 51
Let’s compare against the synchronous implementation to confirm the exact
Signersshape:
🏁 Script executed:
#!/bin/bash # Find synchronous transaction main.py under xrpl/transaction (exclude asyncio) files=$(fd -t f "^main.py$" xrpl/transaction) sync_file=$(echo "$files" | grep -v "asyncio" | head -n1) if [[ -z "$sync_file" ]]; then echo "⚠️ Synchronous main.py not found under xrpl/transaction" exit 0 fi echo "🔍 Inspecting synchronous file: $sync_file" echo -e "\n--- Around multisign logic ---" rg -n -C5 "if multisign" "$sync_file" || echo "No multisign block found" echo -e "\n--- ‘Signers’ usage ---" rg -n -C5 '"Signers"' "$sync_file" || echo "No Signers key usage found"Length of output: 695
Action Required: Confirm Multisign
SignersStructureWe weren’t able to locate any other multisign implementations in the sync codebase to compare against. Please double-check that the new structure in
xrpl/asyncio/transaction/main.py(lines 96–123):transaction_json["Signers"] = [ { "Signer": { "Account": wallet.address, "TxnSignature": signature, "SigningPubKey": wallet.public_key, } } ]• Matches the official XRPL multisign format (an array of objects, each with a single
Signerkey containingAccount,TxnSignature, andSigningPubKey).
• Is correctly handled end-to-end (serialization, signing, deserialization).
• Aligns with any existing tests or examples in your codebase.If you haven’t already, please compare against the XRPL Transaction Format docs to ensure full compatibility.
CHANGELOG.md (1)
13-25: Changelog accurately documents batch transaction support and related improvements.The changelog correctly documents the addition of Batch amendment support (XLS-56d) and the related fixes for multisigning, typing improvements, and TicketSequence handling. The reorganization to separate Added and Fixed sections improves readability.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...current" ledger for extracting sequence number - Increase default maximum payload size...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...r extracting sequence number - Increase default maximum payload size for websocket clie...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
xrpl/asyncio/transaction/main.py
Outdated
| # BaseFee × (1 + Number of Signatures Provided) | ||
| if signers_count is not None and signers_count > 0: | ||
| base_fee += net_fee * (1 + signers_count) | ||
| if transaction.transaction_type == TransactionType.BATCH: |
There was a problem hiding this comment.
There is a mismatch between the fee-calculations in the xrpl-py implementation and that of xrpl.js. Here is a snapshot:
The xrpl.js implementation does not have this transaction-type based discretion of net_fee * signers_count versus net_fee * (1 + signers_count).
Furthermore, the current implementation does not match the rippled cpp code.
I would expect to iterate over all the entries in the BatchSigners list to accurately compute the fees.
I understand that autofill does not have access to the signatures and hence cannot precisely calculate this fee. Should we put out a disclaimer that the client libraries do not match the rippled node in the fee expectations?
There was a problem hiding this comment.
- Turns out that both are wrong and it should just be just
+ base_fee * signersCountin both places - this has been fixed (and tested with integration tests). - rippled sees the transactions after they have been signed. This isn't comparable. The libraries see the transactions before they have been signed, so some parts of the transaction aren't available yet. The same process is done for regular multisign.
There was a problem hiding this comment.
-
Turns out that both are wrong
OK -
It would be helpful to document the differences in fee-calculation between the client libraries and rippled nodes. Users should be aware of this quirk. This is useful for debugging erroring Batch transactions.
There was a problem hiding this comment.
I updated the docs around autofill to call it out.
| The dictionary representation of a Batch. | ||
| """ | ||
| tx_dict = super().to_dict() | ||
| tx_dict["raw_transactions"] = [ |
There was a problem hiding this comment.
You could have implemented this idea by making use of NestedModel.
class RawTransaction(NestedModel):
raw_transaction: Transaction = REQUIRED # type: ignore
class Batch(Transaction):
"""Represents a Batch transaction."""
raw_transactions: List[RawTransaction] = REQUIRED # type: ignore
# instead of the current design
# raw_transactions: List[Transaction] = REQUIRED # type: ignore
# rest of the code
I don't see how this lessens the user-experience. As a bonus, you wouldn't have had to unpack/pack the raw-transactions yourself.
|
@mvadari I only have one concern left that needs to be addressed. Apart from that, this PR looks good. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
xrpl/asyncio/transaction/main.py (1)
562-573:⚠️ Potential issueFix function closure issue with loop variable.
The static analysis correctly identifies that the nested
_validate_fieldfunction doesn't properly capture the loop variableraw_txn_dict. This could lead to unexpected behavior.Move the function definition outside the loop or pass the dictionary as a parameter:
+ def _validate_field(txn_dict: Dict[str, Any], field_name: str, expected_value: Union[str, None]) -> None: + if field_name not in txn_dict: + txn_dict[field_name] = expected_value + elif txn_dict[field_name] != expected_value: + raise XRPLException( + f"Must have a `{field_name}` of {repr(expected_value)} in an " + "inner Batch transaction." + ) + for raw_txn in transaction.raw_transactions: raw_txn_dict = raw_txn.to_dict() # ... existing validation logic ... - def _validate_field(field_name: str, expected_value: Union[str, None]) -> None: - if field_name not in raw_txn_dict: - raw_txn_dict[field_name] = expected_value - elif raw_txn_dict[field_name] != expected_value: - raise XRPLException( - f"Must have a `{field_name}` of {repr(expected_value)} in an " - "inner Batch transaction." - ) - _validate_field("fee", "0") - _validate_field("signing_pub_key", "") - _validate_field("txn_signature", None) + _validate_field(raw_txn_dict, "fee", "0") + _validate_field(raw_txn_dict, "signing_pub_key", "") + _validate_field(raw_txn_dict, "txn_signature", None)🧰 Tools
🪛 Ruff (0.11.9)
563-563: Function definition does not bind loop variable
raw_txn_dict(B023)
564-564: Function definition does not bind loop variable
raw_txn_dict(B023)
565-565: Function definition does not bind loop variable
raw_txn_dict(B023)
🧹 Nitpick comments (1)
xrpl/asyncio/transaction/main.py (1)
512-517: Consider using generator for performance optimization.The static analysis correctly suggests using a generator expression instead of a list comprehension for better memory efficiency.
- base_fee = base_fee * 2 + sum( - [ - int(await _calculate_fee_per_transaction_type(raw_txn, client)) - for raw_txn in batch.raw_transactions - ] - ) + base_fee = base_fee * 2 + sum( + int(await _calculate_fee_per_transaction_type(raw_txn, client)) + for raw_txn in batch.raw_transactions + )🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 512-517: Consider using a generator instead 'sum(int(await _calculate_fee_per_transaction_type(raw_txn, client)) for raw_txn in batch.raw_transactions)'
(R1728)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)xrpl/asyncio/transaction/main.py(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
xrpl/asyncio/transaction/main.py (1)
Learnt from: ckeshava
PR: XRPLF/xrpl-py#757
File: xrpl/asyncio/transaction/main.py:505-509
Timestamp: 2025-06-05T20:52:31.065Z
Learning: The correct fee calculation for XRPL Batch transactions follows the C++ rippled implementation structure: (2 × base_fee) + sum of individual inner transaction fees + (signer_count × base_fee). Each inner transaction's fee should be calculated individually using the same fee calculation logic as standalone transactions, not assumed to be equal to base_fee.
🪛 Ruff (0.11.9)
xrpl/asyncio/transaction/main.py
319-322: Use a single if statement instead of nested if statements
(SIM102)
563-563: Function definition does not bind loop variable raw_txn_dict
(B023)
564-564: Function definition does not bind loop variable raw_txn_dict
(B023)
565-565: Function definition does not bind loop variable raw_txn_dict
(B023)
🪛 Pylint (3.3.7)
xrpl/asyncio/transaction/main.py
[refactor] 512-517: Consider using a generator instead 'sum(int(await _calculate_fee_per_transaction_type(raw_txn, client)) for raw_txn in batch.raw_transactions)'
(R1728)
[error] 537-537: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 538-538: Instance of 'BaseModel' has no 'account' member
(E1101)
[error] 538-538: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 541-541: Instance of 'BaseModel' has no 'raw_transactions' member
(E1101)
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~17-~17: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...current" ledger for extracting sequence number - Increase default maximum payload size...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...r extracting sequence number - Increase default maximum payload size for websocket clie...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.8)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.9)
🔇 Additional comments (10)
xrpl/asyncio/transaction/main.py (8)
42-42: LGTM: Well-structured generic type variable.The generic type variable follows Python typing best practices and provides better type safety for transaction-related functions.
80-84: Excellent typing improvements with generic bounds.The addition of generic type variable
Tbound toTransactionsignificantly improves type safety and developer experience acrosssign,autofill_and_sign, andautofillfunctions.Also applies to: 126-132, 260-263
107-116: Approve updated multisign structure.The change from the removed
Signermodel to a dictionary-based structure is correctly implemented and maintains compatibility with the XRPL multisign format.
240-242: Good safety check for batch inner transactions.This validation correctly prevents direct signing of batch inner transactions, which aligns with the XRPL batch transaction specification.
285-292: Improved ticket sequence handling.The enhanced logic properly handles the case where
ticket_sequenceis present by settingsequenceto 0, which is the correct behavior for ticket-based transactions.
413-413: Fix X-address conversion extraction.Correctly extracts only the classic address from the tuple returned by
xaddress_to_classic_address.
510-518: Verify the batch fee calculation aligns with latest requirements.The current implementation calculates
(2 × base_fee) + sum of individual inner transaction fees, which aligns with the base structure discussed in past reviews. However, based on the retrieved learning, this should also include signer fees whensigners_countis provided.Please confirm that this implementation matches the expected behavior when
signers_countis provided for batch transactions, as the multisign fee addition (lines 521-522) applies to all transaction types including batches.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 512-517: Consider using a generator instead 'sum(int(await _calculate_fee_per_transaction_type(raw_txn, client)) for raw_txn in batch.raw_transactions)'
(R1728)
533-593: Comprehensive batch autofill implementation.The
_autofill_batchfunction provides thorough validation and autofilling for batch inner transactions, including:
- Sequence number management per account
- Validation of required empty/zero fields
- Network ID assignment
- Proper error handling for invalid configurations
The logic correctly enforces batch transaction constraints.
🧰 Tools
🪛 Ruff (0.11.9)
563-563: Function definition does not bind loop variable
raw_txn_dict(B023)
564-564: Function definition does not bind loop variable
raw_txn_dict(B023)
565-565: Function definition does not bind loop variable
raw_txn_dict(B023)
🪛 Pylint (3.3.7)
[error] 537-537: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 538-538: Instance of 'BaseModel' has no 'account' member
(E1101)
[error] 538-538: Instance of 'BaseModel' has no 'sequence' member
(E1101)
[error] 541-541: Instance of 'BaseModel' has no 'raw_transactions' member
(E1101)
CHANGELOG.md (2)
13-13: Accurate documentation of batch amendment support.The changelog entry correctly documents the new
Batchamendment (XLS-56d) support, which aligns with the extensive code changes in this PR.
15-27: Well-organized changelog improvements.The reorganized and expanded "Fixed" section comprehensively covers the various improvements made alongside the batch transaction feature, including autofilling, typing, and fee handling enhancements.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...current" ledger for extracting sequence number - Increase default maximum payload size...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...r extracting sequence number - Increase default maximum payload size for websocket clie...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
| - Handle autofilling better when multisigning transactions | ||
| - Improve typing for transaction-related helper functions | ||
| - Improve handling of `TicketSequence` | ||
| - Fix issue with failing on a higher than expected fee |
There was a problem hiding this comment.
if you can explain the context of this CL entry, that will be helpful.
There was a problem hiding this comment.
Essentially, the max operator wasn't working properly since it was comparing strings, not numbers.
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.