LF: SBUKeyBuiltin clean up. #9572

remyhaemmerle-da · 2021-05-04T12:46:24Z

This PR some minor clean up for #9537 nd #9538.

In particular it:

Refactor the class KeyOperation outside SBUKeyBuiltin
Renames and refactor methods handleKeyFound, handleKeyArchived,
cidToSValue and cidToSExpr to gave them a more obvious meaning.
Replaces immutable.Map#+ by immutable.Map#updated (cosmetic)
Replaces trait by abstract class (cosmetic)
Replaces 'get(???).getOrElse(???)' by getOrElse(???, ???) (cosmetic)

CHANGELOG_BEGIN
CHANGELOG_END

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

remyhaemmerle-da · 2021-05-04T12:48:26Z

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

-      override def handleKeyArchived(machine: Machine, key: GlobalKey): Unit =
-        // TODO (MK) Produce a proper error here.
-        crash(s"Could not find key $key")
-      override def cidToSValue(cid: V.ContractId) =


The important of this method is not the returned type, but the fact it does not check for cid freshness.

remyhaemmerle-da · 2021-05-04T12:48:58Z

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

-        crash(s"Could not find key $key")
-      override def cidToSValue(cid: V.ContractId) =
-        SContractId(cid)
-      override def cidToSExpr(cid: V.ContractId) =


The importand of this methiod is not the return type, but the fact it check for cid freshness.

cocreature

I’m open to merging this but I’d like to be convinced first that this is an improvement: Before it was trivial to see what actually depends on the operation: Everything that called operation in some form.
Now I have to check which method call is private which is protected and for some of the protected calls we’re factoring some of the shared code into those methods so we lose the guarantee that they behave the same for those parts.

cocreature · 2021-05-04T12:49:01Z

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

          val stakeholders = cachedContract.signatories union cachedContract.observers
          throw SpeedyHungry(
            SResultNeedLocalKeyVisible(
              stakeholders,
              onLedger.committers,
              {
                case SVisibleByKey.Visible =>
-                  machine.returnValue = operation.cidToSValue(coid)
+                  handleActiveKey(machine, coid)


not a huge fan of this, you are factoring less than we did before.

cocreature · 2021-05-04T12:49:12Z

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

-        case Some(KeyInactive) =>
-          operation.handleKeyArchived(machine, gkey)
+        case Some(keyMapping) =>
+          handleKnownKey(machine, gkey, keyMapping)


same here, there is less factoring than before.

cocreature · 2021-05-04T12:50:31Z

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

+      SEImportValue(typ, V.ValueContractId(cid))
+
+    // Callback from the engine returned NotFound
+    protected def handleNewKeyNotFound(machine: Machine): Boolean


Suggested change

protected def handleNewKeyNotFound(machine: Machine): Boolean

protected def handleGlobalKeyNotFound(machine: Machine): Boolean

maybe a bit clearer?

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

cocreature · 2021-05-04T12:51:10Z

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

+    // We already saw this key and is still active
+    protected def handleActiveKey(machine: Machine, cid: V.ContractId): Unit
+    // CallBack from the engine returned a new Cid
+    protected def handleNewKeyFound(machine: Machine, cid: V.ContractId): Unit


I’d place this next to the corresponding handleNewKeyNotFound.

I let it here to see the correspondence with cidToSExpr. I will move it before merging, if we merge.

remyhaemmerle-da · 2021-05-04T13:02:52Z

I’m open to merging this but I’d like to be convinced first that this is an improvement: Before it was trivial to see what actually depends on the operation: Everything that called operation in some form.
Now I have to check which method call is private which is protected and for some of the protected calls we’re factoring some of the shared code into those methods so we lose the guarantee that they behave the same for those parts.

Sure, I have put the PR as draft to discuss advantage/inconvenient.
My approach is more traditional Scala/object oriented.

Let me think a bit about your arguments.

This PR some minor clean up for #9537 nd #9538. In particular it: - Remove the unecessary class KeyOperation (SBUKeyBuiltin is already a somehow a key Operation) - Rename and refactor methods handleKeyFound, handleKeyArchived, cidToSValue and cidToSExpr to gave them a more obvious meaning. - Replace immutable.Map#+ by immutable.Map#updated (cosmetic) - Replace trait by abstract class (cosmetic) CHANGELOG_BEGIN CHANGELOG_END

remyhaemmerle-da · 2021-05-05T09:40:43Z

@cocreature do you prefer this version ?

not a huge fan of this, you are factoring less than we did before.

I believe using only method of the form def handleXXX(...): Unit make the API more consistent and easier to understand.

cocreature

Thanks!

cocreature · 2021-05-05T10:18:33Z

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

+  }
+
+  private[this] object KeyOperation {
+    final class Fetch(override val templateId: TypeConName) extends KeyOperation {


why does the operation need to store the template id?

SEImport used it in the case of handleInputKeyFound, required the type.
The type is actually not used in the case of single contract ID, we could use a dummy (wrong !) type instead but I do not really like that.

cocreature · 2021-05-05T10:19:09Z

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

-  private[speedy] sealed abstract class SBUKeyBuiltin(templateId: TypeConName)
-      extends OnLedgerBuiltin(1) {
+  private[this] abstract class KeyOperation {
+    val templateId: TypeConName
    private[this] val typ = AstUtil.TContractId(Ast.TTyCon(templateId))


where is this used?

in cidImport, just 2 lines under.

@victormueller-da

This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @victormueller-da is in charge of this release. Commit log: ``` ca9e89b check whether collection.compat is unused when compiling for Scala 2.12 (#9604) 4a46097 Block concurrent SDK installations. (#9640) c757b61 Check byKey in transaction validation (#9641) 4f0ceff Fix #7170 (#9618) d310668 Add a REPL for each Scala target, for debugging. (#9643) 22b36b0 Include byKey in LF transaction spec (#9642) ca027e3 Add Transaction.contractKeyInputs to inputs of a transaction (#9631) c3d79d4 Allow OverloadedLists in data-dependencies (#9636) f8b35fe Cleans stateUpdates of ReadWriteServiceBridge (#9630) 3750b16 Serializalize byKey flag in Daml-LF transactions (#9632) 3b59f8c ledger-api-bench-tool - prototype [DPP-370] (#9606) f7b33d9 Authorization for daml script exports (#9629) 0d1f3db LF: refactor builtin exceptions in Speedy (#9605) d6d01b0 Swap order SEScopeExercise and SBUBeginExercise (#9621) 6c919b3 sandbox-on-x: Support `beginAfter` in state updates. (#9624) 26a8011 TLS for Daml Script exports (#9626) 9242540 Make error throw a GeneralError. (#9613) bdcf0ec update NOTICES file (#9623) 4c1fbeb Add duplicate contract key checks to Speedy (#9607) 871279f LF: extends LF command with Fetch and Lookup (#9587) 0acc4f1 Patch old Bazel derivation (#9622) b082274 Address vulnerabilities in Navigator (#9617) 560653a Unwind partial transaction on mustFail (#9608) f51145c Support mapping from old to new party ids in daml exports (#9591) 89e46bf Address security vulnerabilities in //compiler/daml-extension (#9615) fd62671 Introduce SINCE-LF-FEATURE in integration tests. (#9616) 48939b5 Address vulnerabilities in //language-support/ts/packages (#9614) cf59246 Add support for daml exceptions for append-only indexer (#9609) ab29f7c Move activeness check of globalKeyInputs into archive (#9610) 5c28de3 Upgrade grunt to address cve (#9611) 22ba5fd Remove builtin exception types from protobuf and ASTs. (#9595) b09a95f Adapt indexer to empty divulgences (#9598) 2fc7489 Filter divulgence to an empty set of parties (#9600) ad45213 participant-integration-api: Avoid the serial EC in integration testing. (#9603) f742a43 Dpp 336 sandbox classic on append only (#9561) e68bc0d Mark reset service tests as flaky (#9602) 2176173 Remove 1.dev-only things from LF 1.13 protofile. (#9599) 03034ec DPP-359 Add indexer flow level benchmark (#9509) 5aa5401 Drop todo for conversion of exception primitives (#9597) 968b5d8 KVL-921 Expose opentelemetry Context from Telemetry and TelemetryContext (#9573) 112c387 Refactor out setGlobalLogLevel into ContextualLogging (#9592) e584fec Drop com.daml.lf.engine.Event.collectEvents (#9596) 80b07da Only archive a key if it was brought into scope before (#9546) 7ea9340 Support old bash in daml-sdk-head (#9593) 45fbdef Handle rollback nodes in protoNodeInfo (#9589) 42d189d Support exceptions in Daml-LF encoder (#9590) 9c3913a LF: SBUKeyBuiltin clean up. (#9572) 5128206 Ledger API Server: Named threadpools (#9588) 26a53d8 Add logging command line option to ledger http service (#9581) 409833f Address rollback todo in ancient migration (#9583) a27b2c5 Address more exception todos (#9582) 2040556 Support rollback nodes in replay adapter (#9584) 192a77a update compat versions for 1.13.0-snapshot.20210504.6833.0.9ae787d0 (#9575) 81d508b LF: make SBuiltins pure (#9580) de80a6d Drop ValueBuiltinException (#9576) 1c456be Release yet another 1.13 snapshot (#9574) 8009891 Disable migration tests on macos on PRs (#9571) 2e93de7 Fix recording exceptions in Spans, add unit tests [KVL-874] (#9544) 121ded3 Accept a comma-separated list of parties for daml ledger export (#9568) ``` Changelog: ``` - [Daml Assistant] The assistant will now avoid installing SDK versions concurrently, waiting for the previous installation to finish before starting the next installation (if still necessary). This fixes a bug where the SDK would become corrupted because two installations were started concurrently. http-json: - add contextual id as logging context to distinguish different application runs in logs - add request id as logging context to distinguish different http requests within an application run - add for non-static endpoints trace logs which show how long processing it took in ns - [Integration Kit] - Created the ledger-api-bench-tool prototype for benchmarking ledger transaction streaming capabilities * [Daml export] Users can now define a mapping from parties in the original ledger state to parties to be used when recontructing the ledger state. The ``parties`` component of the argument to the generated export script now takes a mapping from old party name to new party name. - [Ledger HTTP Json Service] Logging now also tells service name if log level was changed. - [Ledger HTTP Json service] Logging can now be configured via the `--log-level` cli argument. Valid values are `error`, `warn`, `info` (default), `debug`, `trace` ``` CHANGELOG_BEGIN CHANGELOG_END

@victormueller-da

* release 1.13.0-snapshot.20210511.6892.0.ca9e89b3 This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @victormueller-da is in charge of this release. Commit log: ``` ca9e89b check whether collection.compat is unused when compiling for Scala 2.12 (#9604) 4a46097 Block concurrent SDK installations. (#9640) c757b61 Check byKey in transaction validation (#9641) 4f0ceff Fix #7170 (#9618) d310668 Add a REPL for each Scala target, for debugging. (#9643) 22b36b0 Include byKey in LF transaction spec (#9642) ca027e3 Add Transaction.contractKeyInputs to inputs of a transaction (#9631) c3d79d4 Allow OverloadedLists in data-dependencies (#9636) f8b35fe Cleans stateUpdates of ReadWriteServiceBridge (#9630) 3750b16 Serializalize byKey flag in Daml-LF transactions (#9632) 3b59f8c ledger-api-bench-tool - prototype [DPP-370] (#9606) f7b33d9 Authorization for daml script exports (#9629) 0d1f3db LF: refactor builtin exceptions in Speedy (#9605) d6d01b0 Swap order SEScopeExercise and SBUBeginExercise (#9621) 6c919b3 sandbox-on-x: Support `beginAfter` in state updates. (#9624) 26a8011 TLS for Daml Script exports (#9626) 9242540 Make error throw a GeneralError. (#9613) bdcf0ec update NOTICES file (#9623) 4c1fbeb Add duplicate contract key checks to Speedy (#9607) 871279f LF: extends LF command with Fetch and Lookup (#9587) 0acc4f1 Patch old Bazel derivation (#9622) b082274 Address vulnerabilities in Navigator (#9617) 560653a Unwind partial transaction on mustFail (#9608) f51145c Support mapping from old to new party ids in daml exports (#9591) 89e46bf Address security vulnerabilities in //compiler/daml-extension (#9615) fd62671 Introduce SINCE-LF-FEATURE in integration tests. (#9616) 48939b5 Address vulnerabilities in //language-support/ts/packages (#9614) cf59246 Add support for daml exceptions for append-only indexer (#9609) ab29f7c Move activeness check of globalKeyInputs into archive (#9610) 5c28de3 Upgrade grunt to address cve (#9611) 22ba5fd Remove builtin exception types from protobuf and ASTs. (#9595) b09a95f Adapt indexer to empty divulgences (#9598) 2fc7489 Filter divulgence to an empty set of parties (#9600) ad45213 participant-integration-api: Avoid the serial EC in integration testing. (#9603) f742a43 Dpp 336 sandbox classic on append only (#9561) e68bc0d Mark reset service tests as flaky (#9602) 2176173 Remove 1.dev-only things from LF 1.13 protofile. (#9599) 03034ec DPP-359 Add indexer flow level benchmark (#9509) 5aa5401 Drop todo for conversion of exception primitives (#9597) 968b5d8 KVL-921 Expose opentelemetry Context from Telemetry and TelemetryContext (#9573) 112c387 Refactor out setGlobalLogLevel into ContextualLogging (#9592) e584fec Drop com.daml.lf.engine.Event.collectEvents (#9596) 80b07da Only archive a key if it was brought into scope before (#9546) 7ea9340 Support old bash in daml-sdk-head (#9593) 45fbdef Handle rollback nodes in protoNodeInfo (#9589) 42d189d Support exceptions in Daml-LF encoder (#9590) 9c3913a LF: SBUKeyBuiltin clean up. (#9572) 5128206 Ledger API Server: Named threadpools (#9588) 26a53d8 Add logging command line option to ledger http service (#9581) 409833f Address rollback todo in ancient migration (#9583) a27b2c5 Address more exception todos (#9582) 2040556 Support rollback nodes in replay adapter (#9584) 192a77a update compat versions for 1.13.0-snapshot.20210504.6833.0.9ae787d0 (#9575) 81d508b LF: make SBuiltins pure (#9580) de80a6d Drop ValueBuiltinException (#9576) 1c456be Release yet another 1.13 snapshot (#9574) 8009891 Disable migration tests on macos on PRs (#9571) 2e93de7 Fix recording exceptions in Spans, add unit tests [KVL-874] (#9544) 121ded3 Accept a comma-separated list of parties for daml ledger export (#9568) ``` Changelog: ``` - [Daml Assistant] The assistant will now avoid installing SDK versions concurrently, waiting for the previous installation to finish before starting the next installation (if still necessary). This fixes a bug where the SDK would become corrupted because two installations were started concurrently. http-json: - add contextual id as logging context to distinguish different application runs in logs - add request id as logging context to distinguish different http requests within an application run - add for non-static endpoints trace logs which show how long processing it took in ns - [Integration Kit] - Created the ledger-api-bench-tool prototype for benchmarking ledger transaction streaming capabilities * [Daml export] Users can now define a mapping from parties in the original ledger state to parties to be used when recontructing the ledger state. The ``parties`` component of the argument to the generated export script now takes a mapping from old party name to new party name. - [Ledger HTTP Json Service] Logging now also tells service name if log level was changed. - [Ledger HTTP Json service] Logging can now be configured via the `--log-level` cli argument. Valid values are `error`, `warn`, `info` (default), `debug`, `trace` ``` CHANGELOG_BEGIN CHANGELOG_END * Release snapshot 1.14.0 changelog_begin changelog_end Co-authored-by: Azure Pipelines DAML Build <support@digitalasset.com> Co-authored-by: victor.mueller@digitalasset.com <mueller.vpr@gmail.com>

remyhaemmerle-da changed the base branch from main to globalkeyinputs May 4, 2021 12:47

remyhaemmerle-da commented May 4, 2021

View reviewed changes

cocreature reviewed May 4, 2021

View reviewed changes

Base automatically changed from globalkeyinputs to main May 4, 2021 13:49

remyhaemmerle-da added 2 commits May 5, 2021 11:20

Address Moritz' review

172d26b

remyhaemmerle-da force-pushed the remy-key-ops branch from b1d11eb to 172d26b Compare May 5, 2021 09:20

remyhaemmerle-da marked this pull request as ready for review May 5, 2021 09:22

remyhaemmerle-da added 2 commits May 5, 2021 11:40

cosmetic

145c4d6

move SBULookupKey arround

547cddd

remyhaemmerle-da requested a review from cocreature May 5, 2021 10:12

remyhaemmerle-da added the component/daml-engine DAML-LF Engine & Interpreter label May 5, 2021

remyhaemmerle-da added this to the Maintenance milestone May 5, 2021

cocreature approved these changes May 5, 2021

View reviewed changes

remyhaemmerle-da merged commit 9c3913a into main May 6, 2021

remyhaemmerle-da deleted the remy-key-ops branch May 6, 2021 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LF: SBUKeyBuiltin clean up. #9572

LF: SBUKeyBuiltin clean up. #9572

remyhaemmerle-da commented May 4, 2021 •

edited

Loading

remyhaemmerle-da May 4, 2021 •

edited

Loading

remyhaemmerle-da May 4, 2021

cocreature left a comment

cocreature May 4, 2021

cocreature May 4, 2021

cocreature May 4, 2021

cocreature May 4, 2021

remyhaemmerle-da May 4, 2021

remyhaemmerle-da commented May 4, 2021

remyhaemmerle-da commented May 5, 2021 •

edited

Loading

cocreature left a comment

cocreature May 5, 2021

remyhaemmerle-da May 5, 2021

cocreature May 5, 2021

remyhaemmerle-da May 5, 2021

	protected def handleNewKeyNotFound(machine: Machine): Boolean
	protected def handleGlobalKeyNotFound(machine: Machine): Boolean

LF: SBUKeyBuiltin clean up. #9572

LF: SBUKeyBuiltin clean up. #9572

Conversation

remyhaemmerle-da commented May 4, 2021 • edited Loading

Pull Request Checklist

remyhaemmerle-da May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocreature left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remyhaemmerle-da commented May 4, 2021

remyhaemmerle-da commented May 5, 2021 • edited Loading

cocreature left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remyhaemmerle-da commented May 4, 2021 •

edited

Loading

remyhaemmerle-da May 4, 2021 •

edited

Loading

remyhaemmerle-da commented May 5, 2021 •

edited

Loading