Skip to content

Commit

Permalink
Adjust the key gathering context methods to suppress exceptions from …
Browse files Browse the repository at this point in the history
…preHandle, as key gathering should not fail for non-pure checks.

* Modified PreHandleContextImpl to suppress exceptions from PreHandle in `allKeysForTransaction'
* Modified HandleContextImpl to suppress exceptions from PreHandle in `allKeysForTransaction'
* Modified tests to match

Note: These changes assume that services preHandle methods set required keys as early as possible, before making any state-based validations.  To be fully correct we will need to modify a few service handlers that do not meet that expectation.

Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
  • Loading branch information
jsync-swirlds committed Nov 10, 2023
1 parent 8f1a92d commit b320165
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,11 @@ public TransactionKeys allKeysForTransaction(
dispatcher.dispatchPureChecks(nestedTxn);
final var nestedContext = new PreHandleContextImpl(
readableStoreFactory(), nestedTxn, payerForNested, configuration(), dispatcher);
dispatcher.dispatchPreHandle(nestedContext);
try {
dispatcher.dispatchPreHandle(nestedContext);
} catch (final PreCheckException ignored) {
// We must ignore the exception here, as this is key gathering, not transaction validation.
}
return nestedContext;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,11 @@ public TransactionKeys allKeysForTransaction(
dispatcher.dispatchPureChecks(nestedTxn);
final var nestedContext =
new PreHandleContextImpl(storeFactory, nestedTxn, payerForNested, configuration, dispatcher);
dispatcher.dispatchPreHandle(nestedContext);
try {
dispatcher.dispatchPreHandle(nestedContext);
} catch (final PreCheckException ignored) {
// We must ignore the exception here, as this is key gathering, not transaction validation.
}
return nestedContext;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -573,15 +573,12 @@ void testAllKeysForTransactionWithFailingPureCheck() throws PreCheckException {

@Test
void testAllKeysForTransactionWithFailingPreHandle() throws PreCheckException {
// given
doThrow(new PreCheckException(INSUFFICIENT_ACCOUNT_BALANCE))
.when(dispatcher)
.dispatchPreHandle(any());

// when
assertThatThrownBy(() -> context.allKeysForTransaction(defaultTransactionBody(), ERIN.accountID()))
.isInstanceOf(PreCheckException.class)
.has(responseCode(INSUFFICIENT_ACCOUNT_BALANCE));
// gathering keys should not throw exceptions.
assertThatNoException().isThrownBy(() -> context.allKeysForTransaction(defaultTransactionBody(), ERIN.accountID()));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static com.hedera.node.app.spi.fixtures.workflows.ExceptionConditions.responseCode;
import static com.hedera.node.app.workflows.prehandle.PreHandleContextListUpdatesTest.A_COMPLEX_KEY;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatNoException;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.BDDMockito.given;
Expand Down Expand Up @@ -189,10 +190,7 @@ void testAllKeysForTransactionWithFailingPreHandle() throws PreCheckException {
.when(dispatcher)
.dispatchPreHandle(any());

// then
assertThatThrownBy(() -> subject.allKeysForTransaction(TransactionBody.DEFAULT, ERIN.accountID()))
.isInstanceOf(PreCheckException.class)
.has(responseCode(INSUFFICIENT_ACCOUNT_BALANCE));
assertThatNoException().isThrownBy(() -> subject.allKeysForTransaction(TransactionBody.DEFAULT, ERIN.accountID()));
}
}
}

0 comments on commit b320165

Please sign in to comment.