feat: Adds tests that verify IHRC904 airdropToken targeted to an EOA are functional when called by a contract #1185
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…evious PR Signed-off-by: Simeon Nakov <[email protected]>
simzzz
requested review from
georgi-l95,
Ivo-Yankov,
natanasow,
yiliev0 and
a team
as code owners
quiet-node
requested changes
Jan 17, 2025
| @@ -169,16 +169,14 @@ contract Airdrop is HederaTokenService { | |||
| // @param sender The address sending the NFTs | |||
| // @param receivers Array of addresses to receive the NFTs | |||
| // @return responseCode The response code from the airdrop operation (22 = success) | |||
| function nftAirdropDistribute(address token, address sender, address[] memory receivers) public payable returns (int64 responseCode) { | |||
| function nftAirdropDistribute(address token, address sender, address[] memory receivers, int64[] memory serials) public payable returns (int64 responseCode) { | |||
There was a problem hiding this comment.
The NatSpec is missing the new param, serials.
Comment on lines
+228
to
+229
| const HTS_SYSTEM_CONTRACT_ADDRESS = '0.0.359'; | ||
| const HAS_SYSTEM_CONTRACT_ADDRESS = '0.0.362'; |
There was a problem hiding this comment.
Should we call thse HTS_SYSTEM_CONTRACT_ID instead?
Comment on lines
+47
to
+52
| tokenCreateContract = await utils.deployContract( | ||
| Constants.Contract.TokenCreateContract | ||
| ); | ||
| tokenTransferContract = await utils.deployContract( | ||
| Constants.Contract.TokenTransferContract | ||
| ); |
There was a problem hiding this comment.
As discussed offline, let’s drop these changes and include them in a more focused PR in the future, as they are out of scope for this PR.
Let’s use utils.deployContract only for the contracts being tested in this PR to simplify the review process and clearly define the scope.
| const utils = require('../utils'); | ||
| const Constants = require('../../../constants'); | ||
|
|
||
| describe('HRC-904 AirdropContract Test Suite', function () { |
There was a problem hiding this comment.
So in the CI workflow got merged in couple of days ago, it shows that the test filter is HIP904, right? This suite should have the tag as HIP904 for it to be picked up by CI
| expect(nftOwner).to.equal(receiver); | ||
| }); | ||
|
|
||
| it.skip('should airdrop non-fungible tokens (NFT) to multiple accounts', async function () { |
There was a problem hiding this comment.
Why did we skip this again? If intended, let's add an explantory comment.
Comment on lines
-68
to
-201
| ); | ||
|
|
||
| return await ethers.getContractAt( | ||
| Constants.Contract.TokenCreateContract, | ||
| await tokenCreate.getAddress() | ||
| ); | ||
| } | ||
|
|
||
| static async deployTokenCreateCustomContract() { | ||
| const tokenCreateCustomFactory = await ethers.getContractFactory( | ||
| Constants.Contract.TokenCreateCustomContract | ||
| ); | ||
| const tokenCreateCustom = await tokenCreateCustomFactory.deploy( | ||
| Constants.GAS_LIMIT_1_000_000 | ||
| ); | ||
|
|
||
| return await ethers.getContractAt( | ||
| Constants.Contract.TokenCreateCustomContract, | ||
| await tokenCreateCustom.getAddress() | ||
| ); | ||
| } | ||
|
|
||
| static async deployTokenManagementContract() { | ||
| const tokenManagementFactory = await ethers.getContractFactory( | ||
| Constants.Contract.TokenManagementContract | ||
| ); | ||
| const tokenManagement = await tokenManagementFactory.deploy( | ||
| Constants.GAS_LIMIT_1_000_000 | ||
| ); | ||
|
|
||
| return await ethers.getContractAt( | ||
| Constants.Contract.TokenManagementContract, | ||
| await tokenManagement.getAddress() | ||
| ); | ||
| } | ||
|
|
||
| static async deployTokenQueryContract() { | ||
| const tokenQueryFactory = await ethers.getContractFactory( | ||
| Constants.Contract.TokenQueryContract | ||
| ); | ||
| const tokenQuery = await tokenQueryFactory.deploy( | ||
| Constants.GAS_LIMIT_1_000_000 | ||
| ); | ||
|
|
||
| return await ethers.getContractAt( | ||
| Constants.Contract.TokenQueryContract, | ||
| await tokenQuery.getAddress() | ||
| ); | ||
| } | ||
|
|
||
| static async deployTokenTransferContract() { | ||
| const tokenTransferFactory = await ethers.getContractFactory( | ||
| Constants.Contract.TokenTransferContract | ||
| ); | ||
| const tokenTransfer = await tokenTransferFactory.deploy( | ||
| Constants.GAS_LIMIT_1_000_000 | ||
| ); | ||
|
|
||
| return await ethers.getContractAt( | ||
| Constants.Contract.TokenTransferContract, | ||
| await tokenTransfer.getAddress() | ||
| ); | ||
| } | ||
|
|
||
| static async deployHRC719Contract() { | ||
| const hrcContractFactory = await ethers.getContractFactory( | ||
| Constants.Contract.HRC719Contract | ||
| ); | ||
| const hrcContract = await hrcContractFactory.deploy( | ||
| Constants.GAS_LIMIT_1_000_000 | ||
| ); | ||
|
|
||
| return await ethers.getContractAt( | ||
| Constants.Contract.HRC719Contract, | ||
| await hrcContract.getAddress() | ||
| ); | ||
| } | ||
|
|
||
| static async deployERC20Contract() { | ||
| const erc20ContractFactory = await ethers.getContractFactory( | ||
| Constants.Contract.ERC20Contract | ||
| ); | ||
| const erc20Contract = await erc20ContractFactory.deploy( | ||
| Constants.GAS_LIMIT_1_000_000 | ||
| ); | ||
|
|
||
| return await ethers.getContractAt( | ||
| Constants.Contract.ERC20Contract, | ||
| await erc20Contract.getAddress() | ||
| ); | ||
| } | ||
|
|
||
| static async deployERC721Contract() { | ||
| const erc721ContractFactory = await ethers.getContractFactory( | ||
| Constants.Contract.ERC721Contract | ||
| ); | ||
| const erc721Contract = await erc721ContractFactory.deploy( | ||
| Constants.GAS_LIMIT_1_000_000 | ||
| ); | ||
|
|
There was a problem hiding this comment.
Nice! I love this! great work 🙌 ! However, to keep the PR focused and within scope, let’s create a ticket and address these changes in a separate PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR adds tests that verify IHRC904 airdropToken targeted to an EOA are functional when called by a contract, by using the already existing smart contract. This test coverage was in a previous PR and was already reviewed once. Here we separated it from the example contracts so it didn't block the PR and the requested changes, mainly by @quiet-node were addressed.
Related issue(s):
Closes #1109
Notes for reviewer:
One of the tests is currently skipped due to a bug uncovered in the services repo, while testing. The test will fail until the issue is fixed, and a new issue will be created here to remove the skip once the issue in services is resolved.
hashgraph/hedera-services#17409
Checklist