JSON API: etx-500 add type information into the package ids of template ids #19211

raphael-speyer-da · 2024-05-16T05:31:35Z

A template/interface id (ContractTypeId) consists of three parts: package, module and entity.

So far the package portion has been modeled with a String, where that string could represent either a package name or a package id, depending on whether it started with a "#" prefix or not, respectively. However there are parts of the code, namely those that deal with the db cache, that are designed to work with contract type ids whose package portion is only a package id. The type system was not helping us to ensure this so far.

Fortunately the ContractTypeId class is parameterised with the type of the package, so this PR replaces all existing instantiations which used String packages, with either PackageId or PackageRef (which is a sum type which can contain either a PackageId or a PackageName).

The ContractTypeRef type contains all the package info we have about a contract type id. Its original field now returns a ContractTypeId[PackageRef], whereas its latestPkgId returns a ContractTypeId[PackageId]. This helps us to ensure that we are passing around the right kind of package specifiers to the parts of the code that need one or the other.

There are some type aliases used, that may be useful to help read and understand the changes, which tend to look like

   type RequiredPkg = CtId[Ref.PackageRef]
   type RequiredPkgId = CtId[Ref.PackageId]

i.e. if it ends with Pkg it uses a PackgeRef and if it ends with PkgId uses a PackageId.
Note also that the Resolved terminology is an orthogonal concern which refers to whether this id has been determined to belong to a template vs interface (i.e. it extends Definite), or whether that is not yet known.

raphael-speyer-da · 2024-05-17T03:48:35Z

sdk/fmt.sh

@@ -51,7 +51,7 @@ check_diff() {
  # "${@:3}" command
  changed_files=$(git diff --name-only --diff-filter=ACMRT "$1" | grep $2 | grep -E -v '^canton(-3x)?/' || [[ $? == 1 ]])
  if [[ -n "$changed_files" ]]; then
-    run "${@:3}" ${changed_files[@]:-}


As a developer, we now run ./fmt.sh --diff from within the sdk/ dir, but git still returns a list of changed files with a sdk prefix. This change removes the sdk/ prefix from those paths.

I like this change but in a small test I did the prefix removal only applied to the first file, so:

As this is an inf change it may be better to put it in a dedicated PR (@garyverhaegen-da may have an opinion on this)

As the canton pattern match in change files seems to assume no no sdk prefix so will not work

Consider changed_files=$(git diff --name-only --diff-filter=ACMRT "$1" | grep $2 | sed 's/^sdk\///' | grep -E -v '^canton(-3x)?/' || [[ $? == 1 ]])

Extracted here #19237

I like this change but in a small test I did the prefix removal only applied to the first file, so:

As this is an inf change it may be better to put it in a dedicated PR (@garyverhaegen-da may have an opinion on this)

I really don't - I don't use the pre-commit. My only opinion on the whole thing is it should be opt-in rather than opt-out.

raphael-speyer-da · 2024-05-17T03:49:23Z

sdk/ledger-service/db-backend/src/main/scala/com/digitalasset/http/dbbackend/Queries.scala

      log: LogHandler,
      lc: LoggingContextOf[InstanceUUID],
  ): ConnectionIO[SurrogateTpId] = surrogateTemplateId(None, packageId, moduleName, entityName)

  final def surrogateTemplateId(
      packageName: Option[String],
-      packageId: String,


Within the db cache, we always use a package id as the basis for the surrogate template ids, i.e. an integeter which corresponds to a specific template.

raphael-speyer-da · 2024-05-17T03:52:47Z

sdk/ledger-service/fetch-contracts/src/main/scala/fetchcontracts/AcsTxStreams.scala

      parties: domain.PartySet,
-      contractTypeIds: List[ContractTypeId.Resolved],


This is used to build a template filter for grpc requests to the ledger. In these filters the package may be specified with a name or an id, and this function is agnostic to the package type.

raphael-speyer-da · 2024-05-17T03:54:51Z

sdk/ledger-service/fetch-contracts/src/main/scala/fetchcontracts/domain.scala

@@ -89,22 +89,22 @@ package domain {
  )

  object ActiveContract {
-    type ResolvedCtTyId[+LfV] = ActiveContract[ContractTypeId.Resolved, LfV]


ActiveContract is always something that is returned from the ledger, and as such it the template ids are specified with a specific package id.

raphael-speyer-da · 2024-05-17T03:56:10Z

sdk/ledger-service/fetch-contracts/src/main/scala/fetchcontracts/domain.scala


-      def apply(id: ContractTypeId.Resolved): ExtractAs[ContractTypeId.Resolved] = id match {
+      def apply(id: ContractTypeId.Definite[_]): ExtractAs[ContractTypeId.Definite] = id match {


We can build an ExtractAs without considering the form of package of the id we queried, which is why the type parameter for it is _.

raphael-speyer-da · 2024-05-17T04:15:27Z

sdk/ledger-service/fetch-contracts/src/main/scala/fetchcontracts/domain/ContractTypeId.scala


  private[this] def qualifiedName(a: CtId[_]): Ref.QualifiedName =
    Ref.QualifiedName(
      Ref.DottedName.assertFromString(a.moduleName),
      Ref.DottedName.assertFromString(a.entityName),
    )

-  final def toLedgerApiValue(a: RequiredPkg): Ref.Identifier = {
+  final def toLedgerApiValue(a: RequiredPkgId): Ref.Identifier = {


In practice we only use this for calculating a contract key hash. If the type is from a package new enough to have a package name, then the package id portion is ignored and replaced with the name for the purposes of calculating the hash. If it's older, then the package id would be the appropriate package specifier anyway.

raphael-speyer-da · 2024-05-17T04:20:47Z

sdk/ledger-service/fetch-contracts/src/main/scala/fetchcontracts/domain/ContractTypeId.scala

-sealed abstract class ContractTypeRef[+CtTyId](
-    orig: CtTyId,
-    ids: NonEmpty[Seq[CtTyId]],
+sealed abstract class ContractTypeRef[+CtTyId[T] <: ContractTypeId[T]](


Now the original field will always use a package reference, but latestPkgId and allPkgIds will always use package ids. This was the case before, but not encoded by the types as they all used used strings.

To support this, ContractTypeRef, also moves from being parameterised with a type, to being parameterised with a type constructor. This is so that we can have different package specifiers for different fields, which both share the same contract type, e.g. original returns Template[PackageRef] but latestPkgId returns Template[PackageId].

raphael-speyer-da · 2024-05-17T04:26:13Z

sdk/ledger-service/http-json/src/main/scala/com/digitalasset/http/ContractsFetch.scala

@@ -71,7 +71,7 @@ private class ContractsFetch(
      jwt: Jwt,
      ledgerId: LedgerApiDomain.LedgerId,
      parties: domain.PartySet,
-      templateIds: List[domain.ContractTypeId.Resolved],
+      templateIds: List[domain.ContractTypeId.ResolvedPkgId],


Everything within this module relates to the db cache and thus needs to work with package ids.

raphael-speyer-da · 2024-05-17T04:31:02Z

sdk/ledger-service/http-json/src/main/scala/com/digitalasset/http/PackageService.scala

@@ -478,44 +474,48 @@ object PackageService {

  def buildTemplateIdMap[CtId[T] <: ContractTypeId.Definite[T] with ContractTypeId.Ops[CtId, T]](
      idName: PackageNameMap,
-      ids: Set[RequiredPkg[CtId]],
+      ids: Set[CtId[Ref.PackageId]],


These contract ids come out of hte package definition itself, and thus will have the package id embedded within them.

raphael-speyer-da · 2024-05-17T05:10:01Z

sdk/ledger-service/http-json/src/test/scala/com/digitalasset/http/Generators.scala

    genDuplicateModuleEntityApiIdentifiers.map(xs =>
      xs.map(domain.ContractTypeId.Template.fromLedgerApi)
    )

  final case class PackageIdGen[A](gen: Gen[A])

-  implicit val RequiredPackageIdGen: PackageIdGen[String] = PackageIdGen(Gen.identifier)
+  private val genPkgIdentifier = Gen.identifier.map(s => s.substring(0, 64 min s.length))


Package ids may not be longer than 64 chars, so just truncate the randomised values to that length.

…nd in the case that we queried an interface we can still just get the template id out of the ArchivedEvent, rather than having to use the one we queried with

…ore consistent type alias names. We may want to coverhaul these names though as they are still not very clear run-all-tests: true

simonmaxen-da

Looks good apart from the fmt.sh change that would be better moved into a different PR.

simonmaxen-da · 2024-05-20T13:49:10Z

sdk/fmt.sh

@@ -51,7 +51,7 @@ check_diff() {
  # "${@:3}" command
  changed_files=$(git diff --name-only --diff-filter=ACMRT "$1" | grep $2 | grep -E -v '^canton(-3x)?/' || [[ $? == 1 ]])
  if [[ -n "$changed_files" ]]; then
-    run "${@:3}" ${changed_files[@]:-}


I like this change but in a small test I did the prefix removal only applied to the first file, so:

As this is an inf change it may be better to put it in a dedicated PR (@garyverhaegen-da may have an opinion on this)

As the canton pattern match in change files seems to assume no no sdk prefix so will not work

Consider changed_files=$(git diff --name-only --diff-filter=ACMRT "$1" | grep $2 | sed 's/^sdk\///' | grep -E -v '^canton(-3x)?/' || [[ $? == 1 ]])

simonmaxen-da · 2024-05-20T14:37:00Z

sdk/ledger-service/fetch-contracts/src/main/scala/fetchcontracts/domain/ContractTypeId.scala


-  final def fromLedgerApi(in: lav1.value.Identifier): RequiredPkg =
-    apply(in.packageId, in.moduleName, in.entityName)
+  final def fromLedgerApi(in: lav1.value.Identifier): RequiredPkgId =


Do you mean when reading from the DB or reading from the ledger-API ?

erwin-ramirez-da

LGTM

raphael-speyer-da force-pushed the rls/etx-500-add-type-info-for-pkg-id branch 3 times, most recently from 33507f8 to 2b8802f Compare May 16, 2024 23:48

raphael-speyer-da requested review from erwin-ramirez-da and simonmaxen-da May 16, 2024 23:50

raphael-speyer-da force-pushed the rls/etx-500-add-type-info-for-pkg-id branch from 2b8802f to e17517c Compare May 17, 2024 02:05

add more type info into the package ids in the template ids

7cb03aa

raphael-speyer-da force-pushed the rls/etx-500-add-type-info-for-pkg-id branch from e17517c to 7cb03aa Compare May 17, 2024 02:58

better fix for fmt.sh --diff

6af2244

raphael-speyer-da commented May 17, 2024

View reviewed changes

raphael-speyer-da force-pushed the rls/etx-500-add-type-info-for-pkg-id branch from e8d8c63 to 7020195 Compare May 20, 2024 01:02

= added 2 commits May 20, 2024 11:05

ArchivedContract should use a package id rather than a package ref, a…

8c13fca

…nd in the case that we queried an interface we can still just get the template id out of the ArchivedEvent, rather than having to use the one we queried with

mv ContractTypeId.Resolved ContractTypeId.ResolvedPkg, for slightly m…

51f45f0

…ore consistent type alias names. We may want to coverhaul these names though as they are still not very clear run-all-tests: true

raphael-speyer-da force-pushed the rls/etx-500-add-type-info-for-pkg-id branch from 7020195 to 51f45f0 Compare May 20, 2024 01:05

simonmaxen-da approved these changes May 20, 2024

View reviewed changes

erwin-ramirez-da approved these changes May 21, 2024

View reviewed changes

revert changes to fmt.sh to put into a separate PR

6713f61

raphael-speyer-da merged commit 8cf8027 into main-2.x May 22, 2024
17 checks passed

raphael-speyer-da deleted the rls/etx-500-add-type-info-for-pkg-id branch May 22, 2024 03:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON API: etx-500 add type information into the package ids of template ids #19211

JSON API: etx-500 add type information into the package ids of template ids #19211

raphael-speyer-da commented May 16, 2024 •

edited

raphael-speyer-da May 17, 2024

simonmaxen-da May 20, 2024

raphael-speyer-da May 21, 2024

garyverhaegen-da May 21, 2024

raphael-speyer-da May 17, 2024

raphael-speyer-da May 17, 2024

raphael-speyer-da May 17, 2024

raphael-speyer-da May 17, 2024

raphael-speyer-da May 17, 2024 •

edited

raphael-speyer-da May 17, 2024

raphael-speyer-da May 17, 2024

raphael-speyer-da May 17, 2024

raphael-speyer-da May 17, 2024

simonmaxen-da left a comment

simonmaxen-da May 20, 2024

simonmaxen-da May 20, 2024

erwin-ramirez-da left a comment

		parties: domain.PartySet,
		contractTypeIds: List[ContractTypeId.Resolved],


		def apply(id: ContractTypeId.Resolved): ExtractAs[ContractTypeId.Resolved] = id match {
		def apply(id: ContractTypeId.Definite[_]): ExtractAs[ContractTypeId.Definite] = id match {

JSON API: etx-500 add type information into the package ids of template ids #19211

JSON API: etx-500 add type information into the package ids of template ids #19211

Conversation

raphael-speyer-da commented May 16, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphael-speyer-da May 17, 2024 • edited

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

simonmaxen-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwin-ramirez-da left a comment

Choose a reason for hiding this comment

raphael-speyer-da commented May 16, 2024 •

edited

raphael-speyer-da May 17, 2024 •

edited