-
-
Notifications
You must be signed in to change notification settings - Fork 179
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: added pairIdentifiers service #4269
Conversation
packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/types.ts
Show resolved
Hide resolved
@@ -50,6 +58,36 @@ export class JwtBearerAuth implements SIWEInterface, SRPInterface { | |||
return await this.#sdk.signMessage(message); | |||
} | |||
|
|||
async pairIdentifiers(pairing: Pair[]): Promise<void> { |
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.
Hmmm...
I'm hesitant if implementations should be done in this combined class vs in the individual implementations.
Context:
- Combined Class: 1 class; but poor type interface
- Individual Class: the developer can choose a specific implementation & has great type interface.
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.
Yes lets move the implementation to the individual classes, that way the better typed implementations can also utilise this method.
Either do this now, or in a separate implementation.
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.
Only annoying thing is that the implementation is identical on both implementations, so a bit redundant. (Sadly JS does not support the trait system lol).
Even though there would be duplication, I think it is nicer to have that abstraction.
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.
LGTM.
1 comment on adding the implementation to the grouped class.
Not against it, but originally that grouped class was supposed to be a thin wrapper around the individual implementations.
We can refactor this in another PR.
Explanation
Adding pairing profile functionality to the profile sync SDK.
@metamask/profile-sync-controller/sdk
Checklist