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

fix: close ledger sheet in request keys flow #6024

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

alter-eggo
Copy link
Contributor

@alter-eggo alter-eggo commented Dec 18, 2024

Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

You can't remove the handler and default to navigating home, that'll break flows in which the user isn't mean to visit home page, e.g. all the RPC flows

@kyranjamie
Copy link
Collaborator

The routes for the ledger flows are such that /stacks/ or /bitcoin by themselves are not valid. What other ways can we ensure users do land on those routes?

Base automatically changed from feat/display-sbtc to dev December 18, 2024 16:24
@alter-eggo alter-eggo force-pushed the fix/go-back-ledger-request branch from ed39244 to 3992286 Compare December 20, 2024 10:47
@alter-eggo alter-eggo force-pushed the fix/go-back-ledger-request branch 5 times, most recently from b1fb06c to 277bad1 Compare January 20, 2025 15:10
@alter-eggo
Copy link
Contributor Author

alter-eggo commented Jan 20, 2025

@kyranjamie @pete-watters guys could you review/test please

@pete-watters
Copy link
Contributor

@kyranjamie @pete-watters guys could you review/test please

The code looks OK and I will test now but can you please link a ticket to the PR and add it in the commit message?

Otherwise it's not clear what the goal of this PR is

@pete-watters
Copy link
Contributor

I did some testing here and it doesn't reintroduce this bug anymore so that's good.

I did hit an issue when cancelling a transaction though. If I:

  • Sign into Gamma with Ledger
  • try and buy some ordinal
  • cancel signing

It crashes with :

TypeError: Cannot read properties of null (reading 'inputsLength')
    at getPsbtTxInputs (chrome-extension://dbbkfepagfalpohacffnpaodaklhamif/index.js:273885:31)
    at ApproveSignLedgerBitcoinTx (chrome-extension://dbbkfepagfalpohacffnpaodaklhamif/index.js:115547:80)
    at renderWithHooks (chrome-extension://
bug_cancelling.mp4

The transaction stays open on the Ledger too. I double checked and that doesn't happen on dev but I could be wrong.

It's been a while since I checked this though and I can't remember what issue its fixing so if you can link that it would be great.

@alter-eggo alter-eggo force-pushed the fix/go-back-ledger-request branch from 277bad1 to 3dd753f Compare January 22, 2025 11:23
@alter-eggo
Copy link
Contributor Author

@pete-watters I checked and on dev this error happens for me too.
ledger connect success state closing is quite buggy in all the flows, since after showing it we navigate user to the signing page, for example here

I feel like we need to make this state uncancellable, wdyt?

@pete-watters
Copy link
Contributor

@pete-watters I checked and on dev this error happens for me too. ledger connect success state closing is quite buggy in all the flows, since after showing it we navigate user to the signing page, for example here

I feel like we need to make this state uncancellable, wdyt?

Good idea! Maybe it is supposed to be non-cancellable there but became broken?

Comment on lines +111 to +113
appEvents.publish('ledgerBitcoinTxSigningCancelled', {
unsignedPsbt: unsignedTransaction ? bytesToHex(unsignedTransaction.toPSBT()) : '',
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the action is cancelled, why do we return anything at all with the action? Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need for abort handler

    function signingAbortedHandler(msg: GlobalAppEvents['ledgerBitcoinTxSigningCancelled']) {
      if (msg.unsignedPsbt === psbt) {
        appEvents.unsubscribe('ledgerBitcoinTxSigningCancelled', signingAbortedHandler);
        appEvents.unsubscribe('ledgerBitcoinTxSigned', txSignedHandler);
        reject(new Error('User cancelled the signing operation'));
      }
    }

@alter-eggo alter-eggo force-pushed the fix/go-back-ledger-request branch from 3dd753f to efce52d Compare January 24, 2025 12:28
@alter-eggo alter-eggo merged commit a950b32 into dev Jan 24, 2025
32 checks passed
@alter-eggo alter-eggo deleted the fix/go-back-ledger-request branch January 24, 2025 12:34
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.

3 participants