search

XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.48k stars 1.44k forks source link

XLS-46: DynamicNFT #5048

Open tequdev opened 1 week ago

tequdev commented 1 week ago

Replace from https://github.com/XRPLF/rippled/pull/4838 Co-Author: @xVet

High Level Overview of Change

Spec: XLS-46d: Dynamic Non Fungible Tokens (dNFTs)

This Amendment adds functionality to update the URI of NFToken objects by adding:

NFTokenModify: Transactor to initiate the altering of the URI lsfMutable: Flag to be set in a NFTokenMint transaction to allow NFTokenModify to execute successfully on given NFT.

Context of Change

https://github.com/XRPLF/XRPL-Standards/discussions/130

Type of Change

API Impact

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 89.47368% with 6 lines in your changes missing coverage. Please review.

Project coverage is 71.4%. Comparing base (9fec615) to head (47106cc).

:exclamation: Current head 47106cc differs from pull request most recent head e9dd06c

Please upload reports for the commit e9dd06c to get more accurate results.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/5048/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) ```diff @@ Coverage Diff @@ ## develop #5048 +/- ## ========================================= + Coverage 71.3% 71.4% +0.1% ========================================= Files 796 798 +2 Lines 66989 67023 +34 Branches 10890 10865 -25 ========================================= + Hits 47757 47821 +64 + Misses 19232 19202 -30 ``` | [Files](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) | Coverage Δ | | |---|---|---| | [src/ripple/app/tx/impl/NFTokenMint.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Ftx%2Fimpl%2FNFTokenMint.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvdHgvaW1wbC9ORlRva2VuTWludC5jcHA=) | `98.0% <100.0%> (ø)` | | | [src/ripple/app/tx/impl/NFTokenModify.h](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Ftx%2Fimpl%2FNFTokenModify.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvdHgvaW1wbC9ORlRva2VuTW9kaWZ5Lmg=) | `100.0% <100.0%> (ø)` | | | [src/ripple/app/tx/impl/applySteps.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Ftx%2Fimpl%2FapplySteps.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvdHgvaW1wbC9hcHBseVN0ZXBzLmNwcA==) | `86.6% <100.0%> (ø)` | | | [src/ripple/app/tx/impl/details/NFTokenUtils.h](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Ftx%2Fimpl%2Fdetails%2FNFTokenUtils.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvdHgvaW1wbC9kZXRhaWxzL05GVG9rZW5VdGlscy5o) | `100.0% <ø> (ø)` | | | [src/ripple/protocol/Feature.h](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&filepath=src%2Fripple%2Fprotocol%2FFeature.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9wcm90b2NvbC9GZWF0dXJlLmg=) | `100.0% <ø> (ø)` | | | [src/ripple/protocol/impl/Feature.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&filepath=src%2Fripple%2Fprotocol%2Fimpl%2FFeature.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9wcm90b2NvbC9pbXBsL0ZlYXR1cmUuY3Bw) | `94.6% <ø> (ø)` | | | [src/ripple/protocol/impl/TxFormats.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&filepath=src%2Fripple%2Fprotocol%2Fimpl%2FTxFormats.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9wcm90b2NvbC9pbXBsL1R4Rm9ybWF0cy5jcHA=) | `100.0% <100.0%> (ø)` | | | [src/ripple/protocol/nft.h](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&filepath=src%2Fripple%2Fprotocol%2Fnft.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9wcm90b2NvbC9uZnQuaA==) | `100.0% <ø> (ø)` | | | [src/ripple/app/tx/impl/details/NFTokenUtils.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Ftx%2Fimpl%2Fdetails%2FNFTokenUtils.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvdHgvaW1wbC9kZXRhaWxzL05GVG9rZW5VdGlscy5jcHA=) | `93.0% <77.8%> (ø)` | | | [src/ripple/app/tx/impl/NFTokenModify.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Ftx%2Fimpl%2FNFTokenModify.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvdHgvaW1wbC9ORlRva2VuTW9kaWZ5LmNwcA==) | `90.0% <90.0%> (ø)` | | ... and [1584 files with indirect coverage changes](https://app.codecov.io/gh/XRPLF/rippled/pull/5048/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/5048/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/5048?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)
shawnxie999 commented 1 week ago

In other PR, I suggested adding a check so that NFTokenModify would fail if there is any outstanding offers. This is to prevent the issuer from changing the URI and then accept an existing offer.

tequdev commented 1 week ago

In other PR, I suggested adding a check so that NFTokenModify would fail if there is any outstanding offers. This is to prevent the issuer from changing the URI and then accept an existing offer.

As mentioned in this comment, wouldn't that make sense since the issuer can change the URI after the user has bought it? Users need to be aware of the risk to mutable NFTs, just as they need to be aware of the possibility of being burned by the issuer after purchasing a Burnable NFT.

https://github.com/XRPLF/rippled/pull/4838#issuecomment-1994709096

xVet commented 1 week ago

Correct, if you own a dynamic NFT then you agree that it can be changed by the issuer. I wouldn't personally put a check on offers, because this might cut into some use cases people come up with.

shawnxie999 commented 1 week ago

BTW, the "Context of change" section linked to the XLS20 proposal, I think you meant to link to this one https://github.com/XRPLF/XRPL-Standards/discussions/130

tequdev commented 6 days ago

BTW, the "Context of change" section linked to the XLS20 proposal, I think you meant to link to this one XRPLF/XRPL-Standards#130

good catch, fixed!

tequdev commented 2 hours ago

@scottschurr Thanks for the review. I fully agree with your suggestions and have cherry-picked all three commits.