-
Notifications
You must be signed in to change notification settings - Fork 6
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.
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
python: Next-gen dazl connection API #108
Conversation
31555ad
to
0d3311c
Compare
ce68dbe
to
691a578
Compare
ab2fec6
to
8d86132
Compare
8d86132
to
add9018
Compare
3f93460
to
076737d
Compare
631e509
to
4a439b4
Compare
b11f6b2
to
e5ee640
Compare
41c1cde
to
2dcc5a6
Compare
e46714c
to
cb97132
Compare
150fab6
to
41b30b3
Compare
41b30b3
to
391d920
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor comments, but generally this looks very good and I am looking forward to using it.
Given the scope of this change, my suggestion (which may have actually come from you yourself) is to release the as a beta v8, let us get some time with it over the next couple of weeks, and then cut the official v8 once we're sure it works the way we want.
""" | ||
if not self._config.access.ledger_id: | ||
# most calls require a ledger ID; if it wasn't supplied as part of our token or we were | ||
# never given a token in the first place, fetch the ledger ID from the destination |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth letting the user know that this discovery is occurring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya good point—will add some INFO
logs here.
python/dazl/ledger/grpc/conn_aio.py
Outdated
if isinstance(self._config.access, PropertyBasedAccessConfig): | ||
self._config.access.ledger_id = response.ledger_id | ||
else: | ||
raise ValueError("token-based access must supply tokens that provide ledger ID") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is not strictly true, is it? (The immediately preceding lines of code are a mechanism by which the access token is NOT required to have a ledger ID.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rephrased this to perhaps to make it clearer, but the story is nuanced:
- For property-based access on the gRPC Ledger API, the ledger ID can be queried. This is how
dazl
works today, and why you don't really ever need to specify ledger IDs - For property-based access on the HTTP JSON API,
dazl
mints its own unsigned tokens, because even in an un-authed configuration, clients still identify themselves to the server by using tokens. Here, a ledger ID cannot be inferred, so it must be specified. - For token-based access on either gRPC Ledger API or HTTP JSON API, the ledger ID is "part of the spec", so it's required for the token to be considered valid. (And because the token is generally signed, the client can't modify specific claims in the token, particularly ledger ID).
python/dazl/ledger/grpc/conn_aio.py
Outdated
if workflow_id: | ||
# TODO: workflow_id must be a LedgerString; we could enforce some minimal validation | ||
# here to make for a more obvious error than failing on the server-side | ||
return workflow_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like bad things might happen if not workflow_id
... does this just turn into a Nothing
.
|
||
try: | ||
offset = None | ||
async for event in self._acs_events(tx_filter_pb): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noice.
# now start returning events as they come off the transaction stream; note this | ||
# stream will never naturally close, so it's on the caller to call close() or to | ||
# otherwise exit our current context | ||
async for event in self._tx_events(tx_filter_pb, offset): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this look like in the event of a failure? If the input stream goes down, I'm assuming an exception is thrown that the caller can catch and recover, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup exactly! This is one of the things I'm most excited about with respect to the new API— every interaction with the ledger provides an obvious place to wrap with try
/except
, whereas the more callback-heavy approach made this much more difficult/impossible.
tx_filter_pb = G_TransactionFilter(filters_by_party=filters_by_party) | ||
|
||
try: | ||
offset = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth a comment that _acs_events
always emits a boundary with an offset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point—shall add.
warnings.warn(f"Received an unknown event: {event}", ProtocolWarning) | ||
yield event | ||
|
||
if self._continue_stream: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple things:
- If
offset
isNone
here, then something is wrong, no? Does Python have an assert thing that can enforce this invariant? - If the ledger API doesn't guarantee to always be able to produce archived contracts, how long is
offset
guaranteed to be a valid input for_tx_events
? I'm assuming long enough that we don't have to worry here, but not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the first point, not necessarily—offset is None
means that you're connecting to an empty ledger and are interested in hearing create/archive events going forward.
On the second point, I'm not sure…but there isn't much we can do about it from a client-side perspective.
``dazl`` has never had a straightforward way of abandoning event callbacks that were no longer | ||
needed. The new API makes stream lifecycle more explicit and the responsibility of the user of the | ||
library. Disposing of streams is now simpler to reason about. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
391d920
to
420bccd
Compare
420bccd
to
040b3b4
Compare
dazl v8 API
Introduces a new API that embraces multi-party subscriptions, and modernizes and simplifies the implementation.
Fixes
dazl
turns four years old in May 2021, and many of the design decisions that it was originally built with (even predating the Daml gRPC Ledger API as introduced in October 2018) no longer apply:No longer consolidates to a single transaction stream per
Party
SubmitAndWait
: older versions of the ledger API only had aCommandSubmissionService.Submit
call, which required clients to listen toTransactionStream
events in order to know whether or not a command succeeded or not. Establishing independent transaction streams per command submission is obviously an unscalable approach, sodazl
instead listens to the transaction stream and doles out completion events internally.ActiveContractSetService
didn't originally exist, sodazl
has a tradition of reading from the transaction stream, even when it doesn't necessarily need to. This was partially remediated in python: Make initial data reads come from the ACS #10/python: Whoops—actually turn on the ACS fetching for real. #16, butdazl
is still very much predicated on reading from the transaction stream, and incorporation of theActiveContractSetService
is somewhat incomplete. By virtue of the single-stream-per-Party
design, the ACS is also similarly one-per-Party
.dazl
makes applications generally pay for maintaining an ACS nonetheless.dazl
is only asynchronous, correlating exercise results back to call sites was very challenging.dazl
's singular transaction stream-per-Party
model from being workable.gRPC and
asyncio
asyncio
support (L58: Async API for gRPC Python grpc/proposal#155)! This removes the need for the complicated internals indazl
devoted to trying to keep Python thread count under control. Every transaction stream subscription required at least three Python threads to maintain, and this was another reason thatdazl
tried to "conserve" transaction subscriptions.HTTP JSON API support/TypeScript library parity
dazl
currently exposes a library, and it should be possible for an application written againstdazl
to talk to either API.dazl
's; with some minor symbol renames, the APIs are effectively identical.This does not yet include the compatibility layer to help ease the transition from the old API to the new one.
Some new low-level utility libraries have been added: see #172, #175, #176, #179
Due to the size of the change, some long-deprecated APIs were dropped as a prerequisite to this work: #155, #159