Skip to content

Conversation

@stevenj
Copy link
Collaborator

@stevenj stevenj commented Jan 19, 2025

Description

Enhances and generalizes the Catalyst Key ID to a Catlayst ID which can be used in different contexts interchangably.

Description of Changes

See docs in PR

Breaking Changes

Yes, totally different type.
Original type removed.

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

@stevenj stevenj added enhancement New feature or request review me PR is ready for review F14 labels Jan 19, 2025
@stevenj stevenj self-assigned this Jan 19, 2025
@stevenj stevenj changed the title Upgrade the Catalyst Key Id to be a more general Catalyst ID which can be used both as a Key ID and a registration ID. feat(rust): Upgrade the Catalyst Key Id to be a more general Catalyst ID which can be used both as a Key ID and a registration ID. Jan 19, 2025
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@stevenj stevenj added do not merge yet PR is not ready to be merged yet requires architect review Requires at least 1 architect to sign off on PR before merge. labels Jan 20, 2025
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

Mr-Leshiy
Mr-Leshiy previously approved these changes Jan 20, 2025
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Contributor

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

Copy link
Contributor

@neil-iohk neil-iohk left a comment

Choose a reason for hiding this comment

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

Just one item, around validation of nonce, the two separate paths have different validation behavior which could lead to inconsistent state, validation only happens during parsing, direct construction with with_specific_nonce bypasses validation and subsequent serialization would not catch invalid values.

@stevenj stevenj dismissed stale reviews from stanislav-tkach and Mr-Leshiy via fc39558 January 21, 2025 15:56
@github-actions
Copy link
Contributor

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

Mr-Leshiy
Mr-Leshiy previously approved these changes Jan 23, 2025
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@stevenj stevenj merged commit aabb994 into main Jan 24, 2025
22 checks passed
@stevenj stevenj deleted the feat/cat-kid-to-cat-id branch January 24, 2025 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge yet PR is not ready to be merged yet enhancement New feature or request F14 requires architect review Requires at least 1 architect to sign off on PR before merge. review me PR is ready for review

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants