add multi-{key,query} stream to daml-ledger

[garyverhaegen-da]

This is an attempt to address #7034 & #7036. I'd like to gather feedback
before moving further, though, because while I believe (based on limited
problem domain knowledge) that this is the rigth way to address them,
strictly speaking this does not fulfill their stated acceptance
criteria.

In particular, I have chosen to keep limiting both scopes to a single
template ID. Ignoring the fact that I'm having trouble reasoning about
the type of the resulting stream if we accept arbitrary lists of
template IDs in these calls, I'm also questioning whether we want to
encourage our users to build their applications on top of such
dynamically heterogeneous lists.

CHANGELOG_BEGIN

deprecation warning & upgrade instructions to be fleshed out

CHANGELOG_END

[cocreature]

Thanks for looking into this! I fully agree that limiting it to one template type is sensible.

I’ve left two inline comments on useStreamQueries and useStreamFetchByKeys.
Given that they are somewhat different, it might be worth considering to split this into two PRs.
We should also update the react hooks to include this but I’m perfectly happy to do this in a separate PR.

[leo-da]

I have chosen to keep limiting both scopes to a single
template ID. Ignoring the fact that I'm having trouble reasoning about
the type of the resulting stream if we accept arbitrary lists of
template IDs in these calls, I'm also questioning whether we want to
encourage our users to build their applications on top of such
dynamically heterogeneous lists.

@hurryabit had a use case for querying two or more versions of the same template (which would be represented as totally different types):

{"templateIds": ["Iou:Iou", "Iou:Iou2", "Iou:Iou3"]}

Don't know if this is still the case and if we still want to support such queries in the future (only JSON API currently supports such queries, because of its untyped nature). I agree with you @garyverhaegen-da , it does not make much sense to me to allow returning totally unrelated types as part of the same stream.

[cocreature]

Supporting this would significantly impact all the streaming APIs we currently have since the stream type is fixed to one template. I can see some argument for supporting this for non-streaming endpoints where this seems more reasonable (ignoring the fact that I can only supply one query there for now). For now, I’d like to go with the approach here until we get a concrete demand not satisfied by this.

[leo-da]

Supporting this would significantly impact all the streaming APIs we currently have since the stream type is fixed to one template....

I know, I know... I never liked the idea in the beginning. Just saying that there was a use case and @hurryabit really wanted it. So I assumed he still had plans to implement it.

I am happy to forbid such queries on the TypeScript side.

[garyverhaegen-da]

I would expect that in most real scenarios the types are fixed at compile time, whereas I can image scenarios where the queries are dynamically generated (or at least the set of queries is dynamically generated from a superset of predefined ones).

From that perspective, I think the right approach here is to keep a single template type on these methods, but add stream combining functions. So if one wants two different templates in the same stream, they would do something like:

const s1: Stream<Template1, ...> = queryStream(template1, ...);
const s2: Stream<Template2, ...> = queryStream(template2, ...);
const merged: Stream<Template1 | Template2, ...> = merge(s1, s2, stateMergeFn);

So types would be known at compile-time, rather than being a dynamic list, and code handling that merged stream could still be usefully type-checked and reasoned about.

[cocreature]

TypeScript does have variadic tuple types which might make this typesafe even for a single method microsoft/TypeScript#39094 but I expect it’ll be a rather complex API so even if it does work, I’m not sure we want to use it. The downside of something like merge is that you do the merging client side which is less efficient (you end up with 2 streams to the json api and two transaction streams from the json api to the ledger). But I want to see actual numbers on that where this is an issue before we worry about this.

[cocreature]

You could in principle imagine an API that transparently routes individual queries to a single one for performance reasons which I think would probably best from an API view (you don’t have to deal with hetereogenous streams) and performance view (one stream to the json API and one to the ledger) but again, I’d like to see this become an issue before we invest resources into this.

[hurryabit]

The suggested new API considers only the case where you want to send multiple queries for the same template. There's also the case where you want to send multiple queries for different templates. I think this is something we should take into account while designing the API, even if we don't implement it in this PR.

I would also suggest to not deprecate streamQuery but rather keep it around, at least as a convenience wrapper, since it is the common case and also since "query" is one of those words whose plural is a bit more complicated and searching for a function containing "query" will not yield the "queries" function.

The same goes for streamFetchByKey, except for the "complicated plural" argument.

[cocreature]

I would also suggest to not deprecate streamQuery but rather keep it around, at least as a convenience wrapper, since it is the common case and also since "query" is one of those words whose plural is a bit more complicated and searching for a function containing "query" will not yield the "queries" function.

The same goes for streamFetchByKey, except for the "complicated plural" argument.

Yes we agreed on that separately.

As for multiple templates, I’m not convinced that’s a usecase we should support. It makes the whole API significantly more complex since the whole stream type needs changing and I haven’t seen any requests for this.

[garyverhaegen-da]

Under these assumptions:

• We do want to keep a single template type per string.
• We want to keep the existing (singular) functions, and not deprecate them.
• We want to share implementation code between singular and plural versions.
• While we don't deprecate them yet, we do have an eye towards removing them so the singular versions are no longer used by anything in this repo (except their tests, of course).

this PR is ready for review. @hurryabit @cocreature I think it makes more sense to discuss the assumptions before sinking time into reviewing the code, as it looks like we're not fully aligned there.

[hurryabit]

• While we don't deprecate them yet, we do have an eye towards removing them so the singular versions are no longer used by anything in this repo (except their tests, of course).

That's not in the spirit of what I meant. I would suggest to keep actively promoting the use of those functions in the GSG. I think using the more general functions there adds unnecessary complexity to beginners.

[garyverhaegen-da]

That's not in the spirit of what I meant.

I know. You're coming to this a bit late so, as the work is now complete, I thought it would be useful to explicitly list the assumptions under which I did it, in the hope that it will be more efficient to discuss those directly than to discuss the code itself while each holding out to our separate set of assumptions.

[cocreature]

1. Supporting multiple queries per template makes sense from a UX perspective. Supporting multiple templates imho only makes sense from a performance perspective. I’m not convinced that this is actually an issue (we clearly should have numbers for this first) and we did not get any requests for this (we did get requests for multiple filters). On the other hand, it requires significant API changes that either complicate the API or lose type info (e.g. just degrade to a stream of templates without trying to represent the list of possible templates). So I very strongly feel that we should tackle this without mixing in multiple template types.
2. Yes keep the singular functions, don’t deprecate anything.
3. Yes share the implementation where feasible and reasonable.
4. The GSG only uses the react bindings which are not affected by this PR so regardless of what we do they are not affected. I’m not aware of any examples that use @daml/ledger apart from the admin tool in DAVL which is hardly branded as a tutorial or documentation. so it seems a bit pointless to argue about what we advertise. We do need to have that conversation once we change react hooks and I’m open to sticking to the singular version but without seeing an API proposal for that, I don’t think this is a productive conversation.

[cocreature]

Great work, thank you very much!

I’d like to see some integration tests in build-and-lint that actually run against the JSON API before merging.

[cocreature]

Do we still have tests for the singular version?

[cocreature]

Maybe worth factoring this out into a helper function that compares LF values for equality? It’s probably also worth pointing out that the JSON representation accepted by the JSON API is not unique (e.g. records can also be represented as an array). The reason this works anyway is that the typescript type system enforces that users use the same representation when creating a query that will be returned by the JSON API.

[garyverhaegen-da]

I'm not quite sure what you want me to do here. It seems pointless to extract the method if it's just to write keyEqual = (k1, k2) => deepEqual(k1, k2, {strict: true} and then use it only once.

I could see a point in extracting it if we were using it in multiple places, or if we wanted to support more specifically LF equality across representations, but if the type system is enforcing a common one, what's the value in that?

[cocreature]

This was mostly just an idle thought. I briefly wondered if deepEqual was wrong and we need to worry about non-unique representation allowed by the JSON API. If this was just lfValueEqual or something like that, the reader doesn’t have to wonder if deepEqual is sufficient and we could add a comment to lfValueEqual (or whatever you want to call that) explaining why deepEqual is the right implementation.

Feel free to ignore my suggestion and follow your preference here.

[garyverhaegen-da]

I do not know how to translate the JSON representation equivalence rules in a generic context. So I went for documenting that the given keys must be in "output" format instead, as that one is unique.

[garyverhaegen-da]

Actually with the current implementation this is a breaking change for the streamFetchByKey function. So I'll revert that to its original form.

[cocreature]

You mean when called from JS where you can use a different format? I don’t think we should support a JS API that would produce type errors in typescript. It might work occasionally by accident but at least half of our code relies heavily on the assumption that you use the format dictated by the typescript code.

[garyverhaegen-da]

I disagree. We advertize these bindings as JS bindings, not TS ones. We document the LF to JSON conversion as supporting multiple JSON formats for the same LF. Therefore existing JS code that uses a valid input encoding that differs from the output one is valid, and breaking that code is a breaking change.

Plus the cost of keeping that code working is really low here.

[cocreature]

Makes sense here. In general, I’m a bit hesitant to make that guarantee since we don’t test this anywhere and I wouldn’t be surprised if in various places, we do make assumptions about the format.

[garyverhaegen-da]

Yes; this is why I firmly believe the right way to offer backwards compatibility is to not change existing code.

[garyverhaegen-da]

I think I finally got everything working. I'm reproducing the new commit message here; note in particular the deprecation cycle:

This is an attempt to address #7034 & #7036. Strictly speaking, this does not match their acceptance criteria, as this only supports multiple queries, not mixed templates.

Because the two new functions can cover the exact same use cases (and more) as the existing streamQuery and streamFetchByKey, the latter are deprecated. The deprecation cycle I suggest is to deprecate them immediately by annotating them as such in the documentation (done on this PR).

That's it. I do not think we ever need to actually remove them, nor to make them print annoying warnings or anything. There is nothing wrong with the functions as they stand, they just don't fit in the API anymore.

We could, at some point, move them to a separate documentation page, or to the bottom of the existing one, but I feel even removing them from the documentation is unnecessary.

CHANGELOG_BEGIN

• [JavaScript Client Libraries] Two new methods have been added to daml-ledger: streamQueries and streamFetchByKeys. They are similar to the existing singular versions, except they can take multiple queries and multiple keys, respectively, and return a union of what the corresponding individual queries/keys would have. Because these new functions can do everything the existing ones can, we are deprecating the latter, though there is no plan to remove them.

  Upgrade path is straightforward:

  streamQuery(t); => streamQueries(t, []);
  streamQuery(t, undefined); => streamQueries(t, []);
  streamQuery(t, q); => streamQueries(t, [q]);
  streamFetchByKey(t, k); => streamFetchByKey(t, [k]);
  

  There is one caveat, though: streamFetchByKeys is a little bit less lenient in the format in which it expects the key. If your existing code was conforming to the generated TypeScript code we provide, everything should keep working, but if you were using plain JS or bypassing the TS type system, it is possible that you used to construct keys that will no longer be accepted. The new function requires all keys to be given in the output format of the JSON API, which is a little bit more strict than the general JSON <-> LF conversion rules.

CHANGELOG_END

[cocreature]

Looks great, a few final comments.

I thought a bit more about support for multiple templates (I still think this is an orthogonal issue and not something that affects this PR): There is a reason outside of performance: transactionality/atomicity. With a multi-template query you get a consistent ACS snapshot. If you run multiple queries and then merge on the client side you no longer have a consistent snapshot (or at least not unless you invest a significant amount of effort to look at offsets and I’m not sure how to actually make that work in practice (for the JSON API, over the ledger API it’s a bit easier)).
I would argue that consistent ACS snapshots are primarily important for automation not for UIs which isn’t really what we are focused on at the moment for the JS bindings but it’s still worth keeping this in mind. If we do want to support this, I think a separate API for multi-template queries and multi-template streams is probably a better approach than trying to beat this into the existing APIs.

[cocreature]

Makes sense here. In general, I’m a bit hesitant to make that guarantee since we don’t test this anywhere and I wouldn’t be surprised if in various places, we do make assumptions about the format.

[cocreature]

Is this a bugfix? If so what exactly is it fixing, we already seem to emit something very similar in onClose.

[garyverhaegen-da]

This may be worth exploring as a separate PR. The notion of close on Stream seems a bit confused at the moment. As the code stands (without this PR), if you register a listener for 'close' events, you get notified when the underlying WebSocket closes (even though the stream will then try to open a new one and, if it manages to reconnect, will keep sending 'change' events), and you do not get notified when the stream itself gets closed (by calling .close() on it), because the code in .close() removes the listeners before closing the WebSocket connection (and trying to send a message to 'close' listeners). Further, calling .close() also does not really close the stream: while it does remove all the listeners and close the WebSocket connection, it also immediately creates a new WebSocket connection to replace the closed one, as it does not remove the onClose listener on the WebSocket itself.

I believe the following would be a better interface, but am a bit unsure about how to change this without breaking backwards compatibility:

• WebSocket connection closing is an internal detail and should not leak out (assuming we do manage to reconnect).
• Listeners for 'close' events should trigger when the stream will no longer emit events: either .close() has been called, or the WebSocket connection has failed beyond the code's willingness to try reconnecting.
• When .close() is called, the WebSocket connection should be properly terminated (i.e. no reconnection).
• Calling .close() on an already closed stream can be defined as either an error or a no-op, depending on how forgiving we want to be (I'd prefer an error).

As of this PR, 'close' listeners get called when .close() is called, but none of the other issues are addressed. Consequently, the 'close' event is still in the WebSocket 'close' event format. I chose the "normal termination" code of 1000 as it seemed semantically correct here (described as "the connection has reached its natural end after fulfilling its purpose").

[garyverhaegen-da]

I have removed this change and will open a separate PR for this. This on is big enough as it stands.

[garyverhaegen-da]

Tests are failing about 4/10 times without this. With it I got 40 out of 40 successes. I'm not super happy about it, but this being JS, I don't know of any better way to give the collect callback a chance to run.

[cocreature]

Can we make a promise out of the callback and await that?

[garyverhaegen-da]

I suppose we could replace the current collect mechanism with a multi-delivery promise, and then explicitly await each expected message, rather than handling the whole stream as an array.

Syntactically you'd have a few more method calls and await expressions in exchange for this one sleep. In terms of runtime behaviour, I believe the test would time out instead of failing, which means that it would be slower for non-timing-related failures and that it will be much more lenient if things get slow.

I'll let you decide if you think the tradeoff is worth it; I have a small preference for the state it's currently in, but it's not a massive change and I'm willing to open a PR for it if you think that's better.

[cocreature]

Let’s leave it and if it starts flaking we can reconsider.