feat(xrpl): custom definitions support#3229
Conversation
…ncodeForSigning, decode, computeSignature.
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()`
Co-authored-by: tequ <git@tequ.dev>
…ustom_definition_support
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThreads an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tequdev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/xrpl/src/utils/index.ts (1)
17-25:⚠️ Potential issue | 🟡 MinorUse a type-only import for
XrplDefinitionsBase.
XrplDefinitionsBaseis only used in type positions (function parameter annotations), not at runtime. Although this doesn't currently cause issues, usingimport typefollows TypeScript best practices and ensures compatibility with stricter compiler settings likeverbatimModuleSyntax.♻️ Proposed fix
import { encode as rbcEncode, decode as rbcDecode, encodeForMultisigning as rbcEncodeForMultisigning, encodeForSigning as rbcEncodeForSigning, encodeForSigningClaim as rbcEncodeForSigningClaim, - XrplDefinitionsBase, encodeForSigningBatch as rbcEncodeForSigningBatch, } from 'ripple-binary-codec' +import type { XrplDefinitionsBase } from 'ripple-binary-codec'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/utils/index.ts` around lines 17 - 25, The import currently brings in XrplDefinitionsBase at runtime; change it to a type-only import so the symbol is not emitted at runtime. Replace the existing import that includes XrplDefinitionsBase with an import type for XrplDefinitionsBase (keeping the other runtime imports as-is) so functions using XrplDefinitionsBase in parameter annotations still compile under strict TS settings (e.g., verbatimModuleSyntax) without importing the type at runtime.packages/xrpl/src/client/index.ts (1)
863-890:⚠️ Potential issue | 🟠 MajorMissing
definitionsparameter inhashSignedTxcall.At line 883,
hashSignedTx(signedTx)is called without passingthis.definitions. Since the transaction was signed using custom definitions viagetSignedTx, the hash computation should also use the same definitions to correctly decode/encode the transaction.Proposed fix
- const txHash = hashes.hashSignedTx(signedTx) + const txHash = hashes.hashSignedTx(signedTx, this.definitions)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/client/index.ts` around lines 863 - 890, The call to hashSignedTx(signedTx) omits the custom transaction definitions used when signing, so update the hash calculation to pass the same definitions object (this.definitions) — i.e., call hashes.hashSignedTx(signedTx, this.definitions) — so the computed txHash matches the encoding used in getSignedTx; adjust the reference in the function that returns waitForFinalTransactionOutcome(...) to use the corrected txHash.packages/xrpl/src/sugar/submit.ts (1)
238-274:⚠️ Potential issue | 🟠 MajorMissing
definitionsparameter inwallet.sign()call.At line 273,
wallet.sign(tx).tx_blobis called without passingdefinitions. SincegetSignedTxreceivesdefinitionsin its options and uses it for decoding unsigned string transactions (line 266), the signing step should also use the same definitions to properly encode custom transaction types.Proposed fix
- return wallet.sign(tx).tx_blob + return wallet.sign(tx, false, definitions).tx_blob🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/sugar/submit.ts` around lines 238 - 274, The getSignedTx function calls wallet.sign(tx).tx_blob without passing the definitions option, so custom transaction types decoded earlier (via decode(..., definitions)) may be mis-encoded; update the signing call in getSignedTx to pass the same definitions object to wallet.sign (i.e., call wallet.sign(tx, definitions) and return its tx_blob) and ensure the function imports/uses the definitions parameter consistently alongside decode and client.autofill.packages/xrpl/src/models/transactions/transaction.ts (1)
262-270:⚠️ Potential issue | 🟠 Major
validateBaseTransactionblocks custom transaction types beforevalidateTxAgainstCustomDefinitionscan be invoked.The
customDefinitionsparameter is intended to support custom transaction types, butvalidateBaseTransaction(line 269) runs unconditionally before the switch statement and rejects anyTransactionTypenot in the built-inTRANSACTION_TYPESarray. This prevents the default case (line 586) from ever handling custom types withvalidateTxAgainstCustomDefinitions. Either skipvalidateBaseTransactionfor custom types, or move theTransactionTypecheck to occur only after attempting custom definition validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/transaction.ts` around lines 262 - 270, The unconditional call to validateBaseTransaction in validate prevents custom transaction types from reaching validateTxAgainstCustomDefinitions; update validate so it first checks if customDefinitions is provided and the transaction.TransactionType is not in TRANSACTION_TYPES, and in that case skip validateBaseTransaction and attempt validateTxAgainstCustomDefinitions (or alternatively move the built-in TRANSACTION_TYPES check to after the switch/default branch), ensuring validateTxAgainstCustomDefinitions is invoked for custom types before rejecting unknown TransactionType values; look for the validate function, validateBaseTransaction, validateTxAgainstCustomDefinitions, and TRANSACTION_TYPES symbols to implement this control flow change.
🧹 Nitpick comments (1)
packages/xrpl/src/sugar/submit.ts (1)
66-70: Consider:isAccountDeletedecodes without definitions.At line 69,
isAccountDelete(signedTransaction)is called, and this function (lines 305-307) decodes string transactions withoutdefinitions. WhileAccountDeleteis a standard transaction type so this won't cause functional issues, it's inconsistent with the pattern established elsewhere in this file.Proposed fix for consistency
Update
isAccountDeleteto accept and usedefinitions:-function isAccountDelete(transaction: Transaction | string): boolean { - const tx = typeof transaction === 'string' ? decode(transaction) : transaction +function isAccountDelete( + transaction: Transaction | string, + definitions?: XrplDefinitionsBase, +): boolean { + const tx = + typeof transaction === 'string' + ? decode(transaction, definitions) + : transaction return tx.TransactionType === 'AccountDelete' }And update the call site at line 69:
- fail_hard: isAccountDelete(signedTransaction) || failHard, + fail_hard: isAccountDelete(signedTransaction, definitions) || failHard,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/sugar/submit.ts` around lines 66 - 70, The call to isAccountDelete(signedTransaction) omits the consistent use of ripple definitions; update the isAccountDelete function signature to accept a definitions parameter and use it when decoding the signed transaction (modify isAccountDelete to decode with the provided definitions), then update the SubmitRequest creation (the code that builds request with command:'submit', tx_blob:signedTxEncoded, fail_hard:...) to call isAccountDelete(signedTransaction, definitions) so the decode is consistent with other decoders in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/HISTORY.md`:
- Around line 140-142: Remove the duplicate "### Added" heading in the 3.1.0
section of HISTORY.md: locate the repeated "### Added" header and merge its list
item(s) into the existing "### Added" block (keeping the entry "Custom
definitions support for `util.encode`, `util.decode`, `util.encodeForSigning`
and `Wallet.sign`" under that single header), then delete the redundant header
so there is only one "### Added" section for that version.
---
Outside diff comments:
In `@packages/xrpl/src/client/index.ts`:
- Around line 863-890: The call to hashSignedTx(signedTx) omits the custom
transaction definitions used when signing, so update the hash calculation to
pass the same definitions object (this.definitions) — i.e., call
hashes.hashSignedTx(signedTx, this.definitions) — so the computed txHash matches
the encoding used in getSignedTx; adjust the reference in the function that
returns waitForFinalTransactionOutcome(...) to use the corrected txHash.
In `@packages/xrpl/src/models/transactions/transaction.ts`:
- Around line 262-270: The unconditional call to validateBaseTransaction in
validate prevents custom transaction types from reaching
validateTxAgainstCustomDefinitions; update validate so it first checks if
customDefinitions is provided and the transaction.TransactionType is not in
TRANSACTION_TYPES, and in that case skip validateBaseTransaction and attempt
validateTxAgainstCustomDefinitions (or alternatively move the built-in
TRANSACTION_TYPES check to after the switch/default branch), ensuring
validateTxAgainstCustomDefinitions is invoked for custom types before rejecting
unknown TransactionType values; look for the validate function,
validateBaseTransaction, validateTxAgainstCustomDefinitions, and
TRANSACTION_TYPES symbols to implement this control flow change.
In `@packages/xrpl/src/sugar/submit.ts`:
- Around line 238-274: The getSignedTx function calls wallet.sign(tx).tx_blob
without passing the definitions option, so custom transaction types decoded
earlier (via decode(..., definitions)) may be mis-encoded; update the signing
call in getSignedTx to pass the same definitions object to wallet.sign (i.e.,
call wallet.sign(tx, definitions) and return its tx_blob) and ensure the
function imports/uses the definitions parameter consistently alongside decode
and client.autofill.
In `@packages/xrpl/src/utils/index.ts`:
- Around line 17-25: The import currently brings in XrplDefinitionsBase at
runtime; change it to a type-only import so the symbol is not emitted at
runtime. Replace the existing import that includes XrplDefinitionsBase with an
import type for XrplDefinitionsBase (keeping the other runtime imports as-is) so
functions using XrplDefinitionsBase in parameter annotations still compile under
strict TS settings (e.g., verbatimModuleSyntax) without importing the type at
runtime.
---
Nitpick comments:
In `@packages/xrpl/src/sugar/submit.ts`:
- Around line 66-70: The call to isAccountDelete(signedTransaction) omits the
consistent use of ripple definitions; update the isAccountDelete function
signature to accept a definitions parameter and use it when decoding the signed
transaction (modify isAccountDelete to decode with the provided definitions),
then update the SubmitRequest creation (the code that builds request with
command:'submit', tx_blob:signedTxEncoded, fail_hard:...) to call
isAccountDelete(signedTransaction, definitions) so the decode is consistent with
other decoders in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 354528f1-7d81-4bcf-95f8-3a5e29abab72
📒 Files selected for processing (17)
packages/ripple-binary-codec/README.mdpackages/xrpl/HISTORY.mdpackages/xrpl/src/Wallet/index.tspackages/xrpl/src/client/index.tspackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/transaction.tspackages/xrpl/src/sugar/submit.tspackages/xrpl/src/utils/hashes/hashLedger.tspackages/xrpl/src/utils/index.tspackages/xrpl/test/fixtures/requests/index.tspackages/xrpl/test/fixtures/requests/signAsCustomDefinition.jsonpackages/xrpl/test/fixtures/responses/index.tspackages/xrpl/test/fixtures/responses/signAsCustomDefinition.jsonpackages/xrpl/test/fixtures/responses/signCustomDefinition.jsonpackages/xrpl/test/fixtures/rippled/customDefinition.jsonpackages/xrpl/test/fixtures/rippled/index.tspackages/xrpl/test/wallet/index.test.ts
…ustom_definition_support
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/xrpl/src/Wallet/utils.ts (1)
93-93: Minor JSDoc formatting inconsistency.The
@param definitionsis missing the hyphen separator that other parameters in this function use (e.g.,@param tx -,@param privateKey -).📝 Proposed fix
- * `@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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/Wallet/utils.ts` at line 93, Update the JSDoc for the function in packages/xrpl/src/Wallet/utils.ts so the `@param` entry for definitions matches the other param lines (include the " - " hyphen separator); locate the JSDoc block that contains `@param` tx -, `@param` privateKey - and change the `@param` definitions line to "@param definitions - Custom rippled types to use instead of the default. Used for sidechains and amendments." to keep formatting consistent.packages/xrpl/src/Wallet/index.ts (1)
361-361: Minor JSDoc formatting inconsistency.The
@param definitionsis missing the hyphen separator that other parameters in this method use (e.g.,@param transaction -,@param multisign -).📝 Proposed fix
- * `@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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/Wallet/index.ts` at line 361, Update the JSDoc for the Wallet constructor/factory in Wallet/index.ts so the `@param` entry for definitions uses the same hyphen-separated format as the other params (e.g., match style used by `@param` transaction - and `@param` multisign -); locate the JSDoc block above the Wallet constructor or related function and add the missing " - " after "definitions" and include the brief description text to keep formatting consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/xrpl/src/Wallet/index.ts`:
- Line 361: Update the JSDoc for the Wallet constructor/factory in
Wallet/index.ts so the `@param` entry for definitions uses the same
hyphen-separated format as the other params (e.g., match style used by `@param`
transaction - and `@param` multisign -); locate the JSDoc block above the Wallet
constructor or related function and add the missing " - " after "definitions"
and include the brief description text to keep formatting consistent.
In `@packages/xrpl/src/Wallet/utils.ts`:
- Line 93: Update the JSDoc for the function in
packages/xrpl/src/Wallet/utils.ts so the `@param` entry for definitions matches
the other param lines (include the " - " hyphen separator); locate the JSDoc
block that contains `@param` tx -, `@param` privateKey - and change the `@param`
definitions line to "@param definitions - Custom rippled types to use instead of
the default. Used for sidechains and amendments." to keep formatting consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7af2c47a-8234-44d6-850e-4c02915d364d
📒 Files selected for processing (2)
packages/xrpl/src/Wallet/index.tspackages/xrpl/src/Wallet/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/models/transactions/transaction.ts`:
- Around line 261-264: The Batch branch in the export function
validate(transaction: Record<string, unknown>, customDefinitions?:
XrplDefinitionsBase) calls validate(...) recursively for inner transactions but
does not pass along the customDefinitions parameter; update the recursive
validate calls inside the Batch handling (and the similar call at the second
occurrence around the other Batch branch) to forward the customDefinitions
argument so inner/custom transaction types are validated against the provided
XrplDefinitionsBase rather than defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e00dcb52-4e2a-4b13-bc6e-8d8eea092347
📒 Files selected for processing (3)
packages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/transaction.tspackages/xrpl/test/wallet/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/wallet/index.test.ts
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two bugs flagged inline: definitions not forwarded to submitRequest (line 790 in client/index.ts) causing silent failures for custom types, and a design gap where custom transaction types bypass all field-level validation in transaction.ts.
Review by Claude Opus 4.6 · Prompt: V13
| } = {}, | ||
| ): Promise<SubmittableTransaction | string> { | ||
| if (isSigned(transaction)) { | ||
| if (isSigned(transaction, definitions)) { |
There was a problem hiding this comment.
If definitions is undefined, isSigned decodes via standard codec which can't handle custom types — may silently return true for an unsigned custom transaction. This is the downstream consequence of the missing definitions arg in client/index.ts line 790; fixing that call site resolves this.
| throw new ValidationError( | ||
| `Invalid field TransactionType: ${tx.TransactionType}`, | ||
| ) | ||
| if (!customDefinitions?.transactionNames.includes(tx.TransactionType)) { |
There was a problem hiding this comment.
Custom transaction types skip all field-level validation — only validateBaseTransaction runs. Document this limitation and consider a hook for registering field validators for custom types.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/xrpl/src/client/index.ts (1)
878-898:⚠️ Potential issue | 🟠 Major
submitAndWaitstill dropsthis.definitionsinsubmitRequestandhashSignedTx.
getSignedTxis passeddefinitions, but the subsequentsubmitRequest(this, signedTx, opts?.failHard)(line 890) andhashes.hashSignedTx(signedTx)(line 898) do not receivethis.definitions. For custom transaction types this can cause decode/hash failures or a hash mismatch vs. whatWallet.signproduced — sowaitForFinalTransactionOutcomemay never find the tx. The same applies tosubmitat line 805. (This was raised in a prior review forsubmitRequest.)Suggested change
- const response = await submitRequest(this, signedTx, opts?.failHard) + const response = await submitRequest( + this, + signedTx, + opts?.failHard, + this.definitions, + ) ... - const txHash = hashes.hashSignedTx(signedTx) + const txHash = hashes.hashSignedTx(signedTx, this.definitions)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/client/index.ts` around lines 878 - 898, The submitAndWait (and submit) flow currently passes this.definitions into getSignedTx but fails to pass definitions into submitRequest and hashes.hashSignedTx, which breaks custom types/hashes; update calls to submitRequest(this, signedTx, opts?.failHard) and hashes.hashSignedTx(signedTx) to include this.definitions (or pass a merged opts.definitions) so both submitRequest and hashSignedTx receive the same definitions used to build the signedTx, and apply the same fix in the submit function to ensure consistent decoding/hashing for custom transaction types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/client/index.ts`:
- Around line 229-234: Client.definitions is declared but not exposed via
ClientOptions; add an optional definitions?: XrplDefinitionsBase to the
ClientOptions type/interface and assign this.options.definitions (or
options.definitions) to this.definitions in the Client constructor, following
the pattern used for feeCushion/maxFeeXRP, and ensure any internal uses (e.g.,
submit and submitAndWait) read from this.definitions rather than only from
mutated instances so the value is available immediately after new Client(...).
---
Duplicate comments:
In `@packages/xrpl/src/client/index.ts`:
- Around line 878-898: The submitAndWait (and submit) flow currently passes
this.definitions into getSignedTx but fails to pass definitions into
submitRequest and hashes.hashSignedTx, which breaks custom types/hashes; update
calls to submitRequest(this, signedTx, opts?.failHard) and
hashes.hashSignedTx(signedTx) to include this.definitions (or pass a merged
opts.definitions) so both submitRequest and hashSignedTx receive the same
definitions used to build the signedTx, and apply the same fix in the submit
function to ensure consistent decoding/hashing for custom transaction types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8484c7aa-15d0-4b2f-85a6-b59f20ba1164
📒 Files selected for processing (8)
packages/xrpl/HISTORY.mdpackages/xrpl/src/Wallet/index.tspackages/xrpl/src/client/index.tspackages/xrpl/src/models/transactions/transaction.tspackages/xrpl/test/fixtures/responses/signAsCustomDefinition.jsonpackages/xrpl/test/fixtures/responses/signCustomDefinition.jsonpackages/xrpl/test/fixtures/rippled/customDefinition.jsonpackages/xrpl/test/wallet/index.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/xrpl/test/fixtures/responses/signCustomDefinition.json
- packages/xrpl/test/fixtures/responses/signAsCustomDefinition.json
- packages/xrpl/HISTORY.md
| /** | ||
| * Custom rippled types to use instead of the default. Used for sidechains and amendments. | ||
| * | ||
| */ | ||
| public definitions: XrplDefinitionsBase | undefined | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Client.definitions is never wired into ClientOptions.
The new public field is declared but there is no way to set it at construction — users must mutate client.definitions = ... after new Client(...). Consider exposing it through ClientOptions (e.g., options.definitions) and assigning it in the constructor, mirroring feeCushion/maxFeeXRP. This also makes the behavior documentable and consistent with how the property is threaded into submit/submitAndWait.
Suggested change
export interface ClientOptions extends ConnectionUserOptions {
...
timeout?: number
+ /**
+ * Custom rippled transaction type definitions for sidechains/amendments.
+ */
+ definitions?: XrplDefinitionsBase
} public definitions: XrplDefinitionsBase | undefined
...
public constructor(server: string, options: ClientOptions = {}) {
...
this.feeCushion = options.feeCushion ?? DEFAULT_FEE_CUSHION
this.maxFeeXRP = options.maxFeeXRP ?? DEFAULT_MAX_FEE_XRP
+ this.definitions = options.definitions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/client/index.ts` around lines 229 - 234, Client.definitions
is declared but not exposed via ClientOptions; add an optional definitions?:
XrplDefinitionsBase to the ClientOptions type/interface and assign
this.options.definitions (or options.definitions) to this.definitions in the
Client constructor, following the pattern used for feeCushion/maxFeeXRP, and
ensure any internal uses (e.g., submit and submitAndWait) read from
this.definitions rather than only from mutated instances so the value is
available immediately after new Client(...).
Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com>
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| typeof transaction === 'string' | ||
| ? // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- converts JsonObject to correct Transaction type | ||
| (decode(transaction) as unknown as SubmittableTransaction) | ||
| (decode(transaction, definitions) as unknown as SubmittableTransaction) |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
getSignedTx passes definitions to decode() here and to isSigned() above, but the subsequent wallet.sign(tx) call (line 273) does not receive definitions. For custom transaction types, this means the transaction is decoded with custom definitions but signed/encoded with default definitions, producing incorrect signatures or encoding failures.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: On line 273, pass definitions as the third argument to wallet.sign(). The Wallet.sign() method accepts definitions as its third parameter (after multisign). Change:
return wallet.sign(tx).tx_blobto:
return wallet.sign(tx, undefined, definitions).tx_blobThis ensures that custom transaction types are signed and encoded using the same custom definitions that were used for decoding and isSigned() checks earlier in the function.
| const signedTx = await getSignedTx(this, transaction, opts) | ||
| const signedTx = await getSignedTx(this, transaction, { | ||
| ...opts, | ||
| definitions: this.definitions, |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
submitAndWait passes this.definitions to getSignedTx here, but omits it from the subsequent submitRequest() and hashSignedTx() calls below. Compare with submit() which correctly passes this.definitions to submitRequest (line 824). This causes isSigned check, encoding, and hash computation to use wrong (default) definitions for custom types.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Pass this.definitions to both submitRequest and hashes.hashSignedTx in the submitAndWait method, matching the pattern used in submit():
-
On line 909, change:
const response = await submitRequest(this, signedTx, opts?.failHard)
to:
const response = await submitRequest(this, signedTx, opts?.failHard, this.definitions) -
On line 917, change:
const txHash = hashes.hashSignedTx(signedTx)
to:
const txHash = hashes.hashSignedTx(signedTx, this.definitions)
This ensures that custom definitions are used for the isSigned check, encode call in submitRequest, and the decode/encode calls in hashSignedTx, consistent with how submit() already passes this.definitions on line 824.
5 participants
Handover for #2683
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()client.submitAndWaitContext 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)