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

Support passing DID in getAuthMethod #132

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

Conversation

oed
Copy link
Member

@oed oed commented Jan 2, 2023

If I already know the DID it's annoying to have to create an AccountId.

@oed oed requested a review from zachferland January 2, 2023 11:59
@oed oed marked this pull request as ready for review January 2, 2023 11:59
@oed
Copy link
Member Author

oed commented Jan 2, 2023

Context: I built a library for halo chips over the holidays and turns out the Ceramic PKH integration is not so nice.
https://github.com/oed/halo-chip#usage-with-ceramic

@zachferland
Copy link
Contributor

Yeah nice to have the did <-> accountid helpers, maybe to keep the api/types stricter and simpler we could just add helper to go from did to accountid and keep authmethods with accountid, for example

getAuthMethod(ethProvider, getAccountIdByDID(didstring))

I could change it, if we wanted to do that.

I think I would only change to what is here, if we just wanted to consume dids instead of accountIds in the future or wanted to change docs to primarily handle/pass a did instead, which could make sense and is an option.

@oed
Copy link
Member Author

oed commented Jan 9, 2023

Why do we support AccountId in the first place and not DIDs? To me this seems a bit backwards. People are more likely to know what a DID is rather than AccountId I believe. that's why I changed it to accept both as a param.

I think I would only change to what is here, if we just wanted to consume dids instead of accountIds in the future or wanted to change docs to primarily handle/pass a did instead, which could make sense and is an option.

Right, this is where I'm leaning. Is there any reason we would prefer AccountId that I haven't thought of though?

@zachferland
Copy link
Contributor

@oed yeah remembered now that is was somewhat path dependent from 3id authproviders, and being based on the idea that a blockchain authmethod minimally needs a signer, address/account and network and it could be one of many authmethods (past 3id, future), in that context it made a lot of sense, to not confuse with your primary did (but of course didnt have did:pkh even and you did auth with a blockchain account), but even now your with account/address also being a did can be just as confusing as accountids, plus most come to auth with an address.

With did:pkh basically == accountid, it doesnt really matter now, I dont feel strongly. Would have just prefer to keep things more strictly typed and change with breaking change after or if there is use cases that differs enough we would just create additional eth authmethods and helpers specific to that use case and document separate and more clearly, that was one of the intentions with more minimal required interfaces here.

@oed
Copy link
Member Author

oed commented Jan 17, 2023

Ok, what do you suggest as a next step @zachferland ?

I still think that DID makes more sense as an abstraction to expose to users than accountId which is yet another concept.

@zachferland
Copy link
Contributor

Think that will probably be true, but either way trivial with some helper functions. Would prefer format mentioned above, with helper for dids, then trivial to use dids/address/accountid

getAuthMethod(ethProvider, getAccountIdByDID(didstring))

can even move those helpers to a pkh utils lib, to import/export in each these libs with less changes and without adding additional implementation details for other implementors

then at a later point, maybe switch to dids only in a breaking change

@oed
Copy link
Member Author

oed commented Feb 9, 2023

@zachferland sounds good, will go with the getAccountIdByDID approach. Is there a common package I can put it in?

Maybe just in did-session?

@zachferland
Copy link
Contributor

I can make the changes and open a pr, ill create a small story for it

@oed
Copy link
Member Author

oed commented Feb 22, 2023

Thanks @zachferland !

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.

2 participants