-
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
Open
ckeshava
wants to merge
3
commits into
XRPLF:main
Choose a base branch
from
ckeshava:poracle_test
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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