Abort ScheduleSignTransitionLogic if schedule is resolved #1303

tinker-michaelj · 2021-04-22T14:47:41Z

Summary of the change:

Abort from ScheduleSignTransitionLogic if the target schedule is already resolved (either executed or deleted).
For extra safety, make ScheduleReadyForExecution gate the call to txnCtx.trigger by confirming the schedule was successfully marked as executed.

Also create tests that assert both response codes and state changes (including charged service fees for non-SUCCESSful transactions) are as expected when we:

codecov · 2021-04-22T15:09:25Z

Codecov Report

Merging #1303 (6722609) into master (ba21b64) will increase coverage by 0.00%.
The diff coverage is 97.43%.

@@            Coverage Diff            @@
##             master    #1303   +/-   ##
=========================================
  Coverage     87.89%   87.90%           
- Complexity     5672     5677    +5     
=========================================
  Files           450      450           
  Lines         16839    16858   +19     
  Branches       1757     1760    +3     
=========================================
+ Hits          14801    14819   +18     
- Misses         1567     1568    +1     
  Partials        471      471

Impacted Files	Coverage Δ	Complexity Δ
...s/txns/schedule/ScheduleCreateTransitionLogic.java	`94.73% <85.71%> (-1.16%)`	`18.00 <0.00> (ø)`
...a/com/hedera/services/context/ServicesContext.java	`95.06% <100.00%> (+0.04%)`	`259.00 <2.00> (+2.00)`
...edera/services/txns/schedule/ScheduleExecutor.java	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
...ces/txns/schedule/ScheduleSignTransitionLogic.java	`100.00% <100.00%> (ø)`	`13.00 <6.00> (+2.00)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba21b64...6722609. Read the comment docs.

Daniel-K-Ivanov

LGTM!

rbair23 · 2021-04-27T01:14:35Z

hedera-node/src/main/java/com/hedera/services/txns/schedule/ScheduleReadyForExecution.java

-public abstract class ScheduleReadyForExecution {
+import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.OK;
+
+public class ScheduleReadyForExecution {


Let's add some class level documentation to explain what this class is and how it fits into the system

Added class level documentation here.

rbair23 · 2021-04-27T01:15:54Z

hedera-node/src/main/java/com/hedera/services/txns/schedule/ScheduleReadyForExecution.java

@@ -37,16 +39,19 @@
 	}

 	ResponseCodeEnum processExecution(ScheduleID id) throws InvalidProtocolBufferException {
+		var executionStatus = store.markAsExecuted(id);


I prefer to see "final" in situations like this. It makes reading the code easier, because as I read the code (from top to bottom) I can tell just from this line that executionStatus isn't going to be modified anywhere further down. As it is, I'm not sure, so I have to read the rest of the code in the block scope to verify that it is "effectively final".

Made the changes.

rbair23 · 2021-04-27T18:41:31Z

hedera-node/src/main/java/com/hedera/services/txns/schedule/ScheduleReadyForExecution.java

+/**
+ * Defines a class to handle scheduled transaction execution
+ * once the scheduled transaction is signed by the required
+ * number of parties.
+ *
+ * @author Michael Tinker
+ * @author Abhishek Pandey
+ */
+public class ScheduleReadyForExecution {


Thanks for adding some documentation.

I see that the class is non-final, yet the documentation doesn't give me any idea as to whether this is intentional or not. I don't want to see a comment that says something like "This class is non-final so it can be extended", but rather something like "This base class does X. Subclasses should override X, Y, and Z to implement ABC". Something with more meat to it. Or, if the class is not meant to be extended, lets forbid it by making the class final.

Another thought is that you can probably run the comment out to 80 chars instead of breaking at 60 or so.

I believe it could be made final and ScheduleCreateTransitionLogic, ScheduleSignTransitionLogic which extend from this class could use the base class instance as a member variable. Refactoring has introduced some unit tests failures for now which would be fixed with the help of @tinker-michaelj

Refactored to use the new final class ScheduleExecutor, and modified the documentation comments to be around 80 chars.

rbair23 · 2021-04-27T18:44:47Z

hedera-node/src/main/java/com/hedera/services/txns/schedule/ScheduleReadyForExecution.java

+	 * @param id The id of the scheduled transaction.
+	 *
+	 * @return the response code from executing the inner scheduled transaction


I'd like us to document the expected state and conditions for the param and the return value. I see that id is an object, which means it might be null. Is null a valid input? How does it behave if we do pass a null? (Is there a test for this?).

The method documentation suggests that this method checks if the underlying transaction is already executed/deleted. Based on this wording, I would expect the method to return a boolean. But actually, I think this method does more, it just happens to perform this check in addition to other stuff? The method description doesn't seem to align with the method signature.

Thanks for reviewing it, I have changed the method documentation, added tests for not null checks. The inputs now have a @NotNull annotation. The return value would always be a proper ResponseEnumCode.

rbair23 · 2021-04-28T20:40:12Z

hedera-node/src/main/java/com/hedera/services/txns/schedule/ScheduleExecutor.java

+	/**
+	 * Given a {@link ScheduleID}, {@link ScheduleStore}, {@link TransactionContext} it first checks if the underlying
+	 * transaction is already executed/deleted before attempting to execute and then returns response code after
+	 * triggering the underlying transaction. A ResponseEnumCode of OK is returned upon successful trigger of the
+	 * inner transaction.
+	 *
+	 * @param id The id of the scheduled transaction.
+	 * @param store Object to handle scheduled entity type.
+	 * @param context Object to handle inner transaction specific context on a node.
+	 * @return the response code {@link ResponseCodeEnum} from executing the inner scheduled transaction
+	 */
+	ResponseCodeEnum processExecution(ScheduleID id, ScheduleStore store, TransactionContext context) throws


Thanks for this documentation, it is much better! Please also add some indication as to whether the various args and return value can be null. Are we able to use the @NotNull annotation? That might make static code analysis able to interpret the semantics, and not just have it defined in the documentation.

That sounds great. Modified it to have @NotNull annotation.

Signed-off-by: tinker-michaelj <[email protected]>

Signed-off-by: abhishek-hedera <[email protected]>

tinker-michaelj requested a review from Daniel-K-Ivanov April 22, 2021 14:47

Daniel-K-Ivanov previously approved these changes Apr 23, 2021

View reviewed changes

tinker-michaelj dismissed Daniel-K-Ivanov’s stale review via 867c2a9 April 23, 2021 21:43

rbair23 added this to the Hedera 0.13.2 milestone Apr 26, 2021

rbair23 assigned abhishek-hedera Apr 26, 2021

rbair23 previously approved these changes Apr 27, 2021

View reviewed changes

rbair23 reviewed Apr 27, 2021

View reviewed changes

abhishek-hedera dismissed rbair23’s stale review via 40d3a35 April 27, 2021 15:14

abhishek-hedera requested review from qnswirlds, anighanta, ljianghedera and Neeharika-Sompalli April 27, 2021 17:52

abhishek-hedera force-pushed the handle-repeated-signings branch from dd914ee to 1f12ec2 Compare April 27, 2021 18:22

rbair23 reviewed Apr 27, 2021

View reviewed changes

abhishek-hedera force-pushed the handle-repeated-signings branch 2 times, most recently from e183d07 to d261578 Compare April 28, 2021 18:54

rbair23 reviewed Apr 28, 2021

View reviewed changes

SimiHunjan modified the milestones: Hedera 0.13.2, Hedera 0.14.0 Apr 29, 2021

tinker-michaelj and others added 10 commits April 30, 2021 16:32

Abort ScheduleSignTransitionLogic if schedule is resolved

6bf462b

Signed-off-by: tinker-michaelj <[email protected]>

Fix unit test

55809c4

Signed-off-by: tinker-michaelj <[email protected]>

Add two ScheduleSignSpecs

fa7d4b2

Signed-off-by: tinker-michaelj <[email protected]>

Remove duplicate spec

f041a09

Signed-off-by: tinker-michaelj <[email protected]>

Add multiple negative tests to verify single schedule execution

1a80c54

Signed-off-by: tinker-michaelj <[email protected]>

Add more schedule sign specs (TODO - finish)

99084ca

Signed-off-by: tinker-michaelj <[email protected]>

Add negative tests for submit message failure modes

a2c8c55

Signed-off-by: tinker-michaelj <[email protected]>

Include more negative tests cases for failed scheduled xfers

5127bec

Signed-off-by: tinker-michaelj <[email protected]>

Adding scenario for INVALID_ACCOUNT_AMOUNTS in ScheduleExecution

8050dad

added class documentation for ScheduleReadyForExecution

7727760

Signed-off-by: abhishek-hedera <[email protected]>

abhishek-hedera added 4 commits April 30, 2021 16:32

added EET for TRANSFER_LIST_SIZE_EXCEEDED

eaa7221

Signed-off-by: abhishek-hedera <[email protected]>

added EETs and fixed test failures

8aa6180

Signed-off-by: abhishek-hedera <[email protected]>

refactored ScheduleExecutor and addressed comments

9b33851

Signed-off-by: abhishek-hedera <[email protected]>

addressed documentation reviws

6722609

Signed-off-by: abhishek-hedera <[email protected]>

abhishek-hedera force-pushed the handle-repeated-signings branch from e0132a5 to 6722609 Compare April 30, 2021 20:42

abhishek-hedera approved these changes Apr 30, 2021

View reviewed changes

tinker-michaelj merged commit 711321f into master Apr 30, 2021

tinker-michaelj deleted the handle-repeated-signings branch April 30, 2021 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abort ScheduleSignTransitionLogic if schedule is resolved #1303

Abort ScheduleSignTransitionLogic if schedule is resolved #1303

tinker-michaelj commented Apr 22, 2021 •

edited by abhishek-hedera

Loading

codecov bot commented Apr 22, 2021 •

edited

Loading

Daniel-K-Ivanov left a comment

rbair23 Apr 27, 2021

abhishek-hedera Apr 27, 2021

rbair23 Apr 27, 2021

abhishek-hedera Apr 27, 2021

rbair23 Apr 27, 2021

abhishek-hedera Apr 28, 2021

abhishek-hedera Apr 28, 2021

rbair23 Apr 27, 2021

abhishek-hedera Apr 30, 2021

rbair23 Apr 28, 2021

abhishek-hedera Apr 30, 2021

Abort ScheduleSignTransitionLogic if schedule is resolved #1303

Abort ScheduleSignTransitionLogic if schedule is resolved #1303

Conversation

tinker-michaelj commented Apr 22, 2021 • edited by abhishek-hedera Loading

codecov bot commented Apr 22, 2021 • edited Loading

Codecov Report

Daniel-K-Ivanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinker-michaelj commented Apr 22, 2021 •

edited by abhishek-hedera

Loading

codecov bot commented Apr 22, 2021 •

edited

Loading