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

bump version to 0.21.3 #390

Merged
merged 1 commit into from Feb 23, 2022

Conversation

apoelstra
Copy link
Member

We've got a ton of minor changes in, plus fixing the Parity type and adding some extra serde impls. Let's push a minor version out so that we can move on to updating the upstream libsecp.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

For what its worth, LGTM. Although I'm not sure its worth having links to all those docs changes, they are all pretty trivial.

I checked the markdown renders correctly (on GitHub) and that all the links work.

@apoelstra
Copy link
Member Author

Thanks! Normally I wouldn't bother linking to all the docs stuff, but there were a bunch and I could make the links pretty small.

sanket1729
sanket1729 previously approved these changes Feb 2, 2022
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

utACK 61b6add. Looks good to me. Did not verify the release notes.

@apoelstra
Copy link
Member Author

cc @elichai @TheBlueMatt could one of you ack this so I can merge and publish?

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 8, 2022

Note that the Parity change is theoretically breaking change because someone could write this:

let Parity { .. } = some_fn_returning_parity();

(or in other pattern matching)

Which works before and breaks after changing to enum. I know, it's dumb, I posted about this in IRLO recently. My personal approach for my libs is "doing this is too weird and hopefully unlikely and trivial to fix, if you do this sorry-not sorry, I'm making non-breaking release".

While I'm in favor of ignoring this I believe it's the right thing for me to point this out as others may have a different view.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 8, 2022

One more thing, would you be up for ninja-doc-improvement of Parity before release? I now noticed the conversions number -> Parity are totally unclear - what values mean what? Could some panic?

@apoelstra
Copy link
Member Author

@Kixunil sure, go for it.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 9, 2022

Done. While working on it I noticed a few other problems:

  • Parity is serialized as i32 - this may waste space in binary encodings. I think this is significant, do we want this?
  • The serde error message is prefixed with "Expecting" which is wrong.
  • It'd be nicer to map parity error as invalid_value in the Deserialize impl.

@apoelstra
Copy link
Member Author

Oof, we should definitely fix the parity to be serialized as a single byte. I guess we can't do that in a minor release though.

The error stuff I think is ok to fix in a minor release, if you are so inclined.

But bear in mind that my intention is to do a major release basically ASAP after I get this merged, and to use the major release in the next rust-bitcoin release. Mainly I want #384 but I'm happy to mess with the Parity API too.

apoelstra added a commit that referenced this pull request Feb 9, 2022
705c9cf Clarified conversions between `Parity` and integers (Martin Habovstiak)

Pull request description:

  This was discussed in #390 (comment)

ACKs for top commit:
  apoelstra:
    ACK 705c9cf

Tree-SHA512: 3ba2ec566099c3c6d1c6f830e4959312b818b8766d924e3d995e6b23bd196ab747cc03d46f494ef451569188b0163f53e3236cacd20bfae9118ee76bcdbc9c02
@Kixunil
Copy link
Collaborator

Kixunil commented Feb 9, 2022

OK, will try to be quick and submit two PRs.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 9, 2022

I found one more thing: Error doesn't carry the original value which can be annoying for debugging. Also it's not great that it's the same Error type as others while it can only return one variant. (Same stuff I want to solve at bitcoin.)

@sanket1729
Copy link
Member

@Kixunil @apoelstra, anything I can help in unblocking this?

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 11, 2022

@sanket1729 I'm done with my PRs to this one.

elichai
elichai previously approved these changes Feb 15, 2022
Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

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

ACK 61b6add

@apoelstra
Copy link
Member Author

We need #407 first. Sorry, my bad.

In the meantime I'll update the changelog here assuming that we have 407.

@apoelstra
Copy link
Member Author

@real-or-random @elichai can I get an ACK on this?

Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

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

ACK

@apoelstra apoelstra merged commit ce255fd into rust-bitcoin:master Feb 23, 2022
@apoelstra apoelstra deleted the 2022-01--secp-0.21.3 branch February 23, 2022 18:50
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