feat: bitflow implemenation, #5860

kyranjamie · 2024-09-18T09:05:23Z

Try out Leather build 09f4ad6 — Extension build, Test report, Storybook, Chromatic

See @alexp3y's comments below

alexp3y · 2024-09-26T07:26:19Z

This PR implements Bitflow as the provider of swap functionality.

Overview

Adds Bitflow SDK to power token swap (available tokens, swap routes) and removes ALEX SDK usage.
Defaults to best route available.
Swap Confirmation screen now shows DEX hops as well as token Route.
Additional tokens now supported on swap (pricing information via ALEX so not all tokens have fiat values).

Implementation Details

Adds additional Webpack polyfills to support Bitflow SDK usage of dotenv package.
Requires new environment variables:
- BITFLOW_API_HOST
- BITFLOW_API_KEY
- BITFLOW_STACKS_API_HOST
- BITFLOW_READONLY_CALL_API_HOST
LP fee is estimated using logic similar to that on http://app.bitflow.finance/trade

Screenshots

Asset Sheet	Details

kyranjamie

Hey Alex, great work. Left some comments, but otherwise looks like a pretty solid implementation

kyranjamie · 2024-09-26T07:38:32Z

src/app/query/bitflow-sdk/bitflow-possible-swaps.query.ts

+  };
+}
+
+export function useGetBitflowPossibleSwapsQuery(token: string): UseQueryResult<SwapOptions, Error> {


Is this typing needed, or is it implicit given return value? Normally find it's more reliable to use the inferred type vs setting manually

kyranjamie · 2024-09-26T07:38:57Z

src/app/pages/swap/swap.utils.ts

+export function estimateLiquidityFee(dexPath: string[]) {
+  return 0.3 * dexPath.length;
+}


Suggested change

export function estimateLiquidityFee(dexPath: string[]) {

return 0.3 * dexPath.length;

}

export function estimateLiquidityFee(dexPath: string[]) {

return new BigNumber(dexPath.length).times(0.3).toNumber();

}

kyranjamie · 2024-09-26T07:41:13Z

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

+        if (!signedTx)
+          return logger.error('Attempted to generate raw tx, but signed tx is undefined');
+        return await broadcastStacksSwap(signedTx);
+      } catch (error) {}


What happens if the swap fails?

kyranjamie · 2024-09-26T07:43:41Z

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

+        value={`${Array.from(new Set(swapSubmissionData.dexPath))
+          .map(x => capitalize(x.toLowerCase()))
+          .join(', ')} via ${swapSubmissionData.protocol}`}


Ideally this component remains agnostic from any specific dex implementation. e.g. the dexPath property is specific to Bitflow right?

Wonder if this logic value/formatting logic could be defined in BitflowSwapContainer and pass in via the context?

Either way, would be nice to move into a function that lives outside the JSX

kyranjamie · 2024-09-26T07:45:09Z

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

+      };
+
+      if (token.tokenId === BITFLOW_STX_CURRENCY) {
+        const price = convertAmountToFractionalUnit(new BigNumber(prices[Currency.STX] ?? 0), 2);


magic number. What's 2 here?

kyranjamie · 2024-09-26T07:45:50Z

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

+
+      const swapAsset = {
+        currency: token.tokenId as Currency,
+        fallback: token.symbol.slice(0, 2),


not clear what this does? Helper function might explain better?

Essentially fetching price data and building the SwapAsset.

A lot of this is from alex-sdk.hooks.ts, which is where SwapAsset is defined. Most of the swap functionality is built around the SwapAsset concept so I've had to repurpose it to fit Bitflow types.

I'm sure this could be made more clear / efficient but we might want to first migrate the bitflow functionality into mono to keep it DRY.

I left a comment somewhere to revisit SwapAsset so that it before follows patterns we use with building our crypto assets in other parts of the app. Def a good idea to revisit once in the mono repo.

src/app/pages/swap/swap.context.ts

kyranjamie · 2024-09-26T07:47:10Z

webpack/webpack.config.base.js

+      fs: require.resolve('browserify-fs'),
+      path: require.resolve('path-browserify'),
+      os: require.resolve('os-browserify/browser'),
+      process: require.resolve('process/browser'),


Guess Bitflow isn't the best authored lib if we have to add tonnes of node polyfills 🤦🏼

pete-watters · 2024-09-26T08:12:03Z

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

@@ -76,8 +79,8 @@ export function SwapDetails() {
      />
      <SwapDetailLayout
        title="Liquidity provider fee"
-        tooltipLabel="To receive a share of these fees, become a Liquidity Provider on app.alexlab.co."
-        value={`${swapSubmissionData.liquidityFee} ${swapSubmissionData.swapAssetBase.name}`}
+        tooltipLabel="To receive a share of these fees, become a Liquidity Provider on app.bitflow.finance."


Should this be dynamic and change between alexlab.co & bitflow.finance?

Good question. Technically the underlying liquidity pools can span multiple DEXes. Might be weird to try and accommodate any / all LPs Bitflow could use in a swap. Maybe better to remove the comment? @markmhendrickson thoughts?

I think we can remove this tooltip in general and revisit later when we redesign swaps for mobile

As Mark says we can remove, but to make generic we could add a providerDomain to the context, and read that so <SwapDetails /> doesn't know about any given provider

pete-watters · 2024-09-26T08:15:22Z

src/app/routes/app-routes.tsx

@@ -41,7 +41,7 @@ import { RequestError } from '@app/pages/request-error/request-error';
 import { BroadcastError } from '@app/pages/send/broadcast-error/broadcast-error';
 import { sendOrdinalRoutes } from '@app/pages/send/ordinal-inscription/ordinal-routes';
 import { sendCryptoAssetFormRoutes } from '@app/pages/send/send-crypto-asset-form/send-crypto-asset-form.routes';
-import { alexSwapRoutes } from '@app/pages/swap/alex-swap-container';
+import { bitflowSwapRoutes } from '@app/pages/swap/bitflow-swap-container';


Are we replacing alex-swap-container with bitflow-swap-container?

Maybe we can get rid of the ALEX code then

Yeah, it's replacing. Wasn't sure if there'd be any value in keeping the ALEX stuff around, but it probably makes sense to just clear it out.

I would say if it's not much extra work, clear it out and add a comment to https://github.com/leather-io/issues/issues/99 to explain it's being removed.

We will be able to remove "alex-sdk": "2.1.3", then also and close off this #5852

I removed the existing ALEX swap components but the SwapAsset interface imported from mono has a reference to an ALEX SDK type that needs to be referenced. We should be able to refactor that in mono and remove that dependency here but for now it would be difficult to remove the SDK package entirely.

Sounds great 👍 Nice work

pete-watters · 2024-09-26T08:16:34Z

Nice work @alexp3y 👏

It's unclear to me if we are replacing ALEX swap with Bitflow. I thought we were keeping both and letting the user choose the best option?

If we are removing ALEX lets also get rid of the ALEX code

pete-watters · 2024-09-26T08:52:35Z

I tested building this branch and I couldn't get it up and running. I guess as it's still in flight.

I hit this error on fresh install:

I just wanted to double check how the Route looks in the action popup as we don't have much space there

(Action popup is what we call the non-full screen mode called from the extension button, we have a glossary here in case thats helpful)

fbwoolf

Nice work @alexp3y, great to see this being added into swaps.

markmhendrickson · 2024-09-27T11:29:41Z

Nice work, @alexp3y! Can we get a developer build attached to this PR for QA? 🙏

markmhendrickson · 2024-09-27T11:32:50Z

Just to confirm – are we filtering the tokens we show under balances with the BitFlow SDK now as well? Per this thread

I.e. we'll want to show by default all tokens that appear in either the BitFlow SDK and the ALEX SDK (unless the former SDK is essentially a superset of the latter, in which case we can just use BitFlow's), hiding all tokens that don't appear in the SDK(s)

fbwoolf · 2024-09-27T19:12:31Z

.github/workflows/development-extension.yml

@@ -13,6 +13,10 @@ env:
  SEGMENT_WRITE_KEY: ${{ secrets.SEGMENT_WRITE_KEY_STAGING }}
  TRANSAK_API_KEY: ${{ secrets.TRANSAK_API_KEY }}
  BESTINSLOT_API_KEY: ${{ secrets.BESTINSLOT_API_KEY }}
+  BITFLOW_API_HOST: ${{ secrets.BITFLOW_API_HOST }}


I wonder if I needed to add the " " around the value in GH secrets? I didn't so that might be why not working?

Maybe I'll change and rerun the tests to see if they pass.

That isn't right but I wonder if the urls are running into this? https://www.ssw.com.au/rules/handle-special-characters-on-github/

Do we need to keep the urls private, or can those just be exposed in the code as constants?

Good catch. I forgot to add the Bitflow variables to the playwright.yml workflow so the integration tests all broke.

Just added them and tests are passing now.

markmhendrickson · 2024-09-28T05:17:03Z

@314159265359879 could you and/or @DeeList QA this change with a few test swaps? 🙏

DeeList · 2024-09-28T18:47:27Z

@314159265359879 could you and/or @DeeList QA this change with a few test swaps? 🙏

WIll have a look and test today.

314159265359879

Overall swapping works well. I know that setting custom fee and nonce will be covered in a later update, as well as the option to set a custom slippage percentage, those are high on my wish list to make the swap more pro-friendly. I think it is really great to have the Bitflow integration there is support for so many more tokens all at once. Great for anyone who wants to do a quick swap.
I only tested with software wallet because the hardware wallet with app version 0.24.2 has a known issue.

There are so many tokens that it would make sense to add a filter bar at the top so users can type in a partial name or ticker to limit the amount that has to be scrolled through.
The amount on the other side isn't calculated until the user deselects the textbox/moved the cursor elsewhere. I think it is okay to have some delay after the user starts typing but generally it would be a better UX if the other side is calculated without having to click outside the the textbox.

2024-09-29_17-03-42.mp4

I expect this kind of loading as I type the value I want to swap I see how it changes the output I will get:

2024-09-29_17-13-22.mp4

The second step is also titled "Swap" it should be "Review Swap", right?

If these changes can't be part of the current scope, lets make new issues for them to cover later.

markmhendrickson · 2024-09-30T08:18:27Z

It seems we're ordering alphabetically by ticker but then the ordering restarts towards the end of the list, perhaps because there are two separate lists of tokens getting concatenated?

My sense is that it would be better to order (the two lists) in aggregate alphabetically by name instead of ticker. @fabric-8 to confirm the approach he prefers

Screen.Recording.2024-09-30.at.10.16.43.mov

markmhendrickson · 2024-09-30T08:22:13Z

It appears for tokens that don't have fiat values that we need to hide (or display - or similar) the value (instead of showing $0.00) here in the form (such as the case with ALUX). cc @fabric-8 for his preferred design guidance on handling

Edit: It should actually be possible calculate the fiat value of the received token by comparing to the value of the sent token?

alexp3y · 2024-09-30T08:23:18Z

Just to confirm – are we filtering the tokens we show under balances with the BitFlow SDK now as well? Per this thread

I.e. we'll want to show by default all tokens that appear in either the BitFlow SDK and the ALEX SDK (unless the former SDK is essentially a superset of the latter, in which case we can just use BitFlow's), hiding all tokens that don't appear in the SDK(s)

@markmhendrickson Looking at this now. The token filtering logic used in the Asset list is coming from mono, but I might be able to add something here temporarily to account for the newly supported tokens. Eventually we'll need to do a more thorough integration of Bitflow into the mono swap utilities.

markmhendrickson · 2024-09-30T08:23:35Z

This may be a preexisting issue, but I'm noticing that we round token images in the form view but not in the approval view, for at least the received token in case case for example:

markmhendrickson · 2024-09-30T08:27:42Z

Let's format this Powered by string with a final and e.g. Velar and Bitflow via Bitflow or (in cases of more) Velar, ALEX and Bitflow via Bitflow?

markmhendrickson

Needs some minor adjustments but otherwise looks great! 🚀

alexp3y · 2024-09-30T14:59:36Z

@markmhendrickson @fabric-8 Made the following adjustments -
https://github.com/leather-io/extension/actions/runs/11107763878/artifacts/1995824363

Asset balance filtering now include Bitflow tokens by default
Removed fiat quote on swap selection when no pricing available
Updated "Powered By" string format
Swap asset list ordered by display name (instead of ticker) and default fiat caption value to "-" when balance but no pricing available

markmhendrickson · 2024-10-01T05:46:17Z

Looks good! Small additional remaining nitpick (that may be preexisting and doesn't need resolution in this PR if complicated):

When the form is submitted, the amount for the received token reverts to the spinner instead of maintaining the numeric value. Ideally it would maintain the numeric value like the sent amount does above it.

alexp3y force-pushed the feat/bitflow branch 2 times, most recently from 9ad07b6 to f1cc29a Compare September 26, 2024 06:25

alexp3y marked this pull request as ready for review September 26, 2024 07:26

alexp3y requested review from fbwoolf, pete-watters, markmhendrickson and edgarkhanzadian September 26, 2024 07:26

kyranjamie commented Sep 26, 2024

View reviewed changes

pete-watters reviewed Sep 26, 2024

View reviewed changes

fbwoolf approved these changes Sep 26, 2024

View reviewed changes

alexp3y force-pushed the feat/bitflow branch from f1cc29a to 78414cb Compare September 27, 2024 11:31

alexp3y force-pushed the feat/bitflow branch 3 times, most recently from 9cc8be0 to 4e902ac Compare September 27, 2024 17:56

alexp3y enabled auto-merge September 27, 2024 18:09

alexp3y disabled auto-merge September 27, 2024 18:12

fbwoolf reviewed Sep 27, 2024

View reviewed changes

markmhendrickson requested a review from 314159265359879 September 28, 2024 05:16

alexp3y force-pushed the feat/bitflow branch from 4e902ac to f070d54 Compare September 28, 2024 08:39

markmhendrickson requested a review from DeeList September 29, 2024 05:36

314159265359879 reviewed Sep 29, 2024

View reviewed changes

markmhendrickson requested a review from fabric-8 September 30, 2024 08:18

markmhendrickson requested changes Sep 30, 2024

View reviewed changes

alexp3y force-pushed the feat/bitflow branch from f070d54 to 8e0548f Compare September 30, 2024 14:03

alexp3y requested a review from markmhendrickson September 30, 2024 15:00

markmhendrickson approved these changes Oct 1, 2024

View reviewed changes

alexp3y force-pushed the feat/bitflow branch 2 times, most recently from 53a0394 to 47937da Compare October 1, 2024 08:26

alexp3y enabled auto-merge October 1, 2024 10:05

alexp3y force-pushed the feat/bitflow branch from 47937da to df96d94 Compare October 1, 2024 10:58

feat: bitflow implemenation, closes leather-io/issues#99

09f4ad6

alexp3y force-pushed the feat/bitflow branch from df96d94 to 09f4ad6 Compare October 1, 2024 11:09

alexp3y added this pull request to the merge queue Oct 1, 2024

Merged via the queue into dev with commit 9c0a6e8 Oct 1, 2024
29 checks passed

alexp3y deleted the feat/bitflow branch October 1, 2024 11:13

feat: bitflow implemenation, #5860

feat: bitflow implemenation, #5860

Conversation

kyranjamie commented Sep 18, 2024 • edited by leather-bot Loading

alexp3y commented Sep 26, 2024

Overview

Implementation Details

Screenshots

kyranjamie left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-watters commented Sep 26, 2024

pete-watters commented Sep 26, 2024

fbwoolf left a comment

Choose a reason for hiding this comment

markmhendrickson commented Sep 27, 2024

markmhendrickson commented Sep 27, 2024

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

markmhendrickson commented Sep 28, 2024

DeeList commented Sep 28, 2024 • edited Loading

314159265359879 left a comment

Choose a reason for hiding this comment

markmhendrickson commented Sep 30, 2024

markmhendrickson commented Sep 30, 2024 • edited Loading

alexp3y commented Sep 30, 2024

markmhendrickson commented Sep 30, 2024

markmhendrickson commented Sep 30, 2024

markmhendrickson left a comment

Choose a reason for hiding this comment

alexp3y commented Sep 30, 2024

markmhendrickson commented Oct 1, 2024

kyranjamie commented Sep 18, 2024 •

edited by leather-bot

Loading

DeeList commented Sep 28, 2024 •

edited

Loading

markmhendrickson commented Sep 30, 2024 •

edited

Loading