Skip to content
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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions packages/ripple-binary-codec/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

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 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 XrplDefinitionsBase object
  • Example usage with custom definitions

Consider adding a new section that explains:

  1. The purpose and structure of custom definitions
  2. Example of a custom definition object
  3. Complete example showing how to use custom definitions with these methods

Also applies to: 29-29, 57-57, 65-65

Decode a hex-string into a transaction object.
```js
> api.decode('1100612200000000240000000125000000072D0000000055DF530FB14C5304852F20080B0A8EEF3A6BDD044F41F4EBBD68B8B321145FE4FF6240000002540BE4008114D0F5430B66E06498D4CEEC816C7B3337F9982337')
Expand All @@ -26,7 +26,7 @@ Decode a hex-string into a transaction object.
}
```

### encode(json: object): string
### encode(json: object, definitions?: XrplDefinitionsBase): string
Encode a transaction object into a hex-string. Note that encode filters out fields with undefined values.
```js
> api.encode({
Expand All @@ -37,12 +37,12 @@ Encode a transaction object into a hex-string. Note that encode filters out fiel
OwnerCount: 0,
PreviousTxnID: 'DF530FB14C5304852F20080B0A8EEF3A6BDD044F41F4EBBD68B8B321145FE4FF',
Balance: '10000000000',
Account: 'rLs1MzkFWCxTbuAHgjeTZK4fcCDDnf2KRv'
Account: 'rLs1MzkFWCxTbuAHgjeTZK4fcCDDnf2KRv'
})
'1100612200000000240000000125000000072D0000000055DF530FB14C5304852F20080B0A8EEF3A6BDD044F41F4EBBD68B8B321145FE4FF6240000002540BE4008114D0F5430B66E06498D4CEEC816C7B3337F9982337'
```

#### X-Address Compatibility
#### X-Address Compatibility
* ripple-binary-codec handles X-addresses by looking for a few specific files (Account/SourceTag, Destination/DestinationTag).
* If other fields (in the future) must to support X-addresses with tags, this library will need to be updated.
* When decoding rippled binary, the output will always output classic address + tag, with no X-addresses. X-address support only applies when encoding to binary.
Expand All @@ -54,15 +54,15 @@ Encode a transaction object into a hex-string. Note that encode filters out fiel
* When _decoding_, if a currency code is three uppercase letters or numbers (`/^[A-Z0-9]{3}$/`), then it will be decoded into that string. For example,`0000000000000000000000004142430000000000` decodes as `ABC`.
* When decoding, if a currency code is does not match the regex, then it is not considered to be an ISO 4217 or pseudo-ISO currency. ripple-binary-codec will return a 160-bit hex-string (40 hex characters). For example, `0000000000000000000000006142430000000000` (`aBC`) decodes as `0000000000000000000000006142430000000000` because it contains a lowercase letter.

### encodeForSigning(json: object): string
### encodeForSigning(json: object, definitions?: XrplDefinitionsBase): string

Encode the transaction object for signing.

### encodeForSigningClaim(json: object): string

Encode the transaction object for payment channel claim.

### encodeForMultisigning(json: object, signer: string): string
### encodeForMultisigning(json: object, signer: string, definitions?: XrplDefinitionsBase): string

Encode the transaction object for multi-signing.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,
For the sake of completeness, aren't you interested in modifying the signatures of the below two functions?

decodeLedgerData(binary: string): object
encodeForSigningClaim(json: object): string

Aren't custom definitions useful for these methods?


Expand All @@ -72,7 +72,7 @@ Encode the transaction object for multi-signing.
'5D06F4C3362FE1D0'
```

### decodeQuality(value: string): string
### decodeQuality(value: string): string
```js
> api.decodeQuality('5D06F4C3362FE1D0')
'195796912.5171664'
Expand Down
3 changes: 3 additions & 0 deletions packages/xrpl/HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr
* Add missing `lsfAMMNode` flag to `RippleState` ledger object
* Add `PreviousFields` to `DeletedNode` metadata type

### Added
elmurci marked this conversation as resolved.
Show resolved Hide resolved
* Custom definitions support for `util.encode`, `util.decode`, `util.encodeForSignning` and `Wallet.sign`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in util.encodeForSignning


## 3.0.0 (2024-02-01)

### BREAKING CHANGES
Expand Down
28 changes: 22 additions & 6 deletions packages/xrpl/src/Wallet/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
encodeForSigning,
encodeForMultisigning,
encode,
XrplDefinitionsBase,
} from 'ripple-binary-codec'
import {
deriveAddress,
Expand Down Expand Up @@ -367,15 +368,17 @@ export class Wallet {
* @param this - Wallet instance.
* @param transaction - A transaction to be signed offline.
* @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.
* @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.
*/
// eslint-disable-next-line max-lines-per-function -- introduced more checks to support both string and boolean inputs.
// eslint-disable-next-line max-lines-per-function, max-params -- introduced more checks to support string and boolean inputs.
public sign(
this: Wallet,
transaction: Transaction,
multisign?: boolean | string,
definitions?: XrplDefinitionsBase,
): {
tx_blob: string
hash: string
Expand Down Expand Up @@ -406,7 +409,10 @@ export class Wallet {
* This will throw a more clear error for JS users if the supplied transaction has incorrect formatting
*/
// eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- validate does not accept Transaction type
validate(tx as unknown as Record<string, unknown>)
validate(
tx as unknown as Record<string, unknown>,
definitions ? true : false,
)

const txToSignAndEncode = { ...tx }

Expand All @@ -420,20 +426,24 @@ export class Wallet {
txToSignAndEncode,
this.privateKey,
multisignAddress,
definitions,
),
}
txToSignAndEncode.Signers = [{ Signer: signer }]
} else {
txToSignAndEncode.TxnSignature = computeSignature(
txToSignAndEncode,
this.privateKey,
undefined,
definitions,
)
}

const serialized = encode(txToSignAndEncode)
const serialized = encode(txToSignAndEncode, definitions)

return {
tx_blob: serialized,
hash: hashSignedTx(serialized),
hash: hashSignedTx(serialized, definitions),
}
}

Expand Down Expand Up @@ -466,22 +476,28 @@ export class Wallet {
* @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.
* Can be either a classic address or an XAddress.
* @returns A signed transaction in the proper format.
*/
// eslint-disable-next-line max-params -- Needs 4 params
function computeSignature(
tx: Transaction,
privateKey: string,
signAs?: string,
definitions?: XrplDefinitionsBase,
): string {
if (signAs) {
const classicAddress = isValidXAddress(signAs)
? xAddressToClassicAddress(signAs).classicAddress
: signAs

return sign(encodeForMultisigning(tx, classicAddress), privateKey)
return sign(
encodeForMultisigning(tx, classicAddress, definitions),
privateKey,
)
}
return sign(encodeForSigning(tx), privateKey)
return sign(encodeForSigning(tx, definitions), privateKey)
}

/**
Expand Down
12 changes: 8 additions & 4 deletions packages/xrpl/src/models/transactions/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,10 @@ export interface TransactionAndMetadata<
* @throws ValidationError When the Transaction is malformed.
* @category Utilities
*/
export function validate(transaction: Record<string, unknown>): void {
export function validate(
transaction: Record<string, unknown>,
hasCustomDefinition?: boolean,
): void {
const tx = { ...transaction }
if (tx.TransactionType == null) {
throw new ValidationError('Object does not have a `TransactionType`')
Expand Down Expand Up @@ -395,8 +398,9 @@ export function validate(transaction: Record<string, unknown>): void {
break

default:
throw new ValidationError(
`Invalid field TransactionType: ${tx.TransactionType}`,
)
if (!customDefinition)
elmurci marked this conversation as resolved.
Show resolved Hide resolved
throw new ValidationError(
elmurci marked this conversation as resolved.
Show resolved Hide resolved
`Invalid field TransactionType: ${tx.TransactionType}`,
)
}
}
11 changes: 7 additions & 4 deletions packages/xrpl/src/utils/hashes/hashLedger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { bytesToHex } from '@xrplf/isomorphic/utils'
import BigNumber from 'bignumber.js'
import { decode, encode } from 'ripple-binary-codec'
import { decode, encode, XrplDefinitionsBase } from 'ripple-binary-codec'

import { ValidationError, XrplError } from '../../errors'
import type { Ledger } from '../../models/ledger'
Expand Down Expand Up @@ -68,15 +68,18 @@ function addLengthPrefix(hex: string): string {
* @throws ValidationError if the Transaction is unsigned.\
* @category Utilities
*/
export function hashSignedTx(tx: Transaction | string): string {
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)
Comment on lines +73 to +84
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

txObject = tx
}

Expand Down
34 changes: 26 additions & 8 deletions packages/xrpl/src/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
encodeForMultisigning as rbcEncodeForMultisigning,
encodeForSigning as rbcEncodeForSigning,
encodeForSigningClaim as rbcEncodeForSigningClaim,
XrplDefinitionsBase,
} from 'ripple-binary-codec'
import { verify as verifyKeypairSignature } from 'ripple-keypairs'

Expand Down Expand Up @@ -86,20 +87,28 @@ function isValidSecret(secret: string): boolean {
* Encodes a LedgerEntry or Transaction into a hex string
*
* @param object - LedgerEntry or Transaction in JSON format.
* @param definitions Custom rippled types to use instead of the default. Used for sidechains and amendments.
* @returns A hex string representing the encoded object.
*/
function encode(object: Transaction | LedgerEntry): string {
return rbcEncode(object)
function encode(
object: Transaction | LedgerEntry,
definitions?: XrplDefinitionsBase,
): string {
return rbcEncode(object, definitions)
}

/**
* Encodes a Transaction for signing
*
* @param object - LedgerEntry in JSON or Transaction format.
* @param definitions Custom rippled types to use instead of the default. Used for sidechains and amendments.
* @returns A hex string representing the encoded object.
*/
function encodeForSigning(object: Transaction): string {
return rbcEncodeForSigning(object)
function encodeForSigning(
object: Transaction,
definitions?: XrplDefinitionsBase,
): string {
return rbcEncodeForSigning(object, definitions)
}

/**
Expand All @@ -117,20 +126,29 @@ function encodeForSigningClaim(object: PaymentChannelClaim): string {
*
* @param object - Transaction in JSON format.
* @param signer - The address of the account signing this transaction
* @param definitions Custom rippled types to use instead of the default. Used for sidechains and amendments.
* @returns A hex string representing the encoded object.
*/
function encodeForMultiSigning(object: Transaction, signer: string): string {
return rbcEncodeForMultisigning(object, signer)
function encodeForMultiSigning(
object: Transaction,
signer: string,
definitions?: XrplDefinitionsBase,
): string {
return rbcEncodeForMultisigning(object, signer, definitions)
}

/**
* Decodes a hex string into a transaction | ledger entry
*
* @param hex - hex string in the XRPL serialization format.
* @param definitions Custom rippled types to use instead of the default. Used for sidechains and amendments.
* @returns The hex string decoded according to XRPL serialization format.
*/
function decode(hex: string): Record<string, unknown> {
return rbcDecode(hex)
function decode(
hex: string,
definitions?: XrplDefinitionsBase,
): Record<string, unknown> {
return rbcDecode(hex, definitions)
}

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/xrpl/test/fixtures/requests/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ import signAsSign from './signAs.json'
import escrowSign from './signEscrow.json'
import signPaymentChannelClaim from './signPaymentChannelClaim.json'
import ticketSign from './signTicket.json'
import signAsCustomDefinition from './signAsCustomDefinition.json'

const sign = {
normal: normalSign,
ticket: ticketSign,
escrow: escrowSign,
signAs: signAsSign,
signAsCustomDefinition,
}

const getOrderbook = {
Expand Down
19 changes: 19 additions & 0 deletions packages/xrpl/test/fixtures/requests/signAsCustomDefinition.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"Account": "rEuLyBCvcw4CFmzv8RepSiAoNgF8tTGJQC",
"TransactionType": "TokenSwapPropose",
"AccountOther": "rJyZ28c179hKg7Gwt4P2S8zgzUTmMUMmzs",
"Expiration": 773819038,
"Flags": 2147483648,
"Sequence": 33626,
"Fee": "10",
"Amount": {
"currency": "AAA",
"issuer": "rDCcTxoALtAryzk4TE3mxU9hpjRm5vQcxT",
"value": "1"
},
"AmountOther": {
"currency": "BBB",
"issuer": "rDCcTxoALtAryzk4TE3mxU9hpjRm5vQcxT",
"value": "1"
}
}
4 changes: 4 additions & 0 deletions packages/xrpl/test/fixtures/responses/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import signAsSign from './signAs.json'
import escrowSign from './signEscrow.json'
import signPaymentChannelClaim from './signPaymentChannelClaim.json'
import ticketSign from './signTicket.json'
import signCustomDefinition from './signCustomDefinition.json'
import signAsCustomDefinition from './signAsCustomDefinition.json'

const getOrderbook = {
normal: normalOrderBook,
Expand All @@ -26,6 +28,8 @@ const sign = {
ticket: ticketSign,
escrow: escrowSign,
signAs: signAsSign,
signCustomDefinition,
signAsCustomDefinition,
}

const responses = {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"signedTransaction": "1200352280000000240000835A2A2E1F8A9E61D4838D7EA4C6800000000000000000000000000041414100000000008AEEEBA80624759E48F732403D7C70D591BB3D0A68400000000000000A6033D4838D7EA4C6800000000000000000000000000042424200000000008AEEEBA80624759E48F732403D7C70D591BB3D0A73008114A3780F5CB5A44D366520FC44055E8ED44D9A2270803414C52CA1760EA5FAB5DEA0092688910C2A47F73E28F3E010732102A8A44DB3D4C73EEEE11DFE54D2029103B776AA8A8D293A91D645977C9DF5F54474473045022100DEDDC3681C21A419CD05D7EA9D43B0EC12EB73A89EBB375B34E962340491D9E60220269D176AACB31B59B1E749CF3F642B77F76C3C2185B9BC0CA3C45712A060788C8114B3263BD0A9BF9DFDBBBBD07F536355FF477BF0E9E1F1",
"id": "3062208655FA2581C9465197015E1FB3FB3BA2BF47323F8D1516ECF62AF3BA45"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"signedTransaction": "1200352280000000240000835A2A2E1F8A9E61D4838D7EA4C6800000000000000000000000000041414100000000008AEEEBA80624759E48F732403D7C70D591BB3D0A68400000000000000A6033D4838D7EA4C6800000000000000000000000000042424200000000008AEEEBA80624759E48F732403D7C70D591BB3D0A732102A8A44DB3D4C73EEEE11DFE54D2029103B776AA8A8D293A91D645977C9DF5F5447446304402205849DD6E2B936C955C6ECA847ACF7BF74DC363E572001E7FFA16DC0095244A0F02201B27BBA3992FDB0A8ADC24DFEF25AE4C2886786C0D290E8DE65618568E0F4F898114B3263BD0A9BF9DFDBBBBD07F536355FF477BF0E9803414C52CA1760EA5FAB5DEA0092688910C2A47F73E28",
"id": "A8F2FD97564CA3C15A184A2149EB8FDD32D1C14E6251E9892A4D4B2EC2E6D54C"
}
Loading
Loading