Wrapper class for Fee Ledger Object

[ckeshava]

High Level Overview of Change

This PR introduces wrapper for Fees Ledger Object

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)
• Tests (You added tests for code that already exists, or your new feature included in this PR)
• Documentation Updates
• Release

[scottschurr]

Rather than add review notes for every suggested change, I'll describe the process that I followed during my review. I edited all of the changes I suggest into a [FOLD] commit that you can find here: https://github.com/scottschurr/rippled/commits/chenna-feesWrap You are welcome to use that commit any way you like, including cherry-picking. I'm hoping that providing my review comments in this fashion will be more useful than isolated comments.

The feesWrap branch did not build for me initially because the following files...

• src/ripple/app/ledger/impl/BuildLedger.cpp
• src/ripple/app/ledger/impl/InboundLedger.cpp
• src/ripple/app/main/Application.cpp
• src/ripple/app/reporting/ReportingETL.cpp
• src/ripple/nodestore/impl/Shard.cpp

were all missing the following line:

#include <ripple/protocol/Fees.h>

Once I added that #include to all of those files then the project built. I wonder if the reason you did not notice this problem is that you use unity builds? At any rate, with this branch a non-unity debug build failed with errors.

I then looked in src/ripple/protocol/Fees.h

I noticed...

using FeesLgrObjectRd = FeesImpl<false>;

In particular, FeesImpl is not defined as FeesLgrObjectImpl. That felt like an asymmetry to me. I think that it's important that the names of the three related classes (Impl, Rd, and <mutable>) keep a uniform relationship.

You did a good job refining the Fees name to distinguish it from the pre-exisiting Fees class. Indicating that the new one represents a ledger object is a great way to distinguish it from the pre-exisiting Fees class. But I felt like the choice of spelling out "Object" while abbreviating "Lgr" was odd. Since I wanted to change the name of the "Impl" class anyway, I decided that I preferred the name FeesLedgerObjImpl over FeesLgrObjectImpl. It has the same number of characters and emphasizes "Ledger", which I think is more important than "Object" in the name.

So I renamed the "Impl" class, the file that contains it, the two usings, and the name of the #include guard.

Then I grepped for occurrences of FeesImpl and fixed those. I only found those in Keylet.h.

At this point I re-built the project to find all of the #includes that I needed to fix because of the renamed file. I fixed up those files.

While I was fixing that up I noted that the #includes in Ledger.cpp and RCLValidations_test.cpp were not sorted correctly and fixed those up as well.

After that successfully built I did a grep for keylet::fees to look at peek() vs peekSLE() and variable names. That all looked good. I didn't change anything.

I am not keen on using auto for return types unless there's an obvious benefit. When I'm reading source code it's really useful for the return type to be visible as part of the function/method declaration. Without that then I need to go digging through the implementation of the function, and even that may not give me the explicit name of the returned type.

So I changed all of the auto returns in FeeLedgerObj.h to be explicit types.

After that built successfully I did:

git difftool HEAD~2

to look over the accumulation of your changes plus my suggested changes.

I noted that the copyright in FeesLedgerObj.h was 2021, so I changed it to 2023. The copyright date in AcctRoot.h is 2021, but that's probably correct because that's when that code was originally developed.

I also remembered that I had suggested keeping the definitions in Keylet.h alphabetical, so I moved the definition of FeesKeylet below AccountRootKeylet.

That's pretty much the process. I hope that helps.

[scottschurr]

Yeah, I think that comment change helps clarify the meaning. Thanks.

[scottschurr]

Thanks for noticing that Log.h is not needed.

[ckeshava]

CLion did it for me :)

[ckeshava]

Thank you so much, this is very extensive and helpful.

I have specified a non-unity Release build in my CMake configuration options. Is this not sufficient?

Regarding the ordering of the #includes, do you use any tool which indicates that the imports are not ordered? other languages have this type of facility, I'm not aware of it in C++.

[ckeshava]

@scottschurr Thanks for the detailed reply, I'll keep these suggestions in mind.

I'm still unclear on differing compilation behaviours in our machines. I have not switched to a non-unity debug build, so hopefully such discrepencies will not arise again.

Regarding the return types of the functions, I was not 100% sure about the types, so I hid behind auto keyword. I'll try to dig deeper in the future 👍

[scottschurr]

Regarding not knowing return types, here's a trick I use fairly often...

Suppose I have an auto type or some sort of type I can't easily identify. Maybe like this:

return wrapped_->at(~sfBaseFeeDrops);

You can usually coerce the compiler to tell you what the type is by assigning that type to an incompatible type. Like this:

[[maybe_unused]] int* junk = wrapped_->at(~sfBaseFeeDrops);
return wrapped_->at(~sfBaseFeeDrops);

This will (usually) cause the compiler to stop with an error. Often the compiler will be helpful and tell you what type it cannot assign to an int*. By looking at the error message you can (usually) determine the type being returned.

Very occasionally the return type can legally be assigned to junk. Then you have to pick a different type that (hopefully) will not support the assignment.

If you use this trick be sure to remove all vestiges of the experiment you were running before submitting a pull request.

[scottschurr]

Regarding...

I have specified a non-unity Release build in my CMake configuration options. Is this not sufficient?

Got me? I have no experience with CLion. Other than with the debugger, I work pretty much just with a text editor and the command line. I can show you the command lines that I use, but I have no idea what (command line equivalents) CLion produces.

[scottschurr]

Regarding the ordering of the #includes, do you use any tool which indicates that the imports are not ordered? other languages have this type of facility, I'm not aware of it in C++.

In the past clang-format has helped some of the time with keeping the #includes alphabetical. Other than that, maintaining #include order is something that we've done manually.

[ckeshava]

okay 👍 I have cherry-picked your commit to update this PR, thank you very much, I'll keep these suggestions in mind for future PRs.

[scottschurr]

Looks great! Thanks for doing this.

Two suggestions for pushing this forward.

1. Let's hold off on making a pull request against develop until we've had a chance to request reviewers for all of the wrapper pull requests. We can do that at the huddle on June 7th.
2. Squash all of these commits together before making the pull request. Your reviewers do not need to see your development process.

I think that's if from me. Let's hope for the best from the other reviewers.

[ckeshava]

Thank you so much Scott! I'll keep these points in mind :+1 Regards, Keshava

…

On Tue, Jun 6, 2023 at 5:01 PM Scott Schurr ***@***.***> wrote: ***@***.**** approved this pull request. Looks great! Thanks for doing this. Two suggestions for pushing this forward. 1. Let's hold off on making a pull request against develop until we've had a chance to request reviewers for all of the wrapper pull requests. We can do that at the huddle on June 7th. 2. Squash all of these commits together before making the pull request. Your reviewers do not need to see your development process. I think that's if from me. Let's hope for the best from the other reviewers. — Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/scottschurr/rippled/pull/24*pullrequestreview-1466327583__;Iw!!PZTMFYE!9rFDbkzmwOrXBiZtXZIxqeDju94A8WUp7RPxREki6ELNgNZnbb_FOaay05uWzTXdLvIJHvsviPt14dsDItc9V1ai0A$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AFB4TNIPOPIGGJKMCU7SDETXJ7AFHANCNFSM6AAAAAAYUUGWTA__;!!PZTMFYE!9rFDbkzmwOrXBiZtXZIxqeDju94A8WUp7RPxREki6ELNgNZnbb_FOaay05uWzTXdLvIJHvsviPt14dsDItfASRxxgQ$> . You are receiving this because you authored the thread.Message ID: ***@***.***>