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

Add regtest to AddressCompat #64

Closed
wants to merge 1 commit into from

Conversation

crisdut
Copy link
Member

@crisdut crisdut commented Nov 24, 2022

No description provided.

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

LGTM. Can you please also add method is_testnet since downstream the now absent flag testnet was frequently used.

@dr-orlovsky
Copy link
Member

Ok, I digged into the story and found out the following:

  • We already have AddressNetwork type in this crate, which was made specifically for this purpose. It seems I just forgot to use it in AddressCompat.
  • Any change in AddressCompat network information will be a breaking change which can't go into 0.8.x patch releases
  • For v0.9 the crate bitcoin_scripts has moved into a different repository in this org, named bp-foundation. The reason is that this crate does a lot of the stuff which is not specific to the wallets and is used in client-side-validation, bp-core and rgb-core. Moving crate allowed to get rid of all those projects depending on the bloated descriptor wallet.

I made a PR there making AddressCompat to use AddressNetwork, which will solve the regtest support issue - but only for v0.9.

What to do in v0.8? There are two options:

  1. If you really-really need it, we can go back to the PRs you proposed with a temporarily patch moving from the use of AddressCompat to the rust-bitcoin Address type. This will be reverted in v0.9 though.
  2. Otherwise we can leave v0.8 as is and wait for v0.9 to ship.

@crisdut
Copy link
Member Author

crisdut commented Nov 24, 2022

Hi @dr-orlovsky

Can we centralize the discuss here?

I made a PR there making AddressCompat to use AddressNetwork, which will solve the regtest support issue - but only for v0.9.

That's awesome!

What to do in v0.8?

Well, I'm working on some fronts with v0.8 version:

  • Support to LNPBP nodes in the Polar Regtest tool (see here)
  • CI/CD with regtest nodes to increase development velocity in some products build over RGB
  • A project to help newcomers to learn and contribute to LNPBP projects
  • A early project to understand how Storm/RGB/Universal Invoices integration works. The plan is to understand how I will contribute to finishing RGB over LN integration =)

If you really-really need it, we can go back to the PRs you proposed with a temporarily patch moving from the use of AddressCompat to the rust-bitcoin Address type. This will be reverted in v0.9 though.

I would like to finish the list with v0.8 because I am more familiar with this version. In December, I will dive into the v0.9 version, and it's ok to make some changes in my projects after updating to v0.9.

Is that ok for you?

@dr-orlovsky
Copy link
Member

Ok, sure. Can you pls restore your original PRs where you was replacing AddressCompat with rust-bitcoin Address? Sorry for the mess.

Also can you please add the issues to the same repos to revert these changes in v0.9?

@crisdut crisdut closed this Nov 26, 2022
@crisdut crisdut deleted the fix/address-compat branch December 29, 2022 01:45
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