ceramicnetwork / js-did

A simple interface to interact with DIDs that conform to the DID-provider interface.
Other
96 stars 28 forks source link

Support passing DID in `getAuthMethod` #132

Open oed opened 1 year ago

oed commented 1 year ago

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

oed commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

oed commented 1 year ago

Thanks @zachferland !