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

refactor(signer): Add enum for version-specific data to BlockInfo #4981

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

jbencin
Copy link
Contributor

@jbencin jbencin commented Jul 19, 2024

Description

Add enum for version-specific (Signer v0 or v1) data to BlockInfo

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@jbencin jbencin requested review from a team as code owners July 19, 2024 20:45
@jbencin jbencin requested review from a team and removed request for a team July 19, 2024 20:46
obycode
obycode previously approved these changes Jul 23, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM!

@saralab saralab requested review from jferrant and kantai July 23, 2024 14:02
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

I like the idea of using an enum for version specific data here, but I think this breaks v0 at the moment: signed_over is used to indicate whether or not a block has been signed by the local node. Maybe it would make sense to add a new field to the v0 enum for this, or maybe the signed_over field should just move back into the common struct.

stacks-signer/src/signerdb.rs Outdated Show resolved Hide resolved
stacks-signer/src/signerdb.rs Outdated Show resolved Hide resolved
@jbencin jbencin enabled auto-merge July 24, 2024 21:34
@jbencin jbencin added this pull request to the merge queue Jul 24, 2024
Merged via the queue into stacks-network:develop with commit 92e404c Jul 24, 2024
1 check passed
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Oct 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants