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.

Sign up for GitHub

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

fix: Enable NFT Airdrop to distribute multiple serials #17413

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

fix: Enable NFT Airdrop to distribute multiple serials #17413

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ebfa72e
fix: Enable the nft airdrops to distribute multiple serials
stoyanov-st Jan 16, 2025
e5fa1c6
nit: typo
stoyanov-st Jan 17, 2025
f3c676c
fix: add tests to address PR comments
stoyanov-st Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions ...node/app/service/contract/impl/exec/systemcontracts/hts/airdrops/TokenAirdropDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2024 Hedera Hashgraph, LLC
* Copyright (C) 2024-2025 Hedera Hashgraph, LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -99,18 +99,22 @@ private TokenAirdropTransactionBody bodyForAirdrop(
tokenTransferList.transfers(aaList);
}
if (nftAmountsTuple.length > 0) {
final var nftAmount = nftAmountsTuple[0];
final var serial = (long) nftAmount.get(NFT_SERIAL);
final var sender = addressIdConverter.convert(nftAmount.get(NFT_SENDER));
final var receiver = addressIdConverter.convert(nftAmount.get(NFT_RECEIVER));
checkForSystemAccount(receiver);
final var isApproval = (boolean) nftAmount.get(NFT_IS_APPROVAL);
tokenTransferList.nftTransfers(NftTransfer.newBuilder()
.senderAccountID(sender)
.receiverAccountID(receiver)
.serialNumber(serial)
.isApproval(isApproval)
.build());
final var nftTransfersList = new ArrayList<NftTransfer>();
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to create a unit test for this? I realize the method is private but it seems we can maybe create tuples that have more than one not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test.

Arrays.stream(nftAmountsTuple).forEach(nftAmount -> {
final var serial = (long) nftAmount.get(NFT_SERIAL);
final var sender = addressIdConverter.convert(nftAmount.get(NFT_SENDER));
final var receiver = addressIdConverter.convert(nftAmount.get(NFT_RECEIVER));
checkForSystemAccount(receiver);
final var isApproval = (boolean) nftAmount.get(NFT_IS_APPROVAL);
final var nftTransfer = NftTransfer.newBuilder()
.senderAccountID(sender)
.receiverAccountID(receiver)
.serialNumber(serial)
.isApproval(isApproval)
.build();
nftTransfersList.add(nftTransfer);
});
tokenTransferList.nftTransfers(nftTransfersList);
}
transferBuilderList.add(tokenTransferList.build());
});
Expand Down
106 changes: 105 additions & 1 deletion ...om/hedera/services/bdd/suites/contract/precompile/airdrops/AirdropSystemContractTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2024 Hedera Hashgraph, LLC
* Copyright (C) 2024-2025 Hedera Hashgraph, LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -492,4 +492,108 @@ public Stream<DynamicTest> failToUpdateNFTsMetadata(
.andAssert(txn ->
txn.hasKnownStatuses(CONTRACT_REVERT_EXECUTED, INVALID_TOKEN_NFT_SERIAL_NUMBER)));
}

@HapiTest
@Order(12)
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity: What is it about airdrops, or this suite, that these tests must be @OrderedInIsolation? Each individual test looks like it is getting new accounts and tokens injected (or at least that's what I assume those annotations on test method parameters are) so how do they interfere with each other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though we have new tokens and accounts injected the naming logic of the specs is the same e.g. if we have receiver1 used in 2 separate tests getting the balance would most likely result in a race condition (and it did before adding the ordering) as we are accessing them by the name.

Copy link
Member

Choose a reason for hiding this comment

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

Is there something we could suggest to improve the bdd framework that could make it easier to write tests that didn't have this constraint? Becuase of course @Ordered reduces parallelism in test runs making them take more wall clock time, and @OrderedInIsolation is worse. If the framework could be improved multiple suites, including this one, could benefit, and CI runs would be faster.

Copy link
Contributor Author

@stoyanov-st stoyanov-st Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need to spend some time to brainstorm this to come up with a solution and adapt all affected suites

@DisplayName("Distribute NFTs to multiple accounts")
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps also a test w/ multiple airdrops but one or more fail? (I'm not even sure what the behavior of that should be, must be in the HIP though ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we have such tests done or incoming with future PR

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I would have thought this was a natural place for them, but ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test

public Stream<DynamicTest> distributeNfts(
@NonNull @NonFungibleToken(numPreMints = 3) final SpecNonFungibleToken nft,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver1,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver2,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver3) {
return hapiTest(withOpContext((spec, opLog) -> {
allRunFor(
spec,
sender.associateTokens(nft),
receiver1.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver2.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver3.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver1.getInfo(),
receiver1.getInfo(),
receiver3.getInfo(),
nft.treasury().transferNFTsTo(sender, nft, 1L, 2L, 3L));
allRunFor(
spec,
airdropContract
.call(
"nftAirdropDistribute",
nft,
sender,
prepareAccountAddresses(spec, receiver1, receiver2, receiver3))
.gas(1500000),
receiver1.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 1L)),
receiver2.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 1L)),
receiver3.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 1L)),
nft.serialNo(1L).assertOwnerIs(receiver1),
nft.serialNo(2L).assertOwnerIs(receiver2),
nft.serialNo(3L).assertOwnerIs(receiver3));
}));
}

@HapiTest
@Order(13)
@DisplayName("Cannot Distribute 11 NFTs to multiple accounts")
public Stream<DynamicTest> distributeNftsOutOfBound(
@NonNull @NonFungibleToken(numPreMints = 11) final SpecNonFungibleToken nft,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver1,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver2,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver3,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver4,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver5,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver6,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver7,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver8,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver9,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver10,
@NonNull @Account(maxAutoAssociations = -1) final SpecAccount receiver11) {
return hapiTest(withOpContext((spec, opLog) -> {
allRunFor(
spec,
sender.associateTokens(nft),
receiver1.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver2.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver3.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver4.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver5.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver6.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver7.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver8.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver9.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver10.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver11.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
nft.treasury().transferNFTsTo(sender, nft, 1L, 2L, 3L, 4L, 5L, 6L, 7L, 8L, 9L, 10L),
nft.treasury().transferNFTsTo(sender, nft, 11L));
allRunFor(
spec,
airdropContract
.call(
"nftAirdropDistribute",
nft,
sender,
prepareAccountAddresses(
spec,
receiver1,
receiver2,
receiver3,
receiver4,
receiver5,
receiver6,
receiver7,
receiver8,
receiver9,
receiver10,
receiver11))
.andAssert(txn -> txn.hasKnownStatuses(CONTRACT_REVERT_EXECUTED))
.gas(1500000),
receiver1.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver2.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver3.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver4.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver5.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver6.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver7.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver8.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)),
receiver9.getBalance().andAssert(balance -> balance.hasTokenBalance(nft.name(), 0L)));
}));
}
}
Loading
Loading