fix #2911: Update OracleSet transaction model, unit, integ tests #2913
Conversation
WalkthroughThis pull request updates the handling of the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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. 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/src/models/transactions/oracleSet.ts (1)
13-13: Fix import ordering and formatting.The import statement for
HEX_REGEXshould be placed with the other imports before line 13, and spacing should be fixed.- import {HEX_REGEX} from 'ripple-binary-codec/dist/types/uint-64' + import { HEX_REGEX } from 'ripple-binary-codec/dist/types/uint-64'🧰 Tools
🪛 ESLint
[error] 13-13:
ripple-binary-codec/dist/types/uint-64import should occur before import of../../errors(import/order)
[error] 13-13: Replace
HEX_REGEXwith·HEX_REGEX·(prettier/prettier)
packages/xrpl/HISTORY.md (1)
7-9: Changelog Entry Clarity and Traceability:
The new "Fixed" entry under "Unreleased" clearly indicates that the OracleSet transaction now accepts hexadecimal string values for theAssetPricefield. This aligns well with the bug fix outlined in the PR objectives.Suggestion: Consider appending a reference to the associated issue (e.g.,
(#2911)) to improve traceability and to help users quickly correlate the changelog entry with the underlying fix.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/ripple-binary-codec/src/types/uint-64.ts(1 hunks)packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/oracleSet.ts(2 hunks)packages/xrpl/test/integration/transactions/oracleSet.test.ts(2 hunks)packages/xrpl/test/models/oracleSet.test.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
packages/xrpl/src/models/transactions/oracleSet.ts
[error] 13-13: ripple-binary-codec/dist/types/uint-64 import should occur before import of ../../errors
(import/order)
[error] 13-13: Replace HEX_REGEX with ·HEX_REGEX·
(prettier/prettier)
[error] 151-151: Replace isNumber(priceData.PriceData.AssetPrice)·||·(typeof·priceData.PriceData.AssetPrice·===·'string'·&&·HEX_REGEX.test(priceData.PriceData.AssetPrice)) with ⏎··········isNumber(priceData.PriceData.AssetPrice)·||⏎··········(typeof·priceData.PriceData.AssetPrice·===·'string'·&&⏎············HEX_REGEX.test(priceData.PriceData.AssetPrice))⏎········
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: snippets (18.x)
🔇 Additional comments (7)
packages/ripple-binary-codec/src/types/uint-64.ts (1)
132-132: ExportHEX_REGEXto make it available for external validation.This change exposes the internal
HEX_REGEXconstant (which validates hexadecimal strings of 1-16 characters) for use in other modules. This is a good approach for sharing validation patterns consistently across the codebase.packages/xrpl/src/models/transactions/oracleSet.ts (1)
151-151: EnhanceAssetPricevalidation to support hexadecimal strings.Good enhancement to allow both numeric values and properly formatted hexadecimal strings for the
AssetPricefield. This provides more flexibility, especially for large values that can't be precisely represented by JavaScript numbers.The conditional could be formatted more clearly for readability:
- !(isNumber(priceData.PriceData.AssetPrice) || (typeof priceData.PriceData.AssetPrice === 'string' && HEX_REGEX.test(priceData.PriceData.AssetPrice))) + !( + isNumber(priceData.PriceData.AssetPrice) || + (typeof priceData.PriceData.AssetPrice === 'string' && + HEX_REGEX.test(priceData.PriceData.AssetPrice) + ) + )🧰 Tools
🪛 ESLint
[error] 151-151: Replace
isNumber(priceData.PriceData.AssetPrice)·||·(typeof·priceData.PriceData.AssetPrice·===·'string'·&&·HEX_REGEX.test(priceData.PriceData.AssetPrice))with⏎··········isNumber(priceData.PriceData.AssetPrice)·||⏎··········(typeof·priceData.PriceData.AssetPrice·===·'string'·&&⏎············HEX_REGEX.test(priceData.PriceData.AssetPrice))⏎········(prettier/prettier)
packages/xrpl/test/integration/transactions/oracleSet.test.ts (3)
42-52: Add test case for hexadecimal stringAssetPricevalues.Excellent addition of a test case using a large hexadecimal string value for
AssetPrice. The comments clearly explain why a string representation is necessary for large values that exceed JavaScript's number precision limits.
76-76: Update assertion to account for added test case.Correctly updated the assertion to check for 2 items in the PriceDataSeries array.
83-87: Add validation for hexadecimal string serialization.Good addition of an assertion to specifically verify that large hexadecimal string values for
AssetPriceare properly serialized and preserved.packages/xrpl/test/models/oracleSet.test.ts (2)
169-174: LGTM: Updated test case correctly validates non-hexadecimal stringsThe test case has been properly updated to use 'ghij' which cannot be parsed as a hexadecimal string, ensuring validation correctly rejects invalid hex values for the AssetPrice field. This aligns with the PR objective to allow hexadecimal strings while maintaining proper validation.
176-181: Good addition: Adding type validation for array valuesThis new test case is a valuable addition that ensures the validation logic properly rejects array values for the AssetPrice field, strengthening the type safety of the OracleSet transaction model. This complements the existing validations and provides more comprehensive test coverage.
| it(`throws w/ invalid AssetPrice of PriceDataSeries`, function () { | ||
| tx.PriceDataSeries[0].PriceData.AssetPrice = '1234' | ||
| tx.PriceDataSeries[0].PriceData.AssetPrice = 'ghij' // value cannot be parsed as hexadecimal number | ||
| const errorMessage = 'OracleSet: invalid field AssetPrice' | ||
| assert.throws(() => validateOracleSet(tx), ValidationError, errorMessage) | ||
| assert.throws(() => validate(tx), ValidationError, errorMessage) | ||
| }) | ||
|
|
||
| it(`throws w/ invalid AssetPrice type in PriceDataSeries`, function () { | ||
| tx.PriceDataSeries[0].PriceData.AssetPrice = ['sample', 'invalid', 'type'] |
There was a problem hiding this comment.
Can you also add a positive test case with AssetPrice using a valid string value?
| @@ -10,6 +10,8 @@ import { | |||
| validateRequiredField, | |||
| } from './common' | |||
|
|
|||
| import {HEX_REGEX} from 'ripple-binary-codec/dist/types/uint-64' | |||
There was a problem hiding this comment.
Isn't there a HEX_REGEX somewhere in xrpl.js? I don't like the idea of importing from ripple-binary-codec like this
There was a problem hiding this comment.
Yes, it's actually wrapped in a utility function called isHex() in packages/xrpl/src/models/utils/index.ts
There was a problem hiding this comment.
These are the two other definitions of HEX_REGEX outside of ripple-binary-codec package. Both of them do not fit the bill. Perhaps a suitable regex is defined with a slightly different name?
There was a problem hiding this comment.
@khancode That is the second regex in my above message, it does not satisfy our requirements.
There was a problem hiding this comment.
How do they not fit it? What are the requirements?
There was a problem hiding this comment.
If the isHex helper function doesn't work here, then it should be modified. We shouldn't have 5 different ways of checking if a string is hex.
There was a problem hiding this comment.
@khancode The HEX_REGEX in this file accepts any non-zero length string comprising of the specified characters. However, the std::uint64_t cannot accommodate arbitrarily large strings. This is due to the range restrictions of the data type.
@mvadari There are multiple HEX_REGEX values in the code base because rippled has different uses of the hexa-decimal representation. Reconciling all the existing HEX_REGEX values is not possible. For instance, Currency representation uses this regex
I am happy to rename this variable as HEX_REGEX_UINT64, if that helps with readability.
There was a problem hiding this comment.
Why not use isHex and do a separate length check, then?
There was a problem hiding this comment.
I agree with @mvadari . You could also update isHex to take in a length parameter.
| @@ -146,7 +148,7 @@ export function validateOracleSet(tx: Record<string, unknown>): void { | |||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -- we are validating the type | |||
| 'AssetPrice' in priceData.PriceData && | |||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -- we are validating the type | |||
| !isNumber(priceData.PriceData.AssetPrice) | |||
| !(isNumber(priceData.PriceData.AssetPrice) || (typeof priceData.PriceData.AssetPrice === 'string' && HEX_REGEX.test(priceData.PriceData.AssetPrice))) | |||
There was a problem hiding this comment.
This regex test should potentially be included in isNumber
There was a problem hiding this comment.
There are many unsigned int values which aren't std::uint64_t amongst sfABCD values. Such values do not accept a str representation. There are 37 usages of this method in packages/xrpl/. Apart from a case-by-case decision, I don't know of a sweeping way to disambiguate this usage.
There was a problem hiding this comment.
Is that just true of the library or does rippled also not accept hex for smaller int values?
There was a problem hiding this comment.
Based on a perfunctory glance at the code base, rippled does not have any constructors which convert a hexa-decimal string into a std::uint<>_t type. All of them accept a std::uint<>_t type or a byte-string.
c++ code does not need a hexadecimal string -> UINT conversion because the std::uint<>_t types sufficiently represent the values. My understanding is that (non-c++) client libraries rely on strings to preserve the precision of values.
Co-authored-by: Omar Khan <[email protected]>
| ) { | ||
| throw new ValidationError('OracleSet: invalid field AssetPrice') | ||
| } | ||
| /* eslint-enable @typescript-eslint/no-unsafe-member-access */ |
There was a problem hiding this comment.
This helps me address typescript linter errors. Without these directives, the compiler does not understand that priceData array members have the AssetPrice field.
| @@ -173,4 +185,5 @@ export function validateOracleSet(tx: Record<string, unknown>): void { | |||
| } | |||
| return true | |||
| }) | |||
| /* eslint-enable max-statements, max-lines-per-function */ | |||
There was a problem hiding this comment.
I'm re-enabling the directive after this method. I'd like this directive to be enforced after this function definition.
Co-authored-by: Omar Khan <[email protected]>
High Level Overview of Change
Please refer to this issue: #2911
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan