XRPLF / xrpl-py

A Python library to interact with the XRP Ledger (XRPL) blockchain
ISC License
151 stars 86 forks source link

Support for PriceOracles #701

Closed ckeshava closed 6 months ago

ckeshava commented 6 months ago

High Level Overview of Change

This PR adds client library support for Price Oracle amendment. Please refer to the spec: https://github.com/XRPLF/XRPL-Standards/discussions/129 CPP Implementation: https://github.com/XRPLF/rippled/pull/4789

Context of Change

Support for transactions OracleSet and OracleDelete has been added. GetAggregatePrice request is included in the client library.

Type of Change

Did you update CHANGELOG.md?

Test Plan

Unit, Integration and snippet tests have been included. There is some overlap between integration and snippet tests, perhaps they could be streamlined better.

ckeshava commented 6 months ago

@khancode Since I've already written the snippet tests, I feel we can let it stay in the codebase. I'll be mindful about writing them for the future features.

If you feel otherwise, I can remove them too. I don't strongly about it.

mvadari commented 6 months ago

@khancode Since I've already written the snippet tests, I feel we can let it stay in the codebase. I'll be mindful about writing them for the future features.

If you feel otherwise, I can remove them too. I don't strongly about it.

The snippets would still need to be reviewed, since they're intended to be public-facing - they're not tests, they're sample code. If they were just tests I'd agree, but since it's sample code, I do think it'll cause some clutter for devs.

khancode commented 6 months ago

@khancode Since I've already written the snippet tests, I feel we can let it stay in the codebase. I'll be mindful about writing them for the future features. If you feel otherwise, I can remove them too. I don't strongly about it.

The snippets would still need to be reviewed, since they're intended to be public-facing - they're not tests, they're sample code. If they were just tests I'd agree, but since it's sample code, I do think it'll cause some clutter for devs.

I agree with @mvadari . I'd rather have them removed.

ckeshava commented 6 months ago

alrighty

ckeshava commented 6 months ago

c'mon guys, please approve the PR soon! we've had enough back and forth :)

ckeshava commented 6 months ago

I have resolved those comments which I could address -- If any of the reviewers prefer to resolve the comments yourself, please let me know.

ckeshava commented 6 months ago

thanks to all the reviewers for patiently working through this PR.

If there are any other comments, please let me know. I'll create a separate PR.