-
Notifications
You must be signed in to change notification settings - Fork 721
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
Add new interim governance commands: {create, answer, verify}-poll #5050
Add new interim governance commands: {create, answer, verify}-poll #5050
Conversation
I'm wondering if instead of For example |
@newhoggy I actually tried to keep it consistent with other cli commands already 😅 ...
etc... Not saying I am big fan, overall I do agree with you; but I value consistency more than my own preferences and it seems that this current PR is a better fit with the current state. |
As there is no such test, can you add a comment to the PR showing example interactive session that goes from start to finish and includes on-chain interaction as well? |
7adb111
to
b7c7864
Compare
@newhoggy added a quick tutorial as PR description; it should help people get through the steps :) |
Nice! Can you finish off with a section on |
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.
@KtorZ Please break this PR up into smaller commits, starting with the API changes and then propagating those changes to the cli
@Jimbo4350 what's the added value? It's take-all-or-nothing situation. There's no point having the API extension without the CLI commands. So it's one big atomic change. It's not that I can't, but if I can avoid spending half an hour breaking into smaller commits that I'll probably have to amend individually because there are further discussions on the CIP. It is a lot easier to manage as a single commit; especially for conflict resolutions. So unless there's really a strong argument to "break it down", I won't do it. |
Breaking it up in to manageable commits (with good commit messages) makes it easier for reviewers (especially new hires) to follow the changes.
You don't need to do an interactive rebase and amend the commits in this case, you can just append commits e.g "Review feedback ..." |
…mands Fixture keys were generated using the command-line itself. The set of tests cover quite extensively the various commands, as well as a few 'negative' test scenarios. It is more complicated to cover the interactive part of the 'answer-poll' command through those tests; and this is therefore left as manual test. Instructions for executing the sequence will also be provided with the introduction of the commands (e.g. in the description of [PR#5050](IntersectMBO#5050).
b7c7864
to
8cd158f
Compare
@Jimbo4350 done. As a side-note:
I would strongly advise against doing that. This is a nightmare for people coming after that try to make sense of how changes were introduced. Instead, amend commits and make it one atomic changes. I am not fond of over splitting commits into small de-correlated chunks for the same reasons. When features are introduced in one block, it often is the one block that makes sense; because some changes in module X may only be truly justified because of another change in module Y. So looking at them as two separate changes removes context -- and it's often all about context. If that's the preferred way to manage changes on this repository; so be it. I've made a split that hopefully makes sense and makes the review easier. I'll amend commit and force-push the branch if there's any edits.
|
Thanks for adding the quick tutorials. So if I understand correctly, someone can put up a poll somewhere off-chain that they've specified in a And finally I'd like to additionally ask if this is for voting by many people? If so, I have these questions:
If not, I may be misunderstanding the purpose of the PR. Perhaps my questions point to the misunderstanding I have and you can help clear that up. |
Useful to have accessible from external modules that need to construct metadata values.
…alue It is quite common to need to construct long text or bytestring and, it is annoying to have to handle that at call-site every single time. Instead, we can provide smart constructors that takes care of splitting the text or byte string into reasonably-sized chunks. Note that we could also implement a version of those functions that is *more flexible* and only constructs chunks when needed; otherwise returning a plain MetaText or MetaBytes when they fit. For example, in CDDL, we would represent such a text string as: ``` arbitrary_text = text .size (0..64) / [ * text .size (0..64) ] ``` For the sake of keeping things simple however, those functions only implement the list variation.
I'm interested to see the following:
I'm okay with adding the write to file functionality myself later if it isn't included in this PR, but you're welcome to do that too. |
This module is really meant to be driven by the cardano-cli or any client implementation that seeks to (re-)implement the SPO on-chain poll functionality.
This commit also introduces a new type-class 'AsTxMetadata' to hint to the fact that the chosen representation on-the-wire for those various types is a transaction metadata value. The serialization to CBOR becomes then straightforward once we've converted the type into a 'MetadataValue'. Similarly, the deserialization is made simpler by first deserializing an opaque 'MetadataValue', and then inspecting it to see if it has the expected shape.
These commands are pretty straightforward to write by leveraging the newly introduced Cardano.Api.Governance.Poll API. One may ask why use sometimes files, sometimes stderr and sometimes stdout in implementing those commands. As a rule of thumb: - stdout = relevant content that should be structured to be piped into other tools or send to files - stderr = debug information useful to communicate context and details to users A command-line that outputs interactive debug information on stdout is arguably doing something wrong; unless it's the main terminal output of the application (e.g. printing structured logs on stdout). Then, why stdout rather than a file? Because I find the UX a lot better that way. Command lines with too many params are arguably hard to process; and using file as the medium of exchanges makes it harder / prevent piping into other tools easily. When printing structured results on stdout, one can always redirect the output to a file should they want it; so IMO stdout should always be the default; and files used only when necessary. Here I am only using an output file in the case of create-poll as a "build artifact". It allows to produce two outputs with distinct purpose; the file is meant to be shared as file, and thus it makes sense to treat it as such from the CLI as well. It also makes it clearer for users (even though that's going to be only super users here) what is meant to be shared and what is metadata.
Mostly plumbing and parser implementation following what already exists.
This is meant as a way to assert on *expected failures*! Sadly, `try` or other exception handling mechanisms do not work inside of the `TestT` monad, so I had to extract and lift the error to be able to catch it and assert on it. Yet, I need to assert on failures and thus, failures should not crash the test early but be assertable as a possible execution outcome. There's maybe something more clever to do but I only had a day and a half to spend on all this so I'd rather "get it done".
…mands Fixture keys were generated using the command-line itself. The set of tests cover quite extensively the various commands, as well as a few 'negative' test scenarios. It is more complicated to cover the interactive part of the 'answer-poll' command through those tests; and this is therefore left as manual test. Instructions for executing the sequence will also be provided with the introduction of the commands (e.g. in the description of [PR#5050](IntersectMBO#5050).
… Witness} ``` roundtrip GovernancePoll CBOR: OK (0.09s) ✓ roundtrip GovernancePoll CBOR passed 100 tests. roundtrip GovernancePollAnswer CBOR: OK ✓ roundtrip GovernancePollAnswer CBOR passed 100 tests. roundtrip GovernancePollWitness CBOR: OK (0.01s) ✓ roundtrip GovernancePollWitness CBOR passed 100 tests. ```
``` Cardano.Api Test.Cardano.Api.Metadata valid & rountrip text chunks: OK (0.03s) ✓ valid & roundtrip text chunks passed 100 tests. Empty chunks 3% ▌··················· ✓ 1% Single chunks 26% █████▏·············· ✓ 5% Many chunks 71% ██████████████▏····· ✓ 25% valid & rountrip bytes chunks: OK ✓ valid & roundtrip bytes chunks passed 100 tests. Empty chunks 3% ▌··················· ✓ 1% Single chunks 55% ███████████········· ✓ 5% Many chunks 42% ████████▍··········· ✓ 25% ``` Turns out there were two issues: - Empty {text,byte}strings would generate a singleton chunk with an empty value; which is okay semantically but ugly; empty strings now generate an empty chunk. - Metadata values measure the length of UTF-8-encoded strings, which means we can't rely on default text functions to split a text string. This is likely an overkill in many situation in the context of PR#5050 since most questions / answers will be in plain english. However, we can now put emojis and crazy unicode characters in there without problems.
8cd158f
to
160f67b
Compare
@newhoggy Done. Note: there's no instructions whatsoever about what linter, formatter, etc.., are expected; if there are some I couldn't find them. I did find a Makefile which sounded useful with a |
Ah, yes, I see your problem now. The linter is defined here with the version number: https://github.com/input-output-hk/cardano-node/blob/master/.github/workflows/check-hlint.yml But of course, PR does not trigger the CI for forks for security reasons, so it is not obvious that linting is necessary. We need to document that better. We need to consider how to improve the experience for contributions outside of IOG. A good start could be in Github PR template. Thanks for pointing this out. |
I have triggered the build to see if it passes in CI |
Much thanks for the property tests! |
Don't worry about the |
Text.splitAt | ||
|
||
-- | Create a 'TxMetadataValue' from a 'ByteString' as a list of chunks of an | ||
-- accaptable size. |
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.
Typo accaptable
Okay CI passes now. I triggered CI by pushing the commit on my own branch to the repo. I have approved the PR. |
…mands Fixture keys were generated using the command-line itself. The set of tests cover quite extensively the various commands, as well as a few 'negative' test scenarios. It is more complicated to cover the interactive part of the 'answer-poll' command through those tests; and this is therefore left as manual test. Instructions for executing the sequence will also be provided with the introduction of the commands (e.g. in the description of [PR#5050](#5050).
Subsumed by #5112 |
…mands Fixture keys were generated using the command-line itself. The set of tests cover quite extensively the various commands, as well as a few 'negative' test scenarios. It is more complicated to cover the interactive part of the 'answer-poll' command through those tests; and this is therefore left as manual test. Instructions for executing the sequence will also be provided with the introduction of the commands (e.g. in the description of [PR#5050](IntersectMBO#5050).
…mands Fixture keys were generated using the command-line itself. The set of tests cover quite extensively the various commands, as well as a few 'negative' test scenarios. It is more complicated to cover the interactive part of the 'answer-poll' command through those tests; and this is therefore left as manual test. Instructions for executing the sequence will also be provided with the introduction of the commands (e.g. in the description of [PR#5050](IntersectMBO/cardano-node#5050).
…mands Fixture keys were generated using the command-line itself. The set of tests cover quite extensively the various commands, as well as a few 'negative' test scenarios. It is more complicated to cover the interactive part of the 'answer-poll' command through those tests; and this is therefore left as manual test. Instructions for executing the sequence will also be provided with the introduction of the commands (e.g. in the description of [PR#5050](IntersectMBO/cardano-node#5050).
The Cardano Foundation proposes a mechanism for polling Cardano stake pool operators on specific topics. Polls are done on-chain through transaction metadata and authenticated through stake pool credentials (either VRF public key similar to what's described in CIP-0022 or Ed25519 cold key).
The goal is to gather opinions on governance matters such as protocol parameters updates. This standard is meant to be an inclusive interim solution while the work on a larger governance framework such as CIP-1694 continues.
See proposed CIP for details.
This commits adds three new commands:
create-poll: For the current governing entities, as a means to create new polls.
answer-poll: For participants who want to answer a given poll.
verify-poll: For anyone who seek to verify a poll entry (e.g. explorers)
The commands are built to fit and play nicely within the cardano-cli. The poll and answers structures are based on transaction metadata and require to be embedded in an actual transaction. The added commands however only works from metadata and raw "GovernancePoll" envelopes.
Tutorial
Pre-requisites
Here's a quick tutorial on how to interactively submit an answer to an SPO
survey. To start, you must have received a valid survey in the form of a JSON
files properly constructed using the
create-poll
command. The file shouldlook roughly like the following:
poll.json
Creating answer
From there, you can create a metadata entry to answer the survey using the
governance answer-poll
command as such:$ cardano-cli governance answer-poll --poll-file poll.json --signing-key-file key.secret
Note that:
The key specified as
--signing-key-file
can/must be either a VRF secretkey or a stake pool cold private key (Ed25519) in the JSON/Text-Envelope
usual format expected by the cardano-cli. Here are an example of each:
VRF key
Cold key
This command will prompt you to answer interactively. If you do not wish to
be prompt interactively, you can use
--answer
with the index of theanswer.
Running this command will display the survey in a human-readable form, and
prompt you for an answer, as shown below:
You can proceed by typing one of the possible answer index (here,
0
or1
)and a newline. This will print witnessed metadata in the form of a JSON
detailed schema that shall then be posted on-chain in any transaction: the
easiest being to build a simple transaction to yourself carrying the metadata.
Here's an example of metadata using a VRF key and choosing the answer
0
:answer.json
Publishing answer
From there, you can use the
transaction build
command to create a transactionto post on-chain. You'll need a signing key associated to a UTxO with enough
funds to carry the transaction (~0.2 Ada should you make a basic transaction to
yourself).
Assuming you have saved metadata produced from the previous step in a file
called
answer.json
, the command for building the transaction shall look like:You'll need to fill-in
--tx-in
&--change-address
with their correspondingvalues. From there, you can sign
answer.tx
and submit the result as usual. Ifeverything goes well, the cardano-cli should display a transaction id that you
can track on-chain to ensure your reply to the survey was properly published.
Verifying Answers
Finally, it's possible to verify answers seen on-chain using the
governance verify-poll
command. What 'verify' means here is two-folds:Assuming you still have the original
poll.json
file, and some witnessedmetadata (as provided by the
governance answer-poll
command, and as found intransactions on-chain) as
answer.json
, you can verify its validity via:$ cardano-cli governance verify-poll --poll-file poll.json --metadata-file answer.json
On success, this should outputs:
Ok.
Otherwise, the command will formulate an issue with the answer and/or poll.