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

Emanuel-Palestino · 2024-07-18T19:01:36Z

Changes proposed in this pull request

Add an asset prop in liquidity dialog component to receive the asset info.
Get the asset info on the loader of withdraw and deposit components using the getPeer api method.
Get the asset info on the loader of withdraw and deposit components using the getAssetInfo api method.

Context

In the rafiki Admin UI, the liquidity information of peer and asset details are displayed using the current asset scale. When the user wants to deposit or withdraw liquidity, the amount introduced is handle using a scale of 0. So it is ambiguous on what scale the user is withdrawing or depositing.

Fixes: #2798

… withdraw liquidity components

…d withdraw liquidity components

netlify · 2024-07-18T19:01:54Z

✅ Deploy Preview for brilliant-pasca-3e80ec ready!

Name	Link
🔨 Latest commit	`4232609`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/66a913e487382a0008c71c42
😎 Deploy Preview	https://deploy-preview-2817--brilliant-pasca-3e80ec.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

packages/frontend/app/components/LiquidityDialog.tsx

BlairCurrey · 2024-07-22T15:01:14Z

packages/frontend/app/components/LiquidityDialog.tsx

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


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

Based on the asset:

Amount {amount} = {amount / Math.pow(10, asset.scale)} {asset.code} 



{amount / Math.pow(10, asset.scale)} {asset.code}{' '}



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} />

BlairCurrey

@Emanuel-Palestino thank you for the contribution - this looks great. Functionality is good, just left a couple style suggestions.

JoblersTune · 2024-07-22T19:58:50Z

Thanks Blair. Iwiill add my review on Wednesday as well. Just OOO right now.

Added styles for fields consistent Co-authored-by: Blair Currey <[email protected]>

Emanuel-Palestino · 2024-07-23T17:05:29Z

Hello @BlairCurrey , I was thinking about your suggestions using a simplified description and the possibility of adding a dummy input. I think the best option is to use a simplified description, I coded it.

I also thought in another approach:

I can use the addOn input prop to display the asset code in the input, so that with this change, the user can clearly know the unit of their deposit or withdrawal. Then, when the user submits the form, the amount can be captured and converted from original asset scale to 0 (which is the scale the backend expects to receive).
- Screenshot

I would like to know your feedback too @JoblersTune c:

JoblersTune · 2024-07-24T14:23:05Z

@Emanuel-Palestino that's a great suggestion to use the addOn input prop. I think you're onto something. Then we can abstract all the complexity foir the users entirely. What about something like this?

export const LiquidityDialog = ({
  title,
  onClose,
  type,
  asset
}: LiquidityDialogProps) => {
  const [actualAmount, setActualAmount] = useState<number>(0)
  const [errorMessage, setErrorMessage] = useState<string>('')

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

  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' />
      <div className='fixed inset-0 z-10 overflow-y-auto'>
        <div className='flex min-h-full items-center justify-center p-4 text-center'>
          <Dialog.Panel className='relative transform overflow-hidden rounded-lg max-w-full transition-all bg-white px-4 pb-4 pt-5 text-left shadow-xl w-full sm:max-w-lg'>
            <div className='absolute right-0 top-0 pr-4 pt-4'>
              <button
                type='button'
                className='text-gray-400 hover:text-gray-500 focus:outline-none'
                onClick={onClose}
              >
                <span className='sr-only'>Close</span>
                <XIcon className='h-8 w-8' aria-hidden='true' />
              </button>
            </div>
            <div>
              <Dialog.Title
                as='h3'
                className='font-semibold leading-6 text-lg text-center'
              >
                {title}
              </Dialog.Title>
              <div className='mt-2'>
                <Input
                  required
                  type='number'
                  name='displayAmount'
                  label='Amount'
                  onChange={handleChange}
                  addOn={asset.code}
                  step='any'
                  error={errorMessage}
                />
                <Form method='post' replace preventScrollReset>
                  <Input
                    required
                    min={1}
                    type='hidden'
                    name='amount'
                    value={actualAmount}
                  />
                  <div className='flex justify-end py-3'>
                    <Button aria-label={`${type} liquidity`} type='submit'>
                      {type} liquidity
                    </Button>
                  </div>
                </Form>
              </div>
            </div>
          </Dialog.Panel>
        </div>
      </div>
    </Dialog>
  )
}

This way with the visible input we can display the value in asset scale 0, the same way we display the liquidity info, but with the hidden field we can submit the value in the relevant asset scale.

What do you think?

JoblersTune · 2024-07-24T14:25:36Z

packages/frontend/app/routes/assets.$assetId.deposit-liquidity.tsx


  return (
    <LiquidityDialog
      onClose={dismissDialog}
      title='Deposit asset liquidity'
      type='Deposit'
+      asset={asset}


Suggested change

asset={asset}

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

I don't think I understand why you suggested this change, can you explain me please? 🙏

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

Great, it makes sense thank you

JoblersTune · 2024-07-24T14:25:50Z

packages/frontend/app/routes/assets.$assetId.withdraw-liquidity.tsx


  return (
    <LiquidityDialog
      onClose={dismissDialog}
      title='Withdraw asset liquidity'
      type='Withdraw'
+      asset={asset}


Suggested change

asset={asset}

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

JoblersTune · 2024-07-24T14:26:01Z

packages/frontend/app/routes/peers.$peerId.deposit-liquidity.tsx


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


Suggested change

asset={asset}

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

JoblersTune · 2024-07-24T14:26:10Z

packages/frontend/app/routes/peers.$peerId.withdraw-liquidity.tsx


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


Suggested change

asset={asset}

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

BlairCurrey · 2024-07-24T15:37:27Z

Hello @BlairCurrey , I was thinking about your suggestions using a simplified description and the possibility of adding a dummy input. I think the best option is to use a simplified description, I coded it.

I also thought in another approach:

I can use the addOn input prop to display the asset code in the input, so that with this change, the user can clearly know the unit of their deposit or withdrawal. Then, when the user submits the form, the amount can be captured and converted from original asset scale to 0 (which is the scale the backend expects to receive).

Screenshot

I would like to know your feedback too @JoblersTune c:

addOn looks great 👍. And converting to 0 on submit as you and Sarah are suggesting looks like a good solution.

Emanuel-Palestino · 2024-07-24T21:15:47Z

@JoblersTune that's a great implementation. I will work on it.

…s passed as a prop

… on change input event

Emanuel-Palestino · 2024-07-25T16:15:19Z

I have made the implementation. Waiting for your feedback c:

Additionally, working in the project I found and issue and a suggestion to improve de UX related to assets and peers pages.

In the asset details page, when a modal is opened, the scroll is setted to the top. This doesn't happen in peers details page.
I think it's a good idea to make de user input auto focusable, so that the user can type at the same time it opened the modal.

I don't know if I can make this changes in this pull request, or I have to open a new issue.

packages/frontend/app/components/LiquidityDialog.tsx

BlairCurrey · 2024-07-25T18:29:26Z

I have made the implementation. Waiting for your feedback c:

Additionally, working in the project I found and issue and a suggestion to improve de UX related to assets and peers pages.

In the asset details page, when a modal is opened, the scroll is setted to the top. This doesn't happen in peers details page.

I think it's a good idea to make de user input auto focusable, so that the user can type at the same time it opened the modal.

I don't know if I can make this changes in this pull request, or I have to open a new issue.

Looks good to me @Emanuel-Palestino. Just a small fix to make prettier happy. #2817 (comment)

The UX suggestions are good. Lets address that in another issue. Made one here: #2825 Feel free to assign yourself if you'd like.

Emanuel-Palestino · 2024-07-29T14:51:00Z

packages/frontend/app/components/LiquidityDialog.tsx

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

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?

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.

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.

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

@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.

Perfect, I'll make the changes c:

JoblersTune

Thank you so much for your positive and engaged contribution!

…posit input (#2817) * fix(frontend): asset scale consistency in liquidity dialogs. * Ensure asset scale consistency when displaying and withdrawing liquidity by adding asset info to the liquidity dialog component and updating the input handling in Rafiki Admin UI. --------- Co-authored-by: Blair Currey <[email protected]>

Emanuel-Palestino added 3 commits July 18, 2024 11:17

Added onchange handler in liquidity dialog

122f616

fix(frontend): [peers] getting asset details in loader of deposit and…

3a6e746

… withdraw liquidity components

fix(frontend): [assets] getting asset details in loader of deposit an…

0198a91

…d withdraw liquidity components

github-actions bot added the pkg: frontend Changes in the frontend package. label Jul 18, 2024

Emanuel-Palestino mentioned this pull request Jul 18, 2024

[BUG] Admin UI displays liquidity in one asset scale and withdrawal input in a different scale #2798

Closed

sabineschaller requested review from JoblersTune, raducristianpopa and BlairCurrey and removed request for raducristianpopa July 19, 2024 09:13

BlairCurrey changed the title ~~Fix: It is ambiguous on what scale is the withdrawal and deposit input~~ fix(frontend): It is ambiguous on what scale is the withdrawal and deposit input Jul 22, 2024

BlairCurrey reviewed Jul 22, 2024

View reviewed changes

packages/frontend/app/components/LiquidityDialog.tsx Outdated Show resolved Hide resolved

BlairCurrey reviewed Jul 22, 2024

View reviewed changes

Update packages/frontend/app/components/LiquidityDialog.tsx

21aebad

Added styles for fields consistent Co-authored-by: Blair Currey <[email protected]>

JoblersTune reviewed Jul 24, 2024

View reviewed changes

Emanuel-Palestino added 2 commits July 25, 2024 09:05

fix(frontend) [assets, peers] improved the type matching when asset i…

4ca92fe

…s passed as a prop

fix(frontend) Added asset code in user input and handle convertion in…

b988929

… on change input event

BlairCurrey reviewed Jul 25, 2024

View reviewed changes

packages/frontend/app/components/LiquidityDialog.tsx Outdated Show resolved Hide resolved

BlairCurrey mentioned this pull request Jul 25, 2024

LiquidityDialog UX improvements #2825

Closed

Format fixes

d39ffe0

Emanuel-Palestino requested review from JoblersTune and BlairCurrey July 26, 2024 22:15

JoblersTune reviewed Jul 27, 2024

View reviewed changes

fix(frontend) added non positive validation in liquidity dialog

4232609

Emanuel-Palestino requested a review from JoblersTune July 30, 2024 16:25

JoblersTune approved these changes Jul 31, 2024

View reviewed changes

JoblersTune merged commit 9b7fcdc into interledger:main Jul 31, 2024
36 of 42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Emanuel-Palestino commented Jul 18, 2024 •

edited by BlairCurrey

Loading

netlify bot commented Jul 18, 2024 •

edited

Loading

BlairCurrey Jul 22, 2024

BlairCurrey left a comment

JoblersTune commented Jul 22, 2024

Emanuel-Palestino commented Jul 23, 2024

JoblersTune commented Jul 24, 2024

JoblersTune Jul 24, 2024

Emanuel-Palestino Jul 24, 2024

JoblersTune Jul 25, 2024 •

edited

Loading

Emanuel-Palestino Jul 25, 2024

JoblersTune Jul 24, 2024

JoblersTune Jul 24, 2024

JoblersTune Jul 24, 2024

BlairCurrey commented Jul 24, 2024

Emanuel-Palestino commented Jul 24, 2024

Emanuel-Palestino commented Jul 25, 2024

BlairCurrey commented Jul 25, 2024

This comment was marked as resolved.

Emanuel-Palestino Jul 29, 2024

JoblersTune Jul 29, 2024

Emanuel-Palestino Jul 29, 2024 •

edited

Loading

JoblersTune Jul 30, 2024

Emanuel-Palestino Jul 30, 2024

JoblersTune left a comment

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

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

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

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

Conversation

Emanuel-Palestino commented Jul 18, 2024 • edited by BlairCurrey Loading

Changes proposed in this pull request

Context

netlify bot commented Jul 18, 2024 • edited Loading

✅ Deploy Preview for brilliant-pasca-3e80ec ready!

Choose a reason for hiding this comment

BlairCurrey left a comment

Choose a reason for hiding this comment

JoblersTune commented Jul 22, 2024

Emanuel-Palestino commented Jul 23, 2024

JoblersTune commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoblersTune Jul 25, 2024 • 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

BlairCurrey commented Jul 24, 2024

Emanuel-Palestino commented Jul 24, 2024

Emanuel-Palestino commented Jul 25, 2024

BlairCurrey commented Jul 25, 2024

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emanuel-Palestino Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoblersTune left a comment

Choose a reason for hiding this comment

Emanuel-Palestino commented Jul 18, 2024 •

edited by BlairCurrey

Loading

netlify bot commented Jul 18, 2024 •

edited

Loading

JoblersTune Jul 25, 2024 •

edited

Loading

Emanuel-Palestino Jul 29, 2024 •

edited

Loading