Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-0030 | Dapp-Connector #88
CIP-0030 | Dapp-Connector #88
Changes from 6 commits
9a5f0d9
3cbc121
7072b2d
d63230a
db41945
b473884
0b2488a
fa4e7da
0eba4d4
ce242ec
59ffb32
a34e86b
667849d
d4d67bb
a22e4f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, add more authors here to include people who have actively contributed to the writing of this specification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would we use for the threshold? Like @alessandrokonrad who has contributed significantly to discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't sufficient information because wallet state could change in-between pages (either new entries added or a rollback causing entries to be removed)
For the backend, instead of using arbitrary pages we instead give a concrete value like "all values after this address"
Due to this fact, I don't think it necessarily makes sense to have a unified paging system since the anchor to use depends on the data type you're looking for (txs, addresses, etc.)
If you feel this problem isn't really solvable, probably we should at least mention the pagination system has this possibility for error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using a dedicated namespace on the global object instead of prefixed functions. That is, have
Window.Cardano.request_read_access
etc ... ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. A
cardano
object is mentioned below; does it mean that we expect these two next functions to be available globally and to create the globalcardano
reference as a result 🤔 ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. The
cardano
object is first of all injected if thecardano_requests_read_access
function returns true.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did this two-phase so that the cardano object was separate and if it exists then it meant that the rest of the API was injected. It was originally all in there e.g.
cardano.request_read_access()
but we changed the decision later. However, if the user disconnects the wallet then dApps still need to handlecardano.*()
returning an error that the wallet is not connected, so it's possible we could revert to that behavior.At page load those 2 functions are injected, and if
cardano_request_read_access()
is successful then thecardano
object is injected with the rest of the API as alessandro mentioned.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea I had thought of previously was if
cardano_request_read_access()
returns aCardano
instance instead of injecting it asglobal.cardano
. We would still need to inject the type definition if it's not injected at wallet start, and if it is at wallet start we would want to control in the implementation to not allow dApps creating their own object from that type and trying to use it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well at first I had wanted to simplify things so that dApps wouldn't have access to those APIs until they had been granted access without having to check for API access denied errors in every endpoint, but it's not really a valid point once you realize that you'll need to handle those anyway if a wallet cancels access later on after injection. It does let us potentially go with that returning the
cardano
object idea instead of performing a 2nd injection so that was a consideration as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think just hiding the endpoints, doesn't fully prevent someone from accessing the wallet actually, since you communicate with window.postMessage. so the prevention needs to be in the extension itself. I would also prefer to move it under the same name space. and maybe make it shorter like:
cardano.enable
andcardano.is_enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alessandrokonrad It absolutely doesn't on its own. That is up to the wallet's implementation. When we implemented the ergo dApp connector spec in Yoroi we handle the connections from inside the Yoroi extension so Yoroi will only only respond to requests from pages that it considers connected, which can only happen if the connect request was accepted by the user (either by whitelisting the site or by the popup). You can't assume much at all in the way of security guarantees whatsoever in the injected code. That's why we treat the
yoroi-ergo-connector
(especially the injected code) as a trustless relay, and inside ofyoroi-frontend
we verify the inputs and keep track of connections.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerning this, we could potentially expand on this and other implementation-related security issues in the spec, but it might be outside of the scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed how these things work while I was reworking the wallet connection UX for when you have multiple wallets (e.g. Yoroi + AdaLite) that need to inject their API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to return a base64url (or whatever) encoded image for an icon for the wallet too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes an image would be great. Maybe as
URI
, so base64 and http/https links.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/cardano-foundation/CIPs/pull/88/files#r682429605
Can be implemented as a getter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
Change
wallet.requestReadAccess()
->wallet.enable()
wallet.checkReadAccess()
->wallet.isEnabled()
I like to keep things simple and short and it would also align more with the
ethereum
API.Maybe a
wallet.disable()
function would make sense as well (simply removing from whitelist)What about having
wallet.version()
,wallet.name()
as properties directly instead of funtions?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the reason why they had long names before was so we wouldn't inject code that would conflict with existing names, but now that it's something like
cardano.walletNameHere.requestReadAccess()
that sounds like a good idea to simplify it to justcardano.walletNamehere.enable()
.That could work too.
Are we imagining that
wallet.disable()
method to be for like a "sign out" button in the dApps? The way a whitelist is handled in the spec is left up to the wallet if and how they want to implement it, so whether this endpoint would simply end the session or remove seems like it would also be left up to the wallet. So I think the only guarantee we could make on this is that it disconnects the wallet from the page - subsequent calls toenable()
could still reconnect you without user interaction if the wallet implements a whitelist and the site is whitelisted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think the enable/disable functionality only really makes sense if the wallet has a whitelist, otherwise you don't really connect and disconnect and it's just a UI thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even without a whitelist it still makes some sense. You could have it simply disconnect the session between the dApp and the wallet, meaning no more access to the
api
endpoints until the dApp requests to connect again. This could be relevant for privacy-focused wallets/users who would like to control when the dApp can query their wallet I guess. Since the way i see it there are 3 possible connect/whitelist situations:wallet.requestReadAccess()
would resolve successfully without user input, and you could disconnect from a site without removing it from the whitelist.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What call would the dApp need to invoke to fetch the staking key of the wallet, e.g. to perform a stake delegation? It could infer it from the addresses returned via any of the already existing
get_*_address()
call but that seems hacky, so I'd consider adding aget_stake_address()
call tooThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added in a
api.getRewardAddresses()
endpoint.