-
Notifications
You must be signed in to change notification settings - Fork 457
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
refactor: environment config for social wallet #2728
Conversation
Branch preview⏳ Deploying a preview site... |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1100 tests passing in 153 suites. Report generated by 🧪jest coverage report action from 5b92d4b |
src/services/mpc/config.ts
Outdated
} | ||
|
||
/** env variables */ | ||
const SOCIAL_WALLET_IS_PRODUCTION = process.env.NEXT_PUBLIC_IS_PRODUCTION === 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is IS_PRODUCTION
in the constants.ts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this const and use IS_PRODUCTION
directly now.
src/services/mpc/config.ts
Outdated
|
||
const hasRequiredKeys = | ||
keys.length === 4 && | ||
keys.includes(SocialWalletOptionsKeys.web3AuthClientId) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation is a bit excessive/boilerplatey.
Surely there’s a more elegant way to check that an object has certain properties other than getting the array of keys and checking it against each of the required ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to use the in
operator to check if all keys exist in the object.
ab9fdc7
to
64b959b
Compare
src/services/mpc/config.ts
Outdated
try { | ||
return JSON.parse(IS_PRODUCTION ? SOCIAL_WALLET_OPTIONS_PRODUCTION : SOCIAL_WALLET_OPTIONS_STAGING) | ||
} catch (error) { | ||
console.error(error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.error(error) | |
console.error('Error parsing SOCIAL_WALLET_OPTIONS', error) |
@@ -18,9 +18,10 @@ export const useInitMPC = () => { | |||
useInitSocialWallet() | |||
|
|||
useEffect(() => { | |||
if (!chain || !onboard) { | |||
if (!chain || !onboard || !isSocialWalletOptions(SOCIAL_WALLET_OPTIONS)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this check at the entry point to this flow instead?
I.e. not even show the Google button if these env vars aren't set. This is important for forks.
Although, never mind. This feature needs to be explicitely toggled on in the config service anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the config service toggle only changes if the button is enabled or not. We could use the env vars to decide if the feature should be enabled or not though.
What it solves
Uses JSON in config for Social wallet to avoid having too many variables.
Checklist