Skip to content
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.

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

Refactoring: Scheduled Sign #517

Conversation

georgiyazovaliiski
Copy link

The Scheduled Sign Transition Logic refactoring concluded in using the new refactored infrastructure, along with the Schedule model class. Unit tests were updated. E2E tests are passing.

Signed-off-by: Georgi Yazovaliyski georgi.yazovaliiski@gmail.com

Signed-off-by: Georgi Yazovaliyski <georgi.yazovaliiski@gmail.com>
Comment on lines 80 to 87
var outcome = signingOutcome.getLeft();
if (outcome == OK) {
var updatedSchedule = store.get(scheduleId);
txnCtx.setScheduledTxnId(updatedSchedule.scheduledTransactionId());
txnCtx.setScheduledTxnId(schedule.scheduledTransactionId());
if (signingOutcome.getRight()) {
outcome = executor.processExecution(scheduleId, store, txnCtx);
schedule.execute(txnCtx, store::resetPendingCreation);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous execution outcome was coming from the executor.processExecution method. We must think of a solution to either:

  • set the status in schedule.execute method
  • assume it's successful ( it throws InvalidTransactionException on fail )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also check SignatoryUtils for something buggy

@georgiyazovaliiski georgiyazovaliiski marked this pull request as draft September 27, 2021 05:59
Signed-off-by: Georgi Yazovaliyski <georgi.yazovaliiski@gmail.com>
@Daniel-K-Ivanov Daniel-K-Ivanov added the on hold Refers to issues/items/PRs that are on hold label Sep 28, 2021
…gn-internal

# Conflicts:
#	hedera-node/src/main/java/com/hedera/services/store/models/Schedule.java
#	hedera-node/src/main/java/com/hedera/services/store/schedule/ScheduleModelStore.java
#	hedera-node/src/main/java/com/hedera/services/txns/schedule/ScheduleCreateTransitionLogic.java
#	hedera-node/src/main/java/com/hedera/services/txns/schedule/ScheduleSignTransitionLogic.java
#	hedera-node/src/test/java/com/hedera/services/txns/schedule/ScheduleSignTransitionLogicTest.java
Signed-off-by: Georgi Yazovaliyski <georgi.yazovaliiski@gmail.com>
@georgiyazovaliiski georgiyazovaliiski marked this pull request as ready for review October 8, 2021 05:58
@Petyo-Lukanov Petyo-Lukanov added refactoring Refactoring related tasks and removed on hold Refers to issues/items/PRs that are on hold labels Oct 13, 2021
Copy link

@joan41868 joan41868 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall - looks great. I've got a suggestion - as we now have a solid start, given that the ScheduleCreate op is no longer broken, you can merge the internal branch in here in order to get the latest state. (TL;DR : your base branch is outdated )

Copy link

@CordonaCodeCraft CordonaCodeCraft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactoring related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants