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

Add proposal hash check when creating info governance action #910

Merged
merged 10 commits into from
Sep 25, 2024

Conversation

palas
Copy link
Contributor

@palas palas commented Sep 23, 2024

Changelog

- description: |
    Add proposal hash check when creating `info` governance action
  type:
  - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way

Context

This is another step forward on addressing #882.

Previous related PR: #895

So far I have only added the check to governance action create-info. There are several other places where to add the check, but I thought I would have a round of review first, before propagating.

How to trust this PR

I adapted a test to validate the new functionality, and that gives me a lot of confidence. I would try to check that the code style is good, I didn't introduce any bugs, the parameter structure and names make sense, and the help is comprehensive and comprehensible.

This check disables HTTP schemas (and obviously FILE), because I have been told the spec says that only HTTPS and IPFS` are allowed. But I could not find where this is said. Could anybody confirm?

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@palas palas added enhancement New feature or request test Modifies/extends the test suite labels Sep 23, 2024
@palas palas self-assigned this Sep 23, 2024
@palas palas linked an issue Sep 23, 2024 that may be closed by this pull request
@palas palas requested review from a team as code owners September 23, 2024 23:57
@CarlosLopezDeLara
Copy link
Contributor

CarlosLopezDeLara commented Sep 24, 2024

Looks great to me!|

  • As discussed, using either --check-anchor-data or --trust-anchor-data is mandatory.
  • The error messages are clear.
  • When the check is correct we have no news, so it's perfect!
cardano-cli conway governance action create-info (--mainnet | --testnet)
                                                          --governance-action-deposit NATURAL
                                                          ( --deposit-return-stake-verification-key STRING
                                                          | --deposit-return-stake-verification-key-file FILEPATH
                                                          | --deposit-return-stake-key-hash HASH
                                                          | --deposit-return-stake-script-file FILEPATH
                                                          | --deposit-return-stake-address ADDRESS
                                                          )
                                                          --anchor-url TEXT
                                                          --anchor-data-hash HASH
                                                          ( --check-anchor-data
                                                          | --trust-anchor-data
                                                          )
                                                          --out-file FILEPATH
 ./dist-newstyle/build/x86_64-linux/ghc-8.10.7/cardano-cli-9.4.1.0/x/cardano-cli/build/cardano-cli/cardano-cli conway governance action create-info \
--testnet \
--governance-action-deposit 100000000000 \
--deposit-return-stake-verification-key-file ~/tests/example/utxo-keys/stake1.vkey \
--anchor-url https://raw.githubusercontent.com/carloslodelar/proposals/refs/heads/main/WrongURL.jsonld  \
--anchor-data-hash d4efa9e0c3d80f5be35def08551eaf5aef17eae832fdb835e5f4b208d98f3dff \
--check-anchor-data \
--out-file ~/test-conway/testcheckmetadatahash.action
Command failed: governance action create-info  Error: Error while checking proposal hash: \
Bad status code when downloading anchor data: 404 (Not Found: 404: Not Found)
./dist-newstyle/build/x86_64-linux/ghc-8.10.7/cardano-cli-9.4.1.0/x/cardano-cli/build/cardano-cli/cardano-cli conway governance action create-info \
--testnet \
--governance-action-deposit 100000000000 \
--deposit-return-stake-verification-key-file stake1.vkey \
--anchor-url https://c-ipfs-gw.nmkr.io/ipfs/QmXdfDSSkR8TYBntMT4nuTL9sE9E44FJnpWVpaY8ZTfwjj \
--anchor-data-hash d4efa9e0c3d80f5be35def08551eaf5aef17eae832fdb835e5f4b208d98f3d5f \
--check-anchor-data \
--out-file ~/test-conway/testcheckmetadatahash.action

Command failed: governance action create-info  Error: Hashes do not match while checking proposal hashes! 
Expected: "d4efa9e0c3d80f5be35def08551eaf5aef17eae832fdb835e5f4b208d98f3d5f"
  Actual: "d4efa9e0c3d80f5be35def08551eaf5aef17eae832fdb835e5f4b208d98f3dff"
./dist-newstyle/build/x86_64-linux/ghc-8.10.7/cardano-cli-9.4.1.0/x/cardano-cli/build/cardano-cli/cardano-cli conway governance action create-info \
--testnet \
--governance-action-deposit 100000000000 \
--deposit-return-stake-verification-key-file ~/tests/example/utxo-keys/stake1.vkey \
--anchor-url https://c-ipfs-gw.nmkr.io/ipfs/WrongURL  \
--anchor-data-hash d4efa9e0c3d80f5be35def08551eaf5aef17eae832fdb835e5f4b208d98f3dff \
--check-anchor-data \
--out-file ~/test-conway/testcheckmetadatahash.action
Command failed: governance action create-info  
Error: Error while checking proposal hash: Bad status code when downloading anchor data: 400 
(Bad Request: invalid ipfs path: invalid path "/ipfs/WrongURL": 
invalid CID: selected encoding not supported)

Failing to use the any of the flags

./dist-newstyle/build/x86_64-linux/ghc-8.10.7/cardano-cli-9.4.1.0/x/cardano-cli/build/cardano-cli/cardano-cli conway governance action create-info --testnet --governance-action-deposit 100000000000 --deposit-return-stake-verification-key-file ~/tests/example/utxo-keys/stake1.vkey --anchor-url https://raw.githubusercontent.com/carloslodelar/proposals/refs/heads/main/WrongURL.jsonld  --anchor-data-hash d4efa9e0c3d80f5be35def08551eaf5aef17eae832fdb835e5f4b208d98f3dff --out-file ~/test-conway/testcheckmetadatahash.action
Missing: (--check-anchor-data | --trust-anchor-data)

Using http

./dist-newstyle/build/x86_64-linux/ghc-8.10.7/cardano-cli-9.4.1.0/x/cardano-cli/build/cardano-cli/cardano-cli conway governance action create-info --testnet --governance-action-deposit 100000000000 --deposit-return-stake-verification-key-file ~/tests/example/utxo-keys/stake1.vkey --anchor-url http://raw.githubusercontent.com/carloslodelar/proposals/refs/heads/main/WrongURL.jsonld  --anchor-data-hash d4efa9e0c3d80f5be35def08551eaf5aef17eae832fdb835e5f4b208d98f3dff --check-anchor-data \--out-file ~/test-conway/testcheckmetadatahash.action
Command failed: governance action create-info  Error: Error while checking proposal hash: 
Unsupported URL scheme: http:

Copy link
Contributor

@CarlosLopezDeLara CarlosLopezDeLara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@CarlosLopezDeLara
Copy link
Contributor

Want to give this a try, @Crypto2099?

@palas palas added this pull request to the merge queue Sep 25, 2024
Merged via the queue into main with commit 4a5096b Sep 25, 2024
25 checks passed
@palas palas deleted the add-hash-validation branch September 25, 2024 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test Modifies/extends the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] - Add File Hash Validation when Building Transaction
3 participants