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.

Sign up for GitHub

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

Jump to bottom

feat: connection service integration #1102

Merged
merged 8 commits into from
Nov 3, 2023
Merged

feat: connection service integration #1102

merged 8 commits into from
Nov 3, 2023

Conversation

0xmad
Copy link
Member

@0xmad 0xmad commented Oct 31, 2023 •
edited
Loading

Explanation

This PR integrates connection service with current demo and internal services.

Details are below:

  • Add connect/disconnect events
  • Get rid of origin url from metadata
  • Get rid of connected identity from identity service
  • Use connection service for one-to-many relation between identities and origins
  • Remove identity change event and select icon from home list

Related Issues

Closes #945
Closes #888
Closes #964

Screenshots

Expand

image

Manual Testing Steps

Check all the flows because these changes affects all the user scenarios.

Pre-Merge Checklist

Assignees Checklist

  • PR template is filled out
  • Pre-commit and pre-push hook checks are passed
  • E2E tests are passed locally
  • IF this PR fixes a bug, a test that would have caught the bug has been added
  • PR is linked to the appropriate GitHub issue
  • PR has been added to the appropriate release Milestone

Reviewers Checklist

  • Manual testing checked and passed.

@0xmad 0xmad added the 🌱 new feature Troubleshooting new feature issues label Oct 31, 2023
@0xmad 0xmad added this to the v0.6.0 milestone Oct 31, 2023
@0xmad 0xmad self-assigned this Oct 31, 2023
@0xmad 0xmad force-pushed the feature/connect-service-integration branch from b7c286f to da12265 Compare November 1, 2023 13:56
@0xmad
feat: connection service integration
a6c18b4 
- [x] Add connect/disconnect events
- [x] Get rid of origin url from metadata
- [x] Get rid of connected identity from identity service
- [x] Use connection service for one-to-many relation between identities and origins
@0xmad
refactor: injector service
d7cd6f4
@0xmad
chore: rename variables
b1a52a7
@0xmad 0xmad force-pushed the feature/connect-service-integration branch from da12265 to b1a52a7 Compare November 1, 2023 19:21
@0xmad
Copy link
Member Author

0xmad commented Nov 1, 2023

Main e2e tests are fixed here
https://github.com/CryptKeeperZK/crypt-keeper-extension/actions/runs/6725473350/job/18280423410

0xisk
0xisk requested changes Nov 2, 2023
View reviewed changes
Copy link
Member

@0xisk 0xisk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @0xmad! some comments, I am still reviewing it though.

packages/app/src/background/services/connection/ConnectionService.ts Show resolved Hide resolved
packages/app/src/background/services/connection/ConnectionService.ts Outdated Show resolved Hide resolved
packages/app/src/background/services/connection/ConnectionService.ts Show resolved Hide resolved
packages/app/src/background/services/group/GroupService.ts Show resolved Hide resolved
@0xmad 0xmad requested a review from 0xisk November 2, 2023 14:34
@0xisk
Copy link
Member

0xisk commented Nov 2, 2023

Please let's add the icons for the identities.

@0xmad 0xmad requested a review from 0xisk November 2, 2023 20:22
@0xisk 0xisk mentioned this pull request Nov 3, 2023
Open
0xisk
0xisk requested changes Nov 3, 2023
View reviewed changes
Copy link
Member

@0xisk 0xisk left a comment

Choose a reason for hiding this comment

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

I did testing for all the functions, and all works as expected. Just last point is that lets add the icons for each account depending on the commitment instead of the removed check icons.
Screenshot_20231103_143722

packages/app/src/background/services/injector/Injector.ts Show resolved Hide resolved
@0xmad 0xmad requested a review from 0xisk November 3, 2023 14:31
@0xmad 0xmad enabled auto-merge (squash) November 3, 2023 14:32
0xisk
0xisk approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@0xisk 0xisk left a comment

Choose a reason for hiding this comment

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

LGTM! thanks @0xmad!

@0xmad 0xmad merged commit c13c787 into main Nov 3, 2023
10 checks passed
@0xmad 0xmad deleted the feature/connect-service-integration branch November 3, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xisk 0xisk 0xisk approved these changes

@AtHeartEngineer AtHeartEngineer Awaiting requested review from AtHeartEngineer AtHeartEngineer is a code owner

Assignees

@0xmad 0xmad

Labels
🌱 new feature Troubleshooting new feature issues
Projects
Crypt-Keeper Extension
Status: ✅ Done
Milestone
v0.6.0
2 participants
@0xmad @0xisk