XLS-52d: NFTokenMintOffer #4845
Conversation
|
Still need to add some more tests.
|
tequdev
force-pushed
the
featureNFTokenMintOffer
branch
from
e61e31f to
366db6a
Compare
tequdev
force-pushed
the
featureNFTokenMintOffer
branch
from
366db6a to
a7f374e
Compare
intelliot
requested review from
kennyzlei,
gregtatcam,
nbougalis and
shawnxie999
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4845 +/- ##
=========================================
- Coverage 71.3% 71.3% -0.0%
=========================================
Files 796 796
Lines 66900 66966 +66
Branches 10868 10890 +22
=========================================
+ Hits 47702 47739 +37
- Misses 19198 19227 +29
... and 7 files with indirect coverage changes |
tequdev
force-pushed
the
featureNFTokenMintOffer
branch
from
55b3ea2 to
b2e2d0d
Compare
tequdev
force-pushed
the
featureNFTokenMintOffer
branch
from
b2e2d0d to
65d62df
Compare
tequdev
force-pushed
the
featureNFTokenMintOffer
branch
2 times, most recently
from
4a780fa to
455b795
Compare
tequdev
force-pushed
the
featureNFTokenMintOffer
branch
from
455b795 to
6ff088e
Compare
kennyzlei
requested review from
scottschurr
and removed request for
kennyzlei and
gregtatcam
|
For the most part, this is a well done pull request. The tests seem to have good coverage of the new code. I have one serious concern with the implementation. A bunch of the code from NFTokenCreateOffer.cpp has been copied into NFTokenMint. That means when one part of the code changes for maintenance the copy must change as well. Inevitably the two copies will fall out of sync. That's not an immediate problem, but is absolutely a long term problem. One way in which this problem is blindingly apparent is when @shawnxie999 asked that you include code for an amendment that is in review. Optimally that fix should not need to be coded in two different places. One way to address this concern is to share the code, rather than copy it. If the common code is shared, for example by calling a shared routine, then future maintenance will still only need to happen in one place: in the shared code. I wanted to make sure this was possible before I suggested this change. So I have an example of how this code sharing might be done. The top-most commit here shows one way it might be implementated: https://github.com/scottschurr/rippled/commits/tequ-featureNFTokeMintOffer/ You are welcome to cherry-pick that commit if you want. But also feel free to take a different path to a similar solution. If you do cherry-pick that commit then I should probably recuse myself from the remainder of this code review, since I would be blind to mistakes that I might have introduced. I hope that helps. |
|
Thanks, @scottschurr! |
|
@shawnxie999, the most recent commit introduced significant changes. It would be good if you could re-review this pull request. Thanks. |
scottschurr
requested review from
HowardHinnant
and removed request for
nbougalis and
scottschurr
|
I think this pull request looks good, but my view is colored by my own changes to the pull request. So I'm removing myself as a reviewer. @HowardHinnant has agreed to review this pull request in my place, although he's tied up at the moment. He can hopefully get to the review in the next week or two. Sorry for the additional delay. |
|
Needs rebasing against current develop to resolve conflicts. Doing so will also help me see how this PR would change develop. |
| @@ -197,7 +197,10 @@ NFTokenMint::preclaim(PreclaimContext const& ctx) | |||
| auto const nftIssuer = | |||
| ctx.tx[~sfIssuer].value_or(ctx.tx[sfAccount]); | |||
|
|
|||
| if (!ctx.view.exists(keylet::line(nftIssuer, amount->issue()))) | |||
| // If the IOU issuer and the NFToken issuer are the same, | |||
There was a problem hiding this comment.
The commit message should be prefixed with [FOLD] to indicate that this commit should be merged (likely with the previous commit).
| @@ -110,10 +110,10 @@ NFTokenMint::preflight(PreflightContext const& ctx) | |||
| } | |||
| } | |||
|
|
|||
| if (auto exp = ctx.tx[~sfExpiration]; exp == 0) | |||
There was a problem hiding this comment.
The commit message should be prefixed with [FOLD] to indicate that this commit should be merged (likely with the previous commit).
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) ...
High Level Overview of Change
Fixed to allow creating NFTokenOffer in a NFTokenMint transaction.
Added 3 optional fields to NFTokenMint transaction:
AmountDestinationExpirationContext of Change
Type of Change