daml
daml copied to clipboard
`SResultNeedKey` callback takes error argument
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 toSEDamlException(IE.ContractKeyNotFound(_))
- This means the only thing that would change if
ScenarionRunner
's handler ignored theBoolean
would be that execution would fail with thatSEDamlException
rather than theScenarioRunner
exceptions.
- This only happens for
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 IE
s (= interpretation.Error
s), 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
.
/azp run
Azure Pipelines successfully started running 1 pipeline(s).
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 pass
ing negative time, so it's very unlikely someone will miss it.
Wouldn't this be nice? 😄
yes it would 😃