Replace dirAdd() calls with dirAppend() or dirInsert() #3830
Conversation
| "Call dirInsert() instead."); | ||
| return std::nullopt; | ||
| } | ||
| return dirAdd(true, directory, key.key, describe); |
There was a problem hiding this comment.
The "High Level Overview of Change" in the PR threw me off at first since it says "remove the calls to dirAdd()". Perhaps it should clarify what "dirAdd()" should be deleted?
There was a problem hiding this comment.
I appreciate the assert to try to keep us from doing anything weird, but I'm not sure it's a good idea. This function is intended to append to the particular directory, instead of trying to fit it into the first page with an empty slot. It's true that this operation is currently only used by order books (so as to maintain temporal ordering of offers), but there's nothing that prevents this from being used differently, so I think the assert my communicate the wrong information.
For example, imagine if we added a new directory for, say, NFT offers.
There was a problem hiding this comment.
At @nbougalis, I think the assert is a good idea because it helps keep folks less familiar with the code on the straight and narrow. To someone who is not intimate with this code, it may not be obvious whether a particular object should be added or appended to the directory. As it stands there's currently only one situation where append is appropriate: offers in book directories. All other cases use add.
In the future there may, indeed, be other situations where appending is the right thing to do. But the source code is malleable, so the assert can be removed or changed when the new situation arises.
Just my take...
| @@ -186,7 +186,8 @@ CreateCheck::doApply() | |||
| // Note that we we use the value from the sequence or ticket as the | |||
| // Check sequence. For more explanation see comments in SeqProxy.h. | |||
| std::uint32_t const seq = ctx_.tx.getSeqProxy().value(); | |||
| auto sleCheck = std::make_shared<SLE>(keylet::check(account_, seq)); | |||
| Keylet const checkKeylet = keylet::check(account_, seq); | |||
There was a problem hiding this comment.
Since you are in the neighborhood, please correct the comment by removing the extra 'we'.
| describeOwnerDir(dstAccountId), | ||
| viewJ); | ||
| checkKeylet, | ||
| describeOwnerDir(dstAccountId)); |
There was a problem hiding this comment.
Nit: it may be worth replacing sleCheck->key() in the log calls below with the new local checkKeylet.key.
| describeOwnerDir(account_), | ||
| viewJ); | ||
| ticketKeylet, | ||
| describeOwnerDir(account_)); |
There was a problem hiding this comment.
Nit: it may be worth replacing sleTicket->key() in the log call below with the new local ticketKeylet.key.
|
@miguelportilla, thanks for the careful review. I made your suggested changes in this commit: c2a3b9f Let me know if you're good with those changes. If you are, and the branch passes CI, then I'll squash and mark the pull request as passed. Thanks. |
| "Call dirInsert() instead."); | ||
| return std::nullopt; | ||
| } | ||
| return dirAdd(true, directory, key.key, describe); |
There was a problem hiding this comment.
I appreciate the assert to try to keep us from doing anything weird, but I'm not sure it's a good idea. This function is intended to append to the particular directory, instead of trying to fit it into the first page with an empty slot. It's true that this operation is currently only used by order books (so as to maintain temporal ordering of offers), but there's nothing that prevents this from being used differently, so I think the assert my communicate the wrong information.
For example, imagine if we added a new directory for, say, NFT offers.
|
@scottschurr Changes look great, thank you. |
scottschurr
added
the
Ready to merge
High Level Overview of Change
Just tidying up some unfinished business here: #2516
Calls to
dirDelete()had been removed in a previous commit. Now that theSortedDirectoriesamendment is unconditionally enabled, it's okay to remove the calls todirAdd(). Once alldirAdd()calls are removed, thendirAdd()itself can be removed. That's what we do here.Type of Change
This change is purely a refactor and should have no effect on functionality. So the change should not need any mention in the release notes.