MagicV2 + Farcaster login + SMS login #9913
Conversation
| @@ -18,6 +21,12 @@ import { Mava } from './views/components/Mava'; | |||
|
|
|||
| OpenFeature.setProvider(openFeatureProvider); | |||
|
|
|||
| // need to instantiate it early because the farcaster sdk has an async constructor which will cause a race condition | |||
| // if instantiated right before the login is called; | |||
| export const defaultMagic = new Magic(process.env.MAGIC_PUBLISHABLE_KEY!, { | |||
There was a problem hiding this comment.
This might not be the best place to put it, but it needs to be instantiated when the app starts. Any better suggestions?
There was a problem hiding this comment.
not sure but does this cause circular dependency issues? or maybe we could add to packages/commonwealth/client/scripts/controllers/app/login.ts (that needs it - also login.ts is imported in many init flow conditions)?
There was a problem hiding this comment.
Yeah I moved it to login.ts. And yes this is the cause of the test failure issue. Not exactly a circular dependency but more of this Magic constructor calls the DOM, but our tests don't have the DOM so it throws an error.
But basically this is needed because their constructor needs to complete an async request before we can call login. So basically the earlier it is instantiated the better, but still no guarantee that login won't fail when it is called.
There was a problem hiding this comment.
Changes to this file were necessary to fix:
https://github.com/hicommonwealth/commonwealth/actions/runs/11843912133/job/33005933192?pr=9913
The issue was that login.ts now relies on the root entrypoint, causing some dependency issue. By passing in the app to these helpers instead of calling it directly, fixes this issue
| } else if (provider === WalletSsoSource.Farcaster) { | ||
| const bearer = await magic.farcaster.login(); | ||
|
|
||
| const { address } = await handleSocialLoginCallback({ | ||
| bearer, | ||
| walletSsoSource: WalletSsoSource.Farcaster, | ||
| }); | ||
|
|
There was a problem hiding this comment.
do we need to strictly follow this? can we reuse the else block logic below? That would redirect to /finish_social_login and then handleSocialLoginCallback is already called there along with some app init settings as well
There was a problem hiding this comment.
It's a little bit confusing but farcaster is not technically a social login. This is social logins require to use the webhook callback which farcaster does not use since it is decentralized. There is no entity (Like google or whatever) to submit the callback webhook for us.
This basically takes the same path as email login, the only difference is that it needs to call the magic farcaster login method
| @@ -116,7 +117,7 @@ export const ThreadCard = ({ | |||
| const linkedSnapshots = filterLinks(thread.links, LinkSource.Snapshot); | |||
| const linkedProposals = filterLinks(thread.links, LinkSource.Proposal); | |||
|
|
|||
| const isStageDefault = isDefaultStage(thread.stage, customStages); | |||
| const isStageDefault = isDefaultStage(app, thread.stage, customStages); | |||
There was a problem hiding this comment.
this might not work as expected in cases when thread card is used outside communities scope as app?.chain?.meta?.custom_stages would be undefined.
We can instead pass the customStages props to thread card, its already handled for non-community scope cases. For the remaining you'd need to pass customStages={app?.chain?.meta?.custom_stages} to thread card instances in community scope pages and then passing the whole app obj can be removed.
There was a problem hiding this comment.
This is the exact same logic as we had it previously so it will not break anything that was not already broken. The only difference here is that we pass it in the from the ThreadCard instead of importing it and using it in the helper file.
This is only needed for tests to pass due to some test login referencing this helper file and not having the DOM.
| @@ -174,16 +178,16 @@ export function renderMultilineText(text: string) { | |||
| * blocknum helpers | |||
| */ | |||
|
|
|||
| export function blocknumToTime(blocknum: number): moment.Moment { | |||
| export function blocknumToTime(app: IApp, blocknum: number): moment.Moment { | |||
There was a problem hiding this comment.
lets just only pass the props it needs -> block: IBlockInfo. We are trying to minimize and completely remove the app object.
There was a problem hiding this comment.
Good idea, fixed.
|
Don't have a farcaster account so will need to setup one to test. Will try to setup one and test. |
There was a problem hiding this comment.
- The confirmation window looks with broken styles. Not sure if we have control about it but used to work well.
- Not sure if this is part of this PR but getting 400s on searchUserProfile query on onboarding modal.
- So farcaster flow worked fine for me both on signup and signin. However google, discord, twitter didn;t work for me
kurtisassad
force-pushed
the
ka.magicV2
branch
from
0cb5107
to
84ce500
Compare
kurtisassad
changed the title
Link to Issue
Closes: #8713
Closes: #9028
Description of Changes
Note that the SSO v2 will not be able to work for custom domains. As a result I added extra logic to get the v2 to work for our domain only.
Test Plan
After logging in with it, it should prompt you to create an account