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

Re-enable numeraires in onboarding #203

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Re-enable numeraires in onboarding #203

merged 1 commit into from
Sep 26, 2024

Conversation

grod220
Copy link
Contributor

@grod220 grod220 commented Sep 24, 2024

Bug fix. Now that a numeraire is available in the registry, Prax should be recognizing it and during onboarding, giving the users an opportunity to use it. This will enable usdc prices in minifront.

However, due to a react useEffect loading bug, it is skipping this step. This PR fixes that.

Screenshot 2024-09-24 at 9 45 29 PM

Comment on lines +12 to +13
retry: 1,
retryDelay: 0,
Copy link
Contributor Author

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.

Copy link
Contributor

@TalDerei TalDerei Sep 26, 2024

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?

Copy link
Contributor Author

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.

Comment on lines -17 to -21
if (!chainId) {
if (isError) {
console.error(`Could not load numeraires for chainId: ${chainId}`);
}
return [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why the !chainId conditional was necessary. Removing.

Comment on lines -28 to -33
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Comment on lines +30 to +35
// 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]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grod220 grod220 requested a review from a team September 24, 2024 19:50
@grod220 grod220 self-assigned this Sep 25, 2024
Copy link
Contributor

@TalDerei TalDerei left a comment

Choose a reason for hiding this comment

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

issue: the onboarding process can be completed without selecting a token denomination. Should we enforce a selection?

Screenshot 2024-09-25 at 7 39 52 PM

Comment on lines +12 to +13
retry: 1,
retryDelay: 0,
Copy link
Contributor

@TalDerei TalDerei Sep 26, 2024

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?

Copy link
Contributor Author

@grod220 grod220 left a comment

Choose a reason for hiding this comment

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

Should we enforce a selection?

Choosing a numeraire does not feel like a critical setting selection. If there is a fetching error, we want to skip it and not fail hard at this step. Think we are ok to keep it as optional.

Comment on lines +12 to +13
retry: 1,
retryDelay: 0,
Copy link
Contributor Author

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.

@grod220 grod220 merged commit cb29a43 into main Sep 26, 2024
3 checks passed
@grod220 grod220 deleted the numeraires-bug branch September 26, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants