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-0030 | Better describe API extension compatibility #486

Closed

Conversation

stevenj
Copy link
Contributor

@stevenj stevenj commented Mar 23, 2023

Adds specification to better describe the "Extensions Enable" API, with an effort to better control backwards and forwards compatibility.
Define a way to specify experimental extensions that do not compete with official extensions, and describe the path forward.
Add language tags to code blocks.
Use - consistently for unordered lists.
Properly declare HTTP urls.


CIP-0030 with this update, rendered in branch

@stevenj
Copy link
Contributor Author

stevenj commented Mar 23, 2023

This arose out of confusion related to CIP-62 and how to handle the API changes for extensions in a way that didn't break existing wallet implementations of the draft to cip-62.

For example, CIP-62 is still draft, so do we refer to it as {"cip",62} ? But its subject to changes, so how does the dApp and wallet know what CIP-62 is being requested.

The previous enable method takes no parameters. The new enable method takes 1 parameter which defaults to {} but what does {} mean? Does that mean that if a dApp calls enable() with the no parameter nothing is enabled? Not even CIP-30? If so, then that will break all backwards compatibility with any dApp that implements the previous enable method and has not been updated. So the specification I added here allows for backwards and forwards compatibility. dApps which implement enable() will still keep working. wallets which do implement the older enable() and current extensions like the draft CIP-62 will keep functioning and will still be compliant.

In a nutshell, this mostly consists of better documenting corner cases or previously undefined states.
It does not change the actual API surface.

@Ryun1 Ryun1 added the Update Adds content or significantly reworks an existing proposal label Mar 23, 2023
@Ryun1 Ryun1 changed the title docs(cip-30): Better describe API extensions with respect to backward/forward compat. CIP-0030 | Better describe API extensions with respect to backward/forward compat. Mar 23, 2023
@Ryun1 Ryun1 added the Category: Wallets Proposals belonging to the 'Wallets' category. label Mar 23, 2023
Copy link
Collaborator

@Ryun1 Ryun1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I quite like the goal and form of these additions.

This layouts a clear path for how extensions can be developed with backwards and forwards compatibility in mind. This aspect were perhaps overlooked when merging the last CIP-30 change in #446.

type Paginate = {|
page: number,
limit: number,
|};
```

Used to specify optional pagination for some API calls. Limits results to {limit} each page, and uses a 0-indexing {page} to refer to which of those pages of {limit} items each. dApps should be aware that if a wallet is modified between paginated calls that this will change the pagination, e.g. some results skipped or showing up multiple times but otherwise the wallet must respect the pagination order.

#### Extension
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would make extension discovery easier if we have a list of accepted extensions here in the CIP or an attached file to make it machine readable?

CIP-0030/README.md Outdated Show resolved Hide resolved

Extensions that are draft, in development, or prototyped should not use official "cip" numbering.

They should instead be referred to as "x-cip" and the number can be arbitrary and should not be expected to be unique.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the term "x-cip" is somewhat ambiguous, I see no reason we could use something like "experimental" or "draft-cip".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the convention used by the original mime std. i.e. https://www.rfc-editor.org/rfc/rfc1590#section-2.1
That said, I am not concerned about the actual label used, if "x-cip" is considered ambiguous.

Copy link
Collaborator

@rphair rphair Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, when I saw the prefix x- that is exactly what I thought: based on the MIME convention. So it seems intuitive & well precedented to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I didn't know about that :)

Copy link
Collaborator

@rphair rphair Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a quick look, view full headers in almost any email message 🤓

Then, new functionalities are introduced via additional CIPs and may be all or partially supported by wallets.

If the extensions list is not specified, it defaults to an empty list `{}`.
The wallet should treat an empty list as a request to enable **ALL** extensions which are not mutually exclusive.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By enabling all wallets do lose the ability to permission per extension.

Although nothing was stopping wallets from enabling all supported extensions regardless of what dApps were asking for - this would make wallet implementations simpler. Additionally, I am unsure of the real utility of permissioning access to extensions - I cannot think of a usecase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no security implication documented here in CIP-30 for enabling extensions. Nor how such a flow would work.
There is also no signalling back to the dApp that it's security restricted vs just not available.
For example, if a phone app asks to use the camera, it's told either "there is no camera" OR "you don't have permission to use the camera", which are different states to which the app can make different UX decisions.
If this is intended to be a security mechanism, then that should have proper consideration and specification.

CIP-0030/README.md Outdated Show resolved Hide resolved
CIP-0030/README.md Outdated Show resolved Hide resolved
CIP-0030/README.md Outdated Show resolved Hide resolved
CIP-0030/README.md Outdated Show resolved Hide resolved

Yes.
It would be a legitimate implementation of this CIP for a wallet to **ALWAYS** enable every extension it supports, regardless of the requested list of extensions.
The only caveat is that the [`cardano.{walletName}.extensions`](#cardanowalletnamesupportedextensions-extension) property must faithfully reflect the extensions which the wallet API object supports.

##### Should extensions follow a specific format?
Copy link
Collaborator

@Ryun1 Ryun1 Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to make things very explicit - so prospective extension CIPs should clearly outline and log their respective "x-cip" numbering/naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, wallets should list all extensions they support, both experimental/draft and finalized CIPs.

stevenj and others added 2 commits March 23, 2023 20:38
Typo

Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
@rphair rphair changed the title CIP-0030 | Better describe API extensions with respect to backward/forward compat. CIP-0030 | Better describe API extension compatibility Mar 23, 2023
Agree

Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
@Ryun1
Copy link
Collaborator

Ryun1 commented May 5, 2023

Further community input on this would great if possible, tagging some relevant parties @Scitz0 @refi93 @Quantumplation @rooooooooob 😎

@Scitz0
Copy link
Contributor

Scitz0 commented May 6, 2023

I dont have much to add to the conversation but looks fine to me.

@Ryun1
Copy link
Collaborator

Ryun1 commented Oct 3, 2023

I believe I have addressed the concerns raised by this proposal in the changes introduced in #577, do you still want to pursue this update proposal @stevenj?

@Ryun1
Copy link
Collaborator

Ryun1 commented Nov 17, 2023

Moving to close this, as similar changes were introduced in #577.

@stevenj please feel free to reopen if you desire.

@Ryun1 Ryun1 closed this Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Wallets Proposals belonging to the 'Wallets' category. Update Adds content or significantly reworks an existing proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants