Consolidate definitions of fields, objects, transactions, and features #5122

thejohnfreeman · 2024-09-06T22:31:31Z

Leverages a preprocessor technique to put the entire definitions of fields, objects, transactions, and features into one file for each type. When you want to add a field, object, transaction, or feature, you touch just one file. When you want to inspect a definition, you look up just one file.

donovanhide · 2024-09-07T06:40:13Z

This is a useful change and would make the manual process of updating third party libraries with new field types, transactions and ledger entries a little easier.
However, an even more useful change would be to abstract the definition files into easily machine parsable formats, maybe either JSON or CSV, so that any third party API libraries can choose to auto-generate their data structures and stream encoders and decoders from those files, thereby keeping up to date much more easily than using git blame to find recent changes to the large definition files.

thejohnfreeman · 2024-09-07T13:09:28Z

Yes, that would be great, but the preprocessor cannot parse JSON or CSV or any other format. It would require the introduction of an additional build step to generate these headers from that definition file. I would welcome that contribution from someone else, but it's out of scope for my current work. This was a drive-by change I made while working on single asset vault.

donovanhide · 2024-09-07T14:03:43Z

Thanks for the reply. I do think the pool of people who could wrangle the rippled build process to add a code generation stage is probably limited to one :-) If it's not something that appeals, fair enough.

Bronek

Few initial comments, also this: I wonder if this pattern is perhaps too permissive

#pragma push_macro("FOO")
#undef FOO
#pragma push_macro("BAR")
#undef BAR

#define FOO . . .;
#define BAR . . .;

#include <xrpl/protocol/. . .>

#undef FOO
#pragma pop_macro("FOO")
#undef BAR
#pragma pop_macro("BAR")

Could this be better (i.e. more robust) ?

#if defined(FOO) || defined(BAR)
#error "Macro FOO or BAR was found to be unexpectedly defined"
#endif

#define FOO . . .;
#define BAR . . . ;

#include <xrpl/protocol/. . .>

#undef FOO
#undef BAR

Bronek · 2024-09-25T14:00:25Z

include/xrpl/protocol/LedgerFormats.h

-    ltORACLE = 0x0080,
+#pragma push_macro("OBJECT")
+#undef OBJECT
+#define OBJECT(tag, value, name, fields) tag = value,


For consistency, please follow the same style as in Feature.h (vertical spacing)

Would LEDGER_ENTRY be a more specific name for the macro? OBJECT seems very general and suggests it can be used elsewhere.

include/xrpl/protocol/SField.h

include/xrpl/protocol/TxFormats.h

Bronek · 2024-09-25T14:07:08Z

include/xrpl/protocol/features.h

I would put it in include/xrpl/protocol/detail . Also, how about name which clearly refers to a header it is used in, e.g. Feature_list.h or even better Feature.inl ?

I much prefer the simple features.h naming.

I totally do not mind .ipp, or even stick with .h if you insist, but then I would very much prefer if it was moved to detail subdir as not to make clutter. These files are not standalone, like all well-written header files should be, so having a different extension is a hint to the user that IDE errors are expected when viewing them

never mind, I see they have been moved to detail, thanks !

I moved them to detail/ subdirectory.

include/xrpl/protocol/objects.h

src/libxrpl/protocol/TxFormats.cpp

include/xrpl/protocol/features.h

include/xrpl/protocol/objects.h

include/xrpl/protocol/sfields.h

include/xrpl/protocol/transactions.h

mvadari · 2024-09-25T16:12:28Z

This is a useful change and would make the manual process of updating third party libraries with new field types, transactions and ledger entries a little easier. However, an even more useful change would be to abstract the definition files into easily machine parsable formats, maybe either JSON or CSV, so that any third party API libraries can choose to auto-generate their data structures and stream encoders and decoders from those files, thereby keeping up to date much more easily than using git blame to find recent changes to the large definition files.

@donovanhide does the server_definitions RPC solve your problem? https://xrpl.org/docs/references/http-websocket-apis/public-api-methods/server-info-methods/server_definitions

donovanhide · 2024-09-25T19:01:40Z

@mvadari Thanks for pointing it out. It is interesting and I was unaware of it, but it is not the full answer. I need to know all the fields for each ledger entry and transaction type, but that seems to be missing from that response.

tequdev · 2024-10-01T15:42:46Z

I need to know all the fields for each ledger entry and transaction type, but that seems to be missing from that response.

Note that the internal structure of fields defined with CONSTRUCT_UNTYPED_SFIELD, such as the sfMemo field, for example, cannot be known (as before).

donovanhide · 2024-10-01T16:33:10Z

Just to be clear of my use case, it is updating the data structures here:

https://github.com/rubblelabs/ripple/blob/master/data/ledgerentry.go
https://github.com/rubblelabs/ripple/blob/master/data/transaction.go
https://github.com/rubblelabs/ripple/blob/master/data/format.go
https://github.com/rubblelabs/ripple/blob/master/data/flags.go

And the canonical source of truth for these data structures, encodings and flags are the source code files being changed in this PR. It would be much more efficient if my library and others could be derived from an easily machine readable file rather than a hard to diff C++ file. Seeing as this is the essence of the Ripple protocol, it doesn't seem like an unreasonable request :-)

mvadari · 2024-10-02T21:41:01Z

Just to be clear of my use case, it is updating the data structures here:

https://github.com/rubblelabs/ripple/blob/master/data/ledgerentry.go https://github.com/rubblelabs/ripple/blob/master/data/transaction.go https://github.com/rubblelabs/ripple/blob/master/data/format.go https://github.com/rubblelabs/ripple/blob/master/data/flags.go

And the canonical source of truth for these data structures, encodings and flags are the source code files being changed in this PR. It would be much more efficient if my library and others could be derived from an easily machine readable file rather than a hard to diff C++ file. Seeing as this is the essence of the Ripple protocol, it doesn't seem like an unreasonable request :-)

@donovanhide that's a fair statement, but I think it should be in an RPC rather than in the source code (perhaps added to server_definitions). Let's move this discussion to this issue I just opened on the topic.

codecov · 2024-10-02T22:57:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.1%. Comparing base (f0dabd1) to head (9d0c99e).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5122     +/-   ##
=========================================
- Coverage     76.2%   76.1%   -0.0%     
=========================================
  Files          760     762      +2     
  Lines        61557   61469     -88     
  Branches      8124    8124             
=========================================
- Hits         46892   46802     -90     
- Misses       14665   14667      +2

Files with missing lines	Coverage Δ
include/xrpl/protocol/Feature.h	`100.0% <ø> (ø)`
include/xrpl/protocol/SField.h	`100.0% <100.0%> (ø)`
include/xrpl/protocol/detail/ledger_entries.macro	`100.0% <100.0%> (ø)`
include/xrpl/protocol/detail/transactions.macro	`100.0% <100.0%> (ø)`
src/libxrpl/protocol/Feature.cpp	`94.6% <ø> (ø)`
src/libxrpl/protocol/LedgerFormats.cpp	`100.0% <ø> (ø)`
src/libxrpl/protocol/SField.cpp	`77.5% <ø> (ø)`
src/libxrpl/protocol/TxFormats.cpp	`100.0% <ø> (ø)`
src/xrpld/app/tx/detail/CancelCheck.h	`100.0% <ø> (ø)`
src/xrpld/app/tx/detail/CancelOffer.h	`100.0% <ø> (ø)`
... and 14 more

... and 3 files with indirect coverage changes

thejohnfreeman · 2024-10-03T14:10:42Z

Whenever it comes time to merge this branch with the changes in #5143, regardless of whichever lands in develop first, the conflict resolutions can be cherry picked from my vault branch.

gregtatcam

I like this idea of changing just one file a lot but IMHO there are two inconvenient issues:

Because of the macros, some IDE (I have CLion) are not going to be able to resolve the references. For instance, clicking on a field name in transactions.m or ledger_entries.m doesn't navigate to the fields declaration. And searching for the field with the sf prefix is not going to be helpful either - see the next issue.
I'd prefer that the fields keep sf prefix. For instance, when I search for sfAmount I can see the field type and the occurrences. But try to search for Amount and you'll get huge number of hits. Of course I know where to find the field declaration but I have to navigate to this file separately.

gregtatcam · 2024-10-10T18:03:31Z

include/xrpl/protocol/detail/features.m

@@ -0,0 +1,112 @@
+//------------------------------------------------------------------------------


Should the .m files be .h? It's confusing since .m is usually associated with Objective-C.

I asked to use a different extension that cannot be confused with a header, because these files are not headers in normal sense of the word (because of macros they rely on) - they cannot be included as-is, they won't parse in an IDE, they cannot be formatted with clang-format. I suggested .inl as that's typically used for "inline files" but @thejohnfreeman chose different.

It's confusing IMO, but I'm fine with this.

This : https://gcc.gnu.org/onlinedocs/cpp/Header-Files.html suggests headers can be used for macros.

Yes, headers are often used for macro definitions. Also the language attaches no meaning to file extension. However things get a little tricky for IDEs and other tools when, what appears to be a header file from extension, does not work like one. In this case the apparent header files rely on a macro definitions brought from the outside without the relevant #include inside the file, which is not normal practice of how header files work.

If that helps to get the PR approved, I am happy for @thejohnfreeman to rename these files to any extension, including .h. I guess it does not matter much.

.m feels odd but not a showstopper I guess. I'm more concerned about two other issues that I mentioned.

Would everyone like .macro more than both .m and .h?

If for some reason .h is not an option then .macro is better.
But as I mentioned earlier, using macros in general prevents IDE (at least CLion) resolving the fields reference. I find this really inconvenient.

Bronek · 2024-10-11T13:04:58Z

include/xrpl/protocol/detail/ledger_entries.m

+//==============================================================================
+
+#if !defined(LEDGER_ENTRY)
+#error "undefined macro: LEDGER_ENTRY"


I just occurred to me that in order to make this file (and others) similar to a regular header, we could do this instead of #error :

#define LEDGER_ENTRY(...) #pragma message("using dummy LEDGER_ENTRY")

Note the dummy macro is empty. This in turn would make the file well-behaved in IDEs and other tools, meaning it could be renamed back to .h if you so desire.

@gregtatcam any comments on this ?

Sounds good to me.

It still doesn't solve the issue of resolving the fields reference by IDE, does it?

You are right - it does not. An actual macro doing something could do that, maybe ?

gregtatcam

👍 LGTM

XRPLF#5122)

thejohnfreeman added 4 commits September 6, 2024 17:30

Consolidate SField declarations

e635865

Consolidate ledger object definitions

7e4fd55

Consolidate transaction definitions

756dd7f

Consolidate feature definitions

d8f51d5

thejohnfreeman requested review from Bronek and ximinez September 6, 2024 22:31

thejohnfreeman mentioned this pull request Sep 24, 2024

feature: Single Asset Vault #5147

Closed

Bronek reviewed Sep 25, 2024

View reviewed changes

thejohnfreeman requested review from bthomee and removed request for ximinez September 27, 2024 16:24

thejohnfreeman mentioned this pull request Oct 1, 2024

Introduce MPT support (XLS-33d): #5143

Merged

13 tasks

thejohnfreeman added 2 commits October 2, 2024 16:38

Address review

d9fc80a

Merge branch 'develop'

b1a82d9

Sort objects by ledger entry type ID

2cd17d9

Bronek approved these changes Oct 3, 2024

View reviewed changes

thejohnfreeman added 3 commits October 3, 2024 09:35

Rename macro OBJECT -> LEDGER_ENTRY

19e14e1

Rename files

a12eab9

Remove clang directives

9099f71

gregtatcam reviewed Oct 10, 2024

View reviewed changes

Bronek reviewed Oct 11, 2024

View reviewed changes

thejohnfreeman added 4 commits October 11, 2024 13:29

Generate transaction dispatch

a7798c5

Append trailing comments with sfFieldNames

0adfebe

Remove stray semicolon

7821df1

Restore sfFieldName in macro call

cf73cd1

gregtatcam approved these changes Oct 14, 2024

View reviewed changes

thejohnfreeman added 4 commits October 15, 2024 12:46

Implicit Json::StaticString conversion for SField

aa3ac9d

Fix formatting

ec5603a

Rename files .m -> .macro

d1546cd

Automatically define JSS for transactions

dce9646

thejohnfreeman added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Oct 16, 2024

thejohnfreeman added 2 commits October 16, 2024 12:42

Merge branch 'develop'

994996f

Fix formatting

9d0c99e

thejohnfreeman merged commit 63209c2 into XRPLF:develop Oct 16, 2024
20 checks passed

thejohnfreeman deleted the fields branch October 16, 2024 19:02

Bronek pushed a commit to Bronek/rippled that referenced this pull request Oct 17, 2024

Consolidate definitions of fields, objects, transactions, and features (

75e7c36

XRPLF#5122)

This was referenced Nov 6, 2024

Proposed 2.3.0-rc1 #5182

Closed

Proposed 2.3.0-rc1 #5183

Merged

mvadari mentioned this pull request Nov 12, 2024

fix: include index in server_definitions RPC #5190

Merged

6 tasks

vlntb pushed a commit to vlntb/rippled that referenced this pull request Nov 19, 2024

Consolidate definitions of fields, objects, transactions, and features (

4680fdf

XRPLF#5122)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate definitions of fields, objects, transactions, and features #5122

Consolidate definitions of fields, objects, transactions, and features #5122

thejohnfreeman commented Sep 6, 2024

donovanhide commented Sep 7, 2024

thejohnfreeman commented Sep 7, 2024

donovanhide commented Sep 7, 2024

Bronek left a comment •

edited

Loading

Bronek Sep 25, 2024

donovanhide Oct 3, 2024

Bronek Sep 25, 2024

thejohnfreeman Oct 2, 2024

Bronek Oct 3, 2024

Bronek Oct 3, 2024

thejohnfreeman Oct 3, 2024

mvadari commented Sep 25, 2024

donovanhide commented Sep 25, 2024

tequdev commented Oct 1, 2024

donovanhide commented Oct 1, 2024

mvadari commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading

thejohnfreeman commented Oct 3, 2024

gregtatcam left a comment

gregtatcam Oct 10, 2024

Bronek Oct 10, 2024

gregtatcam Oct 10, 2024

gregtatcam Oct 10, 2024

Bronek Oct 11, 2024

gregtatcam Oct 11, 2024

thejohnfreeman Oct 11, 2024

gregtatcam Oct 11, 2024

Bronek Oct 11, 2024

Bronek Oct 11, 2024

gregtatcam Oct 11, 2024

gregtatcam Oct 11, 2024

Bronek Oct 11, 2024

gregtatcam left a comment

		@@ -0,0 +1,112 @@
		//------------------------------------------------------------------------------

Consolidate definitions of fields, objects, transactions, and features #5122

Consolidate definitions of fields, objects, transactions, and features #5122

Conversation

thejohnfreeman commented Sep 6, 2024

donovanhide commented Sep 7, 2024

thejohnfreeman commented Sep 7, 2024

donovanhide commented Sep 7, 2024

Bronek left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari commented Sep 25, 2024

donovanhide commented Sep 25, 2024

tequdev commented Oct 1, 2024

donovanhide commented Oct 1, 2024

mvadari commented Oct 2, 2024 • edited Loading

codecov bot commented Oct 2, 2024 • edited Loading

Codecov Report

thejohnfreeman commented Oct 3, 2024

gregtatcam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

Bronek left a comment •

edited

Loading

mvadari commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading