Skip to content
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

feat: example usage in nft testing of our foundry library and hardhat plugin (#191) #209

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

arianejasuwienas
Copy link
Contributor

@arianejasuwienas arianejasuwienas commented Jan 21, 2025

Description:

  1. Basic usage of our Foundry library and Hardhat plugin has been added for NFTs. Users can now explore how our solution can be utilized in examples.
  2. Adding support for royalties in both hardhat and foundry (it did not work).

Related issue(s):

#191

Notes for reviewer:

Checklist

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

@arianejasuwienas arianejasuwienas added the feature Enhancing an existing feature driven by business requirements. Typically backwards compatible. label Jan 21, 2025
@arianejasuwienas arianejasuwienas self-assigned this Jan 21, 2025
@arianejasuwienas arianejasuwienas requested a review from a team as a code owner January 21, 2025 13:27
src/utils.js Outdated Show resolved Hide resolved
async getNftByTokenIdAndSerial(tokenId, serialId, blockNumber) {
const timestamp = await this.getBlockQueryParam(blockNumber);
return this._get(`tokens/${tokenId}/nft/${serialId}?${timestamp}`);
async getNftByTokenIdAndSerial(tokenId, serialId, _blockNumber) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Previous url was not correct.
  2. It does not support the block numbers ;/ . That is why I'm not checking if the token belongs to any specific account in my tests - it can change the ownership in time.

@arianejasuwienas arianejasuwienas force-pushed the 191-nft-examples-hardhat-foundry branch from 1a56b4a to 5d8b6f7 Compare January 21, 2025 13:32
@arianejasuwienas
Copy link
Contributor Author

A lot of tests started to fail because of the:

     Error: the string "Warning: This is a nightly build of Foundry. It is recommended to use the latest stable version. Visit https://book.getfoundry.sh/announcements for more information. \nTo mute this warning set `FOUNDRY_DISABLE_NIGHTLY_WARNING` in your environment. \n\n" was thrown, throw an Error :)

Should we set this env variable inside the runner? Or update the foundry version used in the runner?

@acuarica
Copy link
Contributor

acuarica commented Jan 21, 2025

Or update the foundry version used in the runner?

@arianejasuwienas

That would be better. However, we need to wait until the action we use support the stable version.

Should we set this env variable inside the runner?

Yes, as a workaround. Here's the PR to fix it #214.

@arianejasuwienas arianejasuwienas force-pushed the 191-nft-examples-hardhat-foundry branch from 5d8b6f7 to 01edc2b Compare January 22, 2025 10:41
@arianejasuwienas arianejasuwienas requested a review from a team as a code owner January 22, 2025 10:41
Signed-off-by: Mariusz Jasuwienas <[email protected]>
Signed-off-by: Mariusz Jasuwienas <[email protected]>
@arianejasuwienas arianejasuwienas marked this pull request as draft January 22, 2025 11:14
…HTSJson when the fallback_fee is null (#191)

Signed-off-by: Mariusz Jasuwienas <[email protected]>
Signed-off-by: Mariusz Jasuwienas <[email protected]>
Signed-off-by: Mariusz Jasuwienas <[email protected]>
Signed-off-by: Mariusz Jasuwienas <[email protected]>
@arianejasuwienas arianejasuwienas marked this pull request as ready for review January 22, 2025 14:14
@@ -201,6 +201,16 @@ contract HtsSystemContractJson is HtsSystemContract {
return tokenInfo;
}

// In order to properly decode the bytes returned by the parseJson into the Solidity Structure, the full,
// correct structure has to be provided in the input json, with all of the corresponding fields.
function _sanitizeFeesStructure(string memory json) private pure returns (string memory) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fallback_fee is nullable in the json response from the mirror node, but abi decode requires it to be set in order to create proper structure. That is why I set the default value here when it is empty.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, but also there is a vm.parseJson(json, ".custom_fees.royalty_fees") that guards against null, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acuarica Only when royalty_fees is null. For the barkaneer I had fallback_fee = null (substructure in the royalty fees object).

// https://hashscan.io/mainnet/token/0.0.4970613
const nft = await getContractAt('IERC721', '0x00000000000000000000000000000000004bd875');
expect(await nft['ownerOf'](1)).to.not.be.equal(
'0x0000000000000000000000000000000000000000'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we compare against the current owner?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can. But we get nft details for the latest block, so all it will take for our tests then is someone who owns this serial id = 1 to transfer it to someone else. (I'll apply your suggestion anyway :) ).

examples/hardhat-hts/test/nft.test.js Show resolved Hide resolved
"version": "0.0.1",
"resolved": "git+ssh://[email protected]/hashgraph/hedera-forking.git#daa80f304e396b35ab84135176ac6e48b8cab35f",
"version": "0.1.1",
"resolved": "git+ssh://[email protected]/hashgraph/hedera-forking.git#8533630770865fc5e3ae0341aee2b71b7d229350",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's install from npm (instead of git) so it's closer to a what an actual user would use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I am getting it directly from npm, @acuarica . Unfortunately current version (0.1.1) won't work for this token anyway. Because it has royalty fees (and because of the incorrrect nft details path).

Perhaps we should split it in 2 PRs.

First: #218

And then, when the library is fixed we can update this package lock.

OR

We can merge this PR and create a new one soon after, with updated dependency (it's up to you). I like this idea even more, because then we would have the fix in the solidity smart contract too (for nullable substructure in royalty fees).

@@ -201,6 +201,16 @@ contract HtsSystemContractJson is HtsSystemContract {
return tokenInfo;
}

// In order to properly decode the bytes returned by the parseJson into the Solidity Structure, the full,
// correct structure has to be provided in the input json, with all of the corresponding fields.
function _sanitizeFeesStructure(string memory json) private pure returns (string memory) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, but also there is a vm.parseJson(json, ".custom_fees.royalty_fees") that guards against null, right?

@@ -9,7 +9,7 @@
"author": "2024 Hedera Hashgraph, LLC",
"license": "Apache-2.0",
"devDependencies": {
"@hashgraph/system-contracts-forking": "hashgraph/hedera-forking",
"@hashgraph/system-contracts-forking": "^0.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned offline, given some fixes are part of this PR, for now we can continue using the package here from GitHub directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Enhancing an existing feature driven by business requirements. Typically backwards compatible.
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

[Hardhat] Is is impossible to access the NFT data or royalty fees
2 participants