-
Notifications
You must be signed in to change notification settings - Fork 520
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 7 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 |
---|---|---|
|
@@ -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 | ||
Decode a hex-string into a transaction object. | ||
```js | ||
> api.decode('1100612200000000240000000125000000072D0000000055DF530FB14C5304852F20080B0A8EEF3A6BDD044F41F4EBBD68B8B321145FE4FF6240000002540BE4008114D0F5430B66E06498D4CEEC816C7B3337F9982337') | ||
|
@@ -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({ | ||
|
@@ -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. | ||
|
@@ -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. | ||
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. Hello,
Aren't custom definitions useful for these methods? |
||
|
||
|
@@ -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' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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`. | ||
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. typo in |
||
|
||
## 3.0.0 (2024-02-01) | ||
|
||
### BREAKING CHANGES | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
|
@@ -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
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 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
However, there are no test cases that explicitly verify the new custom definitions parameter. You should add test cases to cover:
🔗 Analysis chainVerify 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 Let me check for test coverage specifically for the 🏁 Scripts executedThe 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 | ||
} | ||
|
||
|
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" | ||
} | ||
} |
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" | ||
} |
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