feat(xrpl): custom definitions support #2683
Conversation
|
Hey there, did you also check #2214 |
Co-authored-by: Mayukha Vadari <mvadari@gmail.com>
Co-authored-by: tequ <git@tequ.dev>
…n_support Support custom definitions for `client.submit()`, `client.submitAndWait()`
|
@elmurci looks like something broke TS |
Co-authored-by: tequ <git@tequ.dev>
|
@elmurci |
|
@coderabbitai full review |
Actions performedFull review triggered. |
WalkthroughThis pull request introduces optional parameters to several methods in the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint (1.23.1)
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (16)
packages/ripple-binary-codec/README.md (2)
Line range hint
14-24: Add examples demonstrating custom definitions usageThe current examples only show basic usage without custom definitions. To help users understand the new functionality, consider adding examples that demonstrate:
- Encoding a transaction with custom definitions
- Decoding a transaction that uses custom definitions
- Common use cases or scenarios where custom definitions are beneficial
Also applies to: 30-43
Line range hint
1-12: Add architectural overview for custom definitions supportGiven that this change is part of a broader effort to support custom definitions across multiple packages (as mentioned in the PR objectives), consider adding:
- A new section explaining how this package's custom definitions support relates to other XRPL packages
- Integration examples showing how to use custom definitions with both
ripple-binary-codecandxrplpackages- Best practices for working with custom definitions across the ecosystem
packages/xrpl/src/utils/index.ts (1)
91-92: Consider enhancing the documentationWhile the documentation is clear, it could be more helpful by:
- Including an example of custom definitions usage
- Adding a link to more detailed documentation about sidechains and amendments
- Providing guidance on when to use custom definitions
Example enhancement:
* @param definitions Custom rippled types to use instead of the default. Used for sidechains and amendments. +* @example +* ```typescript +* const customDefinitions = { +* // Example custom transaction type for a sidechain +* TRANSACTION_TYPES: { +* SidechainTransaction: 20000 +* } +* }; +* const encoded = encode(transaction, customDefinitions); +* ``` +* @see https://xrpl.org/transaction-types.htmlAlso applies to: 105-106, 130-131, 145-146
packages/xrpl/src/utils/hashes/hashLedger.ts (1)
68-68: LGTM with a documentation enhancement suggestion.The implementation correctly adds support for custom definitions. Consider enhancing the JSDoc for the
definitionsparameter to better explain its purpose and expected format.- * @param definitions - Definitions to use for encoding and decoding. + * @param definitions - Optional custom definitions that override default XRP Ledger type definitions + * for encoding and decoding transactions. Use this when working with non-standard + * transaction types or fields.Also applies to: 73-76
packages/xrpl/src/models/transactions/common.ts (2)
368-370: Improve error message clarityThe error message could be more specific about the validation failure.
- 'TransactionType field is not specified or not a string', + 'Transaction validation failed: TransactionType field must be a string',
365-365: Consider tracking future validation requirementsThe comment suggests potential future validations. Consider adding a TODO comment to track this enhancement.
- // Validate just transaction type for now, leaving it open for further validations against the custom definition spec. + // TODO: Implement additional validations against the custom definition spec (e.g., field types, required fields) + // For now, we only validate the transaction typepackages/xrpl/src/models/transactions/transaction.ts (1)
179-179: Minor wording correction in JSDoc commentIn the JSDoc comment, the description of the
customDefinitionsparameter should reflect its plural nature.Consider updating the description to:
/** * @param transaction - A Transaction. - * @param customDefinitions - Optional parameter to validate against a custom definition. + * @param customDefinitions - Optional parameter to validate against custom definitions. * @throws ValidationError When the Transaction is malformed. * @category Utilities */packages/xrpl/src/sugar/submit.ts (4)
173-176: Standardize parameter structure in 'isSigned' functionThe
isSignedfunction now acceptstransactionand an optionaldefinitionsparameter. For consistency and scalability, consider refactoringisSignedto accept an options object for optional parameters, similar to other functions in the codebase.Apply this diff to refactor
isSigned:-function isSigned( - transaction: SubmittableTransaction | string, - definitions?: XrplDefinitionsBase, -): boolean { +function isSigned( + transaction: SubmittableTransaction | string, + options: { definitions?: XrplDefinitionsBase } = {}, +): boolean { + const { definitions } = optionsAnd update all calls to
isSignedaccordingly.
266-266: Simplify type assertions in decoding transactionsThe type assertion
(decode(transaction, definitions) as unknown as SubmittableTransaction)involves casting tounknown, which may be unnecessary and reduces code readability. Simplify the type assertions by refining the types or using type guards.Consider this modification:
- (decode(transaction, definitions) as unknown as SubmittableTransaction) + decode(transaction, definitions) as SubmittableTransaction
214-215: Improve JSDoc formatting for 'definitions' parameterIn the JSDoc comment for the
definitionsparameter in thegetSignedTxfunction, the description is split across lines without proper indentation. Correct the indentation to maintain consistency and readability in documentation.Apply this diff to fix the indentation:
/** * @param [options.autofill=true] - Optional. Determines whether the transaction should be autofilled (true) * or not (false). Default is true. * @param [options.wallet] - Optional. A wallet to sign the transaction. It must be provided when submitting * an unsigned transaction. Default is undefined. - * @param [options.definitions] - Optional. Custom rippled types to use instead of the default. - * Used for sidechains and amendments. Default is undefined. + * @param [options.definitions] - Optional. Custom rippled types to use instead of the default. + * Used for sidechains and amendments. Default is undefined. * @returns A promise that resolves with the signed transaction. */
244-250: Clarify the options interface in 'getSignedTx' functionIn the
getSignedTxfunction, theoptionsparameter includesautofill,wallet, anddefinitions. For better type clarity and code maintainability, consider defining a named TypeScript interface for these options.For example:
interface GetSignedTxOptions { autofill?: boolean wallet?: Wallet definitions?: XrplDefinitionsBase }And update the function signature:
-export async function getSignedTx( +export async function getSignedTx( client: Client, transaction: SubmittableTransaction | string, - { - autofill = true, - wallet, - definitions, - }: { - autofill?: boolean - wallet?: Wallet - definitions?: XrplDefinitionsBase - } = {}, + options: GetSignedTxOptions = {}, ): Promise<SubmittableTransaction | string> { + const { autofill = true, wallet, definitions } = optionspackages/xrpl/src/Wallet/index.ts (3)
371-371: Fix JSDoc formatting for the 'definitions' parameterThe JSDoc comment for the
definitionsparameter in thesignmethod is missing a hyphen and isn't aligned properly. Consistent formatting enhances readability and maintainability.Apply this diff to correct the JSDoc formatting:
/** * Signs a transaction offline. * * @example * ... * @param multisign - Specify true/false to use multisign or actual address (classic/x-address) to make multisign tx request. - * @param definitions Custom rippled types to use instead of the default. Used for sidechains and amendments. + * @param definitions - Custom rippled types to use instead of the default. Used for sidechains and amendments. * @returns A signed transaction. * @throws ValidationError if the transaction is already signed or does not encode/decode to same result. * @throws XrplError if the issued currency being signed is XRP ignoring case. */
376-376: Consider refactoring to reduce function complexityThe
signmethod now disables ESLint rules formax-lines-per-functionandmax-paramsdue to increased complexity. Refactoring the method could improve readability and maintainability. Consider using an options object for optional parameters or splitting the method into smaller helper functions.
476-476: Fix JSDoc formatting for the 'definitions' parameter incomputeSignatureThe JSDoc comment for the
definitionsparameter in thecomputeSignaturefunction is missing a hyphen. Aligning it properly with the other parameters enhances clarity.Apply this diff to correct the JSDoc formatting:
/** * Signs a transaction with the proper signing encoding. * * @param tx - A transaction to sign. * @param privateKey - A key to sign the transaction with. * @param signAs - Multisign only. An account address to include in the Signer field. - * @param definitions Custom rippled types to use instead of the default. Used for sidechains and amendments. + * @param definitions - Custom rippled types to use instead of the default. Used for sidechains and amendments. * @returns A signed transaction in the proper format. */packages/xrpl/src/client/index.ts (1)
553-556: Optimize response cloning for definitionsUsing
JSON.parse(JSON.stringify(response.result))to clone the response is inefficient and can impact performance. Consider alternative methods:
- Use object spread or
Object.assignif a shallow copy suffices.- Utilize a dedicated deep cloning library like
lodash.clonedeepfor better performance and reliability.- Use
response.resultdirectly if it's safe and there are no mutations applied to it later.packages/xrpl/test/wallet/index.test.ts (1)
1191-1220: Define a specific transaction type instead of usingany.Using
anyand disabling ESLint with// eslint-disable-next-linereduces type safety and may hide potential errors. Consider defining a specific TypeScript interface for the'TokenSwapPropose'transaction type to enhance type safety and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
packages/ripple-binary-codec/README.md(5 hunks)packages/xrpl/HISTORY.md(2 hunks)packages/xrpl/src/Wallet/index.ts(5 hunks)packages/xrpl/src/client/index.ts(5 hunks)packages/xrpl/src/models/transactions/common.ts(2 hunks)packages/xrpl/src/models/transactions/transaction.ts(4 hunks)packages/xrpl/src/sugar/submit.ts(7 hunks)packages/xrpl/src/utils/hashes/hashLedger.ts(2 hunks)packages/xrpl/src/utils/index.ts(3 hunks)packages/xrpl/test/fixtures/requests/index.ts(2 hunks)packages/xrpl/test/fixtures/requests/signAsCustomDefinition.json(1 hunks)packages/xrpl/test/fixtures/responses/index.ts(2 hunks)packages/xrpl/test/fixtures/responses/signAsCustomDefinition.json(1 hunks)packages/xrpl/test/fixtures/responses/signCustomDefinition.json(1 hunks)packages/xrpl/test/fixtures/rippled/index.ts(3 hunks)packages/xrpl/test/wallet/index.test.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/xrpl/test/fixtures/requests/signAsCustomDefinition.json
- packages/xrpl/test/fixtures/responses/signAsCustomDefinition.json
- packages/xrpl/test/fixtures/responses/signCustomDefinition.json
🧰 Additional context used
🪛 Markdownlint (0.35.0)
packages/xrpl/HISTORY.md
42-42: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (20)
packages/xrpl/test/fixtures/requests/index.ts (1)
7-7: LGTM! Changes align with PR objectives.
The addition of signAsCustomDefinition fixture properly supports the new custom definitions feature being added to the XRPL methods.
Let's verify the test fixture file exists and contains valid test data:
Also applies to: 17-17
✅ Verification successful
Let me verify the test data structure further since the initial check shows the JSON file exists but doesn't match the expected structure with tx and expected fields.
Test fixture structure is valid but follows a different pattern
The signAsCustomDefinition.json fixture follows the same structure as other sign-related fixtures in the codebase, containing direct transaction fields rather than the initially expected tx/expected pattern. The file contains all necessary fields for a custom TokenSwapPropose transaction including Account, TransactionType, Amount, and other required properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the signAsCustomDefinition.json fixture exists and contains valid JSON
# Expected: File exists and contains valid JSON structure for custom definition testing
# Check if file exists and is valid JSON
if [ -f "packages/xrpl/test/fixtures/requests/signAsCustomDefinition.json" ]; then
jq '.' packages/xrpl/test/fixtures/requests/signAsCustomDefinition.json
else
echo "Error: signAsCustomDefinition.json not found"
exit 1
fi
# Check if it contains expected custom definition structure
jq 'select(.tx != null and .expected != null)' packages/xrpl/test/fixtures/requests/signAsCustomDefinition.json
Length of output: 727
Script:
#!/bin/bash
# Let's check the structure of other sign-related test fixtures for comparison
fd "sign.*\.json" packages/xrpl/test/fixtures/requests/ --exec jq -c 'keys'
Length of output: 577
packages/xrpl/test/fixtures/responses/index.ts (2)
11-12: LGTM! Import statements follow existing conventions.
The new imports for custom definition test fixtures are properly organized alongside other signing-related fixtures and follow consistent naming patterns.
31-32: LGTM! Sign object properties are well-structured.
The new properties for custom definition support are logically grouped with other signing-related properties and maintain consistent naming with their imports.
packages/xrpl/test/fixtures/rippled/index.ts (2)
8-8: LGTM! Changes follow existing patterns.
The additions maintain consistency with the existing code structure and organization.
Also applies to: 98-100, 126-126
8-8: Verify the structure of customDefinition.json
Let's ensure the custom definition file exists and follows the expected structure for XRPL definitions.
✅ Verification successful
Since the script output shows that the file exists and contains the expected structure with "TYPES", "FIELDS", and "TRANSACTION_TYPES" properties, I can generate the final response.
✓ customDefinition.json verified with valid XRPL definition structure
The custom definition file exists and follows the expected structure containing all required properties: TYPES, FIELDS, and TRANSACTION_TYPES.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of customDefinition.json
# Check if the file exists and examine its content
if [ -f "packages/xrpl/test/fixtures/rippled/customDefinition.json" ]; then
echo "File exists. Checking content structure..."
cat packages/xrpl/test/fixtures/rippled/customDefinition.json | jq '
# Verify expected structure for XRPL definitions
if has("TYPES") and has("FIELDS") and has("TRANSACTION_TYPES") then
"Valid definition structure"
else
"Invalid structure - missing required properties"
end
'
else
echo "customDefinition.json not found"
fi
Length of output: 532
packages/xrpl/src/utils/index.ts (2)
23-23: LGTM: Import of XrplDefinitionsBase type
The addition of the XrplDefinitionsBase type import is appropriate and aligns with the PR's objective to support custom definitions.
94-98: LGTM: Consistent implementation of custom definitions support
The function signature updates and implementations are well-structured:
- Optional
definitionsparameter maintains backward compatibility - Consistent pattern across all encoding/decoding functions
- Proper forwarding to ripple-binary-codec implementations
Let's verify the consistency of these changes across the codebase:
Also applies to: 108-112, 133-138, 148-152
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the consistency of the changes:
LGTM: Verified consistent implementation of custom definitions parameter
The changes are correctly implemented across all encoding/decoding functions:
-
All core encoding functions in
packages/xrpl/src/utils/index.tsconsistently support the optionaldefinitionsparameter:encode(object, definitions?)encodeForSigning(object, definitions?)encodeForMultiSigning(object, signer, definitions?)decode(hex, definitions?)
-
The parameter is properly forwarded to the underlying ripple-binary-codec implementations
-
All usages across the codebase handle both with and without the optional parameter
-
No breaking changes introduced as the parameter remains optional
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all relevant encoding/decoding functions support custom definitions
# Check for any other encoding/decoding functions that might need the definitions parameter
rg -l "rbcEncode|rbcDecode|encodeForSigning|encodeForMultisigning" --type ts
# Check for any direct usage of these functions to ensure they're updated
rg -A 2 "encode\(|encodeForSigning\(|encodeForMultiSigning\(|decode\(" --type ts
Length of output: 38817
packages/xrpl/src/utils/hashes/hashLedger.ts (1)
8-8: LGTM: Import statement updated correctly.
The addition of XrplDefinitionsBase to the imports is consistent with the PR's objective to support custom definitions.
packages/xrpl/src/models/transactions/common.ts (1)
2-2: LGTM: Import addition is consistent with feature objectives
The addition of XrplDefinitionsBase import aligns with the PR's goal of supporting custom definitions.
packages/xrpl/HISTORY.md (1)
42-43: LGTM! The changelog entry is well-formatted and clear.
The entry properly documents the addition of custom definitions support for various utility functions, following the established changelog format.
🧰 Tools
🪛 Markdownlint (0.35.0)
42-42: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
packages/xrpl/src/sugar/submit.ts (1)
57-57: Verify handling of undefined 'definitions' in 'isSigned'
When calling isSigned(signedTransaction, definitions), ensure that the function correctly handles cases where definitions is undefined, defaulting to standard definitions as necessary. This is crucial for maintaining backward compatibility and preventing potential errors during transaction submission.
packages/xrpl/src/Wallet/index.ts (1)
Line range hint 371-443: Ensure unit tests cover usage of custom definitions
With the addition of the optional definitions parameter in the Wallet.sign method, please ensure that unit tests comprehensively cover scenarios where custom definitions are utilized. This includes both single-signature and multisignature transactions to verify that the new functionality behaves as expected.
packages/xrpl/src/client/index.ts (2)
5-5: Import statement verified
The import of XrplDefinitionsBase from 'ripple-binary-codec' is correct and ensures the necessary definitions are available.
222-227: Property 'definitions' added appropriately
The addition of the definitions property to the Client class enhances flexibility for handling custom rippled types, especially for sidechains and amendments.
packages/xrpl/test/wallet/index.test.ts (6)
2-2: Approved: Properly importing XrplDefinitions and decode.
The added import statement is necessary for the new test cases involving custom definitions and the use of decode for transaction handling.
14-14: Approved: Importing rippled fixtures for custom definitions.
Importing rippled from '../fixtures/rippled' is appropriate and enables access to custom definition fixtures required for the tests.
1222-1249: Define a specific transaction type instead of using any.
As in the previous test, avoid using any and disabling ESLint. Defining a TypeScript interface for the 'SomeUnknown' transaction type will enhance type safety and help catch errors at compile time.
1251-1265: Define specific types for transactions instead of using any.
Using any in the multisign test reduces type safety. Define appropriate TypeScript types for the custom transaction used in multisigning to improve code quality and safety.
1230-1231: 🛠️ Refactor suggestion
Use named constants for flag values to improve readability.
Repeated use of magic numbers like Flags: 2147483648 can reduce code clarity. Utilize named constants to make the code self-explanatory and maintainable.
1258-1259: 🛠️ Refactor suggestion
Use named constants for flag values to improve readability.
Again, consider replacing magic numbers like Flags: 2147483648 with named constants to enhance readability and maintainability.
| @@ -10,7 +10,7 @@ Functions to encode/decode to/from the ripple [binary serialization format](http | |||
| ``` | |||
|
|
|||
|
|
|||
| ### decode(binary: string): object | |||
| ### decode(binary: string, definitions?: XrplDefinitionsBase): object | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Documentation needed for the new definitions parameter
The new definitions parameter has been added to multiple methods, but there's no explanation of:
- What constitutes valid definitions
- How to structure the
XrplDefinitionsBaseobject - Example usage with custom definitions
Consider adding a new section that explains:
- The purpose and structure of custom definitions
- Example of a custom definition object
- Complete example showing how to use custom definitions with these methods
Also applies to: 29-29, 57-57, 65-65
| export function hashSignedTx( | ||
| tx: Transaction | string, | ||
| definitions?: XrplDefinitionsBase, | ||
| ): string { | ||
| let txBlob: string | ||
| let txObject: Transaction | ||
| if (typeof tx === 'string') { | ||
| txBlob = tx | ||
| /* eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Required until updated in binary codec. */ | ||
| txObject = decode(tx) as unknown as Transaction | ||
| txObject = decode(tx, definitions) as unknown as Transaction | ||
| } else { | ||
| txBlob = encode(tx) | ||
| txBlob = encode(tx, definitions) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the test coverage analysis, I can now provide a final response:
Test coverage for custom definitions is missing
The existing test suite in packages/xrpl/test/utils/hashes.test.ts has good coverage for hashSignedTx functionality including:
- Hashing signed transactions
- Hashing transaction blobs
- Error cases for unsigned transactions
- Pseudo-transactions
However, there are no test cases that explicitly verify the new custom definitions parameter. You should add test cases to cover:
- Hashing transactions with custom definitions
- Hashing transaction blobs with custom definitions
- Edge cases where custom definitions affect the transaction decoding
🔗 Analysis chain
Verify test coverage for custom definitions.
The changes look good, but we should ensure proper test coverage exists for the custom definitions functionality, especially edge cases.
Let me analyze the test coverage for hashSignedTx with custom definitions by checking the test file.
Let me check for test coverage specifically for the hashSignedTx function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of hashSignedTx with custom definitions
# Look for test files related to hashSignedTx
echo "Looking for test files..."
fd -e test.ts -e spec.ts hashLedger
# Search for test cases using custom definitions
echo "Searching for test cases with custom definitions..."
rg -l "hashSignedTx.*definitions" -t ts
# Check for any existing test patterns we might want to follow
echo "Checking for similar test patterns..."
rg "definitions.*encode|decode" -t ts
Length of output: 30554
Script:
#!/bin/bash
# Check test coverage for hashSignedTx with custom definitions
echo "Checking test file content..."
cat packages/xrpl/test/utils/hashLedger.test.ts
# Look for any other test files that might test hashSignedTx
echo -e "\nChecking for other relevant test files..."
rg -l "hashSignedTx.*test"
Length of output: 7250
Script:
#!/bin/bash
# Search for tests specifically covering hashSignedTx
echo "Searching for hashSignedTx tests..."
rg -C 5 "hashSignedTx" packages/xrpl/test/
# Check if there are any test files we might have missed
echo -e "\nChecking for any other test files that might contain relevant tests..."
fd -e test.ts -e spec.ts -x grep -l "hashSignedTx" {}
Length of output: 18386
| /** | ||
| * Validate that the passed transaction is a valid type against the types provided by the custom definitions. | ||
| * | ||
| * @param tx - A Transaction. | ||
| * @param definitions - Custom definitions | ||
| * @throws When the passed transaction type is not found in the definitions. | ||
| */ | ||
| export function validateTxAgainstCustomDefintions( | ||
| tx: Record<string, unknown>, | ||
| definitions: XrplDefinitionsBase, | ||
| ): void { | ||
| // Validate just transaction type for now, leaving it open for further validations against the custom definition spec. | ||
| const txType = tx.TransactionType | ||
| if (typeof txType !== 'string') { | ||
| throw new ValidationError( | ||
| 'TransactionType field is not specified or not a string', | ||
| ) | ||
| } | ||
|
|
||
| if (!definitions.transactionType[txType]) { | ||
| throw new ValidationError(`Invalid transaction type: ${txType}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for validateTxAgainstCustomDefintions function
The function validateTxAgainstCustomDefintions is used in the transaction validation flow but lacks dedicated test coverage. Specific test cases needed:
- No tests found for validating transactions against custom definitions
- The function is used as a fallback validation in transaction.ts when custom definitions are provided
- Current implementation validates only the transaction type, but lacks tests for both valid and invalid scenarios
🔗 Analysis chain
Verify test coverage for the new validation function
Please ensure that unit tests cover:
- Valid transaction types
- Invalid transaction types
- Invalid input types (non-string TransactionType)
- Edge cases with custom definitions
Let's verify the test coverage:
Additionally, consider these architectural improvements:
- Validate the structure of custom definitions when they're provided
- Add validation for required fields specified in the custom definition
- Consider adding type validation for fields specified in the custom definition
Would you like assistance in implementing these improvements or creating the test cases?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the new validation function
# Look for test files that might contain tests for this function
rg -l "validateTxAgainstCustom" test/
Length of output: 83
Script:
#!/bin/bash
# Let's first find the test directory location
fd -t d "test" .
# Then search for any test files containing the function name
rg -l "validateTxAgainstCustom" .
# Also search for any test files related to transaction validation
rg -l "transaction.*validation" .
Length of output: 511
Script:
#!/bin/bash
# Let's check the content of test files in xrpl package for any related tests
rg -A 5 -B 5 "validateTxAgainstCustom" ./packages/xrpl/test/
# Let's also check how this function is used in transaction.ts
rg -A 5 -B 5 "validateTxAgainstCustom" ./packages/xrpl/src/models/transactions/transaction.ts
# Check for any existing transaction validation tests
rg -A 5 -B 5 "describe.*transaction.*validation" ./packages/xrpl/test/
Length of output: 982
| // eslint-disable-next-line max-params -- Needs 4 params | ||
| export async function submitRequest( | ||
| client: Client, | ||
| signedTransaction: SubmittableTransaction | string, | ||
| failHard = false, | ||
| definitions?: XrplDefinitionsBase, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor 'submitRequest' to use an options object
The function submitRequest now accepts four parameters, exceeding the ESLint max-params limit. Instead of disabling the ESLint rule, consider refactoring the function to accept an options object for optional parameters. This enhances readability and makes it easier to manage additional parameters in the future.
Apply this diff to refactor submitRequest:
-export async function submitRequest(
- client: Client,
- signedTransaction: SubmittableTransaction | string,
- failHard = false,
- definitions?: XrplDefinitionsBase,
-): Promise<SubmitResponse> {
+export async function submitRequest(
+ client: Client,
+ signedTransaction: SubmittableTransaction | string,
+ options: {
+ failHard?: boolean
+ definitions?: XrplDefinitionsBase
+ } = {},
+): Promise<SubmitResponse> {
+ const { failHard = false, definitions } = optionsAnd update the function calls accordingly.
Committable suggestion skipped: line range outside the PR's diff.
| public async getDefinitions(): Promise<void> { | ||
| try { | ||
| const response = await this.request({ | ||
| command: 'server_definitions', | ||
| }) | ||
| this.definitions = new XrplDefinitionsBase( | ||
| JSON.parse(JSON.stringify(response.result)), | ||
| {}, | ||
| ) | ||
| } catch (error) { | ||
| // eslint-disable-next-line no-console -- Print the error to console but allows client to be connected. | ||
| console.error(error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in getDefinitions
Currently, errors in getDefinitions are caught and logged to the console. Logging to the console in a library can be undesirable as it may not be visible to the end-user or could clutter the console output. Consider the following improvements:
- Propagate the error to the caller: Allow the error to bubble up so that the caller can handle it appropriately.
- Emit an error event: Use the event emitter pattern to notify listeners of the error.
| const signedTx = await getSignedTx(this, transaction, { | ||
| ...opts, | ||
| definitions: this.definitions, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent handling of definitions in submitAndWait
Similar to the submit method, the submitAndWait method overwrites any user-provided definitions in opts. Apply the same fix to ensure consistency and respect for user input:
const signedTx = await getSignedTx(this, transaction, {
...opts,
- definitions: this.definitions,
+ definitions: opts?.definitions ?? this.definitions,
})📝 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 signedTx = await getSignedTx(this, transaction, { | |
| ...opts, | |
| definitions: this.definitions, | |
| }) | |
| const signedTx = await getSignedTx(this, transaction, { | |
| ...opts, | |
| definitions: opts?.definitions ?? this.definitions, | |
| }) |
| const signedTx = await getSignedTx(this, transaction, { | ||
| ...opts, | ||
| definitions: this.definitions, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid overwriting user-provided definitions in submit
By spreading ...opts and then setting definitions: this.definitions, any definitions provided by the user in opts will be overwritten by this.definitions. To respect user-supplied definitions, consider prioritizing opts.definitions:
const signedTx = await getSignedTx(this, transaction, {
...opts,
- definitions: this.definitions,
+ definitions: opts?.definitions ?? this.definitions,
})This change ensures that if the user provides definitions in opts, it will be used; otherwise, this.definitions will be applied.
📝 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 signedTx = await getSignedTx(this, transaction, { | |
| ...opts, | |
| definitions: this.definitions, | |
| }) | |
| const signedTx = await getSignedTx(this, transaction, { | |
| ...opts, | |
| definitions: opts?.definitions ?? this.definitions, | |
| }) |
| Flags: 2147483648, | ||
| Sequence: 33626, | ||
| Fee: '10', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use named constants for flag values to improve readability.
Instead of hardcoding magic numbers like Flags: 2147483648, consider using named constants (e.g., Flags: txFlags.Universal) to make the code more readable and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/xrpl/src/models/transactions/transaction.ts (1)
179-186: Enhance JSDoc documentation for customDefinitions parameterWhile the parameter is correctly typed, the documentation could be more descriptive to help users understand:
- The purpose and structure of custom definitions
- When to use this parameter
- Example usage or reference to related documentation
Consider expanding the JSDoc like this:
/** * Verifies various Transaction Types. * Encode/decode and individual type validation. * * @param transaction - A Transaction. - * @param customDefinitions - Optional parameter to validate against a custom definition. + * @param customDefinitions - Optional parameter to validate against custom transaction definitions. + * Use this when working with non-standard transaction types or + * when extending the default XRPL transaction set. * @throws ValidationError When the Transaction is malformed. * @category Utilities */packages/xrpl/src/models/transactions/common.ts (1)
365-366: Consider enhancing validation coverage in future iterations.The current implementation focuses solely on transaction type validation. Consider extending it to include:
- Validation of required fields specified in the custom definition spec
- Type checking for fields based on the definition
- Validation of the definitions parameter structure
Example enhancement:
export function validateTxAgainstCustomDefinitions( tx: Record<string, unknown>, definitions: XrplDefinitionsBase, ): void { // Validate definitions parameter if (!definitions?.transactionType) { throw new ValidationError('Invalid definitions: missing transactionType') } // Validate transaction type const txType = tx.TransactionType if (typeof txType !== 'string') { throw new ValidationError( 'TransactionType field is not specified or not a string', ) } const txDefinition = definitions.transactionType[txType] if (!txDefinition) { throw new ValidationError(`Invalid transaction type: ${txType}`) } // Validate required fields if (txDefinition.required) { for (const field of txDefinition.required) { if (tx[field] === undefined) { throw new ValidationError( `Missing required field for ${txType}: ${field}`, ) } } } // Validate field types if (txDefinition.fields) { for (const [field, fieldDef] of Object.entries(txDefinition.fields)) { if (tx[field] !== undefined) { validateFieldType(tx[field], fieldDef, field) } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/xrpl/src/models/transactions/common.ts(2 hunks)packages/xrpl/src/models/transactions/transaction.ts(4 hunks)
🔇 Additional comments (4)
packages/xrpl/src/models/transactions/transaction.ts (2)
4-4: LGTM: Import statements are properly organized
The new imports for XrplDefinitionsBase and validateTxAgainstCustomDefinitions are correctly placed and align with the PR's objective of supporting custom definitions.
Also applies to: 23-27
420-426: Verify custom definition validation implementation
The default case handling looks good, but let's verify that validateTxAgainstCustomDefinitions properly validates the transaction type against the custom definitions.
Let's check the implementation of validateTxAgainstCustomDefinitions:
✅ Verification successful
Let me gather more information about the validation implementation:
Validation of transaction type against custom definitions is properly implemented
The implementation of validateTxAgainstCustomDefinitions correctly validates that:
- The TransactionType field exists and is a string
- The transaction type is present in the custom definitions' transactionType mapping
The validation is appropriate and matches the error handling in the original code segment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of validateTxAgainstCustomDefinitions
# Search for the implementation
ast-grep --pattern 'function validateTxAgainstCustomDefinitions($_) {
$$$
}'
# Search for its usage to ensure consistent behavior
rg -A 5 'validateTxAgainstCustomDefinitions'
Length of output: 1832
Script:
#!/bin/bash
# Search for the complete implementation of validateTxAgainstCustomDefinitions
cat packages/xrpl/src/models/transactions/common.ts
Length of output: 12412
packages/xrpl/src/models/transactions/common.ts (2)
2-2: LGTM!
The import of XrplDefinitionsBase is correctly placed alongside the existing TRANSACTION_TYPES import from the same package.
354-376: LGTM! Implementation provides a solid foundation for custom definitions support.
The function correctly validates transaction types against custom definitions with proper error handling and type safety.
High Level Overview of Change
In previous versions, custom definition support was added to the
ripple-binary-codecmethods but not the proxy ones in thexrplpackage.This PR adds support for Custom Definitions to the following
xrplmethods:utils.encodeutils.decodeutils.encodeForSigningWallet.signclient.submit()(@tequdev)client.submitAndWait(@tequdev)Context of Change
When using custom definitions, you had to directly use the
encode,decode, andencodeForSigningmethods fromripple-binary-codec. Those features can now be used via the proxy methods in thexrplpackage.Custom definition support has also been added to the
Wallet.signmethod which wasn't the case before.Type of Change
Did you update HISTORY.md?
Test Plan
Added 3 unit tests for:
Wallet.sign(single signature)Wallet.sign(multisignature)Wallet.sign(unknown transaction type)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests