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

feat: add session activity system #1918

Merged
merged 1 commit into from
Jun 27, 2024
Merged

feat: add session activity system #1918

merged 1 commit into from
Jun 27, 2024

Conversation

cajames
Copy link
Contributor

@cajames cajames commented Jun 18, 2024

Hi👋, please prefix this PR's title with:

  • breaking-change: if you have introduced modification that necessitates immediate adjustments by this SDK's users to their applications, clients, or integrations to avert disruptions to existing features or functionalities.
  • feat:, fix:, refactor:, docs:, or chore:.

Summary

Detail and impact of the change

Added

Changed

Deprecated

Removed

Fixed

Security

Anything else worth calling out?

@cajames cajames force-pushed the kingmaking branch 6 times, most recently from 1951d4e to 41119f7 Compare June 21, 2024 12:05
@cajames cajames marked this pull request as ready for review June 23, 2024 07:57
@cajames cajames requested review from a team as code owners June 23, 2024 07:57
@cajames cajames requested a review from Jon-Alonso June 23, 2024 12:21
Jon-Alonso
Jon-Alonso previously approved these changes Jun 24, 2024
Copy link
Contributor

@Jon-Alonso Jon-Alonso left a comment

Choose a reason for hiding this comment

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

Tested locally against sandbox, lgtm

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so for SDK versions. But it's being phased out by the new tracking system. We'll eventually remove it completely.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was already removed by myself here 😄

This was approved on the basis that there are no references to it internally anymore and no one had used it in our team

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah awesome, removing it then.

packages/passport/sdk/src/zkEvm/types.ts Outdated Show resolved Hide resolved
@cajames cajames requested a review from Jon-Alonso June 24, 2024 02:08
@cajames cajames force-pushed the kingmaking branch 2 times, most recently from 5feed74 to b0e2882 Compare June 24, 2024 02:24
nattb8
nattb8 previously approved these changes Jun 24, 2024
Jon-Alonso
Jon-Alonso previously approved these changes Jun 24, 2024
Jon-Alonso
Jon-Alonso previously approved these changes Jun 24, 2024
nattb8
nattb8 previously approved these changes Jun 24, 2024
zaidarain1
zaidarain1 previously approved these changes Jun 24, 2024
pano-skylakis
pano-skylakis previously approved these changes Jun 24, 2024
@cajames cajames added this pull request to the merge queue Jun 24, 2024
@cajames cajames removed this pull request from the merge queue due to a manual request Jun 24, 2024
@cajames cajames dismissed stale reviews from pano-skylakis, zaidarain1, nattb8, and Jon-Alonso via 25070bf June 24, 2024 04:59
Comment on lines +118 to +121
passportEventEmitter.on(
PassportEvents.ACCOUNTS_REQUESTED,
trackSessionActivity,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be executed in the constructor of Passport.ts where we initialise the passportEventEmitter

@cajames cajames force-pushed the kingmaking branch 2 times, most recently from a450a22 to 7f4e825 Compare June 26, 2024 06:53
@cajames cajames added this pull request to the merge queue Jun 27, 2024
Merged via the queue into main with commit d4aa5e2 Jun 27, 2024
16 checks passed
@cajames cajames deleted the kingmaking branch June 27, 2024 02:44
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.

7 participants