Skip to content
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

Merged
merged 10 commits into from
Jul 26, 2023

Conversation

v0d1ch
Copy link
Contributor

@v0d1ch v0d1ch commented Jul 20, 2023

fix #735

Why

Our users seem to have a need to obtain protocol parameters from the REST endpoint.


  • CHANGELOG updated or not needed
  • Documentation updated or not needed
  • Haddocks updated or not needed
  • No new TODOs introduced or explained herafter

@v0d1ch v0d1ch force-pushed the endpoint-to-return-protocol-params branch from facfd9c to 0bc6793 Compare July 20, 2023 14:33
@github-actions
Copy link

github-actions bot commented Jul 20, 2023

Transactions Costs

Sizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using arbitrary values and results are not fully deterministic and comparable to previous runs.

Metadata
Generated at 2023-07-26 09:52:35.902959916 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial 7ceb53f05e444cfdabfd0a37a0590090066da457a1f1db30d613b8bd 4289
νCommit 70e70fc13217bfde96932956656c1d540a743b1588c845ca09dc3723 2124
νHead cda51d313c1c8285b6925ce2413def012db27f544e2bbd79b8173000 9185
μHead 1c0b665fc49bc2e9e2ce4e8252c8f37fe84dd75bd8e086abfdb92685* 4149
  • The minting policy hash is only usable for comparison. As the script is parameterized, the actual script is unique per Head.

Cost of Init Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 4740 14.24 5.57 0.51
2 4948 17.69 6.90 0.56
3 5152 17.75 6.86 0.57
5 5562 21.94 8.42 0.63
10 6585 36.15 13.83 0.83
37 12125 99.91 37.82 1.76

Cost of Commit Transaction

This is using ada-only outputs for better comparability.

UTxO Tx size % max Mem % max CPU Min fee ₳
1 599 14.97 5.73 0.34
2 784 19.75 7.76 0.40
3 969 24.85 9.91 0.47
5 1351 36.28 14.63 0.61
10 2279 71.91 28.91 1.04
13 2846 98.33 39.25 1.35

Cost of CollectCom Transaction

Parties UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
1 57 814 27.79 10.78 0.49
2 113 1134 43.86 17.14 0.68
3 171 1456 61.71 24.27 0.89
4 227 1777 82.85 32.75 1.13

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 648 18.98 8.46 0.39
2 804 20.08 9.60 0.42
3 966 21.78 10.97 0.45
5 1300 24.27 13.37 0.50
10 2132 31.57 19.78 0.64
50 5432 65.66 29.68 1.13

Cost of Contest Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 683 24.44 10.50 0.45
2 841 26.16 11.87 0.48
3 998 27.88 13.24 0.51
5 1336 31.75 16.15 0.58
10 2162 40.34 23.01 0.74
44 7763 98.80 69.64 1.79

Cost of Abort Transaction

Some variation because of random mixture of still initial and already committed outputs.

Parties Tx size % max Mem % max CPU Min fee ₳
1 4855 22.60 9.44 0.61
2 5177 37.05 15.68 0.79
3 5498 54.06 23.07 1.00
4 5817 74.35 31.88 1.24
5 6137 97.72 42.03 1.51

Cost of FanOut Transaction

Involves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.

Parties UTxO UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
5 0 0 4765 8.72 3.59 0.46
5 1 57 4806 10.12 4.41 0.48
5 5 284 4941 15.71 7.72 0.55
5 10 569 5124 22.69 11.85 0.64
5 20 1137 5483 36.67 20.11 0.83
5 30 1710 5852 50.65 28.38 1.02
5 40 2276 6203 64.63 36.65 1.21
5 50 2843 6561 78.62 44.92 1.40
5 65 3697 7103 99.62 57.34 1.68

@github-actions
Copy link

github-actions bot commented Jul 20, 2023

Test Results

335 tests   - 12   330 ✔️  - 12   19m 38s ⏱️ +39s
112 suites  -   3       5 💤 ±  0 
    5 files    -   1       0 ±  0 

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.
Hydra.TUI.Options ‑ no arguments yield default options
Hydra.TUI.Options ‑ parses --cardano-signing-key option
Hydra.TUI.Options ‑ parses --connect option
Hydra.TUI.Options ‑ parses --node-socket option
Hydra.TUI.Options ‑ parses --testnet-magic option
Hydra.TUI.Options ‑ parses --version option
Hydra.TUI/end-to-end smoke tests ‑ display feedback long enough
Hydra.TUI/end-to-end smoke tests ‑ doesn't allow multiple initializations
Hydra.TUI/end-to-end smoke tests ‑ starts & renders
Hydra.TUI/end-to-end smoke tests ‑ supports the full Head life cycle
…
Hydra.API.RestServer/API should respond correctly ‑ GET /protocol-parameters works

♻️ This comment has been updated with latest results.

@v0d1ch v0d1ch changed the title Endpoint to return protocol params Ability to read protocol parameters via REST Jul 21, 2023
@v0d1ch v0d1ch force-pushed the endpoint-to-return-protocol-params branch 2 times, most recently from 163ae71 to 3155e65 Compare July 21, 2023 09:21
@v0d1ch v0d1ch requested review from ch1bo and ffakenz July 21, 2023 09:31
Copy link

@ghost ghost left a 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.

hydra-node/test/Hydra/API/RestServerSpec.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/API/Server.hs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
hydra-node/json-schemas/api.yaml Outdated Show resolved Hide resolved
hydra-node/json-schemas/api.yaml Outdated Show resolved Hide resolved
hydra-node/json-schemas/api.yaml Show resolved Hide resolved
hydra-node/json-schemas/api.yaml Outdated Show resolved Hide resolved
hydra-node/src/Hydra/API/Server.hs Show resolved Hide resolved
@v0d1ch
Copy link
Contributor Author

v0d1ch commented Jul 24, 2023

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 Ledger tx handle to include functions to get/set protocol parameters?

@ghost
Copy link

ghost commented Jul 25, 2023

When you say extract it from the ledger do you mean to update the Ledger tx handle to include functions to get/set protocol parameters?

Yes, something like that. I now realise this might be tricky because we use a Ledger tx interface so this would mean having an associated type PParams tx and corresponding retrieval function 🤔 It just feels like passing the file ties the server to a specific representation and storage

@ch1bo
Copy link
Collaborator

ch1bo commented Jul 25, 2023

It just feels like passing the file ties the server to a specific representation and storage

@abailly-iohk I just re-realized that the Hydra.API.Server is already massively tied to cardano-specific transactions the way the POST /commit endpoint is handled here.

@ghost
Copy link

ghost commented Jul 25, 2023

So perhaps we should just bite the bullet, drop tx parameter, and given that we now know we have a CardanoLedger, retreive the PParams from it?

@v0d1ch v0d1ch force-pushed the endpoint-to-return-protocol-params branch from 557ceb2 to a1c3aca Compare July 25, 2023 13:24
@v0d1ch v0d1ch requested review from a user and ch1bo July 25, 2023 13:35

apiServerSpec

-- REVIEW: we should add more tests for other routes here (eg. /commit)
Copy link
Collaborator

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?

Copy link
Contributor Author

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

v0d1ch and others added 9 commits July 26, 2023 11:39
- 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>
@ch1bo ch1bo force-pushed the endpoint-to-return-protocol-params branch from fa594e2 to 924e0eb Compare July 26, 2023 09:39
@pgrange pgrange merged commit 1fa4185 into master Jul 26, 2023
@pgrange pgrange deleted the endpoint-to-return-protocol-params branch July 26, 2023 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to read protocol parameters via API
4 participants