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

Jump to bottom

feat: Airdrops with treasury and custom fee receivers #14455

Merged

vtronkov merged 10 commits into 13658-add-TokenAirdropHandler-additional-tests from 14423-14443-airdrops-with-treasury-and-custom-fee-recivers

Aug 2, 2024

Contributor

vtronkov commented Jul 29, 2024 •

edited

Loading

Description:

When the receiver is a treasury account or custom fee collector we want to skip the custom fee validation because these users are exempt from paying them.
We also exempt users from paying if the allCollectorsExempt flag is true and the receiver is a fee collector to a token from the same type

Related issue(s):

Fixes #14423
Fixes #14443
Fixes #14479
Fixes #14485

vtronkov added 2 commits

July 29, 2024 17:35


          feat: exempt custom fee validation if the receiver is the treasury or…

432b337

… the custom fee collector

Signed-off-by: Valentin Tronkov <99957253+vtronkov@users.noreply.github.com>


          test: add HAPI test if the receiver is treasury or custom fee collector

c34315f

Signed-off-by: Valentin Tronkov <99957253+vtronkov@users.noreply.github.com>

vtronkov self-assigned this

vtronkov marked this pull request as ready for review

July 29, 2024 14:39

vtronkov requested a review from a team

July 29, 2024 14:39

vtronkov requested review from a team as code owners

July 29, 2024 14:39

vtronkov requested review from Neeharika-Sompalli and thomas-swirlds-labs

July 29, 2024 14:39

vtronkov added this to the v0.54 milestone

This was linked to issues Jul 29, 2024

(AIRDROP - 07) Sender airdrops a token with a custom fee to be paid by the receiver to the fee collector account for the token fee #14423

Closed

(AIRDROP - 09) Sender airdrops a token with a custom fee to be paid by the receiver to the token treasury account #14443

Closed

povolev15 approved these changes

View reviewed changes

Contributor

povolev15 left a comment

LGTM TY @vtronkov

povolev15 added the Token Airdrop label

Contributor

povolev15 commented Jul 29, 2024

please rerun spotless


          nit: spotless

a947cfa

Signed-off-by: Valentin Tronkov <99957253+vtronkov@users.noreply.github.com>

Contributor Author

vtronkov commented Jul 29, 2024

please rerun spotless

Oops. Done :)

david-bakin-sl reviewed

View reviewed changes

Member

david-bakin-sl left a comment

Should have been a unit test that covered this aspect of handle that, now, would fail unless fixed. Sorry I missed that earlier. Not too late to add unit tests to TokenAirdropHandlerTest. (Especially if refactored according to comments below.)

...-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/TokenAirdropHandler.java

                           boolean shouldExecuteCryptoTransfer = false;
                           final var transferListBuilder = TokenTransferList.newBuilder().token(tokenId);
                           // process fungible token transfers if any
                           if (!xfers.transfers().isEmpty()) {
+                              for (var transfer : xfers.transfers()) {

Member

david-bakin-sl Jul 29, 2024

Consider moving this entire loop into a method - or the body of the loop (or both). Would make it possible to unit test.

Same at L222-227 for nfts.

This method handle is incredibly complex - far too complex for reasonable testing. THis code here is a loop in a conditional in a loop. 140 lines in one method ... not a good idea.

Contributor Author

vtronkov Jul 30, 2024

I agree, but since this method is implemented in #14279 - we will refactor it there 👍

Member

david-bakin-sl Jul 30, 2024

ah, ok! thank you!

...-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/TokenAirdropHandler.java Outdated Show resolved Hide resolved

...-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/TokenAirdropHandler.java Outdated Show resolved Hide resolved

...-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/TokenAirdropHandler.java Outdated Show resolved Hide resolved

github-actions bot commented Jul 29, 2024 •

edited

Loading

Node: Unit Test Results

1 546 files 1 546 suites 3h 25m 56s ⏱️
117 464 tests 117 403 ✅ 59 💤 2 ❌
125 678 runs 125 617 ✅ 59 💤 2 ❌

For more details on these failures, see this check.

Results for commit 64a01b6.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 29, 2024 •

edited

Loading

Node: HAPI Test (Node Death Reconnect) Results

3 tests 3 ✅ 5m 1s ⏱️
3 suites 0 💤
3 files 0 ❌

Results for commit 64a01b6.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 29, 2024 •

edited

Loading

Node: HAPI Test (Token) Results

22 files 22 suites 7m 0s ⏱️
281 tests 281 ✅ 0 💤 0 ❌
364 runs 364 ✅ 0 💤 0 ❌

Results for commit 64a01b6.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 29, 2024 •

edited

Loading

Node: HAPI Test (Restart) Results

7 files 7 suites 8m 50s ⏱️
6 tests 6 ✅ 0 💤 0 ❌
7 runs 7 ✅ 0 💤 0 ❌

Results for commit 64a01b6.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 29, 2024 •

edited

Loading

Node: HAPI Test (Crypto) Results

32 files 32 suites 15m 44s ⏱️
364 tests 357 ✅ 0 💤 7 ❌
396 runs 389 ✅ 0 💤 7 ❌

For more details on these failures, see this check.

Results for commit 64a01b6.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 29, 2024 •

edited

Loading

Node: HAPI Test (Misc) Results

61 files 61 suites 16m 1s ⏱️
319 tests 318 ✅ 0 💤 1 ❌
409 runs 408 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 64a01b6.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 29, 2024 •

edited

Loading

Node: HAPI Test (Time Consuming) Results

19 tests 19 ✅ 23m 1s ⏱️
4 suites 0 💤
4 files 0 ❌

Results for commit 64a01b6.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 29, 2024 •

edited

Loading

Node: HAPI Test (Smart Contract) Results

91 files 91 suites 25m 5s ⏱️
658 tests 658 ✅ 0 💤 0 ❌
835 runs 835 ✅ 0 💤 0 ❌

Results for commit 64a01b6.

♻️ This comment has been updated with latest results.

vtronkov added 2 commits

July 29, 2024 19:52


          refactor: use an existing method instead of a new implementation

8b9d912

Signed-off-by: Valentin Tronkov <99957253+vtronkov@users.noreply.github.com>


          test: add hapi tests for airdrop with allCollectorsExempt

a1ba296

Signed-off-by: Valentin Tronkov <99957253+vtronkov@users.noreply.github.com>

vtronkov linked an issue

that may be closed by this pull request

(AIRDROP - 08) Sender airdrops a token with a custom fee to be paid by the receiver to the fee collector account of another token fee when all fee collector accounts are exempt #14479

Closed

MrValioBg reviewed

View reviewed changes

...-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/TokenAirdropHandler.java Outdated Show resolved Hide resolved


          refactor: skipCustomFeeValidation

08745aa

Signed-off-by: Valentin Tronkov <99957253+vtronkov@users.noreply.github.com>

vtronkov requested review from david-bakin-sl and MrValioBg

July 30, 2024 08:31

vtronkov added 2 commits

July 30, 2024 14:26


          refactor: move the test setup in the beforeAll method

f6850b6

Signed-off-by: Valentin Tronkov <99957253+vtronkov@users.noreply.github.com>


          Merge branch 'refs/heads/13658-add-TokenAirdropHandler-additional-tes…

22d82c8

…ts' into 14423-14443-airdrops-with-treasury-and-custom-fee-recivers

vtronkov linked an issue

that may be closed by this pull request

(AIRDROP - 29) Given a token with a custom fee that should be paid by the receiver when the airdrop is sent to a fee collector account of another token with the allCollectorsAreExempt=true then the receiver should not pay any custom fees #14485

Closed

david-bakin-sl approved these changes

View reviewed changes

Member

david-bakin-sl left a comment

I'd still like to see unit tests for handle() - but I'll assume you'll do that when you refactor that method in the base feature branch.

...-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/TokenAirdropHandler.java Outdated Show resolved Hide resolved

...-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/TokenAirdropHandler.java Outdated Show resolved Hide resolved

MrValioBg approved these changes

View reviewed changes

Contributor

MrValioBg left a comment

LGTM!

vtronkov added 2 commits

August 1, 2024 10:44


          Merge branch 'refs/heads/13658-add-TokenAirdropHandler-additional-tes…

315f584

…ts' into 14423-14443-airdrops-with-treasury-and-custom-fee-recivers


          nit: PR comments

64a01b6

Signed-off-by: Valentin Tronkov <99957253+vtronkov@users.noreply.github.com>

vtronkov merged commit c1441cd into 13658-add-TokenAirdropHandler-additional-tests

35 of 43 checks passed

vtronkov deleted the 14423-14443-airdrops-with-treasury-and-custom-fee-recivers branch

August 2, 2024 08:57

This was referenced Aug 2, 2024

(AIRDROP - 07) Sender airdrops a token with a custom fee to be paid by the receiver to the fee collector account for the token fee #14423

Closed

(AIRDROP - 09) Sender airdrops a token with a custom fee to be paid by the receiver to the token treasury account #14443

Closed

(AIRDROP - 08) Sender airdrops a token with a custom fee to be paid by the receiver to the fee collector account of another token fee when all fee collector accounts are exempt #14479

Closed

(AIRDROP - 29) Given a token with a custom fee that should be paid by the receiver when the airdrop is sent to a fee collector account of another token with the allCollectorsAreExempt=true then the receiver should not pay any custom fees #14485

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment