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.

Sign up for GitHub

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

Jump to bottom

Ledger bug: Erroneous Unable to broadcast transaction error #5118

Closed
kyranjamie opened this issue Mar 25, 2024 · 6 comments · Fixed by #5375
Closed

Ledger bug: Erroneous Unable to broadcast transaction error #5118

kyranjamie opened this issue Mar 25, 2024 · 6 comments · Fixed by #5375
Assignees
@kyranjamie @pete-watters
Labels
area:ledger bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds effort:medium Expected to take 2-5 days of integration work
Milestone
Fix urgent bugs

Comments

@kyranjamie
Copy link
Collaborator

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.
@kyranjamie kyranjamie added bug Functionality broken bug-p1 Critical functionality broken for many customers, with no clear workarounds labels Mar 25, 2024
@kyranjamie kyranjamie added this to the Fix urgent bugs milestone Mar 25, 2024
@markmhendrickson
Copy link
Collaborator

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

@pete-watters pete-watters self-assigned this Mar 28, 2024
@markmhendrickson markmhendrickson added bug-p2 Critical functionality broken for few users, with no clear workarounds and removed bug-p1 Critical functionality broken for many customers, with no clear workarounds labels Apr 17, 2024
@pete-watters pete-watters added the effort:medium Expected to take 2-5 days of integration work label Apr 19, 2024
@pete-watters
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Collaborator Author

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 pete-watters linked a pull request May 15, 2024 that will close this issue
chore: add defensive code and better analytics for broadcast errors, … #5375
Merged
pete-watters added a commit that referenced this issue May 15, 2024
@pete-watters
chore: add defensive code and better analytics for broadcast errors, …
1fab7d8 
…ref #5118
@pete-watters pete-watters mentioned this issue May 15, 2024
Merged
@pete-watters
Copy link
Contributor

pete-watters commented May 15, 2024 •
edited

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
Copy link
Collaborator Author

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

pete-watters added a commit that referenced this issue May 17, 2024
@pete-watters
chore: add defensive code and better analytics for broadcast errors, …
5446e92 
…ref #5118
pete-watters added a commit that referenced this issue May 20, 2024
@pete-watters
chore: add defensive code and better analytics for broadcast errors, …
bf36652 
…ref #5118
github-merge-queue bot pushed a commit that referenced this issue May 20, 2024
@pete-watters
chore: add defensive code and better analytics for broadcast errors, …
a36dae4 
…ref #5118
kyranjamie pushed a commit that referenced this issue May 22, 2024
@semantic-release-bot
chore(release): 6.41.0
271875f 
## [6.41.0](v6.40.0...v6.41.0) (2024-05-22)

### Features

* remove increase fee summary page, closes [#5305](#5305) ([2db97bd](2db97bd))

### Bug Fixes

* fix padding on back button, ref leather-wallet/issues[#25](#25) ([3a42fc9](3a42fc9))
* investigate stx fee issues ([c5e04c1](c5e04c1))
* missing token color ([caa4c88](caa4c88))
* regtest address generation, closes [#5401](#5401) ([0c6c4d1](0c6c4d1))
* sip10 token send form fees bug ([5903a7b](5903a7b))
* stamp error reporting ([9b77421](9b77421))
* tsconfig update for tokens pkg ([9699d76](9699d76))

### Internal

* add defensive code and better analytics for broadcast errors, ref [#5118](#5118) ([a36dae4](a36dae4))
* post-release merge back ([ca9cf0b](ca9cf0b))
* remove combined asset model, closes [#48](#48) ([a827b40](a827b40))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kyranjamie kyranjamie

@pete-watters pete-watters

Labels
area:ledger bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds effort:medium Expected to take 2-5 days of integration work
Projects
None yet
Milestone
Fix urgent bugs
Development

Successfully merging a pull request may close this issue.

chore: add defensive code and better analytics for broadcast errors, … leather-wallet/extension
3 participants
@markmhendrickson @kyranjamie @pete-watters