Speedy: Refactor contract Id/Key callback (#10033)

CHANGELOG_BEGIN
CHANGELOG_END

compiler/damlc/tests/daml-test-files/ContractKeyNotEffective.daml

@@ -0,0 +1,18 @@
+-- @ERROR range=14:1-14:19; contract 0055b33c9fd9928bdf0c2c611194865917b9aaf8b4b94a44abc25ddfea6d866cd1 not effective, but we found its key!
+module ContractKeyNotEffective where
+
+import DA.Time
+
+template T
+  with
+    p : Party
+  where
+    signatory p
+    key p : Party
+    maintainer key
+
+fetchByKeyMustFail = scenario do
+  p <- getParty "alice"
+  submit p $ create (T p)
+  pass (- seconds 1)
+  submit p $ fetchByKey @T p

daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/Engine.scala

@@ -326,11 +326,11 @@ class Engine(val config: EngineConfig = new EngineConfig(LanguageVersion.StableV
             },
           )
 
-        case SResultNeedContract(contractId, _, _, _, cbPresent) =>
+        case SResultNeedContract(contractId, _, _, callback) =>
           return Result.needContract(
             contractId,
             { coinst =>
-              cbPresent(coinst)
+              callback(coinst)
               interpretLoop(machine, time)
             },
           )
@@ -342,17 +342,16 @@ class Engine(val config: EngineConfig = new EngineConfig(LanguageVersion.StableV
         case SResultNeedKey(gk, _, cb) =>
           return ResultNeedKey(
             gk,
-            result =>
-              if (cb(result))
-                interpretLoop(machine, time)
-              else
-                ResultError(Error.Interpretation.ContractKeyNotFound(gk.globalKey)),
+            { result =>
+              cb(result)
+              interpretLoop(machine, time)
+            },
           )
 
         case SResultNeedLocalKeyVisible(stakeholders, _, cb) =>
           return ResultNeedLocalKeyVisible(
             stakeholders,
-            result => {
+            { result =>
               cb(result.toSVisibleByKey)
               interpretLoop(machine, time)
             },

daml-lf/interpreter/BUILD.bazel

@@ -34,6 +34,7 @@ da_scala_library(
         "//daml-lf/language",
         "//daml-lf/transaction",
         "//daml-lf/validation",
+        "//libs-scala/scala-utils",
         "@maven//:com_google_protobuf_protobuf_java",
         "@maven//:org_slf4j_slf4j_api",
     ],

daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SBuiltin.scala

@@ -20,6 +20,7 @@ import com.daml.lf.speedy.SValue.{SValue => SV}
 import com.daml.lf.transaction.{Transaction => Tx}
 import com.daml.lf.value.{Value => V}
 import com.daml.lf.transaction.{GlobalKey, GlobalKeyWithMaintainers, Node}
+import com.daml.scalautil.Statement.discard
 
 import scala.jdk.CollectionConverters._
 import scala.collection.immutable.TreeSet
@@ -1023,11 +1024,7 @@ private[lf] object SBuiltin {
               coid,
               templateId,
               onLedger.committers,
-              cbMissing = _ => false,
-              cbPresent = { case V.ContractInst(actualTmplId, V.VersionedValue(_, arg), _) =>
-                // Note that we cannot throw in this continuation -- instead
-                // set the control appropriately which will crash the machine
-                // correctly later.
+              { case V.ContractInst(actualTmplId, V.VersionedValue(_, arg), _) =>
                 if (actualTmplId != templateId) {
                   machine.ctrl =
                     SEDamlException(DamlEWronglyTypedContract(coid, templateId, actualTmplId))
@@ -1139,13 +1136,14 @@ private[lf] object SBuiltin {
     private[this] val typ = AstUtil.TContractId(Ast.TTyCon(templateId))
 
     final protected def importCid(cid: V.ContractId): SEImportValue =
+      // We have to check that the discriminator of cid does not conflict with a local ones
+      // however we cannot raise an exception in case of failure here.
+      // We delegate to CtrlImportValue the task to check cid.
       SEImportValue(typ, V.ValueContractId(cid))
     // Callback from the engine returned NotFound
-    def handleInputKeyNotFound(machine: Machine): Boolean
-    // CallBack from the engine returned a new Cid
     def handleInputKeyFound(machine: Machine, cid: V.ContractId): Unit
     // We already saw this key, but it was undefined or was archived
-    def handleInactiveKey(machine: Machine, gkey: GlobalKey): Unit
+    def handleKeyNotFound(machine: Machine, gkey: GlobalKey): Boolean
     // We already saw this key and it is still active
     def handleActiveKey(machine: Machine, cid: V.ContractId): Unit
 
@@ -1156,30 +1154,30 @@ private[lf] object SBuiltin {
     ): Unit =
       keyMapping match {
         case PartialTransaction.KeyActive(cid) => handleActiveKey(machine, cid)
-        case PartialTransaction.KeyInactive => handleInactiveKey(machine, gkey)
+        case PartialTransaction.KeyInactive => discard(handleKeyNotFound(machine, gkey))
       }
   }
 
   private[this] object KeyOperation {
     final class Fetch(override val templateId: TypeConName) extends KeyOperation {
-      override def handleInputKeyNotFound(machine: Machine): Boolean = false
       override def handleInputKeyFound(machine: Machine, cid: V.ContractId): Unit =
         machine.ctrl = importCid(cid)
-      override def handleInactiveKey(machine: Machine, gkey: GlobalKey): Unit =
+      override def handleKeyNotFound(machine: Machine, gkey: GlobalKey): Boolean = {
         machine.ctrl = SEDamlException(DamlEContractKeyNotFound(gkey))
+        false
+      }
+
       override def handleActiveKey(machine: Machine, cid: V.ContractId): Unit =
         machine.returnValue = SContractId(cid)
     }
 
     final class Lookup(override val templateId: TypeConName) extends KeyOperation {
-      override def handleInputKeyNotFound(machine: Machine): Boolean = {
+      override def handleInputKeyFound(machine: Machine, cid: V.ContractId): Unit =
+        machine.ctrl = SBSome(importCid(cid))
+      override def handleKeyNotFound(machine: Machine, key: GlobalKey): Boolean = {
         machine.returnValue = SValue.SValue.None
         true
       }
-      override def handleInputKeyFound(machine: Machine, cid: V.ContractId): Unit =
-        machine.ctrl = SBSome(importCid(cid))
-      override def handleInactiveKey(machine: Machine, key: GlobalKey): Unit =
-        machine.returnValue = SValue.SValue.None
       override def handleActiveKey(machine: Machine, cid: V.ContractId): Unit =
         machine.returnValue = SOptional(Some(SContractId(cid)))
     }
@@ -1200,7 +1198,11 @@ private[lf] object SBuiltin {
       )
     }
 
-    final def execute(args: util.ArrayList[SValue], machine: Machine, onLedger: OnLedger): Unit = {
+    final override def execute(
+        args: util.ArrayList[SValue],
+        machine: Machine,
+        onLedger: OnLedger,
+    ): Unit = {
       import PartialTransaction.{KeyActive, KeyInactive}
       val keyWithMaintainers = extractKeyWithMaintainers(args.get(0))
       if (keyWithMaintainers.maintainers.isEmpty)
@@ -1244,30 +1246,18 @@ private[lf] object SBuiltin {
                   onLedger.committers,
                   { result =>
                     cacheGlobalLookup(onLedger, gkey, result)
-                    // We need to check if the contract was consumed since we only
-                    // modify keys if the archive was for a key
-                    // already brought into scope.
-                    val activeResult = result match {
-                      case Some(cid) if onLedger.ptx.consumedBy.contains(cid) =>
-                        None
-                      case _ =>
-                        result
-                    }
-                    activeResult match {
-                      case Some(cid) =>
+                    result match {
+                      case Some(cid) if !onLedger.ptx.consumedBy.contains(cid) =>
                         onLedger.ptx = onLedger.ptx.copy(
                           keys = onLedger.ptx.keys.updated(gkey, KeyActive(cid))
                         )
-                        // We have to check that the discriminator of cid does not conflict with a local ones
-                        // however we cannot raise an exception in case of failure here.
-                        // We delegate to CtrlImportValue the task to check cid.
                         operation.handleInputKeyFound(machine, cid)
                         true
-                      case None =>
+                      case _ =>
                         onLedger.ptx = onLedger.ptx.copy(
                           keys = onLedger.ptx.keys.updated(gkey, KeyInactive)
                         )
-                        operation.handleInputKeyNotFound(machine)
+                        operation.handleKeyNotFound(machine, gkey)
                     }
                   },
                 )

daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SResult.scala

@@ -33,11 +33,10 @@ object SResult {
       contractId: ContractId,
       templateId: TypeConName,
       committers: Set[Party],
-      // Callback to signal that the contract was not present
-      // or visible. Returns true if this was recoverable.
-      // TODO (MK) Drop now that tryHandleSubmitMustFail is dead.
-      cbMissing: Unit => Boolean,
-      cbPresent: ContractInst[Value.VersionedValue[ContractId]] => Unit,
+      // Callback
+      // returns the next expression to evaluate.
+      // In case of failure the call back does not throw but returns a SErrorDamlException
+      callback: ContractInst[Value.VersionedValue[ContractId]] => Unit,
   ) extends SResult
 
   /** Machine needs a definition that was not present when the machine was
@@ -73,14 +72,17 @@ object SResult {
       key: GlobalKeyWithMaintainers,
       committers: Set[Party],
       // Callback.
-      // returns true if machine can continue with the given result.
-      cb: Option[ContractId] => Boolean,
+      // In case of failure, the callback sets machine.ctrl to an SErrorDamlException and return false
+      callback: Option[ContractId] => Boolean,
   ) extends SResult
 
   final case class SResultNeedLocalKeyVisible(
       stakeholders: Set[Party],
       committers: Set[Party],
-      cb: SVisibleByKey => Unit,
+      // Callback.
+      // returns the next expression to evaluate.
+      // In case of failure the call back does not throw but set machine.ctrl to an SErrorDamlException
+      callback: SVisibleByKey => Unit,
   ) extends SResult
 
   sealed abstract class SVisibleByKey

daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Speedy.scala

@@ -1372,7 +1372,9 @@ private[lf] object Speedy {
     */
   private[speedy] final case class SpeedyHungry(result: SResult)
       extends RuntimeException
-      with NoStackTrace
+      with NoStackTrace {
+    override def toString = s"SpeedyHungry($result)"
+  }
 
   private[speedy] def deriveTransactionSeed(
       submissionSeed: crypto.Hash,

daml-lf/scenario-interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/ScenarioRunner.scala

@@ -219,14 +219,15 @@ object ScenarioRunner {
 
   // The interface we need from a ledger during submission. We allow abstracting over this so we can play
   // tricks like caching all responses in some benchmarks.
-  trait LedgerApi[R] {
+  abstract class LedgerApi[R] {
     def lookupContract(
         coid: ContractId,
         actAs: Set[Party],
         readAs: Set[Party],
         cbPresent: ContractInst[Value.VersionedValue[ContractId]] => Unit,
-    ): Either[SError, ContractInst[Value.VersionedValue[ContractId]]]
+    ): Either[SError, Unit]
     def lookupKey(
+        machine: Speedy.Machine,
         gk: GlobalKey,
         actAs: Set[Party],
         readAs: Set[Party],
@@ -243,20 +244,21 @@ object ScenarioRunner {
 
   case class ScenarioLedgerApi(ledger: ScenarioLedger)
       extends LedgerApi[ScenarioLedger.CommitResult] {
+
     override def lookupContract(
         acoid: ContractId,
         actAs: Set[Party],
         readAs: Set[Party],
-        cbPresent: ContractInst[Value.VersionedValue[ContractId]] => Unit,
-    ): Either[SError, ContractInst[Value.VersionedValue[ContractId]]] =
-      handleUnsafe(lookupContractUnsafe(acoid, actAs, readAs, cbPresent))
+        callback: ContractInst[Value.VersionedValue[ContractId]] => Unit,
+    ): Either[SError, Unit] =
+      handleUnsafe(lookupContractUnsafe(acoid, actAs, readAs, callback))
 
     private def lookupContractUnsafe(
         acoid: ContractId,
         actAs: Set[Party],
         readAs: Set[Party],
-        cbPresent: ContractInst[Value.VersionedValue[ContractId]] => Unit,
-    ): ContractInst[Value.VersionedValue[ContractId]] = {
+        callback: ContractInst[Value.VersionedValue[ContractId]] => Unit,
+    ) = {
 
       val effectiveAt = ledger.currentTime
 
@@ -268,8 +270,7 @@ object ScenarioRunner {
         acoid,
       ) match {
         case ScenarioLedger.LookupOk(_, coinst, _) =>
-          cbPresent(coinst)
-          coinst
+          callback(coinst)
 
         case ScenarioLedger.LookupContractNotFound(coid) =>
           // This should never happen, hence we don't have a specific
@@ -286,26 +287,33 @@ object ScenarioRunner {
           missingWith(ScenarioErrorContractNotVisible(coid, tid, actAs, readAs, observers))
       }
     }
+
     override def lookupKey(
+        machine: Speedy.Machine,
         gk: GlobalKey,
         actAs: Set[Party],
         readAs: Set[Party],
-        canContinue: Option[ContractId] => Boolean,
+        callback: Option[ContractId] => Boolean,
     ): Either[SError, Unit] =
-      handleUnsafe(lookupKeyUnsafe(gk, actAs, readAs, canContinue))
+      handleUnsafe(lookupKeyUnsafe(machine: Speedy.Machine, gk, actAs, readAs, callback))
 
     private def lookupKeyUnsafe(
+        machine: Speedy.Machine,
         gk: GlobalKey,
         actAs: Set[Party],
         readAs: Set[Party],
-        canContinue: Option[ContractId] => Boolean,
+        callback: Option[ContractId] => Boolean,
     ): Unit = {
+
       val effectiveAt = ledger.currentTime
       val readers = actAs union readAs
 
       def missingWith(err: SError) =
-        if (!canContinue(None))
+        if (!callback(None)) {
+          machine.returnValue = null
+          machine.ctrl = null
           throw SRunnerException(err)
+        }
 
       ledger.ledgerData.activeKeys.get(gk) match {
         case None =>
@@ -318,19 +326,21 @@ object ScenarioRunner {
           ) match {
             case ScenarioLedger.LookupOk(_, _, stakeholders) =>
               if (!readers.intersect(stakeholders).isEmpty)
-                // We should always be able to continue with a Some(_).
+                // We should always be able to continue with a SKeyLookupResult.Found.
                 // Run to get side effects and assert result.
-                assert(canContinue(Some(acoid)))
+                assert(callback(Some(acoid)))
               else
                 throw SRunnerException(
                   ScenarioErrorContractKeyNotVisible(acoid, gk, actAs, readAs, stakeholders)
                 )
             case ScenarioLedger.LookupContractNotFound(coid) =>
-              missingWith(SErrorCrash(s"contract $coid not found, but we found its key!"))
+              missingWith(SErrorCrash(s"contract ${coid.coid} not found, but we found its key!"))
             case ScenarioLedger.LookupContractNotEffective(_, _, _) =>
-              missingWith(SErrorCrash(s"contract $acoid not effective, but we found its key!"))
+              missingWith(
+                SErrorCrash(s"contract ${acoid.coid} not effective, but we found its key!")
+              )
             case ScenarioLedger.LookupContractNotActive(_, _, _) =>
-              missingWith(SErrorCrash(s"contract $acoid not active, but we found its key!"))
+              missingWith(SErrorCrash(s"contract ${acoid.coid} not active, but we found its key!"))
             case ScenarioLedger.LookupContractNotVisible(
                   coid,
                   tid @ _,
@@ -405,13 +415,19 @@ object ScenarioRunner {
           }
         case SResultError(err) =>
           SubmissionError(err, onLedger.ptxInternal)
-        case SResultNeedContract(coid, tid @ _, committers, _, cbPresent) =>
-          ledger.lookupContract(coid, committers, readAs, cbPresent) match {
+        case SResultNeedContract(coid, tid @ _, committers, callback) =>
+          ledger.lookupContract(coid, committers, readAs, callback) match {
             case Left(err) => SubmissionError(err, onLedger.ptxInternal)
             case Right(_) => go()
           }
-        case SResultNeedKey(keyWithMaintainers, committers, cb) =>
-          ledger.lookupKey(keyWithMaintainers.globalKey, committers, readAs, cb) match {
+        case SResultNeedKey(keyWithMaintainers, committers, callback) =>
+          ledger.lookupKey(
+            ledgerMachine,
+            keyWithMaintainers.globalKey,
+            committers,
+            readAs,
+            callback,
+          ) match {
             case Left(err) => SubmissionError(err, onLedger.ptxInternal)
             case Right(_) => go()
           }

daml-lf/scenario-interpreter/src/perf/benches/scala/com/digitalasset/daml/lf/speedy/perf/CollectAuthority.scala

@@ -96,7 +96,7 @@ class CollectAuthorityState {
               callback(value)
             case ScenarioRunner.SubmissionError(err, _) => crash(s"Submission failed $err")
           }
-        case SResultNeedContract(_, _, _, _, _) =>
+        case SResultNeedContract(_, _, _, _) =>
           crash("Off-ledger need contract callback")
         case SResultFinalValue(v) => finalValue = v
         case r => crash(s"bench run: unexpected result from speedy: ${r}")
@@ -165,13 +165,15 @@ class CachedLedgerApi(initStep: Int, ledger: ScenarioLedger)
       coid: ContractId,
       actAs: Set[Party],
       readAs: Set[Party],
-      cbPresent: ContractInst[Value.VersionedValue[ContractId]] => Unit,
-  ): Either[SError.SError, ContractInst[Value.VersionedValue[ContractId]]] = {
+      callback: ContractInst[Value.VersionedValue[ContractId]] => Unit,
+  ): Either[SError.SError, Unit] = {
     step += 1
-    super.lookupContract(coid, actAs, readAs, cbPresent).map { result =>
-      cachedContract += step -> result
-      result
-    }
+    super.lookupContract(
+      coid,
+      actAs,
+      readAs,
+      { coinst => cachedContract += step -> coinst; callback(coinst) },
+    )
   }
 }
 
@@ -184,18 +186,18 @@ class CannedLedgerApi(
       coid: ContractId,
       actAs: Set[Party],
       readAs: Set[Party],
-      cbPresent: ContractInst[Value.VersionedValue[ContractId]] => Unit,
-  ): Either[SError.SError, ContractInst[Value.VersionedValue[ContractId]]] = {
+      callback: ContractInst[Value.VersionedValue[ContractId]] => Unit,
+  ): Either[SError.SError, Unit] = {
     step += 1
     val coinst = cachedContract(step)
-    cbPresent(coinst)
-    Right(coinst)
+    Right(callback(coinst))
   }
   override def lookupKey(
+      machine: Machine,
       gk: GlobalKey,
       actAs: Set[Party],
       readAs: Set[Party],
-      canContinue: Option[ContractId] => Boolean,
+      callback: Option[ContractId] => Boolean,
   ) =
     throw new RuntimeException("Keys are not supported in the benchmark")
   override def currentTime = throw new RuntimeException("getTime is not supported in the benchmark")