Strongly typed wrapper for DepositPreAuthorization Ledger Object #18

ckeshava · 2023-05-25T00:43:21Z

High Level Overview of Change

This PR introduces wrapper for Deposit PreAuth Ledger Object. The existing usages of readSLE and peekSLE functions have been modified to use the read/peek functions respectively.

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

Test Plan

I have not included any new unit tests. Since this is an internal refactor, existing test cases should suffice in validating the new changes. As part of future work, additional test cases can be added pertaining to the APIs of newly added classes.

scottschurr

Nice work! I left some comments for you to consider. Feel free to push back or ask questions on any of these. Thanks!

scottschurr · 2023-05-25T17:05:18Z

src/ripple/protocol/Keylet.h

+
+    using KeyletBase::check;
+
+    DPAuthKeylet(uint256 const& key) : KeyletBase(ltDEPOSIT_PREAUTH, key)


I think this constructor should be explicit. We don't want the compiler to convert any uint256 into a DPAuthKeylet without being asked to do so.

understood, thanks for pointing it out 👍

scottschurr · 2023-05-25T20:48:29Z

src/ripple/app/tx/impl/DepositPreauth.cpp

@@ -171,16 +171,16 @@ DepositPreauth::removeFromLedger(
 {
    // Verify that the Preauth entry they asked to remove is
    // in the ledger.
-    std::shared_ptr<SLE> const slePreauth{
-        view.peekSLE(keylet::depositPreauth(preauthIndex))};
+    std::optional<DPAuthImpl<true>> slePreauth{


Using DPAuthImpl<true> is less convenient than it might be. The AcctRoot.h header file declares the AcctRootImpl template, but it also declares...

using AcctRootRd = AcctRootImpl<false>; using AcctRoot = AcctRootImpl<true>;

If you did something similar in DPAuth.h, then this declaration would be std::optional<DPAuth> which is easier to read.

While we're on this line, the variable name slePreauth is now misleading. The variable is no longer a Serialized Ledger Entry (which is what sle stands for) any more. I suggest you change the variable name to simply preauth. When I integrated the AcctRoot wrapper I tried to be conscientious about fixing the variable names.

Fixing the variable names will be particularly important when the type is auto, not quite as important here, since you have an explicit type. But we shouldn't leave misleading names hanging around.

thanks for calling out the variable name, I'll work on it.

Regarding creating a type-alias for the read-only version, I did consider having using DPAuthRd = DPAuth<true>;. But I felt another alias decreases the readability of the code 😓

But if this is the coding convention, I'll definitely incorporate your suggestion, it's just my personal bias against typedefs 😄

scottschurr · 2023-05-25T21:13:04Z

src/ripple/protocol/DPAuth.h

+
+namespace ripple {
+
+// Wrapper class for DepositPreAuth Ledger Object. This class encapsulates the std::shared_ptr<SLE> to the actual ledger object.


This comment is too long. It should wrap before it gets to 80 characters. Yeah, clang-format may do that for you, but I personally prefer to be in control of how my comments look -- including where the line breaks are.

scottschurr · 2023-05-25T21:14:31Z

src/ripple/protocol/DPAuth.h

+    friend class ApplyView;
+
+public:
+    // Conversion operator from DPAuthImpl<true> to DPAuthImpl<false>.


Nice work with removing the explicit special member functions!

thank you :)

scottschurr · 2023-05-25T21:15:25Z

src/ripple/protocol/DPAuth.h

+        return wrapped_->at(sfOwnerNode);
+    }
+};
+}


I suggest you add the following declarations to the bottom of the file (but still inside the ripple namespace):

using DPAuthRd = DPAuthImpl<false>; using DPAuth = DPAuthImpl<true>;

Given the class rename, please add these declarations to the file, but use the right names:

using DepPreAuthRd = DepPreAuthImpl<false>; using DepPreAuth = DepPreAuthImpl<true>;

After you've changed these names grep the source code for DPAuth. Fix up any instances that you find, either in source or in comments.

The reason I want to introduce the DepPreAuthRd and DepPreAuth names is because DepPreAuthImpl being a template is an implementation artifact. Scott D and were looking for a way to make sure the interfaces of DepPreAuthRd and DepPreAuth are identical where ever possible. And we didn't want to leave behind a maintenance problem. Using a base template for the two types was the best way we identified to accomplish that.

We want people to think of DepPreAuthRd and DepPreAuth as being two distinct types, just
like iterator and const_iterator are two distinct but related types.

scottschurr · 2023-05-25T21:37:20Z

src/ripple/protocol/Indexes.h

 depositPreauth(uint256 const& key) noexcept
 {
-    return {ltDEPOSIT_PREAUTH, key};
+    return {key};


Once the DPAuthKeylet constructor is made explicit, I think you'll change the implementation like this:

return DPAuthKeylet(key);

ok. I have a tangential question, what are the tradeoffs with return DPAuthKeyley{key} ?

I am aware the there could multiple ways to initialize objects, but I am not clear about the differences.
Since this is an aggregate type, I believe using the {} syntax should be okay, but IDK the performance costs.

The runtime costs of () vs {} are identical. The difference is in how permissive the syntaxes are.

This is really guidance that I got from Howard. If I'm remembering correctly, there are situations where braced initialization will consider using an explicit constructor. So if you just casually use {} to initialize stuff, then you may be invoking an explicit constructor without realizing it. Howard said that using = or () for initialization does not have that problem.

For me, at least, C++ is a complicated enough language that I don't want to think, "Hmmm, what kind of initialization should I use here?" every time I initialize a value. So as a heuristic I tend not to use braced initialization unless I have a specific reason.

Note that Howard's guidance disagrees with guidance that Nico Jossutis (at least used to) give. Nico used to like defaulting to braced initialization (without the =).

As I'm looking around now I think you'll see that the safest default is = {<value(s)>}: https://www.josuttis.com/cpp/c++initialization.pdf But I'm not convinced that we could convince rippled developers to type that many characters every time they initialize initialize something.

That's my current take. But I suggest you talk to Howard about it. He has opinions and did the tests to back them up. Let me know what he says.

will do, thanks for sharing your thoughts.
Yes, I saw Nicolai Josutis's talk on the NIghtmare of Initialization. It was a fascinating talk :)

scottschurr · 2023-05-25T21:37:36Z

src/ripple/protocol/impl/Indexes.cpp

-    return {
-        ltDEPOSIT_PREAUTH,
-        indexHash(LedgerNameSpace::DEPOSIT_PREAUTH, owner, preauthorized)};
+    return {indexHash(LedgerNameSpace::DEPOSIT_PREAUTH, owner, preauthorized)};


Once the DPAuthKeylet constructor is made explicit, I think you'll need to change the implementation like this:

return DPAuthKeylet( indexHash(LedgerNameSpace::DEPOSIT_PREAUTH, owner, preauthorized));

scottschurr · 2023-05-25T21:55:54Z

src/ripple/app/tx/impl/DepositPreauth.cpp

@@ -195,7 +195,7 @@ DepositPreauth::removeFromLedger(
    adjustOwnerCount(view, *ownerAcctRoot, -1, app.journal("View"));

    // Remove DepositPreauth from ledger.
-    view.erase(slePreauth);
+    view.erase(slePreauth->slePtr());


This works, but I think a better approach is to call view.erase(*preauth). That passes the (non-optional) wrapper to erase.

right 👍 your structure for the erase function template is beautiful! (which enables the wrapper to be passed into erase and insert)

scottschurr · 2023-05-25T22:01:16Z

src/ripple/rpc/handlers/DepositAuthorized.cpp

            auto const sleDepositAuth =
-                ledger->readSLE(keylet::depositPreauth(dstAcct, srcAcct));
+                ledger->read(keylet::depositPreauth(dstAcct, srcAcct));
            depositAuthorized = static_cast<bool>(sleDepositAuth);


I'd be inclined to change these lines into...

depositAuthorized = ledger->read(keylet::depositPreauth(dstAcct, srcAcct)) .has_value();

That way you don't even need to name a local variable.

great point 👍

scottschurr · 2023-05-25T22:30:00Z

src/ripple/protocol/DPAuth.h

+
+// Wrapper class for DepositPreAuth Ledger Object. This class encapsulates the std::shared_ptr<SLE> to the actual ledger object.
+template <bool Writable>
+class DPAuthImpl final : public LedgerEntryWrapper<Writable>


I'm not really excited about DPAuthImpl as a name. If I'm not already thinking about deposit preauthorization the DP prefix really doesn't get me oriented. But DepositPreauth is already taken. DepositPreAuthorization is getting kind'a long winded, even for me.

What would you think of DepPreAuthImpl? Still not great. We could also leave this as DPAuthImpl for now and see what folks think during the code review. Your call. I guess the good news is that this will usually be a return type declared with auto, so it won't show up in the source code a whole lot.

Agreed, I couldn't think of a better name, so I rushed through with the first one that popped into my head.
I like DepPreAuthImpl too, I'm open to any other suggestions as well 👍

Rename this class to DepPreAuthImpl.

ckeshava · 2023-05-26T19:51:54Z

Ok, thanks for the suggestions @scottschurr this was really helpful.
I squashed my commits and force-pushed into this branch, sorry about that.
I have addressed your comments 👍

scottschurr

Here's the way I think of code reviews. The reviewer is doing you a favor. Most reviewers would rather be writing their own code than reviewing yours. Anything they flag is for a reason, even if you don't necessarily agree 100% with the reason. So when I (personally) get a review comment I implement that change unless I have a defensible reason not to make the change. If it's close to a wash whether my way is better or theirs, then I go with theirs. This helps make my reviewers happier about reviewing my code next time. Do ask questions. But be biased toward assuming the reviewer is probably right.

I suggest you re-review all of your changes using your difftool before submitting any kind of pull request. Walk through the list of requested changes and make sure you either made the change or responded to the comment. In reality you only get one good review out of a reviewer. The second or third time they review your code they can no longer see things with fresh eyes. So you want to make sure that the first thing you show them is your very best effort.

You should make sure to run clang-format over your code before submitting a pull request. I can tell that you did not do that on this branch because you have some lines that exceed 80 characters. I have my text editor set up with a fence at 80 characters. If you use CLion as your editor, then I suggest you see if CLion can do something like that. I bet it can.

You are welcome to disagree with any of the suggestions I make. But if you don't disagree, with a cogent reason, then I'll expect to see the change made in the next revision.

scottschurr · 2023-05-31T22:41:07Z

src/ripple/rpc/handlers/DepositAuthorized.cpp

-            auto const sleDepositAuth =
-                ledger->readSLE(keylet::depositPreauth(dstAcct, srcAcct));
-            depositAuthorized = static_cast<bool>(sleDepositAuth);
+            auto const depositAuthorized =


Compiling this line gives the warning "unused variable 'depositAuthorized'". I saw this because I build with -Dwerr=ON -Dwextra=ON. I think you should build using those flags too.

The bug here is that you've included auto const in your assignment to depositAuthorized. So you're assigning to a brand new variable. The name of that variable happens to shadow a variable of the same name in the outer scope. So the variable in the outer scope was unaffected by the assignment. Only the inner (unused) variable received the assignment.

To fix the bug remove the auto const.

Okay, thanks for this catch. Do you happen to remember which CMake or Conan file needs to be updated with this setting?
I use this command to set up CMake (cmake -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -DCMAKE_BUILD_TYPE=Release ..), but I'm not sure if these configuration options can be passed into CMake and not clang.

You can just add those flags to the cmake command line. Here's the cmake command line I use for a debug build:

cmake -DCMAKE_BUILD_TYPE=Debug -Dunity=OFF -Dassert=ON -Dwerr=ON -Dwextra=ON -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake ${SOURCE_DIR}

src/ripple/protocol/DPAuth.h

scottschurr · 2023-05-31T22:52:28Z

src/ripple/protocol/DPAuth.h

+
+// Wrapper class for DepositPreAuth Ledger Object. This class encapsulates the std::shared_ptr<SLE> to the actual ledger object.
+template <bool Writable>
+class DPAuthImpl final : public LedgerEntryWrapper<Writable>


Rename this class to DepPreAuthImpl.

scottschurr · 2023-05-31T22:52:59Z

src/ripple/protocol/DPAuth.h

+        return wrapped_->at(sfOwnerNode);
+    }
+};
+}


Given the class rename, please add these declarations to the file, but use the right names:

using DepPreAuthRd = DepPreAuthImpl<false>; using DepPreAuth = DepPreAuthImpl<true>;

After you've changed these names grep the source code for DPAuth. Fix up any instances that you find, either in source or in comments.

The reason I want to introduce the DepPreAuthRd and DepPreAuth names is because DepPreAuthImpl being a template is an implementation artifact. Scott D and were looking for a way to make sure the interfaces of DepPreAuthRd and DepPreAuth are identical where ever possible. And we didn't want to leave behind a maintenance problem. Using a base template for the two types was the best way we identified to accomplish that.

We want people to think of DepPreAuthRd and DepPreAuth as being two distinct types, just
like iterator and const_iterator are two distinct but related types.

src/ripple/app/tx/impl/DepositPreauth.cpp

scottschurr · 2023-06-01T00:20:32Z

src/ripple/protocol/Keylet.h

+template<bool>
+class DPAuthImpl;
+
+struct DPAuthKeylet final : public KeyletBase {
+    template <bool Writable>
+    using TWrapped = DPAuthImpl<Writable>;
+
+    using KeyletBase::check;
+
+    explicit DPAuthKeylet(uint256 const& key) : KeyletBase(ltDEPOSIT_PREAUTH, key)
+    {
+    }
+};
+


