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

Add RPC selection to onboarding flow #868

Merged
merged 20 commits into from
Apr 3, 2024

Conversation

jessepinho
Copy link
Contributor

@jessepinho jessepinho commented Apr 1, 2024

screencap.mov

In this PR

  • Add an RPC selection step to the onboarding flow
  • Rework how RPC impls are defined, so that we don't define them until we have a gRPC endpoint set in local extension storage.
  • Add addListener/removeListener methods to ExtensionStorage, so that we can wait to make requests until the grpcEndpoint property exists in extension storage.
  • Use the above two points to wait to start up the extension services until an RPC endpoint has been chosen.
    • Note that this means we no longer get errors on extension install! Yay!

Next PR

  • Add RPC selection to the Settings page of the extension

Relates to #605

@jessepinho jessepinho force-pushed the jessepinho/rpc-endpoint-onboarding-web-605 branch from fe48dd9 to 6a956dd Compare April 2, 2024 00:59
await finalOnboardingSave(password);
navigate(PagePath.ONBOARDING_SUCCESS);
await onboardingSave(password);
navigate(PagePath.SET_RPC_ENDPOINT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I didn't make this change in the RestorePassword component. It doesn't seem to be part of the onboarding flow, and I actually am unclear on if that component is actually used anywhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to delete it. It's unused for like 8 months.

@jessepinho jessepinho force-pushed the jessepinho/rpc-endpoint-onboarding-web-605 branch from 5a8d317 to 1705365 Compare April 2, 2024 21:45
@jessepinho jessepinho force-pushed the jessepinho/rpc-endpoint-onboarding-web-605 branch from 5d6587a to 10efebb Compare April 3, 2024 00:32
@jessepinho jessepinho force-pushed the jessepinho/rpc-endpoint-onboarding-web-605 branch from 82b4c69 to 412d3b3 Compare April 3, 2024 01:03
* </SelectList>
* ```
*/
export const SelectList = ({ children }: { children: ReactNode }) => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@@ -0,0 +1,66 @@
import type { Meta, StoryObj } from '@storybook/react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

const handleSubmit = async (e: FormEvent<HTMLFormElement>) => {
e.preventDefault();
await setGrpcEndpointInZustand(grpcEndpoint);
void chrome.runtime.sendMessage(ServicesMessage.OnboardComplete);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@turbocrime please confirm that this is correct (moving OnboardComplete) to this step, since this is the new last step of onboarding.

@jessepinho jessepinho changed the title WIP: Add RPC selection to onboarding flow Add RPC selection to onboarding flow Apr 3, 2024
@jessepinho jessepinho requested a review from turbocrime April 3, 2024 03:35
@jessepinho jessepinho requested a review from grod220 April 3, 2024 03:35
@jessepinho jessepinho marked this pull request as ready for review April 3, 2024 03:35
apps/extension/src/get-rpc-impls.ts Show resolved Hide resolved
apps/extension/src/service-worker.ts Outdated Show resolved Hide resolved
await finalOnboardingSave(password);
navigate(PagePath.ONBOARDING_SUCCESS);
await onboardingSave(password);
navigate(PagePath.SET_RPC_ENDPOINT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to delete it. It's unused for like 8 months.

apps/extension/src/shared/rpc-endpoints.ts Outdated Show resolved Hide resolved
Comment on lines +17 to +20
const randomlySortedEndpoints = useMemo(() => [...RPC_ENDPOINTS].sort(randomSort), []);
const [grpcEndpoint, setGrpcEndpoint] = useState(randomlySortedEndpoints[0]!.url);
const setGrpcEndpointInZustand = useStore(state => state.network.setGRPCEndpoint);
const customRpcEndpointInput = useRef<HTMLInputElement | null>(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about moving all this logic into zustand with an RPC slice? My hunch is perhaps we'd want to re-use this within our settings tab in the extension and moving the logic out likely makes this easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It also reduces the redundancies with setGrpcEndpointInZustand & setGrpcEndpoint

Comment on lines +69 to +70
onSelect={() => {
if (!isCustomRpcEndpoint) setGrpcEndpoint('');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do some validation like the way we do in settings right now? If it cannot connect to the grpc url, we do not allow it to be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to take the liberty of doing this (and addressing your comment above about moving logic into Zustand) in a separate PR, since it looks like that'll require some (necessary) refactoring that I was anyway going to do when adding a similar RPC selection form to the extension's settings page.

packages/storage/src/chrome/base.ts Show resolved Hide resolved
Copy link
Collaborator

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

Left some comments, think there are some areas it would be helpful to add to

@jessepinho jessepinho merged commit 71b71e8 into main Apr 3, 2024
6 checks passed
@jessepinho jessepinho deleted the jessepinho/rpc-endpoint-onboarding-web-605 branch April 3, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants