-
Notifications
You must be signed in to change notification settings - Fork 84
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
Implement a 100 Ada hard-coded commit limit in the hydra-node #763
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 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 review tries to apply the perfection game protocol
I will describe what I like about this P.R. and then what improvement I can think of to make it perfect.
Ask for clarifications if my comments are not clear.
If you disagree with my comments, just ignore them and don’t answer or explain.
Hence I, in advance, approve the P.R.
To describe what I would change, I will try to extensively use the suggest change feature of GitHub as I believe this is the most efficient way to upload data from my brain to the authors' brains. That being said, the suggestions I will write will probably not compile or pass the test so do not just accept them but consider them for what they are: the description of an improvement to make the P.R. perfect.
What I like about this P.R.:
- One single commit with precise and small perimeter. It really makes it a pleasure to review.
- Tests have been introduced to cover the new case.
For me to find it perfect you would have to:
- see other improvement proposals inline
65f1ef9
to
898c679
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.
You found the right place where to check for this, but we require this to be documented. For this, I would suggest moving this into a top-level variable and encode the maximum supported and tried to commit value in a dedicated error type. That way, users will get a clearly understandable error when they try to commit too much.
e57ea92
to
558e452
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.
what is the idea when we want to remove this limit?
- revert the PR?
- remove the hardcode?
should we have this procedure documented somewhere?
would not be better to have a maybe limit option as part of our node configuration?
maybe this way, we can configure the limit and accept to join a head if it is under 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.
We should not hard-code the value more than once and return it to the user. Also knowing how much they tried to commit is likely helpful.
558e452
to
89a701e
Compare
4c2b400
to
b5818f1
Compare
b5818f1
to
dc7af42
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.
Ideally we would be using the Lovelace
type directly to describe that the amounts are lovelace (and not ada).
I do approve already though, as this could be also done after if you don't want to do it right away.
Co-authored-by: Pascal Grange <pgrange@users.noreply.github.com>
Co-authored-by: Pascal Grange <pgrange@users.noreply.github.com>
Co-authored-by: Pascal Grange <pgrange@users.noreply.github.com>
dc7af42
to
d7eba73
Compare
d7eba73
to
a968fa9
Compare
fix #762