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: use "inline" auto-association fee payments #14291

Closed

tinker-michaelj wants to merge 15 commits into develop from 14284-direct-auto-association

Collaborator

tinker-michaelj commented Jul 17, 2024 •

edited

Loading

Description:

Switches from doing auto-associations via dispatch to doing them inline in the CryptoTransferHandler.
- This implies the handler must charge fees whenever the context is not an internal dispatch from the ContractService (in that case the cost comes out of the gas budget for the EVM transaction).
- Hence exposes chargeNetworkFee() in HandleContext.
Since there is no longer a synthetic TokenAssociate child, adds throttle check based on the number of automatic token associations in the children that are there.
Updates unit and @HapiTest expectations accordingly.

povolev15 and others added 10 commits

July 3, 2024 12:14


          feat: disable DAB from release 0.52 (#14118)

4df75d2

Signed-off-by: Lev Povolotsky <[email protected]>


          feat: (cherry-pick) HIP-904 Charge automatic associations during `Cry…

a5c0418

…ptoTransfer` (#14123)

Signed-off-by: Neeharika-Sompalli <[email protected]>


          chore: Bump hapi version to 0.52.0 (#14113)

7ab4855

Signed-off-by: Thomas Moran <[email protected]>


          feat: implement HIP-632 isAuthorizedRaw method (quasi-cherry-pick f…

2238f53

…rom develop) (#14131)

Signed-off-by: David S Bakin <[email protected]>


          fix: [release/0.52] register empty gradle tasks at root (releaseAdhoc…

230e203

…Commit, releaseDevelopCommit, releaseDevelopDailySnapshot, releaseDevelopSnapshot, releasePrereleaseChannel) (#14150)

Signed-off-by: Elijah Lynn <[email protected]>


          build: filter publishing by deactivating tasks (#14160)

Signed-off-by: Jendrik Johannes <[email protected]>


          fix: cherry-pick to 0.52 fix for charging correct gas for new isAutho…

4fd1137

…rizedRaw system contract method (#14193)

Signed-off-by: David S Bakin <[email protected]>


          fix: cherry-pick, to 0.52, #14210 isAuthorizedRaw to charge all gas o…

0a932bc

…n failure (#14218)

Signed-off-by: David S Bakin <[email protected]>


          chore: standardize max lifetime and auto-renew period (#14219)

2b65890

Signed-off-by: Michael Tinker <[email protected]>


          Charge auto-association fees "inline"

Signed-off-by: Michael Tinker <[email protected]>

tinker-michaelj requested a review from a team as a code owner

July 17, 2024 19:41

tinker-michaelj requested a review from a team

July 17, 2024 19:41

tinker-michaelj requested review from a team, rbair23, dalvizu, poulok, netopyr, Nana-EC, SimiHunjan, steven-sheehy and nathanklick as code owners

July 17, 2024 19:41

tinker-michaelj changed the base branch from develop to release/0.52

July 17, 2024 19:41

tinker-michaelj requested a review from a team as a code owner

July 17, 2024 19:41

david-bakin-sl previously approved these changes

View reviewed changes

Member

david-bakin-sl left a comment •

edited

Loading

Some comments for your consideration.

(Also, is this a chore and not a fix or maybe feat? Also a reminder the PR description is TBS.)

hedera-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/workflows/HandleContext.java Outdated Show resolved Hide resolved

...a/com/hedera/node/app/service/token/impl/handlers/transfer/AssociateTokenRecipientsStep.java Outdated Show resolved Hide resolved

...a/com/hedera/node/app/service/token/impl/handlers/transfer/AssociateTokenRecipientsStep.java Outdated Show resolved Hide resolved

tinker-michaelj added this to the v0.52 milestone

tinker-michaelj self-assigned this

tinker-michaelj changed the title ~~chore: use "inline" auto-association fee payments~~ feat: use "inline" auto-association fee payments


          Add tests, fix typo in throttle advisor

a2db3b0

Signed-off-by: Michael Tinker <[email protected]>

tinker-michaelj dismissed david-bakin-sl’s stale review via

a2db3b0

July 18, 2024 02:06

tinker-michaelj added 3 commits

July 17, 2024 21:08


          Fix javadoc

7cf2509

Signed-off-by: Michael Tinker <[email protected]>


          Clarify javadoc

8c08eaf

Signed-off-by: Michael Tinker <[email protected]>


          Address reviewer comment

Signed-off-by: Michael Tinker <[email protected]>

github-actions bot commented Jul 18, 2024 •

edited

Loading

Node: Unit Test Results

1 542 files 1 542 suites 3h 26m 32s ⏱️
115 770 tests 115 711 ✅ 59 💤 0 ❌
124 085 runs 124 026 ✅ 59 💤 0 ❌

Results for commit 66d8919.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 18, 2024 •

edited

Loading

Node: HAPI Test (Node Death Reconnect) Results

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

Results for commit 66d8919.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 18, 2024 •

edited

Loading

Node: HAPI Test (Restart) Results

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

Results for commit 66d8919.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 18, 2024 •

edited

Loading

Node: HAPI Test (Token) Results

22 files 22 suites 6m 34s ⏱️
272 tests 272 ✅ 0 💤 0 ❌
352 runs 352 ✅ 0 💤 0 ❌

Results for commit 66d8919.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 18, 2024 •

edited

Loading

Node: HAPI Test (Crypto) Results

24 files 24 suites 14m 39s ⏱️
362 tests 362 ✅ 0 💤 0 ❌
369 runs 369 ✅ 0 💤 0 ❌

Results for commit 66d8919.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 18, 2024 •

edited

Loading

Node: HAPI Test (Misc) Results

51 files 51 suites 20m 38s ⏱️
357 tests 357 ✅ 0 💤 0 ❌
375 runs 375 ✅ 0 💤 0 ❌

Results for commit 66d8919.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 18, 2024 •

edited

Loading

Node: HAPI Test (Time Consuming) Results

19 tests 19 ✅ 22m 49s ⏱️
4 suites 0 💤
4 files 0 ❌

Results for commit 66d8919.

♻️ This comment has been updated with latest results.

github-actions bot commented Jul 18, 2024 •

edited

Loading

Node: HAPI Test (Smart Contract) Results

73 files 73 suites 23m 56s ⏱️
681 tests 681 ✅ 0 💤 0 ❌
735 runs 735 ✅ 0 💤 0 ❌

Results for commit 66d8919.

♻️ This comment has been updated with latest results.


          Revert HapiTest expectations for inline charging

66d8919

Signed-off-by: Michael Tinker <[email protected]>

david-bakin-sl reviewed

View reviewed changes

Member

david-bakin-sl left a comment

LGTM but this is a comment not an approval because I want to come back to it.

...a/com/hedera/node/app/service/token/impl/handlers/transfer/AssociateTokenRecipientsStep.java

+                          // We only charge auto-association fees inline if this is a user disaptch; in that case the
+                          // contract service will take the auto-association costs from the remaining EVM gas
+                          if (context.recordBuilders()
+                                  .getOrCreate(SingleTransactionRecordBuilder.class)

Member

david-bakin-sl Jul 18, 2024

(Gotta say, isn't obvious at this line how a method called getOrCreate works given an interface as the target class - had to trace this one down ...)

...a/node/app/service/token/impl/test/handlers/transfer/AdjustFungibleTokenChangesStepTest.java

@@ @@ -146,7 +152,7 @@ void doesTokenBalanceChangesWithAllowances() { @@
                       assertThat(receiverAccountBefore.numberPositiveBalances()).isEqualTo(2);
                       assertThat(senderRelBefore.balance()).isEqualTo(1000L);
                       // There is an association happening during the transfer for auto creation
-                      assertThat(receiverRelBefore.balance()).isEqualTo(1);
+                      assertThat(receiverRelBefore.balance()).isEqualTo(0);

Member

david-bakin-sl Jul 18, 2024

Why wasn't this assert needed (or useful) also at L106 above? (Only instance in these tests modified in this PR where it wasn't.)

david-bakin-sl previously approved these changes

View reviewed changes

Member

david-bakin-sl left a comment

DOne looking. LGTM

Neeharika-Sompalli previously approved these changes

View reviewed changes

Member

Neeharika-Sompalli left a comment

LGTM! We should probably target this to develop

tinker-michaelj changed the base branch from release/0.52 to develop

July 24, 2024 19:30

tinker-michaelj dismissed stale reviews from Neeharika-Sompalli and david-bakin-sl

July 24, 2024 19:30

The base branch was changed.

Collaborator Author

tinker-michaelj commented Jul 24, 2024

Will retarget to develop.

tinker-michaelj closed this

tinker-michaelj deleted the 14284-direct-auto-association branch

August 27, 2024 13:28

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

Reviewers

david-bakin-sl david-bakin-sl left review comments

Neeharika-Sompalli Neeharika-Sompalli left review comments

rbair23 Awaiting requested review from rbair23

dalvizu Awaiting requested review from dalvizu

poulok Awaiting requested review from poulok

netopyr Awaiting requested review from netopyr

Nana-EC Awaiting requested review from Nana-EC

SimiHunjan Awaiting requested review from SimiHunjan

steven-sheehy Awaiting requested review from steven-sheehy

nathanklick Awaiting requested review from nathanklick

Labels

None yet