fix: Enable NFT Airdrop to distribute multiple serials #17413
Conversation
Signed-off-by: Stanimir Stoyanov <[email protected]>
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
...a/com/hedera/services/bdd/suites/contract/precompile/airdrops/AirdropSystemContractTest.java
Outdated
Show resolved
Hide resolved
| @@ -492,4 +492,108 @@ public Stream<DynamicTest> failToUpdateNFTsMetadata( | |||
| .andAssert(txn -> | |||
| txn.hasKnownStatuses(CONTRACT_REVERT_EXECUTED, INVALID_TOKEN_NFT_SERIAL_NUMBER))); | |||
| } | |||
|
|
|||
| @HapiTest | |||
| @Order(12) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think we need to spend some time to brainstorm this to come up with a solution and adapt all affected suites
|
|
||
| @HapiTest | ||
| @Order(12) | ||
| @DisplayName("Distribute NFTs to multiple accounts") |
There was a problem hiding this comment.
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 ...)
There was a problem hiding this comment.
I believe we have such tests done or incoming with future PR
There was a problem hiding this comment.
Ok. I would have thought this was a natural place for them, but ok.
Signed-off-by: Stanimir Stoyanov <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17413 +/- ##
============================================
+ Coverage 68.69% 68.99% +0.29%
- Complexity 22520 22771 +251
============================================
Files 2605 2619 +14
Lines 96927 98272 +1345
Branches 10101 10184 +83
============================================
+ Hits 66586 67800 +1214
- Misses 26535 26644 +109
- Partials 3806 3828 +22
... and 146 files with indirect coverage changes |
| .serialNumber(serial) | ||
| .isApproval(isApproval) | ||
| .build()); | ||
| final var nftTransfersList = new ArrayList<NftTransfer>(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Added the test.
Signed-off-by: Stanimir Stoyanov <[email protected]>
Description:
This PR fixes the limit of one NFT serial Airdropped with a single transaction.
WIth this logic we can now distribute a NFT to multiple receivers.
The configured limits for airdrops are still applied here.
Related issue(s):
Fixes #17409
Notes for reviewer:
Checklist