Skip to content

Updates for feat/ldk-node-serializable#32

Merged
piotr-iohk merged 2 commits into
mainfrom
feat/ldk-node-serializable
Oct 14, 2025
Merged

Updates for feat/ldk-node-serializable#32
piotr-iohk merged 2 commits into
mainfrom
feat/ldk-node-serializable

Conversation

@piotr-iohk
Copy link
Copy Markdown
Collaborator

@piotr-iohk piotr-iohk self-assigned this Oct 14, 2025
Comment thread test/helpers/actions.ts Outdated
let address = '';
if (which === 'bitcoin') {
address = uri.replace(/^bitcoin:/, '').replace(/\?.*$/, '');
if (!address.startsWith('bcrt')) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Beware of this check... It is only valid for REGTEST.

Please research "the valid bitcoin addresses prefix for each network" and best to adjust this so it accepts for the valid prefix for each bitcoin network.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough, although I believe that the tests will be executed only against regtest.

Comment thread test/helpers/actions.ts Outdated
if (!ln) {
throw new Error(`No lightning invoice found in uri: ${uri}`);
}
if (!ln.startsWith('lnbcrt')) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same here, 'lnbcrt' is the valid bolt11 prefix for REGTEST, for other networks is different.

@ovitrif
Copy link
Copy Markdown
Collaborator

ovitrif commented Oct 14, 2025

Please add me as reviewer from now on, wherever it can help. I want to have a better understanding of these tests for when they fail so I can predict what might be wrong, instead of always asking you to investigate.

@piotr-iohk piotr-iohk force-pushed the feat/ldk-node-serializable branch from ded38bb to d225c1c Compare October 14, 2025 12:28
Comment thread README.md Outdated
Comment on lines +11 to +13
Absolutely — that’s a super clean way to present it 👌
A two-column **Android | iOS** table makes your setup instantly clear for contributors.
Here’s a polished Markdown version that fits your style:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

"You're Absolutely Right!"™ 🚀

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

that belongs to different PR, and also 🤦

@ovitrif
Copy link
Copy Markdown
Collaborator

ovitrif commented Oct 14, 2025

I think this can be already merged 👍🏻

@piotr-iohk piotr-iohk force-pushed the feat/ldk-node-serializable branch from 5103916 to d225c1c Compare October 14, 2025 18:18
@piotr-iohk piotr-iohk merged commit ce79279 into main Oct 14, 2025
@piotr-iohk piotr-iohk deleted the feat/ldk-node-serializable branch October 14, 2025 18:18
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.

2 participants