-
Notifications
You must be signed in to change notification settings - Fork 6
Add author preconfiguration #118
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
base: production
Are you sure you want to change the base?
Conversation
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’ve made a first quick read of the PR and pointed to some things that we should change, as well as questions that might lead to doing things differently.
backend/README.md
Outdated
# 1. Name-only: | ||
# { "name": "Cardano Foundation" } | ||
# 2. With full cryptographic witness: | ||
# { "name": "...", "witnessAlgorithm": "ed25519", "publicKey": "...", "signature": "..." } |
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.
"signature" can not be part of the pre-configuration, as you can’t pre-sign a non-existing rationale. So let’s also leave the witness out, as the witness will be fully provided by the person when signing. So let’s only support name-only for pre-configurations.
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.
Resolved in 63620a3
frontend/src/Main.elm
Outdated
networkIdTyped = | ||
Address.networkIdFromInt networkId |> Maybe.withDefault Testnet | ||
|
||
-- Decode authorPreconfig manually to handle name-only authors |
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.
Why does it need to be handled here again? compared to voterPreconfig which isn’t handled here?
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.
Resolved in d0ea2e1
-} | ||
authorWitnessDecoder : JD.Decoder AuthorWitness | ||
authorWitnessDecoder = |
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.
Let’s not do 3 different formats here, we should only support one format which has the shape of the one in the CIP:
"authors": [
{
"name": "Ryan Williams",
"witness": {
"witnessAlgorithm": "CIP-0008",
"publicKey": "7ea09a34...37480a",
"signature": "84582a...71409"
}
}
We can require that the environment variable to provide pre-defined authors just provide the name anyway, without witness, because the full witness will be generated by the signature creation.
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.
Resolved in d8a944d
frontend/src/Page/Preparation.elm
Outdated
The CIP-100 verification API only accepts HTTPS URLs. | ||
-} | ||
ipfsToHttpsUrl : String -> String | ||
ipfsToHttpsUrl url = |
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.
Is this manually done everywhere else? or is there already a function doing that at the other places where we do the conversion?
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.
There are multiple places where the IPFS to HTTPS is done manually instead of using the ipfsToHttpsUrl function. For consistency I will make them all use the same function from Helper.
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.
Resolved in 09083c7
|
||
proxyRequestBody = | ||
JE.object | ||
[ ( "url", JE.string verificationApiUrl ) |
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’m curious, why not make the request directly? Is it because of CORS? Because any request we send through the server before going to final destination double the traffic and the server costs. If it’s just CORS, we should modify the verifycardanomessage server to accept direct request from the domains we use for the voting tool. Also, let’s move that into the Api module, which contains all requests of the website.
case prop.metadata of | ||
RemoteData.Success _ -> | ||
verifyCip100Metadata actionId prop.metadataUrl | ||
|> Cmd.map ctx.wrapMsg |
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.
Let’s avoid sending a request and make the verify server work for every proposal metada we receive. We can make the request only when a proposal is selected. This will significantly reduce both the traffic and the cpu usage for the verify server.
frontend/src/Page/Preparation.elm
Outdated
errorMessage = | ||
case decodingError of | ||
JD.Failure message _ -> | ||
if String.contains "No witness found for author" message then |
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.
Let’s not manipulate stringly typed errors. If we want to identify a specific type of error, let’s return a typed error in the decoder.
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.
Resolved in 7547fc8
frontend/src/Page/Preparation.elm
Outdated
|
||
viewSelectedProposal : ViewContext msg -> ActiveProposal -> Html msg | ||
viewSelectedProposal ctx { id, actionType, metadata, metadataUrl, metadataHash } = | ||
viewSelectedProposal : ViewContext msg -> InnerModel -> ActiveProposal -> Html msg |
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.
Since we only need 1 field which is the cip100Verification
field, let’s just pass that field instead of the whole model.
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.
Resolved in c001977
Cool to see that the verify website also has an API endpoint now for verifications. Though as suggested in the review above, we should be using it more sparingly, only after proposal selection, instead of when loading proposals. Thanks for sharing some visuals also! This way I don’t have to run the thing immediately. Some remarks:
|
|
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.
Pull Request Overview
This PR adds author signature preconfiguration functionality to streamline rationale authoring, fixes CIP-100 signature structure to use nested "witness" fields, and enhances UI for signature verification. The changes focus on improving author management and signature validation workflows.
- Add support for preconfigured authors that can be set via environment variables
- Update CIP-100 signature structure to use nested "witness" format per standard
- Implement automatic signature verification with visual feedback and status indicators
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
frontend/static/index.html | Add author preconfiguration transformation and initialization |
frontend/src/ProposalMetadata.elm | Update decoder to use CIP-100 nested witness format |
frontend/src/Page/Preparation.elm | Add CIP-100 verification API, signature validation, and author preconfiguration support |
frontend/src/Page/Cart.elm | Replace local IPFS URL conversion with shared helper function |
frontend/src/Main.elm | Thread author preconfiguration through application state |
frontend/src/Helper.elm | Add shared IPFS URL conversion utility and improve text wrapping |
frontend/src/Api.elm | Use shared IPFS URL conversion helper |
backend/server.py | Add preconfigured authors environment variable support |
backend/README.md | Document new author preconfiguration environment variable |
backend/.env.example | Add example configuration for preconfigured authors |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.