input-output-hk / nami

Nami Wallet is a browser based wallet extension to interact with the Cardano blockchain. Support requests: https://iohk.zendesk.com/hc/en-us/requests/new
https://namiwallet.io
Apache License 2.0
372 stars 167 forks source link

CIP-30 implementation - missing {walletname} #103

Open codefly opened 2 years ago

codefly commented 2 years ago

According to current spec, the wallets should implement:

cardano.{walletName}.{methodname}

However it looks like what is currently implemented is cardano.{methodname}

Implementing the CIP30 API at the root level of the cardano object means this will cause conflicts if the user has multiple wallets installed.

vsubhuman commented 2 years ago

https://github.com/Berry-Pool/nami-wallet/blob/main/src/pages/Content/injected.js#L37-L53

Not only the injected object misses the wallet name, but also the initial inject is supposed to only have the enable, isEnabled, apiVersion, name, and icon fields. The rest of the functions must be in the full API object which is only returned (initialised) when the enable() is called. See "Initial API" in the CIP: https://github.com/cardano-foundation/CIPs/tree/master/CIP-0030#initial-api vs the "Full API".

Note also plz that object window.cardano is supposed to be potentially shared by multiple apps injecting their code, so it must initialized something like:

window.cardano = {
  ...(window.cardano||{}),
  nami: {
    ... // initial api
    enable(): Promise<FullAPI>
  }
}
rooooooooob commented 2 years ago

@alessandrokonrad simply was the first one to start implementing the spec while it was still just a draft PR and this was the original specification (without namespacing). We just merged the CIP-0030 PR this week so hopefully we can get the outstanding differences fixed up soon!

It should be noted that his difference with signData shouldn't be an issue as we simply wanted to get the first CIP PR merged right now and I already put up a PR for how we will handle that endpoint (which is more of how nami-wallet is doing it already)

alessandrokonrad commented 2 years ago

Yes will update the API soon, but will keep support for the current implementation for a short while, since a few dApps rely on it. Also CIP-30 does not support any events yet. The question is if I implement CIP-30 shall I strictly implement it according to the specs or is it okay to add my own custom event functions for now? @rooooooooob

rooooooooob commented 2 years ago

Yeah we didn't finalize the event API before we merged it, just like the data sign endpoint. That's why it was gutted last-minute. I would personally leave the data sign endpoint as-is (or try and follow what's in that PR but iirc the only difference is your address format) and maybe the events on your end but make sure you very clearly document that they are subject to change and are pending revisions to the CIP-30 spec, at least until we get somewhat of a consensus in on how we're handling events. It's always good to have something to play around with to figure out what works for dApp creators, but we also want to be very clear they it's not yet standardized and is subject to change. cardano-foundation/CIPs#151 I just made a pending PR to CIP-30 where we can hold the discussions on events until everyone settles on something.

Maybe for those endpoints you could have an experimental section after the CIP-30 conformant subset that talks about those and links to the PRs until we get them finalized.

mateusap1 commented 2 years ago

Any updates on this?