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

#4232 signer modal component #4362

Closed
wants to merge 10 commits into from
Closed

#4232 signer modal component #4362

wants to merge 10 commits into from

Conversation

IlyaSmiyukha
Copy link
Contributor

closes #4232
@thesan @chrlschwb
Check pls PR. I created a modal and stuck a bit with the backend. I'm not sure how properly connect the new API to the current frontend

@vercel
Copy link

vercel bot commented Apr 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
dao ❌ Failed (Inspect) Jun 10, 2023 9:34am
pioneer-2 ❌ Failed (Inspect) Jun 10, 2023 9:34am
pioneer-2-storybook ✅ Ready (Inspect) Visit Preview Jun 10, 2023 9:34am

Copy link
Contributor

@chrlschwb chrlschwb left a comment

Choose a reason for hiding this comment

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

ok

@chrlschwb chrlschwb requested a review from thesan April 21, 2023 16:05
@chrlschwb chrlschwb added community-dev issue suitable for community-dev pipeline jsg-code-review labels Apr 21, 2023
@IlyaSmiyukha
Copy link
Contributor Author

Great, thanks. Will check it

Copy link
Member

@thesan thesan left a comment

Choose a reason for hiding this comment

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

@IlyaSmiyukha great job creating the modal and the transaction flow, this can be painful write.

I have a few suggestion but most importantly this PR still needs to implement the BE mutation, please feel free to ask me on discord if you're stuck there.

Also please add a story for the EmailSubscriptionModal.


import { EmailSubscriptionForm } from './types'

export function createBatch(transactionParams: EmailSubscriptionForm, api: Api | undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

The "transaction" will actually just be a GQL mutation to the BE endpoint (because these membership data won't be stored on chain for confidentiality reasons), so this function won't be useful AFAICS.

packages/ui/src/memberships/types/Member.ts Outdated Show resolved Hide resolved
@thesan thesan added the scope:notifications Notifications subsystem label May 30, 2023
Copy link
Member

@thesan thesan left a comment

Choose a reason for hiding this comment

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

@IlyaSmiyukha if I understand correctly you want to implement the backend mutation in a different PR. I think it's a good idea. You could also choose to implement the signature step in a different PR and just keep the "prepare" step and the "transaction" step (just an infinite WaitModal for now).

This feature is pretty complex feature so I think splitting it into 3 PRs makes sense. WDYT ?

Comment on lines 34 to 43
return (
<SignTransactionModal
buttonText="Sign and subscribe"
transaction={createBatch(state.context.form, api)}
signer={member.controllerAccount}
service={state.children.transaction}
>
<TextMedium>You subscribed to emal.</TextMedium>
</SignTransactionModal>
)
Copy link
Member

Choose a reason for hiding this comment

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

Here too the logic shoiuld be similar to @common/modals/OnBoardingModal/OnBoardingModal:

Suggested change
return (
<SignTransactionModal
buttonText="Sign and subscribe"
transaction={createBatch(state.context.form, api)}
signer={member.controllerAccount}
service={state.children.transaction}
>
<TextMedium>You subscribed to emal.</TextMedium>
</SignTransactionModal>
)
return (
<WaitModal
onClose={hideModal}
title="Pending transaction"
description="Registering email address..."
/>
)

modalData: { member },
} = useModal<EmailSubscriptionModalCall>()
const [state, send] = useMachine(EmailSubscriptionMachine)

Copy link
Member

@thesan thesan May 31, 2023

Choose a reason for hiding this comment

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

I think the sign modal can be triggered by simply calling:

const signature = await api.sign(member.controllerAccount, `${member.id}:${timestamp}`)

Keep in mind the signup mutation is:

signup(memberId: String! signature: String! timestamp: BigInt! name: String! email: String): String

(As documented in the backend readme). So the signature and timestamp have to be sent too.

Copy link
Member

@thesan thesan left a comment

Choose a reason for hiding this comment

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

@IlyaSmiyukha I just realized that this PR targets dev but it should target backend.

Please:

  1. Retarget the PR (maybe you'll need to rebase the changes).
  2. Address the change requests below.
  3. Remove @/memberships/modals/EmailSubscriptionModal/utils.ts.

Then this PR can be merged, and you can implement the signature step in a new PR.

Comment on lines +23 to +32
const signModal = async () => {
const timestamp = new Date()
api?.sign(member.controllerAccount, `${member.id}:${timestamp}`)
}

useEffect(() => {
if (state.matches('signature')) {
signModal()
}
}, [state])
Copy link
Member

Choose a reason for hiding this comment

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

This is failing because the sign method is not implemented on the "ProxyApi". I think the best would be to implement this method as well as this logic in another PR.

Comment on lines +44 to +46
if (state.matches('transaction')) {
return <WaitModal onClose={hideModal} title="Pending transaction" description="Registering email address..." />
}
Copy link
Member

Choose a reason for hiding this comment

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

In the meantime this loader can show for both the signature and transaction steps:

Suggested change
if (state.matches('transaction')) {
return <WaitModal onClose={hideModal} title="Pending transaction" description="Registering email address..." />
}
if (state.matches('signature') || state.matches('transaction')) {
return <WaitModal onClose={hideModal} title="Pending transaction" description="Registering email address..." />
}

Comment on lines +34 to +42
if (state.matches('prepare')) {
return (
<EmailSubscriptionFormModal
onClose={hideModal}
onSubmit={(params: EmailSubscriptionForm) => send('DONE', { form: params })}
member={member}
/>
)
}
Copy link
Member

Choose a reason for hiding this comment

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

This step doesn't exist anymore

Comment on lines +13 to +15
interface TransactionContext {
signature?: EventRecord[]
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is relevant here

import { EmailSubscriptionForm } from './types'

interface EmailSubscriptionContext {
form?: EmailSubscriptionForm
Copy link
Member

Choose a reason for hiding this comment

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

The context should include the email, the timestamp, and the signature

@IlyaSmiyukha IlyaSmiyukha changed the base branch from dev to backend June 13, 2023 16:33
@IlyaSmiyukha IlyaSmiyukha changed the base branch from backend to dev June 15, 2023 14:40
@IlyaSmiyukha IlyaSmiyukha deleted the feature/email-subscription branch June 22, 2023 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-dev issue suitable for community-dev pipeline scope:notifications Notifications subsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Email notifications: signer modal component and transaction
3 participants