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

Remove check of drep metadata size. #818

Merged
merged 1 commit into from
Jul 19, 2024
Merged

Remove check of drep metadata size. #818

merged 1 commit into from
Jul 19, 2024

Conversation

CarlosLopezDeLara
Copy link
Contributor

@CarlosLopezDeLara CarlosLopezDeLara commented Jul 4, 2024 •
edited by carbolymer
Loading

Changelog

- description: |
    Remove check of Drep metadata size, always return the hash of the file passed by the user. This to be compatible with CIP119.
    Upgrade cardano-api-9.1.0.0
# 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

Resolves: #792
Depends on: IntersectMBO/cardano-api#569 which is not on the tagged cardano-api-9.0.0.0 . Therefore this requires a new release of cardano-api.

How to trust this PR

Before this PR we had:

cardano-cli conway governance drep metadata-hash --drep-metadata-file drep.jsonld
Command failed: governance drep metadata-hash  Error: DRep metadata validation error: DRep metadata must consist of at most 512 bytes, but it consists of 178103 bytes.

With this PR we always return the hash and the resulting hash matches the one on the test vector of CIP-119 AND matches the hash obtained with governance hash anchor-data AND matches the result of using blake2b directly:

$ ./dist-newstyle/build/x86_64-linux/ghc-8.10.7/cardano-cli-8.25.0.0/x/cardano-cli/build/cardano-cli/cardano-cli conway governance drep metadata-hash
 --drep-metadata-file tmp/drep.jsonld 
a14a5ad4f36bddc00f92ddb39fd9ac633c0fd43f8bfa57758f9163d10ef916de

$ ./dist-newstyle/build/x86_64-linux/ghc-8.10.7/cardano-cli-8.25.0.0/x/cardano-cli/build/cardano-cli/cardano-cli conway governance hash anchor-data --file-text tmp/drep.jsonld 
a14a5ad4f36bddc00f92ddb39fd9ac633c0fd43f8bfa57758f9163d10ef916de

$ b2sum -l 256 tmp/drep.jsonld 
a14a5ad4f36bddc00f92ddb39fd9ac633c0fd43f8bfa57758f9163d10ef916de  tmp/drep.jsonld

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Self-reviewed the diff

carbolymer
carbolymer reviewed Jul 4, 2024
View reviewed changes
cabal.project Outdated Show resolved Hide resolved
carbolymer
carbolymer reviewed Jul 4, 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.

LGTM, but I agree with @smelc here: https://github.com/IntersectMBO/cardano-cli/pull/818/files#r1665385383

@CarlosLopezDeLara CarlosLopezDeLara force-pushed the clr/drepmetadata branch 2 times, most recently from a62c1ea to 37c4e4b Compare July 4, 2024 16:42
smelc
smelc approved these changes Jul 5, 2024
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.

Looking good! 💯

cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/DRep.hs Outdated Show resolved Hide resolved
@palas
Copy link
Contributor

palas commented Jul 15, 2024

Backup: https://github.com/IntersectMBO/cardano-cli/tree/backup/clr/drepmetadata

@palas
Copy link
Contributor

palas commented Jul 15, 2024

FYI: I have rebased your branch because we have done changes to the formatting. I have made a copy of the unrebased branch that you can find in my previous comment in this PR.

@carbolymer carbolymer requested review from a team as code owners July 19, 2024 12:23
@CarlosLopezDeLara @carbolymer
Use hashDrepMetadata from cardano-api
9aedc74 
* The simplified hashDRepMetadata no longer checks for the size of the DRep metadata, instead it only return the hash of the file given by the user.
* Remove no longer needed errors.
* Update golden tests  to use test vector from CIP 119
* Upgrate cardano-api-9.1.0.0
Jimbo4350
Jimbo4350 approved these changes Jul 19, 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

@carbolymer carbolymer added this pull request to the merge queue Jul 19, 2024
Merged via the queue into main with commit cace0e6 Jul 19, 2024
24 checks passed
@carbolymer carbolymer deleted the clr/drepmetadata branch July 19, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@smelc smelc smelc approved these changes

@carbolymer carbolymer carbolymer approved these changes

@newhoggy newhoggy Awaiting requested review from newhoggy

@palas palas Awaiting requested review from palas

@kevinhammond kevinhammond Awaiting requested review from kevinhammond

@disassembler disassembler Awaiting requested review from disassembler

Assignees
No one assigned
Labels
conway-feature DO NOT MERGE
Projects
None yet
Milestone
SanchoNet Phase 7 (Queries and extras)
Development

Successfully merging this pull request may close these issues.

[FR] - remove limit on DREP metadata
5 participants
@CarlosLopezDeLara @palas @carbolymer @smelc @Jimbo4350