Fix disclosure bug in the presence of upgrades (#19167)

* repro of #19162 as a test and fix for daml3 * use a different party for exercising a choice on the disclosed contract * factorize TemplateTypeRep svalue creation * Move makePair/makeTriplet to Converter.makeTuple
2024-09-19 16:57:40 +03:00 · 2024-05-14 17:51:50 +02:00 · 2024-05-14 17:51:50 +02:00 · 31453459fe
commit 31453459fe
parent c1ced1fb8b
11 changed files with 169 additions and 94 deletions
--- a/sdk/daml-script/converter/BUILD.bazel
+++ b/sdk/daml-script/converter/BUILD.bazel
@ -20,6 +20,7 @@ da_scala_library(
        "//daml-lf/data",
        "//daml-lf/interpreter",
        "//daml-lf/language",
+        "//daml-lf/stable-packages",
        "//daml-lf/transaction",
    ],
 )
--- a/sdk/daml-script/converter/src/main/scala/com/daml/script/converter/Converter.scala
+++ b/sdk/daml-script/converter/src/main/scala/com/daml/script/converter/Converter.scala
@ -3,16 +3,18 @@

 package com.daml.script.converter

+import com.daml.lf.data.ImmArray
+import com.daml.lf.data.Ref._
+import com.daml.lf.language.Ast
+import com.daml.lf.speedy.SValue._
+import com.daml.lf.speedy.{ArrayList, SValue}
+import com.daml.lf.stablepackages.StablePackagesV2
+import com.daml.lf.value.Value.ContractId
+import scalaz.std.either._
+import scalaz.syntax.bind._
+
 import java.util
 import scala.jdk.CollectionConverters._
-import scalaz.syntax.bind._
-import scalaz.std.either._
-import com.daml.lf.data.{ImmArray, Ref}
-import Ref._
-import com.daml.lf.language.Ast
-import com.daml.lf.speedy.{ArrayList, SValue}
-import SValue._
-import com.daml.lf.value.Value.ContractId

 class ConverterException(message: String) extends RuntimeException(message)

@ -35,6 +37,12 @@ private[daml] object Converter {
    SRecord(ty, fieldNames, args) // TODO: construct SRecord directly from Map
  }

+  def makeTuple(v1: SValue, v2: SValue): SValue =
+    record(StablePackagesV2.Tuple2, ("_1", v1), ("_2", v2))
+
+  def makeTuple(v1: SValue, v2: SValue, v3: SValue): SValue =
+    record(StablePackagesV2.Tuple3, ("_1", v1), ("_2", v2), ("_3", v3))
+
  /** Unpack one step of a Pure/Roll-style free monad representation,
    * with the assumption that `f` is a variant type.
    */
--- a/sdk/daml-script/daml/Daml/Script.daml
+++ b/sdk/daml-script/daml/Daml/Script.daml
@ -302,7 +302,7 @@ data QueryDisclosurePayload a = QueryDisclosurePayload
  { parties : [Party]
  , tplId : TemplateTypeRep
  , cid : ContractId ()
-  , continue : Optional Text -> a
+  , continue : Optional (TemplateTypeRep, Text) -> a
  , locations : [(Text, SrcLoc)]
  } deriving Functor

@ -317,7 +317,7 @@ queryDisclosure p c = lift $ Free $ QueryDisclosure QueryDisclosurePayload with
    parties = toParties p
    tplId = templateTypeRep @t
    cid = cid
-    continue = pure . fmap \blob -> Disclosure tplId cid blob
+    continue = pure . fmap \(actualTplId, blob) -> Disclosure actualTplId cid blob
    locations = getCallStack callStack
  where
    tplId = templateTypeRep @t
--- a/sdk/daml-script/daml3/Daml/Script/Internal/Questions/Query.daml
+++ b/sdk/daml-script/daml3/Daml/Script/Internal/Questions/Query.daml
@ -36,11 +36,15 @@ data QueryContractId = QueryContractId with
  parties : [Party]
  tplId : TemplateTypeRep
  cid : ContractId ()
-instance IsQuestion QueryContractId (Optional (AnyTemplate, Text)) where command = "QueryContractId"
+instance IsQuestion QueryContractId (Optional (AnyTemplate, TemplateTypeRep, Text)) where command = "QueryContractId"

 -- | Query for the contract with the given contract id.
 --
 -- Returns `None` if there is no active contract the party is a stakeholder on.
+-- Otherwise returns a triplet (anyTemplate, templateId, blob) where anyTemplate
+-- is the contract upgraded or downgraded to `t`, templateId is the ID of the
+-- template as stored in the ledger (may be different from `t`), and blob is the
+-- disclosure of the template as stored in the ledger (of type templateId).
 --
 -- WARNING: Over the gRPC and with the JSON API
 -- in-memory backend this performs a linear search so only use this if the number of
@ -48,7 +52,7 @@ instance IsQuestion QueryContractId (Optional (AnyTemplate, Text)) where command
 --
 -- This is semantically equivalent to calling `query`
 -- and filtering on the client side.
-queryContractId_ : forall t p. (Template t, IsParties p) => HasCallStack => p -> ContractId t -> Script (Optional (AnyTemplate, Text))
+queryContractId_ : forall t p. (Template t, IsParties p) => HasCallStack => p -> ContractId t -> Script (Optional (AnyTemplate, TemplateTypeRep, Text))
 queryContractId_ p c = lift $ QueryContractId with
    parties = toParties p
    tplId = templateTypeRep @t
@ -58,14 +62,13 @@ queryContractId_ p c = lift $ QueryContractId with
 --    convert = fmap $ fromSome . fromAnyTemplate

 queryContractId: forall t p. (Template t, HasEnsure t, IsParties p) => HasCallStack => p -> ContractId t -> Script (Optional t)
-queryContractId p c = fmap (fmap $ fromSome . fromAnyTemplate . fst) $ queryContractId_ p c
+queryContractId p c = fmap (fmap $ \(anyTpl, _, _) -> fromSome (fromAnyTemplate anyTpl)) $ queryContractId_ p c

 -- TODO https://github.com/digital-asset/daml/issues/17755
 --  clean the API for different query function
 queryDisclosure: forall t p. (Template t, IsParties p) => HasCallStack => p -> ContractId t -> Script (Optional Disclosure)
-queryDisclosure p c = fmap (fmap  $ \(_, blob) -> Disclosure tplId cid blob) $ queryContractId_ p c
+queryDisclosure p c = fmap (fmap  $ \(_, tplId, blob) -> Disclosure tplId cid blob) $ queryContractId_ p c
 where
-    tplId = templateTypeRep @t
    cid = coerceContractId c

 data QueryInterface = QueryInterface with
--- a/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/Converter.scala
+++ b/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/Converter.scala
@ -104,8 +104,14 @@ abstract class ConverterMethods(stablePackages: StablePackages) {
    )
  }

+  private[lf] def fromTemplateTypeRep(templateId: SValue): SValue =
+    record(stablePackages.TemplateTypeRep, ("getTemplateTypeRep", templateId))
+
  private[lf] def fromTemplateTypeRep(templateId: value.Identifier): SValue =
-    record(stablePackages.TemplateTypeRep, ("getTemplateTypeRep", fromIdentifier(templateId)))
+    fromTemplateTypeRep(fromIdentifier(templateId))
+
+  private[lf] def fromTemplateTypeRep(templateId: Identifier): SValue =
+    fromTemplateTypeRep(STypeRep(TTyCon(templateId)))

  private[lf] def fromAnyContractId(
      scriptIds: ScriptIds,
@ -246,10 +252,7 @@ abstract class ConverterMethods(stablePackages: StablePackages) {
      ("getAnyContractKey", SAny(key.ty, key.key)),
      (
        "getAnyContractKeyTemplateTypeRep",
-        record(
-          stablePackages.TemplateTypeRep,
-          ("getTemplateTypeRep", STypeRep(TTyCon(key.templateId))),
-        ),
+        fromTemplateTypeRep(key.templateId),
      ),
    )

--- a/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/v1/Converter.scala
+++ b/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/v1/Converter.scala
@ -214,10 +214,9 @@ object Converter extends script.ConverterMethods(StablePackagesV2) {
  ): Either[String, SValue] = {
    for {
      anyTpl <- fromContract(translator, contract)
-    } yield record(
-      StablePackagesV2.Tuple2,
-      ("_1", SContractId(contract.contractId)),
-      ("_2", anyTpl),
+    } yield makeTuple(
+      SContractId(contract.contractId),
+      anyTpl,
    )
  }

--- a/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/v1/ScriptF.scala
+++ b/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/v1/ScriptF.scala
@ -6,31 +6,29 @@ package engine
 package script
 package v1

-import java.time.Clock
-import org.apache.pekko.stream.Materializer
 import com.daml.grpc.adapter.ExecutionSequencerFactory
-import com.digitalasset.canton.ledger.api.domain.{User, UserRight}
 import com.daml.lf.data.FrontStack
-import com.daml.lf.{CompiledPackages, command}
-import com.daml.lf.engine.preprocessing.ValueTranslator
-import com.daml.lf.data.Ref.{Identifier, Name, PackageId, Party, UserId}
+import com.daml.lf.data.Ref._
 import com.daml.lf.data.Time.Timestamp
-import com.daml.lf.language.{Ast}
-import com.daml.lf.speedy.SExpr.{SEAppAtomic, SEValue}
-import com.daml.lf.speedy.{ArrayList, SError, SValue}
-import com.daml.lf.speedy.SExpr.SExpr
+import com.daml.lf.engine.preprocessing.ValueTranslator
+import com.daml.lf.language.Ast
+import com.daml.lf.speedy.SExpr.{SEAppAtomic, SEValue, SExpr}
 import com.daml.lf.speedy.SValue._
 import com.daml.lf.speedy.Speedy.PureMachine
-import com.daml.lf.stablepackages.StablePackagesV2
+import com.daml.lf.speedy.{ArrayList, SError, SValue}
 import com.daml.lf.value.Value
 import com.daml.lf.value.Value.ContractId
-import scalaz.{Foldable, OneAnd}
-import scalaz.syntax.traverse._
+import com.daml.lf.{CompiledPackages, command}
+import com.daml.script.converter.Converter.{makeTuple, toContractId, toText}
+import com.digitalasset.canton.ledger.api.domain.{User, UserRight}
+import org.apache.pekko.stream.Materializer
 import scalaz.std.either._
 import scalaz.std.list._
 import scalaz.std.option._
-import com.daml.script.converter.Converter.{toContractId, toText}
+import scalaz.syntax.traverse._
+import scalaz.{Foldable, OneAnd}

+import java.time.Clock
 import scala.concurrent.{ExecutionContext, Future}

 sealed trait ScriptF
@ -231,6 +229,7 @@ object ScriptF {
      } yield SEAppAtomic(SEValue(continue), Array(SEValue(SList(res))))

  }
+
  final case class QueryContractId(
      parties: OneAnd[Set, Party],
      tplId: Identifier,
@ -251,7 +250,14 @@ object ScriptF {
        optR <- client.queryContractId(parties, tplId, cid)
        optR <- Converter.toFuture(
          if (asDisclosure)
-            Right(optR.map(c => SValue.SText(c.blob.toHexString)))
+            Right(
+              optR.map(c =>
+                makeTuple(
+                  Converter.fromTemplateTypeRep(c.templateId),
+                  SValue.SText(c.blob.toHexString),
+                )
+              )
+            )
          else
            optR.traverse(Converter.fromContract(env.valueTranslator, _))
        )
@ -272,11 +278,6 @@ object ScriptF {
        esf: ExecutionSequencerFactory,
    ): Future[SExpr] = {

-      def makePair(v1: SValue, v2: SValue): SValue = {
-        import com.daml.script.converter.Converter.record
-        record(StablePackagesV2.Tuple2, ("_1", v1), ("_2", v2))
-      }
-
      for {
        viewType <- Converter.toFuture(env.lookupInterfaceViewTy(interfaceId))
        client <- Converter.toFuture(env.clients.getPartiesParticipant(parties))
@ -287,7 +288,7 @@ object ScriptF {
            .traverse { case (cid, optView) =>
              optView match {
                case None =>
-                  Right(makePair(SContractId(cid), SOptional(None)))
+                  Right(makeTuple(SContractId(cid), SOptional(None)))
                case Some(view) =>
                  for {
                    view <- Converter.fromInterfaceView(
@ -296,7 +297,7 @@ object ScriptF {
                      view,
                    )
                  } yield {
-                    makePair(SContractId(cid), SOptional(Some(view)))
+                    makeTuple(SContractId(cid), SOptional(Some(view)))
                  }
              }
            }
--- a/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/v2/Converter.scala
+++ b/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/v2/Converter.scala
@ -221,26 +221,23 @@ object Converter extends script.ConverterMethods(StablePackagesV2) {
      )
      .map { xs => SList(xs.to(FrontStack)) }

-  // Convert an active contract to AnyTemplate
-  def fromContract(
-      translator: preprocessing.ValueTranslator,
-      contract: ScriptLedgerClient.ActiveContract,
-      enableContractUpgrading: Boolean = false,
-  ): Either[String, SValue] =
-    fromAnyTemplate(translator, contract.templateId, contract.argument, enableContractUpgrading)
-
  // Convert a Created event to a pair of (ContractId (), AnyTemplate)
  def fromCreated(
      translator: preprocessing.ValueTranslator,
      contract: ScriptLedgerClient.ActiveContract,
+      targetTemplateId: Identifier,
      enableContractUpgrading: Boolean = false,
  ): Either[String, SValue] = {
    for {
-      anyTpl <- fromContract(translator, contract, enableContractUpgrading)
-    } yield record(
-      StablePackagesV2.Tuple2,
-      ("_1", SContractId(contract.contractId)),
-      ("_2", anyTpl),
+      anyTpl <- fromAnyTemplate(
+        translator,
+        targetTemplateId,
+        contract.argument,
+        enableContractUpgrading,
+      )
+    } yield makeTuple(
+      SContractId(contract.contractId),
+      anyTpl,
    )
  }

--- a/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/v2/ScriptF.scala
+++ b/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/v2/ScriptF.scala
@ -6,42 +6,32 @@ package engine
 package script
 package v2

-import java.time.Clock
-import org.apache.pekko.stream.Materializer
 import com.daml.grpc.adapter.ExecutionSequencerFactory
-import com.digitalasset.canton.ledger.api.domain.{User, UserRight}
-import com.daml.lf.data.FrontStack
 import com.daml.lf.CompiledPackages
-import com.daml.lf.data.Ref.{
-  Identifier,
-  Location,
-  Name,
-  PackageId,
-  PackageName,
-  PackageVersion,
-  Party,
-  UserId,
-}
+import com.daml.lf.data.FrontStack
+import com.daml.lf.data.Ref._
 import com.daml.lf.data.Time.Timestamp
 import com.daml.lf.engine.preprocessing.ValueTranslator
 import com.daml.lf.engine.script.v2.ledgerinteraction.ScriptLedgerClient
+import com.daml.lf.interpretation.{Error => IE}
 import com.daml.lf.language.{Ast, LanguageVersion}
-import com.daml.lf.speedy.{ArrayList, SError, SValue}
-import com.daml.lf.speedy.SBuiltinFun.SBVariantCon
+import com.daml.lf.speedy.SBuiltinFun.{SBToAny, SBVariantCon}
 import com.daml.lf.speedy.SExpr._
 import com.daml.lf.speedy.SValue._
+import com.daml.lf.speedy.{ArrayList, SError, SValue}
 import com.daml.lf.stablepackages.StablePackagesV2
 import com.daml.lf.value.Value
 import com.daml.lf.value.Value.ContractId
-import scalaz.{Foldable, OneAnd}
-import scalaz.syntax.traverse._
+import com.daml.script.converter.Converter.{makeTuple, toContractId, toText}
+import com.digitalasset.canton.ledger.api.domain.{User, UserRight}
+import org.apache.pekko.stream.Materializer
 import scalaz.std.either._
 import scalaz.std.list._
 import scalaz.std.option._
-import com.daml.script.converter.Converter.{toContractId, toText}
-import com.daml.lf.interpretation.{Error => IE}
-import com.daml.lf.speedy.SBuiltinFun.SBToAny
+import scalaz.syntax.traverse._
+import scalaz.{Foldable, OneAnd}

+import java.time.Clock
 import scala.concurrent.{ExecutionContext, Future}
 import scala.util.{Failure, Success}

@ -234,7 +224,7 @@ object ScriptF {
                    StablePackagesV2.Either,
                    Name.assertFromString("Right"),
                    1,
-                    makePair(SList(rs), tree),
+                    makeTuple(SList(rs), tree),
                  )
                }
            )
@ -270,7 +260,7 @@ object ScriptF {
            .to(FrontStack)
            .traverse(
              Converter
-                .fromCreated(env.valueTranslator, _, client.enableContractUpgrading)
+                .fromCreated(env.valueTranslator, _, tplId, client.enableContractUpgrading)
            )
        )
      } yield SEValue(SList(res))
@ -292,18 +282,24 @@ object ScriptF {
        optR <- Converter.toFuture(
          optR.traverse(c =>
            Converter
-              .fromContract(env.valueTranslator, c, client.enableContractUpgrading)
-              .map(makePair(_, SText(c.blob.toHexString)))
+              .fromAnyTemplate(
+                env.valueTranslator,
+                tplId,
+                c.argument,
+                client.enableContractUpgrading,
+              )
+              .map(
+                makeTuple(
+                  _,
+                  Converter.fromTemplateTypeRep(c.templateId),
+                  SText(c.blob.toHexString),
+                )
+              )
          )
        )
      } yield SEValue(SOptional(optR))
  }

-  private[this] def makePair(v1: SValue, v2: SValue): SValue = {
-    import com.daml.script.converter.Converter.record
-    record(StablePackagesV2.Tuple2, ("_1", v1), ("_2", v2))
-  }
-
  final case class QueryInterface(
      parties: OneAnd[Set, Party],
      interfaceId: Identifier,
@ -324,7 +320,7 @@ object ScriptF {
            .traverse { case (cid, optView) =>
              optView match {
                case None =>
-                  Right(makePair(SContractId(cid), SOptional(None)))
+                  Right(makeTuple(SContractId(cid), SOptional(None)))
                case Some(view) =>
                  for {
                    view <- Converter.fromInterfaceView(
@ -333,7 +329,7 @@ object ScriptF {
                      view,
                    )
                  } yield {
-                    makePair(SContractId(cid), SOptional(Some(view)))
+                    makeTuple(SContractId(cid), SOptional(Some(view)))
                  }
              }
            }
@ -398,7 +394,7 @@ object ScriptF {
        )
        optR <- Converter.toFuture(
          optR.traverse(
-            Converter.fromCreated(env.valueTranslator, _, client.enableContractUpgrading)
+            Converter.fromCreated(env.valueTranslator, _, tplId, client.enableContractUpgrading)
          )
        )
      } yield SEValue(SOptional(optR))
@ -776,7 +772,7 @@ object ScriptF {
        case Failure(
              Script.FailedCmd(cmdName, _, err)
            ) =>
-          import com.daml.lf.scenario.{Pretty, Error}
+          import com.daml.lf.scenario.{Error, Pretty}
          val msg = err match {
            case e: Error => Pretty.prettyError(e).render(10000)
            case e => e.getMessage
--- a/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/v2/ledgerinteraction/grpcLedgerClient/GrpcLedgerClient.scala
+++ b/sdk/daml-script/runner/src/main/scala/com/daml/lf/engine/script/v2/ledgerinteraction/grpcLedgerClient/GrpcLedgerClient.scala
@ -157,7 +157,23 @@ class GrpcLedgerClient(
            )
        val blob =
          Bytes.fromByteString(createdEvent.createdEventBlob)
-        (ScriptLedgerClient.ActiveContract(templateId, cid, argument, blob), key)
+        val disclosureTemplateId =
+          Converter
+            .fromApiIdentifier(
+              createdEvent.templateId.getOrElse(
+                throw new ConverterException("missing required template_id in CreatedEvent")
+              )
+            )
+            .getOrElse(throw new ConverterException("invalid template_id in CreatedEvent"))
+        (
+          ScriptLedgerClient.ActiveContract(
+            disclosureTemplateId,
+            cid,
+            argument,
+            blob,
+          ),
+          key,
+        )
      })
    )
  }
--- a/sdk/daml-script/test/daml/upgrades/QueryDisclosure.daml
+++ b/sdk/daml-script/test/daml/upgrades/QueryDisclosure.daml
@ -0,0 +1,51 @@
+-- Copyright (c) 2024 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+-- SPDX-License-Identifier: Apache-2.0
+
+module QueryDisclosure (main) where
+
+import UpgradeTestLib
+import qualified V1.QueryDisclosure as V1
+import qualified V2.QueryDisclosure as V2
+import DA.Optional (fromSome)
+
+{- PACKAGE
+name: query-disclosure
+versions: 2
+-}
+
+{- MODULE
+package: query-disclosure
+contents: |
+  module QueryDisclosure where
+
+  template QueryDisclosureTemplate
+    with
+      party: Party
+      newField : Optional Text -- @V 2
+    where
+      signatory party
+
+      choice QueryDisclosureChoice : ()
+        with
+          ctl: Party
+        where
+          controller ctl
+          do pure ()
+-}
+
+main : Script ()
+main = tests
+  [ ( "Disclosure retrieved with an upgraded template ID are valid disclosures"
+    , queriedDisclosuresAreValid
+    )
+  ]
+
+queriedDisclosuresAreValid : Script ()
+queriedDisclosuresAreValid = do
+  a <- allocatePartyOn "alice" participant0
+  b <- allocatePartyOn "bob" participant0
+
+  cid <- a `submit` createExactCmd V1.QueryDisclosureTemplate with party = a
+  let v2Cid = coerceContractId @V1.QueryDisclosureTemplate @V2.QueryDisclosureTemplate cid
+  disclosure <- fromSome <$> queryDisclosure a v2Cid
+  submitWithDisclosures b [disclosure] $ exerciseCmd v2Cid (V2.QueryDisclosureChoice b)