Skip to content

feat: Add Formats and Fields to server_definitions#6321

Open
godexsoft wants to merge 57 commits intoXRPLF:developfrom
godexsoft:feature/flags-server-definitions
Open

feat: Add Formats and Fields to server_definitions#6321
godexsoft wants to merge 57 commits intoXRPLF:developfrom
godexsoft:feature/flags-server-definitions

Conversation

@godexsoft
Copy link
Collaborator

@godexsoft godexsoft commented Feb 3, 2026

High Level Overview of Change

Closes #5151
Attempts to implement XRPLF/XRPL-Standards#418

This is a combination of #5702 (bulk of functionality) and #5616 (tests).

If merged:

  • Issue with tfBatchMask not matching the previous constant is fixed and covered by an extra test in Batch_test.cpp.
  • Modernized the code where possible, removed incorrect uses of initializer_list where applicable.

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

Comment on lines +270 to +295
auto txCommonFields = std::set<std::string>();
for (auto const& element : TxFormats::getCommonFields())
{
Json::Value elementObj = Json::objectValue;
elementObj[jss::name] = element.sField().getName();
elementObj[jss::optionality] = element.style();
defs_[jss::TRANSACTION_FORMATS][jss::common].append(elementObj);
txCommonFields.insert(element.sField().getName());
}

for (auto const& format : TxFormats::getInstance())
{
auto const& soTemplate = format.getSOTemplate();
Json::Value templateArray = Json::arrayValue;
for (auto const& element : soTemplate)
{
if (txCommonFields.count(element.sField().getName()) > 0)
continue; // skip common fields, already added
Json::Value elementObj = Json::objectValue;
elementObj[jss::name] = element.sField().getName();
elementObj[jss::optionality] = element.style();
templateArray.append(elementObj);
}
defs_[jss::TRANSACTION_FORMATS][format.getName()] = templateArray;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

just out of curiosity, is there any benefit in inserting common fields first? Will they actually be ordered to be first in the printout in this case? I think we don't need to do any separation at all and just dump all the fields, regardless

Copy link
Collaborator

Choose a reason for hiding this comment

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

in case you decide to leave this behind, txCommonFields.count(element.sField().getName()) > 0 -> txCommonFields.contains(element.sField().getName())

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 checked and no it does not make it appear first (it's actually last in the list for both).
I don't think the intention was to have them first or ordered in any specific way, other than for the common fields to land under 'common' instead of injecting them into each format. Will apply your suggestion 👍

Copy link
Collaborator

@mvadari mvadari Feb 20, 2026

Choose a reason for hiding this comment

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

The JSON library automatically sorts in alphabetical order

Comment on lines +297 to +323
defs_[jss::LEDGER_ENTRY_FORMATS] = Json::objectValue;
defs_[jss::LEDGER_ENTRY_FORMATS][jss::common] = Json::arrayValue;
auto ledgerCommonFields = std::set<std::string>();
for (auto const& element : LedgerFormats::getCommonFields())
{
Json::Value elementObj = Json::objectValue;
elementObj[jss::name] = element.sField().getName();
elementObj[jss::optionality] = element.style();
defs_[jss::LEDGER_ENTRY_FORMATS][jss::common].append(elementObj);
ledgerCommonFields.insert(element.sField().getName());
}
for (auto const& format : LedgerFormats::getInstance())
{
auto const& soTemplate = format.getSOTemplate();
Json::Value templateArray = Json::arrayValue;
for (auto const& element : soTemplate)
{
if (ledgerCommonFields.count(element.sField().getName()) > 0)
continue; // skip common fields, already added
Json::Value elementObj = Json::objectValue;
elementObj[jss::name] = element.sField().getName();
elementObj[jss::optionality] = element.style();
templateArray.append(elementObj);
}
defs_[jss::LEDGER_ENTRY_FORMATS][format.getName()] = templateArray;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

same q here

Copilot AI review requested due to automatic review settings February 20, 2026 15:19

This comment was marked as resolved.

@godexsoft godexsoft self-assigned this Feb 22, 2026
@godexsoft godexsoft added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Feb 22, 2026
Copilot AI review requested due to automatic review settings February 25, 2026 00:10

This comment was marked as resolved.

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

This is marked as "Ready to merge", but it's not clear to me that all of @mvadari's questions have been addressed.

In addition, I took a skim through the PR, and noticed a couple of possible improvements.

@ximinez ximinez removed the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Feb 25, 2026
@mvadari mvadari added the Blocked: Needs Final XLS The corresponding final XLS must have been merged before this PR is merged. label Feb 25, 2026
@godexsoft godexsoft requested review from mvadari and ximinez February 26, 2026 19:16
@godexsoft godexsoft added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Blocked: Needs Final XLS The corresponding final XLS must have been merged before this PR is merged. labels Mar 2, 2026
Copilot AI review requested due to automatic review settings March 2, 2026 21:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

various purposes, but attempting to use such values to identify
on-ledger objects will results in an invariant failure.
@note Values outside this range may be used internally by the code for various purposes, but
attempting to use such values to identify on-ledger objects will results in an invariant failure.
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

In this doc comment, the phrase "will results in an invariant failure" is grammatically incorrect (should be "will result"). Please fix to avoid propagating typos into generated docs and developer references.

Suggested change
attempting to use such values to identify on-ledger objects will results in an invariant failure.
attempting to use such values to identify on-ledger objects will result in an invariant failure.

Copilot uses AI. Check for mistakes.
{
BEAST_EXPECT(result[jss::result].isMember(jss::LEDGER_ENTRY_FORMATS));

// Note: For the purposes of software maintainance, this test does not exhaustively
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

Typo in comment: "maintainance" should be "maintenance".

Suggested change
// Note: For the purposes of software maintainance, this test does not exhaustively
// Note: For the purposes of software maintenance, this test does not exhaustively

Copilot uses AI. Check for mistakes.
Comment on lines +25 to +31
## XRP Ledger server version 3.2.0

[Version 3.2.0](https://github.com/XRPLF/rippled/releases/tag/3.2.0)

### Additions in 3.2.0

- `server_definitions`: Added the following new sections to the response ([#6321](https://github.com/XRPLF/rippled/pull/6321)):
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

This new changelog entry for "XRP Ledger server version 3.2.0" doesn’t include a release date, while the surrounding entries do. Please either add the release date (if known) or adjust the heading/link text to match the established format used in this file for unreleased/new versions.

Copilot uses AI. Check for mistakes.
ximinez
ximinez previously approved these changes Mar 4, 2026
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

All of my concerns have been addressed, though Copilot identified a couple of things.

@ximinez ximinez dismissed their stale review March 4, 2026 22:22

I haven't done a deep review, so I'm just going to dismiss this approval. There are plenty of other approvals to proceed with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add transaction and ledger entry formats to server_definitions

6 participants