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

fix: fix bug with sending ordinals from ledger, #ref 5253 #5268

Merged
merged 1 commit into from
Apr 19, 2024
Merged

fix: fix bug with sending ordinals from ledger, #ref 5253 #5268

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 61 additions & 11 deletions src/app/features/ledger/hooks/use-ledger-navigate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ export function useLedgerNavigate() {
return navigate(RouteUrls.ConnectLedger, {
replace: true,
relative: 'route',
state: { tx: bytesToHex(psbt), inputsToSign },
state: {
tx: bytesToHex(psbt),
inputsToSign,
backgroundLocation: { pathname: RouteUrls.Home },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested pre 6262267 and we didn't need to do this before and I'm honestly not certain why we do now.

I have done local testing and confirmed I can send an ordinal from my ledger to my software wallet. Once it has arrived I will open this for review.

Other flows like send/btc/ don't need this backgroundLocation as they are page based rather than dialog based so I need to test them now.

Likely I will need to add some other code to ensure they have the correct background but that is a lesser issue than not being able to send ordinals at all on Ledger

Copy link
Collaborator

Choose a reason for hiding this comment

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

Other flows like send/btc/ don't need this backgroundLocation as they are page based rather than dialog based so I need to test them now.

MM, just read this. How can we set bg location dynamically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tested sending BTC and PSBT signing on Gamma from ledger and it's working.

I have a hook from before that re-directs if there is no background location so we could use that or else try and have a set location it takes from global state.

},
});
},

Expand All @@ -59,55 +63,101 @@ export function useLedgerNavigate() {
},

toConnectionSuccessStep(chain: SupportedBlockchains) {
return navigate(RouteUrls.ConnectLedgerSuccess, { replace: true, state: { chain } });
return navigate(RouteUrls.ConnectLedgerSuccess, {
replace: true,
state: {
chain,
backgroundLocation: { pathname: RouteUrls.Home },
},
});
},

toErrorStep(chain: SupportedBlockchains, errorMessage?: string) {
return navigate(RouteUrls.ConnectLedgerError, {
replace: true,
state: { latestLedgerError: errorMessage, chain },
state: {
latestLedgerError: errorMessage,
chain,
backgroundLocation: { pathname: RouteUrls.Home },
},
});
},

toAwaitingDeviceOperation({ hasApprovedOperation }: { hasApprovedOperation: boolean }) {
return navigate(RouteUrls.AwaitingDeviceUserAction, {
replace: true,
state: { hasApprovedOperation },
state: {
hasApprovedOperation,
backgroundLocation: { pathname: RouteUrls.Home },
},
});
},

toPublicKeyMismatchStep() {
return navigate(RouteUrls.LedgerPublicKeyMismatch, { replace: true });
return navigate(RouteUrls.LedgerPublicKeyMismatch, {
replace: true,
state: {
backgroundLocation: { pathname: RouteUrls.Home },
Copy link
Collaborator

Choose a reason for hiding this comment

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

This overlay modal isn't always on homepage fyi. It might show in on send tokens page, signPsbt RPC call etc

},
});
},

toDevicePayloadInvalid() {
return navigate(RouteUrls.LedgerDevicePayloadInvalid, { replace: true });
return navigate(RouteUrls.LedgerDevicePayloadInvalid, {
replace: true,
state: {
backgroundLocation: { pathname: RouteUrls.Home },
},
});
},

toOperationRejectedStep(description?: string) {
return navigate(RouteUrls.LedgerOperationRejected, {
replace: true,
state: { description },
state: {
backgroundLocation: { pathname: RouteUrls.Home },
description,
},
});
},

toDeviceDisconnectStep() {
return navigate(RouteUrls.LedgerDisconnected, { replace: true });
return navigate(RouteUrls.LedgerDisconnected, {
replace: true,
state: {
backgroundLocation: { pathname: RouteUrls.Home },
},
});
},

toBroadcastErrorStep(error: string) {
return navigate(RouteUrls.LedgerBroadcastError, { replace: true, state: { error } });
return navigate(RouteUrls.LedgerBroadcastError, {
replace: true,
state: {
backgroundLocation: { pathname: RouteUrls.Home },
error,
},
});
},

cancelLedgerAction() {
return navigate('..', { relative: 'path', replace: true, state: { ...location.state } });
// for send ordinal '..' brings you back to the `choose-fee` step but you lose context of the ordinal
const backLocation = location.pathname.match(RouteUrls.SendOrdinalInscription)
? RouteUrls.Home
: '..';
Comment on lines +144 to +147
Copy link
Collaborator

Choose a reason for hiding this comment

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

When we have to add logic like this, we might as well just write a switch statement to each possible case.

The idea is that .. is always the route from which the flow started. Any way we can solve this without having to do this?

Is it too late to just revert the /* routing set up 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's never too late to revert anything. And this is a real pain I agree.

Unfortunately .. seems to give un-predictable results and in this scenario with ledger it just sends them back to send/ordinal-inscription/choose-fee but doesn't keep the fee modal open.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I understand the problem, with /* anything that comes after is treated as a single route via wildcard match, thus breaking the ledger implementation that relies on route changes

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pete-watters wonder if you can try something:

We're missing a path on the outer route. I wonder if that means it thinks /* routes are the same, but if we put a path in, does that behaviour change?

export function ledgerSignTxRoutes({ component, customRoutes }: LedgerSignTxRoutesProps) {
  return (
    <Route path="/" element={component}>
      {customRoutes}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I'll test that now and try see if I can both fix this and change the pattern to be a bit cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kyranjamie if I add this route wrapper the whole APP won't even load, crashing with:
Screenshot 2024-04-19 at 11 56 25

I think it would be prudent to merge this fix for a P1 bug first, then continue efforts refactoring the routes to implement a more permanent solution for this and all of our routes.

The routing setup now is also proving tricky when implementing the Approval UX so I think it's worth re-reviewing it as a hole.

I checked back and I originally added the /* to the route to suppress this warning:
Screenshot 2024-04-19 at 12 00 16

I will take a look at the suggestions that provides to use / or * but I think that was causing some other issues.

It could be better to remove the /* get rid of this and accept that we will get this warning but have a simpler setup


return navigate(backLocation, {
relative: 'path',
replace: true,
state: { ...location.state },
});
},

cancelLedgerActionAndReturnHome() {
return navigate(RouteUrls.Home);
},
}),

[location.state, navigate]
[location, navigate]
);
}
Loading