-
Notifications
You must be signed in to change notification settings - Fork 421
Add support for Testnet4
#4148
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
base: main
Are you sure you want to change the base?
Add support for Testnet4
#4148
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -194,6 +194,7 @@ impl Display for Currency { | |
let currency_code = match *self { | ||
Currency::Bitcoin => "bc", | ||
Currency::BitcoinTestnet => "tb", | ||
Currency::BitcoinTestnet4 => "tb", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this what other implementations are doing? would be a shame to lose the one-to-one mapping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT yes. BOLT11 speaks about 'testnet' overall, without a specific version. I considered a PR to introduce a change specific to Testnet4 (e.g. Not sure how this was handled during previous testnet upgrades though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lightning postdates tn3 |
||
Currency::Regtest => "bcrt", | ||
Currency::Simnet => "sb", | ||
Currency::Signet => "tbs", | ||
|
@@ -475,6 +476,7 @@ mod test { | |
|
||
assert_eq!("bc", Currency::Bitcoin.to_string()); | ||
assert_eq!("tb", Currency::BitcoinTestnet.to_string()); | ||
assert_eq!("tb", Currency::BitcoinTestnet4.to_string()); | ||
assert_eq!("bcrt", Currency::Regtest.to_string()); | ||
assert_eq!("sb", Currency::Simnet.to_string()); | ||
assert_eq!("tbs", Currency::Signet.to_string()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also consider not adding a new
Currency
variant, but if we just keptCurrency::BitcoinTestnet
we wouldn't be able to make this mapping back toTestnet4
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already kinda cant, though? If someone reads an invoice with the tb prefix they'll get a
Currency
/Network
of testnet, which won't map to tn4.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so what resolution do you suggest then? Opening a spec PR for a new BOLT11 prefix for Testnet4? As I mentioned below I was close to do this, but refrained so far as I thought the idea was that Testnet3 eventually is just superseded by Testnet4 and goes away. But maybe it's the easiest solution here, though we'll need to double-check that other impls are onboard with it ofc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I dunno, its definitely awk either way. We should at least explore it at the spec level, I imagine, but we can also map
Testnet4
toCurrency::BitcoinTestnet
for now so that you can build an invoice fromNetwork::Testnet4
?