Infer templateId and choiceName in daml trigger commands #2945

aherrmann-da · 2019-09-18T07:43:05Z

Infers templateId from templateArg in createCmd.
Infers choiceName from choiceArg in exerciseCmd.
Introduces AnyContractId, which carries templateId and contractId.
Infers templateId from contractId: AnyContractId in exerciseCmd.
The template type still needs to be passed explicitly in exerciseCmd.
In future we may wish to pass ContractId t instead of AnyContractId. This would require something like fromAnyContractId : AnyContractId -> Option (ContractId t)
Extends the ACS test-case to use exerciseCmd.
Introduces an indirection, where the trigger first creates AssetMirrorProposal and then exercises the Accept choice that creates the actual AssetMirror.

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Add a line to the release notes, if appropriate
Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

Add a layer of indirection to test `exerciseCmd` in the ACS test. The trigger first creates a `AssetMirrorProposal`, only when the proposal has been created, will it exercise the `Accept` choice to create the actual `AssetMirror` contract.

cocreature

Looks great with some minor comments, thank you!

cocreature · 2019-09-18T07:57:08Z

triggers/daml/Daml/Trigger.daml

@@ -24,6 +24,11 @@ data Identifier = Identifier
  , entityName : Text
  } deriving (Show, Eq)

+data AnyContractId = AnyContractId


Shouldn’t we expose this type (but not the constructors). I also wonder if we might just want to keep calling this ContractId for now until we have a variant with a phantom type parameter but I guess changing it now is better for backwards compatibility.

Indeed, we should expose it. I overlooked that, good catch!

Even if we introduce a ContractId t with a phantom type parameter, we'd still need something like AnyContractId for the Created and Archived events. I figured once we introduce ContractId t we need to change exerciseCmd anyway, but we can keep Created and Archived untouched.

I have no strong opinion on the name, though, so happy to rename it if you prefer.

Let’s keep AnyContractId for now.

Shouldn’t we expose this type (but not the constructors)

For now we need access to the templateId field in ACS.daml updateEvent.

Seems reasonable 👍

cocreature · 2019-09-18T07:57:51Z

triggers/daml/Daml/Trigger.daml

-createCmd : Template t => TemplateId -> t -> Command
-createCmd templateId templateArg =
-  CreateCommand templateId (toLedgerValue templateArg)
+createCmd : Template t => t -> Command


Very nice, this is starting to look very usable!

cocreature · 2019-09-18T07:59:11Z

triggers/runner/src/main/scala/com/daml/trigger/Runner.scala

@@ -177,13 +181,21 @@ object Converter {
      ("name", SText(id.entityName)))
  }

+  private def fromAnyContractId(triggerIds: TriggerIds, templateId: value.Identifier, contractId: String): SValue = {


Should this take a value of type AnyContractId? Seems more consistent with toAnyContractId.

In principle that would be more consistent, yes. It's a bit noisy, though, because here we have a value.Identifier, but AnyContractId holds an Identifier. So, we'd be converting back and forth between the two identifier types.

Ah I see, makes sense to keep it this way then. There are clearly too many things called Identifier here 🙂

triggers/runner/src/main/scala/com/daml/trigger/Runner.scala

cocreature · 2019-09-18T08:04:06Z

triggers/tests/daml/ACS.daml

-    updateAcs acs (Created _ cId tId)
-      | tId.entityName == "Asset" = TM.insert cId tId acs
+    updateAcs acs (Created _ cId)
+      | cId.templateId.entityName == "Asset" = TM.insert cId.contractId cId.templateId acs


I’m confused, it looks like you don’t expose AnyContractId (or more likely I’m too stupid to see it) but you can still access the fields?

Indeed, that's why I didn't notice that I hadn't exposed it. I don't know how the field is accessible without export. The code still compiles, even if I explicitly export the type without the constructors.

Oh I think that might be because of how our record desugaring (the dot syntax used here) works. That does seem rather confusing and potentially problematic since it removes the ability to have abstract record types.

hurryabit

Nice. Thanks.

hurryabit · 2019-09-19T06:38:30Z

triggers/daml/Daml/Trigger.daml

@@ -24,6 +25,11 @@ data Identifier = Identifier
  , entityName : Text
  } deriving (Show, Eq)

+data AnyContractId = AnyContractId
+  { templateId : Identifier
+  , contractId : Text


Can we use something like ContractId () here? There as a DAML-LF function coerce_contract_id : ContractId a -> ContractId b which we could use then.

We could but I think it makes more sense to keep this consistent with AnyTemplate. I also deliberately don’t want to use the DAML-LF ContractId type to avoid annoyances around the absence of things like Ord instances.

aherrmann added 6 commits September 18, 2019 09:31

Infer templateId from createCmd argument

6c8201e

Infer templateId and choiceName in exerciseCmd

1a2cb49

Use exerciseCmd in ACS test

b8563d3

Add a layer of indirection to test `exerciseCmd` in the ACS test. The trigger first creates a `AssetMirrorProposal`, only when the proposal has been created, will it exercise the `Accept` choice to create the actual `AssetMirror` contract.

Remove toTemplateId

a857f57

getTemplateId --> toTemplateId

c73c401

Tuple2 --> AnyContractId

b40acf9

aherrmann-da requested a review from cocreature September 18, 2019 07:43

cocreature approved these changes Sep 18, 2019

View reviewed changes

aherrmann added 3 commits September 18, 2019 10:06

Run Scala formatter

35cea55

Export AnyContractId

9a00500

extractTemplateId|ChoiceName

833cab6

aherrmann-da added the automerge label Sep 18, 2019

mergify bot merged commit df7262e into master Sep 18, 2019

mergify bot deleted the trigger-infer-ids branch September 18, 2019 10:06

hurryabit reviewed Sep 19, 2019

View reviewed changes

cocreature added this to the DAML Triggers milestone Oct 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infer templateId and choiceName in daml trigger commands #2945

Infer templateId and choiceName in daml trigger commands #2945

aherrmann-da commented Sep 18, 2019

cocreature left a comment

cocreature Sep 18, 2019

aherrmann-da Sep 18, 2019

cocreature Sep 18, 2019

aherrmann-da Sep 18, 2019

cocreature Sep 18, 2019

cocreature Sep 18, 2019

cocreature Sep 18, 2019

aherrmann-da Sep 18, 2019

cocreature Sep 18, 2019

cocreature Sep 18, 2019

aherrmann-da Sep 18, 2019

cocreature Sep 18, 2019

hurryabit left a comment

hurryabit Sep 19, 2019

cocreature Sep 19, 2019

Infer templateId and choiceName in daml trigger commands #2945

Infer templateId and choiceName in daml trigger commands #2945

Conversation

aherrmann-da commented Sep 18, 2019

Pull Request Checklist

cocreature left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hurryabit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment