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 CBOR instances for DefaultVote #4860

Merged
merged 1 commit into from
Feb 3, 2025
Merged

Add CBOR instances for DefaultVote #4860

merged 1 commit into from
Feb 3, 2025

Conversation

jasagredo
Copy link
Contributor

@jasagredo jasagredo commented Jan 29, 2025

Description

This is necessary for IntersectMBO/ouroboros-consensus#1371

Checklist

  • Commits in meaningful sequence and with useful messages
  • Tests added or updated when needed
  • CHANGELOG.md files updated for packages with externally visible changes

    New section is never added with the code changes. (See RELEASING.md)
  • Versions updated in .cabal and CHANGELOG.md files when necessary, according to the
    versioning process.
  • Version bounds in .cabal files updated when necessary

    If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • Code formatted (use scripts/fourmolize.sh)
  • Cabal files formatted (use scripts/cabal-format.sh)
  • hie.yaml updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@jasagredo jasagredo requested a review from a team as a code owner January 29, 2025 10:26
@jasagredo
Copy link
Contributor Author

I don't know exactly how you like to define CBOR instances. If the structure shall be different, do tell me.

Copy link
Contributor

@aniketd aniketd left a comment

Choose a reason for hiding this comment

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

I'm not sure about the CBOR instances, since we may need to consider whether non-default votes share any tag-space. Just left a comment about the CHANGELOG.

eras/conway/impl/CHANGELOG.md Outdated Show resolved Hide resolved
@jasagredo jasagredo force-pushed the js/cbor-defaultvote branch from a1b5663 to 5093b24 Compare January 29, 2025 11:01
@jasagredo jasagredo requested a review from aniketd January 29, 2025 11:02
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you!

@jasagredo jasagredo force-pushed the js/cbor-defaultvote branch from 5093b24 to e59a601 Compare January 31, 2025 09:59
@jasagredo
Copy link
Contributor Author

I used fail instead of error. I think this is ready to merge but I will leave to you ledger team to click the button 👍

@lehins lehins enabled auto-merge February 1, 2025 01:31
@lehins lehins force-pushed the js/cbor-defaultvote branch from e59a601 to 79a1b99 Compare February 2, 2025 23:51
@jasagredo
Copy link
Contributor Author

I think @aniketd has to dismiss his request for changes in order to merge this

@teodanciu teodanciu dismissed aniketd’s stale review February 3, 2025 13:31

Comments are addressed, dismissing in order to unblock merging.

@lehins lehins merged commit 513b568 into master Feb 3, 2025
544 of 586 checks passed
@lehins lehins deleted the js/cbor-defaultvote branch February 3, 2025 13:32
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.

3 participants