Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ValueEnricher in ScenarioRunner #10897

Merged
merged 1 commit into from
Sep 22, 2021
Merged

Use ValueEnricher in ScenarioRunner #10897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Use ValueEnricher in ScenarioRunner. 
1st attempt. Causes package recompilation (bad!).

CHANGELOG_BEGIN
CHANGELOG_END

fix build

Change ValueEnricher interface to work without passing an Engine

ValueEnricher has optional preprocessor

simplify new interface to ValueEnricher: caller passes translateValue function
  • Loading branch information
@nickchapman-da
nickchapman-da committed Sep 22, 2021
commit 41938f83258d81c55debb2a54376ebca57fd51c6
28 changes: 21 additions & 7 deletions daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/ValueEnricher.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,35 @@
package com.daml.lf
package engine

import com.daml.lf.data.Ref.{Identifier, Name}
import com.daml.lf.data.Ref.{Identifier, Name, PackageId}
import com.daml.lf.language.{Ast, LookupError}
import com.daml.lf.transaction.Node.{GenNode, KeyWithMaintainers}
import com.daml.lf.transaction.{CommittedTransaction, Node, NodeId, VersionedTransaction}
import com.daml.lf.value.Value
import com.daml.lf.value.Value.VersionedValue
import com.daml.lf.speedy.SValue

