-
Notifications
You must be signed in to change notification settings - Fork 24
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
[ID-1897] Improve magic session management #1965
base: main
Are you sure you want to change the base?
Conversation
…thub.com/immutable/ts-immutable-sdk into feature/ID-1897-magic-session-management
☁️ Nx Cloud ReportCI is running/has finished running commands for commit a765274. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
const user = await this.#authManager.getUser(); | ||
const zkEvmUser = this.#getZkEvmUser(user); |
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 will have unintended consequences on the passthrough methods when getUser
is called. Two scenarios that I can think of:
- If a token refresh is triggered, then we'll need to wait for that process to resolve before we can pass the method along to the node
- If a token refresh is triggered and it fails, then this error will be surfaced to the caller
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.
If a token refresh is triggered, then we'll need to wait for that process to resolve before we can pass the method along to the node
await this.#authManager.getUser();
already awaits the refreshing promise so I think this is fine?
If a token refresh is triggered and it fails, then this error will be surfaced to the caller
Edit: we can just fail silently on this check and let the methods logic handle it with how they see fit.
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.
https://immutable.atlassian.net/browse/ID-1897
Summary
Magic recommends that Passport integrates a little differently, specifically around checking if the magic user is logged in. This also affects how we initialise the magic signer.
Detail and impact of the change
Added
Changed
connectEvm
or theIMXProvider
is initialised.Deprecated
Removed
Fixed
Security
Anything else worth calling out?