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

Build cmd - Add options to disable automatic metadata-hash checks on votes and proposals included in the transaction. #1065

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Build cmd - Add options to disable automatic metadata-hash checks on votes and proposals included in the transaction. #1065

wants to merge 1 commit into from
+64 −2

Conversation

CarlosLopezDeLara
Copy link
Contributor

Changelog

- description: |
   Build cmd - Add options to disable automatic metadata-hash checks on votes and proposals included in the transaction:  `[--disable-metadata-checks-on-proposals]` and `[--disable-metadata-checks-on-votes]`

# uncomment types applicable to the change:
  type:
   - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
   - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

#951 (comment)

How to trust this PR

The simplest way to test it is using a proposal with intentionally wrong metadata hash. The check is skipped and transaction is built.

cardano-cli conway transaction build \
--proposal-script-file ./guardrailscriptV2/guardrail-script.plutus \
--tx-in-collateral 276fc38aa1aa2db62a5fabf3c515454f97576082035aa65ec0b6754be8b45dd8#0 \
--proposal-redeemer-value '{}' \
--tx-in 276fc38aa1aa2db62a5fabf3c515454f97576082035aa65ec0b6754be8b45dd8#0 \
--change-address addr_test1qq5s5ux58j6g8gcp5clwykcj9umhsdh3yg88plrmqhgurztqn7jtt86nwlru6v8ulh8alw7gmgp7w0tl2sd5jpwhg5jqhg8xjk \
--proposal-file example/transactions/treasury.action \
--disable-metadata-checks-on-proposals \
--out-file example/transactions/treasury-tx.raw
Estimated transaction fee: 309632 Lovelace

whereas without the flag we get:

cardano-cli conway transaction build \
--proposal-script-file ./guardrailscriptV2/guardrail-script.plutus \
--tx-in-collateral 276fc38aa1aa2db62a5fabf3c515454f97576082035aa65ec0b6754be8b45dd8#0 \
--proposal-redeemer-value '{}' \
--tx-in 276fc38aa1aa2db62a5fabf3c515454f97576082035aa65ec0b6754be8b45dd8#0 \
--change-address addr_test1qq5s5ux58j6g8gcp5clwykcj9umhsdh3yg88plrmqhgurztqn7jtt86nwlru6v8ulh8alw7gmgp7w0tl2sd5jpwhg5jqhg8xjk \
--proposal-file example/transactions/treasury.action \
--out-file example/transactions/treasury-tx.raw
Command failed: transaction build  Error: Hash of the file is not valid. Url: https://tinyurl.com/3wrwb2as Hashes do not match!
Expected: "52e69500a92d80f2126c836a4903dc582006709f004cf7a28ed648f732dff8d1"
  Actual: "52e69500a92d80f2126c836a4903dc582006709f004cf7a28ed648f732dff8d2"

The same with votes:

cardano-cli conway transaction build \
--tx-in 2ca3717e5bcab69f599758c6d0a2314945515b6388e53d4e7b5cd3a31c74192e#0 \
--change-address addr_test1qq5s5ux58j6g8gcp5clwykcj9umhsdh3yg88plrmqhgurztqn7jtt86nwlru6v8ulh8alw7gmgp7w0tl2sd5jpwhg5jqhg8xjk \
--vote-file example/transactions/treasury-cc1.vote \
--vote-file example/transactions/treasury-cc2.vote \
--vote-file example/transactions/treasury-cc3.vote \
--witness-override 4 \
--disable-metadata-checks-on-votes \
--out-file example/transactions/treasury-cc-votes-tx.raw
Estimated transaction fee: 214561 Lovelace

Without the flag:

cardano-cli conway transaction build \
--tx-in 2ca3717e5bcab69f599758c6d0a2314945515b6388e53d4e7b5cd3a31c74192e#0 \
--change-address addr_test1qq5s5ux58j6g8gcp5clwykcj9umhsdh3yg88plrmqhgurztqn7jtt86nwlru6v8ulh8alw7gmgp7w0tl2sd5jpwhg5jqhg8xjk \
--vote-file example/transactions/treasury-cc1.vote \
--vote-file example/transactions/treasury-cc2.vote \
--vote-file example/transactions/treasury-cc3.vote \
--witness-override 4 \
--out-file example/transactions/treasury-cc-votes-tx.raw
Command failed: transaction build  Error: Hash of the file is not valid. Url: https://tinyurl.com/3wrwb2as Hashes do not match!
Expected: "52e69500a92d80f2126c836a4903dc582006709f004cf7a28ed648f732dff8d1"
  Actual: "52e69500a92d80f2126c836a4903dc582006709f004cf7a28ed648f732dff8d2"

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

@CarlosLopezDeLara CarlosLopezDeLara added the enhancement New feature or request label Feb 12, 2025
@CarlosLopezDeLara CarlosLopezDeLara self-assigned this Feb 12, 2025
@CarlosLopezDeLara CarlosLopezDeLara added this pull request to the merge queue Feb 13, 2025
@Jimbo4350 Jimbo4350 removed this pull request from the merge queue due to a manual request Feb 13, 2025
Jimbo4350
Jimbo4350 requested changes Feb 13, 2025
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Commands/Transaction.hs Outdated
@@ -133,6 +133,8 @@ data TransactionBuildCmdArgs era = TransactionBuildCmdArgs
, mUpdateProposalFile :: !(Maybe (Featured ShelleyToBabbageEra era (Maybe UpdateProposalFile)))
, voteFiles :: ![(VoteFile In, Maybe CliVoteScriptRequirements)]
, proposalFiles :: ![(ProposalFile In, Maybe CliProposalScriptRequirements)]
, disableMetadataChecksOnProposals :: !Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use Bools. It's not too bad here but boolean blindness is a thing: https://yveskalume.dev/boolean-blindness-dont-represent-state-with-boolean

Copy link
Contributor

Choose a reason for hiding this comment

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

In particular for a check flag it can be confusing, having a type like:

data MetadataCheck =
  CheckMetadata
  | DontCheckMetadata

will be clearer

smelc
smelc approved these changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@smelc smelc left a comment

Choose a reason for hiding this comment

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

Approving with the caveat that @Jimbo4350's point is addressed 👍

Jimbo4350
Jimbo4350 requested changes Feb 13, 2025
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.

I need to look again. Please clean up your commit message. "fix formatting" is not useful!

cardano-cli/src/Cardano/CLI/EraBased/Options/Transaction.hs
pMetadataChecksOnProposals =
Opt.flag EnableMetadataCheck DisableMetadataCheck $
mconcat
[ Opt.long "disable-metadata-checks-on-proposals"
Copy link
Contributor

Choose a reason for hiding this comment

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

Right so now we have a common prefix: disable-metadata-checks

Repeating this is going to add to our already verbose commands. I'll have another look tomorrow to suggest an alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to favor clarity over shortness with the flag names. The shortest I can come up with without loosing clarity is:

--disable-vote-metadata-check
--disable-proposal-metadata-check

Copy link
Contributor

@Jimbo4350 Jimbo4350 Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What we are looking for is the following:

[--update-proposal-file FILEPATH [--disable-proposal-metadata-check]]

@CarlosLopezDeLara CarlosLopezDeLara force-pushed the clr/disable-hash-checks branch 2 times, most recently from 19d9c53 to 53773b5 Compare February 13, 2025 20:50
@CarlosLopezDeLara
Add new flags to disable metadata checks on proposals and votes to bu…
2ec4618 
…ild cmd.

The build cmd automatically runs checks on the metadata hashes on proposals and votes included in the transaction. The new flags allow the user to disable such checks. Useful for testing purposes and for users that may want to avoid CLI reaching out to the internet.

--disable-metadata-checks-on-proposals
--disable-metadata-checks-on-votes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 requested changes

@carbolymer carbolymer carbolymer approved these changes

@smelc smelc smelc approved these changes

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

@palas palas Awaiting requested review from palas palas 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

Requested changes must be addressed to merge this pull request.

Assignees

@CarlosLopezDeLara CarlosLopezDeLara

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

Successfully merging this pull request may close these issues.
4 participants
@CarlosLopezDeLara @carbolymer @smelc @Jimbo4350