-
Notifications
You must be signed in to change notification settings - Fork 524
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
Integration test for OracleSet txn #2795
base: main
Are you sure you want to change the base?
Changes from all commits
b6ba24e
cd6c998
d53dd5e
b970c4c
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 |
---|---|---|
|
@@ -90,7 +90,7 @@ export function isString(str: unknown): str is string { | |
* @returns Whether the number is properly formed. | ||
*/ | ||
export function isNumber(num: unknown): num is number { | ||
return typeof num === 'number' | ||
return typeof num === 'number' || typeof num === 'bigint' | ||
} | ||
|
||
/** | ||
|
@@ -400,15 +400,19 @@ export function validateBaseTransaction(common: Record<string, unknown>): void { | |
* | ||
* @param amount - An Amount to parse for its value. | ||
* @returns The parsed amount value, or NaN if the amount count not be parsed. | ||
* @throws ValidationError, if the input Amount is invalid | ||
* @throws SyntaxError, if Amount cannot be parsed by BigInt constructor | ||
*/ | ||
export function parseAmountValue(amount: unknown): number { | ||
export function parseAmountValue(amount: unknown): bigint { | ||
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. Couldn't the amount be a floating point number? In that case, it would be 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. Thanks for pointing this out. I missed the fact that I wrote this PR with the perspective of However, you are correct. We cannot make this assumption for the rest of |
||
if (!isAmount(amount)) { | ||
return NaN | ||
throw new ValidationError( | ||
'parseAmountValue: Specified input Amount is invalid', | ||
) | ||
} | ||
if (typeof amount === 'string') { | ||
return parseFloat(amount) | ||
return BigInt(amount) | ||
} | ||
return parseFloat(amount.value) | ||
return BigInt(amount.value) | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,10 @@ import { testTransaction } from '../utils' | |
|
||
// how long before each test case times out | ||
const TIMEOUT = 20000 | ||
// Upper bound admissible value for AssetPrice field | ||
// large numeric values necessarily have to use str type in Javascript | ||
// number type uses double-precision floating point representation, hence represents a smaller range of values | ||
const MAX_64_BIT_UNSIGNED_INT = '18446744073709551615' | ||
|
||
describe('OracleSet', function () { | ||
let testContext: XrplIntegrationTestContext | ||
|
@@ -39,6 +43,14 @@ describe('OracleSet', function () { | |
Scale: 3, | ||
}, | ||
}, | ||
{ | ||
PriceData: { | ||
BaseAsset: 'XRP', | ||
QuoteAsset: 'INR', | ||
AssetPrice: BigInt(MAX_64_BIT_UNSIGNED_INT), | ||
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. Does this not work out of the box if you specify the number as a string? |
||
Scale: 3, | ||
}, | ||
}, | ||
], | ||
Provider: stringToHex('chainlink'), | ||
URI: '6469645F6578616D706C65', | ||
|
@@ -62,12 +74,18 @@ describe('OracleSet', function () { | |
assert.equal(oracle.Owner, testContext.wallet.classicAddress) | ||
assert.equal(oracle.AssetClass, tx.AssetClass) | ||
assert.equal(oracle.Provider, tx.Provider) | ||
assert.equal(oracle.PriceDataSeries.length, 1) | ||
assert.equal(oracle.PriceDataSeries.length, 2) | ||
assert.equal(oracle.PriceDataSeries[0].PriceData.BaseAsset, 'XRP') | ||
assert.equal(oracle.PriceDataSeries[0].PriceData.QuoteAsset, 'USD') | ||
assert.equal(oracle.PriceDataSeries[0].PriceData.AssetPrice, '2e4') | ||
assert.equal(oracle.PriceDataSeries[0].PriceData.Scale, 3) | ||
assert.equal(oracle.Flags, 0) | ||
|
||
// validate the serialization of large AssetPrice values | ||
assert.equal( | ||
oracle.PriceDataSeries[1].PriceData.AssetPrice, | ||
'ffffffffffffffff', | ||
) | ||
}, | ||
TIMEOUT, | ||
) | ||
|
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
Consider updating
parseAmountValue
to handlebigint
With the inclusion of
bigint
in theisNumber
function, it's important to ensure that theparseAmountValue
function can handlebigint
values correctly. Currently, it usesparseFloat
, which may not be suitable forbigint
values.Consider updating the
parseAmountValue
function to handlebigint
values. Here's a suggested implementation:This change ensures that
bigint
values are correctly converted to numbers, while maintaining the existing behavior for other types.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.
OracleSet
transaction'sAssetPrice
field is used as anAmount
field.bigint
intonumber
type causes loss of precision. I'd prefer to do it the other way around.Amount
field is not as large as that ofbigint
.IssuedCurrencies
use54
bits whereasXRP
Amounts need57
bits, to represent their highest values. The former case needs further investigation, appropos theparseAmountValue
function.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.
@ckeshava, thank you for the clarification. You make a valid point regarding the potential loss of precision when converting
bigint
tonumber
. It's important to preserve data integrity by avoiding such type coercion. Given thatAssetPrice
inOracleSet
is not used as anAmount
field, and considering the precision requirements forIssuedCurrencies
andXRP
amounts, further investigation intoparseAmountValue
is indeed warranted before making any changes.(🐰🤔)
✏️ Learnings added
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.
Continuing on the last bullet point, I incorporated the suggestions in this commit d53dd5e
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.
@ckeshava, great work! I see that you've updated the
parseAmountValue
function to returnbigint
and adjusted the validation logic accordingly. The addition of comprehensive unit tests forparseAmountValue
enhances the robustness of the code. This effectively addresses the previous concern about loss of precision when convertingbigint
tonumber
. Thank you for incorporating these changes!(🐰😊)
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 5244
🧠 Learnings used
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
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.
thanks rabbit :)
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.
@ckeshava, you're welcome! Happy to help!
(🐰🎉)
🧠 Learnings used