Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: feature strict encoding on monero types #90

Merged

Conversation

h4sh3d
Copy link
Contributor

@h4sh3d h4sh3d commented Jul 6, 2022

Add a feature monero, disable by default, implementing Strict Encode and Strict Decode on main Monero types from monero-rs/monero-rs.

h4sh3d added 2 commits July 6, 2022 14:51
Add a feature `monero`, disable by default, implementing Strict Encode
and Strict Decode on main Monero types from monero-rs/monero-rs.
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

I love your formatting, but it is not compatible with CI, so pls can you run cargo +nightly fmt --all. Also clippy has some issues. Otherwise LGTM - I did some comments but they are not obligatory (only if you'd like them).

Comment on lines 5 to 6
// Dr. Maxim Orlovsky <orlovsky@pandoracore.com>
// h4sh3d <h4sh3d@protonmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

It is just you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b5507de

impl StrictEncode for PaymentId {
#[inline]
fn strict_encode<E: io::Write>(&self, mut e: E) -> Result<usize, Error> {
Ok(e.write(&self.to_fixed_bytes())?)
Copy link
Member

Choose a reason for hiding this comment

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

We have impl StrictEncode for [T; const LEN], so you can just use self.to_fixed_bytes().strict_encode(e)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f7d1702

Comment on lines 203 to 205
let mut bytes = [0u8; 8];
d.read_exact(&mut bytes)?;
Ok(Self::from(bytes))
Copy link
Member

Choose a reason for hiding this comment

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

Ibid: you can just say StrictDecode::strict_decode(d).map(Self::from)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f7d1702

Comment on lines 212 to 213
self.major.strict_encode(&mut e)?;
self.minor.strict_encode(e)
Copy link
Member

Choose a reason for hiding this comment

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

There is a macro for that strict_encode_list!(e; self.major, self.minor)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f7d1702

Comment on lines 220 to 222
let major = u32::strict_decode(&mut d)?;
let minor = u32::strict_decode(d)?;
Ok(Self { major, minor })
Copy link
Member

Choose a reason for hiding this comment

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

Another macro strict_decode_self(d; major, minor)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f7d1702

Comment on lines 226 to 258
impl StrictEncode for KeyPair {
#[inline]
fn strict_encode<E: io::Write>(&self, mut e: E) -> Result<usize, Error> {
self.view.strict_encode(&mut e)?;
self.spend.strict_encode(e)
}
}

impl StrictDecode for KeyPair {
#[inline]
fn strict_decode<D: io::Read>(mut d: D) -> Result<Self, Error> {
let view = PrivateKey::strict_decode(&mut d)?;
let spend = PrivateKey::strict_decode(d)?;
Ok(Self { view, spend })
}
}

impl StrictEncode for ViewPair {
#[inline]
fn strict_encode<E: io::Write>(&self, mut e: E) -> Result<usize, Error> {
self.view.strict_encode(&mut e)?;
self.spend.strict_encode(e)
}
}

impl StrictDecode for ViewPair {
#[inline]
fn strict_decode<D: io::Read>(mut d: D) -> Result<Self, Error> {
let view = PrivateKey::strict_decode(&mut d)?;
let spend = PublicKey::strict_decode(d)?;
Ok(Self { view, spend })
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Here you can also use macros

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f7d1702

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Perfect, thank you! On last thing: we need to add to CI monero feature to ensure that it won't break. Also need to fix docs in one place.

@@ -48,6 +53,7 @@ pub trait Strategy {
/// - [`BitcoinConsensus`]
/// - [`Wrapped`]
/// - [`UsingUniformAddr`]
/// - [`MoneroConsensus`]
Copy link
Member

Choose a reason for hiding this comment

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

This will fail on docs compiling w/o monero feature. We need

Suggested change
/// - [`MoneroConsensus`]
#[cfg_attr(feature = "monero", doc = "- [`MoneroConsensus`]")]

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Oh, I see, the docs are always built with all features and there is no strict-encoding-crate specific feature builds. Merging now

@dr-orlovsky dr-orlovsky merged commit 7c9267b into LNP-BP:master Jul 8, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants