Skip to content
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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

matthewmuscat
Copy link
Contributor

@matthewmuscat matthewmuscat commented Jul 10, 2024

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

  • Checks if magic user is logged in
  • Addresses magic rate limiting issues by reducing the number of calls
  • Initialise the magic signer for all RPC calls, not when connectEvm or the IMXProvider is initialised.

Deprecated

Removed

Fixed

Security

Anything else worth calling out?

Copy link

nx-cloud bot commented Jul 22, 2024

☁️ Nx Cloud Report

CI 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


🟥 Failed Commands
nx affected -t typecheck --parallel=5
✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@matthewmuscat matthewmuscat changed the title ID-1897 WIP [ID-1897] Improve magic wallet initialisation Jul 24, 2024
@matthewmuscat matthewmuscat marked this pull request as ready for review July 24, 2024 23:46
@matthewmuscat matthewmuscat requested a review from a team as a code owner July 24, 2024 23:46
@matthewmuscat matthewmuscat changed the title [ID-1897] Improve magic wallet initialisation [ID-1897] Improve magic session management Jul 25, 2024
Comment on lines 146 to 147
const user = await this.#authManager.getUser();
const zkEvmUser = this.#getZkEvmUser(user);
Copy link
Contributor

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:

  1. 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
  2. If a token refresh is triggered and it fails, then this error will be surfaced to the caller

Copy link
Contributor Author

@matthewmuscat matthewmuscat Jul 26, 2024

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants