-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Adds Controller required for seed #14889
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
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
I have read the CLA Document and I hereby sign the CLA |
@@ -0,0 +1,80 @@ | |||
import { AuthSessionResult } from 'expo-auth-session'; |
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.
@chaitanyapotti I would recommend that you add these directories to the .github/CODEOWNERS
file.
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.
Left some comments
} from '@metamask/seedless-onboarding-controller'; | ||
import { Encryptor, LEGACY_DERIVATION_OPTIONS } from '../../../Encryptor'; | ||
|
||
export const web3AuthNetwork = process.env.Web3AuthNetwork as Web3AuthNetwork; |
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.
export const web3AuthNetwork = process.env.Web3AuthNetwork as Web3AuthNetwork; | |
export const web3AuthNetwork = process.env.WEB3_AUTH_NETWORK as Web3AuthNetwork; |
Also, should this be moved into a constants file or just use the env var inline
export const web3AuthNetwork = process.env.Web3AuthNetwork as Web3AuthNetwork; | ||
|
||
if (!web3AuthNetwork) { | ||
throw new Error('Missing environment variables'); |
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.
throw new Error('Missing environment variables'); | |
throw new Error('Missing environment variable WEB3_AUTH_NETWORK'); |
} | ||
|
||
const encryptor = new Encryptor({ | ||
keyDerivationOptions: LEGACY_DERIVATION_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.
I'm not familiar with the different types of derivation options but I know we're using it in many places. @ccharly Can you take a look at this if it's fine?
) { | ||
return baseControllerMessenger.getRestricted({ | ||
name: 'SeedlessOnboardingController', | ||
allowedEvents: [], |
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 don't have visibility to the seedless onboarding controller. Does the controller use actions or events of other controllers?
@@ -485,5 +485,8 @@ | |||
} | |||
] | |||
} | |||
}, | |||
"SeedlessOnboardingController": { |
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.
To be safe, let's also create a new migration to prepopulate the controller with this state
package.json
Outdated
@@ -332,6 +340,7 @@ | |||
"react-native-fs": "^2.20.0", | |||
"react-native-gesture-handler": "^1.10.3", | |||
"react-native-get-random-values": "^1.8.0", | |||
"react-native-google-acm": "git+https://github.com/Web3Auth/react-native-google-acm.git#edf4e52397f766d56d1644d908246e358f3cf774", |
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.
Did we bypass the paid version by forking our own and adding our own implementation?
} | ||
|
||
export class OAuthService { | ||
public localState: { |
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.
Extract types
ReduxService.store.dispatch({ | ||
type: UserActionType.LOADING_SET, | ||
payload: { | ||
loadingMsg: 'Logging in...', |
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.
Probably not necessary to pass in payload since this is static. We can just set it in the reducer itself
data: AuthResponse, | ||
authConnection: AuthConnection, | ||
): Promise<{ | ||
type: 'success' | '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.
Extract into enum
} | ||
|
||
if (!AuthServerUrl || !AuthConnectionId || !GroupedAuthConnectionId) { | ||
throw new Error('Missing environment variables'); |
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.
Let's move this condition to the top of the file below imports and specify explicitly, which env vars are missing
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.
|
d61cbbb
to
6895dbe
Compare
Description
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist