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

Add RPC for server_definitions to rippled #4703

Merged
merged 37 commits into from Oct 18, 2023

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented Sep 13, 2023

High Level Overview of Change

Add a new RPC / WS call: server_definitions, which returns an SDK-compatible definitions.json generated from the rippled instance currently running. This will alleviate definitions juggling, especially with side chains which may have exotic features and serialized fields. Software will be able to query server_definitions against a node from the network they want to connect to and immediately know how to speak that node's language, even if new features are added to it in the future (as long as there are no new serialized types that the software doesn't know how to serialize/deserialize).

Example:

> {"command": "server_definitions"}
< {
    "result": {
        "FIELDS": [
            [
                "Generic",
                {
                    "isSerialized": false,
                    "isSigningField": false,
                    "isVLEncoded": false,
                    "nth": 0,
                    "type": "Unknown"
                }
            ],
            [
                "Invalid",
                {
                    "isSerialized": false,
                    "isSigningField": false,
                    "isVLEncoded": false,
                    "nth": -1,
                    "type": "Unknown"
                }
            ],
            [
                "ObjectEndMarker",
                {
                    "isSerialized": false,
                    "isSigningField": true,
                    "isVLEncoded": false,
                    "nth": 1,
                    "type": "STObject"
                }
            ],
        ...

Context of Change

I took #4402 and modified it to avoid using magic_enum. Closes #3657.

Type of Change

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

Test Plan

I haven't written tests yet - I wanted to get this out first, because I'm sure there will be plenty of other issues.

src/ripple/protocol/KnownFormats.h Outdated Show resolved Hide resolved
Comment on lines 55 to 58
#pragma push_macro("XMACRO")
#undef XMACRO

#define XMACRO(STYPE) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Yuck. Consider using Boost.Describe at least.

Copy link
Collaborator Author

@mvadari mvadari Oct 2, 2023

Choose a reason for hiding this comment

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

I can't figure out how to properly use Boost.Describe - I keep getting a ton of errors. The documentation is very unclear, since the API has changed a lot in the past few versions. Even ChatGPT/Bard isn't able to provide much help.

bool
hashMatches(uint256 hash) const
{
return defsHash && *defsHash == hash;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed: you can directly compare an std::optional<T> with a T and it will do the right thing: return hash == defsHash; is enough.

Although, as I said before, it's unclear why defsHash is an std::optional or even a variable at all. You could just implement this as:

return to_string(hash) == defs[jss::value].asString()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the std::optional in 5fc2ee5, IMO it's a bit clearer and slightly more performant to have the separate variable.

src/ripple/rpc/handlers/ServerInfo.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/handlers/ServerInfo.cpp Show resolved Hide resolved
src/ripple/protocol/SField.h Outdated Show resolved Hide resolved
src/ripple/protocol/SField.h Outdated Show resolved Hide resolved
src/ripple/protocol/SField.h Outdated Show resolved Hide resolved
src/ripple/protocol/SField.h Outdated Show resolved Hide resolved
return RPC::invalid_field_error(jss::hash);
}

static const Definitions defs{};
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, but I don't know that you actually need an instance of a class for this: you may as well just have something like inline const Json::Value definitions = []() { ... }();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO things like the hashMatches method are a bit neater with a class as opposed to something inline. If you think I should refactor to remove the class though, I can.

@sublimator
Copy link
Contributor

Nice. Been wanting this for a while:
#1972

src/ripple/protocol/SField.h Show resolved Hide resolved
src/ripple/protocol/jss.h Show resolved Hide resolved
src/ripple/app/main/Main.cpp Show resolved Hide resolved
src/ripple/rpc/handlers/ServerInfo.cpp Show resolved Hide resolved
src/ripple/rpc/handlers/ServerInfo.cpp Outdated Show resolved Hide resolved
src/ripple/rpc/handlers/ServerInfo.cpp Outdated Show resolved Hide resolved
@ckeshava
Copy link
Collaborator

The PR looks nice. I've still left some suggestions, but they are my subjective opinions and shouldn't stop the PR from going into production

@mvadari
Copy link
Collaborator Author

mvadari commented Oct 18, 2023

@intelliot this is ready to merge 👍

@mvadari
Copy link
Collaborator Author

mvadari commented Oct 18, 2023

I don't have the powers to retry actions but it looks like something failed with the container setup in the dependency test.

@intelliot
Copy link
Collaborator

  1. I clicked the button to retry the CI job
  2. I've asked @nbougalis to confirm that he doesn't have any remaining blockers (i.e. comments/concerns that still must be addressed)

@intelliot intelliot modified the milestones: 2024 release, 2.0 Oct 18, 2023
@intelliot intelliot dismissed nbougalis’s stale review October 18, 2023 21:52

"If others have signed off, please use their sign off instead."

Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

@ckeshava has approved the latest code in this PR (aside from the merge commit)

@intelliot
Copy link
Collaborator

intelliot commented Oct 18, 2023

Suggested commit message:

feat(rpc): add `server_definitions` method (#4703)

Add a new RPC / WS call for `server_definitions`, which returns an
SDK-compatible `definitions.json` (binary enum definitions) generated by
the server. This enables clients/libraries to dynamically work with new
fields and features, such as ones that may become available on side
chains. Clients query `server_definitions` on a node from the network
they want to work with, and immediately know how to speak that node's
binary "language", even if new features are added to it in the future
(as long as there are no new serialized types that the software doesn't
know how to serialize/deserialize).

Example:

```js
> {"command": "server_definitions"}
< {
    "result": {
        "FIELDS": [
            [
                "Generic",
                {
                    "isSerialized": false,
                    "isSigningField": false,
                    "isVLEncoded": false,
                    "nth": 0,
                    "type": "Unknown"
                }
            ],
            [
                "Invalid",
                {
                    "isSerialized": false,
                    "isSigningField": false,
                    "isVLEncoded": false,
                    "nth": -1,
                    "type": "Unknown"
                }
            ],
            [
                "ObjectEndMarker",
                {
                    "isSerialized": false,
                    "isSigningField": true,
                    "isVLEncoded": false,
                    "nth": 1,
                    "type": "STObject"
                }
            ],
        ...
\```

Close #3657

---------

Co-authored-by: Richard Holland <richard.holland@starstone.co.nz>

@intelliot intelliot merged commit 078bd60 into XRPLF:develop Oct 18, 2023
17 checks passed
@intelliot intelliot added Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Feature Request Used to indicate requests to add new features Will Need Documentation Added to API Changelog labels Oct 18, 2023
@intelliot intelliot mentioned this pull request Oct 23, 2023
1 task
@kennyzlei
Copy link
Collaborator

For Clio, we are considering adding this support in 2.2 release as it needs some time to investigate support for this API. This means for s1/s2, rippled 2.0 release would mean we would need to wait for Clio 2.2 to have this RPC accessible

XRPLF/clio#942

@mvadari
Copy link
Collaborator Author

mvadari commented Oct 24, 2023

For Clio, we are considering adding this support in 2.2 release as it needs some time to investigate support for this API. This means for s1/s2, rippled 2.0 release would mean we would need to wait for Clio 2.2 to have this RPC accessible

XRPLF/clio#942

@kennyzlei is it possible to just forward to rippled in 2.1? It should be pretty performant as-is and shouldn't need to be called too often.

@kennyzlei
Copy link
Collaborator

@mvadari I believe this will depend on the timing of our Clio releases. We are starting to release more frequently with 2.1 slotted to release in 2 weeks, so 2.2 should not be delayed by much. We need the investigation time to decide if Clio should forward these requests or do something, which may be tight to complete within 2 weeks

ckniffen pushed a commit to XRPLF/xrpl.js that referenced this pull request Oct 31, 2023
@SaxenaKaustubh
Copy link
Collaborator

Worked on this with @mvadari on this change. After a few iterations, we have this working the way we want. This concludes the manual testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added to API Changelog API Change Clio Reviewed Feature Request Used to indicate requests to add new features Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Testable Will Need Documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Consider automatically generate enum definitions during compilation
9 participants