Sherzodbakhodirov/emb 253 implement sms webauthn for wallet kit#1037
Draft
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements SMS OTP (One-Time Password) login functionality for the wallet kit, adding it as an alternative authentication method alongside the existing email OTP login. The implementation generalizes the existing email-based OTP flow to support both email and SMS authentication methods with shared UI components and state management.
Changes:
- Added SMS OTP login functionality with new
SmsLoginContextandSmsInputcomponent - Refactored state management to use generic
identifier(email/phone) andotpLoginStatusinstead of email-specific naming - Extended type definitions to support SMS authentication events and login results
- Updated views and components to conditionally handle both email and SMS login flows
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
yarn.lock |
Added libphonenumber-js dependency for phone number validation |
packages/@magic-sdk/types/src/modules/auth-types.ts |
Added SMS OTP event types (emit and receive) for MFA and recovery code flows |
packages/@magic-sdk/provider/src/modules/auth.ts |
Added event handlers for SMS OTP MFA and recovery code verification |
packages/@magic-ext/wallet-kit/src/views/RecoveryCode.tsx |
Updated to use generic otpLoginStatus instead of emailLoginStatus |
packages/@magic-ext/wallet-kit/src/views/OtpView.tsx |
Refactored from EmailOTPView to support both email and SMS with conditional rendering |
packages/@magic-ext/wallet-kit/src/views/OAuthPendingView.tsx |
Updated to use otpLoginStatus for error checking |
packages/@magic-ext/wallet-kit/src/views/MfaView.tsx |
Updated to use generic otpLoginStatus |
packages/@magic-ext/wallet-kit/src/views/LoginView.tsx |
Added SMS input component and conditional error/loading state handling |
packages/@magic-ext/wallet-kit/src/views/LoginSuccessView.tsx |
Updated to use generic identifier instead of email-specific field |
packages/@magic-ext/wallet-kit/src/views/DeviceVerificationView.tsx |
Updated to use generic fields and SMS-aware display logic |
packages/@magic-ext/wallet-kit/src/types.ts |
Added SmsLoginResult type and InvalidPhoneNumber error message |
packages/@magic-ext/wallet-kit/src/reducer.ts |
Renamed email-specific state/actions to generic OTP equivalents |
packages/@magic-ext/wallet-kit/src/index.ts |
Exported new SmsLoginResult type |
packages/@magic-ext/wallet-kit/src/hooks/useMfa.ts |
Added SMS context support for MFA flows |
packages/@magic-ext/wallet-kit/src/extension.ts |
Added loginWithSMS method |
packages/@magic-ext/wallet-kit/src/context/SmsLoginContext.tsx |
New context provider implementing SMS OTP login flow with event handling |
packages/@magic-ext/wallet-kit/src/context/EmailLoginContext.tsx |
Updated action types to use generic OTP naming |
packages/@magic-ext/wallet-kit/src/components/SmsInput.tsx |
New component for SMS phone number input with validation |
packages/@magic-ext/wallet-kit/src/components/EmailInput.tsx |
Refactored to accept external error and loading state |
packages/@magic-ext/wallet-kit/src/MagicWidget.tsx |
Added SmsLoginProvider and updated view routing |
packages/@magic-ext/wallet-kit/package.json |
Added libphonenumber-js dependency |
Comments suppressed due to low confidence (3)
packages/@magic-ext/wallet-kit/src/views/OtpView.tsx:88
- The Button component uses the onPress prop throughout the codebase, not onClick. This should be changed to onPress={handleResend} to maintain API consistency with the rest of the codebase.
packages/@magic-ext/wallet-kit/src/views/DeviceVerificationView.tsx:30 - The DeviceVerificationView component only uses useEmailLogin().cancelLogin for both email and SMS flows. When loginMethod is 'sms', it should use useSmsLogin().cancelLogin instead to properly cancel the SMS login flow. Consider using conditional logic similar to the OtpView to select the correct context based on loginMethod.
const { cancelLogin } = useEmailLogin();
const { otpLoginStatus, identifier, loginMethod } = state;
useEffect(() => {
if (otpLoginStatus === 'device_approved') {
dispatch({ type: 'OTP_SENT' });
}
}, [otpLoginStatus]);
// For SMS login, device verification is done via SMS, not email
const isSms = loginMethod === 'sms';
return (
<>
<WidgetHeader onPressBack={cancelLogin} showHeaderText={false} />
packages/@magic-ext/wallet-kit/src/views/OtpView.tsx:55
- The icon naming convention in the codebase follows the "Ico" prefix pattern (e.g., IcoEmail, IcoArrowRight, IcoShield). However, IconSms uses "Icon" prefix instead. Verify that this is the correct export name from @magiclabs/ui-components. If the library exports it as IcoSms, it should be updated to match the convention throughout the codebase.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📦 Pull Request
[Provide a general summary of the pull request here.]
✅ Fixed Issues
🚨 Test instructions
[Describe any additional context required to test the PR/feature/bug fix.]
Please 🚨 ONLY ADD ONE 🚨 of the following labels, failing to do so may lead to adverse versioning of your changes when published:
patch: Bug Fix?minor: New Feature?major: Breaking Change?skip-release: It's unnecessary to publish this change.Special Note
Please avoid adding any of the
Prioritylabels as they conflict with the labels above ☝️