Implement FreezeService preHandle #5988

kimbor · 2023-04-05T20:04:37Z

Implement FreezeService preHandle for Admin module

Issue #4464

Signed-off-by: Kim Rader <kim.rader@swirldslabs.com>

github-actions · 2023-04-05T20:17:11Z

Node: Unit Test Results

  1 298 files   1 298 suites 1h 51m 11s ⏱️
97 051 tests 97 044 ✔️ 7 💤 0 ❌
98 691 runs 98 684 ✔️ 7 💤 0 ❌

Results for commit d4f5a25.

♻️ This comment has been updated with latest results.

codecov · 2023-04-05T20:22:38Z

Codecov Report

Patch coverage: 71.42% and project coverage change: +23.19 🎉

Comparison is base (c43b5f1) 68.14% compared to head (d4f5a25) 91.34%.

Additional details and impacted files

@@              Coverage Diff               @@
##             develop    #5988       +/-   ##
==============================================
+ Coverage      68.14%   91.34%   +23.19%     
+ Complexity     21953    16984     -4969     
==============================================
  Files           1960     1274      -686     
  Lines         133554    48540    -85014     
  Branches        7519     4849     -2670     
==============================================
- Hits           91013    44339    -46674     
+ Misses         41079     3275    -37804     
+ Partials        1462      926      -536

Impacted Files	Coverage Δ
...p/service/admin/impl/ReadableSpecialFileStore.java	`0.00% <0.00%> (ø)`
...admin/impl/codec/MonoSpecialFilesAdapterCodec.java	`95.00% <ø> (ø)`
...-app/src/main/java/com/hedera/node/app/Hedera.java	`0.00% <0.00%> (ø)`
...edera/node/app/state/merkle/MerkleHederaState.java	`61.86% <0.00%> (ø)`
...app/workflows/dispatcher/ReadableStoreFactory.java	`53.84% <0.00%> (-9.80%)`	⬇️
...pp/workflows/dispatcher/TransactionDispatcher.java	`44.44% <0.00%> (ø)`
...e/app/service/network/impl/NetworkServiceImpl.java	`100.00% <ø> (ø)`
...app/service/admin/impl/handlers/FreezeHandler.java	`90.47% <89.47%> (-9.53%)`	⬇️
...node/app/service/admin/impl/FreezeServiceImpl.java	`100.00% <100.00%> (ø)`

... and 921 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2023-04-05T20:27:30Z

Node: E2E Test Results

    1 files     1 suites 16m 45s ⏱️
308 tests 308 ✔️ 0 💤 0 ❌
326 runs 326 ✔️ 0 💤 0 ❌

Results for commit d4f5a25.

♻️ This comment has been updated with latest results.

github-actions · 2023-04-05T20:39:20Z

Node: Integration Test Results

    3 files     3 suites 13m 32s ⏱️
150 tests 150 ✔️ 0 💤 0 ❌
151 runs 151 ✔️ 0 💤 0 ❌

Results for commit d4f5a25.

♻️ This comment has been updated with latest results.

Signed-off-by: Kim Rader <kim.rader@swirldslabs.com>

netopyr

LGTM from a hedera-app perspective.

...min-service-impl/src/main/java/com/hedera/node/app/service/admin/impl/FreezeServiceImpl.java

...vice-impl/src/main/java/com/hedera/node/app/service/admin/impl/ReadableSpecialFileStore.java

...ervice-impl/src/main/java/com/hedera/node/app/service/admin/impl/handlers/FreezeHandler.java

Signed-off-by: Michael Heinrichs <netopyr@users.noreply.github.com>

Signed-off-by: Kim Rader <kim.rader@swirldslabs.com>

sonarcloud · 2023-04-06T18:50:55Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

81.7% Coverage
2.0% Duplication

netopyr

LGTM from hedera-app perspective

iwsimon

LGTM

povolev15

LGTM

Neeharika-Sompalli

Review post merge.

Neeharika-Sompalli · 2023-04-13T12:46:34Z

...ervice-impl/src/main/java/com/hedera/node/app/service/admin/impl/handlers/FreezeHandler.java

+
+                    // FREEZE_ONLY requires a valid start_time
+                case FREEZE_ONLY -> verifyFreezeStartTimeIsInFuture(
+                        freezeTxn, context.getTxn().transactionID().transactionValidStart());


Please use a new variable for context.getTxn().transactionID().transactionValidStart() on line 82

Neeharika-Sompalli · 2023-04-13T12:47:14Z

...ervice-impl/src/main/java/com/hedera/node/app/service/admin/impl/handlers/FreezeHandler.java

+
+        FreezeTransactionBody freezeTxn = context.getTxn().freeze();
+
+        try {


I would suggest all validation go into a method called validate or preCheck or similar. And we call that method as first step in preHandle

Neeharika-Sompalli · 2023-04-13T12:49:26Z

...ervice-impl/src/main/java/com/hedera/node/app/service/admin/impl/handlers/FreezeHandler.java

+
+            // all checks have passed
+            context.status(OK);


I believe the default status is OK if nothing is set. So, I think this is not needed

Neeharika-Sompalli · 2023-04-13T12:49:59Z

...ervice-impl/src/main/java/com/hedera/node/app/service/admin/impl/handlers/FreezeHandler.java

+        } catch (PreCheckException e) {
+            // instead of catching this exception, would like to allow it to propagate up
+            // this will be implemented in issue #5880
+            context.status(e.responseCode());
+        }


We should not be catching any exceptions and setting status on context. It should be thrown as PrecheckException

Neeharika-Sompalli · 2023-04-13T12:56:43Z

.../hedera-app/src/main/java/com/hedera/node/app/workflows/dispatcher/ReadableStoreFactory.java

+    /**
+     * Get a {@link ReadableScheduleStore}
+     *
+     * @return a new {@link ReadableScheduleStore}
+     */


Typo ReadableSpecialFileStore

Implement FreezeService preHandle

095653d

Signed-off-by: Kim Rader <kim.rader@swirldslabs.com>

kimbor mentioned this pull request Apr 5, 2023

Implement FreezeService preHandle #5890

Closed

kimbor self-assigned this Apr 5, 2023

Merge branch 'develop' into 04464-AdminService-preHandle

bfc8e0f

kimbor marked this pull request as ready for review April 5, 2023 20:10

kimbor requested review from a team and povolev15 as code owners April 5, 2023 20:10

kimbor requested review from tomzhenghedera, nathanklick, timo0, iwsimon and netopyr April 5, 2023 20:10

kimbor added the CI:FinalChecks label Apr 5, 2023

kimbor added 2 commits April 5, 2023 14:29

Fix some tests

cbab8c6

Signed-off-by: Kim Rader <kim.rader@swirldslabs.com>

Merge branch 'develop' into 04464-AdminService-preHandle

93d8715

netopyr previously approved these changes Apr 6, 2023

View reviewed changes

iwsimon reviewed Apr 6, 2023

View reviewed changes

...min-service-impl/src/main/java/com/hedera/node/app/service/admin/impl/FreezeServiceImpl.java Show resolved Hide resolved

iwsimon reviewed Apr 6, 2023

View reviewed changes

...vice-impl/src/main/java/com/hedera/node/app/service/admin/impl/ReadableSpecialFileStore.java Show resolved Hide resolved

iwsimon reviewed Apr 6, 2023

View reviewed changes

...ervice-impl/src/main/java/com/hedera/node/app/service/admin/impl/handlers/FreezeHandler.java Show resolved Hide resolved

netopyr and others added 3 commits April 6, 2023 11:35

05875 query workflow improvements (#5971)

da74236

Signed-off-by: Michael Heinrichs <netopyr@users.noreply.github.com>

Rename the stateKey SPECIAL_FILES_KEY to UPGRADE_FILES_KEY

6300048

Signed-off-by: Kim Rader <kim.rader@swirldslabs.com>

Suppress a warning

7086d16

Signed-off-by: Kim Rader <kim.rader@swirldslabs.com>

kimbor dismissed netopyr’s stale review via 7086d16 April 6, 2023 18:37

Merge branch 'develop' into 04464-AdminService-preHandle

d4f5a25

kimbor requested review from iwsimon and netopyr April 6, 2023 19:11

netopyr approved these changes Apr 6, 2023

View reviewed changes

iwsimon approved these changes Apr 6, 2023

View reviewed changes

povolev15 approved these changes Apr 6, 2023

View reviewed changes

qnswirlds approved these changes Apr 6, 2023

View reviewed changes

kimbor merged commit 1c85769 into develop Apr 7, 2023
9 of 11 checks passed

kimbor deleted the 04464-AdminService-preHandle branch April 7, 2023 00:08

Neeharika-Sompalli reviewed Apr 13, 2023

View reviewed changes

nickpoorman added this to the v0.38.0 milestone Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement FreezeService preHandle #5988

Implement FreezeService preHandle #5988

kimbor commented Apr 5, 2023

github-actions bot commented Apr 5, 2023 •

edited

codecov bot commented Apr 5, 2023 •

edited

github-actions bot commented Apr 5, 2023 •

edited

github-actions bot commented Apr 5, 2023 •

edited

netopyr left a comment

sonarcloud bot commented Apr 6, 2023

netopyr left a comment

iwsimon left a comment

povolev15 left a comment

Neeharika-Sompalli left a comment •

edited

Neeharika-Sompalli Apr 13, 2023

Neeharika-Sompalli Apr 13, 2023

Neeharika-Sompalli Apr 13, 2023

Neeharika-Sompalli Apr 13, 2023

Neeharika-Sompalli Apr 13, 2023


		FreezeTransactionBody freezeTxn = context.getTxn().freeze();

		try {

Implement FreezeService preHandle #5988

Implement FreezeService preHandle #5988

Conversation

kimbor commented Apr 5, 2023

github-actions bot commented Apr 5, 2023 • edited

Node: Unit Test Results

codecov bot commented Apr 5, 2023 • edited

Codecov Report

github-actions bot commented Apr 5, 2023 • edited

Node: E2E Test Results

github-actions bot commented Apr 5, 2023 • edited

Node: Integration Test Results

netopyr left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 6, 2023

netopyr left a comment

Choose a reason for hiding this comment

iwsimon left a comment

Choose a reason for hiding this comment

povolev15 left a comment

Choose a reason for hiding this comment

Neeharika-Sompalli left a comment • edited

Choose a reason for hiding this comment

Neeharika-Sompalli Apr 13, 2023

Choose a reason for hiding this comment

Neeharika-Sompalli Apr 13, 2023

Choose a reason for hiding this comment

Neeharika-Sompalli Apr 13, 2023

Choose a reason for hiding this comment

Neeharika-Sompalli Apr 13, 2023

Choose a reason for hiding this comment

Neeharika-Sompalli Apr 13, 2023

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2023 •

edited

codecov bot commented Apr 5, 2023 •

edited

github-actions bot commented Apr 5, 2023 •

edited

github-actions bot commented Apr 5, 2023 •

edited

Neeharika-Sompalli left a comment •

edited