// Provide methods to add missing information in values (and value containers):
// - type constructor in records, variants, and enums
// - Records' field names
final class ValueEnricher(engine: Engine) {

def enrichValue(typ: Ast.Type, value: Value): Result[Value] =
engine.preprocessor.translateValue(typ, value).map(_.toUnnormalizedValue)
final class ValueEnricher(
compiledPackages: CompiledPackages,
translateValue: (Ast.Type, Value) => Result[SValue],
loadPackage: (PackageId, language.Reference) => Result[Unit],
) {

def this(engine: Engine) = {
this(
engine.compiledPackages(),
engine.preprocessor.translateValue,
engine.loadPackage,
)
}
Comment on lines +25 to +31
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not really like this java-like overriding of class constructor.
We are not using it at all in the daml-lf module.
I think this is there in scala for compatibility reason with java, but it is really not very idiomatic.

I would prefer to have an apply method in the companion object.

object ValueEnricher{

  def apply(engine: Engine): ValueEnricher =
    new ValueEnricher(
      engine.compiledPackages(),
      engine.preprocessor.translateValue,
      engine.loadPackage,
    )
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment Remy.

Yeah. I am also not a big fan of these secondary this constructors. The syntax is odd and rather constrained. See below.

So I had already tried your suggestion of the apply method in a companion object. The problem is that all callers must be updated from new ValueEnricher(engine) to ValueEnricher(engine) -- which is fine -- but without the new at the call site, it seems that ValueEnricher looks more like a case class, and obscures the fact that we are dealing with a mutable object.

But my opinions are weakly held here... If we think it is acceptable/better without the new I can make the change.

(
Aside: I found the this syntax is very limited. The following is not allowed syntactically:

  def this(engine: Engine) = {
    val compiledPackages = engine.compiledPackages()
    this(
      compiledPackages,
      engine.preprocessor.translateValue,
      engine.loadPackage,
    )
  }

-->

daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/ValueEnricher.scala:26: error: 'this' expected but 'val' found.
    val compiledPackages = engine.compiledPackages()
    ^

er, why? 🤷 ... oh of course, scala 😄
)


def enrichValue(typ: Ast.Type, value: Value): Result[Value] = {
translateValue(typ, value).map(_.toUnnormalizedValue)
}

def enrichVersionedValue(
typ: Ast.Type,
Expand All @@ -44,13 +59,12 @@ final class ValueEnricher(engine: Engine) {
def enrichContract(tyCon: Identifier, value: Value): Result[Value] =
enrichValue(Ast.TTyCon(tyCon), value)

private[this] def interface = engine.compiledPackages().interface
private[this] def interface = compiledPackages.interface

private[this] def handleLookup[X](lookup: => Either[LookupError, X]) = lookup match {
case Right(value) => ResultDone(value)
case Left(LookupError.MissingPackage(pkgId, context)) =>
engine
.loadPackage(pkgId, context)
loadPackage(pkgId, context)
.flatMap(_ =>
lookup match {
case Right(value) => ResultDone(value)
Expand Down
2 changes: 1 addition & 1 deletion ...engine/src/main/scala/com/digitalasset/daml/lf/engine/preprocessing/ValueTranslator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import com.daml.lf.value.Value._

import scala.annotation.tailrec

private[engine] final class ValueTranslator(
private[lf] final class ValueTranslator(
interface: language.PackageInterface,
// See Preprocessor scala doc for more details about the following flags.
forbidV0ContractId: Boolean,
Expand Down
2 changes: 2 additions & 0 deletions daml-lf/scenario-interpreter/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ da_scala_test_suite(
deps = [
":scenario-interpreter",
"//daml-lf/data",
"//daml-lf/engine",
"//daml-lf/interpreter",
"//daml-lf/language",
"//daml-lf/transaction",
Expand Down Expand Up @@ -77,6 +78,7 @@ da_scala_benchmark_jmh(
"//daml-lf/archive:daml_lf_archive_reader",
"//daml-lf/archive:daml_lf_dev_archive_proto_java",
"//daml-lf/data",
"//daml-lf/engine",
"//daml-lf/interpreter",
"//daml-lf/language",
"//daml-lf/scenario-interpreter",
Expand Down
40 changes: 33 additions & 7 deletions daml-lf/scenario-interpreter/src/main/scala/com/digitalasset/daml/lf/ScenarioRunner.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ package scenario

import com.daml.lf.data.Ref._
import com.daml.lf.data.{ImmArray, Ref, Time}
import com.daml.lf.engine.Engine
import com.daml.lf.engine.{Engine, ValueEnricher, Result, ResultDone, ResultError}
import com.daml.lf.engine.preprocessing.ValueTranslator
import com.daml.lf.language.{Ast, LookupError}
import com.daml.lf.transaction.{GlobalKey, NodeId, SubmittedTransaction}
import com.daml.lf.transaction.{GlobalKey, NodeId, SubmittedTransaction, CommittedTransaction}
import com.daml.lf.value.Value.{ContractId, ContractInst}
import com.daml.lf.speedy._
import com.daml.lf.speedy.SResult._
Expand Down Expand Up @@ -139,9 +140,6 @@ final case class ScenarioRunner(
ScenarioSuccess(ledger, machine.traceLog, machine.warningLog, diff, steps, finalValue)
}

private def crash(reason: String) =
throw Error.Internal(reason)

private def getParty(partyText: String, callback: Party => Unit) = {
val mangledPartyText = partyNameMangler(partyText)
Party.fromString(mangledPartyText) match {
Expand All @@ -158,6 +156,9 @@ final case class ScenarioRunner(

object ScenarioRunner {

private def crash(reason: String) =
throw Error.Internal(reason)

@deprecated("can be used only by sandbox classic.", since = "1.4.0")
def getScenarioLedger(
engine: Engine,
Expand Down Expand Up @@ -408,16 +409,41 @@ object ScenarioRunner {
traceLog = traceLog,
warningLog = warningLog,
commitLocation = location,
transactionNormalization = false,
)
val onLedger = ledgerMachine.withOnLedger(NameOf.qualifiedNameOfCurrentFunc)(identity)

def enrich(tx: SubmittedTransaction): SubmittedTransaction = {
val config = Engine.DevEngine().config
val valueTranslator =
new ValueTranslator(
interface = compiledPackages.interface,
forbidV0ContractId = config.forbidV0ContractId,
requireV1ContractIdSuffix = config.requireSuffixedGlobalContractId,
)
def translateValue(typ: Ast.Type, value: Value): Result[SValue] =
valueTranslator.translateValue(typ, value) match {
case Left(err) => ResultError(err)
case Right(sv) => ResultDone(sv)
}
def loadPackage(pkgId: PackageId, context: language.Reference): Result[Unit] = {
crash(LookupError.MissingPackage.pretty(pkgId, context))
}
val enricher = new ValueEnricher(compiledPackages, translateValue, loadPackage)
def consume[V](res: Result[V]): V =
res match {
case ResultDone(x) => x
case x => crash(s"unexpected Result when enriching value: $x")
}
SubmittedTransaction(consume(enricher.enrichTransaction(CommittedTransaction(tx))))
}

@tailrec
def go(): SubmissionResult[R] = {
ledgerMachine.run() match {
case SResult.SResultFinalValue(resultValue) =>
onLedger.ptxInternal.finish match {
case PartialTransaction.CompleteTransaction(tx, locationInfo, _) =>
ledger.commit(committers, readAs, location, tx, locationInfo) match {
ledger.commit(committers, readAs, location, enrich(tx), locationInfo) match {
case Left(err) =>
SubmissionError(err, onLedger.ptxInternal)
case Right(r) =>
Expand Down