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.

Sign up for GitHub

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

Jump to bottom

Create Party enum and deprecate ElligatorSwiftParty in favor of it #752

Merged
merged 2 commits into from
Oct 17, 2024
Merged

Create Party enum and deprecate ElligatorSwiftParty in favor of it #752

merged 2 commits into from
Oct 17, 2024

Conversation

shinghim
Copy link
Contributor

The initial naming of ElligatorSwiftParty wasn't very descriptive, so it will be deprecated in favor of a more descriptive Party enum. I updated shared_secret and shared_secret_with_hasher to accept the new Party enum as well - I'm not sure if there's a better way to do it, but changing it to an impl Into<Party> should preserve backwards compatibility

Fixes #741

@shinghim shinghim force-pushed the 741-elligator branch 2 times, most recently from 8337acc to c83c5a7 Compare October 15, 2024 02:39
apoelstra
apoelstra reviewed Oct 15, 2024
View reviewed changes
src/ellswift.rs Outdated Show resolved Hide resolved
apoelstra
apoelstra reviewed Oct 15, 2024
View reviewed changes
src/ellswift.rs Outdated
pub enum ElligatorSwiftParty {
/// We are the initiator of the ECDH
A,
/// We are the responder of the ECDH
B,
}

#[allow(deprecated)]
#[allow(dead_code)] // We aren't using this anymore in this library, but users might be using it
Copy link
Member

Choose a reason for hiding this comment

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

In c83c5a7:

How can users be using this if it's a private method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what i was thinking - removed this bit

Copy link
Member

Choose a reason for hiding this comment

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

This is not resolved. Now this is dead code and is causing a local CI failure for me.

Copy link
Member

Choose a reason for hiding this comment

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

In 3b7d393

@apoelstra
Copy link
Member

Also, we should consider backporting this to the version used by rust-bitcoin 0.32.

@tcharding
Copy link
Member

This PR is doing two things, one of which is not in the title so is obfuscated. Please either do two separate PRs or describe the whole change set (probably still requires two patches).

@shinghim shinghim marked this pull request as draft October 16, 2024 01:49
@shinghim shinghim force-pushed the 741-elligator branch 2 times, most recently from 86fe99a to e04237b Compare October 16, 2024 02:13
@shinghim shinghim changed the title Deprecate ElligatorSwiftParty in favor of Party Create Party enum and deprecate ElligatorSwiftParty in favor of it Oct 16, 2024
@shinghim shinghim force-pushed the 741-elligator branch 3 times, most recently from d9d378e to e30f8f8 Compare October 16, 2024 02:22
@shinghim
Create Party enum
3b7d393 
Introducing this enum as a replacement for ElligatorSwiftParty. Party is
more descriptive and should cause less confusion than
ElligatorSwiftParty
@shinghim
Deprecate ElligatorSwiftParty in favor of Party
bf2c35c
@shinghim shinghim marked this pull request as ready for review October 16, 2024 02:53
@shinghim
Copy link
Contributor Author

@tcharding done

tcharding
tcharding approved these changes Oct 17, 2024
View reviewed changes
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.

ACK bf2c35c

@tcharding
Copy link
Member

The deprecation stuff is particularly nice, well thought out.

@apoelstra
Copy link
Member

apoelstra commented Oct 17, 2024 •
edited
Loading

3b7d393 is still in the PR. lemme check why clippy is running locally on this commit even though it's not the tip..

apoelstra
apoelstra approved these changes Oct 17, 2024
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 3b7d393; successfully ran local tests

apoelstra
apoelstra approved these changes Oct 17, 2024
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 3b7d393; successfully ran local tests

apoelstra
apoelstra approved these changes Oct 17, 2024
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK bf2c35c; successfully ran local tests

@apoelstra
Copy link
Member

Lol, there we go, I got it.

I had been sorting commits alphabetically instead of topologically when deciding what the "tip" was.

@apoelstra apoelstra merged commit 36be55f into rust-bitcoin:master Oct 17, 2024
30 checks passed
This was referenced Oct 17, 2024
Merged
Open
apoelstra added a commit that referenced this pull request Oct 18, 2024
@apoelstra
Merge #753: Backport #752 to 0.29.x
3c95e1d 
f12bde6 Deprecate `ElligatorSwiftParty` in favor of `Party` (Shing Him Ng)
33fda15 Create `Party` enum (Shing Him Ng)

Pull request description:

  Also, we should consider backporting this to the version used by rust-bitcoin 0.32.

  _Originally posted by apoelstra in #752 (comment)

  Backport #752 to the [version used by rust-bitcoin 0.32](https://github.com/rust-bitcoin/rust-bitcoin/blob/7af9e33f2b9033cf2701725eba280e14ebda0cf5/bitcoin/Cargo.toml#L35)

ACKs for top commit:
  apoelstra:
    ACK f12bde6; successfully ran local tests

Tree-SHA512: e8184c0df1f19a6512b1168bb1cf49e906de6d7f51ef1f9a4e3977422c36e603c3325fedb1485efa49ea8cb0361b54a293cdfefef10f3370541c8086b2b28bff
@shinghim shinghim deleted the 741-elligator branch October 19, 2024 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcharding tcharding tcharding approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ElligatorSwift "A/B" enum should have better-named variants
3 participants
@shinghim @apoelstra @tcharding