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

Send SATS fails and lands on unknown route #4635

Closed
pete-watters opened this issue Dec 4, 2023 · 10 comments · Fixed by #4665
Closed

Send SATS fails and lands on unknown route #4635

pete-watters opened this issue Dec 4, 2023 · 10 comments · Fixed by #4665
Assignees
Labels
area:brc20 bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds

Comments

@pete-watters
Copy link
Contributor

I was testing this issue and encountered a bug when trying to send SATS.

I have SATS in one account and wanted to transfer them to another. I am not sure if this is supposed to work, any ideas @markmhendrickson ?

It seems weird as

  • the route for send/brc20/sats/error page I land on isn't defined
  • when I am doing a Send and choosing SATS it doesn't ask me where I want to send them
Kapture.2023-12-04.at.11.25.03.mp4
@pete-watters pete-watters self-assigned this Dec 4, 2023
@markmhendrickson markmhendrickson added bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds area:brc20 labels Dec 4, 2023
@markmhendrickson
Copy link
Collaborator

markmhendrickson commented Dec 5, 2023

I'm getting the same but unsure the problem – will need checking the error logs, etc. to diagnose

The user should see a confirmation screen upon submission of the review screen

@markmhendrickson
Copy link
Collaborator

@pete-watters FYI I've moved this to the top of the backlog

@markmhendrickson
Copy link
Collaborator

I'm also seeing a related issue where my balance isn't recognized during sending. Prefer I file a separate issue or want to tackle jointly here?

Screenshot 2023-12-08 at 07 36 28 Screenshot 2023-12-08 at 07 36 19

@pete-watters
Copy link
Contributor Author

I'm also seeing a related issue where my balance isn't recognized during sending. Prefer I file a separate issue or want to tackle jointly here?

Screenshot 2023-12-08 at 07 36 28 Screenshot 2023-12-08 at 07 36 19

It's OK to have them both fixed in this issue. Do you know if there was a design to go with this originally or else a ticket for how it's supposed to work?

I can try figure it out anyway. It seems it fails at creating the inscription which is the step before choosing a recipient?

@markmhendrickson
Copy link
Collaborator

@mica000 mind linking any of the BRC-20 materials we might have in Figma?

@markmhendrickson
Copy link
Collaborator

It seems it fails at creating the inscription which is the step before choosing a recipient?

Sending BRC-20 is unique in that it's a 2-step process, in which during the send flow, the only thing that really happens is the creation of an inscription via the OrdinalsBot API. No recipient is indicated within the send flow at this time.

Then later, the user comes back to the home screen and sends the inscription via the normal flow, indicating the recipient at that point.

@markmhendrickson
Copy link
Collaborator

It's also entirely possible there's an underlying Ordinalsbot API error thrown here and we aren't catching it, so it's worth checking. And we can contact their team as needed to help diagnose.

@pete-watters
Copy link
Contributor Author

pete-watters commented Dec 8, 2023

I checked this out and after clicking Create transfer inscription we get a 400 Bad Request error from https://blockstream.info/api/tx

It complains about:

sendrawtransaction RPC error: {"code":-26,"message":"non-mandatory-script-verify-flag (Witness program hash mismatch)"}

Then we crash unhandled.

This method fails in bitcoin-client.ts:

  async broadcastTransaction(tx: string) {
    // TODO: refactor to use `axios`
    // https://github.com/leather-wallet/extension/issues/4521
    // eslint-disable-next-line no-restricted-globals
    return fetch(`${this.configuration.baseUrl}/tx`, {
      method: 'POST',
      body: tx,
      headers: {
        'Content-Type': 'application/x-www-form-urlencoded',
      },
    });
  }

We added this functionality in this release

I think it may have been broken in #4316) by these refactors of useGenerateUnsignedNativeSegwitSingleRecipientTx :

Going back further to commit e9da65164273476ae4ba0c2de185dc07c7230d34 and it was working so it's something on our side. Any ideas here @kyranjamie ?

Screenshot 2023-12-08 at 14 31 36

@kyranjamie
Copy link
Collaborator

Yep, looks like the refactor in the Ledger work broke this. We generate the tx, then try and broadcast it.

Previously this hook generated and signed the tx, which it no longer does. We need to sign the transaction with useSignBitcoinTx, finalise it, then broadcast it.

@pete-watters
Copy link
Contributor Author

Thanks for the help here @kyranjamie . I think I have fixed it here as I submitted some things successfully.

@markmhendrickson in my fixing of this I had to spend ~ $70 on fees, sending a couple of transactions:
Screenshot 2023-12-08 at 15 55 09

I saw what I think is another bug also where if I sign out of my wallet and then into our test wallet, my Pending BRC-20 transfers showed in the other wallet. This could be some weird caching issue.

Kapture.2023-12-08.at.16.16.42.mp4

This bug is fixed but I'm confused what the next step for moving these tokens is

  1. I think we can have a better UX for where we show the Pending BTC-20 transfers - maybe it should be in Activity?
  2. The Pending part is gone from my wallet now, as is the cost of the fees but I don't know what I'm supposed to do next. After some time should an Ordinal Inscription show up in my collectibles? If so, maybe we can add a placeholder entry there as a visual queue

kyranjamie pushed a commit that referenced this issue Dec 12, 2023
## [6.19.0](v6.18.0...v6.19.0) (2023-12-12)

### Features

* add rpc method for signing stacks messages ([e77a8d8](e77a8d8))

### Bug Fixes

* add background location to brc-20 modal and fix typo ([7483b9e](7483b9e))
* broadcast ledger swap ([886309b](886309b))
* fix brc-20 sendby signing transaction before finalising then broadcasting, closes [#4635](#4635) ([5aa7c3c](5aa7c3c))
* keep search params while doing background location redirect ([6b7ce6a](6b7ce6a))
* playwright error ([5bef424](5bef424))
* revert signing logic to try both keys, closes [#4645](#4645) ([8b1be50](8b1be50))
* selecting testnet in tests ([d275d8c](d275d8c))
* swap test route path ([24d3677](24d3677))

### Internal

* add ledger signing routes to BRC-20 send ([42ee981](42ee981))
* add swap tests ([bd6dc1a](bd6dc1a))
* enable ledger swaps to test ([6539075](6539075))
* post-release merge back ([9414a9b](9414a9b))
* **signing:** support non-index zero input signing, closes [#4620](#4620), [#4628](#4628) ([d2edb18](d2edb18))
Nithishvb pushed a commit to Nithishvb/extension that referenced this issue Dec 13, 2023
Nithishvb pushed a commit to Nithishvb/extension that referenced this issue Dec 13, 2023
## [6.19.0](leather-io/extension@v6.18.0...v6.19.0) (2023-12-12)

### Features

* add rpc method for signing stacks messages ([e77a8d8](leather-io@e77a8d8))

### Bug Fixes

* add background location to brc-20 modal and fix typo ([7483b9e](leather-io@7483b9e))
* broadcast ledger swap ([886309b](leather-io@886309b))
* fix brc-20 sendby signing transaction before finalising then broadcasting, closes [leather-io#4635](leather-io#4635) ([5aa7c3c](leather-io@5aa7c3c))
* keep search params while doing background location redirect ([6b7ce6a](leather-io@6b7ce6a))
* playwright error ([5bef424](leather-io@5bef424))
* revert signing logic to try both keys, closes [leather-io#4645](leather-io#4645) ([8b1be50](leather-io@8b1be50))
* selecting testnet in tests ([d275d8c](leather-io@d275d8c))
* swap test route path ([24d3677](leather-io@24d3677))

### Internal

* add ledger signing routes to BRC-20 send ([42ee981](leather-io@42ee981))
* add swap tests ([bd6dc1a](leather-io@bd6dc1a))
* enable ledger swaps to test ([6539075](leather-io@6539075))
* post-release merge back ([9414a9b](leather-io@9414a9b))
* **signing:** support non-index zero input signing, closes [leather-io#4620](leather-io#4620), [leather-io#4628](leather-io#4628) ([d2edb18](leather-io@d2edb18))
pete-watters added a commit to Nithishvb/extension that referenced this issue Jan 17, 2024
pete-watters pushed a commit to Nithishvb/extension that referenced this issue Jan 17, 2024
## [6.19.0](leather-io/extension@v6.18.0...v6.19.0) (2023-12-12)

### Features

* add rpc method for signing stacks messages ([e77a8d8](leather-io@e77a8d8))

### Bug Fixes

* add background location to brc-20 modal and fix typo ([7483b9e](leather-io@7483b9e))
* broadcast ledger swap ([886309b](leather-io@886309b))
* fix brc-20 sendby signing transaction before finalising then broadcasting, closes [leather-io#4635](leather-io#4635) ([5aa7c3c](leather-io@5aa7c3c))
* keep search params while doing background location redirect ([6b7ce6a](leather-io@6b7ce6a))
* playwright error ([5bef424](leather-io@5bef424))
* revert signing logic to try both keys, closes [leather-io#4645](leather-io#4645) ([8b1be50](leather-io@8b1be50))
* selecting testnet in tests ([d275d8c](leather-io@d275d8c))
* swap test route path ([24d3677](leather-io@24d3677))

### Internal

* add ledger signing routes to BRC-20 send ([42ee981](leather-io@42ee981))
* add swap tests ([bd6dc1a](leather-io@bd6dc1a))
* enable ledger swaps to test ([6539075](leather-io@6539075))
* post-release merge back ([9414a9b](leather-io@9414a9b))
* **signing:** support non-index zero input signing, closes [leather-io#4620](leather-io#4620), [leather-io#4628](leather-io#4628) ([d2edb18](leather-io@d2edb18))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:brc20 bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants