add `Fallback` getter that returns `Address` #2023

futurepaul · 2023-02-08T15:18:48Z

closes #882

I wasn't sure the exact right strategy and wrote this before seeing the most recent comment on this issue, so feel free to close if this is the wrong direction.

A few notes:

I don't know of a way to impl Into because the Fallback on its own isn't enough to create an Address, you also need a network.
I wasn't sure what to do about returning a Result (and what errors that would require) because this is data that's already been successfully parsed and theoretically isn't wrong. Therefore I'm using unwraps which feels wrong, so would appreciate some input on that.
There's a potentially deeper fix here by using the bitcoin lib's WitnessVersion, Script, and hash types inside Fallback (there's even a todo on Fallback about using better types)
I'm not exactly sure why I didn't end up having to clone even though I take a reference, but I'm not going to look a gift horse in the mouth.
I convert Currency::Simnet to Network::Regtest, not sure if that's correct

jkczyz · 2023-02-09T19:03:43Z

Thanks for putting this together!

closes #882

I wasn't sure the exact right strategy and wrote this before seeing the most recent comment on this issue, so feel free to close if this is the wrong direction.

Chatted with @TheBlueMatt. From the perspective from Invoice interface, this looks like the direction we want.

A few notes:

I don't know of a way to impl Into because the Fallback on its own isn't enough to create an Address, you also need a network.

You could implement it for a tuple (&Fallback, Currency), but seems like we'd rather not use Into anyhow.

I wasn't sure what to do about returning a Result (and what errors that would require) because this is data that's already been successfully parsed and theoretically isn't wrong. Therefore I'm using unwraps which feels wrong, so would appreciate some input on that.

There's a potentially deeper fix here by using the bitcoin lib's WitnessVersion, Script, and hash types inside Fallback (there's even a todo on Fallback about using better types)

For these two points, yeah, it would better if we use better types. Could you update the enum variants to use them?

I'm not exactly sure why I didn't end up having to clone even though I take a reference, but I'm not going to look a gift horse in the mouth.

Clone wasn't need but the bytes are eventually copied, so somewhat similar.

I convert Currency::Simnet to Network::Regtest, not sure if that's correct

This seems right. Not sure how an LND-specific concept got into here lol. We inherited this code. 😛

lightning-invoice/src/lib.rs

lightning-invoice/src/utils.rs

futurepaul · 2023-02-09T23:48:31Z

Down to one unwrap in the address creation, so that's nice. Can do a Vec<Result<Address>> if that's feels better

codecov-commenter · 2023-02-10T00:17:11Z

Codecov Report

Patch coverage: 74.07% and project coverage change: +0.23 🎉

Comparison is base (56146e7) 91.04% compared to head (61db97d) 91.28%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2023      +/-   ##
==========================================
+ Coverage   91.04%   91.28%   +0.23%     
==========================================
  Files          99      102       +3     
  Lines       51722    49994    -1728     
  Branches    51722    49994    -1728     
==========================================
- Hits        47091    45635    -1456     
+ Misses       4631     4359     -272

Impacted Files	Coverage Δ
lightning-invoice/src/ser.rs	`92.43% <0.00%> (+0.24%)`	⬆️
lightning-invoice/src/lib.rs	`79.91% <68.42%> (-6.41%)`	⬇️
lightning-invoice/src/de.rs	`75.81% <100.00%> (-6.58%)`	⬇️

... and 99 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

lightning-invoice/src/lib.rs

jkczyz

Sorry about the delay. Travel go the best of me.

lightning-invoice/src/ser.rs

lightning-invoice/src/de.rs

jkczyz · 2023-02-13T16:17:45Z

lightning-invoice/src/de.rs

 				if bytes.len() != 20 {
 					return Err(ParseError::InvalidPubKeyHashLength);
 				}
-				//TODO: refactor once const generics are available
-				let mut pkh = [0u8; 20];
-				pkh.copy_from_slice(&bytes);
+				let pkh = match PubkeyHash::from_slice(&bytes) {
+					Ok(pkh) => pkh,
+					Err(_) => return Err(ParseError::InvalidPubKeyHash),
+				};


from_slice will check the length, so we can remove our explicit check in favor of the new one. Probably could have the match pattern on the exact error variant. That way we won't need to add a new error variant and we'll fail to compile if new variants are added upstream.

nice, thank you this worked out great

jkczyz · 2023-02-13T16:17:56Z

lightning-invoice/src/de.rs

 				if bytes.len() != 20 {
 					return Err(ParseError::InvalidScriptHashLength);
 				}
-				let mut sh = [0u8; 20];
-				sh.copy_from_slice(&bytes);
+				let sh = match ScriptHash::from_slice(&bytes) {
+					Ok(sh) => sh,
+					Err(_) => return Err(ParseError::InvalidScriptHash),
+				};


Likewise here.

yeah I got to get rid of all three errors I added!

lightning-invoice/src/lib.rs

TheBlueMatt · 2023-03-20T16:45:05Z

Any update on addressing the review feedback @futurepaul?

futurepaul · 2023-03-22T16:49:56Z

Sorry for the long delay, will be able to get back to this in about a week

futurepaul · 2023-04-04T17:12:07Z

okay @jkczyz @TheBlueMatt this addresses the review feedback

TheBlueMatt

Nice!

lightning-invoice/src/de.rs

lightning-invoice/src/lib.rs

TheBlueMatt · 2023-04-04T21:02:21Z

Please feel free to squash down your fixups so that each commit stands on its own and doesn't fix issues introduced in previous commits in the same PR. THen we'll get another reviewer here.

futurepaul · 2023-04-04T21:35:10Z

couldn't think of a clean way to separate out this work because the use of the bitcoin types directly impacted how the getter is written so hope it's okay I squashed down to one commit

jkczyz · 2023-04-06T02:35:21Z

couldn't think of a clean way to separate out this work because the use of the bitcoin types directly impacted how the getter is written so hope it's okay I squashed down to one commit

FWIW, you could add fallback_addresses in a second commit since the first one would have all the types changed.

jkczyz

Just some nits that I missed but otherwise looks good.

lightning-invoice/src/de.rs

lightning-invoice/src/lib.rs

jkczyz self-requested a review February 8, 2023 15:42

TheBlueMatt assigned TheBlueMatt and jkczyz Feb 8, 2023

jkczyz reviewed Feb 9, 2023

View reviewed changes

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

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

benthecarman reviewed Feb 12, 2023

View reviewed changes

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

jkczyz reviewed Feb 13, 2023

View reviewed changes

TheBlueMatt reviewed Apr 4, 2023

View reviewed changes

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

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

futurepaul force-pushed the fallback-to-address branch from 61db97d to faf1e23 Compare April 4, 2023 21:34

jkczyz previously approved these changes Apr 6, 2023

View reviewed changes

jkczyz reviewed Apr 6, 2023

View reviewed changes

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

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

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

Fallback: add Address getter and use bitcoin types

cf0a90b

futurepaul dismissed jkczyz’s stale review via cf0a90b April 6, 2023 15:49

futurepaul force-pushed the fallback-to-address branch from faf1e23 to cf0a90b Compare April 6, 2023 15:49

TheBlueMatt approved these changes Apr 7, 2023

View reviewed changes

jkczyz approved these changes Apr 7, 2023

View reviewed changes

jkczyz merged commit 1ceb41e into lightningdevkit:main Apr 7, 2023
14 checks passed

optout21 mentioned this pull request Apr 18, 2024

Add splicing feature flag #3001

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `Fallback` getter that returns `Address` #2023

add `Fallback` getter that returns `Address` #2023

futurepaul commented Feb 8, 2023

jkczyz commented Feb 9, 2023

futurepaul commented Feb 9, 2023 •

edited

codecov-commenter commented Feb 10, 2023 •

edited

jkczyz left a comment

jkczyz Feb 13, 2023

futurepaul Apr 4, 2023

jkczyz Feb 13, 2023

futurepaul Apr 4, 2023

TheBlueMatt commented Mar 20, 2023

futurepaul commented Mar 22, 2023

futurepaul commented Apr 4, 2023

TheBlueMatt left a comment

TheBlueMatt commented Apr 4, 2023

futurepaul commented Apr 4, 2023

jkczyz commented Apr 6, 2023

jkczyz left a comment

add Fallback getter that returns Address #2023

add Fallback getter that returns Address #2023

Conversation

futurepaul commented Feb 8, 2023

jkczyz commented Feb 9, 2023

futurepaul commented Feb 9, 2023 • edited

codecov-commenter commented Feb 10, 2023 • edited

Codecov Report

jkczyz left a comment

Choose a reason for hiding this comment

jkczyz Feb 13, 2023

Choose a reason for hiding this comment

futurepaul Apr 4, 2023

Choose a reason for hiding this comment

jkczyz Feb 13, 2023

Choose a reason for hiding this comment

futurepaul Apr 4, 2023

Choose a reason for hiding this comment

TheBlueMatt commented Mar 20, 2023

futurepaul commented Mar 22, 2023

futurepaul commented Apr 4, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 4, 2023

futurepaul commented Apr 4, 2023

jkczyz commented Apr 6, 2023

jkczyz left a comment

Choose a reason for hiding this comment

add `Fallback` getter that returns `Address` #2023

add `Fallback` getter that returns `Address` #2023

futurepaul commented Feb 9, 2023 •

edited

codecov-commenter commented Feb 10, 2023 •

edited