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

handle wallet birthday height for non-mainnet chain ids #216

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

TalDerei
Copy link
Contributor

@TalDerei TalDerei commented Oct 16, 2024

references #214

unblocks beta testers to receive test funds

@TalDerei TalDerei self-assigned this Oct 16, 2024
@TalDerei TalDerei requested review from grod220 and a team October 16, 2024 16:06
Copy link
Contributor

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

I think I've realized that the core of the issue can be fixed be re-ordering the onboarding steps.

So instead of:

New seedphrase + birthday -> password -> rpc -> frontend -> numeraire

We can do:

rpc -> New seedphrase + birthday -> password -> frontend -> numeraire

That way, things downstream that require knowledge of the chain id don't have to assume mainnet. This prevents the need for us to have steps after to correct incorrect state.

Edit: In this case, we couldn't forget that we have to hold services init with the import-seed-phrase route completes the birthday step.

Copy link
Contributor

Choose a reason for hiding this comment

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

issue: This actually too late. The wallet birthday at this point has already been passed to the block processor. It will only pick up this change after the service worker resets.

Copy link
Contributor Author

@TalDerei TalDerei Oct 21, 2024

Choose a reason for hiding this comment

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

this is a great catch. I was under the impression that block syncing started after the last onboarding step, but it actually begins after selecting the rpc endpoint. I revised the logic and migrated it into the grpc page.

@TalDerei
Copy link
Contributor Author

TalDerei commented Oct 21, 2024

I think I've realized that the core of the issue can be fixed be re-ordering the onboarding steps.

This is tricky. I've considered this, but separating the rpc and frontend steps doesn't feels natural to the onboarding flow in my opinion.

@grod220 grod220 self-requested a review October 21, 2024 18:06
const onSuccess = (): void => {
// For beta testing: set the wallet block height to zero for non-mainnet chain IDs.
// This logic only runs after the user selects their rpc endpoint.
const onSuccess = async (): Promise<void> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: unfortunately, I still think this is running after services has initialized. Sadly it looks like the useGrpcEndpointForm is incredibly complicated. It's an example of where useEffect() adds complexity... See:

Copy link
Contributor Author

@TalDerei TalDerei Oct 22, 2024

Choose a reason for hiding this comment

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

tricky, maybe migrate the logic directly into onSubmit?

Copy link
Contributor Author

@TalDerei TalDerei Oct 22, 2024

Choose a reason for hiding this comment

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

like why can't we reorder the operations inside the onSubmit async function?

        await setGrpcEndpoint(grpcEndpointInput);

        const storedParams = await localExtStorage.get('params');
        if (storedParams) {
          const parsedParams = JSON.parse(storedParams) as AppParameters;
          if (!parsedParams.chainId.includes('penumbra-1')) {
            await localExtStorage.set('walletCreationBlockHeight', 0);
          }
        }
        
        // block service init
        
        void chrome.runtime.sendMessage(ServicesMessage.ClearCache);
              
         ...

       await onSuccess();

to ensure a proper sequencing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there must be a way to explicitly block on wallet service init until this code block completes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or we can block wallet service init directly inside onboardGrpcEndpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

settled on fcd8e41 for simplicity and readability.

@TalDerei
Copy link
Contributor Author

how can we test this with the beta chrome extension per penumbra-zone/web#1853? cc @VanishMax

@grod220
Copy link
Contributor

grod220 commented Oct 22, 2024

how can we test this with the beta chrome extension

Whenever we want, we can publish to the prax staging env:

https://chromewebstore.google.com/detail/prax-wallet-beta/ejpfkiblcablembkdhcofhokccbbppnc?authuser=0&hl=en

We should probably create a github action that we can manually run to help us with this

@grod220 grod220 mentioned this pull request Oct 23, 2024
@grod220
Copy link
Contributor

grod220 commented Oct 23, 2024

Alternative suggestion for using a beforeSubmit hook: #220

Copy link
Contributor

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

Could you write a few unit tests for correctBirthdayHeightIfNeeded()?

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.

3 participants