feat: getnftinfo will return its metadata and creation time#225
feat: getnftinfo will return its metadata and creation time#225arianejasuwienas wants to merge 5 commits intomainfrom
Conversation
7134471 to
012735b
Compare
acuarica
left a comment
There was a problem hiding this comment.
Looking good, thanks for sending this.
contracts/HtsSystemContractJson.sol
Outdated
| string memory metadata = mirrorNode().getNftMetadata(address(this), serialId); | ||
| string memory createdTimestamp = mirrorNode().getNftCreatedTimestamp(address(this), serialId); |
There was a problem hiding this comment.
we should unify these two calls to the MN into one, maybe something like getNftMetadataAndCreatedTimestamp, and extract both fields.
src/index.js
Outdated
| tokenId, | ||
| blockNumber, | ||
| nrequestedSlot, | ||
| atob(metadata), |
There was a problem hiding this comment.
Let's use Buffer.from(metadata, 'base64').toString() to be consistent with similar code below.
src/index.js
Outdated
| `Failed to get the metadata of the NFT ${tokenId}#${serialId}` | ||
| ); | ||
| const timestamp = Number(created_timestamp.split('.')[0]).toString(16).padStart(64, '0'); | ||
| const metadataEncoded = Buffer.from(metadata).toString('hex'); |
There was a problem hiding this comment.
did you mean
| const metadataEncoded = Buffer.from(metadata).toString('hex'); | |
| const metadataEncoded = Buffer.from(metadata, 'base64').toString('hex'); |
?
There was a problem hiding this comment.
No, the metadata is just a raw, encoded value - it can be any string. I'm simply calculating the bytes of the string without doing anything else to it. @acuarica
18fb2c1 to
1f1b731
Compare
|
@acuarica Thank you for review! I've applied your suggestions (except for this one: #225 (comment), where I've left a comment). |
| const metadataEncoded = Buffer.from(metadata, 'utf-8').toString('hex'); | ||
| const metadataLength = metadata.length.toString(16).padStart(64, '0'); | ||
| const stringTypeIndicator = '40'.padStart(64, '0'); | ||
| const bytes = `${timestamp}${stringTypeIndicator}${metadataLength}${metadataEncoded}`; |
There was a problem hiding this comment.
how does this work? does this request occupy multiple slots? Do we have a unit test in ERC721.t.sol to cover this? If not, we should add one.
There was a problem hiding this comment.
Below are the contents of the memory starting at this slot:
It is a tuple containing an int64 followed by a string, which means it may occupy more than one slot. Note that IERC721 does not use it; instead, tokenURI is stored in a separate slot. I haven't changed that for now because it makes sense - we avoid having to decode the metadata value every time tokenURI is queried. Let me know if we want to change that.
There was a problem hiding this comment.
Here is this implementation with dedicated tokenUri slot removed:
One test would need to be rewritten, since we have no byte comparating function prepared yet to handle this operation; since I'm not sure if we want to go this way I've skipped this test instead of rewriting it for now.
8d9e7d8 to
e362dc8
Compare
Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
… docs (#198) Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
e362dc8 to
5d88244
Compare
|
@arianejasuwienas marked this as draft because moving forward we should also include validation tests. |
Description:
A call to the getNonFungibleTokenInfo method on the HTS address (0x167) will now correctly populate the "metadata" and "creationTime" fields.
Related issue(s):
Fixes #198
Notes for reviewer:
Until now, these fields were mocked because they were not supported by IERC721 and required a direct implementation.
Checklist