feat: add support for Batch amendment #2801
Conversation
…o better-scripts
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces several updates across multiple files, primarily focusing on the addition of batch transaction handling within the XRPL ecosystem. Key changes include the introduction of new interfaces and functions for batch transactions, updates to transaction validation mechanisms, and enhancements to the autofill functionality for batch transactions. Additionally, the spell checker configuration is updated to recognize the term "Batchnet." The release history for the Changes
Possibly related PRs
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
Documentation and Community
|
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- ignore type-assertions on the DeliverMax property | ||
| // @ts-expect-error -- DeliverMax property exists only at the RPC level, not at the protocol level | ||
| if (tx.TransactionType === 'Payment' && tx.DeliverMax != null) { |
There was a problem hiding this comment.
This is moved to autofill.ts
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
🧹 Outside diff range and nitpick comments (11)
packages/xrpl/src/models/transactions/ledgerStateFix.ts (1)
10-19: Consider enhancing the interface documentation.While the interface is well-structured, the documentation could be more comprehensive. Consider adding:
- Description of what a LedgerStateFix transaction does
- Purpose and constraints of each field
- Example usage
Example enhancement:
/** * Represents a LedgerStateFix transaction which [describe purpose]. * * @category Transaction Models * @property {number} LedgerFixType - [describe purpose and valid values] * @property {string} [Owner] - [describe when this is used] */packages/ripple-binary-codec/src/hash-prefixes.ts (1)
38-39: Consider adding JSDoc documentation for the batch prefix.For consistency and maintainability, consider adding a JSDoc comment explaining the purpose of the batch prefix, similar to how other prefixes are documented.
paymentChannelClaim: bytes(0x434c4d00), - // batch + // batch transaction data for signing batch: bytes(0x42434800),packages/xrpl/test/models/LedgerStateFix.test.ts (1)
6-10: Consider enhancing the documentation.While the documentation explains the purpose of the test suite, it would be helpful to include:
- Brief description of what a LedgerStateFix transaction is
- Expected use cases
- Link to relevant specifications or documentation
packages/xrpl/src/models/transactions/index.ts (1)
89-90: Consider maintaining alphabetical order for exports.While the new exports are correctly implemented, they should be moved to maintain the alphabetical ordering pattern used throughout the file. The
Batchexport should be placed betweenAMMWithdrawandCheckCancel, andLedgerStateFixshould be placed betweenEnableAmendmentandNFTokenAcceptOffer.Apply this diff to maintain consistency:
export { AMMWithdraw } from './AMMWithdraw' +export { Batch } from './batch' export { CheckCancel } from './checkCancel' // ... other exports export { EnableAmendment, EnableAmendmentFlags } from './enableAmendment' +export { LedgerStateFix } from './ledgerStateFix' export { NFTokenAcceptOffer } from './NFTokenAcceptOffer' // ... remaining exports -export { Batch } from './batch' -export { LedgerStateFix } from './ledgerStateFix'packages/ripple-binary-codec/HISTORY.md (1)
5-6: Enhance the changelog entry for better clarity and consistency.While the entry correctly documents the Batch amendment support, consider these improvements for better documentation:
- Add the "Added" category heading for consistency with other entries
- Include more details about the specific features added, such as:
- Batch transaction handling
- Multi-account transaction signing
- BatchTxn and TxIDs field autofilling
Apply this diff to enhance the entry:
## Unreleased -* Support for the `Batch` amendment (XLS-56). +### Added +* Support for the `Batch` amendment (XLS-56): + * Added support for multi-account Batch transactions + * Added BatchTxn and TxIDs field autofilling + * Added helper functions for signing and combining Batch transactionspackages/ripple-binary-codec/src/binary.ts (2)
180-186: Add JSDoc documentation for BatchObject interface.The interface would benefit from detailed documentation explaining:
- The purpose of the BatchObject
- The meaning and constraints of the
flagsfield- The format requirements for
txIDsAdd this documentation:
/** * Interface describing fields required for a Batch signer + * @property flags - Flags indicating batch transaction properties + * @property txIDs - Array of transaction IDs included in the batch */ interface BatchObject extends JsonObject {
196-201: Use consistent error message quotes.The error messages use different quote styles. Maintain consistency with single quotes.
Update the error messages:
- throw Error("No field `flags'") + throw Error('No field `flags`')packages/ripple-binary-codec/test/signing-data-encoding.test.ts (1)
245-266: Test implementation looks good, consider adding edge cases.The test case effectively validates the basic batch signing functionality with clear setup and expectations. However, consider enhancing test coverage with additional cases:
- Edge cases:
- Empty txIDs array
- Single txID
- Maximum allowed txIDs
- Error cases:
- Invalid txID format
- Invalid flags
Here's a suggested implementation for additional test cases:
it('handles empty txIDs array', function () { const json = { flags: 1, txIDs: [] } const actual = encodeForSigningBatch(json) expect(actual).toBe([ '42434800', // hash prefix '00000001', // flags '00000000', // txIds length ].join('')) }) it('handles single txID', function () { const json = { flags: 1, txIDs: ['ABE4871E9083DF66727045D49DEEDD3A6F166EB7F8D1E92FE868F02E76B2C5CA'] } const actual = encodeForSigningBatch(json) expect(actual).toBe([ '42434800', // hash prefix '00000001', // flags '00000001', // txIds length 'ABE4871E9083DF66727045D49DEEDD3A6F166EB7F8D1E92FE868F02E76B2C5CA' ].join('')) }) it('throws error for invalid txID format', function () { const json = { flags: 1, txIDs: ['invalid_hash'] } expect(() => encodeForSigningBatch(json)).toThrow() })packages/xrpl/test/utils/hashes.test.ts (1)
216-237: Enhance test coverage and documentation for batch transactions.While the basic test case is good, consider the following improvements:
- Add comments explaining the significance of the test values (e.g., NetworkID 21336).
- Add test cases for:
- Invalid BatchTxn fields
- Different BatchIndex values
- Different OuterAccount values
- Missing BatchTxn object
- Document how the expected hash was calculated.
Example of improved test structure:
describe('hashSignedTx - batch transaction', () => { const baseBatchTx: BatchInnerTransaction = { Account: 'rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK', Amount: '1000000', BatchTxn: { BatchIndex: 0, OuterAccount: 'rJCxK2hX9tDMzbnn3cg1GU2g19Kfmhzxkp', Sequence: 470, }, Destination: 'rJCxK2hX9tDMzbnn3cg1GU2g19Kfmhzxkp', Fee: '0', NetworkID: 21336, // Batchnet network identifier Sequence: 0, SigningPubKey: '', TransactionType: 'Payment', } it('hashes a valid batch transaction', () => { assert.equal( hashSignedTx(baseBatchTx), '795AAC88B59E95C3497609749127E69F12958BC016C600C770AEEB1474C840B4', ) }) it('fails when BatchTxn is missing', () => { const { BatchTxn, ...txWithoutBatch } = baseBatchTx assert.throws(() => hashSignedTx(txWithoutBatch), ValidationError) }) it('generates different hashes for different BatchIndex values', () => { const txWithDifferentIndex = { ...baseBatchTx, BatchTxn: { ...baseBatchTx.BatchTxn, BatchIndex: 1 }, } assert.notEqual(hashSignedTx(baseBatchTx), hashSignedTx(txWithDifferentIndex)) }) })packages/xrpl/src/sugar/autofill.ts (1)
425-446: Avoid Mutating Original Transaction ObjectsIn
autofillBatchTxn, properties are directly added to the original transaction objects:txn.BatchTxn = batchTxn as BatchTxnMutating the original objects can lead to unintended side effects, especially if these objects are used elsewhere. Consider creating a deep copy of each transaction before modifying it.
Example adjustment:
const txn = { ...rawTxn.RawTransaction } // Creates a shallow copy // Proceed to modify txn safelypackages/xrpl/src/Wallet/batchSigner.ts (1)
180-185: Update documentation link to reflect 'Batch' transactionsThe comment references multi-signing and links to https://xrpl.org/multi-signing.html. Since this file pertains to batch transactions, consider updating the link to point to the relevant documentation for batch transactions to provide clearer context.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (27)
- .vscode/settings.json (1 hunks)
- packages/ripple-binary-codec/HISTORY.md (1 hunks)
- packages/ripple-binary-codec/src/binary.ts (2 hunks)
- packages/ripple-binary-codec/src/enums/definitions.json (11 hunks)
- packages/ripple-binary-codec/src/hash-prefixes.ts (1 hunks)
- packages/ripple-binary-codec/src/index.ts (4 hunks)
- packages/ripple-binary-codec/test/signing-data-encoding.test.ts (2 hunks)
- packages/xrpl/HISTORY.md (1 hunks)
- packages/xrpl/package.json (1 hunks)
- packages/xrpl/src/Wallet/batchSigner.ts (1 hunks)
- packages/xrpl/src/Wallet/defaultFaucets.ts (2 hunks)
- packages/xrpl/src/Wallet/index.ts (1 hunks)
- packages/xrpl/src/client/index.ts (2 hunks)
- packages/xrpl/src/models/transactions/batch.ts (1 hunks)
- packages/xrpl/src/models/transactions/common.ts (2 hunks)
- packages/xrpl/src/models/transactions/index.ts (1 hunks)
- packages/xrpl/src/models/transactions/ledgerStateFix.ts (1 hunks)
- packages/xrpl/src/models/transactions/metadata.ts (2 hunks)
- packages/xrpl/src/models/transactions/transaction.ts (6 hunks)
- packages/xrpl/src/sugar/autofill.ts (4 hunks)
- packages/xrpl/src/utils/hashes/hashLedger.ts (1 hunks)
- packages/xrpl/test/client/autofill.test.ts (1 hunks)
- packages/xrpl/test/models/Batch.test.ts (1 hunks)
- packages/xrpl/test/models/LedgerStateFix.test.ts (1 hunks)
- packages/xrpl/test/utils/hashes.test.ts (2 hunks)
- packages/xrpl/test/wallet/authorizeChannel.test.ts (1 hunks)
- packages/xrpl/test/wallet/batchSigner.test.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/xrpl/src/sugar/autofill.ts
[error] 405-406: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/Batch.test.ts
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/LedgerStateFix.test.ts
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 Gitleaks
packages/xrpl/test/client/autofill.test.ts
448-448: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
455-455: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
502-502: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
509-509: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (52)
packages/xrpl/src/models/transactions/ledgerStateFix.ts (2)
1-8: LGTM! Imports are well-organized and properly scoped.The imports are specific and all are utilized within the implementation.
21-33: 🛠️ Refactor suggestionConsider strengthening the validation logic.
While the basic validation is in place, consider these enhancements:
- Validate that TransactionType is specifically 'LedgerStateFix'
- Add range validation for LedgerFixType if there are valid value constraints
- If Owner is an account address, validate its format
Let's check if there are any constraints on LedgerFixType values:
Consider enhancing the validation:
export function validateLedgerStateFix(tx: Record<string, unknown>): void { validateBaseTransaction(tx) + if (tx.TransactionType !== 'LedgerStateFix') { + throw new Error('Transaction type must be LedgerStateFix') + } + validateRequiredField(tx, 'LedgerFixType', isNumber) + const ledgerFixType = tx.LedgerFixType as number + // Add range validation if applicable + // if (ledgerFixType < MIN || ledgerFixType > MAX) { + // throw new Error('Invalid LedgerFixType value') + // } validateOptionalField(tx, 'Owner', isString) + if (tx.Owner !== undefined) { + // Add account address format validation if applicable + // validateAccountAddress(tx.Owner as string) + } }packages/ripple-binary-codec/src/hash-prefixes.ts (1)
38-39: LGTM! The new batch prefix follows established patterns.The implementation correctly follows the existing pattern of hash prefixes, using ASCII characters (BCH) followed by a null terminator.
packages/xrpl/test/wallet/authorizeChannel.test.ts (1)
6-30: LGTM! Test organization improved with describe block.The addition of the describe block enhances test organization and follows testing best practices.
.vscode/settings.json (1)
6-6: LGTM! Verify the term usage across the codebase.The addition of "Batchnet" to the spell checker dictionary is properly formatted and positioned alphabetically.
Let's verify the usage of this term in the codebase:
✅ Verification successful
The term "Batchnet" is correctly used in the codebase
The verification confirms that "Batchnet" is actively used in
packages/xrpl/src/Wallet/defaultFaucets.tswhere it's:
- Defined as a faucet endpoint
- Mapped in the faucet network configuration
- Used in network detection logic
The addition to the spell checker dictionary is justified and matches the actual usage in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that "Batchnet" is actually used in the codebase # and not just added to the dictionary without purpose # Test: Search for "Batchnet" usage rg -i '\bBatchnet\b' --type-add 'code:*.{ts,js,json}' -t codeLength of output: 396
packages/xrpl/src/Wallet/defaultFaucets.ts (3)
17-17: Verify the security and reliability of the external faucet service.The new Batchnet faucet URL points to a third-party domain (nerdnest.xyz). Please ensure:
- The service is reliable and maintained
- The service uses HTTPS for secure communication
- There's a fallback mechanism if the service is unavailable
23-23: LGTM! Consistent with existing network paths.The Batchnet path mapping follows the established pattern and maintains API consistency with other networks.
Line range hint
17-43: Verify Batchnet faucet endpoint configuration across the codebase.Let's ensure the Batchnet faucet endpoint is consistently referenced and properly configured throughout the codebase.
✅ Verification successful
Let me generate additional verification scripts to get a more complete picture of the faucet implementation and its usage.
This will help us verify:
- Test coverage for faucet functionality
- Consistency in client URL parsing logic
- Environment configurations for faucets
- Overall implementation pattern of faucet-related functions
Let me try one final verification with modified patterns to better understand the faucet implementation.
Based on the comprehensive verification results, I can now provide a final response:
Batchnet faucet implementation is properly configured and consistent
The verification shows that:
- The Batchnet faucet URL is properly defined in
FaucetNetworkenum- The faucet path
/accountsis consistently configured inFaucetNetworkPaths- The
getFaucetHostfunction correctly handles Batchnet URL detection- The implementation is covered by test cases in
fundWallet.test.ts- The URL validation pattern follows the same consistent pattern as other networks (Testnet, Devnet)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of Batchnet faucet URL and related configurations # Test 1: Search for any hardcoded references to the Batchnet faucet URL echo "Checking for Batchnet faucet URL references..." rg -i "batch.faucet.nerdnest.xyz" # Test 2: Search for any Batchnet-related configuration or environment variables echo "Checking for Batchnet-related configurations..." rg -i "batchnet.*faucet|faucet.*batchnet" # Test 3: Search for any test files using the Batchnet faucet echo "Checking for Batchnet faucet usage in tests..." rg -i "batchnet" -g "*test*"Length of output: 748
Script:
#!/bin/bash # Description: Perform deeper analysis of faucet implementation and usage # Test 1: Check for faucet-related test files echo "Checking for faucet-related test files..." fd -g "*faucet*test*.{ts,js}" # Test 2: Check for faucet implementation details echo "Checking for faucet implementation details..." ast-grep --pattern 'function $FUNC($CLIENT: Client) { $$$ return FaucetNetwork.$NET $$$ }' # Test 3: Check for any other client URL parsing logic echo "Checking for client URL parsing..." rg -B2 -A2 "client.*url.*includes" # Test 4: Check for any environment configurations related to faucets echo "Checking for faucet environment configurations..." fd -g "*.{json,env}*" --exec rg -i "faucet|batchnet"Length of output: 604
Script:
#!/bin/bash # Description: Final verification of faucet implementation # Test 1: Check for all faucet-related files echo "Checking for all faucet-related files..." fd "faucet" --type f # Test 2: Look for any faucet-related tests with different naming echo "Checking for faucet tests with different naming..." rg -l "getFaucetHost|FaucetNetwork" -g "*test*.{ts,js}" # Test 3: Check for any network-related configurations echo "Checking for network configurations..." rg -B3 -A3 "FaucetNetwork|getFaucetHost" # Test 4: Check for any client URL validation patterns echo "Checking for URL validation patterns..." rg -B2 -A2 "connectionUrl.*includes"Length of output: 10754
packages/xrpl/test/models/LedgerStateFix.test.ts (2)
43-71: LGTM! Comprehensive validation testing.The test cases for invalid field types are well structured and verify both specific and general validation functions.
1-72: Verify LedgerStateFix transaction type registration.Let's ensure the LedgerStateFix transaction type is properly registered in the transaction types definition.
🧰 Tools
🪛 Biome
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/transactions/metadata.ts (2)
3-3: LGTM! Import statement follows consistent pattern.The import statement correctly brings in the required types for batch transaction support, maintaining consistency with other transaction-related imports.
101-102: LGTM! Verify BatchMetadata interface structure.The type extension correctly follows the established pattern for transaction-specific metadata.
Let's verify that the BatchMetadata interface properly extends TransactionMetadataBase:
✅ Verification successful
The verification results show that
BatchMetadatainterface is properly defined inpackages/xrpl/src/models/transactions/batch.tsand correctly extendsTransactionMetadataBase. Let me verify theTransactionMetadataBaseinterface to ensure complete type safety.
Type extension is correctly implemented and type-safe ✅
The verification confirms that:
BatchMetadataproperly extendsTransactionMetadataBase- Both interfaces are well-defined with appropriate fields
- The type extension maintains type safety by inheriting all base metadata fields while adding batch-specific
BatchExecutionsarray🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify BatchMetadata interface structure # Expected: BatchMetadata should extend TransactionMetadataBase # Search for BatchMetadata interface definition ast-grep --pattern 'interface BatchMetadata extends TransactionMetadataBase { $$$ }'Length of output: 719
Script:
#!/bin/bash # Search for TransactionMetadataBase interface definition ast-grep --pattern 'interface TransactionMetadataBase { $$$ }'Length of output: 748
packages/xrpl/package.json (2)
67-67: Clarify the test configuration change.The test:watch script has been modified to use unit tests (jest.config.unit.js) instead of integration tests. While this might be intentional, please confirm if:
- This change was deliberate and not accidental
- Developers are aware that watch mode will now only run unit tests
- There's a specific reason for excluding integration tests from watch mode
Line range hint
2-2: Verify the major version bump aligns with the changes.The version bump to 4.0.0 indicates a breaking change. While adding Batch transaction support is a significant feature, please ensure the HISTORY.md has been properly updated to document all breaking changes that justify this major version increment.
packages/xrpl/test/models/Batch.test.ts (2)
1-11: LGTM! Well-structured test setup with clear documentation.The imports are appropriate, and the JSDoc documentation clearly describes the purpose of these tests.
14-71: Verify test data alignment with implementation.Let's verify that the test data structure matches the actual implementation requirements.
✅ Verification successful
Let me gather more information about the Batch transaction implementation to properly verify the test data alignment.
Based on the gathered information, I can now generate the final response as we have the interface definitions and validation requirements to compare against the test data.
Test data correctly aligns with implementation requirements
The test data structure in
Batch.test.tsperfectly matches theBatchinterface requirements:
- Has required
TransactionType: 'Batch'- Contains
BatchSignersarray with correctBatchSignerinterface structure- Includes
RawTransactionsarray with properBatchInnerTransactionstructure including:
- Required
BatchTxnfield withOuterAccount,Sequence, andBatchIndex- Transaction-specific fields (
Account,Amount,Destination, etc.)- Contains optional
TxIDsarray🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Batch transaction type definition and validation rules ast-grep --pattern 'interface Batch { $$$ }' # Look for any additional validation rules in the implementation rg -A 5 'function validateBatch'Length of output: 1115
Script:
#!/bin/bash # Look for Batch interface definition and type requirements rg -A 15 "export (interface|type) Batch" # Search for RawTransaction interface/type definition rg -A 10 "export (interface|type) RawTransaction" # Look for BatchSigner interface/type definition rg -A 5 "export (interface|type) BatchSigner" # Search for any additional validation rules specific to batch transactions rg -A 5 "function validate.*Batch" --type typescriptLength of output: 4408
packages/ripple-binary-codec/src/index.ts (4)
3-3: LGTM: Import follows existing patternsThe BatchObject import is correctly placed and follows the established naming convention.
18-18: LGTM: Consistent destructuring patternThe signingBatchData addition maintains the established pattern and ordering.
153-153: LGTM: Export follows established patternsThe export of encodeForSigningBatch maintains consistent ordering and naming.
3-3: Verify batch transaction implementation dependenciesLet's verify the supporting implementations for batch transactions:
Also applies to: 18-18, 114-119
✅ Verification successful
Let me gather more information about the batch transaction implementation to make a comprehensive verification.
Let me also check if there are any test files specifically for batch operations:
And let's look at the actual implementation of signingBatchData:
Let me check one final aspect - the test implementation details to ensure proper test coverage:
Batch transaction implementation is properly supported
Based on the verification results, the batch transaction implementation is complete and well-structured:
BatchObjectinterface is properly defined with required fields (flagsandtxIDs)signingBatchDatafunction is implemented with appropriate validation and serialization- Test coverage exists with a specific test case that verifies:
- Batch blob creation
- Proper serialization format including hash prefix, flags, and transaction IDs
- Expected hexadecimal output
The implementation follows the expected pattern for transaction serialization in the codebase and includes proper error handling for missing required fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check BatchObject interface and signingBatchData implementation echo "Checking BatchObject interface definition:" rg -p "interface BatchObject" ./packages/ripple-binary-codec/src/ echo "\nChecking signingBatchData implementation:" rg -p "function signingBatchData" ./packages/ripple-binary-codec/src/ echo "\nChecking for batch transaction tests:" rg -p "describe.*[Bb]atch" ./packages/ripple-binary-codec/test/Length of output: 687
Script:
#!/bin/bash # Check BatchObject interface implementation details echo "BatchObject interface and implementation details:" rg -A 10 "interface BatchObject" ./packages/ripple-binary-codec/src/binary.ts echo "\nChecking BatchObject usage:" rg "BatchObject" ./packages/ripple-binary-codec/src/ -A 3 echo "\nChecking for batch-related tests (broader search):" rg "batch" ./packages/ripple-binary-codec/test/ -iLength of output: 2502
Script:
#!/bin/bash fd "batch" ./packages/ripple-binary-codec/test/Length of output: 47
Script:
#!/bin/bash rg -A 10 "function signingBatchData" ./packages/ripple-binary-codec/src/binary.tsLength of output: 451
Script:
#!/bin/bash # Get the full test implementation for batch-related tests echo "Batch test implementation:" rg -A 20 "can create batch blob" ./packages/ripple-binary-codec/test/signing-data-encoding.test.ts # Check for any batch-related fixtures or test data echo "\nChecking for batch test fixtures:" fd "batch" ./packages/ripple-binary-codec/test/fixtures/Length of output: 970
packages/ripple-binary-codec/src/binary.ts (2)
223-223: LGTM!The export updates correctly expose the new batch-related functionality while maintaining alphabetical ordering.
Also applies to: 233-233
202-204: Verify HashPrefix.batch and binary codec integration.Let's verify the integration points for this new functionality.
✅ Verification successful
Let me gather more specific information about the HashPrefix definition and integration.
Let me check one more thing to verify the HashPrefix.batch definition since it wasn't visible in the previous results.
Let me check one final thing - the content of the hash-prefixes file that's being imported.
Based on all the gathered information, I can now provide the final response:
HashPrefix.batch is properly defined and integrated
The verification shows that:
- HashPrefix.batch is correctly defined in ripple-binary-codec (0x42434800 - "BCH")
- The binary codec integration is complete with proper exports
- Test coverage exists in packages/xrpl/test/models/Batch.test.ts
- The implementation follows the same pattern as other hash prefix usages in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify HashPrefix.batch definition and binary codec integration # Test 1: Check if HashPrefix.batch is properly defined echo "Checking HashPrefix.batch definition..." rg -l "batch.*=.*0x" packages/ # Test 2: Check usage of BatchObject and signingBatchData echo "Checking usage of new batch functionality..." rg -l "import.*BatchObject|import.*signingBatchData" packages/ # Test 3: Check for test coverage echo "Checking test coverage..." rg -l "describe.*Batch|test.*Batch" packages/Length of output: 522
Script:
#!/bin/bash # Check HashPrefix definition and its usage echo "Checking HashPrefix definition..." rg -A 5 "enum HashPrefix" packages/ # Check the actual implementation in binary.ts echo "Checking binary.ts implementation..." rg -A 10 "const prefix = HashPrefix\.batch" packages/ # Check imports and exports echo "Checking batch-related exports..." rg "export.*BatchObject|export.*signingBatchData" packages/ # Check test implementations for coverage echo "Checking test implementations..." rg -A 5 "describe\('Batch" packages/Length of output: 2189
Script:
#!/bin/bash # Search for batch-related constants in HashPrefix echo "Searching for batch-related constants in HashPrefix..." rg -B 2 -A 2 "batch.*=.*0x|BATCH.*=.*0x" packages/ # Check if there are any imports of HashPrefix in binary.ts echo "Checking HashPrefix imports in binary.ts..." rg "import.*HashPrefix" packages/ripple-binary-codec/src/binary.ts # Check the complete binary.ts file for context echo "Checking complete binary.ts..." cat packages/ripple-binary-codec/src/binary.tsLength of output: 7201
Script:
#!/bin/bash # Check the hash-prefixes file content echo "Checking hash-prefixes.ts content..." cat packages/ripple-binary-codec/src/hash-prefixes.ts # Also check if there's another HashPrefix definition in xrpl package echo "Checking xrpl package HashPrefix..." cat packages/xrpl/src/utils/hashes/HashPrefix.tsLength of output: 2485
packages/xrpl/src/utils/hashes/hashLedger.ts (1)
87-90:⚠️ Potential issueReview the TypeScript error suppression approach.
While the change correctly adds support for batch transactions, the use of
@ts-expect-errorsuggests a potential type definition issue that should be addressed more systematically.Consider these improvements:
- Instead of suppressing the TypeScript error, define proper types for batch transactions:
interface BatchTransaction extends Transaction { BatchTxn?: { // Define the expected structure of BatchTxn } }
- Update the function signature to use the new type:
-export function hashSignedTx(tx: Transaction | string): string { +export function hashSignedTx(tx: BatchTransaction | string): string {This approach would:
- Provide better type safety
- Make the batch transaction support more explicit
- Remove the need for error suppression
- Improve code maintainability
Let's verify if the BatchTransaction type is already defined elsewhere:
✅ Verification successful
Let me try different search patterns to gather more information about batch transactions in the codebase.
The previous commands failed due to incorrect file type specification. These new commands should help us understand:
- Where BatchTxn is being used
- If there are any existing batch-related type definitions
- Any files that might be related to batch transactions
This will help us make an informed decision about the TypeScript error suppression approach.
Let me check the actual type definitions in the batch.ts file to make a final assessment.
This will help us understand:
- The existing BatchTxn interface and related types
- How Transaction type is defined and whether it needs modification
Based on the gathered information, I can now provide a final response:
The TypeScript error suppression is appropriate in this context.
The code is actually well-structured with proper type definitions. The
BatchTxninterface andBatchInnerTransactiontype are already defined inpackages/xrpl/src/models/transactions/batch.ts. The@ts-expect-errorusage is justified because:
- The
BatchTxnproperty is properly typed through theBatchInnerTransactiontype which extendsTransaction- The error suppression is specifically scoped to the runtime type checking in
hashSignedTx- The type assertion is necessary due to the dynamic nature of transaction validation where properties need to be checked for undefined
The current implementation maintains type safety through:
- Clear interface definitions (
BatchTxn,BatchInnerTransaction)- Runtime validation (
validateBatchfunction)- Proper import and usage patterns (seen in
autofill.tsand other files)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing BatchTransaction type definitions rg -t typescript "interface.*Batch.*Transaction" # Search for BatchTxn property usage ast-grep --pattern 'interface $_ { $$$ BatchTxn?: $_ $$$ }'Length of output: 153
Script:
#!/bin/bash # Search for BatchTxn usage in TypeScript files rg --type-add 'ts:*.ts' --type ts 'BatchTxn' # Search for any batch transaction related types or interfaces rg --type-add 'ts:*.ts' --type ts 'type.*Batch|interface.*Batch' # Look for any files that might contain batch transaction definitions fd -e ts -e tsx batch -iLength of output: 3013
Script:
#!/bin/bash # Get the content of the batch.ts file to understand the type definitions cat packages/xrpl/src/models/transactions/batch.ts # Check if Transaction type already includes BatchTxn rg --type-add 'ts:*.ts' --type ts "interface Transaction"Length of output: 4781
packages/ripple-binary-codec/test/signing-data-encoding.test.ts (1)
6-6: LGTM: Import addition is correct.The
encodeForSigningBatchimport is properly added to support the new batch transaction functionality.packages/xrpl/test/utils/hashes.test.ts (1)
13-13: LGTM!The import statement follows the existing pattern and is correctly placed with other transaction-related imports.
packages/xrpl/src/models/transactions/transaction.ts (4)
17-17: LGTM: Import statements follow the established pattern.The new imports for
BatchandLedgerStateFixare properly organized and consistent with the codebase's import style.Also applies to: 30-30
110-110: LGTM: Type definitions properly updated.The
BatchandLedgerStateFixtypes are correctly added to theSubmittableTransactionunion type, maintaining alphabetical ordering.Also applies to: 121-121
326-329: LGTM: LedgerStateFix validation follows established patterns.The validation logic for
LedgerStateFixis properly implemented, following the same pattern as other transaction types.
273-284: Review the Batch validation implementation for robustness.The current implementation has several areas that warrant attention:
Type Safety:
- The code uses
@ts-expect-errorand type assertions, suggesting potential type system issues- The comment about "dependency cycles" needs more context
Error Handling:
- Consider enhancing error messages to clearly indicate which inner transaction failed validation
- Add context about the batch index when an inner transaction fails
Performance:
- For large batches, recursive validation could be computationally expensive
- Consider adding a maximum limit to the number of inner transactions
Let's verify the implementation of
Batchtransaction validation:packages/xrpl/src/models/transactions/common.ts (2)
Line range hint
192-240: LGTM! Well-structured parameter handling.The changes to validation functions are well-designed:
- Clear separation between field access (
param) and error reporting (paramName)- Maintains backward compatibility
- Uses appropriate nullish coalescing for parameter name fallback
Line range hint
167-240: Verify batch transaction validation coverage.The new utility functions and validation improvements align well with batch transaction support. However, ensure that batch transaction validation is thoroughly tested, particularly edge cases involving arrays and nested objects.
Let's verify the test coverage:
✅ Verification successful
Let's examine the test files specifically related to batch transactions to verify the validation coverage.
Let's examine the actual validation implementation in the Batch transaction model.
Based on the evidence gathered, I can now provide a final response:
Batch transaction validation is properly implemented and tested
The validation implementation and test coverage show:
- Required fields like
RawTransactionsare properly validated- Optional fields like
BatchSignersandTxIDsare properly validated- Edge cases are covered with tests for:
- Missing required fields
- Invalid field types
- Invalid array contents
- Base transaction validation
The new utility functions are well integrated into the batch transaction validation system with comprehensive test coverage for both positive and negative cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for batch transaction validation # Test: Look for batch transaction test files fd -e test.ts -e spec.ts batch # Test: Check for validation test cases in test files rg -l "describe.*validation|test.*validation|it.*validation" $(fd -e test.ts -e spec.ts)Length of output: 10403
Script:
#!/bin/bash # Check validation test coverage in batch-related files # Look for validation tests in Batch.test.ts echo "=== Batch.test.ts validation tests ===" rg "describe|it" packages/xrpl/test/models/Batch.test.ts -A 2 # Look for validation tests in batchSigner.test.ts echo -e "\n=== batchSigner.test.ts validation tests ===" rg "describe|it" packages/xrpl/test/wallet/batchSigner.test.ts -A 2 # Look for validation function usage in the implementation echo -e "\n=== Validation function usage in batch implementation ===" rg "validateRequiredField|validateOptionalField" packages/xrpl/src/ -A 2Length of output: 27789
Script:
#!/bin/bash # Check Batch transaction validation implementation # Look for Batch validation code echo "=== Batch validation implementation ===" rg -A 10 "function validate.*Batch" packages/xrpl/src/models/transactions/batch.ts # Look for Batch model tests specifically focused on validation echo -e "\n=== Batch validation test cases ===" rg -B 2 -A 5 "throws|doesNotThrow" packages/xrpl/test/models/Batch.test.tsLength of output: 2330
packages/ripple-binary-codec/src/enums/definitions.json (9)
263-272: LGTM: BatchIndex field definition is correct.The new
BatchIndexfield is properly defined as aUInt8type with appropriate serialization and signing flags. The field number (20) follows the sequential ordering within the UInt8 type group.
383-392: LGTM: LedgerFixType field definition is correct.The new
LedgerFixTypefield is properly defined as aUInt16type with appropriate serialization and signing flags. The field number (21) follows the sequential ordering within the UInt16 type group.
2003-2012: LGTM: InnerResult field definition is correct.The new
InnerResultfield is properly defined as aBlobtype with appropriate serialization and signing flags. The field number (30) follows the sequential ordering within the Blob type group.
2173-2182: LGTM: OuterAccount field definition is correct.The new
OuterAccountfield is properly defined as anAccountIDtype with appropriate serialization and signing flags. The field number (24) follows the sequential ordering within the AccountID type group.
2223-2232: LGTM: TxIDs field definition is correct.The new
TxIDsfield is properly defined as aVector256type with appropriate serialization and signing flags. The field number (5) follows the sequential ordering within the Vector256 type group.
2603-2642: LGTM: STObject field definitions are correct.The new STObject fields are properly defined with appropriate serialization and signing flags:
RawTransaction(nth: 33)BatchExecution(nth: 34)BatchTxn(nth: 35)BatchSigner(nth: 36)All field numbers follow the sequential ordering within the STObject type group.
2832-2861: LGTM: STArray field definitions are correct.The new STArray fields are properly defined with appropriate serialization and signing flags:
BatchExecutions(nth: 26)RawTransactions(nth: 27)BatchSigners(nth: 28, non-signing field)All field numbers follow the sequential ordering within the STArray type group.
2931-2931: LGTM: Transaction result codes are properly defined.The new transaction result codes are added in appropriate categories:
temINVALID_BATCH(-251): Malformed transaction errortecBATCH_FAILURE(192): Claim-engine errorThe values follow the existing pattern and numbering scheme.
Also applies to: 3048-3049
3100-3101: LGTM: Transaction types are properly defined.The new transaction types are added with sequential values:
LedgerStateFix(53)Batch(54)The values follow the existing pattern and numbering scheme.
packages/xrpl/HISTORY.md (4)
9-10: Add support forBatchamendmentThe changelog indicates that support for the
Batchamendment has been added. This is a significant feature addition that enables batch transaction functionality within the XRPL.js library.
Line range hint
12-24: Major version update with breaking changes and new featuresVersion 4.0.0 introduces significant changes:
- Breaking change: Uses rippled API version v2 as default while maintaining v1 support
- New features added:
nfts_by_issuerAPI definitioninclude_deletedparameter for ledger entry requests- Support for
fixPreviousTxnIDamendment- Support for user version of
featureRPC- Added
hashfield toledgercommand response- Removal of Hooks testnet faucet references
This is a well-documented major version update that follows semantic versioning principles.
Line range hint
26-42: Version 3.1.0 brings important changes and improvementsNotable changes include:
- Breaking changes:
- Flag name change from
tfNoRippleDirecttotfNoDirectRipple- Node.js minimum version upgrade to 18
- Native JavaScript environment for
fetch- New features:
- Support for Price Oracles amendment (XLS-47)
- Multiple bug fixes including:
- Channel type typo fix
- Client request handling improvements
- AMMDeposit flag additions
- Metadata type improvements
This release shows good attention to both feature additions and bug fixes while maintaining backward compatibility where possible.
Line range hint
44-82: Major version 3.0.0 modernizes the codebaseThis is a significant update that includes:
- Breaking changes:
- TypeScript upgrade to 5.x
- Removal of Node 14 support
- Replacement of crypto polyfills with modern alternatives
- Changes to proxy configuration
- Redefinition of Transaction types
- Switch from string to number for certain return values
- Buffer replaced with UInt8Array
- Bundling improvements:
- Simplified bundler configurations
- Removal of multiple polyfills
- Non-breaking changes:
- Deprecation of certain utility functions in favor of newer alternatives
This update represents a significant modernization of the codebase while maintaining clear documentation of breaking changes.
packages/xrpl/test/wallet/batchSigner.test.ts (1)
53-54: Verify the validity of 'Fee' and 'Sequence' valuesThe inner transactions have
FeeandSequencefields set to'0'. Please confirm that settingFeeandSequenceto'0'is acceptable for Batch transactions and conforms to the ledger's requirements.Also applies to: 70-71
packages/xrpl/src/sugar/autofill.ts (1)
373-407: Re-evaluate the Logic forDeliverMaxandAmountFieldsThe current implementation requires
DeliverMaxandAmountto be identical if both are provided:if (tx.Amount != null && tx.Amount !== tx.DeliverMax) { throw new ValidationError( 'PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided', ) }In the XRPL protocol,
DeliverMaxis used in partial payments to specify the maximum amount the sender is willing to deliver, which can differ fromAmount. Forcing them to be identical may prevent valid use cases of partial payments.Please verify whether this constraint aligns with the intended functionality. If partial payments are to be supported, consider adjusting the logic to accommodate differing values for
DeliverMaxandAmount.🧰 Tools
🪛 Biome
[error] 405-406: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/Wallet/index.ts (1)
410-413: Validation added for Batch inner transactionsThe added check correctly prevents signing Batch inner transactions using this method, enhancing transaction validation and error handling.
packages/xrpl/src/client/index.ts (2)
65-66: Imports added correctly.The import statements for
autofillBatchTxnandhandleDeliverMaxare correctly added.
690-691: Updated handling of 'Payment' transactions withhandleDeliverMax.The code correctly replaces previous logic with a call to
handleDeliverMaxfor 'Payment' transactions.packages/xrpl/src/Wallet/batchSigner.ts (3)
190-194: Good implementation of 'compareBatchSigners' functionThe
compareBatchSignersfunction correctly compares signer accounts numerically to ensure proper sorting of theBatchSignersarray, which is essential for transaction validation.
55-83:⚠️ Potential issueEnsure consistent structure of 'BatchSigner'
The structure of
BatchSignerdiffers based on themultisignAddresscondition:
- When
multisignAddressis provided,BatchSignerincludes aSignersarray containing aSignerobject.- When
multisignAddressis not provided,BatchSignercontainsSigningPubKeyandTxnSignaturedirectly.Please verify that this discrepancy is intentional and aligns with the expected format for batch transactions. Ensuring a consistent structure improves maintainability and reduces potential errors.
170-178:⚠️ Potential issueVerify exclusion of transaction account from 'BatchSigners'
In the
getTransactionWithAllBatchSignersfunction, the code filters out anyBatchSignerwhereBatchSigner.Accountmatchestransactions[0].Account:.filter((signer) => signer.BatchSigner.Account !== transactions[0].Account)Please verify if excluding the transaction's own account from the
BatchSignersarray is intentional and compliant with the protocol specifications. If the transaction's account should include its own signature inBatchSigners, this filter may omit necessary data.
| const secpWallet = Wallet.fromSeed('snGHNrPbHrdUcszeuDEigMdC1Lyyd', { | ||
| algorithm: ECDSA.secp256k1, | ||
| }) | ||
| const channelId = | ||
| '5DB01B7FFED6B67E6B0414DED11E051D2EE2B7619CE0EAA6286D67A3A4D5BDB3' | ||
| const amount = '1000000' | ||
|
|
||
| assert.equal( | ||
| authorizeChannel(secpWallet, channelId, amount), | ||
| '304402204E7052F33DDAFAAA55C9F5B132A5E50EE95B2CF68C0902F61DFE77299BC893740220353640B951DCD24371C16868B3F91B78D38B6F3FD1E826413CDF891FA8250AAC', | ||
| ) | ||
| }) | ||
| assert.equal( | ||
| authorizeChannel(secpWallet, channelId, amount), | ||
| '304402204E7052F33DDAFAAA55C9F5B132A5E50EE95B2CF68C0902F61DFE77299BC893740220353640B951DCD24371C16868B3F91B78D38B6F3FD1E826413CDF891FA8250AAC', | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing test robustness.
While the current tests verify the core functionality, consider adding the following test cases for improved coverage:
- Invalid channel ID format
- Invalid amount format
- Edge cases (empty values, extremely large amounts)
- Error scenarios
This would ensure the function handles error cases gracefully.
Example test structure:
describe('authorizeChannel', function () {
// Existing happy path tests...
describe('error cases', function () {
it('throws on invalid channel ID format', function () {
const wallet = Wallet.fromSeed('snGHNrPbHrdUcszeuDEigMdC1Lyyd', {
algorithm: ECDSA.secp256k1,
})
assert.throws(() => {
authorizeChannel(wallet, 'invalid-id', '1000000')
}, /Invalid channel ID format/)
})
it('throws on invalid amount format', function () {
const wallet = Wallet.fromSeed('snGHNrPbHrdUcszeuDEigMdC1Lyyd', {
algorithm: ECDSA.secp256k1,
})
const channelId = '5DB01B7FFED6B67E6B0414DED11E051D2EE2B7619CE0EAA6286D67A3A4D5BDB3'
assert.throws(() => {
authorizeChannel(wallet, channelId, 'invalid-amount')
}, /Invalid amount format/)
})
})
})Also applies to: 22-29
| if (connectionUrl.includes('batchnet')) { | ||
| return FaucetNetwork.Batchnet | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using more specific URL matching for Batchnet.
The current implementation checks for any URL containing 'batchnet', which could potentially match unintended URLs. Consider using a more specific check, such as checking the hostname directly.
- if (connectionUrl.includes('batchnet')) {
+ const url = new URL(connectionUrl)
+ if (url.hostname.includes('batchnet')) {
return FaucetNetwork.Batchnet
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (connectionUrl.includes('batchnet')) { | |
| return FaucetNetwork.Batchnet | |
| } | |
| const url = new URL(connectionUrl) | |
| if (url.hostname.includes('batchnet')) { | |
| return FaucetNetwork.Batchnet | |
| } |
| beforeEach(function () { | ||
| tx = { | ||
| Account: 'rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn', | ||
| TransactionType: 'LedgerStateFix', | ||
| LedgerFixType: 1, | ||
| Owner: 'rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn', | ||
| } as any | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety in test fixtures.
Instead of using as any, consider:
- Creating a proper type for the test transaction
- Using a partial type with required fields
interface LedgerStateFixTransaction {
Account: string
TransactionType: 'LedgerStateFix'
LedgerFixType: number
Owner: string
}
beforeEach(function () {
tx = {
Account: 'rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn',
TransactionType: 'LedgerStateFix',
LedgerFixType: 1,
Owner: 'rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn',
} satisfies LedgerStateFixTransaction
})| it('verifies valid LedgerStateFix', function () { | ||
| assert.doesNotThrow(() => validateLedgerStateFix(tx)) | ||
| assert.doesNotThrow(() => validate(tx)) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more test cases for edge scenarios.
The current test suite covers basic validation, but consider adding:
- Boundary value tests for LedgerFixType (min/max values)
- Tests for empty or null Owner field
- Tests for malformed account addresses
Also, instead of using the delete operator which can impact performance, consider:
it('throws w/ missing LedgerFixType', function () {
- delete tx.LedgerFixType
+ tx = {
+ ...tx,
+ LedgerFixType: undefined,
+ }Also applies to: 28-41
| beforeEach(function () { | ||
| tx = { | ||
| Account: 'rJCxK2hX9tDMzbnn3cg1GU2g19Kfmhzxkp', | ||
| BatchSigners: [ | ||
| { | ||
| BatchSigner: { | ||
| Account: 'rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK', | ||
| SigningPubKey: | ||
| '02691AC5AE1C4C333AE5DF8A93BDC495F0EEBFC6DB0DA7EB6EF808F3AFC006E3FE', | ||
| TxnSignature: | ||
| '30450221008E595499C334127A23190F61FB9ADD8B8C501D543E37945B11FABB66B097A6130220138C908E8C4929B47E994A46D611FAC17AB295CFB8D9E0828B32F2947B97394B', | ||
| }, | ||
| }, | ||
| ], | ||
| Flags: 1, | ||
| RawTransactions: [ | ||
| { | ||
| RawTransaction: { | ||
| Account: 'rJCxK2hX9tDMzbnn3cg1GU2g19Kfmhzxkp', | ||
| Amount: '5000000', | ||
| BatchTxn: { | ||
| BatchIndex: 1, | ||
| OuterAccount: 'rJCxK2hX9tDMzbnn3cg1GU2g19Kfmhzxkp', | ||
| Sequence: 215, | ||
| }, | ||
| Destination: 'rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK', | ||
| Fee: '0', | ||
| NetworkID: 21336, | ||
| Sequence: 0, | ||
| SigningPubKey: '', | ||
| TransactionType: 'Payment', | ||
| }, | ||
| }, | ||
| { | ||
| RawTransaction: { | ||
| Account: 'rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK', | ||
| Amount: '1000000', | ||
| BatchTxn: { | ||
| BatchIndex: 0, | ||
| OuterAccount: 'rJCxK2hX9tDMzbnn3cg1GU2g19Kfmhzxkp', | ||
| Sequence: 470, | ||
| }, | ||
| Destination: 'rJCxK2hX9tDMzbnn3cg1GU2g19Kfmhzxkp', | ||
| Fee: '0', | ||
| NetworkID: 21336, | ||
| Sequence: 0, | ||
| SigningPubKey: '', | ||
| TransactionType: 'Payment', | ||
| }, | ||
| }, | ||
| ], | ||
| TransactionType: 'Batch', | ||
| TxIDs: [ | ||
| 'ABE4871E9083DF66727045D49DEEDD3A6F166EB7F8D1E92FE868F02E76B2C5CA', | ||
| '795AAC88B59E95C3497609749127E69F12958BC016C600C770AEEB1474C840B4', | ||
| ], | ||
| } as any | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and test data configuration.
While the test data is comprehensive, consider these improvements:
- Replace the
as anytype assertion with a proper type definition for batch transactions - Consider extracting network-specific values (like NetworkID: 21336) into constants or configuration
Example implementation:
interface BatchTransaction {
Account: string;
BatchSigners: Array<{
BatchSigner: {
Account: string;
SigningPubKey: string;
TxnSignature: string;
};
}>;
// ... other fields
}
const NETWORK_ID = 21336;
beforeEach(function () {
tx = {
// ... existing initialization
NetworkID: NETWORK_ID,
} satisfies BatchTransaction;
})| /** | ||
| * Replaces Amount with DeliverMax if needed. | ||
| * | ||
| * @param tx - The transaction object. | ||
| * @throws ValidationError if Amount and DeliverMax are both provided but do not match. | ||
| */ | ||
| export function handleDeliverMax(tx: Payment): void { | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- ignore type-assertions on the DeliverMax property | ||
| // @ts-expect-error -- DeliverMax property exists only at the RPC level, not at the protocol level | ||
| if (tx.DeliverMax != null) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- needed here | ||
| if (tx.Amount == null) { | ||
| // If only DeliverMax is provided, use it to populate the Amount field | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- ignore type-assertions on the DeliverMax property | ||
| // @ts-expect-error -- DeliverMax property exists only at the RPC level, not at the protocol level | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, no-param-reassign -- known RPC-level property | ||
| tx.Amount = tx.DeliverMax | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- ignore type-assertions on the DeliverMax property | ||
| // @ts-expect-error -- DeliverMax property exists only at the RPC level, not at the protocol level | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- needed here | ||
| if (tx.Amount != null && tx.Amount !== tx.DeliverMax) { | ||
| throw new ValidationError( | ||
| 'PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided', | ||
| ) | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- ignore type-assertions on the DeliverMax property | ||
| // @ts-expect-error -- DeliverMax property exists only at the RPC level, not at the protocol level | ||
| // eslint-disable-next-line no-param-reassign -- needed here | ||
| delete tx.DeliverMax | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve Type Definitions for DeliverMax Handling
The handleDeliverMax function is manipulating the DeliverMax property, which leads to multiple type assertion comments like @ts-expect-error. This indicates that DeliverMax is not defined in the Payment type.
To enhance type safety and code readability, consider extending the Payment interface to include DeliverMax as an optional property. This approach eliminates the need for repeated type assertions and suppressing TypeScript errors.
Apply this diff to update the type definitions and clean up the code:
+import { Amount, Payment } from '../models/transactions'
+
+interface DeliverMaxPayment extends Payment {
+ DeliverMax?: Amount
+}
-export function handleDeliverMax(tx: Payment): void {
+export function handleDeliverMax(tx: DeliverMaxPayment): void {
if (tx.DeliverMax != null) {
if (tx.Amount == null) {
tx.Amount = tx.DeliverMax
}
if (tx.Amount != null && tx.Amount !== tx.DeliverMax) {
throw new ValidationError(
'PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided',
)
}
- delete tx.DeliverMax
+ tx.DeliverMax = undefined
}
}📝 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.
| /** | |
| * Replaces Amount with DeliverMax if needed. | |
| * | |
| * @param tx - The transaction object. | |
| * @throws ValidationError if Amount and DeliverMax are both provided but do not match. | |
| */ | |
| export function handleDeliverMax(tx: Payment): void { | |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- ignore type-assertions on the DeliverMax property | |
| // @ts-expect-error -- DeliverMax property exists only at the RPC level, not at the protocol level | |
| if (tx.DeliverMax != null) { | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- needed here | |
| if (tx.Amount == null) { | |
| // If only DeliverMax is provided, use it to populate the Amount field | |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- ignore type-assertions on the DeliverMax property | |
| // @ts-expect-error -- DeliverMax property exists only at the RPC level, not at the protocol level | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, no-param-reassign -- known RPC-level property | |
| tx.Amount = tx.DeliverMax | |
| } | |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- ignore type-assertions on the DeliverMax property | |
| // @ts-expect-error -- DeliverMax property exists only at the RPC level, not at the protocol level | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- needed here | |
| if (tx.Amount != null && tx.Amount !== tx.DeliverMax) { | |
| throw new ValidationError( | |
| 'PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided', | |
| ) | |
| } | |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- ignore type-assertions on the DeliverMax property | |
| // @ts-expect-error -- DeliverMax property exists only at the RPC level, not at the protocol level | |
| // eslint-disable-next-line no-param-reassign -- needed here | |
| delete tx.DeliverMax | |
| } | |
| } | |
| import { Amount, Payment } from '../models/transactions' | |
| interface DeliverMaxPayment extends Payment { | |
| DeliverMax?: Amount | |
| } | |
| /** | |
| * Replaces Amount with DeliverMax if needed. | |
| * | |
| * @param tx - The transaction object. | |
| * @throws ValidationError if Amount and DeliverMax are both provided but do not match. | |
| */ | |
| export function handleDeliverMax(tx: DeliverMaxPayment): void { | |
| if (tx.DeliverMax != null) { | |
| if (tx.Amount == null) { | |
| tx.Amount = tx.DeliverMax | |
| } | |
| if (tx.Amount != null && tx.Amount !== tx.DeliverMax) { | |
| throw new ValidationError( | |
| 'PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided', | |
| ) | |
| } | |
| tx.DeliverMax = undefined | |
| } | |
| } |
🧰 Tools
🪛 Biome
[error] 405-406: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| ) | ||
| }) | ||
|
|
||
| it('should autofill Batch transaction with single account', async function () { |
There was a problem hiding this comment.
Duplicate Test Case Names
The test case starting at line 493 has the same name as a previous test case, which can lead to confusion during testing. Since this test involves a Batch transaction with multiple accounts, consider renaming it to better reflect its purpose.
Apply this diff to rename the test case:
-it('should autofill Batch transaction with single account', async function () {
+it('should autofill Batch transaction with multiple accounts', async function () {📝 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.
| it('should autofill Batch transaction with single account', async function () { | |
| it('should autofill Batch transaction with multiple accounts', async function () { |
packages/xrpl/src/Wallet/index.ts
Outdated
| @@ -407,6 +407,10 @@ export class Wallet { | |||
| */ | |||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- validate does not accept Transaction type | |||
| validate(tx as unknown as Record<string, unknown>) | |||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- needed here for JS validation | |||
| if ((tx as unknown as Record<string, unknown>).BatchTxn != null) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify type assertions for better readability
The double type assertion (tx as unknown as Record<string, unknown>) can be simplified to improve code clarity and maintainability.
Consider updating the validate function to accept the Transaction type directly, eliminating the need for redundant type casting.
| * @param multisign - Specify true/false to use multisign or actual address (classic/x-address) to make multisign tx request. | ||
| * @throws ValidationError if the transaction is malformed. | ||
| */ | ||
| // eslint-disable-next-line max-lines-per-function -- TODO: refactor |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to comply with ESLint rules instead of disabling
The ESLint rule max-lines-per-function is disabled for the signMultiBatch function:
// eslint-disable-next-line max-lines-per-function -- TODO: refactorConsider refactoring the signMultiBatch function to reduce its length and complexity, adhering to the ESLint rule. This could involve extracting helper functions or simplifying logic, which enhances code readability and maintainability.
| const fieldsToSign = { | ||
| flags: transaction.Flags, | ||
| txIDs: transaction.TxIDs, | ||
| } |
There was a problem hiding this comment.
Correct field names in 'fieldsToSign' object
The field names in the fieldsToSign object should use PascalCase to match the expected transaction fields. Incorrect casing may lead to issues during the signing process.
Apply this diff to correct the field names:
const fieldsToSign = {
- flags: transaction.Flags,
- txIDs: transaction.TxIDs,
+ Flags: transaction.Flags,
+ TxIDs: transaction.TxIDs,
}📝 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.
| const fieldsToSign = { | |
| flags: transaction.Flags, | |
| txIDs: transaction.TxIDs, | |
| } | |
| const fieldsToSign = { | |
| Flags: transaction.Flags, | |
| TxIDs: transaction.TxIDs, | |
| } |
|
We should think about adding the ability to generate the index/hash for all objects especially NFTokenID. |
Agreed, but I think that can be a separate PR. |
|
Integration tests are expected to fail, since there is an integration test for |
High Level Overview of Change
This PR:
BatchtransactionsBatchtransactionsBatchtransactionsautofillfor filling in theBatchTxnandTxIDsfieldsContext of Change
XRPLF/rippled#5060
Type of Change
Did you update HISTORY.md?
Test Plan
Added tests for the new features.