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

[Spec] Auth Multi Account #355

Merged
merged 3 commits into from
Dec 6, 2022
Merged

[Spec] Auth Multi Account #355

merged 3 commits into from
Dec 6, 2022

Conversation

flypaper0
Copy link
Contributor

@flypaper0 flypaper0 commented Dec 2, 2022

Description

To support multi-account feature SDK should not store Account object. Account will be passed with respond method.

This makes problematic to generate finalised SIWE message for clients. I suggest to open SIWEMessageFormatter interface to allow this:

protocol SIWEMessageFormatting {
    func formatMessage(from payload: PayloadParams, address: String) -> String?
}

@flypaper0 flypaper0 requested a review from llbartekll December 2, 2022 15:24
@vercel
Copy link

vercel bot commented Dec 2, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
walletconnect-docs ✅ Ready (Inspect) Visit Preview Dec 6, 2022 at 8:35AM (UTC)


// request wallet authentication
public abstract request(params: RequestParams, topic: string ): Promise<{ uri, id }>;

// respond wallet authentication
public abstract respond(params: RespondParams): Promise<boolean>;
public abstract respond(params: RespondParams, account: Account): Promise<boolean>;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the Account made of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Account removed from sdk initializer and moved here. We can call it iss. Account object is a wrapper around CAIP-10 string

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest not changing the name and sticking to the iss. In general, not all sdks introduced the Account so from the spec perspective it's more readable having iss

@jakubuid
Copy link
Contributor

jakubuid commented Dec 5, 2022

Description

To support multi-account feature SDK should not store Account object. Account will be passed with respond method.

This makes problematic to generate finalised SIWE message for clients. I suggest to open SIWEMessageFormatter interface to allow this:

protocol SIWEMessageFormatting {
    func formatMessage(from payload: PayloadParams, address: String) -> String?
}
  • What about adding formatMessage(from payload: PayloadParams, address: String) -> String? to the AuthSDK?
  • Why the return type is nullable?

@bkrem bkrem requested a review from ganchoradkov December 5, 2022 10:20
@flypaper0
Copy link
Contributor Author

Description

To support multi-account feature SDK should not store Account object. Account will be passed with respond method.
This makes problematic to generate finalised SIWE message for clients. I suggest to open SIWEMessageFormatter interface to allow this:

protocol SIWEMessageFormatting {
    func formatMessage(from payload: PayloadParams, address: String) -> String?
}
  • What about adding formatMessage(from payload: PayloadParams, address: String) -> String? to the AuthSDK?
  • Why the return type is nullable?

If payload chainId is invalid string we returns nil

@bkrem
Copy link
Member

bkrem commented Dec 5, 2022

What about adding formatMessage(from payload: PayloadParams, address: String) -> String? to the AuthSDK?

@jakobuid @flypaper0 this would be an internal util though wouldn't it, not part of the public API? Is there a case where this needs to be called from the top-level by an implementing dev?

EDIT: ah hold on I see now, so it gets bubbled up to top level via on("auth_request", ...), formatted with whatever account by the responder and then the responder adds that account to respond.

So yeah on board with Jakob's proposal to just make this part of the public API then rather than an appendix util.

@jakubuid
Copy link
Contributor

jakubuid commented Dec 5, 2022

What about adding formatMessage(from payload: PayloadParams, address: String) -> String? to the AuthSDK?

@jakobuid @flypaper0 this would be an internal util though wouldn't it, not part of the public API? Is there a case where this needs to be called from the top-level by an implementing dev?

EDIT: ah hold on I see now, so it gets bubbled up to top level via on("auth_request", ...), formatted with whatever account by the responder and then the responder adds that account to respond.

So yeah on board with Jakob's proposal to just make this part of the public API then rather than an appendix util.

Yup, because we have access to the iss/address only when it's passed by a dev, so when on("auth_request") is triggered, a dev calls formatMessage with payloadParams and address. It returns a SIWE that is displayed on the device. Then by pressing respond they pass the same iss/address and we send the signature.

Copy link
Contributor

@jakubuid jakubuid left a comment

Choose a reason for hiding this comment

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

@flypaper0 let's add formatMessage method to AuthSDK

@flypaper0 flypaper0 merged commit 8688f84 into main Dec 6, 2022
@chadyj chadyj deleted the feature/auth-multi-account branch June 29, 2023 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants