-
Notifications
You must be signed in to change notification settings - Fork 87
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
Ability to read protocol parameters via REST #989
Conversation
facfd9c
to
0bc6793
Compare
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.
|
Test Results335 tests - 12 330 ✔️ - 12 19m 38s ⏱️ +39s Results for commit a1c3aca. ± Comparison against base commit 730b8e8. This pull request removes 13 and adds 1 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
163ae71
to
3155e65
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.
Rather than using directly the pparams from the file, I would extract them from the ledger which would then make it trivial to support the use case where people want to update the PParams inside the Head. Seems a bit contrived, I know, but I can imagine cases where people would like to have a fees evolving in the head for example, or perhaps change the cost model, or something else.
When you say extract it from the ledger do you mean to update the |
Yes, something like that. I now realise this might be tricky because we use a |
@abailly-iohk I just re-realized that the |
So perhaps we should just bite the bullet, drop |
557ceb2
to
a1c3aca
Compare
|
||
apiServerSpec | ||
|
||
-- REVIEW: we should add more tests for other routes here (eg. /commit) |
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. But what do you want to get reviewed here when you already know what we should do? IMO you could make this a TODO and provide a rationale in the PR.. or just do 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.
I promoted it to a todo. I'll do it in a separate PR since /commit
is a bit more involved then /protocol-parameters
- Also add the new route in the http server to make the test green
- Pass protocol params to the api code to be returned in the endpoint - Match expected protocol parameters in the test
- Problem here is that async api schema is not really suitable for REST endpoints. The second problem is what to give as an example for protocol params? Protocol parameters file is pretty big and I am not sure it brings value to the users to see a big json output in the docs?
Co-authored-by: Sebastian Nagel <ch1bo@users.noreply.github.com>
Co-authored-by: Sebastian Nagel <ch1bo@users.noreply.github.com>
fa594e2
to
924e0eb
Compare
fix #735
Why
Our users seem to have a need to obtain protocol parameters from the REST endpoint.