Prohibit contract IDs in contract keys and add key maintainers to exercises #4048

oggy- · 2020-01-14T19:05:43Z

As discussed with @bame-da @remyhaemmerle-da and @gerolf-da , this completely removes the support for contract IDs in contract keys. This is a backwards incompatible change, but lookupByKey already didn't support such keys, so it is unlikely that this was used.

Additionally, this adds key maintainers to NodeExercises. I also took the opportunity to remove some duplication in the compiler around keys and maintainers.

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
Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
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.

oggy- · 2020-01-14T20:00:33Z

Mmm, OK, I'm getting some CI errors that I can't make sense of, I will figure it out tomorrow and reopen the PR

…rcises CHANGELOG_BEGIN - [DAML-LF] Prohibit contract IDs in contract keys completely. Previously, creating keys containing absolute (but not relative) contract IDs was allowed, but `lookupByKey` on such a key would crash. Minor backwards incompatibility. CHANGELOG_END

digitalasset-cla · 2020-01-14T20:30:20Z

All committers have signed the CLA.

remyhaemmerle-da

Nice thanks.
Though, I would like that @S11001001 takes a look at transaction encoding code.

remyhaemmerle-da · 2020-01-15T09:46:24Z

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

@@ -59,6 +59,7 @@ private[digitalasset] object VersionTimeline {
      //  * change the following line when LF 1.8 is frozen.
      //  * do not insert line after this once until 1.8 is frozen.
      This(This(ValueVersion("7"))),
+      This(That(TransactionVersion("9"))),


Move this line before the FIXME comment. between line 57 and 58.
It is fine since GenMap (the only addition to value version 7) has never been release.

Changed now

daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/types/Ledger.scala

remyhaemmerle-da · 2020-01-15T10:14:15Z

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

                ()
              }
-            case (None, _) | (Some(_), true) =>
+            case (None, _, _) | (Some(_), true, _) =>
              Right(())


[Cosmetic]
I found this pattern matching with booleans a bit confusing.
I woul prefer something like

_ <- e.key match { case Some(KeyWithMaintainers(k, maintainers)) if !(transactionVersion precedes minContractKeyInExercise) => if ((transactionVersion precedes minMaintainersInExercise) && maintainers.nonEmpty) Left(EncodeError(transactionVersion, isTooOldFor = "maintainers in NodeExercises")) else encodeVal(k).map { encodedKey => exBuilder.setContractKey(encodedKey._2) exBuilder.addAllKeyMaintainers(maintainers.toSet[String].asJava) () } case _ => Right(()) }

I also messed up the logic here. I also changed the style now but I'm ambivalent to that.

remyhaemmerle-da · 2020-01-15T10:14:56Z

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

