Add contract keys to created events in the Ledger API #1586

stefanobaghino-da · 2019-06-11T14:17:40Z

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

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.

stefanobaghino-da · 2019-06-11T16:42:28Z

Note

I would greatly benefit from some input from either @S11001001 or @leo-da on how to reflect this change in the Scala bindings (so that this information can also be added to the navigator).

@gerolf-da suggested using a type dependent type on Templates, which seemed reasonable but didn't lead me anywhere. If you could provide some guidance on how you think this change can be added, it would really help me.

stefanobaghino-da · 2019-06-11T16:45:01Z

Nonetheless, I would consider merging this PR as-is (modulo the review) and add the Scala bindings and navigator support for contract keys as a separate contribution (just to keep things simple).

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

...indings-rxjava/src/test/scala/com/daml/ledger/rxjava/grpc/helpers/TransactionGenerator.scala

...uite/scala/com/digitalasset/platform/tests/integration/ledger/api/TransactionServiceIT.scala

...ger-api-common/src/main/scala/com/digitalasset/platform/participant/util/LfEngineToApi.scala

...uite/scala/com/digitalasset/platform/tests/integration/ledger/api/TransactionServiceIT.scala

gerolf-da

Thank you @stefanobaghino-da.

...uite/scala/com/digitalasset/platform/tests/integration/ledger/api/TransactionServiceIT.scala

Addresses #1586 (comment) Addresses #1586 (comment)

gerolf-da

Thanks, @stefanobaghino-da 👍

stefanobaghino-da · 2019-06-13T09:28:00Z

@S11001001 your explicit approval is required in order to merge this PR.

stefanobaghino-da · 2019-06-13T14:02:44Z

As discussed off-band with @gerolf-da, it would be desirable to improve the codegen by exposing contract keys

only if they are defined over the template, and
with the generated type rather then the raw bindings Value

gerolf-da

As discussed offline, please only generate a contract key field in the java codegen of the right type if there is an key defined in the model.

Also adds support in daml-lf/interface for contract keys Addresses #1586 (review)

stefanobaghino-da · 2019-06-14T10:36:21Z

@gerolf-da The latest commit should fix it, thanks for the guidance.

@S11001001 I added contract keys support to the interface reader, you may want to have a look at it.

gerolf-da · 2019-06-14T11:46:44Z

...degen/src/main/scala/com/digitalasset/daml/lf/codegen/backend/java/inner/TemplateClass.scala

+        contractKeyFieldName)
+    } else {
+      spec.addStatement(
+        "return new $T($L, $L, $L)",


You could build up the return statement with something like the following to avoid having to duplicate the statement itself:

val names = Vector(idFieldName, dataFieldName, agreementFieldName) ++ maybeContractKeyClassName.map(_ => contractKeyFieldName) val params = names.map(n => CodeBlock.of("$L", n)).asJava val allParams = CodeBlock.join(params, ", ") spec.addStatement("return new $T($L)", className, params)

Thanks, both my Scala and my javapoet became rusty after the last few months. 🙂

Addressed by 4249373

gerolf-da

Awesome, @stefanobaghino-da. Very smoothly integrated. 👏
Only had 1 comment that you could also apply to other places in this commit where you find you had to duplicate code depending on whether the key was defined or not.

gerolf-da

🎉

Fixes #1268

…/digitalasset/platform/tests/integration/ledger/api/TransactionServiceIT.scala Co-Authored-By: Stephen Compall <scompall@nocandysw.com>

…ledger/rxjava/grpc/helpers/TransactionGenerator.scala Co-Authored-By: Stephen Compall <scompall@nocandysw.com>

…/digitalasset/platform/tests/integration/ledger/api/TransactionServiceIT.scala Co-Authored-By: Stephen Compall <scompall@nocandysw.com>

Addresses #1586 (comment) Addresses #1586 (comment)

…orm/participant/util/LfEngineToApi.scala Co-Authored-By: Stephen Compall <scompall@nocandysw.com>

Applies the suggestion introduced by 7cc5c3e

Also adds support in daml-lf/interface for contract keys Addresses #1586 (review)

jberthold-da · 2019-06-18T02:01:13Z

...degen/src/main/scala/com/digitalasset/daml/lf/codegen/backend/java/inner/TemplateClass.scala

+  private val getArguments = CodeBlock.of("event.getArguments()")
+  private val getAgreementText = CodeBlock.of("event.getAgreementText()")
+  private def getContractKey(t: TypeName) =
+    CodeBlock.of("event.getContractKey().map($T::fromValue)", t)


Unfortunately this seems to not work as expected when t is a TupleN for some N.
The fromValue functions for tuples expect additional arguments to convert the components.

Totally right, fixed by #1743, thanks for the detailed report.

stefanobaghino-da force-pushed the ledger-api-contract-keys branch 3 times, most recently from 4426faa to 861f85c Compare June 11, 2019 16:39

stefanobaghino-da requested a review from gerolf-da June 11, 2019 16:39

stefanobaghino-da marked this pull request as ready for review June 11, 2019 16:39

stefanobaghino-da requested review from bethaitman, leo-da, nicholassmith-da and S11001001 as code owners June 11, 2019 16:39

stefanobaghino-da force-pushed the ledger-api-contract-keys branch 4 times, most recently from d99c68b to 33bb02e Compare June 12, 2019 16:45

S11001001 requested changes Jun 12, 2019

View reviewed changes

gerolf-da reviewed Jun 13, 2019

View reviewed changes

...uite/scala/com/digitalasset/platform/tests/integration/ledger/api/TransactionServiceIT.scala Outdated Show resolved Hide resolved

...uite/scala/com/digitalasset/platform/tests/integration/ledger/api/TransactionServiceIT.scala Outdated Show resolved Hide resolved

stefanobaghino-da added a commit that referenced this pull request Jun 13, 2019

Add run suffix to transaction service integration tests

f8650fb

Addresses #1586 (comment) Addresses #1586 (comment)

stefanobaghino-da added a commit that referenced this pull request Jun 13, 2019

Re-introduce unused bindings

cae6a33

Addresses #1586 (comment) Addresses #1586 (comment)

stefanobaghino-da requested review from S11001001 and gerolf-da June 13, 2019 09:13

stefanobaghino-da added the automerge label Jun 13, 2019

gerolf-da approved these changes Jun 13, 2019

View reviewed changes

stefanobaghino-da removed the automerge label Jun 13, 2019

S11001001 approved these changes Jun 13, 2019

View reviewed changes

gerolf-da requested changes Jun 13, 2019

View reviewed changes

stefanobaghino-da force-pushed the ledger-api-contract-keys branch from dd21811 to c65b649 Compare June 14, 2019 10:35

stefanobaghino-da added a commit that referenced this pull request Jun 14, 2019

Contract keys to be typed after generated types

c65b649

Also adds support in daml-lf/interface for contract keys Addresses #1586 (review)

stefanobaghino-da requested review from gerolf-da and S11001001 June 14, 2019 10:36

gerolf-da reviewed Jun 14, 2019

View reviewed changes

stefanobaghino-da added a commit that referenced this pull request Jun 14, 2019

Address #1586 (comment)

4249373

gerolf-da approved these changes Jun 14, 2019

View reviewed changes

stefanobaghino-da added the automerge label Jun 14, 2019

stefanobaghino-da force-pushed the ledger-api-contract-keys branch from 6240a38 to bbb2a07 Compare June 14, 2019 15:24

stefanobaghino-da and others added 12 commits June 14, 2019 18:11

Add contract keys to created events in the Ledger API

ae7d93a

Fixes #1268

Update ledger/ledger-api-integration-tests/src/test/itsuite/scala/com…

36dc49f

…/digitalasset/platform/tests/integration/ledger/api/TransactionServiceIT.scala Co-Authored-By: Stephen Compall <scompall@nocandysw.com>

Update language-support/java/bindings-rxjava/src/test/scala/com/daml/…

c00f1ff

…ledger/rxjava/grpc/helpers/TransactionGenerator.scala Co-Authored-By: Stephen Compall <scompall@nocandysw.com>

Update ledger/ledger-api-integration-tests/src/test/itsuite/scala/com…

03967a6

…/digitalasset/platform/tests/integration/ledger/api/TransactionServiceIT.scala Co-Authored-By: Stephen Compall <scompall@nocandysw.com>

Add run suffix to transaction service integration tests

762521d

Addresses #1586 (comment) Addresses #1586 (comment)

Re-introduce unused bindings

e5f959d

Addresses #1586 (comment) Addresses #1586 (comment)

Update ledger/ledger-api-common/src/main/scala/com/digitalasset/platf…

d81fa43

…orm/participant/util/LfEngineToApi.scala Co-Authored-By: Stephen Compall <scompall@nocandysw.com>

Use newly introduce assertOrRuntimeEx method

b8963a4

Applies the suggestion introduced by 7cc5c3e

Contract keys to be typed after generated types

f585a23

Also adds support in daml-lf/interface for contract keys Addresses #1586 (review)

Address #1586 (comment)

b012282

Fix compilation error from previous commit

60d4e01

Fix compilation errors in navigator

20b0f18

stefanobaghino-da force-pushed the ledger-api-contract-keys branch from bbb2a07 to 20b0f18 Compare June 14, 2019 16:11

mergify bot merged commit 6ba26f4 into master Jun 14, 2019

mergify bot deleted the ledger-api-contract-keys branch June 14, 2019 16:45

This was referenced Jun 14, 2019

Add key to all events #1680

Closed

Add contract key support to Scala bindings and navigator #1681

Closed

Add contract key support for created events to Haskell bindings #1688

Closed

jberthold-da reviewed Jun 18, 2019

View reviewed changes

jberthold-da mentioned this pull request Jun 18, 2019

Java codegen produces invalid code for keys composed of tuples #1728

Closed

S11001001 mentioned this pull request Jul 10, 2020

include contract keys in the Scala codegen output plan when using --root #6696

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add contract keys to created events in the Ledger API #1586

Add contract keys to created events in the Ledger API #1586

stefanobaghino-da commented Jun 11, 2019 •

edited

stefanobaghino-da commented Jun 11, 2019

stefanobaghino-da commented Jun 11, 2019

gerolf-da left a comment

gerolf-da left a comment

stefanobaghino-da commented Jun 13, 2019

stefanobaghino-da commented Jun 13, 2019

gerolf-da left a comment

stefanobaghino-da commented Jun 14, 2019

gerolf-da Jun 14, 2019

stefanobaghino-da Jun 14, 2019

stefanobaghino-da Jun 14, 2019

gerolf-da left a comment

gerolf-da left a comment

jberthold-da Jun 18, 2019

stefanobaghino-da Jun 18, 2019

Add contract keys to created events in the Ledger API #1586

Add contract keys to created events in the Ledger API #1586

Conversation

stefanobaghino-da commented Jun 11, 2019 • edited

Pull Request Checklist

stefanobaghino-da commented Jun 11, 2019

Note

stefanobaghino-da commented Jun 11, 2019

gerolf-da left a comment

Choose a reason for hiding this comment

gerolf-da left a comment

Choose a reason for hiding this comment

stefanobaghino-da commented Jun 13, 2019

stefanobaghino-da commented Jun 13, 2019

gerolf-da left a comment

Choose a reason for hiding this comment

stefanobaghino-da commented Jun 14, 2019

gerolf-da Jun 14, 2019

Choose a reason for hiding this comment

stefanobaghino-da Jun 14, 2019

Choose a reason for hiding this comment

stefanobaghino-da Jun 14, 2019

Choose a reason for hiding this comment

gerolf-da left a comment

Choose a reason for hiding this comment

gerolf-da left a comment

Choose a reason for hiding this comment

jberthold-da Jun 18, 2019

Choose a reason for hiding this comment

stefanobaghino-da Jun 18, 2019

Choose a reason for hiding this comment

stefanobaghino-da commented Jun 11, 2019 •

edited