fix: fix bug with sending ordinals from ledger, #ref 5253 #5268
Conversation
pete-watters
force-pushed
the
fix/5253/ledger-broken-send-ordinal
branch
from
e6f1935 to
aee828b
Compare
| state: { | ||
| tx: bytesToHex(psbt), | ||
| inputsToSign, | ||
| backgroundLocation: { pathname: RouteUrls.Home }, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
pete-watters
force-pushed
the
fix/5253/ledger-broken-send-ordinal
branch
from
aee828b to
b878f29
Compare
| return navigate(RouteUrls.LedgerPublicKeyMismatch, { | ||
| replace: true, | ||
| state: { | ||
| backgroundLocation: { pathname: RouteUrls.Home }, |
There was a problem hiding this comment.
This overlay modal isn't always on homepage fyi. It might show in on send tokens page, signPsbt RPC call etc
| // 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 | ||
| : '..'; |
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@kyranjamie if I add this route wrapper the whole APP won't even load, crashing with:
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:
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
|
@kyranjamie I've tested this and it fixes the bug of not being able to send ordinals from ledger so opening it for review. I started work on adding a test for send inscriptions but was having difficulty with the In general this approach of routes and background locations is awful and is causing us a lot of problems. We'll need to come up with a better solution for that but for now this should at least unblock users from sending on ledger. I'd been working on this set of fixes to other ledger issues, in between fighting fires, so will look at this again when improving that |
Will do. I took #4467 into my backlog so will get working on this. |
This PR fixes a bug with the
Send Ordinalflow when logged in with Ledger. It was introduced in 6262267The issue is that the ledger dialogs were not appearing underneath the
send-ordinalflow, only on ledger, due to them not having a dialog background location set.We had a similar problem with the
Choose feedialog here before: https://github.com/leather-wallet/extension/pull/4632/files.The underlying issue comes from the change made to make sure we overlay modals on the correct background.
Linking this PR for the record also: #4316