feat(minifront): #1211: remove swap restrictions #1337

VanishMax · 2024-06-19T12:28:59Z

What's done:

extend BalanceSelector to show not only assets with balances but all available assets
fix the issues with empty wallets not rendering a swap block correctly
reduce the height of BalanceSelecor and AssetSelector to 90dvh
auto focus the search inputs in BalanceSelecor and AssetSelector
change validations of the swap input to allow entering any possible values

The balance selector now looks like this:

jessepinho

Great work! A lot of little notes here and there, but they're all relatively small things, so feel free to merge after addressing.

jessepinho · 2024-06-19T23:32:44Z

apps/minifront/src/components/shared/balance-selector.tsx

+  return false;
+};
+
+const mergeBalancesAndAssets = (balances: BalancesResponse[] = [], assets: Metadata[] = []) => {


suggestion (non-blocking): add a return type to make it clear what this function does

Suggested change

const mergeBalancesAndAssets = (balances: BalancesResponse[] = [], assets: Metadata[] = []) => {

const mergeBalancesAndAssets = (balances: BalancesResponse[] = [], assets: Metadata[] = []): (BalancesResponse | Metadata)[] => {

jessepinho · 2024-06-19T23:33:49Z

apps/minifront/src/components/shared/balance-selector.tsx

@@ -9,19 +9,101 @@ import { getAddressIndex } from '@penumbra-zone/getters/address-view';
 import { getDisplayDenomFromView, getSymbolFromValueView } from '@penumbra-zone/getters/value-view';
 import { Box } from '@repo/ui/components/ui/box';
 import { motion } from 'framer-motion';
+import { Metadata } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/asset/v1/asset_pb';
+import { filterMetadataBySearch } from './asset-selector';


issue: importing a helper from inside another component is a little messy.

suggestion: how about putting it in a ./helpers.ts file?

jessepinho · 2024-06-19T23:36:49Z

apps/minifront/src/components/shared/balance-selector.tsx


-const bySearch = (search: string) => (balancesResponse: BalancesResponse) =>
+const isMetadata = (asset: BalancesResponse | Metadata): asset is Metadata => {
+  return Boolean('symbol' in asset && 'name' in asset && 'display' in asset);


issue: I believe you can use asset.getType() to canonically get the message type.

e.g.,

Suggested change

return Boolean('symbol' in asset && 'name' in asset && 'display' in asset);

return asset.getType() === Metadata

This is provided by the bufbuild tooling, although I can't find docs for getType()

Thanks, didn't know about this! Got to modify the suggestion a little to return asset.getType().typeName === Metadata.typeName; to make it work

jessepinho · 2024-06-19T23:36:54Z

apps/minifront/src/components/shared/balance-selector.tsx

+};
+
+const isBalance = (asset: BalancesResponse | Metadata): asset is BalancesResponse => {
+  return 'balanceView' in asset && 'accountAddress' in asset;


same note here

jessepinho · 2024-06-19T23:46:57Z

apps/minifront/src/components/shared/balance-selector.tsx

+  onSelect: (value: BalancesResponse | Metadata) => void;
+}
+
+const BalanceItem = ({ asset, value, onSelect }: BalanceItemProps) => {


suggestion: could we move this into its own file?

This file is quite long. I'm generally opposed to having two components in the same file unless one is very small. But this one has its own helpers, etc., so the two are a bit tangled up.

I'd move BalanceSelector to shared/balance-selector/index.tsx, and BalanceItem to shared/balance-selector/balance-item.tsx

jessepinho · 2024-06-20T00:05:21Z

packages/ui/components/ui/icon-input.tsx

-  icon,
-  placeholder,
-}: {
+interface IconInputProps extends Omit<InputProps, 'onChange'> {


issue: we shouldn't be allowing all props to pass through to underlying components in our component design system

This is part of a bigger discussion*, but in short, we've had issues with our component system becoming quite messy because any props can be passed through. For example, this change to the props interface now allows children, className, etc. to be passed to <IconInput />. children doesn't make sense to pass to it, and className allows consumers to drastically change the appearance of <IconInput /> in ways they shouldn't be able to.

Instead, please just add autoFocus as a prop; we can keep adding more props as we need them, rather than allowing all props by default.

* I'm soon going to be converting #432 to a tracking issue so that we can gradually shift our design system components away from our current model.

Thanks for the detailed explanation, it really helped me understand the idea! Changed this to only add autoFocus

jessepinho · 2024-06-20T00:06:36Z

apps/minifront/src/state/swap/instant-swap.ts

@@ -31,6 +34,7 @@ import { divideAmounts } from '@penumbra-zone/types/amount';
 import { bech32mAssetId } from '@penumbra-zone/bech32m/passet';
 import { SwapSlice } from '.';
 import { sendSimulateTradeRequest } from './helpers';
+import { amountMoreThanBalance } from '../send';


Since amountMoreThanBalance is now used by multiple slices, please extract it to the ./helpers.ts file, so that slices aren't reaching into each other's code.

jessepinho · 2024-06-20T00:06:47Z

apps/minifront/src/state/swap/instant-swap.ts

+  if (!assetIn) throw new Error('`assetIn` is undefined');
+  if (!assetOut) throw new Error('`assetOut` is undefined');
+  if (!isValidAmount(amount, assetIn)) throw new Error('Invalid amount');


Good catch 👍🏻

jessepinho · 2024-06-20T00:07:13Z

apps/minifront/src/state/swap/index.ts

@@ -15,6 +15,7 @@ import {
  InstantSwapSlice,
  createInstantSwapSlice,
  instantSwapSubmitButtonDisabledSelector,
+  isValidAmount,


Please extract isValidAmount to ./helpers.ts, so that slices aren't reaching into each other's code.

jessepinho · 2024-06-20T00:08:18Z

apps/minifront/src/state/swap/index.ts

+  setAssetIn: (asset?: BalancesResponse) => void;
  setAmount: (amount: string) => void;
-  setAssetOut: (metadata: Metadata) => void;
+  setAssetOut: (metadata?: Metadata) => void;


Are these changes necessary? Won't they always be defined if these methods are being called?

Ah, I see why they're needed from the swap loader.

I have a PR (#1316) to remove this loader, so I guess it's up to you whether to rebase on that once it's merged and then change this requirement, or just change this requirement now and tweak the loader which is going to be removed soon anyway... if that makes sense 😆

Noticed the removal of swap loader. I added some changes to the zquery fetchers of swappableAssets and balancesResponses, so this code with optional parameters is irrelevant now

# Conflicts: # apps/minifront/src/components/shared/balance-selector.tsx # apps/minifront/src/components/swap/swap-form/token-swap-input.tsx # apps/minifront/src/components/swap/swap-loader.ts # apps/minifront/src/state/swap/index.ts

VanishMax added 3 commits June 19, 2024 14:23

feat(minifront): #1211: extend BalanceSelector to accept metadata assets

21f460f

feat(minifront): #1211: apply correct validations for swap action

223124c

chore: changesets

35b85b9

VanishMax requested a review from a team June 19, 2024 12:28

VanishMax self-assigned this Jun 19, 2024

VanishMax added 2 commits June 19, 2024 15:00

chore: fix linting

812d440

chore: fix linting

4498e7a

jessepinho approved these changes Jun 20, 2024

View reviewed changes

VanishMax added 4 commits June 21, 2024 12:24

fix(minifront): #1211: set initial swap assets correctly

e0909db

fix(minifront): #1211: make the code cleaner after the review

d88e6f1

fix(ui): #1211: don't let IconInput accept all InputProps

9199fdf

VanishMax merged commit 97b7231 into main Jun 21, 2024
6 checks passed

VanishMax deleted the feat/#1211-remove-swap-restrictions branch June 21, 2024 15:17

jessepinho mentioned this pull request Jul 2, 2024

asset selection drop down menu #1281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(minifront): #1211: remove swap restrictions #1337

feat(minifront): #1211: remove swap restrictions #1337

VanishMax commented Jun 19, 2024

jessepinho left a comment

jessepinho Jun 19, 2024

jessepinho Jun 19, 2024

jessepinho Jun 19, 2024

VanishMax Jun 21, 2024

jessepinho Jun 19, 2024

jessepinho Jun 19, 2024

jessepinho Jun 20, 2024

VanishMax Jun 21, 2024

jessepinho Jun 20, 2024

jessepinho Jun 20, 2024

jessepinho Jun 20, 2024

jessepinho Jun 20, 2024

jessepinho Jun 20, 2024

VanishMax Jun 21, 2024

	const mergeBalancesAndAssets = (balances: BalancesResponse[] = [], assets: Metadata[] = []) => {
	const mergeBalancesAndAssets = (balances: BalancesResponse[] = [], assets: Metadata[] = []): (BalancesResponse \| Metadata)[] => {

	return Boolean('symbol' in asset && 'name' in asset && 'display' in asset);
	return asset.getType() === Metadata

feat(minifront): #1211: remove swap restrictions #1337

feat(minifront): #1211: remove swap restrictions #1337

Conversation

VanishMax commented Jun 19, 2024

jessepinho 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