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

feat(rust/cardano-blockchain-types): Add CIP36 #125

Merged
merged 19 commits into from
Dec 31, 2024

Conversation

bkioshn
Copy link
Contributor

@bkioshn bkioshn commented Dec 27, 2024

Description

Move CIP36 validation and decoding out of cardano-chain-follower

Related Issue(s)

#121

Description of Changes

This CIP36 registration is divided into 3 part

  1. key_registration - 61284 decoding
  2. registration_witness - 61285 decoding
  3. Basic validation
  • Signature validation of the registration witness 61285 against the stake public key in key registration 61284.
  • Payment address network validation against the network. The given network should match the network tag within the payment address.
  • Purpose validation, the purpose should be 0 for Catalyst (whenis_strict_catalyst is true).
  • Voting keys validation, Catalyst supports only a single voting key per registration when is_strict_catalyst is true.

Additional change

  • add decode_helper.rs helper function for CBOR decoder

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@bkioshn bkioshn marked this pull request as draft December 27, 2024 08:02
@bkioshn bkioshn self-assigned this Dec 27, 2024
@bkioshn bkioshn added the enhancement New feature or request label Dec 27, 2024
Copy link
Contributor

github-actions bot commented Dec 27, 2024

Test Report | ${\color{lightgreen}Pass: 264/264}$ | ${\color{red}Fail: 0/264}$ |

@bkioshn bkioshn marked this pull request as ready for review December 31, 2024 02:04
@bkioshn bkioshn merged commit 3c9befe into fix/cardano-bc-types-base-change Dec 31, 2024
24 checks passed
@bkioshn bkioshn deleted the feat/cip36 branch December 31, 2024 11:32
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

These changes need to be made.

@@ -0,0 +1,3 @@
//! Utility functions.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make a new public library crate cbork-util move this module to it.
For now, its all that cbork-util will contain.

{
Ok(cip36_key_registration)
} else {
Err(decode::Error::message(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This not verbose enough.
We should not be keeping it a secret why we failed to validate. We know what the errors are, we should explicitly list them as this is an End User report. Not an internal report.

The original code was:

  // Validate that all keys required to be present in the CIP36 Metadata are present.
        if !found_keys.contains(&1) {
            validation_report.push(
                "The CIP36 Metadata Voting Key/Delegation is missing from the data.".to_string(),
            );
        }
        if !found_keys.contains(&2) {
            validation_report
                .push("The CIP36 Metadata Stake Address is missing from the data.".to_string());
        }
        if !found_keys.contains(&3) {
            validation_report
                .push("The CIP36 Metadata Payment Address is missing from the data.".to_string());
        }
        if !found_keys.contains(&4) {
            validation_report
                .push("The CIP36 Metadata Nonce is missing from the data.".to_string());
        }

And it needs to be at least this verbose.

match key {
Cip36KeyRegistrationKeys::VotingKey => {
if !found_keys.insert(key as u16) {
return Err(decode::Error::message(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do not fail fast, accumulate all possible errors in a validation_report like the original code intended to do.
This is to inform the user of all the reasons we failed to validate the metadata, and stopping on the first is unhelpful and frustrating UX.

apskhem pushed a commit that referenced this pull request Jan 9, 2025
…hange (#123)

* feat(rust): add cardano-blockchain-types crate

* fix(rust): Remove unused dependencies

* fix(cardano-blockchain-types): time_to_slot calculation

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): remove justfile

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): point new should take type Slot and Blake2bHash

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): Fork type

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): point and fuzzy point test

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): add Fork increment function

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): add comment on tag 259

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): add Fork decrement function

Signed-off-by: bkioshn <[email protected]>

* test(rust): try earthly no-cache

Signed-off-by: bkioshn <[email protected]>

* test(rust): try earthly no-cache and fix doc artifact

Signed-off-by: bkioshn <[email protected]>

* test(rust): remove no-cache

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): expose Fork and Network

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): add partailOrd to Fork

Signed-off-by: bkioshn <[email protected]>

* Update rust/cardano-blockchain-types/src/point.rs

Co-authored-by: Stanislav Tkach <[email protected]>

* fix(cardano-blockchain-types): cleanup

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): testdoc

Signed-off-by: bkioshn <[email protected]>

* Update rust/cardano-blockchain-types/src/point.rs

* fix(cardano-blockchain-types): cleanup

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): format

* fix(cardano-blockchain-types): add validate PR title

* fix(cardano-blockchain-types): comments

* fix(cardano-blockchain-types): fix hash_or_default

* fix(cardano-blockchain-types): redundant code

* test: no cache

* test: revert change

* test ci

* test ci

* test ci

* test ci

* test ci

* test ci

* test ci

* test ci

* test ci

* test ci

* revert change

* test ci

* revert change

* fix(rust/cardano-blockchain-types): add more functionality to `Slot` (#124)

* fix(cardano-blockchain-types): add more trait to slot

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): add serde serialize to slot

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): format

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): use sat_mul for slot

Signed-off-by: bkioshn <[email protected]>

---------

Signed-off-by: bkioshn <[email protected]>

* feat(rust/cardano-blockchain-types): Add CIP36 (#125)

* feat(cardano-blockchain-types): add cip36

Signed-off-by: bkioshn <[email protected]>

* feat(cardano-blockchain-types): add decode helper utils

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): fix type

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): add clone

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): fix type

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): check dup keys

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): fix linter

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): fix visibility and comment

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): add validation test

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): key registration decoding test

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): move voting pk to its own file

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): derive debug

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): use .context

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): fix getter

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): check required keys

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): network tag of payment addr

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): add constructor

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): linter

Signed-off-by: bkioshn <[email protected]>

---------

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): expose from_saturating (#131)

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): slot bigint conversion

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): txn index conversion

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): conversion

Signed-off-by: bkioshn <[email protected]>

* fix(rust/cardano-blockchain-types): fix CIP36 (#133)

* fix(cardano-blockchain-types): key registration

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): registration witness

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): voting pk

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): validation test

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 constructor

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): format

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): move cip36 to be under metadata

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): use decode_helper from cbork-utils

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): visibility, cip36 constructor

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 constructor

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): handle unset data

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 err report

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): update cbor-utils tag

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 now contain validation

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 validation implement getter

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): remove validation from cip36 struct

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): improve verifying key error log

Signed-off-by: bkioshn <[email protected]>

---------

Signed-off-by: bkioshn <[email protected]>

* fix(rust/cardano-blockchain-types): implement new error report for CIP36 (#142)

* fix(cardano-blockchain-types): key registration

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): registration witness

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): voting pk

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): validation test

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 constructor

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): format

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): move cip36 to be under metadata

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): use decode_helper from cbork-utils

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): visibility, cip36 constructor

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 constructor

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): handle unset data

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 err report

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): update cbor-utils tag

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 now contain validation

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 validation implement getter

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): remove validation from cip36 struct

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): improve verifying key error log

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): implement new error report

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): function name

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): print report

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): version for catalyst-types

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): move found key check to its own function

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): pass cip36 ProblemReport as context

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): format

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): key reg redundant code and missing key checking

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): remove cip36validation struct

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): add more fileds, fix return error

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): add cip36_from_block function

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): remove serde_json

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): format

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): impl display for cip36 error

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): cip36 key reg function

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): problem report conversion err

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): impl display for cip36

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): err report getter

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): update catalyst-types tag

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): return type for cip36 constructor

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): logic

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): format

Signed-off-by: bkioshn <[email protected]>

* fix(cardano-blockchain-types): rename ctx

Signed-off-by: bkioshn <[email protected]>

---------

Signed-off-by: bkioshn <[email protected]>

---------

Signed-off-by: bkioshn <[email protected]>
Co-authored-by: Steven Johnson <[email protected]>
Co-authored-by: Steven Johnson <[email protected]>
Co-authored-by: Stanislav Tkach <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants