LF: Move KV's InputsAndEffects to LF land #9361

remyhaemmerle-da · 2021-04-08T13:15:24Z

We move the code from computing from KV to LF.
Testing + more refactoring will follows in subsequent PRs.

CHANGELOG_BEGIN
CHANGELOG_END

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.

remyhaemmerle-da · 2021-04-08T13:16:53Z

@andreaslochbihler-da , @simonmaxen-da Do the function computeInputs and `computeEffects' are usefull for you ?

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

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

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

andreaslochbihler-da · 2021-04-08T15:09:15Z

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

+  *     contracts should be created. The key should be a combination of the transaction
+  *     id and the relative contract id (that is, the node index).
+  * @param keys
+  *     The contract keys created or updated as part of the transaction.


This information is not enough to check the preconditions of a transaction w.r.t. the ledger state. For example, it's not clear whether the keys should be unassigned or assigned at the beginning of the transaction.

Yes, I need this to.

andreaslochbihler-da

This information is not enough to check the preconditions of a transaction w.r.t. the ledger state. For example, it's not clear > whether the keys should be unassigned or assigned at the beginning of the transaction.

Let's tackle that in an upcoming PR.

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

andreaslochbihler-da · 2021-04-08T15:11:56Z

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

+
+        case exe: Node.NodeExercises[NodeId, Value.ContractId] =>
+          addContractInput(exe.targetCoid)
+          exe.key.foreach(addContractKey(exe.templateId, _))


Why do you have to add the key to the inputs if the byKey flag is not set?

~~We don't. If the byKey flag is not set, then exe.key = None.~~

Let's postpone semantics consideration to another PR.
For now, I just move the code around keeping as much as possible the original behavior of the utility as defined in KV.

andreaslochbihler-da

I don't think these functions are particularly useful for Canton as is.

CHANGELOG_BEGIN CHANGELOG_END

miklos-da

Looks good overall, thanks, @remyhaemmerle-da!

miklos-da · 2021-04-15T16:56:49Z

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

+  * @param contracts
+  *     The contracts fetched or looked up by the transaction.
+  * @param parties
+  *     The union of the informees of all the nodes of the transaction


Suggested change

* The union of the informees of all the nodes of the transaction

* The union of the informees of all the nodes of the transaction.

miklos-da · 2021-04-15T16:57:04Z

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

+
+  val Empty = Effects(TreeSet.empty, TreeMap.empty, TreeMap.empty)
+
+  /** Compute the effects of a DAML transaction, that is, the created and consumed contracts. */


Suggested change

/** Compute the effects of a DAML transaction, that is, the created and consumed contracts. */

/** Compute the effects of a Daml transaction, that is, the created and consumed contracts. */

miklos-da · 2021-04-15T16:57:20Z

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

+    updatedContractKeys: TreeMap[GlobalKey, Option[ContractId]],
+)
+
+/** utilities to compute the effects of a DAML transaction */


Feel free to remove this comment (i.e., there's only a single method here which is well-documented).

miklos-da · 2021-04-15T16:59:49Z

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

+  * @param parties
+  *     The union of the informees of all the nodes of the transaction
+  * @param keys
+  *     The contract keys created, exercise, fetched or lookup or updated


Suggested change

* The contract keys created, exercise, fetched or lookup or updated

* The contract keys created, exercised, fetched, looked up or updated

miklos-da · 2021-04-15T17:01:00Z

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

+  /** Compute the inputs to a DAML transaction (in traversal order), that is, the referenced contracts, parties,
+    *  and key.


Suggested change

/** Compute the inputs to a DAML transaction (in traversal order), that is, the referenced contracts, parties,

* and key.

/** Compute the inputs to a Daml transaction (in traversal order), that is, the referenced contracts, parties,

* and keys.

miklos-da · 2021-04-15T17:03:35Z

...ant-state/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/Conversions.scala


  def globalKeyToStateKey(key: GlobalKey): DamlStateKey = {
    DamlStateKey.newBuilder
      .setContractKey(encodeGlobalKey(key))
      .build
  }

+  def contractKeyToStateKey(tmplId: Identifier, key: Value[ContractId]): DamlStateKey =


Suggested change

def contractKeyToStateKey(tmplId: Identifier, key: Value[ContractId]): DamlStateKey =

def contractKeyToStateKey(templateId: Identifier, key: Value[ContractId]): DamlStateKey =

I applied this in #9429 and #9433

miklos-da · 2021-04-15T17:08:03Z

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

+
+import scala.collection.immutable.{TreeMap, TreeSet}
+
+/** The effects of a transaction, that is:


Are the comments in the original InputsAndEffects.Effects regarding semantics of consumed/create contracts captured somewhere else in the code?

miklos-da · 2021-04-15T17:11:25Z

...te/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/KeyValueSubmission.scala

+      val inputDamlStatesFromTx = new util.ArrayList[DamlStateKey]()
+      transaction.Inputs.collectInputs(
+        tx,
+        { cid => inputDamlStatesFromTx.add(Conversions.contractIdToStateKey(cid)); () },


Why not use native Scala types here for improving readability and calling .asJava when constructing the protobuf message?

I apply the suggestion in #9429

miklos-da · 2021-04-15T17:12:03Z

...icipant-state/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/package.scala

@@ -1,7 +1,8 @@
 // Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
 // SPDX-License-Identifier: Apache-2.0

-package com.daml.ledger.participant.state
+package com.daml


Please revert this change. We don't use this package notation in kvutils.

I revert this change in #9433

ghost · 2021-04-15T11:51:45Z

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

@@ -39,6 +39,8 @@ object GlobalKey {
  @throws[IllegalArgumentException]
  def assertBuild(templateId: Ref.TypeConName, key: Value[ContractId]): GlobalKey =
    data.assertRight(build(templateId, key))
+
+  implicit val ordering = Ordering.by[GlobalKey, crypto.Hash](_.hash)


I think this might work and it's a little terser.

Suggested change

implicit val ordering = Ordering.by[GlobalKey, crypto.Hash](_.hash)

implicit val ordering: Ordering[GlobalKey] = Ordering.by(_.hash)

remyhaemmerle-da · 2021-04-16T14:15:06Z

subsumed by #9429 and #9433

remyhaemmerle-da requested review from cocreature and miklos-da April 8, 2021 13:15

remyhaemmerle-da requested review from fabiotudone-da and gerolf-da as code owners April 8, 2021 13:15

remyhaemmerle-da requested a review from a user April 8, 2021 13:15

remyhaemmerle-da added the component/daml-lf DAML-LF label Apr 8, 2021

remyhaemmerle-da added this to the Maintenance milestone Apr 8, 2021

ghost reviewed Apr 8, 2021

View reviewed changes

daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/Util.scala Outdated Show resolved Hide resolved