By the time everything is done there will be quite a number of these specialized Keylets in the file. I think it would be a good idea to (almost) alphabetize the Keylets. Put KeyletBase first, followed by Keylet. Then alphabetize the remaining Keylets as they are added.

okay. By alphabetize, you mean capitalize the first letter of the keylet? like DepPreAuthKeylet ?

By alphabetize, you mean capitalize the first letter of the keylet? like DepPreAuthKeylet ?

Sorry for not being clear. By the time you are done Keylet.h will contain a bunch of classes derived from KeyletBase. I'm suggesting that the file should keep (most) of those definitions in alphabetical order so they are easier for folks to find. For example, this file now contains class definitions in this order:

KeyletBase Keylet DepPreAuthKeylet AccountRootKeylet

I think it makes sense for KeyletBase and Keylet to stay at the top. But the remainder should be alphabetical. So, for this revision of the file they'd be in this order:

KeyletBase Keylet AccountRootKeylet DepPreAuthKeylet

It's not really important right now. But by the time you get to the end of the project there will be quite a number of Keylets in here, and they will be easier to find if they are alphabetical.

Make sense?

yes okay, I'll keep this in mind 👍

yeah, it will be easier to organize them in alphabetical order once all the PRs are merged into once place

src/ripple/protocol/Keylet.h

ckeshava · 2023-06-01T17:19:15Z

I'm really sorry that I didn't address your comments from last week. I got caught up in the other PRs and forgot to update this one, won't repeat it again.

scottschurr

Looks good, but a couple of notes:

The most recent changes were not clang-formatted. I did that and pushed the commit here: https://github.com/scottschurr/rippled/commits/chenna-depositPAuthWrapper You can pick that up if yo want to cherry-pick it. Or you can do the clang-format yourself.
While I was in there I moved DepPreAuthKeylet below AcctRootKeylet in Keylet.h. That will make it easier to alphabetize the entries as they are merged.
Somehow or other the bottom-most commit looks like it is now attributed to me, not to you. This is all mostly your work, and should be attributed to you. When you squash the commits for the pull request that you submit to develop would you please make sure that commit is attributed to you?

Thanks.

ckeshava · 2023-06-09T00:06:27Z

Thanks for the suggestions Scott! I've been manually formatting my commits, I need to set up a hook. I've cherry-picked your commit. Thanks for your kind words,I'm not able to figure out how to change the author to myself. I cherry-picked some of your commits, I think that might have caused you to be the author. I'm completely fine with it Regards, Keshava

…

On Thu, Jun 8, 2023 at 3:29 PM Scott Schurr ***@***.***> wrote: ***@***.**** approved this pull request. Looks good, but a couple of notes: 1. The most recent changes were not clang-formatted. I did that and pushed the commit here: https://github.com/scottschurr/rippled/commits/chenna-depositPAuthWrapper <https://urldefense.com/v3/__https://github.com/scottschurr/rippled/commits/chenna-depositPAuthWrapper__;!!PZTMFYE!_D50yGhU3CDeKk3WehGIFo5rX8mIfi6uxQInHx7f7rjhsyqOSc4wGeTuFbEiJSjHZ0VcJHcRLW2v4iRZ70sVwrxctQ$> You can pick that up if yo want to cherry-pick it. Or you can do the clang-format yourself. 2. While I was in there I moved DepPreAuthKeylet below AcctRootKeylet in Keylet.h. That will make it easier to alphabetize the entries as they are merged. 3. Somehow or other the bottom-most commit looks like it is now attributed to me, not to you. This is all mostly your work, and should be attributed to you. When you squash the commits for the pull request that you submit to develop would you please make sure that commit is attributed to you? Thanks. — Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/scottschurr/rippled/pull/18*pullrequestreview-1470854394__;Iw!!PZTMFYE!_D50yGhU3CDeKk3WehGIFo5rX8mIfi6uxQInHx7f7rjhsyqOSc4wGeTuFbEiJSjHZ0VcJHcRLW2v4iRZ70uzNKmqaw$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AFB4TNKCEBTGZRP477F4PVLXKJG6FANCNFSM6AAAAAAYODGB44__;!!PZTMFYE!_D50yGhU3CDeKk3WehGIFo5rX8mIfi6uxQInHx7f7rjhsyqOSc4wGeTuFbEiJSjHZ0VcJHcRLW2v4iRZ70tv3hN_CA$> . You are receiving this because you authored the thread.Message ID: ***@***.***>

scottschurr · 2023-06-09T00:44:56Z

Try something like this:

git commit --amend --author="John Doe <[email protected]>"

…Keylet is used to distinguish between various other types of SLEs update readSLE and peekSLE function calls associated with keylet::depositPreAuth into the new read, peek functions

ckeshava · 2023-06-09T01:03:48Z

That worked! Thanks scott! Regards, Keshava

…

On Thu, Jun 8, 2023 at 5:45 PM Scott Schurr ***@***.***> wrote: Try something like this: git commit --amend --author="John Doe ***@***.***>" — Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/scottschurr/rippled/pull/18*issuecomment-1583683693__;Iw!!PZTMFYE!_ZuK_8Wjz9EYDAZIXSTnc8PfO0kuZ58e-xpE9HEFoNOrnSaM5bRCO83vElzORf9WulVp47rQAIVzdcNI41nSn0AIMA$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AFB4TNPQZREI7DAVXG7X3WTXKJWZHANCNFSM6AAAAAAYODGB44__;!!PZTMFYE!_ZuK_8Wjz9EYDAZIXSTnc8PfO0kuZ58e-xpE9HEFoNOrnSaM5bRCO83vElzORf9WulVp47rQAIVzdcNI41m-lhYmGw$> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ckeshava force-pushed the depositPAuthWrapper branch from 413bcd1 to 6698e93 Compare May 25, 2023 00:45

scottschurr reviewed May 25, 2023

View reviewed changes

ckeshava force-pushed the depositPAuthWrapper branch from 6698e93 to 207daca Compare May 26, 2023 19:50

scottschurr mentioned this pull request May 26, 2023

Strongly Typed Ledger Object wrapper for DepositPreAuth #17

Closed

7 tasks

scottschurr requested changes Jun 1, 2023

View reviewed changes

scottschurr approved these changes Jun 8, 2023

View reviewed changes

ckeshava force-pushed the depositPAuthWrapper branch from dd39cd1 to f5e48bf Compare June 9, 2023 00:03

Introduce Strongly typed DepositPreAuth wrapper. A new DepositPreAuth…

a4bca7f

…Keylet is used to distinguish between various other types of SLEs update readSLE and peekSLE function calls associated with keylet::depositPreAuth into the new read, peek functions

ckeshava force-pushed the depositPAuthWrapper branch from f5e48bf to a4bca7f Compare June 9, 2023 01:03


		using KeyletBase::check;

		DPAuthKeylet(uint256 const& key) : KeyletBase(ltDEPOSIT_PREAUTH, key)


		namespace ripple {

		// Wrapper class for DepositPreAuth Ledger Object. This class encapsulates the std::shared_ptr<SLE> to the actual ledger object.

Strongly typed wrapper for DepositPreAuthorization Ledger Object #18

Are you sure you want to change the base?

Strongly typed wrapper for DepositPreAuthorization Ledger Object #18

Conversation

ckeshava commented May 25, 2023

High Level Overview of Change

Context of Change

Type of Change

Test Plan

scottschurr 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

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

ckeshava commented May 26, 2023

scottschurr 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

ckeshava commented Jun 1, 2023

scottschurr left a comment

Choose a reason for hiding this comment

ckeshava commented Jun 9, 2023 via email

scottschurr commented Jun 9, 2023

ckeshava commented Jun 9, 2023 via email