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 JSON schema checking functionality for DRep and Gov action metadata #995

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

palas
Copy link
Contributor

@palas palas commented Dec 20, 2024

Changelog

- description: |
    Added schema checking functionality for DRep registration, DRep update, and GovAction metadata, based on CIP-0100, CIP-0108, and CIP-0119. Updated tests. Refactored test data for improved code clarity.
  type:
  - feature
  - refactoring
  - test

Context

The objective is addressing this issue: #906

This PR is based on cardano-api PR #713, which provides the required functionality, and this PR should not be merged until the cardano-api PR gets released and the source repo stanza is removed.

How to trust this PR

I think, checking the changes to the tests here is key, and the main part. The actual functionality is in cardano-api PR #713 (I would check that one together with this). The latest commit should also be pure refactoring.

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 DO NOT MERGE next-update Needs a dependency to be updated to be merged labels Dec 20, 2024
@palas palas self-assigned this Dec 20, 2024
@palas palas linked an issue Dec 20, 2024 that may be closed by this pull request
@palas palas changed the title Gov action schema checking Add JSON schema checking functionality for DRep and Gov action metadata Dec 20, 2024
@palas palas force-pushed the gov-action-schema-checking branch 2 times, most recently from 92bd7ee to 18fc533 Compare December 20, 2024 15:36
@gitmachtl
Copy link
Contributor

gitmachtl commented Dec 21, 2024

I just took a look at the testfiles and i noticed the one which has the authors signed with method "cip-0008".

The signature provided in this example is the COSE_SIGN1, ok. But the publicKey is not really the COSE_KEY, its only the public key. As CIP-0008 supports actually different methods, do we simply demand that the

COSE_Key map label '1' (kty) is '1' (OKP)
COSE_Key map label '3' (alg) is '-8' (EdDSA)
COSE_Key map label '-1' (crv) is '6' (Ed25519)

?

If so, is this written somewhere in the CIP-100 (and dependencies) ?

Or should in the case of using method "cip-0008" the publickey actually be the COSE_Key ?

@Ryun1 @CarlosLopezDeLara

@palas palas force-pushed the gov-action-schema-checking branch from b33d89f to b3830df Compare February 12, 2025 12:46
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.

Nothing in the Haskell code indicates what the CIP is and links that to what is being checked/modified as a result of the CIP. We are setting ourselves up for failure when there are potentially dozens of cips and it's not easily verifiable as to what we have/have not implemented. Propose a solution to this and lets talk about it before you propagate it.

GovernanceActionsMismatchedHashError checkType (L.anchorDataHash anchor) hash
TrustHash -> pure ()
carryHashChecksWrapper checkHash anchor checkType =
firstExceptT (GovernanceActionsHashCheckError checkType) $
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm looking forward to wrapper code like this disappearing when we adopt RIO.

@gitmachtl
Copy link
Contributor

Not sure how useful it is putting time into this right now. There are a lot of ongoing discussion currently to change/edit/add stuff in der governance metadata CIPs. F.e. to make it more resistent against a replay attack for signed metadata.

So not sure what checks are done in the cli about those metadata jsons, and if it makes sense to continue integrating this.

Most users are using 3rd party tools anyway for this.

@palas
Copy link
Contributor Author

palas commented Feb 14, 2025

Nothing in the Haskell code indicates what the CIP is and links that to what is being checked/modified as a result of the CIP. We are setting ourselves up for failure when there are potentially dozens of cips and it's not easily verifiable as to what we have/have not implemented. Propose a solution to this and lets talk about it before you propagate it.

It would be ideal to have a way of validate the JSON schemas directly. I was trying that at the beginning, but JSON schema primitives are written quite informally and in natural language. And I couldn't find robust validators, from my view. It may be worth writing our own validator and give it some base implementations for native values. It may be less work than implementing validators for every CIP.

For getting a measure of completion, we could scan the CIPs for JSON schemas. But it is tricky to map them to actual artifacts. In fact, I am not sure that it is that well defined in the CIPs for now. We could try to request some structure from the CIPs that unequivocally identifies the artifacts they affect. But we would need some list that identifies all possible artifacts and gives them some unique identifier that can be mentioned in the CIP.

@palas
Copy link
Contributor Author

palas commented Feb 14, 2025

Not sure how useful it is putting time into this right now. There are a lot of ongoing discussion currently to change/edit/add stuff in der governance metadata CIPs. F.e. to make it more resistent against a replay attack for signed metadata.

So not sure what checks are done in the cli about those metadata jsons, and if it makes sense to continue integrating this.

Most users are using 3rd party tools anyway for this.

Maybe it is not the best time for this, then.

@Jimbo4350, should we close this PR and park the task for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request next-update Needs a dependency to be updated to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Governance metadata is not validated
3 participants