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.

Sign up for GitHub

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

Jump to bottom

Add hash validation and support for HTTP(S) and IPFS to command hash anchor-data #895

Merged
merged 18 commits into from
Sep 19, 2024
Merged

Add hash validation and support for HTTP(S) and IPFS to command hash anchor-data #895

merged 18 commits into from
Sep 19, 2024

Conversation

palas
Copy link
Contributor

@palas palas commented Sep 12, 2024

Changelog

- description: |
    Added hash validation and support for HTTP(S) and IPFS to command `hash anchor-data`
  type:
  - feature        # introduces a new feature

Context

This PR is a first step towards solving: #882
But it doesn't completely address it because the check should be in the commands mentioned and not in a separate command. That will be addressed in a separate PR.

How to trust this PR

The tests should provide a lot of assurance. I think the most unverified bit is the URL structure for IPFS_GATEWAYS, but I have tried it against existing gateways and seems to work. Other than that, code style, and maybe code organisation? We may need to move the verification code somewhere else in order to be able to use it in the next PR.

The PR adds quite a few packages, but it was mainly for testing. For querying it uses http-client (and http-client-tls for HTTPS support).

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 the enhancement New feature or request label Sep 12, 2024
@palas palas self-assigned this Sep 12, 2024
@palas palas linked an issue Sep 12, 2024 that may be closed by this pull request
[FR] - Add File Hash Validation when Building Transaction #882
Closed
@palas palas force-pushed the add-hash-validation branch 10 times, most recently from 44caf5e to cf9cf24 Compare September 13, 2024 22:22
@palas palas requested review from a team as code owners September 13, 2024 22:22
@palas palas force-pushed the add-hash-validation branch 2 times, most recently from eeb9869 to 02d8367 Compare September 13, 2024 22:27
carbolymer
carbolymer reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

Nice! Quite elegant solution.

cardano-cli/test/cardano-cli-test/Test/Cli/Hash.hs Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Run/Hash.hs Outdated
Comment on lines 67 to 76
case mExpectedHash of
Just expectedHash
| hash /= expectedHash ->
left $ HashMismatchedHashError expectedHash hash
| otherwise -> liftIO $ putStrLn "Hashes match!"
Nothing -> writeHash hash
Copy link
Contributor

Choose a reason for hiding this comment

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

This code bit is surprising a little: specifying the hash in the CLI argument, makes the command ignore --out-file argument. I agree that's redundant here, but for the sake of consistency, I would still add writing output file even when the hash is specified as a command line argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now changed it for outputting only in case of --out-file and not in case of stdout (dda90ab)
I the case of stdout I am guessing it would be confusing to mix both results

Copy link
Contributor

Choose a reason for hiding this comment

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

You can print the information message to stderr and the hash to stdout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still will be mixed usually, and it is not an error. Not sure. Another option is to print the message to a file instead of the hash when --out-file is used. Or to make them mutually exclusive

Copy link
Contributor

@carbolymer carbolymer Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well stderr isn't reserved only for errors, but is usually used for any kind of diagnostics. I guess ignoring --out-file would work too if we print a warning here.

@CarlosLopezDeLara thoughts on the UX here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried making them mutually exclusive in: cabb16a
I think that works quite well, maybe the problem was trying to do two conflicting things at the same time

Jimbo4350
Jimbo4350 approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 . Couple of comments and have somebody from devx look at your nix changes.

cardano-cli/src/Cardano/CLI/Types/Errors/HashCmdError.hs Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Run/Hash.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Run/Hash.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Run/Hash.hs Outdated Show resolved Hide resolved
.gitattributes Show resolved Hide resolved
cardano-cli/test/cardano-cli-test/Test/Cli/Hash.hs Outdated Show resolved Hide resolved
cardano-cli/test/cardano-cli-test/Test/Cli/Hash.hs Outdated Show resolved Hide resolved
cardano-cli/test/cardano-cli-test-lib/Test/Cardano/CLI/Util.hs Outdated Show resolved Hide resolved
flake.nix Show resolved Hide resolved
@palas palas force-pushed the add-hash-validation branch 2 times, most recently from 47d4f27 to 4f7cdac Compare September 19, 2024 01:02
angerman
angerman approved these changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@angerman angerman left a comment

Choose a reason for hiding this comment

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

LGTM

flake.nix Show resolved Hide resolved
@palas palas added this pull request to the merge queue Sep 19, 2024
Merged via the queue into main with commit ddcf25f Sep 19, 2024
25 checks passed
@palas palas deleted the add-hash-validation branch September 19, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer left review comments

@angerman angerman angerman approved these changes

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

@smelc smelc Awaiting requested review from smelc smelc is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

Assignees

@palas palas

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] - Add File Hash Validation when Building Transaction
4 participants
@palas @angerman @carbolymer @Jimbo4350