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

Old BTC address format support #570

Closed
Nackoo2000 opened this issue Jun 13, 2023 · 11 comments · Fixed by #665
Closed

Old BTC address format support #570

Nackoo2000 opened this issue Jun 13, 2023 · 11 comments · Fixed by #665
Assignees

Comments

@Nackoo2000
Copy link
Collaborator

We need to support sending to the old format of the BTC address.

image

@roeierez roeierez added this to the v0.1.0-alpha milestone Jun 20, 2023
@roeierez roeierez assigned ademar111190 and unassigned ok300 Jun 26, 2023
@ubbabeck
Copy link
Contributor

It recognises the address and successfully validates it.

2023-07-10T10:16:36.192659 :: V :: BitcoinAddressTextFormField :: Calling validator for bc1qpyncvfcknjx9f7xmm0jhtj2mzcelf7ccjkenn2ejk9z8yvlf33hserkwkm ::  :: 0 ::  ::  :: 
2023-07-10T10:16:36.193189 :: V :: AccountBloc :: isValidBitcoinAddress: bc1qpyncvfcknjx9f7xmm0jhtj2mzcelf7ccjkenn2ejk9z8yvlf33hserkwkm ::  :: 0 ::  ::  :: 
2023-07-10T10:16:36.195308 :: V :: BitcoinAddressTextFormField :: Address bc1qpyncvfcknjx9f7xmm0jhtj2mzcelf7ccjkenn2ejk9z8yvlf33hserkwkm validation result ValidatorHolder{valid: true, inLock: true, locked true, hash: 634443779} ::  :: 0 ::  ::  :: 

when trying to broadcast the transaction it returns the following error:

2023-07-10T10:18:53.264574 :: D :: BreezBridge :: node-logs: DEBUG   plugin-gl-plugin-internal: Read response from JSON-RPC: {\"error\":{\"code\":301,\"message\":\"Could not afford all using all 0 available UTXOs: all short\"},\"id\":0,\"jsonrpc\":\"2.0\"} ::  :: 0 ::  ::  ::

@roeierez roeierez modified the milestones: v0.1.0-alpha, v0.1.1-alpha Jul 13, 2023
@Nackoo2000
Copy link
Collaborator Author

The old format is not recognised when scanning from the balance screen
I have to scan it from Send>Send to BTC Address>Scan

video_2023-10-03_19-26-57.mp4

@ubbabeck
Copy link
Contributor

ubbabeck commented Oct 5, 2023

[Edit ] We need to make sure that sending scanning works to all of the following addresses.

  • SegWit V0 (bech32 starts with bc1q..)
  • P2PKH, Legacy, (starts with 1..)
  • P2SH, Script, (Starts with 3..)
  • P2TR, Taproot, (bech32m starts with bc1p..)

@ok300
Copy link
Collaborator

ok300 commented Oct 5, 2023

On SDK level, they work. I tested sending to all 4 a few days ago.

@ubbabeck
Copy link
Contributor

ubbabeck commented Oct 5, 2023

Awesome, in that case it's must be an issue in how we handle it when scanning.

@Nackoo2000
Copy link
Collaborator Author

Nackoo2000 commented Oct 9, 2023

I am not sure if this is for this issue or a different one:
This is when scanning a BTC address generated in BreezM. Scanned with BreezC
When I deleted the text "BITCOIN:" before the text the payment was made successfully
image

@ubbabeck
Copy link
Contributor

ubbabeck commented Oct 9, 2023

I am not sure if this is for this issue or a different one: This is when scanning a BTC address generated in BreezM. Scanned with BreezC When I deleted the text "BITCOIN:" before the text the payment was made successfully

This can be an issue on it's own since it is tied to bip 21(https://bitcoinqr.dev/). Though not sure if this is something we should add here in c-breez or add to the breez-sdk.

@ademar111190
Copy link
Collaborator

I see two different things here:

  • The parser on the sdk side should deal with the bitcoin prefix generating a BitcoinAddressData where the address does not have the prefix "bitcoin:"
  • The app should show an invalid address if the user adds the "bitcoin:" prefix (what seems is already doing)
    So for me; It seems a different issue on sdk side.

@ok300
Copy link
Collaborator

ok300 commented Oct 10, 2023

The SDK accepts "bitcoin:..." addresses as valid and removes the prefix in the returned BitcoinAddressData (see unit test).

It does the same for other BIP-21 URI params, like label or amount.

@ademar111190
Copy link
Collaborator

@Nackoo2000 Can you confirm the address with "bitcoin:" prefix are working for you? On my tests it is working after the changes on #665

@Nackoo2000
Copy link
Collaborator Author

Yes all good.

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 a pull request may close this issue.

5 participants