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.

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: Nonce form #2120

Merged
merged 7 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
140 changes: 127 additions & 13 deletions src/components/tx-flow/common/TxNonce/index.tsx
Original file line number Diff line number Diff line change
@@ -1,29 +1,143 @@
import { type ChangeEvent, useCallback, useContext } from 'react'
import { Skeleton, Typography } from '@mui/material'
import { memo, type ReactElement, type SyntheticEvent, useCallback, useContext, useMemo } from 'react'

import {
Autocomplete,
Box,
IconButton,
InputAdornment,
Skeleton,
Tooltip,
Popper,
type PopperProps,
type AutocompleteValue,
type MenuItemProps,
MenuItem,
} from '@mui/material'
import { SafeTxContext } from '../../SafeTxProvider'
import RotateLeftIcon from '@mui/icons-material/RotateLeft'
import NumberField from '@/components/common/NumberField'
import { isMultisigExecutionInfo, isTransactionListItem } from '@/utils/transaction-guards'
import { uniqBy } from 'lodash'
import useTxQueue, { useQueuedTxByNonce } from '@/hooks/useTxQueue'
import useSafeInfo from '@/hooks/useSafeInfo'
import css from './styles.module.css'
import useAddressBook from '@/hooks/useAddressBook'
import { getLatestTransactions } from '@/utils/tx-list'
import { getTransactionType } from '@/hooks/useTransactionType'

const CustomPopper = function (props: PopperProps) {
return <Popper {...props} sx={{ width: '300px !important' }} placement="bottom-start" />
}

const NonceFormOption = memo(function NonceFormOption({
nonce,
menuItemProps,
}: {
nonce: number
menuItemProps: MenuItemProps
}): ReactElement {
const addressBook = useAddressBook()
const transactions = useQueuedTxByNonce(nonce)

const label = useMemo(() => {
const [{ transaction }] = getLatestTransactions(transactions)
return getTransactionType(transaction, addressBook).text
}, [addressBook, transactions])

return (
<MenuItem key={nonce} {...menuItemProps}>
{nonce} ({label} transaction)
</MenuItem>
)
})

const TxNonce = () => {
const { nonce, setNonce, safeTx } = useContext(SafeTxContext)
const { page } = useTxQueue()
const { safe } = useSafeInfo()
const safeNonce = safe.nonce || 0
Copy link
Member

Choose a reason for hiding this comment

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

You can use the hook useIsValidNonce instead (don't rememeber the exact name).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we have useValidateNonce but it returns a boolean. Not sure why we have this fallback here since safe.nonce can't be undefined but is -1 by default. Since we only use the value for validation I suggest we remove this fallback and use safe.nonce directly.

const { nonce, setNonce, safeTx, recommendedNonce } = useContext(SafeTxContext)
const isEditable = !safeTx || safeTx?.signatures.size === 0
const readonly = !isEditable

const queuedTxs = useMemo(() => {
if (!page || page.results.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove page.results.length === 0 here as that is an empty array which also return in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! I also wrote some unit tests to validate this.

return []
}

const txs = page.results.filter(isTransactionListItem).map((item) => item.transaction)

return uniqBy(txs, (tx) => {
return isMultisigExecutionInfo(tx.executionInfo) ? tx.executionInfo.nonce : ''
})
}, [page])

const options = useMemo(() => {
return queuedTxs
.map((tx) => (isMultisigExecutionInfo(tx.executionInfo) ? tx.executionInfo.nonce : undefined))
.filter((nonce) => nonce !== undefined) as number[]
}, [queuedTxs])
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this into a hook? Like usePreviousNonces?


// TODO: Add error/warning somewhere
const validateInput = (value: string) => {
if (!Number.isInteger(value)) {
return false
} else if (Number(value) < safeNonce) {
return false
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously we displayed error messages but with the new input field there is no room for it.


const onChange = useCallback(
(e: ChangeEvent<HTMLElement>) => {
const newNonce = Number(e.target.textContent)
setNonce(newNonce)
const handleChange = useCallback(
(e: SyntheticEvent, value: string | AutocompleteValue<unknown, false, false, false>) => {
const nonce = Number(value)
Copy link
Member

Choose a reason for hiding this comment

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

To me personally it always feels a bit weird if an Input puts in "0" when I clear it.
But Number("") is 0. It still works fine but feels weird if my nonce is "9" and I want to put in "10" for instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I think having the cursor on the right of the 0 is also confusing even if the 0 is replaced once I start typing another number. Previously it was possible to have the input empty. I will look into restoring that behaviour

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked into this and it seems we are storing the nonce as a number now and use undefined as the loading state so the current context doesn't give us the option to set the nonce to something other than a number. The input field is controlled by the context value so we would have to adjust the context i.e. store the nonce as a string but I am not sure if we want this. Wdyt @katspaugh

Copy link
Member

@katspaugh katspaugh Jun 19, 2023

Choose a reason for hiding this comment

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

The input can operate on strings and the context can operate on numbers.
You can probably do something like:

<Input
  value={String(nonce || '')}
  onChange={(e) => {
    setNonce(e.target.valueAsNumber)
  }}
/>

Copy link
Member Author

@usame-algan usame-algan Jun 19, 2023

Choose a reason for hiding this comment

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

Problem is that nonce will always be defined so String(nonce || '') would never result in an empty string. We can either change how we store nonce in the Context or make a copy of it for the nonce form and operate on that. The latter option was how we had it before where we copied the nonce value into RHF but it adds more complexity to the form since we have to keep it up to date depending on the queue.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added an additional isEmpty flag in the form now to achieve this.

if (isNaN(nonce)) return
setNonce(nonce)
},
[setNonce],
)

if (nonce === undefined) return <Skeleton variant="rounded" width={40} height={26} />

return (
<Typography variant="h4" fontWeight={700}>
#
<span className={css.input} contentEditable={isEditable} onBlur={onChange} suppressContentEditableWarning>
{nonce}
</span>
</Typography>
<Box display="flex" alignItems="center" gap={1}>
Nonce
<Autocomplete
value={nonce}
inputValue={nonce.toString()}
freeSolo
onChange={handleChange}
onInputChange={handleChange}
options={options}
disabled={readonly}
getOptionLabel={(option) => option.toString()}
renderOption={(props, option: number) => <NonceFormOption menuItemProps={props} nonce={option} />}
disableClearable
componentsProps={{
paper: {
elevation: 2,
},
}}
renderInput={(params) => (
<NumberField
{...params}
InputProps={{
...params.InputProps,
endAdornment: recommendedNonce !== undefined && recommendedNonce !== nonce && (
<InputAdornment position="end" className={css.adornment}>
<Tooltip title="Reset to recommended nonce">
<IconButton onClick={() => setNonce(recommendedNonce)} size="small" color="primary">
<RotateLeftIcon fontSize="small" />
</IconButton>
</Tooltip>
</InputAdornment>
),
readOnly: readonly,
}}
className={css.input}
/>
)}
PopperComponent={CustomPopper}
/>
</Box>
)
}

Expand Down
16 changes: 5 additions & 11 deletions src/components/tx-flow/common/TxNonce/styles.module.css
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
.input {
white-space: nowrap;
width: 200px;
overflow: hidden;
.input :global .MuiOutlinedInput-root {
padding: 0;
}

.input br {
display: none;
}

.input * {
display: inline;
white-space: nowrap;
.adornment {
margin-left: 0;
margin-right: 4px;
}
153 changes: 0 additions & 153 deletions src/components/tx/NonceForm/index.tsx

This file was deleted.