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

CIP-30 implementation - missing {walletname} #103

Open
codefly opened this issue Nov 18, 2021 · 5 comments
Open

CIP-30 implementation - missing {walletname} #103

codefly opened this issue Nov 18, 2021 · 5 comments

Comments

@codefly
Copy link

codefly commented Nov 18, 2021

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
Copy link

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
Copy link

@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
Copy link
Contributor

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
Copy link

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
Copy link

Any updates on this?

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

No branches or pull requests

5 participants