feat: add support for Batch amendment #2801
Conversation
…o better-scripts
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/ripple-binary-codec/src/types/hash.ts(2 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)
✅ Files skipped from review due to trivial changes (1)
- packages/xrpl/HISTORY.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ripple-binary-codec/test/signing-data-encoding.test.ts
- packages/xrpl/package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/ripple-binary-codec/src/types/hash.ts (1)
packages/isomorphic/src/utils/shared.ts (1)
HEX_REGEX(3-3)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (18.x)
🔇 Additional comments (1)
packages/ripple-binary-codec/src/types/hash.ts (1)
3-3: LGTM! Good addition of defensive validation.The import of
HEX_REGEXis appropriate for validating hexadecimal string inputs, which strengthens the robustness of hash processing in batch transactions.
| if (!HEX_REGEX.test(value)) { | ||
| throw new Error(`Invalid hash string ${value}`) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Excellent defensive programming practice.
The hexadecimal validation prevents invalid strings from being processed, which is crucial for maintaining data integrity in hash operations. The error message is descriptive and will help with debugging.
Note that the HEX_REGEX pattern allows empty strings due to the * quantifier. Verify this is the intended behavior for your use case.
🏁 Script executed:
#!/bin/bash
# Verify if empty strings should be allowed as valid hash inputs
echo "Checking HEX_REGEX pattern and testing empty string behavior..."
# Check the regex pattern definition
rg -A 2 -B 2 "HEX_REGEX.*=" packages/isomorphic/src/utils/shared.ts
# Search for any existing hash creation with empty strings to understand expected behavior
rg -A 3 -B 3 "Hash\.from\(\s*['\"][\s]*['\"]"
rg -A 3 -B 3 "new.*Hash.*['\"][\s]*['\"]"Length of output: 48187
Ensure HEX_REGEX doesn’t inadvertently allow empty strings
Great defensive programming here—the check and error are clear and helpful. However, the current pattern in packages/isomorphic/src/utils/shared.ts is:
export const HEX_REGEX = /^[A-F0-9]*$/iuThe * quantifier lets an empty string pass. If you never intend to accept zero‐length hashes, update it to require at least one hex digit:
-export const HEX_REGEX = /^[A-F0-9]*$/iu
+export const HEX_REGEX = /^[A-F0-9]+$/iu• File to update:
- packages/isomorphic/src/utils/shared.ts
• Change * → + on the quantifier so empty inputs are rejected.
If you do intend to allow empty hashes, please add a comment to clarify this special case.
🤖 Prompt for AI Agents
In packages/isomorphic/src/utils/shared.ts around the definition of HEX_REGEX,
change the quantifier from '*' to '+' so the regex requires at least one
hexadecimal digit and does not match empty strings. This prevents empty strings
from being considered valid hex hashes, aligning with the validation logic in
packages/ripple-binary-codec/src/types/hash.ts. If empty hashes are intended to
be valid, instead add a comment explaining this special case.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/src/Wallet/batchSigner.ts(1 hunks)packages/xrpl/src/models/transactions/batch.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/src/Wallet/batchSigner.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/xrpl/src/models/transactions/batch.ts (4)
packages/xrpl/src/models/transactions/common.ts (8)
GlobalFlagsInterface(311-313)BaseTransaction(318-393)validateBaseTransaction(404-465)validateRequiredField(243-269)isArray(225-227)isRecord(78-80)validateOptionalField(283-303)isString(88-90)packages/xrpl/src/models/common/index.ts (1)
Signer(40-46)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)packages/xrpl/src/models/utils/flags.ts (1)
hasFlag(171-180)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: unit (22.x)
- GitHub Check: integration (18.x)
- GitHub Check: integration (22.x)
- GitHub Check: build-and-lint (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: unit (20.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
packages/xrpl/src/models/transactions/batch.ts (8)
1-16: Import structure looks good.The imports are well-organized and include all necessary validation utilities and types for implementing Batch transaction support.
23-28: Batch flags are correctly defined.The flag values use proper hexadecimal constants that align with the Batch amendment specification.
36-41: Interface properly extends global flags.The
BatchFlagsInterfacecorrectly extendsGlobalFlagsInterfaceand includes all batch-specific flags as optional boolean properties.
43-53: Inner transaction constraints are well-designed.The
BatchInnerTransactiontype appropriately constrains inner transactions by:
- Setting
Feeto '0' (batch handles fees)- Setting
SigningPubKeyto empty string- Using
nevertypes to prevent signing fields that should not be present- Excluding
LastLedgerSequencewhich is handled at the batch levelThese constraints align with the Batch amendment requirements.
55-65: BatchSigner interface structure is correct.The interface properly models the batch signer structure with required
Accountfield and optional signing-related fields, supporting both single-sign and multi-sign scenarios.
70-78: Batch transaction interface is well-structured.The interface extends
BaseTransactionappropriately and defines the requiredRawTransactionsarray and optionalBatchSignersarray with correct nested structures.
87-120: Validation logic for RawTransactions is comprehensive.The validation properly:
- Validates the array structure and individual transaction objects
- Prevents nesting of Batch transactions
- Enforces the required
tfInnerBatchTxnflag on inner transactions- Uses descriptive error messages with proper array indexing
The approach of throwing an error when the flag is missing (rather than auto-adding it) is the correct design choice for a validation function.
122-152: BatchSigners validation is thorough and well-implemented.The validation logic properly:
- Validates the optional array structure
- Checks each signer object and its required/optional fields
- Uses consistent error message formatting with proper indexing
- Validates nested Signers array when present
All field validations use appropriate type guards and provide clear, actionable error messages.
|
@khancode I had to make some changes to resolve a dependency cycle, you might want to do a quick re-review |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/xrpl/test/wallet/batchSigner.test.ts (1)
6-7: Consider enum naming clarity.The
ECDSAenum is used for bothsecp256k1anded25519algorithms. Sinceed25519is not technically an ECDSA algorithm, this could be confusing. Consider renaming toAlgorithmor similar for clarity in future updates.
🧹 Nitpick comments (2)
packages/xrpl/test/models/batch.test.ts (1)
115-115: Consider using undefined assignment instead of delete.For better performance, consider using undefined assignment instead of the delete operator.
- delete tx.RawTransactions + tx.RawTransactions = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 115-115: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/wallet/batchSigner.test.ts (1)
338-338: Consider using undefined assignment instead of delete.For better performance, consider using undefined assignment instead of the delete operator.
- delete badTx1.BatchSigners + badTx1.BatchSigners = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 338-338: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.vscode/settings.json(2 hunks)packages/xrpl/src/Wallet/index.ts(5 hunks)packages/xrpl/src/models/transactions/batch.ts(1 hunks)packages/xrpl/src/models/utils/flags.ts(3 hunks)packages/xrpl/src/models/utils/index.ts(2 hunks)packages/xrpl/src/utils/hashes/hashLedger.ts(3 hunks)packages/xrpl/test/integration/transactions/batch.test.ts(1 hunks)packages/xrpl/test/models/batch.test.ts(1 hunks)packages/xrpl/test/wallet/batchSigner.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/xrpl/src/utils/hashes/hashLedger.ts
- packages/xrpl/src/Wallet/index.ts
- packages/xrpl/test/integration/transactions/batch.test.ts
- .vscode/settings.json
- packages/xrpl/src/models/transactions/batch.ts
🧰 Additional context used
🧠 Learnings (1)
packages/xrpl/test/wallet/batchSigner.test.ts (1)
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
🧬 Code Graph Analysis (2)
packages/xrpl/src/models/utils/flags.ts (1)
packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
packages/xrpl/src/models/utils/index.ts (1)
packages/xrpl/src/models/transactions/transaction.ts (1)
Transaction(197-197)
🪛 Biome (1.9.4)
packages/xrpl/test/models/batch.test.ts
[error] 115-115: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/wallet/batchSigner.test.ts
[error] 338-338: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: unit (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: integration (20.x)
- GitHub Check: build-and-lint (22.x)
🔇 Additional comments (15)
packages/xrpl/src/models/utils/index.ts (2)
1-1: LGTM: Clean import addition for Transaction type.The import is necessary for the new
hasFlagfunction's type parameter.
32-52: Excellent utility function implementation.The
hasFlagfunction is well-designed with:
- Proper null safety checks
- Support for both numeric and object-style flags
- Clear parameter typing and documentation
- Appropriate delegation to existing
isFlagEnabledutilityThis will be valuable for the new Batch transaction flag handling.
packages/xrpl/test/models/batch.test.ts (4)
1-11: LGTM: Well-organized test imports and structure.The test suite is properly set up with all necessary imports and clear documentation.
62-65: Comprehensive validation testing.Good approach testing both the specific
validateBatchfunction and the genericvalidatefunction to ensure consistency.
67-103: LGTM: Single-account Batch test covers important use case.This test properly validates that Batch transactions work correctly when all inner transactions come from the same account (no BatchSigners needed).
105-175: Excellent negative test coverage.The test suite thoroughly covers all the important error conditions:
- Invalid BatchSigners types
- Missing/invalid RawTransactions
- Nested Batch transactions (correctly disallowed)
- Missing tfInnerBatchTxn flag
This ensures robust validation of the Batch transaction structure.
🧰 Tools
🪛 Biome (1.9.4)
[error] 115-115: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/utils/flags.ts (3)
11-12: LGTM: Clean imports for Batch and global flag support.The new imports properly support the Batch transaction type and global flags functionality.
56-56: LGTM: Consistent Batch transaction integration.The Batch entry in the
txToFlagmapping follows the established pattern and enables proper flag handling for Batch transactions.
103-133: Excellent refactor with improved validation.The updated
convertTxFlagsToNumberfunction provides significant improvements:
- Stricter validation: Now throws
ValidationErrorfor unknown flags instead of silently returning 0- Global flags support: Properly handles global flags like
tfInnerBatchTxnfor all transaction types- Better error messages: Descriptive error messages help with debugging
- Robust logic: Validates against both transaction-specific and global flags appropriately
This change makes the flag handling much more reliable and supports the new Batch transaction requirements.
packages/xrpl/test/wallet/batchSigner.test.ts (6)
1-33: Comprehensive test setup with proper wallet generation.The test setup covers all necessary scenarios with different cryptographic algorithms and wallet types. The hardcoded seeds are appropriate for consistent test behavior as previously confirmed.
83-121: Excellent coverage of cryptographic algorithm support.The tests properly validate signing with both secp256k1 and ed25519 algorithms, ensuring the batch signing functionality works correctly across different key types.
145-203: Comprehensive multisign testing.The tests thoroughly cover multisign scenarios including:
- Basic multisign functionality
- Multisign with regular key specification
- Proper signature structure validation
This ensures robust support for complex signing scenarios in batch transactions.
270-283: LGTM: Thorough combineBatchSigners testing.The tests properly validate that the function works with both transaction objects and serialized strings, and correctly sorts signers. This ensures consistent behavior regardless of input format.
285-317: Excellent edge case testing for submitter signature removal.The test correctly validates that signatures from the Batch transaction submitter are properly excluded from the final BatchSigners, which is important for transaction validation.
319-371: Comprehensive error handling validation.The error tests thoroughly cover all failure scenarios:
- Empty transaction arrays
- Non-Batch transactions
- Missing BatchSigners
- Signed inner transactions
- Mismatched transaction content
This ensures robust validation and helpful error messages for incorrect usage.
🧰 Tools
🪛 Biome (1.9.4)
[error] 338-338: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick commen F438 ts (1)
packages/xrpl/test/wallet/batchSigner.test.ts (1)
345-345: Replace delete operator with undefined assignment for better performance.The
deleteoperator can impact performance by changing the object's hidden class. Setting the property toundefinedachieves the same test goal more efficiently.- delete badTx1.BatchSigners + badTx1.BatchSigners = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 345-345: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/test/wallet/batchSigner.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/xrpl/test/wallet/batchSigner.test.ts (1)
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
🪛 Biome (1.9.4)
packages/xrpl/test/wallet/batchSigner.test.ts
[error] 345-345: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit (22.x)
- GitHub Check: browser (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (20.x)
- GitHub Check: Analyze (javascript)
- GitHub Check: integration (20.x)
🔇 Additional comments (4)
packages/xrpl/test/wallet/batchSigner.test.ts (4)
1-48: Well-structured test setup with comprehensive wallet coverage.The test setup appropriately covers different cryptographic algorithms and includes helpful address comments for clarity. The hardcoded seeds are appropriate for test consistency as confirmed in previous reviews.
54-228: Excellent test coverage for signMultiBatch functionality.The test suite comprehensively covers different signing scenarios including various cryptographic algorithms, multisign capabilities, and proper error handling. The test structure is well-organized with clear setup and precise assertions.
230-344: Comprehensive test coverage for combineBatchSigners with excellent edge case handling.The test suite thoroughly validates signature combination logic including sorting, serialized inputs, and submitter signature removal. The test structure and assertions are well-designed.
352-378: Thorough error case validation with appropriate error messages.The error test cases effectively validate input validation and provide clear, descriptive error messages. This ensures robust error handling for edge cases in production usage.
| }) | ||
|
|
||
| it('verifies valid Batch', function () { | ||
| assert.doesNotThrow(() => validateBatch(tx)) |
There was a problem hiding this comment.
Yes, I understand that validateBatch also checks the BatchSigners field.
However, validate(tx) invokes the respective validateBatch(tx) here. So, I don't see the point of explicitly invoking the validateBatch method in the unit tests.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/xrpl/src/sugar/autofill.ts (1)
422-423: Delete operator usage is moved code.Based on past review discussion, this code was moved rather than newly written, so performance optimizations would be out of scope for this PR.
🧰 Tools
🪛 Biome (1.9.4)
[error] 422-423: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🧹 Nitpick comments (1)
packages/xrpl/src/sugar/autofill.ts (1)
267-320: Review fee calculation refactoring for Batch transactions.The refactoring looks good overall, but there are a few considerations:
- The Batch fee calculation (lines 294-302) uses
reducewith async operations, which could be inefficient for large batches- The doubled base fee for Batch transactions (line 302) appears intentional but should be documented
Consider adding a comment explaining the doubled base fee for Batch transactions:
+ // Batch transactions have doubled base fee plus sum of inner transaction fees baseFee = BigNumber.sum(baseFee.times(2), rawTxFees)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/sugar/autofill.ts(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/xrpl/HISTORY.md
🧰 Additional context used
🧠 Learnings (1)
packages/xrpl/src/sugar/autofill.ts (1)
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/src/sugar/autofill.ts:374-408
Timestamp: 2025-04-16T15:55:59.970Z
Learning: Code that is moved without substantial changes in a PR should not be the target of refactoring suggestions as it would be out of scope for the PR's primary purpose.
🪛 Biome (1.9.4)
packages/xrpl/src/sugar/autofill.ts
[error] 422-423: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: integration (22.x)
- GitHub Check: browser (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: Analyze (javascript)
- GitHub Check: unit (22.x)
🔇 Additional comments (2)
packages/xrpl/src/sugar/autofill.ts (2)
213-225: LGTM: Clean extraction of sequence number logic.The new helper function properly encapsulates the sequence number retrieval logic and improves code reusability.
434-492: Validate Batch transaction autofill logic.The
autofillBatchTxnfunction implements the core logic for handling Batch transactions with proper validation. A few observations:
- The sequence number management for multiple accounts is well-handled
- The validation rules for inner transactions are correctly enforced
- The special case for the Batch account's sequence (line 454) adds 1 to account for the outer transaction
Consider adding error context to the validation failures for better debugging:
} else if (txn.Fee !== '0') { - throw new XrplError('Must have `Fee of "0" in inner Batch transaction.') + throw new XrplError(`Must have Fee of "0" in inner Batch transaction. Found: ${txn.Fee}`) }Similarly for other validation errors to help identify which inner transaction failed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/test/wallet/batchSigner.test.ts (1)
345-345: Replace delete operator with undefined assignmentThe
deleteoperator can impact performance. Use undefined assignment instead.- delete badTx1.BatchSigners + badTx1.BatchSigners = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 345-345: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/test/client/autofill.test.ts(3 hunks)packages/xrpl/test/wallet/batchSigner.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/client/autofill.test.ts
🧰 Additional context used
🧠 Learnings (1)
packages/xrpl/test/wallet/batchSigner.test.ts (1)
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/wallet/batchSigner.test.ts:0-0
Timestamp: 2025-04-16T15:28:21.204Z
Learning: In the XRPL.js library, hardcoded seeds in test files are acceptable as they don't represent protected data or real funds - they're only used for consistent test behavior.
🪛 Biome (1.9.4)
packages/xrpl/test/wallet/batchSigner.test.ts
[error] 345-345: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
packages/xrpl/test/wallet/batchSigner.test.ts (4)
1-52: LGTM: Test setup is well-structuredThe imports, wallet creation, and test data setup are properly configured. The hardcoded seeds and addresses are appropriate for test consistency as confirmed in previous discussions.
53-228: LGTM: Comprehensive coverage of signMultiBatch functionalityThe test cases thoroughly cover:
- Different cryptographic algorithms (secp256k1, ed25519)
- Regular key and multisign scenarios
- Proper error handling for invalid inputs
The expected results with hardcoded addresses ensure signing accuracy.
368-377: LGTM: Test correctly validates flag differencesThis test appropriately checks that
combineBatchSignersfails when transactions have different flags. The signing oftx2instead ofbadTx2is intentional - the function should fail due to flag mismatch before reaching signature validation, as explained in previous discussions.
230-378: LGTM: Thorough coverage of combineBatchSigners functionalityThe test suite comprehensively covers:
- Combining valid transactions and serialized blobs
- Proper signer sorting behavior
- Exclusion of batch submitter's signer
- Comprehensive error handling for various invalid inputs
The test logic correctly validates the complex signature combination requirements for batch transactions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 345-345: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
High Level Overview of Change
This PR:
BatchtransactionsBatchtransactionsBatchtransactionsautofillto ensure all autofill-able fields are handledContext of Change
XRPLF/rippled#5060
Type of Change
Did you update HISTORY.md?
Test Plan
Added tests for the new features.