@@ -232,9 +239,9 @@ object TransactionCoder {
      keyWithMaintainers: TransactionOuterClass.KeyWithMaintainers)
    : Either[DecodeError, KeyWithMaintainers[Val]] =
    for {
-      mainteners <- toPartySet(keyWithMaintainers.getMaintainersList)


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

remyhaemmerle-da

Please change the order of This(That(TransactionVersion("9"))) in VersionTimeline.

Co-Authored-By: Remy <remy.haemmerle@daml.com>

…ansaction/TransactionCoder.scala Co-Authored-By: Remy <remy.haemmerle@daml.com>

remyhaemmerle-da · 2020-01-15T12:24:20Z

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

@@ -55,6 +55,7 @@ private[digitalasset] object VersionTimeline {
      This(That(TransactionVersion("8"))),
      Both(This(ValueVersion("5")), LanguageVersion(LMV.V1, "6")),
      Both(This(ValueVersion("6")), LanguageVersion(LMV.V1, "7")),
+      This(That(TransactionVersion("9"))),


S11001001

To be clear, you intend to invalidate prior valid transactions with contract IDs in contract keys. I am not really comfortable with this, but will defer to @remyhaemmerle-da's judgement.

As defined, it would be improper to only mention this restriction in a since version 9 section in transaction.rst (as commented elserwhere), since you are imposing the restriction retroactively.

S11001001 · 2020-01-15T16:54:33Z

...f/transaction/src/test/scala/com/digitalasset/daml/lf/transaction/TransactionCoderSpec.scala

@@ -315,6 +322,8 @@ class TransactionCoderSpec
        (x: GenNode[Nid, Cid, Val]) => withoutExerciseResult(withoutContractKeyInExercise(x)))
    else if (txvMin precedes minContractKeyInExercise)
      transactionWithout(tx, (x: GenNode[Nid, Cid, Val]) => withoutContractKeyInExercise(x))
+    else if (txvMin precedes minMaintainersInExercise)
+      transactionWithout(tx, (x: GenNode[Nid, Cid, Val]) => withoutMaintainersInExercise(x))


You need to do withoutMaintainersInExercise in both prior if branches (see how withoutContractKeyInExercise is also done in the branch prior its precedence test), or (ideally) reorganize this function so the precedence tests are independent of each other.

Added and reorganized. Less efficient when no changes need to be done, but this is test code.

S11001001 · 2020-01-15T17:09:39Z

daml-lf/spec/transaction.rst

@@ -503,6 +505,11 @@ Containing the result of the exercised choice.
 New optional field `contract_key` is now set when the exercised
 contract has a contract key defined.


Restriction on key contents must be mentioned here, and also under message KeyWithMaintainers.

Incidentally, it would be cleaner to use message KeyWithMaintainers instead of adding field key_maintainers as well, but I'll let @remyhaemmerle-da speak on that.

Added and using KeyWithMaintainers now.

The restriction still must be mentioned here, because this part does not use KeyWithMaintainers, even though it will in version 9.

S11001001 · 2020-01-16T14:29:15Z

daml-lf/spec/transaction.rst

+
+The key may not contain contract IDs. This change is backwards
+incompatible; that is, transactions containing contract IDs in keys
+cannot be deserialized any more.


As mentioned in a previous comment

it would be improper to only mention this restriction in a since version 9 section in transaction.rst (as commented elsewhere), since you are imposing the restriction retroactively.

That's because you can always read the spec such that if you are dealing with version n, you can ignore all sections since version e where e > n; that is not the case here.

Effectively, the restriction you are introducing is since version 3, in other words, on all KeyWithMaintainers.

S11001001 · 2020-01-16T14:29:56Z

daml-lf/spec/transaction.rst

@@ -503,6 +505,11 @@ Containing the result of the exercised choice.
 New optional field `contract_key` is now set when the exercised
 contract has a contract key defined.


The restriction still must be mentioned here, because this part does not use KeyWithMaintainers, even though it will in version 9.

…transactions.rst

S11001001

With these changes we make clear that the new restriction is not associated with version 9.

daml-lf/spec/transaction.rst

Co-Authored-By: Stephen Compall <stephen.compall@daml.com>

…-asset/daml into add-key-maintainers-to-exercises

remyhaemmerle-da

Need futher discussion with me and @gerolf-da

remyhaemmerle-da

Please add to the changelog something to explain the change in contract key.
Thanks for the help.

oggy- requested review from a user, gerolf-da, bame-da and remyhaemmerle-da January 14, 2020 19:05

oggy- requested review from leo-da, nemanja-da, rautenrieth-da, S11001001, SamirTalwar and stefanobaghino-da as code owners January 14, 2020 19:05

oggy- force-pushed the add-key-maintainers-to-exercises branch from 8e550cc to 1f1c197 Compare January 14, 2020 19:27

oggy- closed this Jan 14, 2020

oggy- reopened this Jan 14, 2020

oggy- force-pushed the add-key-maintainers-to-exercises branch from 1f1c197 to 1d1f228 Compare January 14, 2020 20:30

remyhaemmerle-da reviewed Jan 15, 2020

View reviewed changes

remyhaemmerle-da requested changes Jan 15, 2020

View reviewed changes

oggy- and others added 5 commits January 15, 2020 12:07

Apply suggestions from code review

f05f20f

Co-Authored-By: Remy <remy.haemmerle@daml.com>

Update daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/tr…

baac6bc

…ansaction/TransactionCoder.scala Co-Authored-By: Remy <remy.haemmerle@daml.com>

Address remy's comments + fix some suggested changes

62e33ee

scalafmt

3b78600

Fix transaction encoding

8fa86b2

remyhaemmerle-da reviewed Jan 15, 2020

View reviewed changes

scalafmt

2e037a6

oggy- requested a review from remyhaemmerle-da January 15, 2020 15:13

S11001001 requested changes Jan 15, 2020

View reviewed changes

remyhaemmerle-da approved these changes Jan 15, 2020

View reviewed changes

Stephen's comments

b4024ad

oggy- requested a review from S11001001 January 16, 2020 11:39

S11001001 requested changes Jan 16, 2020

View reviewed changes

Address the backwards incompatibility in the appropriate sections of …

042f27d

…transactions.rst

oggy- requested a review from S11001001 January 16, 2020 15:32

Merge branch 'master' into add-key-maintainers-to-exercises

7347c49

S11001001 requested changes Jan 16, 2020

View reviewed changes

daml-lf/spec/transaction.rst Outdated Show resolved Hide resolved

daml-lf/spec/transaction.rst Show resolved Hide resolved

daml-lf/spec/transaction.rst Outdated Show resolved Hide resolved

daml-lf/spec/transaction.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

d90360c

Co-Authored-By: Stephen Compall <stephen.compall@daml.com>

S11001001 approved these changes Jan 16, 2020

View reviewed changes

oggy- added 2 commits January 16, 2020 17:55

Adjust KV utils to the .proto change

342c158

Merge branch 'add-key-maintainers-to-exercises' of github.com:digital…

5180d55

…-asset/daml into add-key-maintainers-to-exercises

remyhaemmerle-da requested changes Jan 17, 2020

View reviewed changes

remyhaemmerle-da approved these changes Jan 20, 2020

View reviewed changes

Treat as a bugfix

dc2b942

oggy- merged commit 589f710 into master Jan 20, 2020

oggy- deleted the add-key-maintainers-to-exercises branch January 20, 2020 15:36

remyhaemmerle-da mentioned this pull request Jun 5, 2020

Remove support for absolute contract IDs in contract keys #3634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prohibit contract IDs in contract keys and add key maintainers to exercises #4048

Prohibit contract IDs in contract keys and add key maintainers to exercises #4048

oggy- commented Jan 14, 2020

oggy- commented Jan 14, 2020

digitalasset-cla commented Jan 14, 2020 •

edited

Loading

remyhaemmerle-da left a comment

remyhaemmerle-da Jan 15, 2020 •

edited

Loading

oggy- Jan 15, 2020

remyhaemmerle-da Jan 15, 2020

oggy- Jan 15, 2020

remyhaemmerle-da Jan 15, 2020

remyhaemmerle-da left a comment

remyhaemmerle-da Jan 15, 2020

S11001001 left a comment

S11001001 Jan 15, 2020

oggy- Jan 16, 2020

S11001001 Jan 15, 2020

oggy- Jan 16, 2020

S11001001 Jan 16, 2020

S11001001 Jan 16, 2020

S11001001 Jan 16, 2020

S11001001 left a comment

remyhaemmerle-da left a comment

remyhaemmerle-da left a comment

		@@ -503,6 +505,11 @@ Containing the result of the exercised choice.
		New optional field `contract_key` is now set when the exercised
		contract has a contract key defined.

Prohibit contract IDs in contract keys and add key maintainers to exercises #4048

Prohibit contract IDs in contract keys and add key maintainers to exercises #4048

Conversation

oggy- commented Jan 14, 2020

Pull Request Checklist

oggy- commented Jan 14, 2020

digitalasset-cla commented Jan 14, 2020 • edited Loading

remyhaemmerle-da left a comment

Choose a reason for hiding this comment

remyhaemmerle-da Jan 15, 2020 • edited Loading

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

remyhaemmerle-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S11001001 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

S11001001 left a comment

Choose a reason for hiding this comment

remyhaemmerle-da left a comment

Choose a reason for hiding this comment

remyhaemmerle-da left a comment

Choose a reason for hiding this comment

digitalasset-cla commented Jan 14, 2020 •

edited

Loading

remyhaemmerle-da Jan 15, 2020 •

edited

Loading