-
Notifications
You must be signed in to change notification settings - Fork 512
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?
Conversation
WalkthroughThe changes include updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PriceData
participant Transaction
User->>PriceData: Submit AssetPrice
PriceData->>Transaction: Validate AssetPrice (number | bigint | string)
Transaction-->>User: Confirmation of valid AssetPrice
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/xrpl/test/integration/transactions/oracleSet.test.ts (2)
46-53
: LGTM: New PriceData entry added correctlyThe addition of a new PriceData entry for 'XRP/INR' with the maximum AssetPrice value aligns well with the PR objective. The use of BigInt ensures correct handling of the large value.
Consider adding a comment explaining the purpose of this maximum value test case for better clarity:
PriceDataSeries: [ { PriceData: { BaseAsset: 'XRP', QuoteAsset: 'USD', AssetPrice: 740, Scale: 3, }, }, + // Test case for maximum admissible AssetPrice value { PriceData: { BaseAsset: 'XRP', QuoteAsset: 'INR', AssetPrice: BigInt(MAX_64_BIT_UNSIGNED_INT), Scale: 3, }, }, ],
84-88
: LGTM: Assertion for large AssetPrice serialization addedThe new assertion correctly validates the serialization of the maximum 64-bit unsigned integer AssetPrice. This is crucial for ensuring that large AssetPrice values are handled and serialized correctly.
Consider adding a comment explaining the expected hexadecimal value for better clarity:
+ // 'ffffffffffffffff' is the hexadecimal representation of 2^64 - 1 assert.equal( oracle.PriceDataSeries[1].PriceData.AssetPrice, 'ffffffffffffffff', )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/xrpl/src/models/common/index.ts (1 hunks)
- packages/xrpl/src/models/transactions/common.ts (1 hunks)
- packages/xrpl/test/integration/transactions/oracleSet.test.ts (3 hunks)
🔇 Additional comments (6)
packages/xrpl/test/integration/transactions/oracleSet.test.ts (2)
16-19
: LGTM: Well-defined constant with clear explanationThe addition of
MAX_64_BIT_UNSIGNED_INT
is a good practice. The constant value is correct, and the comment clearly explains why a string is used instead of a number type, which is crucial for handling large numeric values in JavaScript.
77-77
: LGTM: Assertion updated correctlyThe assertion for
PriceDataSeries.length
has been correctly updated to 2, reflecting the addition of the new PriceData entry. This ensures that both entries are present in the Oracle object after the transaction.packages/xrpl/src/models/common/index.ts (1)
189-189
: LGTM! Consider verifying usage across the codebase.The addition of
bigint
to theAssetPrice
type is a good improvement. It allows for representing larger numerical values, which aligns with the PR objective of validating the highest admissibleAssetPrice
value in the OracleSet transaction.To ensure this change doesn't introduce any unexpected behavior, please run the following script to check for any other occurrences of
AssetPrice
in the codebase:This will help identify any areas that might need updates due to this change.
packages/xrpl/src/models/transactions/common.ts (3)
84-84
: Summary of review findingsThe change to include
bigint
in theisNumber
function is a positive addition that aligns with the PR objective of validating highAssetPrice
values in OracleSet transactions. However, this change has potential impacts on various parts of the codebase that should be addressed:
- Verify the usage of
isNumber
throughout the codebase to ensure compatibility with the newbigint
inclusion.- Check and potentially update the type definitions and usages of transaction fields that use
validateOptionalField
withisNumber
.- Update the
parseAmountValue
function to handlebigint
values correctly.These steps will ensure that the change is fully integrated and doesn't cause unintended side effects in the codebase.
Please run the suggested verification scripts and make any necessary updates before merging this PR.
84-84
: Verify impact on transaction field validationsThe change in
isNumber
affects the validation of several transaction fields through thevalidateOptionalField
function. Please ensure that allowingbigint
for the following fields is intentional and doesn't cause issues elsewhere in the codebase:
Sequence
LastLedgerSequence
SourceTag
TicketSequence
NetworkID
Consider running the following script to check the types and usages of these fields:
#!/bin/bash # Description: Check types and usages of affected transaction fields # Search for type definitions of affected fields echo "Searching for type definitions of affected fields:" rg --type typescript 'Sequence|LastLedgerSequence|SourceTag|TicketSequence|NetworkID' -C 3 --glob '**/*.ts' # Search for usages of these fields echo "\nSearching for usages of affected fields:" rg --type typescript --type javascript 'Sequence|LastLedgerSequence|SourceTag|TicketSequence|NetworkID' -C 3If necessary, update the type definitions of these fields to include
bigint
or add appropriate type checks in their usage.
84-84
: LGTM! Consider verifying the impact on the codebase.The change to include
bigint
in theisNumber
function is a good addition, especially for handling very large numbers likeAssetPrice
in OracleSet transactions. This change is backwards compatible and aligns with the PR objective.To ensure this change doesn't have unintended consequences, please run the following script to check for any other uses of
isNumber
in the codebase:This will help identify any places where the new
bigint
inclusion might affect existing code.✅ Verification successful
Change Verified!
The update to include
bigint
in theisNumber
function has been reviewed across the codebase, and no unintended consequences were found. The function's usage in various validation contexts remains consistent and aligns with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of isNumber function calls in the codebase # Search for isNumber function calls echo "Searching for isNumber function calls:" rg --type typescript --type javascript 'isNumber\(' -C 3 # Search for type assertions using isNumber echo "\nSearching for type assertions using isNumber:" rg --type typescript --type javascript 'is number' -C 3Length of output: 496
Script:
#!/bin/bash # Description: Find all occurrences of isNumber function calls and type assertions in .ts and .js files # Search for isNumber function calls in TypeScript and JavaScript files echo "Searching for isNumber function calls in .ts and .js files:" rg 'isNumber\(' --glob '*.ts' --glob '*.js' -C 3 # Search for type assertions using isNumber in TypeScript and JavaScript files echo "\nSearching for type assertions using isNumber in .ts and .js files:" rg 'is number' --glob '*.ts' --glob '*.js' -C 3Length of output: 10208
@@ -81,7 +81,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' |
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 handle bigint
With the inclusion of bigint
in the isNumber
function, it's important to ensure that the parseAmountValue
function can handle bigint
values correctly. Currently, it uses parseFloat
, which may not be suitable for bigint
values.
Consider updating the parseAmountValue
function to handle bigint
values. Here's a suggested implementation:
export function parseAmountValue(amount: unknown): number {
if (!isAmount(amount)) {
return NaN;
}
if (typeof amount === 'string') {
return parseFloat(amount);
}
if (typeof amount.value === 'bigint') {
return Number(amount.value);
}
return parseFloat(amount.value);
}
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.
- I'm not aware of cases where
OracleSet
transaction'sAssetPrice
field is used as anAmount
field. - Even if that is the case, type-coercion of
bigint
intonumber
type causes loss of precision. I'd prefer to do it the other way around. - The precision supported by the
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
to number
. It's important to preserve data integrity by avoiding such type coercion. Given that AssetPrice
in OracleSet
is not used as an Amount
field, and considering the precision requirements for IssuedCurrencies
and XRP
amounts, further investigation into parseAmountValue
is indeed warranted before making any changes.
(🐰🤔)
✏️ Learnings added
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2795
File: packages/xrpl/src/models/transactions/common.ts:84-84
Timestamp: 2024-10-03T02:06:25.875Z
Learning: When dealing with `bigint` values, avoid converting them to `number` to prevent loss of precision.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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 return bigint
and adjusted the validation logic accordingly. The addition of comprehensive unit tests for parseAmountValue
enhances the robustness of the code. This effectively addresses the previous concern about loss of precision when converting bigint
to number
. Thank you for incorporating these changes!
(🐰😊)
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Show the diff for the commit d53dd5eb6726177d218f3da11bc9eb266b4c0085
git show d53dd5eb6726177d218f3da11bc9eb266b4c0085
Length of output: 5244
🧠 Learnings used
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2795
File: packages/xrpl/src/models/transactions/common.ts:84-84
Timestamp: 2024-10-03T02:06:26.057Z
Learning: When dealing with `bigint` values, avoid converting them to `number` to prevent loss of precision.
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
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2795
File: packages/xrpl/src/models/transactions/common.ts:84-84
Timestamp: 2024-10-03T02:06:26.057Z
Learning: When dealing with `bigint` values, avoid converting them to `number` to prevent loss of precision.
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.
LGTM
I think it's worth to mention this fix in HISTORY.md so people can update accordingly and avoid this issue
Thanks for the quick approval @pdp2121 If you feel strongly about it, I can include that change too |
It's a change in the library, it should be mentioned. It matters for people testing on Devnet. |
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.
Why are there now keypairs changes in this PR?
…int, instead of number type. Include unit tests for comprehensive coverage of this 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.
Caution
Inline review comments failed to post
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/secret-numbers/test/api.test.ts (1)
Line range hint
1-94
: Consider adding explanatory comments for the changesThe changes in this file are focused on updating expected values rather than modifying test logic. To improve future maintainability and provide context for these changes:
Consider adding comments at the beginning of each modified test suite explaining the reason for the updates. For example:
// The following expected values were updated on [DATE] due to [REASON] // (e.g., changes in the underlying cryptographic algorithms, XRPL updates, etc.)If these changes are related to a specific issue or pull request, consider adding a reference to it in the comments.
This additional context will be valuable for future developers working on this codebase and will help in understanding the evolution of the account generation process.
packages/xrpl/src/models/transactions/NFTokenAcceptOffer.ts (1)
74-80
: Improved error handling for NFTokenBrokerFee parsing.The changes enhance the robustness of the
validateNFTokenBrokerFee
function by usingparseAmountValue
and providing a more descriptive error message. This is a good improvement.Consider adding the actual value that failed to parse in the error message for easier debugging:
throw new ValidationError( - 'NFTokenAcceptOffer: invalid NFTokenBrokerFee. BigInt constructor could not parse NFTokenBrokerFee', + `NFTokenAcceptOffer: invalid NFTokenBrokerFee. BigInt constructor could not parse NFTokenBrokerFee: ${tx.NFTokenBrokerFee}`, )packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (1)
110-119
: Improved error handling for Amount parsingThe changes enhance the robustness of the
validateNFTokenBuyOfferCases
function by introducing better error handling for theAmount
field parsing. This improvement provides clearer feedback in case of invalid input and maintains the existing check for non-positive amounts.Consider extracting the error message to a constant for easier maintenance and potential reuse:
const INVALID_AMOUNT_ERROR = 'NFTokenCreateOffer: Invalid Amount, Amount field could not be parsed by BigInt constructor' // Then in the catch block: throw new ValidationError(INVALID_AMOUNT_ERROR)This minor refactoring would improve code maintainability and consistency.
packages/xrpl/test/models/NFTokenCreateOffer.test.ts (1)
14-55
: LGTM: Comprehensive test suite forparseAmountValue
.The new test suite for
parseAmountValue
is well-structured and covers important scenarios:
- Large amount values (including the upper bound for XRP and issued currencies)
- Non-positive amount values
- Invalid amount values
The tests are thorough and include appropriate assertions and error checks.
Consider adding a test case for the maximum XRP amount (100 billion XRP) to ensure it's handled correctly:
it('validates maximum XRP amount', function () { assert.equal( parseAmountValue('100000000000000000'), BigInt('100000000000000000'), ) })packages/xrpl/src/models/transactions/common.ts (1)
359-371
: LGTM: ImprovedparseAmountValue
function withbigint
supportThe changes to
parseAmountValue
are well-implemented:
- Returning
bigint
instead ofnumber
preserves precision for large values.- Throwing a
ValidationError
for invalid inputs improves error handling.- Using
BigInt
constructor for parsing maintains consistency and precision.Consider wrapping the
BigInt
calls in a try-catch block to handle potentialSyntaxError
s and convert them toValidationError
s for consistency. This would make the error handling more robust and align with the function's documented behavior. Here's a suggested implementation:export function parseAmountValue(amount: unknown): bigint { if (!isAmount(amount)) { throw new ValidationError( 'parseAmountValue: Specified input Amount is invalid', ) } try { if (typeof amount === 'string') { return BigInt(amount) } return BigInt(amount.value) } catch (error) { if (error instanceof SyntaxError) { throw new ValidationError('parseAmountValue: Amount cannot be parsed as BigInt') } throw error } }
🛑 Comments failed to post (1)
packages/ripple-keypairs/test/api.test.ts (1)
22-26:
⚠️ Potential issueVerify the seed prefix check for ed25519
The test case has been updated to use ed25519, which is correct. However, there might be an issue with the seed prefix check:
- The current check
expect(seed.startsWith('s')).toBeTruthy()
is too broad.- Typically, ed25519 seeds start with 'sEd', not just 's'.
Consider updating the prefix check to be more specific:
expect(seed.startsWith('sEd')).toBeTruthy()This will ensure that the generated seed is specifically for the ed25519 algorithm.
Sorry, my bad. I messed up the git history. I rectified it with the relevant cherry-picked commits. |
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/xrpl/test/models/NFTokenCreateOffer.test.ts (2)
14-55
: LGTM: Comprehensive test suite forparseAmountValue
.The new test suite for
parseAmountValue
is well-structured and covers important scenarios:
- Large amount values (both XRP and issued currencies)
- Non-positive amount values
- Invalid amount values
The tests are correctly implemented and use appropriate assertions. The use of both XRP and issued currency amounts provides good coverage.
Consider adding a test case for the maximum XRP amount (100 billion XRP) to ensure it's handled correctly:
it('validates maximum XRP amount', function () { assert.equal( parseAmountValue('100000000000000000'), BigInt('100000000000000000'), ) })
Line range hint
1-301
: Summary: Improved test coverage withparseAmountValue
tests.This change introduces a new test suite for the
parseAmountValue
function while preserving the existing tests forNFTokenCreateOffer
. The new tests cover important scenarios for amount parsing, including edge cases and error conditions. This addition enhances the overall test coverage of the library without disrupting the existing functionality.Consider organizing the test file into separate describe blocks for
parseAmountValue
andNFTokenCreateOffer
to improve readability and maintainability as the test suite grows.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- packages/xrpl/HISTORY.md (1 hunks)
- packages/xrpl/src/models/common/index.ts (1 hunks)
- packages/xrpl/src/models/transactions/NFTokenAcceptOffer.ts (1 hunks)
- packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (1 hunks)
- packages/xrpl/src/models/transactions/common.ts (2 hunks)
- packages/xrpl/test/integration/transactions/oracleSet.test.ts (3 hunks)
- packages/xrpl/test/models/NFTokenCreateOffer.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/xrpl/HISTORY.md
- packages/xrpl/src/models/common/index.ts
- packages/xrpl/src/models/transactions/NFTokenAcceptOffer.ts
- packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts
- packages/xrpl/src/models/transactions/common.ts
🔇 Additional comments (7)
packages/xrpl/test/integration/transactions/oracleSet.test.ts (5)
16-19
: LGTM: Well-defined constant with clear commentsThe introduction of
MAX_64_BIT_UNSIGNED_INT
is a good practice. The constant value is correct, and the use of a string type is well-justified. The accompanying comments provide clear explanation, which enhances code readability and maintainability.
46-53
: LGTM: Enhanced test coverage with edge caseThe addition of a new PriceData entry for XRP/INR with the maximum 64-bit unsigned integer value as AssetPrice is an excellent enhancement to the test coverage. It ensures that the system can handle edge cases with large asset prices correctly. The use of BigInt for the AssetPrice value is appropriate for maintaining precision.
77-77
: LGTM: Updated assertion matches new test caseThe assertion update correctly reflects the addition of a new PriceData entry to the PriceDataSeries. This change ensures that the test accurately verifies the presence of both PriceData entries in the Oracle object.
84-88
: LGTM: Crucial assertion for large AssetPrice serializationThe addition of this assertion is crucial for verifying the correct serialization of large AssetPrice values. The expected hexadecimal value 'ffffffffffffffff' correctly represents the maximum 64-bit unsigned integer. This test significantly enhances confidence in the system's ability to handle and serialize large asset prices accurately.
Line range hint
1-92
: Overall assessment: Excellent enhancements to test coverageThe changes in this file significantly improve the test coverage for the OracleSet transaction, particularly for handling and serializing large AssetPrice values. These modifications align perfectly with the PR objectives and provide crucial validation for edge cases. The code is well-structured, clearly commented, and the new assertions are appropriate and valuable.
However, considering the comments in the PR discussion:
To address the concern raised by mvadari about documenting this change in the HISTORY.md file, I suggest running the following command to check if there's any mention of this test enhancement:
If the command doesn't return any results, consider adding a brief note about this test enhancement in the HISTORY.md file, even though it doesn't directly affect library users. This documentation would be valuable for developers working with the Devnet, as mentioned in the PR discussion.
packages/xrpl/test/models/NFTokenCreateOffer.test.ts (2)
3-9
: LGTM: Import statements updated correctly.The new imports for
IssuedCurrencyAmount
andparseAmountValue
are correctly added and necessary for the new test suite.
Line range hint
56-301
: Existing tests forNFTokenCreateOffer
preserved.The existing test suite for
NFTokenCreateOffer
remains unchanged, which is good. These tests continue to provide coverage for various scenarios related to theNFTokenCreateOffer
transaction type.
Validate highest admissible
AssetPrice
value in the OracleSet transaction.Type of Change
Did you update HISTORY.md?