-
Notifications
You must be signed in to change notification settings - Fork 2
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
Re-enable numeraires in onboarding #203
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,26 +3,25 @@ import { ChainRegistryClient } from '@penumbra-labs/registry'; | |
import { useMemo } from 'react'; | ||
|
||
export const useNumeraires = (chainId?: string) => { | ||
const { data, isLoading, error, isError } = useQuery({ | ||
const { data, isLoading, isError } = useQuery({ | ||
queryKey: ['registry', chainId], | ||
queryFn: async () => { | ||
const registryClient = new ChainRegistryClient(); | ||
return registryClient.remote.get(chainId!); | ||
}, | ||
retry: 1, | ||
retryDelay: 0, | ||
staleTime: Infinity, | ||
enabled: Boolean(chainId), | ||
}); | ||
|
||
const numeraires = useMemo(() => { | ||
if (!chainId) { | ||
if (isError) { | ||
console.error(`Could not load numeraires for chainId: ${chainId}`); | ||
} | ||
return []; | ||
Comment on lines
-17
to
-21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why the !chainId conditional was necessary. Removing. |
||
if (isError) { | ||
console.error(`Could not load numeraires for chainId: ${chainId}`); | ||
} | ||
|
||
return data?.numeraires.map(n => data.getMetadata(n)) ?? []; | ||
}, [data, chainId, isError]); | ||
|
||
return { numeraires, isLoading, error }; | ||
return { numeraires, isLoading, isError }; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
import { AllSlices, useStore } from '../../state'; | ||
import { useChainIdQuery } from '../../hooks/chain-id'; | ||
import { useEffect, useState } from 'react'; | ||
import { ServicesMessage } from '../../message/services'; | ||
import { SelectList } from '@repo/ui/components/ui/select'; | ||
|
@@ -13,39 +12,36 @@ const useNumerairesSelector = (state: AllSlices) => { | |
selectedNumeraires: state.numeraires.selectedNumeraires, | ||
selectNumeraire: state.numeraires.selectNumeraire, | ||
saveNumeraires: state.numeraires.saveNumeraires, | ||
networkChainId: state.network.chainId, | ||
}; | ||
}; | ||
|
||
export const NumeraireForm = ({ | ||
isOnboarding, | ||
onSuccess, | ||
chainId, | ||
}: { | ||
chainId?: string; | ||
isOnboarding?: boolean; | ||
onSuccess: () => void | Promise<void>; | ||
onSuccess: () => void; | ||
}) => { | ||
const { chainId } = useChainIdQuery(); | ||
const { selectedNumeraires, selectNumeraire, saveNumeraires, networkChainId } = | ||
useStore(useNumerairesSelector); | ||
|
||
// 'chainId' from 'useChainIdQuery' is not available during onboarding, | ||
// this forces you to use two sources to guarantee 'chainId' for both settings and onboarding | ||
const { numeraires } = useNumeraires(chainId ?? networkChainId); | ||
Comment on lines
-28
to
-33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of doing both (serving the settings & onboarding simultaneously internally), I changed the component so that chainId is passed in |
||
const { selectedNumeraires, selectNumeraire, saveNumeraires } = useStore(useNumerairesSelector); | ||
const { numeraires, isLoading, isError } = useNumeraires(chainId); | ||
|
||
// If query errors or there aren't numeraires in the registry, can skip | ||
useEffect(() => { | ||
if (numeraires.length === 0) { | ||
void onSuccess(); | ||
if (isError || (!isLoading && numeraires.length === 0)) { | ||
onSuccess(); | ||
} | ||
}, [numeraires]); | ||
}, [numeraires.length, isError, isLoading]); | ||
Comment on lines
+30
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous source of the bug. During the loading state, the numeraires array was length zero. Hence it was being skipped. Now it only skips if there is an error or it's still loading. |
||
|
||
const [loading, setLoading] = useState(false); | ||
|
||
const handleSubmit = () => { | ||
setLoading(true); | ||
void (async function () { | ||
await saveNumeraires(); | ||
await chrome.runtime.sendMessage(ServicesMessage.ChangeNumeraires); | ||
await onSuccess(); | ||
void chrome.runtime.sendMessage(ServicesMessage.ChangeNumeraires); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does not feel necessary to await the response here, hold up the UI from progressing. The fire-and-forget works. |
||
onSuccess(); | ||
})(); | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A single retry feels ok, without specifying these, react-query tends to retry indefinitely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: seems like react-query defaults to three consecutive retries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Maybe it's the delay then that causes such long retry times. Alas, for our case, we want to fail fast given this is not a critical setting to select.