Skip to content
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: add OracleSet and OracleDelete transactions #983

Merged
merged 24 commits into from
May 24, 2024
Merged

Conversation

pdp2121
Copy link
Collaborator

@pdp2121 pdp2121 commented May 16, 2024

High Level Overview of Change

Title said it all.

Context of Change

PriceOracle amendment:
XRPLF/XRPL-Standards#129

Type of Change

  • New feature (non-breaking change which adds functionality)

Before / After

OracleSet transaction page:

Screenshot 2024-05-23 at 9 20 35 PM

OracleDelete transaction page:
Screenshot 2024-05-23 at 7 19 06 PM

Table Detail view:

Screenshot 2024-05-23 at 9 35 05 PM

@pdp2121 pdp2121 requested review from mvadari and khancode May 16, 2024 15:12
package.json Outdated Show resolved Hide resolved
return (
<div className="oracle-delete">
{
// OracleDocumentID could be zero.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a TODO for a future PR, but do you think it'd be useful to have info here about the oracle that's being deleted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People seem to ask about way we could filter tx by OracleDocumentID so I deemed that showing the ID might be important

@pdp2121 pdp2121 requested a review from mvadari May 16, 2024 17:37
@pdp2121 pdp2121 requested a review from mvadari May 22, 2024 14:55
<span className="case-sensitive">{priceDataObj.assetPrice}</span>
</>
)}
{index < tx.priceDataSeries.length - 1 && ', '}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this

@pdp2121 pdp2121 requested a review from mvadari May 22, 2024 22:15
Copy link
Collaborator

@khancode khancode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@pdp2121 pdp2121 requested a review from mvadari May 24, 2024 14:50
@@ -0,0 +1,12 @@
import { createSimpleWrapperFactory, expectSimpleRowText } from '../../test'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests that are testing React components should use .test.tsx as their file ending.


// Convert scaled price (assetPrice) to original price using formula:
// originalPrice = assetPrice / 10**scale
// More details: https://github.com/XRPLF/XRPL-Standards/discussions/129
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should link to the published standard, not the discussion: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-47d-PriceOracles


describe('convertScaledPrice', () => {
it('should convert scaled price to original price correctly using scale factor', () => {
// scaled price: 5083
Copy link
Collaborator

@mvadari mvadari May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want this to be easier to read (and avoid the need for a comment), you can convert the numbers to hex in the test itself instead of just using the hex

@pdp2121 pdp2121 merged commit 33ca4f4 into staging May 24, 2024
4 checks passed
@pdp2121 pdp2121 deleted the add-oracle-set-tx branch May 24, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants