Repurpose templateId in ExerciseCommand and add interfaceId in ExerciseEvent #13660

remyhaemmerle-da · 2022-04-21T21:34:35Z

CHANGELOG_BEGIN
CHANGELOG_END

sofiafaro-da · 2022-04-22T12:55:00Z

daml-lf/language/src/main/scala/com/digitalasset/daml/lf/language/PackageInterface.scala

+        case Some(choice) =>
+          Right(PackageInterface.ChoiceInfo.Template(choice))
+        case None =>
+          template.inheritedChoices.get(chName) match {


What happens with a choice name collision here?

The collision check is still in place, so nothing problematic happens.
If we disable the check, we will just get one random choice among the one that collides.

This is only used to provide a backward compatible API for Canton until they update their protocol (to track of the interfaceId field of the ExerciseNode). I am not sure it is worthwhile to make its semantics more sensible, since we should drop soon or latter.

We could decide to make the ledger API more lenient: try to infer the choice among all the inherited choice and crash if it is ambiguous. We could reuse this lookup for this purpose. In this case we will change it to produces error in case of collision.

I will add a comment to remove it (together with ReplayCommand.LenientExercise) once Canton have fix their protocol.

...ngine/src/main/scala/com/digitalasset/daml/lf/engine/preprocessing/CommandPreprocessor.scala

sofiafaro-da · 2022-04-22T12:58:38Z

daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/ReplayCommand.scala

+  /** Exercise a template choice, by template Id or interface Id. */
+  final case class LenientExercise(
+      templateId: TypeConName,
+      contractId: Value.ContractId,
+      choiceId: ChoiceName,
+      argument: Value,
+  ) extends ReplayCommand


I'm a bit confused. It looks like the implementation for LenientExercise allows exercising only by template id?

Maybe worth updating the comment.

Good catch I will udpate the comment.

sofiafaro-da · 2022-04-22T13:02:33Z

...ngine/src/main/scala/com/digitalasset/daml/lf/engine/preprocessing/CommandPreprocessor.scala

+      case command.ReplayCommand.Exercise(templateId, mbIfaceId, coid, choiceId, argument) =>
+        mbIfaceId match {
+          case Some(ifaceId) =>
+            unsafePreprocessExerciseInterface(ifaceId, coid, choiceId, argument)


If you have both an interfaceId and templateId, you should pass both to the preprocessor, and use the templateid in the exercise guard (via the speedy command ExerciseByInterface)

There's also an argument to be made here that you should be able to pass a requiring interface id as the "template id" and the required interface id as the "interface id", and have that interpreted by the ExerciseByInheritedInterface speedy command.

Oh if this is replay commands then this last point shouldn't matter, since you know the actually know the template id. I thought this was api commands.

My PR drops the possibility to do dynamic type checking in the ledger API as you described here.
With my changes, you have:

in case of template choice you use the templateID

in case of interface choice you use the interfaceID.

I wanted my change on the API to be minimal. The PR just adds the optional interfaceId filed to the ExerciseEvent API output. No change to input command (in particular we still have unique identifier in the ExerciceCommand).

With my changes, one will need to use an intermediary CreateAndExercise to perform the type checking (or any other guard).

I am fine to add the field in the Exercise Command and add back the dynamic type check in an upcoming PR.

I was confused -- I thought I was looking at changes to API command, but this is a change in the Replay command. Here for replay commands, the template id argument always refers to a template id, and the interface id always refers to the interface that defines the choice (if the choice was defined in an interface). And since it's a replay we don't need the extra type checks in the guard. So this looks fine. 👍

...ngine/src/main/scala/com/digitalasset/daml/lf/engine/preprocessing/CommandPreprocessor.scala

daml-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/ScriptF.scala

remyhaemmerle-da · 2022-04-22T15:56:33Z

@rgugliel-da @phoebe-da, the change of ReplayCommand.Exercise is breaking for Canton.
You can use the ReplayCommand.LenientExercise instead, the time you fix the Canton protocol in order to track properly the interface ID of the Exercise node.

Basically LenientExercise will give you the expected result as long there is not ambiguous choice for the exercise template.
Currently we check the choices are unambiguous, but we will drop this constrains soon. You can take a look a #13653 for a bit more details.

sofiafaro-da · 2022-04-26T09:30:49Z

daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/InterfacesTest.scala

    "be unable to exercise T1 by interface I2 (stopped in preprocessor)" in {
      val command = ApiCommand.Exercise(idT1, cid1, "C2", ValueRecord(None, ImmArray.empty))
      preprocessApi(command) shouldBe a[Left[_, _]]
    }


please also add a test where you try to exercise an interface choice (C1) by the template (T1) and it gets stopped in preprocessor; because you should only be able to exercise interface choice by interface id

sofiafaro-da

Looks good to me, thank you!

ledger-api/grpc-definitions/com/daml/ledger/api/v1/commands.proto

ledger-api/grpc-definitions/com/daml/ledger/api/v1/event.proto

rautenrieth-da

Is it sufficiently well documented that running the dev version of daml means you have no data continuity?

For sandbox (and I assume VMBC ledgers), --dev-mode-unsafe only says "Turns on development mode. Development mode allows development versions of Daml-LF language.". The less dangerous --early-access-unsafe says "Enable preview version of the next Daml-LF language. Should not be used in production.".

Also, the status of this work seems to be PoC. Are we certain that the ledger API changes are good as is? It would be confusing if we merged this to master, then released an SDK version, and then realized we have to change the new fields.

remyhaemmerle-da · 2022-05-10T12:12:19Z

Is it sufficiently well documented that running the dev version of daml means you have no data continuity?

If the documentation is not clear enough, we should extend it.

We have the following warning in SandboxServer, this should be probably push to VMBC and Canton:

Using early access mode is dangerous as the backward compatibility of future SDKs is not guaranteed.
Should be used for testing purpose only.

For sandbox (and I assume VMBC ledgers), --dev-mode-unsafe only says "Turns on development mode. Development mode allows development versions of Daml-LF language."

Because each change to LF dev is breaking, we cannot provide any form of data continuity for the dev version of LF.

The less dangerous --early-access-unsafe says "Enable preview version of the next Daml-LF language. Should not be used in production.".

For early access it is a bit different. we provide data continuity on a principal of best effort.
The marshaling "hack" we are doing here is clearly not good enough for early access.
We do need to add the interfaceId column as described in the design document I provided offline.

Also, the status of this work seems to be PoC. Are we certain that the ledger API changes are good as is? It would be confusing if we merged this to master, then released an SDK version, and then realized we have to change the new fields.

For the language perspective, the language team is pretty confident this is a good idea.

rautenrieth-da

As discussed elsewhere:

It's not clear yet how we will store the interface name in the database, however we can merge this as is: the new data is only generated using the unsupported dev version of Daml, and the ledger api changes are benign.

CHANGELOG_BEGIN CHANGELOG_END

sofiafaro-da

LGTM! 👍

remyhaemmerle-da force-pushed the remy-lf-interface branch from 0df48dd to c99239a Compare April 22, 2022 10:13

sofiafaro-da reviewed Apr 22, 2022

View reviewed changes

remyhaemmerle-da mentioned this pull request Apr 22, 2022

Allow template to implement interfaces that define the same choice name #13653

Closed

19 tasks

sofiafaro-da reviewed Apr 22, 2022

View reviewed changes

...ngine/src/main/scala/com/digitalasset/daml/lf/engine/preprocessing/CommandPreprocessor.scala Show resolved Hide resolved

sofiafaro-da reviewed Apr 22, 2022

View reviewed changes

remyhaemmerle-da force-pushed the remy-lf-interface branch from c99239a to 4678e2d Compare April 22, 2022 13:08

akrmn reviewed Apr 22, 2022

View reviewed changes

...ngine/src/main/scala/com/digitalasset/daml/lf/engine/preprocessing/CommandPreprocessor.scala Show resolved Hide resolved

akrmn reviewed Apr 22, 2022

View reviewed changes

daml-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/ScriptF.scala Show resolved Hide resolved

remyhaemmerle-da marked this pull request as ready for review April 22, 2022 15:49

remyhaemmerle-da requested review from stefanobaghino-da, ray-roestenburg-da, a team and meiersi-da as code owners April 22, 2022 15:49

sofiafaro-da reviewed Apr 26, 2022

View reviewed changes

sofiafaro-da approved these changes Apr 26, 2022

View reviewed changes

remyhaemmerle-da changed the title ~~LF: add interfaceId in command~~ Add interfaceId to ExerciseCommand and ExerciseEvent Apr 26, 2022

remyhaemmerle-da added interfaces mvp component/ledger component/daml-engine labels Apr 26, 2022

sofiafaro-da mentioned this pull request Apr 26, 2022

Improve required interface choice lookup in PackageInterface #13630

Closed

mziolekda reviewed Apr 27, 2022

View reviewed changes

ledger-api/grpc-definitions/com/daml/ledger/api/v1/commands.proto Outdated Show resolved Hide resolved

mziolekda reviewed Apr 27, 2022

View reviewed changes

ledger-api/grpc-definitions/com/daml/ledger/api/v1/commands.proto Outdated Show resolved Hide resolved

mziolekda reviewed Apr 27, 2022

View reviewed changes

ledger-api/grpc-definitions/com/daml/ledger/api/v1/event.proto Show resolved Hide resolved

rautenrieth-da reviewed May 10, 2022

View reviewed changes

remyhaemmerle-da closed this May 10, 2022

remyhaemmerle-da reopened this May 10, 2022

remyhaemmerle-da force-pushed the remy-lf-interface branch 2 times, most recently from c0d8256 to 7661736 Compare May 10, 2022 13:11

rautenrieth-da approved these changes May 12, 2022

View reviewed changes

remyhaemmerle-da force-pushed the remy-lf-interface branch 2 times, most recently from 00f338f to a03edff Compare May 16, 2022 07:16

remyhaemmerle-da enabled auto-merge (squash) May 16, 2022 07:18

remyhaemmerle-da disabled auto-merge May 16, 2022 08:46

LF: add interfaceId in command

c536422

CHANGELOG_BEGIN CHANGELOG_END

remyhaemmerle-da force-pushed the remy-lf-interface branch from a03edff to c536422 Compare May 16, 2022 09:46

cosmetic

7c4bfb0

remyhaemmerle-da enabled auto-merge (squash) May 16, 2022 14:30

remyhaemmerle-da changed the title ~~Add interfaceId to ExerciseCommand and ExerciseEvent~~ Repurpose templateId in ExerciseCommand and add interfaceId in ExerciseEvent May 16, 2022

sofiafaro-da approved these changes May 16, 2022

View reviewed changes

remyhaemmerle-da merged commit 6f6a305 into main May 16, 2022

remyhaemmerle-da deleted the remy-lf-interface branch May 16, 2022 15:30

S11001001 mentioned this pull request Jul 14, 2022

fix interface ID translation between Java ExercisedEvent and protoc representation #14423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repurpose templateId in ExerciseCommand and add interfaceId in ExerciseEvent #13660

Repurpose templateId in ExerciseCommand and add interfaceId in ExerciseEvent #13660

remyhaemmerle-da commented Apr 21, 2022 •

edited

Loading

sofiafaro-da Apr 22, 2022 •

edited

Loading

remyhaemmerle-da Apr 22, 2022 •

edited

Loading

sofiafaro-da Apr 22, 2022 •

edited

Loading

sofiafaro-da Apr 22, 2022

remyhaemmerle-da Apr 22, 2022 •

edited

Loading

sofiafaro-da Apr 22, 2022

sofiafaro-da Apr 22, 2022 •

edited

Loading

sofiafaro-da Apr 22, 2022

remyhaemmerle-da Apr 22, 2022 •

edited

Loading

sofiafaro-da Apr 22, 2022

remyhaemmerle-da commented Apr 22, 2022

sofiafaro-da Apr 26, 2022

sofiafaro-da left a comment

rautenrieth-da left a comment

remyhaemmerle-da commented May 10, 2022 •

edited

Loading

rautenrieth-da left a comment

sofiafaro-da left a comment

Repurpose templateId in ExerciseCommand and add interfaceId in ExerciseEvent #13660

Repurpose templateId in ExerciseCommand and add interfaceId in ExerciseEvent #13660

Conversation

remyhaemmerle-da commented Apr 21, 2022 • edited Loading

sofiafaro-da Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

remyhaemmerle-da Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

sofiafaro-da Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

sofiafaro-da Apr 22, 2022

Choose a reason for hiding this comment

remyhaemmerle-da Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

sofiafaro-da Apr 22, 2022

Choose a reason for hiding this comment

sofiafaro-da Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

sofiafaro-da Apr 22, 2022

Choose a reason for hiding this comment

remyhaemmerle-da Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

sofiafaro-da Apr 22, 2022

Choose a reason for hiding this comment

remyhaemmerle-da commented Apr 22, 2022

sofiafaro-da Apr 26, 2022

Choose a reason for hiding this comment

sofiafaro-da left a comment

Choose a reason for hiding this comment

rautenrieth-da left a comment

Choose a reason for hiding this comment

remyhaemmerle-da commented May 10, 2022 • edited Loading

rautenrieth-da left a comment

Choose a reason for hiding this comment

sofiafaro-da left a comment

Choose a reason for hiding this comment

remyhaemmerle-da commented Apr 21, 2022 •

edited

Loading

sofiafaro-da Apr 22, 2022 •

edited

Loading

remyhaemmerle-da Apr 22, 2022 •

edited

Loading

sofiafaro-da Apr 22, 2022 •

edited

Loading

remyhaemmerle-da Apr 22, 2022 •

edited

Loading

sofiafaro-da Apr 22, 2022 •

edited

Loading

remyhaemmerle-da Apr 22, 2022 •

edited

Loading

remyhaemmerle-da commented May 10, 2022 •

edited

Loading