-
Notifications
You must be signed in to change notification settings - Fork 86
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
Refactor ConfirmedSnapshot #533
Conversation
Transactions CostsSizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using
Cost of Init Transaction
Cost of Commit TransactionCurrently only one UTxO per commit allowed (this is about to change soon)
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.
|
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 already a good improvement!
Maybe we can make some access onto this structure simpler in the HeadLogic
module (number . getSnapshot) confirmedSnapshot + 1 == sn
is a bit verbose), but that can be story of another PR.
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.
The tests are failing because you need to update the api.yaml
with the new JSON schema for ConfirmedSnapshot
, here https://github.com/input-output-hk/hydra-poc/blob/5681f52646fc17224453be6b1624a61794cbbcd7/hydra-node/json-schemas/api.yaml#L610
658efbd
to
667469c
Compare
I convert it back to a draft as I'd like to go through some checklist proposal: Pre-submit checklist
|
I would argue that No Errors in the CI should not be part of the checklist as it's automatically tested in the PR. I mean you won't merge a PR when CI is red would you (except in exceptional circumstnaces like the ones we faced with this failing ci/hydra job...) |
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.
Still approve :)
19ff3c4
to
682af05
Compare
682af05
to
f84b488
Compare
An InitialSnapshot always has number 0 and an empty list of confirmed transaction. So we make it explicit in the type system to avoid issues with that.
Co-authored-by: Sebastian Nagel <ch1bo@users.noreply.github.com>
On os x if jsonschema is not installed the error message was cryptic.
This code is working with jsonschema 4.16.0 so I'm releasing the constraint here to just check that jsonschema versio is higher than 3.2.0
They might be usefull to fix tests but don't belong to git repo.
f84b488
to
3bd7fac
Compare
An InitialSnapshot always has number 0 and an empty list of confirmed transactions.
So we make it explicit in the type system to avoid issues with that.