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

feat: add Borsh #150

Merged
merged 3 commits into from Nov 23, 2022
Merged

feat: add Borsh #150

merged 3 commits into from Nov 23, 2022

Conversation

Cifko
Copy link
Contributor

@Cifko Cifko commented Nov 15, 2022

Add Borsh(De)Serialize for keys and signatures.

Copy link
Contributor

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

This also needs to be behind a borsh feature, as I mentioned in tari_utilities

@CjS77
Copy link
Contributor

CjS77 commented Nov 15, 2022

This also needs to be behind a borsh feature, as I mentioned in tari_utilities

+1 on this

Will have to use a pattern similar to:

use std::fmt::{Debug, Display};

pub struct Foo<T> {
    bar: T
}

impl<T> Foo<T> 
where
 T: Debug,
 {
     pub fn baz(&self) {
         print!("{:?}", self.bar)
     }
 }
 
#[cfg(feat)]
impl<T> Foo<T> 
where
 T: Debug,
 T: Display, 
 {
     pub fn baz2(&self) {
         print!("{}", self.bar)
     }
 }

@Cifko Cifko force-pushed the add-borsh2 branch 3 times, most recently from e1cce8d to 8295bd1 Compare November 18, 2022 13:13
CjS77
CjS77 previously approved these changes Nov 21, 2022
Copy link
Contributor

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

lgtm

src/keys.rs Outdated
@@ -40,6 +42,49 @@ pub trait SecretKey: ByteArray + Clone + PartialEq + Eq + Add<Output = Self> + D
/// implementations need to implement this trait for them to be used in Tari.
///
/// See [SecretKey](trait.SecretKey.html) for an example.
/// cbindgen:ignore
#[cfg(feature = "borsh")]
pub trait PublicKey:
Copy link
Member

@sdbondi sdbondi Nov 22, 2022

Choose a reason for hiding this comment

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

I think having a different trait depending on whether the borsh feature flag is enabled or not could be troublesome.

Any reason we require these trait bounds in the trait itself? They can always be specified where needed.

@sdbondi
Copy link
Member

sdbondi commented Nov 22, 2022

Suggest these changes:
crypto.diff.txt

@stringhandler stringhandler merged commit 4a4b633 into tari-project:main Nov 23, 2022
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

4 participants