Implement book_changes RPC #300

godexsoft · 2022-09-09T00:14:02Z

Fixes #284

Ported book_changes rpc call from rippled.
Implemented book_changes support in test.py

To try you can execute something like this: ./test.py --ip 127.0.0.1 --port 51233 --ledger 74256240 book_changes
Example output:

{
    "result": {
        "changes": [
            {
                "close": "9240",
                "currency_a": "XRP_drops",
                "currency_b": "raq7pGaYrLZRa88v6Py9V5oWMYEqPYr8Tz/5377697373546563680000000000000000000000",
                "high": "9240",
                "low": "9199.998196515558",
                "open": "9200",
                "volume_a": "96285669",
                "volume_b": "10444.094543578"
            },
            {
                "close": "379325.64751315",
                "currency_a": "XRP_drops",
                "currency_b": "rsoLo2S1kiGeCcn6hCUXVrCpGMWLrRrLZz/534F4C4F00000000000000000000000000000000",
                "high": "379325.64751315",
                "low": "379325.64751315",
                "open": "379325.64751315",
                "volume_a": "113941838",
                "volume_b": "300.38"
            }
        ],
        "ledger_hash": "F4FC90CC918C4E4EC33A015CE39603D83CBB44A7E97EE06DAA5CFAC8B835CD1F",
        "ledger_index": 74256240,
        "ledger_time": 715968871,
        "type": "bookChanges",
        "validated": true
    },
    "status": "success",
    "type": "response",
    "warnings": [
        {
            "id": 2001,
            "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request"
        }
    ]
}

Please note: despite the warning suggesting to set ledger_index to current - this will not work as this call is not forwarding to rippled but rather uses clio's local database.

There is also currently no unit-tests for this. We should add it at some point when we have refactored for testability.

* Port book_changes RPC call from rippled * Refactor for readability and modern cpp

natenichols

Only requesting changes because I'd like to look into a way to do this without using const_cast.

If it turns out that we have to do that, I won't die on this hill, but I'd prefer it if we found a different way.

natenichols · 2022-09-09T14:33:55Z

src/rpc/handlers/BookChanges.cpp

+
+class BookChangesHandler
+{
+    std::reference_wrapper<Context const> context_;


If you've deleted copy assignment, you can probably just use Context const&

I'd rather not have const/ref members ever, no matter what

Why do we want to avoid ref members?

@cjcobb23 for me personally it's enough that it's a core guideline. I understand it's ok if you know what you are doing but i'd try to avoid it most of the time. using reference_wrapper is totally fine - it's just a pointer inside and basically a zero-cost abstraction.

natenichols · 2022-09-09T14:35:22Z

src/rpc/handlers/BookChanges.cpp

+    inline void
+    reset() noexcept
+    {
+        tally_.clear();


Why do we need reset() .

If the class is instantiated each time we handle, couldn't we just put this functionality in the constructor.

Just because in my code i don't reuse it does not mean one could not reuse it some day later. So this is for correctness sake and it also does nothing on the first call to it as far as i understand

natenichols · 2022-09-09T14:37:28Z

src/rpc/handlers/BookChanges.cpp

+class BookChangesHandler
+{
+    std::reference_wrapper<Context const> context_;
+    std::map<std::string, BookChange> tally_;


These aren't initialized until we call reset.

can we just initialize these here with = {}. I don't think it will change anything, I just prefer explicitly instantiating member variables.

Why would that be?
Just because i'm not explicitly setting a value in initialization list nor in the members section does not mean default constructor will not be called for a user defined type.
See https://godbolt.org/z/ovnW6erx3

Having that said we can absolutely do the = {} thingy for good measure :)

Yea, these are exactly the same functionally.

I prefer explicitly calling the default constructor w/ = {} or Foo foo{} because its more explicit, but that's just me.

Up to you here.

natenichols · 2022-09-09T14:38:59Z

src/rpc/handlers/BookChanges.cpp

+            !node.isFieldPresent(sfPreviousFields))
+            return;
+
+        auto& finalFields = (const_cast<STObject&>(node))


Vetoing const_cast.

Why do we need const cast here. There's got to be a way to do this without requiring this. Look in STObject.h in rippled to see if there's a way that doesn't require this.

Oh god i did not even notice it tbh. - this was copied as is from the original implementation in rippled. I'll definitely try to find a way around this - thanks

How did const_cast get into the rippled implementation. Has this been merged yet.

Yea it was. I'll submit a patch to rippled later - no worries.

* Port book_changes RPC call from rippled * Refactor for readability and modern cpp

godexsoft added 8 commits September 9, 2022 01:02

Implement book_changes RPC

d94c14d

* Port book_changes RPC call from rippled * Refactor for readability and modern cpp

[FOLD] Fix clang-format

8df5aa7

[FOLD] Move transaction fetching code and use context_

c330f0e

[FOLD] Fix clang-format

294a7cb

[FOLD] Remove tmp log line and make things const

98537dd

[FOLD] Mark shouldCancelOffer const

ca71990

[FOLD] Make constructor explicit

12991bb

[FOLD] Cleanup a bit

72d59f4

natenichols suggested changes Sep 9, 2022

View reviewed changes

[FOLD] Fix const_cast and revert stuff that not all compilers can do yet

fc3c147

godexsoft requested a review from natenichols September 9, 2022 16:23

natenichols approved these changes Sep 9, 2022

View reviewed changes

godexsoft merged commit 0b454a2 into XRPLF:develop Sep 9, 2022

godexsoft mentioned this pull request Sep 16, 2022

Implement book_changes subscription #312

Closed

manojsdoshi pushed a commit to manojsdoshi/clio that referenced this pull request Nov 2, 2022

Implement book_changes RPC (XRPLF#300)

1701d2c

* Port book_changes RPC call from rippled * Refactor for readability and modern cpp

manojsdoshi pushed a commit to manojsdoshi/clio that referenced this pull request Nov 2, 2022

Implement book_changes RPC (XRPLF#300)

43923e0

* Port book_changes RPC call from rippled * Refactor for readability and modern cpp

manojsdoshi pushed a commit to manojsdoshi/clio that referenced this pull request Nov 2, 2022

Implement book_changes RPC (XRPLF#300)

5d76feb

* Port book_changes RPC call from rippled * Refactor for readability and modern cpp

manojsdoshi mentioned this pull request Nov 2, 2022

Proposed 1.0.3-rc1 #381

Merged

Implement book_changes RPC #300

Implement book_changes RPC #300

Uh oh!

Conversation

godexsoft commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natenichols left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natenichols Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

godexsoft commented Sep 9, 2022 •

edited

Loading

natenichols Sep 9, 2022 •

edited

Loading