-
Notifications
You must be signed in to change notification settings - Fork 286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a get IRC27NFT Metadata guide #1583
Add a get IRC27NFT Metadata guide #1583
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice article :) just some suggestions regarding structure
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
I have addressed all the review suggestions. Please take a look and let me know if there are any further changes needed. |
I noticed this URL (../../../reference/magic-contract/ISCSandbox/#getirc27nftdata) on the page is causing the test/build to fail. Trying to get the right URL. Will send a new PR after fixing it |
aa001a1
to
62983d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great article ❤️
I would like to change it a little bit tho.
Let me first try to explain the "issue".
When you send an NFT on L1 to the L1 account of the chain, it gets automatically registered as ERC721 token in our EVM. But as your IRC27 token is not fully compatible with the tokenUri
of ERC721
which returns a json, our EVM creates that json out of the uri
, the description
and the name
of the IRC27 NFT on L1.
So normally people would never use the getIRC27NFTData
function. The would just call the standard tokenUri
of ERC721
.
So with that in mind, I think the first thing we should do, is is adding an admonition that tells people to use tokenUri if possible. Only if the need some of the unique metadata of IRC27 they should use this (or technically if they develop something themselves to transfer this NFTs or whatever).
This also means we are more interested in decoding the metadata.uri.
Btw you can see how this function is used to get the tokenUri here
So TL:DR;
- Add admonition pointing people to https://wiki.iota.org/isc/how-tos/core-contracts/nft/use-as-erc721/ and telling them about the tokenUri
- Rewrite encoding to actually decode the JSON from metadata.uri.
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.0.0-rc.6/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.1/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.1/docs/_partials/how-tos/token/_get-nft-metadata.md
Outdated
Show resolved
Hide resolved
62983d9
to
0f6293c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on slack I think we can get rid of the encoding stuff. Sorry for realising that soo late.
Also you need to change version 1.3. In 1.1 some of this things are not available yet.
So I would move it to version 1.3 and we can think if it makes sense to add a stripped down version to 1.1, but I think just adding it to 1.3 should be fine.
docs/build/isc/v1.1/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.1/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.1/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.1/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.1/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.1/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
docs/build/isc/v1.1/docs/how-tos/core-contracts/nft/get-nft-metadata.md
Outdated
Show resolved
Hide resolved
Ok, thanks for the suggestions. I'll implement them and send a new push request. |
0f6293c
to
81a63ec
Compare
@Dr-Electron, I have updated the doc to follow your suggestions. Please take a look and let me know. Thanks. |
@Ginowine, could you fix the broken links in the test workflow? |
@lucas-tortora, I fixed the link pointing to the mint-nft.md file. For the other link errors below, Kevin said the preview doesn’t work because my fork doesn’t know iota-wiki deployment secrets and that we can ignore it as long as the build workflow succeeds.
The below link was provided by Kevin.
It would be great if I could get the correct link to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should fix the build :)
Co-authored-by: Lucas Tortora <[email protected]>
Thank you so much, @lucas-tortora, for the suggestions on how to fix the build. Noted. |
docs/build/isc/v1.1/docs/how-tos/core-contracts/nft/get-nft-metadata.mdx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just pushed a formatting fix and added the page to the sidebar in version 1.3.
Once we move this example into the wasp repo I would prefer using the using for
pattern to import the asNFTID()
library function to be able to use it on variables directly. I prefer that syntax. It is way clearer imo
Description of change
This PR creates a new page titled "Get NFT Metadata". The page explains how to utilize the getIRC27NFTData function within a smart contract to fetch information about a specific IRC27 NFT on the IOTA Network
Links to any relevant issues
fixes #1571
Type of change
Change checklist