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(frontend): It is ambiguous on what scale is the withdrawal and deposit input #2817

Merged
merged 8 commits into from
Jul 31, 2024
Merged

fix(frontend): It is ambiguous on what scale is the withdrawal and deposit input #2817

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
122f616
Added onchange handler in liquidity dialog
Emanuel-Palestino Jul 18, 2024
3a6e746
fix(frontend): [peers] getting asset details in loader of deposit and…
Emanuel-Palestino Jul 18, 2024
0198a91
fix(frontend): [assets] getting asset details in loader of deposit an…
Emanuel-Palestino Jul 18, 2024
21aebad
Update packages/frontend/app/components/LiquidityDialog.tsx
Emanuel-Palestino Jul 23, 2024
4ca92fe
fix(frontend) [assets, peers] improved the type matching when asset i…
Emanuel-Palestino Jul 25, 2024
b988929
fix(frontend) Added asset code in user input and handle convertion in…
Emanuel-Palestino Jul 25, 2024
d39ffe0
Format fixes
Emanuel-Palestino Jul 25, 2024
4232609
fix(frontend) added non positive validation in liquidity dialog
Emanuel-Palestino Jul 30, 2024
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
17 changes: 16 additions & 1 deletion packages/frontend/app/components/LiquidityDialog.tsx
View file
Open in desktop

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I move the input out of the form, but I notice when the user submits the form it doesn't make the native html validation and it doesn't prevent the user from submitting any amount. The validation is made succesfully until the amount reaches the backend.
I understand that what you mention is also important. What do you think? @JoblersTune

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I was hoping the error message would cover that, even more so with your added minimum specification. Can you give me an example of where it is still failing @Emanuel-Palestino?

Copy link
Contributor Author

@Emanuel-Palestino Emanuel-Palestino Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure. When I submit a negative value or a very small value, it submits to the backend.

  • Negative values, the error message doesn't appear and the validation is made in the backend.
    image

  • With very small value, the error message appear (although it doesn't prevent to the user from submit). The validation is made in the backend.
    image

The message displayed at the top, is made by the backend. @JoblersTune these are the examples where it fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Emanuel-Palestino good catch on the negative values, thanks. We do most of our validation on the backend already. It creates a simpler frontend with faster load times and less JS complexity. We want to validate our input on the backend since that is where we'll be using it, so while it's nice to have frontend, reactive and immediate feedback it can lead to slower load times and some redundancy since backend validation is required as well.

So it's fine to get a server-side error for incorrect data entries. However, let's mitigate this by just adding a frontend message for the case where the value is negative as well, to give users a clearer error where we can.

const handleChange = (e: ChangeEvent<HTMLInputElement>) => {
    const userInput = e.target.value
    const scaledInput = parseFloat(userInput) * Math.pow(10, asset.scale)
    const integerScaledInput = Math.floor(scaledInput)
    if (scaledInput < 0) {
      const error = 'The amount should be a positive value'
      setErrorMessage(error)
    } else if (scaledInput !== integerScaledInput) {
      const error = 'The asset scale cannot accomodate this value'
      setErrorMessage(error)
    } else {
      setErrorMessage('')
    }
    setActualAmount(integerScaledInput)
  }

And then keep that input value above the form element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, I'll make the changes c:

Original file line number Diff line number Diff line change
@@ -1,19 +1,29 @@
import { Dialog } from '@headlessui/react'
import { Form } from '@remix-run/react'
import { useState } from 'react'
import { XIcon } from '~/components/icons'
import { Button, Input } from '~/components/ui'

type BasicAsset = {
code: string
scale: number
}

type LiquidityDialogProps = {
title: string
onClose: () => void
type: 'Deposit' | 'Withdraw'
asset: BasicAsset
}

export const LiquidityDialog = ({
title,
onClose,
type
type,
asset
}: LiquidityDialogProps) => {
const [amount, setAmount] = useState<number>(0)

return (
<Dialog as='div' className='relative z-10' onClose={onClose} open={true}>
<div className='fixed inset-0 bg-tealish/30 bg-opacity-75 transition-opacity' />
Expand Down Expand Up @@ -45,7 +55,12 @@ export const LiquidityDialog = ({
type='number'
name='amount'
label='Amount'
onChange={e => setAmount(Number(e.target.value))}
/>
<div className='text-gray-500 text-sm mt-2'>
<p>Based on the asset:</p>
<p>Amount {amount} = {amount / Math.pow(10, asset.scale)} {asset.code} </p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be simplified a bit. I don't think we need to restate the amount since its in the input nor that it's translating to the current asset scale because of the context.

I think this would be fine:

Suggested change
<p>Based on the asset:</p>
<p>Amount {amount} = {amount / Math.pow(10, asset.scale)} {asset.code} </p>
<p>
{amount / Math.pow(10, asset.scale)} {asset.code}{' '}
</p>

image

But perhaps a dummy input field for the asset scale really drives home the point (user can see 0 and 100 => 100, 2 and 100 => 1, etc.):

                  <Input
                    required
                    disabled
                    type='number'
                    label='Asset Scale'
                    value={asset.scale}
                  />

image

</div>
<div className='flex justify-end py-3'>
<Button aria-label={`${type} liquidity`} type='submit'>
{type} liquidity
Expand Down
27 changes: 22 additions & 5 deletions packages/frontend/app/routes/assets.$assetId.deposit-liquidity.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,45 @@
import { type ActionFunctionArgs } from '@remix-run/node'
import { useNavigate } from '@remix-run/react'
import { json, type ActionFunctionArgs } from '@remix-run/node'
import { useLoaderData, useNavigate } from '@remix-run/react'
import { v4 } from 'uuid'
import { LiquidityDialog } from '~/components/LiquidityDialog'
import { depositAssetLiquidity } from '~/lib/api/asset.server'
import { depositAssetLiquidity, getAssetInfo } from '~/lib/api/asset.server'
import { messageStorage, setMessageAndRedirect } from '~/lib/message.server'
import { amountSchema } from '~/lib/validate.server'
import { redirectIfUnauthorizedAccess } from '../lib/kratos_checks.server'
import { type LoaderFunctionArgs } from '@remix-run/node'
import { z } from 'zod'

export const loader = async ({ request }: LoaderFunctionArgs) => {
export const loader = async ({ request, params }: LoaderFunctionArgs) => {
const cookies = request.headers.get('cookie')
await redirectIfUnauthorizedAccess(request.url, cookies)
return null

const assetId = params.assetId

const result = z.string().uuid().safeParse(assetId)
if (!result.success) {
throw json(null, { status: 400, statusText: 'Invalid asset ID.' })
}

const asset = await getAssetInfo({ id: result.data })

if (!asset) {
throw json(null, { status: 404, statusText: 'Asset not found.' })
}

return json({ asset })
}

export default function AssetDepositLiquidity() {
const navigate = useNavigate()
const dismissDialog = () => navigate('..', { preventScrollReset: true })
const { asset } = useLoaderData<typeof loader>()

return (
<LiquidityDialog
onClose={dismissDialog}
title='Deposit asset liquidity'
type='Deposit'
asset={asset}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
asset={asset}
asset={{ code: asset.code, scale: asset.scale }}

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 don't think I understand why you suggested this change, can you explain me please? 🙏

Copy link
Collaborator

@JoblersTune JoblersTune Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure. Since the definition for the BasicAsset is only scale and code this makes it more clear what we are passing in (not the entire asset is used, only code and scale). In other words it has better type matching.

type BasicAsset = {
  code: string
  scale: number
}

type LiquidityDialogProps = {
  title: string
  onClose: () => void
  type: 'Deposit' | 'Withdraw'
  asset: BasicAsset
}

So having

return (
    <LiquidityDialog
      onClose={dismissDialog}
      title='Deposit asset liquidity'
      type='Deposit'
      asset={{ code: asset.code, scale: asset.scale }}
    />
)

Explicitly matches what we have set in the props definition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, it makes sense thank you

/>
)
}
Expand Down
27 changes: 22 additions & 5 deletions packages/frontend/app/routes/assets.$assetId.withdraw-liquidity.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,45 @@
import { type ActionFunctionArgs } from '@remix-run/node'
import { useNavigate } from '@remix-run/react'
import { json, type ActionFunctionArgs } from '@remix-run/node'
import { useLoaderData, useNavigate } from '@remix-run/react'
import { v4 } from 'uuid'
import { LiquidityDialog } from '~/components/LiquidityDialog'
import { withdrawAssetLiquidity } from '~/lib/api/asset.server'
import { getAssetInfo, withdrawAssetLiquidity } from '~/lib/api/asset.server'
import { messageStorage, setMessageAndRedirect } from '~/lib/message.server'
import { amountSchema } from '~/lib/validate.server'
import { redirectIfUnauthorizedAccess } from '~/lib/kratos_checks.server'
import { type LoaderFunctionArgs } from '@remix-run/node'
import { z } from 'zod'

export const loader = async ({ request }: LoaderFunctionArgs) => {
export const loader = async ({ request, params }: LoaderFunctionArgs) => {
const cookies = request.headers.get('cookie')
await redirectIfUnauthorizedAccess(request.url, cookies)
return null

const assetId = params.assetId

const result = z.string().uuid().safeParse(assetId)
if (!result.success) {
throw json(null, { status: 400, statusText: 'Invalid asset ID.' })
}

const asset = await getAssetInfo({ id: result.data })

if (!asset) {
throw json(null, { status: 404, statusText: 'Asset not found.' })
}

return json({ asset })
}

export default function AssetWithdrawLiquidity() {
const navigate = useNavigate()
const dismissDialog = () => navigate('..', { preventScrollReset: true })
const { asset } = useLoaderData<typeof loader>()

return (
<LiquidityDialog
onClose={dismissDialog}
title='Withdraw asset liquidity'
type='Withdraw'
asset={asset}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
asset={asset}
asset={{ code: asset.code, scale: asset.scale }}

/>
)
}
Expand Down
27 changes: 22 additions & 5 deletions packages/frontend/app/routes/peers.$peerId.deposit-liquidity.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,45 @@
import { type ActionFunctionArgs } from '@remix-run/node'
import { useNavigate } from '@remix-run/react'
import { json, type ActionFunctionArgs } from '@remix-run/node'
import { useLoaderData, useNavigate } from '@remix-run/react'
import { v4 } from 'uuid'
import { LiquidityDialog } from '~/components/LiquidityDialog'
import { depositPeerLiquidity } from '~/lib/api/peer.server'
import { depositPeerLiquidity, getPeer } from '~/lib/api/peer.server'
import { messageStorage, setMessageAndRedirect } from '~/lib/message.server'
import { amountSchema } from '~/lib/validate.server'
import { redirectIfUnauthorizedAccess } from '../lib/kratos_checks.server'
import { type LoaderFunctionArgs } from '@remix-run/node'
import { z } from 'zod'

export const loader = async ({ request }: LoaderFunctionArgs) => {
export const loader = async ({ request, params }: LoaderFunctionArgs) => {
const cookies = request.headers.get('cookie')
await redirectIfUnauthorizedAccess(request.url, cookies)
return null

const peerId = params.peerId

const result = z.string().uuid().safeParse(peerId)
if (!result.success) {
throw json(null, { status: 400, statusText: 'Invalid peer ID.' })
}

const peer = await getPeer({ id: result.data })

if (!peer) {
throw json(null, { status: 400, statusText: 'Peer not found.' })
}

return json({ asset: peer.asset })
}

export default function PeerDepositLiquidity() {
const navigate = useNavigate()
const dismissDialog = () => navigate('..', { preventScrollReset: true })
const { asset } = useLoaderData<typeof loader>()

return (
<LiquidityDialog
onClose={dismissDialog}
title='Deposit peer liquidity'
type='Deposit'
asset={asset}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
asset={asset}
asset={{ code: asset.code, scale: asset.scale }}

/>
)
}
Expand Down
27 changes: 22 additions & 5 deletions packages/frontend/app/routes/peers.$peerId.withdraw-liquidity.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,45 @@
import { type ActionFunctionArgs } from '@remix-run/node'
import { useNavigate } from '@remix-run/react'
import { json, type ActionFunctionArgs } from '@remix-run/node'
import { useLoaderData, useNavigate } from '@remix-run/react'
import { v4 } from 'uuid'
import { LiquidityDialog } from '~/components/LiquidityDialog'
import { withdrawPeerLiquidity } from '~/lib/api/peer.server'
import { getPeer, withdrawPeerLiquidity } from '~/lib/api/peer.server'
import { messageStorage, setMessageAndRedirect } from '~/lib/message.server'
import { amountSchema } from '~/lib/validate.server'
import { redirectIfUnauthorizedAccess } from '../lib/kratos_checks.server'
import { type LoaderFunctionArgs } from '@remix-run/node'
import { z } from 'zod'

export const loader = async ({ request }: LoaderFunctionArgs) => {
export const loader = async ({ request, params }: LoaderFunctionArgs) => {
const cookies = request.headers.get('cookie')
await redirectIfUnauthorizedAccess(request.url, cookies)
return null

const peerId = params.peerId

const result = z.string().uuid().safeParse(peerId)
if (!result.success) {
throw json(null, { status: 400, statusText: 'Invalid peer ID.' })
}

const peer = await getPeer({ id: result.data })

if (!peer) {
throw json(null, { status: 400, statusText: 'Peer not found.' })
}

return json({ asset: peer.asset })
}

export default function PeerWithdrawLiquidity() {
const navigate = useNavigate()
const dismissDialog = () => navigate('..', { preventScrollReset: true })
const { asset } = useLoaderData<typeof loader>()

return (
<LiquidityDialog
onClose={dismissDialog}
title='Withdraw peer liquidity'
type='Withdraw'
asset={asset}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
asset={asset}
asset={{ code: asset.code, scale: asset.scale }}

/>
)
}
Expand Down