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

Fix generated textEnvelope type on registering a Stake Address Registration Certificate #390

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

felix-lipski
Copy link
Contributor

@felix-lipski felix-lipski commented Nov 29, 2023

Changelog

- description: |
    split cases in textEnvelopeType
    account for eras after conway using forEraInEon
# 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
  # - improvement    # QoL changes e.g. refactoring
  - 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

Fixes the issue #465 in cardano-cli

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

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

@felix-lipski felix-lipski force-pushed the felix-lipski/fix/cert-conway-envelope-type branch from 919681c to f41ac38 Compare November 29, 2023 14:23
@felix-lipski felix-lipski marked this pull request as ready for review November 29, 2023 15:55
@felix-lipski felix-lipski changed the title Draft: Fix generated textEnvelope type on registering a Stake Address Registration Certificate Fix generated textEnvelope type on registering a Stake Address Registration Certificate Nov 29, 2023
@smelc
Copy link
Contributor

smelc commented Nov 30, 2023

LGTM (nice usage of forEraInEon 👏), but could we had a golden test to make sure we keep track of this bug?

Relevant doc to write the tests:

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 🎉. Can you squash your commits?

@felix-lipski felix-lipski force-pushed the felix-lipski/fix/cert-conway-envelope-type branch from 9b7a015 to 0913bd2 Compare November 30, 2023 11:52
@gitmachtl
Copy link
Contributor

gitmachtl commented Dec 1, 2023

did a test with cli8.16.0.1 by manually changing the textEnvelope type to CertificateConway for a deregistration certificate. resulted in an error during the transaction build-raw action:

TextEnvelope type error: Expected: CertificateShelley Actual: CertificateConway

so it looks like the cli is not aware of this type name yet for processing a deregistration certificate. and i guess its the same for the registration certificate aswell.

@felix-lipski
Copy link
Contributor Author

LGTM (nice usage of forEraInEon 👏), but could we had a golden test to make sure we keep track of this bug?

Relevant doc to write the tests:

Golden tests done in cli: IntersectMBO/cardano-cli#499

@felix-lipski felix-lipski force-pushed the felix-lipski/fix/cert-conway-envelope-type branch 2 times, most recently from c336d3c to 206f22a Compare December 6, 2023 16:08
@felix-lipski felix-lipski force-pushed the felix-lipski/fix/cert-conway-envelope-type branch from 206f22a to ef62e85 Compare December 7, 2023 10:51
@felix-lipski felix-lipski added this pull request to the merge queue Dec 7, 2023
Merged via the queue into main with commit f912683 Dec 7, 2023
22 checks passed
@felix-lipski felix-lipski deleted the felix-lipski/fix/cert-conway-envelope-type branch December 7, 2023 13:22
smelc added a commit to IntersectMBO/cardano-cli that referenced this pull request Jan 2, 2024
newhoggy added a commit that referenced this pull request Mar 11, 2024
…ano-api-8.27.0.0

Upgrade to `cardano-api-8.27.0.0`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants