`SResultNeedKey` callback takes error argument

[akrmn]

Currently, SResultNeedKey's callback, unlike any other SResult*'s, returns a Boolean. All handlers of SResultNeedKey ignore the value returned by the callback, except for the one in ScenarioRunner (via missingWith), which uses it to choose what to do in an error condition:

• When the return value is true, no exception is thrown and control goes back to the engine.
• When the return value is false, an exception for the error condition is thrown in the handler.
  • This only happens for KeyOperation.Fetch, independently of the input to the callback.
  • We also know that KeyOperation.Fetch.handleKeyNotFound sets control to SEDamlException(IE.ContractKeyNotFound(_))
  • This means the only thing that would change if ScenarionRunner's handler ignored the Boolean would be that execution would fail with that SEDamlException rather than the ScenarioRunner exceptions.

The idea here is that this callback, rather than return a Boolean for the ledger to decide whether to use a custom error or a default SEDamlException(IE.ContractKeyNotFound(_)), now takes that (optional) custom error as an argument. In case of error, the callback sets execution control to the custom error, if set, or otherwise to the default error. This only happens for KeyOperation.Fetch, since that's the only case that returned false, and true just hands control back to the engine. Meanwhile, KeyOperation.Lookup just ignores the custom error, since all it needs to do is return None.

All the places that used to discard the result of this callback now call it with the custom error set to None, which means that KeyOperation.Fetch will fail with the default error (and KeyOperation.Lookup will simply return None), as before. The single place that treated false differently, ScenarioRunner's missingWith, now calls the callback with the Some custom errors.

As a final point, note that the custom error argument has type Option[() => IE] rather than just Option[IE]. This is because the missingWith errors are not IEs (= interpretation.Errors), so that wouldn't type check, but we also don't want to throw them from ScenarioRunner because, if the underlying operation is a KeyOperation.Lookup, execution should carry on. Making the error call-by-name allows us to sneak those values in, so the error is thrown only when KeyOperation.Fetch.handleKeyNotFound tries to evaluate the () => IE.

[akrmn]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[akrmn]

Thanks for your review @nickchapman-da!

But the code is still quite complex because, if I understand correctly, you have managed to cause no behaviour change

Yes exactly. I think at least this shows exactly where the complexity is, so I'm happy if it doesn't get merged as is but rather becomes a step in fixing this mess.

to be sure I understand... we are going to error anyway., in the next step, with a slightly different error message.

Correct. To be more specific, this is the case when the Boolean is false (i.e. KeyOperation.Fetch). When it's true (i.e. KeyOperation.Lookup), we are not going to error at all but just return None : Optional ContractId - this is why missingWith cannot throw the error itself

If we are happy with this small change then we get to completely kill the odd/special implementation for one specific error case, as wrapped by one specific application:

SResultNeedKey's / KeyOperation.Fetch / ScenarioRunner

Yup! I'm not sure there's much value in the (4) more specific error messages in ScenarioRunner, considering one is the default ContractKeyNotFound and of the other three only one (contract <cid> not effective, but we found its key) is tested (the remaining ones are not found, but… and not active, but…). Even then, the not effective error only comes up when passing negative time, so it's very unlikely someone will miss it.

Wouldn't this be nice? 😄

yes it would 😃