rippled
rippled copied to clipboard
XRPLF
XLS-52d: NFTokenMintOffer
High Level Overview of Change
Fixed to allow creating NFTokenOffer in a NFTokenMint transaction.
Added 3 optional fields to NFTokenMint transaction:
-
Amount -
Destination -
Expiration
Context of Change
- https://github.com/XRPLF/XRPL-Standards/discussions/147
Type of Change
- [x] New feature (non-breaking change which adds functionality)
- [x] Tests (you added tests for code that already exists, or your new feature included in this PR)
Still need to add some more tests.
- [x] Added fields
- [x] NFTokenOffer field created in NFTokenMint
- [x] offer_id added to meta in NFTokenMint
Codecov Report
Attention: Patch coverage is 97.83784% with 4 lines in your changes missing coverage. Please review.
Project coverage is 71.3%. Comparing base (
3f5e321) to head (7bc8302).
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
| Files | Coverage Δ | |
|---|---|---|
| src/ripple/app/tx/impl/NFTokenCreateOffer.cpp | 100.0% <100.0%> (+3.2%) |
:arrow_up: |
| src/ripple/app/tx/impl/NFTokenMint.cpp | 98.0% <100.0%> (+0.8%) |
:arrow_up: |
| src/ripple/app/tx/impl/details/NFTokenUtils.h | 100.0% <ø> (ø) |
|
| src/ripple/protocol/Feature.h | 100.0% <ø> (ø) |
|
| src/ripple/protocol/impl/Feature.cpp | 94.6% <ø> (ø) |
|
| src/ripple/protocol/impl/NFTokenOfferID.cpp | 86.4% <100.0%> (ø) |
|
| src/ripple/protocol/impl/TxFormats.cpp | 100.0% <ø> (ø) |
|
| src/ripple/app/tx/impl/details/NFTokenUtils.cpp | 93.4% <96.2%> (+1.2%) |
:arrow_up: |
... and 7 files with indirect coverage changes
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.
@shawnxie999, the most recent commit introduced significant changes. It would be good if you could re-review this pull request. Thanks.
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.
