fixInnerObjTemplate2 amendment #5047
Conversation
Apply inner object templates to all remaining (non-AMM) inner objects. Adds a unit test for applying the template to sfMajorities. Other remaining inner objects showed no problems having templates applied.
scottschurr
added
Bug
Amendment
Perf impact not expected
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5047 +/- ##
=========================================
- Coverage 71.3% 71.3% -0.0%
=========================================
Files 796 796
Lines 66987 66902 -85
Branches 10892 10866 -26
=========================================
- Hits 47793 47703 -90
- Misses 19194 19199 +5
... and 1581 files with indirect coverage changes |
|
|
||
| // The if is complicated because inner object templates were added in | ||
| // two phases: | ||
| // 1. If there are no available Rules, then always apply the template. |
There was a problem hiding this comment.
Why should the template be applied if there is no Rules?
There was a problem hiding this comment.
@gregtatcam, great question.
The way getCurrentTransactionRules() works is:
- If we're currently inside a transaction then the
Rulesare set and available. - If this code is ever called when we're not inside a transaction, then the optional is unseated.
This means that if the optional is unseated then we're not inside a transaction, and any changes we make will not be transaction breaking. We have two choices we could make at this point:
- If we're not inside a transaction, stick with the old behavior.
- If we're not inside a transaction apply the new behavior.
In my opinion the new behavior (apply the template) is strictly safer than the old behavior (don't apply the template). So I'm choosing to bias the change to honor safety.
Thoughts?
There was a problem hiding this comment.
Thanks for the clarification. Applying the new behavior sounds right to me. One argument to use the new behavior is when a unit-test doesn't call a transaction but does call a function with this amendment, for instance some utility. We want to make sure that the unit-test fails in this case if there is a difference in before/after.
| std::optional<Rules> const& rules = getCurrentTransactionRules(); | ||
| bool const isAMMObj = name == sfAuctionSlot || name == sfVoteEntry; | ||
| if (!rules || (rules->enabled(fixInnerObjTemplate) && isAMMObj) || | ||
| (rules->enabled(fixInnerObjTemplate2) && !isAMMObj)) |
There was a problem hiding this comment.
This code was my only concern. Usually, when we introduce an amendment I want code where it's trivially clear that the code with the amendment switched off is not transaction breaking. That's not true here. I understand why it's not true, but that means we need to audit the old code to make sure all the old existing calls to makeInnerObject are for auction slots or vote entries.
I took a look at the old code and that does appear to be true. No action needed on this comment, I just wanted to add a note that I did that audit in case anyone wants to dummy check me.
There was a problem hiding this comment.
This block of code also got my attention and I did review the old existing calls to makeInnerObject to make sure it looked right.
|
Thanks for the reviews! This PR needs to go through the physical restructure re-mapping process before it can be merged. So I'll do the remapping soon. Once that is done I'll mark the PR as passed. |
…re-innerobj-templ-restructure
|
The physical restructure is done. Unit tests, start, sync, and graceful stop all work on my Mac for both debug and release builds. The CI problem with coverage looks like a host thing. I'm marking this pull request passed. |
scottschurr
added
the
Passed
* upstream/develop: (32 commits) fixInnerObjTemplate2 amendment (XRPLF#5047) Set version to 2.3.0-b1 Ignore restructuring commits (XRPLF#4997) Recompute loops (XRPLF#4997) Rewrite includes (XRPLF#4997) Rearrange sources (XRPLF#4997) Move CMake directory (XRPLF#4997) Add bin/physical.sh (XRPLF#4997) Prepare to rearrange sources: (XRPLF#4997) Change order of checks in amm_info: (XRPLF#4924) Add the fixEnforceNFTokenTrustline amendment: (XRPLF#4946) Replaces the usage of boost::string_view with std::string_view (XRPLF#4509) docs: explain how to find a clang-format patch generated by CI (XRPLF#4521) XLS-52d: NFTokenMintOffer (XRPLF#4845) chore: remove repeat words (XRPLF#5041) Expose all amendments known by libxrpl (XRPLF#5026) fixReducedOffersV2: prevent offers from blocking order books: (XRPLF#5032) Additional unit tests for testing deletion of trust lines (XRPLF#4886) Fix conan typo: (XRPLF#5044) Add new command line option to make replaying transactions easier: (XRPLF#5027) ...
* fixInnerObjTemplate2 amendment: Apply inner object templates to all remaining (non-AMM) inner objects. Adds a unit test for applying the template to sfMajorities. Other remaining inner objects showed no problems having templates applied. * Move CMake directory * Rearrange sources * Rewrite includes * Recompute loops --------- Co-authored-by: Pretty Printer <cpp@ripple.com>
High Level Overview of Change
Apply inner object templates to all remaining (non-AMM) inner objects.
Context of Change
It was discovered that the inner objects added to the code base for the AMM were not properly protected by their templates. That oversight was corrected by the
fixInnerObjTemplateamendment.An audit showed that there were additional inner objects that did not have their templates applied. This pull request applies templates to those additional inner objects.
Adds a unit test for applying the template to sfMajorities. Other remaining inner objects showed no problems having templates applied.
Type of Change