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

LF: V1 Contract ID check in Preprocessor #10687

Merged
merged 3 commits into from
Aug 30, 2021
Merged

LF: V1 Contract ID check in Preprocessor #10687

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
463c1e5
LF: V1 Contract ID check in Preprocessor
remyhaemmerle-da Aug 26, 2021
78d8f0d
Address Moritz' review
remyhaemmerle-da Aug 30, 2021
88c5c2d
Add reason in IllegalContractId
remyhaemmerle-da Aug 30, 2021
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
3 changes: 2 additions & 1 deletion daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/Engine.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ class Engine(val config: EngineConfig = Engine.StableConfig) {
private[engine] val preprocessor =
new preprocessing.Preprocessor(
compiledPackages = compiledPackages,
requiredCidSuffix = config.requireSuffixedGlobalCids,
requireV1ContractId = config.requireV1ContractId,
requireV1ContractIdSuffix = config.requireSuffixedGlobalContractId,
)

def info = new EngineInfo(config)
Expand Down
5 changes: 2 additions & 3 deletions daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/EngineConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,8 @@ final case class EngineConfig(
stackTraceMode: Boolean = false,
profileDir: Option[Path] = None,
contractKeyUniqueness: ContractKeyUniquenessMode = ContractKeyUniquenessMode.On,
// TODO: https://github.com/digital-asset/daml/issues/10504
// switch the default to true
requireSuffixedGlobalCids: Boolean = false,
requireV1ContractId: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe forbidV0 would be more future proof if we ever add v2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so in canton, I would rename use use of requireV1ContractId to forbidV0ContractId.

And no need to use requireV1ContractIdSuffix, right, @remyhaemmerle-da ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@oliverse-da you should use both: forbidV0ContractId to make sure you never get a global v0 cid. requireV1ContractIdSuffix to make sure that all global v1 cids have a suffix.

requireSuffixedGlobalContractId: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

did you drop the todo intentionally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. After our discussion last week where you told me you are not concern about misusage of the flags (because we have only 3 actively maintain ledgers) I just thought the default behavior does not really matter.

) {

private[lf] def getCompilerConfig: speedy.Compiler.Config =
Expand Down
6 changes: 3 additions & 3 deletions daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/Error.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,14 @@ object Error {
override val message: String,
) extends Error

final case class ValueNesting(value: Value[Value.ContractId]) extends Error {
final case class ValueNesting(culprit: Value[Value.ContractId]) extends Error {
override def message: String =
s"Provided value exceeds maximum nesting level of ${Value.MAXIMUM_NESTING}"
}

final case class NonSuffixedCid(cid: Value.ContractId.V1) extends Error {
final case class IllegalContractId(cid: Value.ContractId) extends Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we include the reason here and tell users that it failed because of lack of suffix or because they used v0?

Copy link
Collaborator Author

@remyhaemmerle-da remyhaemmerle-da Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I did not feel it was necessary, in the sens that if it's happen it means the user try to feed to the ledger ID that it has cooked himself. If the user want to shoot himself in a foot, I do not see why I shoot tell him which foot is it.
If you really think it is useful, I will add the reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough, I tend to lean on the side of including too much information rather than too little in case we need it later but I don’t feel all that strongly about this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

88c5c2d

override def message: String =
s"Provided Contract ID ${cid.coid} is not suffixed"
s"""Illegal Contract ID "${cid.coid}""""
}

final case class RootNode(nodeId: NodeId, override val message: String) extends Error
Expand Down
12 changes: 10 additions & 2 deletions ...ne/src/main/scala/com/digitalasset/daml/lf/engine/preprocessing/CommandPreprocessor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,17 @@ import scala.annotation.tailrec

private[lf] final class CommandPreprocessor(
interface: language.Interface,
requiredCidSuffix: Boolean,
// See Preprocessor.requiredCidSuffix for more details about the following flags.
requireV1ContractId: Boolean,
requireV1ContractIdSuffix: Boolean,
) {

val valueTranslator = new ValueTranslator(interface, requiredCidSuffix)
val valueTranslator =
new ValueTranslator(
interface = interface,
requireV1ContractId = requireV1ContractId,
requireV1ContractIdSuffix = requireV1ContractIdSuffix,
)

import Preprocessor._

Expand Down Expand Up @@ -88,6 +95,7 @@ private[lf] final class CommandPreprocessor(
}

// returns the speedy translation of an LF command together with all the contract IDs contains inside.
@throws[Error.Preprocessing.Error]
private[preprocessing] def unsafePreprocessCommand(
cmd: command.Command
): speedy.Command = {
Expand Down
17 changes: 13 additions & 4 deletions ...lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/preprocessing/Preprocessor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import scala.annotation.tailrec
* - checks value nesting does not overpass 100;
* - checks a LF command/value is properly typed according the
* Daml-LF package definitions;
* - checks for Contract ID suffix (see [[requiredCidSuffix]]);
* - checks for Contract ID suffix (see [[requireV1ContractIdSuffix]]);
* - translates a LF command/value into speedy command/value; and
* - translates a complete transaction into a list of speedy
* commands.
Expand All @@ -31,20 +31,29 @@ import scala.annotation.tailrec
* Daml-LF package definitions against the command should
* resolved/typechecked. It is updated dynamically each time the
* [[ResultNeedPackage]] continuation is called.
* @param requiredCidSuffix when `true` the preprocessor will reject
* @param requireV1ContractId when `true` the preprocessor will reject
* any value/command/transaction that contains V0 Contract IDs
* without suffixed.
* @param requireV1ContractIdSuffix when `true` the preprocessor will reject
* any value/command/transaction that contains V1 Contract IDs
* without suffixed.
*/
private[engine] final class Preprocessor(
compiledPackages: MutableCompiledPackages,
requiredCidSuffix: Boolean = true,
requireV1ContractId: Boolean = true,
requireV1ContractIdSuffix: Boolean = true,
) {

import Preprocessor._

import compiledPackages.interface

val commandPreprocessor = new CommandPreprocessor(interface, requiredCidSuffix)
val commandPreprocessor =
new CommandPreprocessor(
interface = interface,
requireV1ContractId = requireV1ContractId,
requireV1ContractIdSuffix = requireV1ContractIdSuffix,
)
val transactionPreprocessor = new TransactionPreprocessor(commandPreprocessor)

// This pulls all the dependencies of in `typesToProcess0` and `tyConAlreadySeen0`
Expand Down
35 changes: 23 additions & 12 deletions ...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 @@ -16,8 +16,9 @@ import scala.annotation.tailrec

private[engine] final class ValueTranslator(
interface: language.Interface,
// See Preprocessor.requiredCidSuffix for more details about the following flag.
requiredCidSuffix: Boolean,
// See Preprocessor.requiredCidSuffix for more details about the following flags.
requireV1ContractId: Boolean,
requireV1ContractIdSuffix: Boolean,
) {

import Preprocessor._
Expand All @@ -42,18 +43,28 @@ private[engine] final class ValueTranslator(
go(fields, Map.empty)
}

private[preprocessing] val unsafeTranslateCid: ContractId => SValue.SContractId =
if (requiredCidSuffix) {
case cid1: ContractId.V1 =>
if (cid1.suffix.isEmpty)
throw Error.Preprocessing.NonSuffixedCid(cid1)
private[this] val unsafeTranslateV1Cid: ContractId.V1 => SValue.SContractId =
if (requireV1ContractIdSuffix)
cid =>
if (cid.suffix.isEmpty)
throw Error.Preprocessing.IllegalContractId(cid)
else
SValue.SContractId(cid1)
case cid0: ContractId.V0 =>
SValue.SContractId(cid0)
}
SValue.SContractId(cid)
else
SValue.SContractId(_)

private[this] val unsafeTranslateV0Cid: ContractId.V0 => SValue.SContractId =
if (requireV1ContractId)
cid => throw Error.Preprocessing.IllegalContractId(cid)
else
SValue.SContractId
SValue.SContractId(_)

@throws[Error.Preprocessing.Error]
private[preprocessing] def unsafeTranslateCid(cid: ContractId): SValue.SContractId =
cid match {
case cid1: ContractId.V1 => unsafeTranslateV1Cid(cid1)
case cid0: ContractId.V0 => unsafeTranslateV0Cid(cid0)
}

// For efficient reason we do not produce here the monad Result[SValue] but rather throw
// exception in case of error or package missing.
Expand Down
Loading