-
Notifications
You must be signed in to change notification settings - Fork 79
Introduce Payjoin version enum #668
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,3 +2,5 @@ | |
| //! | ||
| //! This module contains types and methods used to implement Payjoin. | ||
| pub mod error; | ||
|
|
||
| pub mod version; | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,47 @@ | ||||
| use core::fmt; | ||||
|
|
||||
| use serde::{Serialize, Serializer}; | ||||
|
|
||||
| /// The Payjoin version | ||||
| /// | ||||
| /// From [BIP 78](https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#optional-parameters), | ||||
| /// the supported versions should be output as numbers: | ||||
| /// | ||||
| /// ```json | ||||
| /// { | ||||
| /// "errorCode": "version-unsupported", | ||||
| /// "supported" : [ 2, 3, 4 ], | ||||
| /// "message": "The version is not supported anymore" | ||||
| /// } | ||||
| /// ``` | ||||
| /// | ||||
| /// # Note | ||||
| /// - Only [`Serialize`] is implemented for json array serialization in the `unsupported-version` error message, | ||||
| /// not [`serde::Deserialize`], as deserialization is not required for this type. | ||||
| /// - [`fmt::Display`] and [`fmt::Debug`] output the `u8` representation for compatibility with BIP 78/77 | ||||
| /// and to match the expected wire format. | ||||
| #[repr(u8)] | ||||
| #[derive(Clone, Copy, PartialEq, Eq)] | ||||
| pub enum Version { | ||||
| /// BIP 78 Payjoin | ||||
| One = 1, | ||||
| /// BIP 77 Async Payjoin | ||||
| Two = 2, | ||||
| } | ||||
|
|
||||
| impl fmt::Display for Version { | ||||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { (*self as u8).fmt(f) } | ||||
| } | ||||
|
|
||||
| impl fmt::Debug for Version { | ||||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Display::fmt(self, f) } | ||||
| } | ||||
|
|
||||
| impl Serialize for Version { | ||||
| fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||||
| where | ||||
| S: Serializer, | ||||
| { | ||||
| (*self as u8).serialize(serializer) | ||||
| } | ||||
| } | ||||
|
Comment on lines
+40
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this not missing a matching deserialize impl?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
rust-payjoin/payjoin/src/receive/error.rs Line 220 in eb26494
is it okay to leave out until we are actually using it?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! very well. In this case, please document the omission and design decisions on the |
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change strictly necessary? for
Versioncompilation i.e. all_core-enabled compilations? If so, shouldn't the redundant explicit feature be removed fromv2 = ["serde"...]be removed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's needed for
Serialize- I'll remove the redundant feature inv2There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And is that serde::Serialize required for the
v1feature used exclusively?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From BIP-77:
If we aren't using the
vparameter, we can probably keep it tov1exclusivelyThere was a problem hiding this 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 our code reflects that suggested omission yet, and it also seems based on your later comment that
serdeis still required for the error. If for some reason that error that serializes a version array never gets hit, v1 exclusivity would be great. If not, it's ok to keep in core.