signature for email modal #4474
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
@IlyaSmiyukha technically it works but it's not an acceptable solution because creating api instances is resource intensive, this is why Atlas and Pioneer ended up moving the api instance on web workers (the page would freeze for users with lower resource computer). In this case the api instance is garbage collected and it's created again if the modal is reopened. I made sure of this using my browser profiler and the main thread does always spike shortly after opening the modal :
(the 1st row shows app screenshots, I couldn't figure out how to make it bigger).
Instead what I'm suggesting is to:
- On the client: Go to
@/proxyApi/client/ProxyApi.tsand add asign(account: string, message: any)method on the class. This method should callpostMessagewith a new message type and the parameters, and returnfirstValueFrom(messages.pipe(...))by also filtering from a new message type and mapping it to the signature value returned be the worker. - Then on the web worker: (
@/proxyApi/worker/index.ts) theapi.signmethod can be called with the correct value and when it resolves the signature should be passed back to the client withpostMessage(similarly to how it was done 1).
I haven't tried it but it seems the sign method is also called from the web worker on Atlas so it should work. You can check the other asynchronous messages passed to and from the web worker for reference and don't hesitate to ask me if you get stuck.
There was a problem hiding this comment.
Nice work ! It's great the wallet instance was usable for this instead of having to mess with the web worker.
Now please use send to update the machine context with the signature and timestamp.
Also I just merged the new App tests from dev into backend. So in packages/ui/src/app/App.stories.tsx could you please as part of this PR:
- Define the member emails for the tests/stories. This can be done easily by adding this to the mocks:
localStorage: { membersEmail: { alice: 'alice@example.com', bob: 'bob@example.com' } }
- Start adding a test for this flow. For this you will need a story where the emails above are not defined. To do that I usually use storybook
argsso that instead of just defining:membersEmail: { alice: 'alice@ex...I would define something like that:membersEmail: args.hasRegisteredEmail ? { alice: 'alice@ex... } : {}. Now by creating a story with{ args: { hasRegisteredEmail: false } }the modal will show and the test flow can start.
| const element = await screen.getByText('Sign up to email notifications') | ||
| expect(element) | ||
| await userEvent.click(screen.getByText('Not now')) | ||
| expect(element).not.toBeInTheDocument() |
Co-authored-by: Theophile Sandoz <theophile.sandoz@gmail.com>
Co-authored-by: Theophile Sandoz <theophile.sandoz@gmail.com>
Co-authored-by: Theophile Sandoz <theophile.sandoz@gmail.com>
Co-authored-by: Theophile Sandoz <theophile.sandoz@gmail.com>
|
@thesan |
| target: 'canceled', | ||
| action: 'BACK', |
There was a problem hiding this comment.
For this to work the BACK action should be define in the EmailSubscriptionEvent and also the target should be prepare
| @@ -16,6 +16,7 @@ type EmailSubscriptionState = | |||
| | { value: 'transaction'; context: Context } | |||
| | { value: 'success'; context: Context } | |||
| | { value: 'error'; context: Context } | |||
| | { value: 'canceled'; context: EmptyObject } | |||
There was a problem hiding this comment.
The state is "cancel" but I don't think it needs to be defined since it just goes back to the "prepare" state
| @@ -40,11 +54,17 @@ export const EmailSubscriptionModal = () => { | |||
| ) | |||
| } | |||
|
|
|||
| if (state.matches('error')) { | |||
| if (state.matches('error') || state.matches('canceled')) { | |||
There was a problem hiding this comment.
Also if the "cancel" state just goes back to the "prepare" state it shouldn't show the failure modal.
packages/ui/src/app/App.stories.tsx
Outdated
| await userEvent.type(modal.getByPlaceholderText('Add email for notifications here'), 'test@email.com') | ||
| await waitFor(() => expect(button.closest('button')).toBeEnabled()) | ||
| await userEvent.click(button) | ||
| expect(modal.getByText('Transaction was canceled.')) |
There was a problem hiding this comment.
At this point the machine gets in the cancel state because wallet? signer.signPayload is undefined, the code tries to call it, and all errors are assumed to be the user cancelling the signature. So this is not correct. Instead please change the accounts mock so you can pass it a storybook action called "sign" which will be added to the mock wallet. Currently WALLET is a constant in the module scope so this should be changed.
closes #4232