Support submitMustFail in DAML Script Service #7076
Conversation
aherrmann-da
force-pushed
the
submit-must-fail
branch
from
3f2200f to
6775841
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // This branch is superseded by SubmitMustFail below, | ||
| // however, it is maintained for backwards | ||
| // compatibility with DAML script DARs generated by | ||
| // older SDK versions that didn't distinguish Submit | ||
| // and SubmitMustFail. |
There was a problem hiding this comment.
I have confirmed that replacing this branch by Future.failed(...) breaks the corresponding compatibility test.
There was a problem hiding this comment.
We don’t seem to have a submitMustFail in the compat tests. Should we add one?
There was a problem hiding this comment.
Oh nvm, I just saw that you added it 🤦
changelog_begin changelog_end
Pushes handling of `submitMustFail` into the DAML script interpreter rather than DAML script itself.
aherrmann-da
force-pushed
the
submit-must-fail
branch
from
bf2311b to
d75d28c
Compare
aherrmann-da
requested review from
cocreature,
garyverhaegen-da and
hurryabit
as code owners
| // This branch is superseded by SubmitMustFail below, | ||
| // however, it is maintained for backwards | ||
| // compatibility with DAML script DARs generated by | ||
| // older SDK versions that didn't distinguish Submit | ||
| // and SubmitMustFail. |
There was a problem hiding this comment.
We don’t seem to have a submitMustFail in the compat tests. Should we add one?
| @@ -463,6 +476,73 @@ class IdeClient(val compiledPackages: CompiledPackages) extends ScriptLedgerClie | |||
| Future.fromTry(result) | |||
| } | |||
|
|
|||
| override def submitMustFail(party: SParty, commands: List[ScriptLedgerClient.Command])( | |||
There was a problem hiding this comment.
I am slightly worried that we are effectively replicating a significant part of the logic of submit. What happens if we replace the whole implementation here by something that catches SError and flips it?
There was a problem hiding this comment.
By catching SError you mean the SResultError branch? AFAIU that's not enough as we also want to flip in other cases, e.g. on commitTransaction error in SResultScenarioCommit. Conversely we don't always want to recover from Failure, e.g. AFAIU we don't want to recover from Failure(new RuntimeException(s"Unexpected abort: $x")). The other submit implementations use Left for errors that should be caught by submitMustFail but the Left parameter is restricted to StatusRuntimeException which doesn't really work in this case.
An alternative might be to use a more general or a trait associated type instead of StatusRuntimeException and then use Success(Left(_)) in IdeClient.submit for errors that submitMustFail should catch.
There was a problem hiding this comment.
Side note: SResultScenarioCommit was a bug, fixed in #7089. Doesn’t change anything here.
There are a few cases to consider here:
- The future completes successfully. In that case, we want to turn it into a failure.
- The future fails with
SError. This includes calls toerroras well asScenarioErrorCommitErrorwhich we will get back from a failed commit for things like authorization errors. - Other stuff, e.g., the wildcard match at the end which we hope not to hit.
My idea would be to call submit and then handle the three cases.
- This is an actual failure. We expected things to fail and they didn’t.
- This is the successful case. We expected a failure during submission and we got one.
- This is a crash, something went wrong.
There was a problem hiding this comment.
Ah, I see. That seems to be possible. I've pushed a commit implementing this.
| // This branch is superseded by SubmitMustFail below, | ||
| // however, it is maintained for backwards | ||
| // compatibility with DAML script DARs generated by | ||
| // older SDK versions that didn't distinguish Submit | ||
| // and SubmitMustFail. |
There was a problem hiding this comment.
Oh nvm, I just saw that you added it 🤦
...-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/LedgerInteraction.scala
Show resolved
Hide resolved
Closes #7008
Pushes handling of
submitMustFailinto the DAML script interpreter rather than DAML script itself.For backwards compatibility we keep the implementation of the old handling around.
Pull Request Checklist
CHANGELOG_BEGINandCHANGELOG_ENDtagsNOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines runtotrigger the build.