refactor: bitcoin swaps #6052

fbwoolf · 2024-12-31T20:41:40Z

Try out Leather build 2c7556b — Extension build, Test report, Storybook, Chromatic

This PR proposes a refactor to how we have swap containers currently setup. I've introduced a route param :origin here to identify the origin chain for the swap, which helps to create two separate context providers for Bitcoin and Stacks. One hook is used to aggregate all swappable assets, but based on the origin chain we can determine which quote assets are available for the swap. I think this will be easier to scale if we add other apis, or new chains in the future.

fbwoolf · 2025-01-02T16:45:20Z

Was thinking more on this ...if we don't want a new route param, which might be the case, now that I have the two context providers setup, I could easily determine the origin chain based on the base asset selected. I could add a new prop to the SwapAsset instead of using the route. Maybe we don't want the user to have to type that into the url path?

kyranjamie · 2025-01-03T10:58:05Z

Think declaring more details in the routes is a smart idea. Ideally, the route nav could be a fn outside the JSX to avoid inline string manip.

Is the idea that we then conditionally set the context based on the :origin param?

fbwoolf · 2025-01-03T14:49:12Z

Think declaring more details in the routes is a smart idea. Ideally, the route nav could be a fn outside the JSX to avoid inline string manip.

Is the idea that we then conditionally set the context based on the :origin param?

Great, I did a lot of work here refactoring to use the same pattern I setup in the mobile send forms. There is a generic context that both share and extend a base context. Look in SwapContainer to see where the provider is set based on the :origin route param and I use loaders to set the right data for each provider.

kyranjamie

Sorry for slow review @fbwoolf, this is a huge refactor, . Amazing work.

src/app/pages/home/components/account-actions.tsx

src/app/pages/swap/components/swap-asset-sheet/components/use-swap-asset-list.tsx

src/app/pages/swap/components/swap-details/bitcoin-swap-details.tsx

src/app/pages/swap/components/swap-review/swap-review.tsx

src/app/pages/swap/form/swap-form.tsx

kyranjamie · 2025-01-06T16:23:01Z

src/app/pages/swap/hooks/use-sbtc-deposit-transaction.tsx

-        btcSigner.sign(sBtcDeposit.transaction);
-        sBtcDeposit.transaction.finalize();
-        logger.info('Deposit', { deposit: sBtcDeposit });
+        signer.sign(deposit.transaction);


Can this be signed by software wallets only? Why doesn't it use our tx signer useSignBitcoinTx?

Yeah, I had a note to refactor with Ledger integration bc I just didn't realize that should be used. I'll look at changing here.

I ended leaving for now bc Ledger still hits this issue. I'll have to address in the follow up issue to get Ledger working with sBTC deposits...

kyranjamie · 2025-01-06T16:25:55Z

src/app/pages/swap/swap-container.tsx

+  switch (origin) {
+    case 'bitcoin':


Same thought here on routes: rather than one component doing the routing, we can catch the components in the routes. Think this is re-implementing router functionality

something like

// nested under parent routes <Route path="bitcoin" element={<ComponentWithAllTheBitcoinLoadersAndProvider />}

I actually don't think the router works this way? I've been trying to look at this, but I don't think your suggestion allows us to have the :origin be dynamic, and we need the provider to wrap all the routes, am I missing something? The provider/container is the parent route. The docs I've ref'd suggest using useParams() with dynamic routes to render components conditionally (like in a switch statement). We need it to be dynamic based on what the user selects from the list.

Maybe you are just saying to hard code it in the route rather than have it be dynamic?

Nvm here, I just had to rebuild my changes and works like I wanted. I thought I had missed something, but was just my local env.

Maybe you are just saying to hard code it in the route rather than have it be dynamic?

This, yeah. I had a poke around your code to understand better. My earlier suggestion doesn't work. I was thinking you can read statically defined values from the dynamic param, but you can't do that.

The route structure you've come up with is a smart way of routing to different containers, but it might be better to define separate routes, rather than use dynamic route params like :origin, because it isn't really dynamic. We have to manually make a loader component etc and route to separate components for both bitcoin, stacks, and any future origins. It's not like we're reading a random id, for example, they're predefined.

The component that "switches" based on a pre-defined value, is really doing the job of the router.

The origin value could equally be defined on and read from the container itself, not via the routes, while still using the same route pattern here.

This is similar to the routes in ledgerRequestKeysRoutes. Some incomplete code changes here, but that kinda show what I mean

also be careful using word origin because it is a global var. I've been caught out by this before, perhaps another name is better

Ok, great, this is the direction I started going in yest bc I couldn't get the other to work consistently like I wanted. Appreciate the feedback, and agree on all points. Will continue with this today.

@kyranjamie I think this is ready to look over again. I ended up just hardcoding a few new routes for each chain separately rather than doing the replace bc it just seemed cleaner for now. There are multiple places the route is needed including in a nec shared location for rpc openSwap.

I fixed the white screen. The zero BTC balance was returning null in the utxo loader. Currently, we allow zero balances in the list and the form to load if selected ...so I removed the returned null.

src/app/pages/swap/hooks/use-all-swappable-assets.tsx

kyranjamie · 2025-01-07T07:56:51Z

btw in your branch (with none of my suggested changes) I can get to blank screen

fbwoolf · 2025-01-08T00:49:56Z

btw in your branch (with none of my suggested changes) I can get to blank screen

How did you get here? I think the same might be happening in the failing test, but I can't recreate locally. I see it hitting a white screen when running the test, but I can't manually recreate using the same steps.

EDIT: Nvm, I see why. No need to review yet bc still refactoring.

kyranjamie · 2025-01-09T08:47:26Z

src/shared/route-urls.ts

@@ -72,11 +72,13 @@ export enum RouteUrls {
  SendOrdinalInscriptionError = 'error',

  // Swap routes
-  Swap = '/swap/:base/:quote?',
+  SwapBitcoinAsset = '/swap/bitcoin/:base/:quote?',


Suggested change

SwapBitcoinAsset = '/swap/bitcoin/:base/:quote?',

SwapBitcoinAsset = '/swap/{chain}/:base/:quote?',

Not necessary, but if you did wanna keep this less repetitive, can still replace the string when declaring the routes in the route gen fn

The reason I ended up not doing this is bc I can replace it when gen in the routes, but then every time I need to use it I also have to do a replace or export a shared const with the replace so we can dynamically change the :base and :quote. When I had it that way initially it felt less clean, but I can switch it back if you think better? I was just exporting then a group of routes with the replace 'SwapRouteUrls'.

Maybe then it makes sense just to keep it dynamic as :chain?

Happy to leave it as it it. Definitely not worth returning to dynamic routes.

It's a slight reduction in repetition to use "templated routes" (I'll call them), but not needed, and indeed you'd need to call .replace('{chain}', chain) in many places/

I think the templating is better for scaling so will change back this morn.

kyranjamie · 2025-01-09T09:31:49Z

src/app/pages/swap/components/swap-details/stacks-swap-details.tsx

+import { SwapDetailsLayout } from './swap-details.layout';
+import { getStacksSwapDataFromRouteQuote } from './swap-details.utils';
+
+function RouteNames(props: { router?: SwapAsset[] }) {


Another name/greater qualification might be better here, I thought this was to do with the router, not the route that the funds take across dexes

fbwoolf requested review from kyranjamie, alexp3y and alter-eggo December 31, 2024 20:41

fbwoolf force-pushed the refactor/bitcoin-swaps branch 2 times, most recently from 750a165 to 6a28a73 Compare January 6, 2025 14:23

kyranjamie reviewed Jan 6, 2025

View reviewed changes

kyranjamie reviewed Jan 7, 2025

View reviewed changes

src/app/pages/swap/hooks/use-all-swappable-assets.tsx Outdated Show resolved Hide resolved

fbwoolf force-pushed the refactor/bitcoin-swaps branch 3 times, most recently from 099a539 to 9556361 Compare January 8, 2025 00:47

fbwoolf force-pushed the refactor/bitcoin-swaps branch 3 times, most recently from d807ce5 to 76ae676 Compare January 8, 2025 19:52

fbwoolf requested a review from kyranjamie January 8, 2025 20:34

refactor: bitcoin swaps

2c7556b

fbwoolf force-pushed the refactor/bitcoin-swaps branch from 76ae676 to 2c7556b Compare January 8, 2025 21:34

kyranjamie reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: bitcoin swaps #6052

refactor: bitcoin swaps #6052

fbwoolf commented Dec 31, 2024 •

edited by leather-bot

Loading

fbwoolf commented Jan 2, 2025

kyranjamie commented Jan 3, 2025

fbwoolf commented Jan 3, 2025

kyranjamie left a comment

kyranjamie Jan 6, 2025

fbwoolf Jan 6, 2025 •

edited

Loading

fbwoolf Jan 8, 2025

kyranjamie Jan 6, 2025

fbwoolf Jan 6, 2025

fbwoolf Jan 6, 2025

fbwoolf Jan 6, 2025

kyranjamie Jan 7, 2025 •

edited

Loading

kyranjamie Jan 7, 2025

fbwoolf Jan 7, 2025

fbwoolf Jan 8, 2025

kyranjamie commented Jan 7, 2025

fbwoolf commented Jan 8, 2025 •

edited

Loading

kyranjamie Jan 9, 2025

fbwoolf Jan 9, 2025

fbwoolf Jan 9, 2025

kyranjamie Jan 9, 2025 •

edited

Loading

fbwoolf Jan 9, 2025

kyranjamie Jan 9, 2025

	SwapBitcoinAsset = '/swap/bitcoin/:base/:quote?',
	SwapBitcoinAsset = '/swap/{chain}/:base/:quote?',

refactor: bitcoin swaps #6052

Are you sure you want to change the base?

refactor: bitcoin swaps #6052

Conversation

fbwoolf commented Dec 31, 2024 • edited by leather-bot Loading

fbwoolf commented Jan 2, 2025

kyranjamie commented Jan 3, 2025

fbwoolf commented Jan 3, 2025

kyranjamie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbwoolf Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyranjamie Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyranjamie commented Jan 7, 2025

fbwoolf commented Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyranjamie Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbwoolf commented Dec 31, 2024 •

edited by leather-bot

Loading

fbwoolf Jan 6, 2025 •

edited

Loading

kyranjamie Jan 7, 2025 •

edited

Loading

fbwoolf commented Jan 8, 2025 •

edited

Loading

kyranjamie Jan 9, 2025 •

edited

Loading