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

Should we have CompressedPublicKey type? #826

Closed
Kixunil opened this issue Feb 10, 2022 · 18 comments
Closed

Should we have CompressedPublicKey type? #826

Kixunil opened this issue Feb 10, 2022 · 18 comments
Labels
1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release brainstorm code quality Makes code easier to understand and less likely to lead to problems error handling Issues and PRs related to error handling, mainly error refactoring epic help wanted

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 10, 2022

PublicKey tracks whether it's compressed dynamically which leads to error checking when creating wpubkey_hash.
Having a separate type for it would push the responsibility to the caller which might be able to statically guarantee compressed key.
However it's quite a bit of added API surface just to remove a few error paths. Is it worth it?

@Kixunil Kixunil added 1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release brainstorm code quality Makes code easier to understand and less likely to lead to problems error handling Issues and PRs related to error handling, mainly error refactoring epic help wanted labels Feb 10, 2022
@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 10, 2022

Note that we could easily support both in relevant functions:

#[non_exhaustive]
struct UncompressedPublicKey;

impl TryFrom<PublicKey> for CompressedPublicKey { 
    type error = UncompressedPublicKey;
    ...
}

pub fn p2wpkh<Pk: TryInto<CompressedPublicKey>>(pk: Pk, network: Network) -> Result<Address, Pk::Error> {
...
}

@apoelstra
Copy link
Member

It would remove more than a few error paths in rust-miniscript.

I'm in favor of this in principle. I think we've tried it in the past and it was hard to make workable. Maybe with 1.41.0 and TryFrom/TryInto it'll be more ergonomic.

@dr-orlovsky
Copy link
Collaborator

We have a lot of previous discussions on the topic. Just for the reference few of them:

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 18, 2022

@dr-orlovsky thanks for references! After reading them I think people are worried about type blow up. I think a good compromise would be having CompressedPublicKey and PublicKey, no UncompressedPublicKey. AFAIK uncomressed keys are only used in contexts where both are usable but there are places where only compressed is accepted.

@tcharding
Copy link
Member

tcharding commented Feb 28, 2022

What is the intended meaning of the 'help wanted' label @Kixunil? (In general I mean, not just for this issue.)

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 28, 2022

AFAIK it's asking for outside help (in this case feedback on API).

However maybe we should have a special label for requesting API feedback. Maybe even some event source for API consumers to subscribe to.

@tcharding
Copy link
Member

However maybe we should have a special label for requesting API feedback. Maybe even some event source for API consumers to subscribe to.

Sounds like another useful thing to do on a mailing list, any project maintainer of a crate that depends on rust-bitcoin stack can then subscribe and share feedback/ideas.

@dr-orlovsky
Copy link
Collaborator

@tcharding can't this be done with GitHub discussions? Just worried about multiplying communication channels

@tcharding
Copy link
Member

AFAICT GitHub discussions is per repository, I think a communication channel that is for all Bitcoin projects that are written in Rust (specifically all the ones that depend on crates in this organisation) would be useful. I'm pushing for a rust-bitcoin-dev mailing list.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Mar 14, 2022

I don't see a big problem with (ab)using the bitcoin crate for all of them.

@tcharding
Copy link
Member

Fair point, and I suppose I can get notifications from github discussions via email.

@dr-orlovsky
Copy link
Collaborator

BTW it seems I have seen discussions at the org level

@tcharding
Copy link
Member

I don't see it, I looked at an organisation of which I'm the 'owner' (or creator or whatever) and I couldn't see it either? Are you sure of this?

@dr-orlovsky
Copy link
Collaborator

Screen Shot 2022-03-18 at 12 12 06

@tcharding
Copy link
Member

Yeah I saw teams but thought that wasn't suitable to us because its by definition exclusive, we want to include random contributors. Feels like teams is aimed at private, corporate, dev teams.

@dr-orlovsky
Copy link
Collaborator

@tcharding actually discussions on the org level are already in beta: community/community#13192

This is not the same as a team discussions. I think it could be a nice replacement for a mail list.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 15, 2022

We should also have CompressedPrivateKey (naming is not great since it's actually public key compresed but can't think of better) that should be produced by BIP32 APIs.

@tcharding
Copy link
Member

Done in #2277

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release brainstorm code quality Makes code easier to understand and less likely to lead to problems error handling Issues and PRs related to error handling, mainly error refactoring epic help wanted
Projects
None yet
Development

No branches or pull requests

4 participants