-
Notifications
You must be signed in to change notification settings - Fork 88
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
ADR: Resource based API #1028
ADR: Resource based API #1028
Conversation
Transactions CostsSizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using
Script summary
Cost of Init Transaction
Cost of Commit TransactionThis is using ada-only outputs for better comparability.
Cost of CollectCom Transaction
Cost of Close Transaction
Cost of Contest Transaction
Cost of Abort TransactionSome variation because of random mixture of still initial and already committed outputs.
Cost of FanOut TransactionInvolves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.
End-To-End Benchmark ResultsThis page is intended to collect the latest end-to-end benchmarks results produced by Hydra's Continuous Integration system from the latest Please take those results with a grain of salt as they are currently produced from very limited cloud VMs and not controlled hardware. Instead of focusing on the absolute results, the emphasis should be on relative results, eg. how the timings for a scenario evolve as the code changes. Generated at 2023-09-06 13:45:30.118655386 UTC 3-nodes ScenarioA rather typical setup, with 3 nodes forming a Hydra head.
Baseline ScenarioThis scenario represents a minimal case and as such is a good baseline against which to assess the overhead introduced by more complex setups. There is a single hydra-node d with a single client submitting single input and single output transactions with a constant UTxO set of 1.
|
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.
All in all very clear proposal. I approve in draft status.
Please take a look at the questions in my comments.
| Path | GET | POST | PATCH | DELETE | | ||
| :-------------------------- | :-------------------------- | :--------------------- | ------- | :----------------- | | ||
| `/node/head/status` | `HeadStatus(..)` | - | - | - | | ||
| `/node/head/utxo` | confirmed utxo | - | - | - | |
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'm guessing utxo is meant for utxo set here.
If I'm correct, what would mean confirmed utxo? Is it for the utxo that are included in a snapshot?
Also, I've noticed we currently have some issues with the clients only having access to the utxo included in a snapshot (see #612 ). I feel like /node/head/utxo
, if containing the set of utxo included in a snapshot would exhibit the same problem and I would like us to solve it before. In my opinion, the client should, by default, only access the latest view of its node ledger when constructing a transaction. Whether or not these utxo have been included in a snapshot should not be considered at this step.
Last, but not least, we can currently have issues when the utxo set is very big and the snapshot confirmed message is heavy and slows down the node. That led some users to ask for utxo query primitives instead of getting the full utxo set. I'm mentioning this, although I'm not sure we want to address it right now as having a huge utxo set would make it impossible to close the head with the current implementation anyway.
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 agree: We should probably already provide sub-paths for individual UTxO and we already have had requests for more powerful UTxO query capabilities (eg. #797)
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 points.
We usually mean any number of unspent transaction outputs when we just say utxo
or UTxO
. This is also how it is documented right now in the api reference and glossary IIRC.
I commented below that we likely need both, local and confirmed versions of utxo and transactions.
I will add a section on how the querying of such big resources could be improved using content types and query parameters (and maybe even pagination?). But maybe I also don't want to take a decision on that in this ADR (it's rather a consequence) and ultimately we might not be providing for this and suggest indexers instead?
| :-------------------------- | :-------------------------- | :--------------------- | ------- | :----------------- | | ||
| `/node/head/status` | `HeadStatus(..)` | - | - | - | | ||
| `/node/head/utxo` | confirmed utxo | - | - | - | | ||
| `/node/head/transactions` | confirmed txs | `NewTx` + responses | - | - | |
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.
Same question: does confirmed txs mean they have been included in a snapshot?
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.
Yes, that is what I meant. I had Snapshot{confirmed}
here before. Maybe that would be more clear?
| `/node/head/status` | `HeadStatus(..)` | - | - | - | | ||
| `/node/head/utxo` | confirmed utxo | - | - | - | | ||
| `/node/head/transactions` | confirmed txs | `NewTx` + responses | - | - | | ||
| `/node/head/ledger` | `localUTxO` and `localTxs` | - | - | - | |
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.
Now I think I understand the following:
/node/head/utxo
-> utxo set of the last known validated snapshot/node/head/transactions
-> transactions included in the last known validated snapshot (transitively)/node/head/ledger
-> utxo set and transactions of the local ledger, irrespective of the snapshots
I'm not sure we want to expose so much details to the customer. This notion of local transactions and local utxo set feels very internal to the Hydra protocol specification/implementation.
I think, as a client of Hydra, that I want to be able to :
- init/open/close/contest/fanout a head
- build new L2 transactions
- know about the finality of a transactions on L2
For 1. I need the most recent view of the ledger from my node. Otherwise, I could build locally invalid transactions (see #612 ) so /node/head/ledger
would be enough, although I would name it differently.
For 2. I need the information that a given transaction has been included in a signed snapshot and, for that, I guess I would want to have an entry point to get the status of the transaction itself and it would inform me about the fact that it's pending and snapshotted.
That would lead to the following entry points:
/node/head/utxo
-> utxo set known by my local node/node/head/transactions
-> transactions known by my local node-
/node/head/transactions/<id>
-> informations about transaction of id id, can be used to check for L2 finality of a transaction
/node/head/ledger
-> removed
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 would say exactly the other way around?
For 1) you would care about which UTxO are available on the L2 after opening and which would be available if we close (any non confirmed utxo would not be included!)
For 2) you would need the most recent local view of the UTxO set, such that your node accepts transactions moving this forward.
For 3) we can do this individual query as you propose, but I would advise against it as it does not allow us to setup a subscription for all confirmed L2 transactions, which is a use case we know exists.As you might
So we are in the situation where likely need both, the local view and the confirmed view. I will try to make that separation more clear in the example resources and also should make it clear that the ADR is not primarily about specifying the full API, but more to exercise the concept of mapping endpoints to existing semantics and see whether we could address the requirements from the context.
binary (`cbor`) output of `transaction` fields in several of these using a | ||
query parameter. | ||
|
||
- Many of these features have been added in a "quick and dirty" way, by monkey |
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.
Does this adr assume also changing the "quick and dirty" way of doing things?
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.
Yes. I added some notes on how content types and query parameters can lead to a fine grained control and more standard way of configuring responses.
into topics on the API layer. | ||
|
||
- Each resource has a _latest_ state, which can be queried or clients may | ||
subscribe to any model changes. |
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 would separate that in 2 statements to make it clearer there are 2 different things, eg. :
- Clients can synchronously query the latest state of each resource
- Clients can subscribe to be notified of state changes for each resource
Note this definitely looks very similar to what GraphQL provides :)
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.
Are subscriptions in scope of standard graphql? Couldn't see it in most implementations and only apollo supports that.
It's quite inspired by graphql and maybe just doing that is simpler? Happy to discuss!
subscribe to any model changes. | ||
|
||
- A resource's `model` type needs to be a result of a pure `projection` from | ||
server output events, i.e. `project :: model -> ServerOutput -> model`. |
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 is not clear to me: How can a type
be the result of projection
? Do you mean that each resource's value is the result of accumulating (folding? reducing?) some sequence of ServerOutput
s?
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.
Yes. Updated the wording on this one. Hope it's more clear. Not sure though if projection is the right word here. I also like "reduce" as it may be familiar with most people from the frontend world of redux state management systems.
|
||
| Path | GET | POST | PATCH | DELETE | | ||
| :-------------------------- | :-------------------------- | :--------------------- | ------- | :----------------- | | ||
| `/node/head/status` | `HeadStatus(..)` | - | - | - | |
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.
why have /node
as the first item in the path?
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. The idea here really was to make sub-paths always included in the parent. /node
would contain all the remaining data available at /node/*
endpoints. There is also the intention to keep /
with the same semantics as before.
Shall we
- Make it an exception to the rule and say
/
will contain not everything in/*
, but the original behavior of accepting a websocket upgrade with the same messages as before? or - Rename
/node
to/v1
and make/v1/*
include all the data from sub-paths to make it more consistent?
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 opted for doing 2) and would love your feedback again.
| Path | GET | POST | PATCH | DELETE | | ||
| :-------------------------- | :-------------------------- | :--------------------- | ------- | :----------------- | | ||
| `/node/head/status` | `HeadStatus(..)` | - | - | - | | ||
| `/node/head/utxo` | confirmed utxo | - | - | - | |
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 agree: We should probably already provide sub-paths for individual UTxO and we already have had requests for more powerful UTxO query capabilities (eg. #797)
c018af5
to
b7d4a5e
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.
I love it, thanks for considering my suggestions and incorporating them in such a thorough way. I have only added minor comments which can definitely be ignored.
to know the latest UTxO set for constructing transactions. | ||
|
||
- Inclusion of the whole UTxO set in the head is not always desirable and | ||
filtering by address would be beneficial. (not addressed in this ADR though) |
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.
Perhaps just mention adding querying capabilities in general and point to the relevant discussion (#797)?
filtering by address would be beneficial. (not addressed in this ADR though) | ||
|
||
- As [ADR-15](/adr/15) also proposes, some clients may not need (or should | ||
not have) access to administrative information. |
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.
Mention this won't be addressed in this ADR either? Or is it?
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.
Indirectly. There is a consequence (number 4) which mentions this again.
not have) access to administrative information. | ||
|
||
- It is often a good idea to separate the responsibilities of Commands and | ||
Queries (CQRS), as well as the model they use. |
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.
Add link to https://martinfowler.com/bliki/CQRS.html ?
## Decision | ||
|
||
- Drop `GetUTxO` and `GetUTxOResponse` messages as they advocate a | ||
request/response way of querying. |
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.
🎉
connection, push the _latest_ state as first message and any resource | ||
state updates after. | ||
|
||
- Other HTTP verbs may be accepted by a resource handler, i.e. to issue |
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.
HEAD
or OPTIONS
are not really about commands and the former might be useful in general to eg. retrieve the size of a response.
via the corresponding websocket connection. | ||
|
||
- `Accept` request headers can be used to configure the `Content-Type` of the | ||
response |
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.
❤️
| `/v1/` | all `/v1/*` data | - | - | - | | ||
|
||
Multiple heads are out of scope now and hence paths are not including a | ||
`<headId>` variable section. |
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.
As this is an ADR, and not an implementation plan, I would like to suggest we include the headId
in the path to queries retrieving the state of a particular head. I think multiple heads are not that far down the road, this does not add much complexity, and seems to be more consistent with traditional practices.
I am fine with leaving it as it is if you think it's not worth the hassle.
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'd like to keep it out for now.
|
||
- Versioned API allows clients to detect incompatibility easily. | ||
|
||
- Need to rewrite how the `hydra-tui` is implemented. |
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.
... or implement a hydra-web-ui
;) ?
This should make it more clear that content-types and query parameters should be used to negotiate response structure on a per-resource basis
The included example resources should only be seen as that. Not all details about separating clearly between local and confirmed utxo/txs are resolved in this ADR.
b7d4a5e
to
19c936c
Compare
An approach to compartmentalize the API into smaller parts to make them available for a wider range of use cases.
Inspired by previous experience on using HTTP methods for commands and HTTP upgraded connections for subscriptions.
Also very similar to GraphQL.