feat: [Hardhat] implement nft support for the json rpc api library and js mock … #173

arianejasuwienas · 2025-01-07T11:08:54Z

Description:
Implementing support for NFTs in the JS library for the JSON-RPC mock, as well as in the JSON-RPC server that utilizes this library.

The reason I've used persistent storage here:

We cannot determine which NFT numbers have been minted unless someone specifically queries them. This can be addressed in one of two ways:

Dynamic loading on first access: Load each NFT's details dynamically the first time its slot is queried. Once accessed, store information about the existence of this NFT. This is my current approach. However, this may result in a memory leak/optimisation when a large number of tests or NFT requests are made, as memory is never freed (this is where the name: persistentStorage comes from).
Preloading All NFTs: Load the details of all NFTs each time the token storage is accessed. While this avoids memory leaks, it could lead to excessive memory usage and generate a large number of unnecessary API requests for NFTs that we are not interested about :/ .

Why it might be an issue:

Let's consider scenario where:

We have a long tokenUri that spans more than one storage slot.
Someone attempts to access the value in the second slot without first querying the initial slot.

We won’t be able to provide the correct result. This is because persistentStorage is only populated when the request for the first slot of the string is made (only then we can deduce the serial NFT number from the slot address). I don't think it's not a critical issue though...

Related issue(s):

#80

Notes for reviewer:

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

acuarica

looking good, left some questions/comments

src/slotmap.js

acuarica · 2025-01-07T21:52:21Z

src/index.js

+        persistentSlotMapOf(tokenId).store(nrequestedSlot, atob(metadata));
+    }
+    let unresolvedValues = persistentSlotMapOf(tokenId).load(nrequestedSlot);


moreover, not sure this works as intended, since it's going to always write when asked for token uri.

When querying the first slot holding the tokenUri, it will store the values for all slots containing this single string.

test/data/CFNFTFF/getAllowanceForToken_0.0.4436613_0.0.4454450.json

acuarica · 2025-01-08T14:41:10Z

To improve the discussion regarding persistentSlotMapOf, please describe how it works in the PR.

…server (#80) Signed-off-by: Mariusz Jasuwienas <[email protected]>

Signed-off-by: Mariusz Jasuwienas <[email protected]>

acuarica · 2025-01-08T18:34:52Z

src/index.d.ts

+     * @param serialId The serial id of the NFT.
+     * @param blockNumber
+     */
+    getNftByTokenIdAndNumber(


nit: to keep the naming consistent

Suggested change

getNftByTokenIdAndNumber(

getNftByTokenIdAndSerial(

src/slotmap.js

acuarica · 2025-01-08T19:05:51Z

src/slotmap.js

+/**
+ * @type {Array<PersistentStorage>}
+ */
+const persistentStorage = [];


This is actually a Map, whose keys are addresses (and blockNumber, see below), right? We can use a Map or object to be more explicit and avoid doing the lookup below.

acuarica · 2025-01-08T19:14:07Z

src/slotmap.js

+/**
+ * @type {Array<PersistentStorage>}
+ */
+const persistentStorage = [];


I would move this global state to index.js.

acuarica · 2025-01-08T21:41:01Z

src/slotmap.js

+ * @returns {PersistentSlotMap} An object with `load` and `store` methods.
+ */
+function persistentSlotMapOf(target) {
+    const found = persistentStorage.filter(occupiedSlot => occupiedSlot.target === target);


what about blockNumber? The key should be target and blockNumber because the user can change the blockNumber.

Signed-off-by: Mariusz Jasuwienas <[email protected]>

mishomihov00

Review applies to .github/workflows/test.yml

…e index.js file, making the blocknumber part of its key (#80) Signed-off-by: Mariusz Jasuwienas <[email protected]>

arianejasuwienas · 2025-01-09T09:02:42Z

@acuarica Thank you for the review! I’ve applied your suggestions - please let me know if everything looks good now.

acuarica

looking good, final touches

acuarica · 2025-01-09T20:00:57Z

src/slotmap.js

-module.exports = { slotMapOf, packValues };
+/**
+ * Represents the value in the persistent storage.
+ * Each token has its own SlotMap. Any value can be assigned to this storage


Suggested change

* Each token has its own SlotMap. Any value can be assigned to this storage

* Each token and `blockNumber` has its own SlotMap. Any value can be assigned to this storage

acuarica · 2025-01-09T20:06:27Z

src/slotmap.js

+        const initialized = this._map.get(key) || new SlotMap();
+        if (!this._map.has(key)) {
+            this._map.set(key, initialized);
+        }
+        return initialized;


to avoid the double guard

Suggested change

const initialized = this._map.get(key) || new SlotMap();

if (!this._map.has(key)) {

this._map.set(key, initialized);

}

return initialized;

let slotMap = this._map.get(key);

if (slotMap === undefined) {

slotMap = new SlotMap();

this._map.set(key, slotMap);

}

return slotMap;

acuarica · 2025-01-09T20:07:12Z

src/slotmap.js

+     * @param {string} tokenId
+     * @param {number} blockNumber
+     * @param {bigint} slot
+     */


nit: let's add return type for clarity.

acuarica · 2025-01-09T20:09:24Z

src/slotmap.js

+     * @param {string} tokenId
+     * @param {number} blockNumber
+     */
+    _init(tokenId, blockNumber) {


Suggested change

_init(tokenId, blockNumber) {

_getSlotMap(tokenId, blockNumber) {

acuarica · 2025-01-09T23:41:04Z

src/slotmap.js

@@ -274,4 +274,54 @@ function slotMapOf(token) {
    return map;
 }

-module.exports = { slotMapOf, packValues };
+/**
+ * Represents the value in the persistent storage.


There is some information in the PR description that would be really useful here. In particular 1. of section "The reason I've used persistent storage here" and section "Why it might be an issue".

We should include that info as a JS doc here.

arianejasuwienas force-pushed the 80-add-nft-support-to-the-js-library branch from 53f31d6 to 0303268 Compare January 7, 2025 11:16

arianejasuwienas changed the title ~~feat: implement nft support for the json rpc api library and js mock …~~ feat: [JS Library] implement nft support for the json rpc api library and js mock … Jan 7, 2025

arianejasuwienas mentioned this pull request Jan 7, 2025

feat: [Foundry] implement nft support in hts emulation #172

Merged

2 tasks

arianejasuwienas self-assigned this Jan 7, 2025

arianejasuwienas added the New Feature A new feature, service, or documentation. Major changes that are not backwards compatible. label Jan 7, 2025

arianejasuwienas linked an issue Jan 7, 2025 that may be closed by this pull request

[Hardhat] Implement HTS state emulation for NFT methods #80

Open

arianejasuwienas marked this pull request as ready for review January 7, 2025 11:58

arianejasuwienas requested a review from a team as a code owner January 7, 2025 11:58

arianejasuwienas requested a review from acuarica January 7, 2025 11:58

arianejasuwienas changed the title ~~feat: [JS Library] implement nft support for the json rpc api library and js mock …~~ feat: [Hardhat] implement nft support for the json rpc api library and js mock … Jan 7, 2025

acuarica added feature Enhancing an existing feature driven by business requirements. Typically backwards compatible. and removed New Feature A new feature, service, or documentation. Major changes that are not backwards compatible. labels Jan 7, 2025

acuarica requested changes Jan 7, 2025

View reviewed changes

arianejasuwienas mentioned this pull request Jan 8, 2025

feat: removing unneeded flag (#80) #174

Closed

2 tasks

feat: implement nft support for the json rpc api library and js mock …

d6c17f3

…server (#80) Signed-off-by: Mariusz Jasuwienas <[email protected]>

arianejasuwienas force-pushed the 80-add-nft-support-to-the-js-library branch from 2d33834 to d6c17f3 Compare January 8, 2025 15:04

feat: enable mock server tests in the workflow (#80)

45996f4

Signed-off-by: Mariusz Jasuwienas <[email protected]>

arianejasuwienas requested review from a team as code owners January 8, 2025 15:06

arianejasuwienas requested a review from rbarkerSL January 8, 2025 15:06

acuarica reviewed Jan 8, 2025

View reviewed changes

fix: applying PR suggestion, renaming method (#80)

40eece1

Signed-off-by: Mariusz Jasuwienas <[email protected]>

mishomihov00 previously approved these changes Jan 9, 2025

View reviewed changes

fix: applying PR suggestion, moving persistent storage variable to th…

9f4940e

…e index.js file, making the blocknumber part of its key (#80) Signed-off-by: Mariusz Jasuwienas <[email protected]>

arianejasuwienas dismissed mishomihov00’s stale review via 9f4940e January 9, 2025 08:43

acuarica reviewed Jan 9, 2025

View reviewed changes

acuarica mentioned this pull request Jan 9, 2025

Convert getHtsStorageAt into a method to support persistent state #178

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [Hardhat] implement nft support for the json rpc api library and js mock … #173

feat: [Hardhat] implement nft support for the json rpc api library and js mock … #173

arianejasuwienas commented Jan 7, 2025 •

edited

Loading

acuarica left a comment

acuarica Jan 7, 2025

arianejasuwienas Jan 8, 2025

acuarica commented Jan 8, 2025

acuarica Jan 8, 2025

acuarica Jan 8, 2025 •

edited

Loading

acuarica Jan 8, 2025

acuarica Jan 8, 2025

mishomihov00 left a comment

arianejasuwienas commented Jan 9, 2025

acuarica left a comment

acuarica Jan 9, 2025

acuarica Jan 9, 2025

acuarica Jan 9, 2025

acuarica Jan 9, 2025

acuarica Jan 9, 2025

	* Each token has its own SlotMap. Any value can be assigned to this storage
	* Each token and `blockNumber` has its own SlotMap. Any value can be assigned to this storage

-        const initialized = this._map.get(key) || new SlotMap();
-        if (!this._map.has(key)) {
-            this._map.set(key, initialized);
-        }
-        return initialized;
+        let slotMap = this._map.get(key);
+        if (slotMap === undefined) {
+            slotMap = new SlotMap();
+            this._map.set(key, slotMap);
+        }
+        return slotMap;

	_init(tokenId, blockNumber) {
	_getSlotMap(tokenId, blockNumber) {

feat: [Hardhat] implement nft support for the json rpc api library and js mock … #173

Are you sure you want to change the base?

feat: [Hardhat] implement nft support for the json rpc api library and js mock … #173

Conversation

arianejasuwienas commented Jan 7, 2025 • edited Loading

The reason I've used persistent storage here:

Why it might be an issue:

acuarica left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acuarica commented Jan 8, 2025

Choose a reason for hiding this comment

acuarica Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishomihov00 left a comment

Choose a reason for hiding this comment

arianejasuwienas commented Jan 9, 2025

acuarica left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arianejasuwienas commented Jan 7, 2025 •

edited

Loading

acuarica Jan 8, 2025 •

edited

Loading