add feature for Invoice ser/de #1548

NicolaLS · 2022-06-16T20:17:31Z

Add feature "serde_invoice" for conditional dependency on serde to serialize/deserialize invoice

codecov-commenter · 2022-06-16T20:46:27Z

Codecov Report

Merging #1548 (6c15de1) into main (c180ddd) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1548      +/-   ##
==========================================
- Coverage   90.96%   90.93%   -0.04%     
==========================================
  Files          80       80              
  Lines       43533    43610      +77     
  Branches    43533    43610      +77     
==========================================
+ Hits        39599    39655      +56     
- Misses       3934     3955      +21

Impacted Files	Coverage Δ
lightning-invoice/src/lib.rs	`87.39% <ø> (ø)`
lightning/src/chain/onchaintx.rs	`93.98% <0.00%> (-0.93%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.89% <0.00%> (-0.26%)`	⬇️
lightning/src/routing/scoring.rs	`97.08% <0.00%> (ø)`
lightning/src/ln/channelmanager.rs	`84.38% <0.00%> (+0.04%)`	⬆️
lightning/src/routing/gossip.rs	`91.94% <0.00%> (+0.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c180ddd...6c15de1. Read the comment docs.

TheBlueMatt · 2022-06-16T21:07:13Z

I'm not sure I fully understand the motivation for this - you're adding methods to do the serialization not implementing serde traits, so it seems like its the same amount of code to just call the existing string-serialization methods?

NicolaLS · 2022-06-17T04:00:40Z

NicolaLS · 2022-06-17T04:05:42Z

I'm not sure I fully understand the motivation for this - you're adding methods to do the serialization not implementing serde traits, so it seems like its the same amount of code to just call the existing string-serialization methods?

(accidentally closed) Hi, thank you for the reply, I'm not sure if I understand. You mean to serialize as string I don't need to use serializer.serialize_str(invoice.to_string().as_str()) but can use serializer.collect_str(&format_args!("{}", invoice)) because Invoice already has the Display trait to serialize ? The motivation for this is that people who need to build data structures also containing an Invoice can simply #[serde(with = "serde_invoice")] and don't need to write the module themself.

NicolaLS · 2022-06-17T21:27:10Z

I'm not sure I fully understand the motivation for this - you're adding methods to do the serialization not implementing serde traits, so it seems like its the same amount of code to just call the existing string-serialization methods?

Ok now someone explained it to me. Does my approach make sense now ? (just force pushed)

TheBlueMatt

Right, this makes more sense now. I'm still kinda curious what your use-case is - I assume you're storing invoices in some larger struct and want to serde it?

TheBlueMatt · 2022-06-18T14:02:35Z

lightning-invoice/Cargo.toml


 [dev-dependencies]
 lightning = { version = "0.0.108", path = "../lightning", default-features = false, features = ["_test_utils"] }
 hex = "0.4"
+serde = { version = "1.0.118", features = ["derive"]}
+serde_json = "1.0.81"


Is this used?

Oh my bad I used it in the doc tests in the earlier version and forgot to delete it

NicolaLS · 2022-06-18T14:19:04Z

I am currently working on a JSON-2.0 RPC for minimint. using it like this:
https://github.com/fedimint/minimint/pull/55/files#diff-ada820502ef583047ac9262cc7d894a6be849b896451d190e747ae1b19fa0fa2R278-R282
https://github.com/fedimint/minimint/pull/55/files#diff-a8408d213ca89969480cabf47c07feef29f344c37fe9562dfb2f9c73a9fba9a7R275-R280

TheBlueMatt

LGTM, basically.

TheBlueMatt · 2022-06-18T15:30:01Z

lightning-invoice/Cargo.toml

@@ -17,6 +17,7 @@ rustdoc-args = ["--cfg", "docsrs"]
 default = ["std"]
 no-std = ["hashbrown", "lightning/no-std", "core2/alloc"]
 std = ["bitcoin_hashes/std", "num-traits/std", "lightning/std", "bech32/std"]
+serde_invoice = ["serde"]


Should we bother specifying the feature here? Even if we don't put it here users can use serde as a feature to turn on the dependency.

TheBlueMatt · 2022-06-18T15:30:16Z

lightning-invoice/src/lib.rs

@@ -35,6 +35,8 @@ extern crate secp256k1;
 extern crate alloc;
 #[cfg(any(test, feature = "std"))]
 extern crate core;
+#[cfg(feature = "serde_invoice")]


Lets use the implicit serde feature instead of serde_invoice for the cfg tags.

OK (new to rust so I thought there has to be a name)

TheBlueMatt · 2022-06-18T19:00:18Z

LGTM, this should get some kinda test, though.

tnull

Generally looks good, mod two nits. 👍

tnull · 2022-06-20T08:34:16Z

lightning-invoice/src/lib.rs

 #[cfg(test)]
 mod test {
 	use bitcoin_hashes::hex::FromHex;
 	use bitcoin_hashes::sha256;
+	use Invoice;


nit: No need to import this.

tnull · 2022-06-20T08:35:43Z

lightning-invoice/src/lib.rs

+	#[test]
+	fn test_serde() {
+		let invoice = "lnbc100p1psj9jhxdqud3jxktt5w46x7unfv9kz6mn0v3jsnp4q0d3p2sfluzdx45tqcs\
+							h2pu5qc7lgq0xs578ngs6s0s68ua4h7cvspp5q6rmq35js88zp5dvwrv9m459tnk2zunwj5jalqtyxqulh0l\


nit: These lines shouldn't indented as much. :)

tnull · 2022-06-20T08:41:56Z

lightning-invoice/src/lib.rs

+		let serialized_invoice = serde_json::to_string(&invoice).unwrap();
+		let deserialized_invoice: Invoice = serde_json::from_str(serialized_invoice.as_str()).unwrap();
+		assert_eq!(invoice, deserialized_invoice);
+		assert_eq!(invoice.to_string(), deserialized_invoice.to_string());


Could you also test here that the serialized invoice actually matches the initial string provided? Otherwise there might be the slight possibility that you make a mistake in the initial parsing and here only ever confirm that the logic is able to deserialize whatever you're serializing from that.

But serde_json::to_string(...) serializes as JSON so the str is not "lnbc1..." but ""lnbc1..."" so I can't test that right ?

Well, if it's indeed only about the wrapping "s, then you could just strip them before comparing the strings?

Right, that works I'll force push again thank you

lightning-invoice/src/lib.rs

tnull

LGTM, mod one nit. Sorry :)

lightning-invoice/src/lib.rs

…or invoice

NicolaLS closed this Jun 17, 2022

NicolaLS reopened this Jun 17, 2022

NicolaLS force-pushed the serde-module branch 2 times, most recently from bdbdc54 to c7e1c4a Compare June 17, 2022 21:25

TheBlueMatt reviewed Jun 18, 2022

View reviewed changes

NicolaLS force-pushed the serde-module branch from c7e1c4a to f9ddadd Compare June 18, 2022 14:21

TheBlueMatt reviewed Jun 18, 2022

View reviewed changes

NicolaLS force-pushed the serde-module branch from f9ddadd to aa5faef Compare June 18, 2022 16:22

NicolaLS force-pushed the serde-module branch from aa5faef to e9df749 Compare June 18, 2022 21:01

NicolaLS marked this pull request as ready for review June 20, 2022 07:17

tnull reviewed Jun 20, 2022

View reviewed changes

NicolaLS force-pushed the serde-module branch from e9df749 to fb2c32e Compare June 20, 2022 09:06

NicolaLS commented Jun 20, 2022

View reviewed changes

lightning-invoice/src/lib.rs Show resolved Hide resolved

tnull reviewed Jun 20, 2022

View reviewed changes

lightning-invoice/src/lib.rs Outdated Show resolved Hide resolved

add optional dependency on serde to implement serialize/deserialize f…

6c15de1

…or invoice

NicolaLS force-pushed the serde-module branch from fb2c32e to 6c15de1 Compare June 20, 2022 09:49

tnull approved these changes Jun 20, 2022

View reviewed changes

TheBlueMatt approved these changes Jun 21, 2022

View reviewed changes

TheBlueMatt merged commit f512586 into lightningdevkit:main Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add feature for Invoice ser/de #1548

add feature for Invoice ser/de #1548

NicolaLS commented Jun 16, 2022

codecov-commenter commented Jun 16, 2022 •

edited

Loading

TheBlueMatt commented Jun 16, 2022

NicolaLS commented Jun 17, 2022

NicolaLS commented Jun 17, 2022

NicolaLS commented Jun 17, 2022

TheBlueMatt left a comment

TheBlueMatt Jun 18, 2022

NicolaLS Jun 18, 2022

NicolaLS commented Jun 18, 2022

TheBlueMatt left a comment

TheBlueMatt Jun 18, 2022

TheBlueMatt Jun 18, 2022

NicolaLS Jun 18, 2022

TheBlueMatt commented Jun 18, 2022

tnull left a comment

tnull Jun 20, 2022

tnull Jun 20, 2022

tnull Jun 20, 2022

NicolaLS Jun 20, 2022

tnull Jun 20, 2022

NicolaLS Jun 20, 2022

tnull left a comment

add feature for Invoice ser/de #1548

add feature for Invoice ser/de #1548

Conversation

NicolaLS commented Jun 16, 2022

codecov-commenter commented Jun 16, 2022 • edited Loading

Codecov Report

TheBlueMatt commented Jun 16, 2022

NicolaLS commented Jun 17, 2022

NicolaLS commented Jun 17, 2022

NicolaLS commented Jun 17, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolaLS commented Jun 18, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 18, 2022

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 16, 2022 •

edited

Loading