Scenario service error reporting fix (#16503)

* Return error through grpc with print in unexpected cases Fix a couple error cases * Fix spacing test * Refactor HashingError to have explicit case
2024-09-20 01:07:18 +03:00 · 2023-03-16 14:42:39 +00:00 · 2023-03-16 14:42:39 +00:00 · ddc27715ab
commit ddc27715ab
parent 01952a1fe7
9 changed files with 52 additions and 42 deletions
--- a/compiler/damlc/tests/daml-test-files/AllocatePartyError.daml
+++ b/compiler/damlc/tests/daml-test-files/AllocatePartyError.daml
@ -1,10 +1,7 @@
 -- Copyright (c) 2020, Digital Asset (Switzerland) GmbH and/or its affiliates.
 -- All rights reserved.

-- @TODO Scenario service consumes this error: Invalid party name: #party
-- @ERROR range=13:1-13:5; Scenario service backend error
-
-- @ERROR range=19:1-19:9; Invalid party name: #party
+-- @ERROR range=10:1-10:5; Invalid party name: #party

 module AllocatePartyError where

@ -13,9 +10,3 @@ import Daml.Script
 main = script do
    _ <- allocateParty "#party"
    pure ()
-
-- @ENABLE-SCENARIOS
-
-mainScen = scenario do
-    _ <- getParty "#party"
-    pure ()
--- a/compiler/damlc/tests/daml-test-files/ContractIdInContractKeySkipCheck.daml
+++ b/compiler/damlc/tests/daml-test-files/ContractIdInContractKeySkipCheck.daml
@ -5,14 +5,13 @@
 -- defining the type that contains a contract id in a separate module, the
 -- runtime check is still performed.

-- @ERROR range=40:1-40:17; Contract IDs are not supported in contract key
-- @TODO Below should be `Contract IDs are not supported in contract key` however this error isn't networked over from the ledger
-- @ERROR range=49:1-49:13; Scenario service backend error
-- @ERROR range=55:1-55:19; Contract IDs are not supported in contract key
-- @ERROR range=94:1-94:14; Contract IDs are not supported in contract key
-- @ERROR range=98:1-98:13; Contract IDs are not supported in contract key
-- @ERROR range=102:1-102:14; Contract IDs are not supported in contract key
-- @ERROR range=106:1-106:16; Contract IDs are not supported in contract key
+-- @ERROR range=39:1-39:17; Contract IDs are not supported in contract key
+-- @ERROR range=48:1-48:13; Contract IDs are not supported in contract key
+-- @ERROR range=54:1-54:19; Contract IDs are not supported in contract key
+-- @ERROR range=93:1-93:14; Contract IDs are not supported in contract key
+-- @ERROR range=97:1-97:13; Contract IDs are not supported in contract key
+-- @ERROR range=101:1-101:14; Contract IDs are not supported in contract key
+-- @ERROR range=105:1-105:16; Contract IDs are not supported in contract key

 module ContractIdInContractKeySkipCheck where

--- a/compiler/damlc/tests/src/DA/Test/ScriptService.hs
+++ b/compiler/damlc/tests/src/DA/Test/ScriptService.hs
@ -578,7 +578,7 @@ main =
                expectScriptSuccess rs (vr "partyManagement") $ \r ->
                  matchRegex r "Active contracts:  #0:0\n\nReturn value: {}\n\n$"
                expectScriptFailure rs (vr "duplicateAllocateWithHint") $ \r ->
-                  matchRegex r "Tried to allocate a party that already exists:  alice"
+                  matchRegex r "Tried to allocate a party that already exists: alice"
                expectScriptSuccess rs (vr "partyWithEmptyDisplayName") $ \r ->
                  matchRegex r "Active contracts:  #0:0\n\nReturn value: {}\n\n$"
            , testCase "queryContractId/Key" $ do
--- a/compiler/scenario-service/client/src/DA/Daml/LF/PrettyScenario.hs
+++ b/compiler/scenario-service/client/src/DA/Daml/LF/PrettyScenario.hs
@ -381,9 +381,9 @@ prettyScenarioErrorError (Just err) =  do
    ScenarioErrorErrorScenarioMustfailSucceeded _ ->
      pure "A must-fail commit succeeded."
    ScenarioErrorErrorScenarioInvalidPartyName name ->
-      pure $ "Invalid party name: " <-> ltext name
+      pure $ "Invalid party name:" <-> ltext name
    ScenarioErrorErrorScenarioPartyAlreadyExists name ->
-      pure $ "Tried to allocate a party that already exists: " <-> ltext name
+      pure $ "Tried to allocate a party that already exists:" <-> ltext name

    ScenarioErrorErrorScenarioContractNotVisible ScenarioError_ContractNotVisible{..} ->
      pure $ vcat
@ -436,7 +436,7 @@ prettyScenarioErrorError (Just err) =  do
            $ prettyMay "<missing template id>" (prettyDefName world) scenarioError_WronglyTypedContractExpected
        ]
    ScenarioErrorErrorContractIdInContractKey ScenarioError_ContractIdInContractKey{..} ->
-      pure $ "Contract IDs are not supported in contract key: " <->
+      pure $ "Contract IDs are not supported in contract key:" <->
        prettyMay "<missing contract key>"
          (prettyValue' False 0 world)
          scenarioError_ContractIdInContractKeyKey
--- a/compiler/scenario-service/server/src/main/scala/com/digitalasset/daml/lf/scenario/Context.scala
+++ b/compiler/scenario-service/server/src/main/scala/com/digitalasset/daml/lf/scenario/Context.scala
@ -233,9 +233,16 @@ class Context(
        e.cause match {
          case e: Error => handleFailure(e)
          case e: speedy.SError.SError => handleFailure(Error.RunnerException(e))
-          case _ => Failure(e)
+          case e => {
+            // We can't send _everything_ over without changing internal, nicer to put a print here.
+            e.printStackTrace
+            handleFailure(Error.Internal("ScriptF.FailedCmd unexpected cause: " + e.getMessage))
+          }
        }
-      case Failure(e) => Failure(e)
+      case Failure(e) => {
+        e.printStackTrace
+        handleFailure(Error.Internal("Unexpected error type from script runner: " + e.getMessage))
+      }
    }
  }
 }
--- a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/crypto/Hash.scala
+++ b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/crypto/Hash.scala
@ -36,16 +36,17 @@ object Hash {
  val version = 0.toByte
  val underlyingHashLength = 32

-  private case class HashingError(msg: String) extends Exception with NoStackTrace
+  sealed abstract class HashingError(val msg: String) extends Exception with NoStackTrace
+  object HashingError {
+    final case class ForbiddenContractId()
+        extends HashingError("Contract IDs are not supported in contract keys.")
+  }

-  private def error(msg: String): Nothing =
-    throw HashingError(msg)
-
-  private def handleError[X](x: => X): Either[String, X] =
+  private def handleError[X](x: => X): Either[HashingError, X] =
    try {
      Right(x)
    } catch {
-      case HashingError(msg) => Left(msg)
+      case e: HashingError => Left(e)
    }

  def fromBytes(bs: Bytes): Either[String, Hash] =
@ -82,7 +83,7 @@ object Hash {
  }

  private[lf] val noCid2String: Value.ContractId => Nothing =
-    _ => error("Contract IDs are not supported in contract keys.")
+    _ => throw HashingError.ForbiddenContractId()

  private[crypto] sealed abstract class Builder(cid2Bytes: Value.ContractId => Bytes) {

@ -307,7 +308,7 @@ object Hash {
  def hashContractKey(
      templateId: Ref.Identifier,
      key: Value,
-  ): Either[String, Hash] =
+  ): Either[HashingError, Hash] =
    handleError(assertHashContractKey(templateId, key))

  // This function assumes that `arg` is well typed, i.e. :
@ -324,7 +325,7 @@ object Hash {
  def hashContractInstance(
      templateId: Ref.Identifier,
      arg: Value,
-  ): Either[String, Hash] =
+  ): Either[HashingError, Hash] =
    handleError(assertHashContractInstance(templateId, arg))

  def hashChangeId(
--- a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/GlobalKey.scala
+++ b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/GlobalKey.scala
@ -28,13 +28,13 @@ final class GlobalKey private (
 object GlobalKey {

  // Will fail if key contains contract ids
-  def build(templateId: Ref.TypeConName, key: Value): Either[String, GlobalKey] =
+  def build(templateId: Ref.TypeConName, key: Value): Either[crypto.Hash.HashingError, GlobalKey] =
    crypto.Hash.hashContractKey(templateId, key).map(new GlobalKey(templateId, key, _))

  // Like `build` but,  in case of error, throws an exception instead of returning a message.
  @throws[IllegalArgumentException]
  def assertBuild(templateId: Ref.TypeConName, key: Value): GlobalKey =
-    data.assertRight(build(templateId, key))
+    data.assertRight(build(templateId, key).left.map(_.msg))
 }

 final case class GlobalKeyWithMaintainers(
--- a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/TransactionCoder.scala
+++ b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/TransactionCoder.scala
@ -414,7 +414,7 @@ object TransactionCoder {
        keyWithMaintainers.getKeyVersioned,
        keyWithMaintainers.getKeyUnversioned,
      )
-      gkey <- GlobalKey.build(templateId, value).left.map(DecodeError)
+      gkey <- GlobalKey.build(templateId, value).left.map(hashErr => DecodeError(hashErr.msg))
    } yield GlobalKeyWithMaintainers(gkey, maintainers)
  }

@ -907,7 +907,7 @@ object TransactionCoder {
    for {
      tmplId <- ValueCoder.decodeIdentifier(rawTmplId)
      value <- ValueCoder.decodeValue(ValueCoder.NoCidDecoder, nodeVersion, rawKey)
-      key <- GlobalKey.build(tmplId, value).left.map(DecodeError)
+      key <- GlobalKey.build(tmplId, value).left.map(hashErr => DecodeError(hashErr.msg))
    } yield key

  /*
--- a/daml-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/ledgerinteraction/IdeLedgerClient.scala
+++ b/daml-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/ledgerinteraction/IdeLedgerClient.scala
@ -12,11 +12,12 @@ import com.daml.ledger.api.domain.{IdentityProviderId, ObjectMeta, PartyDetails,
 import com.daml.lf.data.Ref._
 import com.daml.lf.data.{ImmArray, Ref, Time}
 import com.daml.lf.engine.preprocessing.ValueTranslator
+import com.daml.lf.interpretation.Error.ContractIdInContractKey
 import com.daml.lf.language.Ast
 import com.daml.lf.language.Ast.TTyCon
 import com.daml.lf.scenario.{ScenarioLedger, ScenarioRunner}
 import com.daml.lf.speedy.Speedy.Machine
-import com.daml.lf.speedy.{SValue, TraceLog, WarningLog}
+import com.daml.lf.speedy.{SValue, TraceLog, WarningLog, SError}
 import com.daml.lf.transaction.{
  GlobalKey,
  IncompleteTransaction,
@ -29,7 +30,6 @@ import com.daml.lf.value.Value
 import com.daml.lf.value.Value.ContractId
 import com.daml.logging.LoggingContext
 import com.daml.platform.localstore.InMemoryUserManagementStore
-import com.daml.script.converter.ConverterException
 import io.grpc.StatusRuntimeException
 import scalaz.OneAnd
 import scalaz.OneAnd._
@ -226,9 +226,17 @@ class IdeLedgerClient(
      ec: ExecutionContext,
      mat: Materializer,
  ): Future[Option[ScriptLedgerClient.ActiveContract]] = {
+    val keyValue = key.toUnnormalizedValue
+    def keyBuilderError(err: crypto.Hash.HashingError): Future[GlobalKey] =
+      Future.failed(
+        err match {
+          case crypto.Hash.HashingError.ForbiddenContractId() =>
+            SError.SErrorDamlException(ContractIdInContractKey(keyValue))
+        }
+      )
    GlobalKey
-      .build(templateId, key.toUnnormalizedValue)
-      .fold(err => Future.failed(new ConverterException(err)), Future.successful(_))
+      .build(templateId, keyValue)
+      .fold(keyBuilderError(_), Future.successful(_))
      .flatMap { gkey =>
        ledger.ledgerData.activeKeys.get(gkey) match {
          case None => Future.successful(None)
@ -427,9 +435,13 @@ class IdeLedgerClient(
          val candidates = namePrefix #:: LazyList.from(1).map(namePrefix + _.toString())
          Success(candidates.find(s => !(usedNames contains s)).get)
        }
+      party <- Ref.Party
+        .fromString(name)
+        .fold(msg => Failure(scenario.Error.InvalidPartyName(name, msg)), Success(_))
+
      // Create and store the new party.
      partyDetails = PartyDetails(
-        party = Ref.Party.assertFromString(name),
+        party = party,
        displayName = Some(displayName),
        isLocal = true,
        metadata = ObjectMeta.empty,