BESU-146 - return better gas estimate service error message if possible

[anthonybuckle]

Signed-off-by: Anthony Buckle anthonybuckle@gmail.com

PR description

Here is a suggestion to improve gas estimate service error messages concerning jira ticket BESU-146.

Fixed Issue(s)

Fixes: https://jira.hyperledger.org/browse/BESU-146

[CjHare]

It would be good to see an addition test in EthEstimateGasTest that covers the flow you've changed i.e an error is encountered by the function returned by gasEstimateResponse().

[anthonybuckle]

Hi CjHare, Thanks for the feedback and no problem. Are you referring to the case where gasEstimateResponse(requestContext) fails and returns a null value. That would lead line 68 orElse(errorResponse(requestContext, JsonRpcError.INTERNAL_ERROR)) returning an error response. It would need a test case. Do you know any good ways to make gasEstimateResponse(requestContext) fail in the test cases using when().thenReturn(). Those methods are private in the class.

[CjHare]

Ah, actually I was thinking a test for the flow that goes through line 93 (the replacement of null for errorResponse(request, result.getValidationResult());, where the result is unsuccessful and the validation result would be something other than INTERNAL_ERROR.

The case you raise is a also good one, let me ponder over it 🤔

[CjHare]

@anthonybuckle, I suspect the case you mention (the orElse() on line 68) may already be covered by shouldReturnErrorWhenTransientTransactionProcessorReturnsEmpty()

[CjHare]

This test would cover the additional flow incorporating the simulation validation result 🙂

  @Test
  public void shouldReturnErrorWhenTransactionProcessorReturnsUnsuccessfulSimulation() {
    final JsonRpcRequestContext request = ethEstimateGasRequest(callParameter());
    setupUnsuccessfulSimulation(UPFRONT_COST_EXCEEDS_BALANCE);

    final JsonRpcResponse expectedResponse =
        new JsonRpcErrorResponse(null, JsonRpcError.TRANSACTION_UPFRONT_COST_EXCEEDS_BALANCE);

    Assertions.assertThat(method.response(request))
        .isEqualToComparingFieldByField(expectedResponse);
  }

  private void setupUnsuccessfulSimulation(final TransactionValidator.TransactionInvalidReason reason){
    final  TransactionSimulatorResult simulation = mock(TransactionSimulatorResult.class);
    when(simulation.isSuccessful()).thenReturn(false);
    when(simulation.getValidationResult()).thenReturn( ValidationResult.invalid(reason));
    when(transactionSimulator.process(eq(modifiedCallParameter()), eq(1L)))
        .thenReturn(Optional.of( simulation ));
  }

[anthonybuckle]

Hi @CjHare, I confirmed that test case shouldReturnErrorWhenTransientTransactionProcessorReturnsEmpty does handle the orElse() case. I also added the exceeds balance test case as you suggested. Thanks for your help. Cheers.

[timbeiko]

@anthonybuckle if you'd like this change included in the release notes, please add a line about it to the changelog. Thanks!

[anthonybuckle]

@anthonybuckle if you'd like this change included in the release notes, please add a line about it to the changelog. Thanks!

@timbeiko Sure no problem. A PR for the updated changelog is here: #451