Ledger bug: Erroneous `Unable to broadcast transaction` error

leather-io / extension

Leather browser extension

https://leather.io

MIT License

305 stars 144 forks source link

Ledger bug: Erroneous `Unable to broadcast transaction` error #5118

Open kyranjamie opened 7 months ago

kyranjamie commented 7 months ago

Don't have a stack trace to share, but I encountered this yesterday broadcasting a tx with Ledger.

On the broadcast success screen, there's a "Cannot read property of undefined reading .address" error. Looks like an issue with route state.

Because there's a runtime error, we show the "Unable to broadcast screen" even if the transaction does broadcast sucessfully.

markmhendrickson commented 7 months ago

Is this a P2 since it presumably isn't affecting most Ledger users?

pete-watters commented 6 months ago

I investigated this and have as yet been unable to reproduce. Is there a particular type of transaction I need to broadcast? Is it doing an action on an external site?

I have tested:

sending BTC on testnet
sending STX between accounts
swapping STX to welshcorgi

All of these actions completed without any console errors. Maybe there's a specific action to catch?

I can't find any calls to get(location.state that use address.

pete-watters commented 6 months ago

@kyranjamie when you have time can you give some help reproducing this? I tried a few things and was unfortunately unable to trigger the error.

I have seen similar issues like this sometimes if I am serving the APP in dev mode and auto reload is triggered.

kyranjamie commented 6 months ago

I would try triggering some Bitcoin sends with Ledger from the sendTransfer flow. I wonder if there are any cases where we navigate to the confirmation step without address?

pete-watters commented 6 months ago

I spent some time on this and performed a lot of testing and unfortunately have as yet been unable to replicate this.

I prepared this PR that adds some defensive code and analytics to when we error in the hope it will help identify it in the future.

I tried lots of different transfers:

those listed here
BTC sends with all fee types including custom fees
STX sends
performing transactions then removing the ledger mid transaction
I searched the codebase for possible areas where calls to .address could fail and added some defensive code where it could help
I went back to v6.31.0 and searched through the code then also and couldn't find anything obvious
I searched in Sentry and couldn't find any Cannot read property of undefined reading .address

I found two more errors which could possibly be related but those were first seen in v6.36.0 so thats unlikely:

kyranjamie commented 6 months ago

If you're unable to replicate at all, then we can close. More analytics are a good idea 👍🏼

kyranjamie commented 3 months ago

Re-opening this issue. I used Lockstacks again yesterday and again run into the Cannot read property of undefined reading .address error.

It's very confusing because the transaction does broadcast, but we show a generic error because of this issue. We should not be handling form state in this way.

Also noticed this

https://github.com/leather-io/extension/blob/e6863fc5bb37b3991fc6de9dd9798cf6edfb8bc9/src/app/features/stacks-transaction-request/hooks/use-stacks-transaction-summary.ts#L57

Type casting is a mistake here, we don't know that it's a TokenTransferPayload for sure

markmhendrickson commented 3 months ago

@kyranjamie is this P1 or P2?

kyranjamie commented 3 months ago

I think we should consider a https://github.com/leather-io/extension/labels/bug-p1

markmhendrickson commented 3 months ago

Related thread