ledger-api: Kill legacy identifier #2211

remyhaemmerle-da · 2019-07-18T09:13:59Z

We drop support of (deprecated) legacy identifier in the ledger API.
This fixes #2208

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
Add a line to the release notes, if appropriate

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.

stefanobaghino-da

Kill it with fire! 🔥

gerolf-da

👍

...scala/bindings/src/main/scala/com/digitalasset/ledger/client/binding/binding/Primitive.scala

S11001001 · 2019-07-18T16:41:12Z

...scala/bindings/src/main/scala/com/digitalasset/ledger/client/binding/binding/Primitive.scala

-      * gRPC Identifier was built with the very old and ambiguous two-argument
-      * form.  Use [[LegacyTemplateId]] instead if you want to extract
-      * the two-argument form.
+    /** Package ID, module name, and entity name.
      */
    def unapply[Tpl](t: TemplateId[Tpl]): Option[(String, String, String)]


Suggested change

def unapply[Tpl](t: TemplateId[Tpl]): Option[(String, String, String)]

def unapply[Tpl](t: TemplateId[Tpl]): Some[(String, String, String)]

If you don't put Some here too it won't be visible.

...scala/bindings/src/main/scala/com/digitalasset/ledger/client/binding/binding/Primitive.scala

S11001001 · 2019-07-18T16:41:56Z

...scala/bindings/src/main/scala/com/digitalasset/ledger/client/binding/binding/Primitive.scala

      // TODO SC DEL-6727 use this instead with daml-lf value interface
      // rpcvalue.Identifier unapply t.unwrap
      t.unwrap match {
-        case rpcvalue.Identifier(packageId, _, moduleName, entityName)
+        case rpcvalue.Identifier(packageId, moduleName, entityName)
            if moduleName.nonEmpty && entityName.nonEmpty =>
          Some((packageId, moduleName, entityName))
        case _ => None


Suggested change

case _ => None

I see the reasoning behind your suggestion but I'd suggest to keep the default case and throw a telling exception there: MatchErrors tend to be very vague and it would be easier to understand the issue if the exception had a meaningful message based on the context.

There is no possibility of a MatchError in the set of changes I've suggested. That is to say, the prior case becomes exhaustive when you remove the condition.

Now there isn't, which is what makes the situation exceptional. In general, I hardly see good reasons to leave generic MatchErrors with very error messages leaving a lot to be said being possibly thrown at runtime when a more meaningful and contextualized. If there is not possibility that the match couldn't fail, wouldn't it be possible to simply do something like

val unwrapped = t.unwrap Some((unwrapped.packageId, unwrapped.moduleName, unwrapped.entityName))

Seems more direct and readable.

Seems more direct and readable.

You are right but I can't express that in GH change suggestions :)

val rpcvalue.Identifier(packageId, moduleName, entityName) = t.unwrap Some((packageId, moduleName, entityName))

nickchapman-da

The one line change to the haskell Convert,hs looks good to me.

gerolf-da · 2019-07-23T07:57:04Z

...scala/bindings/src/main/scala/com/digitalasset/ledger/client/binding/binding/Primitive.scala

-        case _ => None
-      }
+    override def unapply[Tpl](t: TemplateId[Tpl]): Some[(String, String, String)] = {
+      val rpcvalue.Identifier(packageId, moduleName, entityName) = t.unwrap


No we don't check anymore that moduleName and entityName are non-empty.

For what I understand, the test was used to check if it is a new/old identifier.

Okay, but a templateId is only valid if the individual strings coming from the wire aren't empty. But maybe this is not the place to check this.

The check is done inside the ledger API.
We probably do not need to duplicate the test in Scala binding side.

remyhaemmerle-da requested review from gerolf-da and stefanobaghino-da July 18, 2019 09:14

remyhaemmerle-da requested review from leo-da, rautenrieth-da and S11001001 as code owners July 18, 2019 09:14

remyhaemmerle-da added the component/ledger Sandbox and Ledger API label Jul 18, 2019

remyhaemmerle-da added this to the Maintenance milestone Jul 18, 2019

remyhaemmerle-da force-pushed the ledger-drop-deprecated-id branch from 6653eef to 60bcbf7 Compare July 18, 2019 09:18

stefanobaghino-da approved these changes Jul 18, 2019

View reviewed changes

gerolf-da approved these changes Jul 18, 2019

View reviewed changes

remyhaemmerle-da force-pushed the ledger-drop-deprecated-id branch 3 times, most recently from eee4617 to 7427e78 Compare July 18, 2019 12:59

remyhaemmerle-da requested a review from nickchapman-da as a code owner July 18, 2019 12:59

remyhaemmerle-da force-pushed the ledger-drop-deprecated-id branch 2 times, most recently from 610336f to b662cfa Compare July 18, 2019 14:08

remyhaemmerle-da changed the title ~~ledger-api: drop support for legacy identifier~~ ledger-api: Kill legacy identifier Jul 18, 2019

remyhaemmerle-da force-pushed the ledger-drop-deprecated-id branch from b662cfa to 653ff3b Compare July 18, 2019 14:16

S11001001 requested changes Jul 18, 2019

View reviewed changes

nickchapman-da reviewed Jul 18, 2019

View reviewed changes

remyhaemmerle-da force-pushed the ledger-drop-deprecated-id branch from 653ff3b to 7a0d73f Compare July 23, 2019 07:42

gerolf-da reviewed Jul 23, 2019

View reviewed changes

remyhaemmerle-da requested a review from S11001001 July 23, 2019 08:18

remyhaemmerle-da force-pushed the ledger-drop-deprecated-id branch from 7a0d73f to 958fc45 Compare July 23, 2019 16:29

S11001001 approved these changes Jul 23, 2019

View reviewed changes

remyhaemmerle-da added the automerge-after-rebase label Jul 23, 2019

remyhaemmerle-da added 3 commits July 23, 2019 20:06

ledger-api: drop support for legacy identifier

bbdacf0

address scala-codegen: Stephen's comments

55240cd

language support: fix test

2898f71

cocreature force-pushed the ledger-drop-deprecated-id branch from 958fc45 to 2898f71 Compare July 23, 2019 20:06

mergify bot merged commit 53de5ed into master Jul 23, 2019

mergify bot deleted the ledger-drop-deprecated-id branch July 23, 2019 20:34

gerolf-da mentioned this pull request Aug 9, 2019

Ledger API Identifier improvements #1484

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ledger-api: Kill legacy identifier #2211

ledger-api: Kill legacy identifier #2211

remyhaemmerle-da commented Jul 18, 2019 •

edited

Loading

stefanobaghino-da left a comment

gerolf-da left a comment

S11001001 Jul 18, 2019 •

edited

Loading

remyhaemmerle-da Jul 23, 2019

S11001001 Jul 23, 2019

S11001001 Jul 18, 2019

stefanobaghino-da Jul 19, 2019 •

edited

Loading

S11001001 Jul 19, 2019 •

edited

Loading

stefanobaghino-da Jul 22, 2019

S11001001 Jul 23, 2019

remyhaemmerle-da Jul 23, 2019 •

edited

Loading

nickchapman-da left a comment

gerolf-da Jul 23, 2019

remyhaemmerle-da Jul 23, 2019

gerolf-da Jul 23, 2019

remyhaemmerle-da Jul 23, 2019

	def unapply[Tpl](t: TemplateId[Tpl]): Option[(String, String, String)]
	def unapply[Tpl](t: TemplateId[Tpl]): Some[(String, String, String)]

ledger-api: Kill legacy identifier #2211

ledger-api: Kill legacy identifier #2211

Conversation

remyhaemmerle-da commented Jul 18, 2019 • edited Loading

Pull Request Checklist

stefanobaghino-da left a comment

Choose a reason for hiding this comment

gerolf-da left a comment

Choose a reason for hiding this comment

S11001001 Jul 18, 2019 • edited Loading

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

stefanobaghino-da Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

S11001001 Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remyhaemmerle-da Jul 23, 2019 • edited Loading

Choose a reason for hiding this comment

nickchapman-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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remyhaemmerle-da commented Jul 18, 2019 •

edited

Loading

S11001001 Jul 18, 2019 •

edited

Loading

stefanobaghino-da Jul 19, 2019 •

edited

Loading

S11001001 Jul 19, 2019 •

edited

Loading

remyhaemmerle-da Jul 23, 2019 •

edited

Loading