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

en/de-codeable for lightning_invoice::Invoice #230

Merged
merged 1 commit into from
Jul 9, 2022

Conversation

NicolaLS
Copy link
Contributor

@NicolaLS NicolaLS commented Jul 9, 2022

  1. helps to put invoices into db
  2. will be needed to fix this when this PR is available

Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

Concept ACK, just two nits. If you find out what is doing the reformatting and if you can either turn it off or enforce it via CI that would be good. Worst case we merge with the reformatting, it's just annoying long term because it makes git blame less useful.

@@ -8,26 +8,27 @@ edition = "2018"
[dependencies]
anyhow = "1.0.58"
async-trait = "0.1"
bitcoin = { version = "0.28.1", features = [ "rand", "serde" ] }
bitcoin = { version = "0.28.1", features = ["rand", "serde"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

What software is doing the reformatting here? I'd rather avoid that since it's not checked by CI and will thus diverge again (leading to autoreformatting once you touch it again).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think IntelliJ idea Settings>tools>actions on safe, I turned that on because I thought it would sort the dependencies but it dosn't do that and I forgot to turn it off

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, there is another setting under Settings>Languages&Frameworks>Rust>Rustfmt "Run rustfmt on Save", that's what I have turned on and what CI verifies. That's better imo because it's independent of any particular IDE.

Comment on lines 228 to 229
let invoice_string =
String::from_utf8(Decodable::consensus_decode(d)?).map_err(DecodeError::from_err)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can directly decode strings, no need to go via Vec<u8>

Copy link
Contributor

Choose a reason for hiding this comment

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

So something like this I think ...

String::consensus_decode(d)?
    .parse::<lightning_invoice::Invoice>()
    .map_err(DecodeError::from_err)

@justinmoon
Copy link
Contributor

justinmoon commented Jul 9, 2022

Also, let's add a unittest. I think the following would work:

    #[test_log::test]
    fn test_invoice() {
        let invoice_str = "lnbc100p1psj9jhxdqud3jxktt5w46x7unfv9kz6mn0v3jsnp4q0d3p2sfluzdx45tqcs\
			h2pu5qc7lgq0xs578ngs6s0s68ua4h7cvspp5q6rmq35js88zp5dvwrv9m459tnk2zunwj5jalqtyxqulh0l\
			5gflssp5nf55ny5gcrfl30xuhzj3nphgj27rstekmr9fw3ny5989s300gyus9qyysgqcqpcrzjqw2sxwe993\
			h5pcm4dxzpvttgza8zhkqxpgffcrf5v25nwpr3cmfg7z54kuqq8rgqqqqqqqq2qqqqq9qq9qrzjqd0ylaqcl\
			j9424x9m8h2vcukcgnm6s56xfgu3j78zyqzhgs4hlpzvznlugqq9vsqqqqqqqlgqqqqqeqq9qrzjqwldmj9d\
			ha74df76zhx6l9we0vjdquygcdt3kssupehe64g6yyp5yz5rhuqqwccqqyqqqqlgqqqqjcqq9qrzjqf9e58a\
			guqr0rcun0ajlvmzq3ek63cw2w282gv3z5uupmuwvgjtq2z55qsqqg6qqqyqqqrtnqqqzq3cqygrzjqvphms\
			ywntrrhqjcraumvc4y6r8v4z5v593trte429v4hredj7ms5z52usqq9ngqqqqqqqlgqqqqqqgq9qrzjq2v0v\
			p62g49p7569ev48cmulecsxe59lvaw3wlxm7r982zxa9zzj7z5l0cqqxusqqyqqqqlgqqqqqzsqygarl9fh3\
			8s0gyuxjjgux34w75dnc6xp2l35j7es3jd4ugt3lu0xzre26yg5m7ke54n2d5sym4xcmxtl8238xxvw5h5h5\
			j5r6drg6k6zcqj0fcwg";
        let invoice = invoice_str.parse::<lightning_invoice::Invoice>().unwrap();
        test_roundtrip(invoice);
    }

Took that invoice from your serde PR.

Otherwise, looks great. Thanks for adding this!

Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

Thx!

@elsirion elsirion merged commit 7bfe007 into fedimint:master Jul 9, 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

3 participants