-
Notifications
You must be signed in to change notification settings - Fork 514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(xrpl): custom definitions support #2683
base: main
Are you sure you want to change the base?
Changes from 15 commits
23411ac
9249824
b529149
a618fb9
b283fc3
4689b02
59c2074
6089035
7021e63
02a8503
3153659
b047f04
eeb9f43
2900ab3
2c60a1e
071beb5
04446ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,6 +2,7 @@ | |||||||||||||||||
|
||||||||||||||||||
/* eslint-disable max-lines -- Client is a large file w/ lots of imports/exports */ | ||||||||||||||||||
import { EventEmitter } from 'eventemitter3' | ||||||||||||||||||
import { XrplDefinitionsBase } from 'ripple-binary-codec' | ||||||||||||||||||
|
||||||||||||||||||
import { | ||||||||||||||||||
RippledError, | ||||||||||||||||||
|
@@ -218,6 +219,12 @@ class Client extends EventEmitter<EventTypes> { | |||||||||||||||||
*/ | ||||||||||||||||||
public buildVersion: string | undefined | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* Custom rippled types to use instead of the default. Used for sidechains and amendments. | ||||||||||||||||||
* | ||||||||||||||||||
*/ | ||||||||||||||||||
public definitions: XrplDefinitionsBase | undefined | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* API Version used by the server this client is connected to | ||||||||||||||||||
* | ||||||||||||||||||
|
@@ -526,6 +533,33 @@ class Client extends EventEmitter<EventTypes> { | |||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* Get Definitions from server_definitions | ||||||||||||||||||
* | ||||||||||||||||||
* @returns void | ||||||||||||||||||
* @example | ||||||||||||||||||
* ```ts | ||||||||||||||||||
* const { Client } = require('xrpl') | ||||||||||||||||||
* const client = new Client('wss://s.altnet.rippletest.net:51233') | ||||||||||||||||||
* await client.getDefinitions() | ||||||||||||||||||
* console.log(client.definitions) | ||||||||||||||||||
* ``` | ||||||||||||||||||
*/ | ||||||||||||||||||
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) | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
Comment on lines
+548
to
+561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve error handling in Currently, errors in
|
||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* Tells the Client instance to connect to its rippled server. | ||||||||||||||||||
* | ||||||||||||||||||
|
@@ -760,7 +794,10 @@ class Client extends EventEmitter<EventTypes> { | |||||||||||||||||
wallet?: Wallet | ||||||||||||||||||
}, | ||||||||||||||||||
): Promise<SubmitResponse> { | ||||||||||||||||||
const signedTx = await getSignedTx(this, transaction, opts) | ||||||||||||||||||
const signedTx = await getSignedTx(this, transaction, { | ||||||||||||||||||
...opts, | ||||||||||||||||||
definitions: this.definitions, | ||||||||||||||||||
}) | ||||||||||||||||||
Comment on lines
+797
to
+800
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Avoid overwriting user-provided definitions in By spreading const signedTx = await getSignedTx(this, transaction, {
...opts,
- definitions: this.definitions,
+ definitions: opts?.definitions ?? this.definitions,
}) This change ensures that if the user provides 📝 Committable suggestion
Suggested change
|
||||||||||||||||||
return submitRequest(this, signedTx, opts?.failHard) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -834,7 +871,10 @@ class Client extends EventEmitter<EventTypes> { | |||||||||||||||||
wallet?: Wallet | ||||||||||||||||||
}, | ||||||||||||||||||
): Promise<TxResponse<T>> { | ||||||||||||||||||
const signedTx = await getSignedTx(this, transaction, opts) | ||||||||||||||||||
const signedTx = await getSignedTx(this, transaction, { | ||||||||||||||||||
...opts, | ||||||||||||||||||
definitions: this.definitions, | ||||||||||||||||||
}) | ||||||||||||||||||
Comment on lines
+874
to
+877
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consistent handling of definitions in Similar to the const signedTx = await getSignedTx(this, transaction, {
...opts,
- definitions: this.definitions,
+ definitions: opts?.definitions ?? this.definitions,
}) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||
|
||||||||||||||||||
const lastLedger = getLastLedgerSequence(signedTx) | ||||||||||||||||||
if (lastLedger == null) { | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import { isValidClassicAddress, isValidXAddress } from 'ripple-address-codec' | ||
import { TRANSACTION_TYPES } from 'ripple-binary-codec' | ||
import { TRANSACTION_TYPES, XrplDefinitionsBase } from 'ripple-binary-codec' | ||
|
||
import { ValidationError } from '../../errors' | ||
import { | ||
|
@@ -351,6 +351,30 @@ export function validateBaseTransaction(common: Record<string, unknown>): void { | |
validateOptionalField(common, 'NetworkID', isNumber) | ||
} | ||
|
||
/** | ||
* 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( | ||
intelliot marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Missing test coverage for validateTxAgainstCustomDefintions function The function
🔗 Analysis chainVerify test coverage for the new validation function Please ensure that unit tests cover:
Let's verify the test coverage: Additionally, consider these architectural improvements:
Would you like assistance in implementing these improvements or creating the test cases? 🏁 Scripts executedThe 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 |
||
|
||
/** | ||
* Parse the value of an amount, expressed either in XRP or as an Issued Currency, into a number. | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Documentation needed for the new
definitions
parameterThe new
definitions
parameter has been added to multiple methods, but there's no explanation of:XrplDefinitionsBase
objectConsider adding a new section that explains:
Also applies to: 29-29, 57-57, 65-65