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

NEP: dApp Platform Provider Interface #69

Open
wants to merge 97 commits into
base: master
from

Conversation

@nickfujita
Copy link

commented Oct 3, 2018

A proposal for a standardized protocol interface between dApp developers and dApp API platform providers.

nickfujita added some commits Aug 28, 2018

@nickfujita nickfujita changed the title dApp Platform Provider Interface NEP: dApp Platform Provider Interface Oct 3, 2018

nep-dapi.mediawiki Outdated Show resolved Hide resolved
nep-dapi.mediawiki Outdated Show resolved Hide resolved
nep-dapi.mediawiki Outdated Show resolved Hide resolved
nep-dapi.mediawiki Outdated Show resolved Hide resolved
@evgenyboxer
Copy link

left a comment

Great job!

Here are couple of other useful methods: (all are promises)

  1. signMessage - Given a payload, allow the user to sign the message contents.
  2. getNetwork - Returns the users network - MainNet/TestNet/PrivateNet.
  3. isLoggedIn - Returns true if logged in.
  4. isReady - Might be specific to nex extension, but there are certain checks that need to be done before accepting calls from the dapp.

Events:

  1. network_changed - fired when a user changes his network.
  2. account_changed - fired when a user logs in/out.
  3. ready - fired when the provider is ready.

let me know what you think @nickfujita

nep-dapi.mediawiki Outdated Show resolved Hide resolved
nep-dapi.mediawiki Outdated Show resolved Hide resolved
@igormcoelho

This comment has been minimized.

Copy link

commented Aug 2, 2019

@nickfujita I've added some comments, sorry if thats too much, we can close them if not nevessary. I know this is nearly final (or final), and much has been discussed already (I didnt follow this conversation from beggining), so many things here may already be final decision (at least some typos can be corrected ;) )

author: string,
email: string,
description: string,
needsStorage?: boolean,

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Aug 2, 2019

Perhaps these features can stay in a struct named features here... like features : { storage : true, dynamicInvoke : true, payable : true }, which is easier to extend in the future (or personalize by protocol, Neo2, Ont, Neo3, ...).

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 5, 2019

Author

How is updating attributes in a features struct any different than updating the same on the main object. In either case, such attributes will have to be added or removed, and checked for by name.

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Aug 6, 2019

Just for better organization perhaps.... contract.info.name , ... contract.features.hasStorage ...

isPayable?: boolean;
parameterList: string;
returnType: string;
code: string;

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Aug 2, 2019

code should be named script, in my opinion.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 5, 2019

Author

I have no opposition to this. As long as all other wallet providers and dapps are okay with making the update.

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Aug 6, 2019

If people are already using like this, thats fine. But I think that some libraries/sdk could easily adjust stuff when this is final.

NETWORK_CHANGED = 'NETWORK_CHANGED',
CONNECTED = 'CONNECTED',
DISCONNECTED = 'DISCONNECTED',
BLOCK_HEIGHT_CHANGED = 'BLOCK_HEIGHT_CHANGED',

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Aug 2, 2019

BLOCK_HEIGHT_CHANGED, or BLOCKCHAIN_HEIGHT_CHANGED (or just HEIGHT_CHANGED) ?

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 5, 2019

Author

I have no opposition to this. As long as all other wallet providers and dapps are okay with making the update.

This comment has been minimized.

Copy link
@igormcoelho

=====ACCOUNT_CHANGED=====

On a ACCOUNT_CHANGED event, the callback will fire with a single argument of the new account. This occurs when an account is already connected to the dapp, and the user has changed the connected account from the dapi provider side.

This comment was marked as resolved.

Copy link
@igormcoelho

igormcoelho Aug 2, 2019

typo dapi -> dAPI

nickfujita added some commits Aug 5, 2019

@nickfujita

This comment has been minimized.

Copy link
Author

commented Aug 5, 2019

@igormcoelho Thank you for your detailed and thoughtful feedback. I have made updates where appropriate, and left my comments inline above.

@corollari

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Given that currently metamask is having a lot of problems maintaining backwards compatibility with old dApps in the face of breaking changes in web3, would it be possible to learn from their mistakes and instead of providing an interface that is supposed to be consumed directly by the developers just provide a provider object that can be used with a js library that will have to be directly included by the developers themselves? This way it's possible to make breaking changes to the developer-facing API without having to break all old dApps.

This is not really an issue with this proposal itself (which could just be used for the provider object) but with the way the several browser extensions are injecting it directly into the website and encouraging developers to directly consume it. If you decide to follow my proposal it may be interesting to just add some wording to the proposal advising against this.

@corollari

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Also, it may be interesting to add some method to get the current block but this is not important.

@igormcoelho

This comment has been minimized.

Copy link

commented Aug 6, 2019

Thanks for the responses @nickfujita.. my intent was trying to maje it better,although I know its a challenge to change things that many providers are already using... others are directly connected to neo rpc standard,which is not perfect too.
On general, a smart dev can use any.sort of api, as long as things are explained.

This looks like a good standard afterall, due to all these limitations, and perhaps we should just close all my comments, since this is a "nearly final" proposal (no need to change small things). Congratulations.

@nickfujita

This comment has been minimized.

Copy link
Author

commented Aug 7, 2019

@corollari

Great point! I am also very much in agreement with this. Rather than injecting the latest version of the method interface onto the window of the dApp website, providers should be providing properly versioned packages.

In the case of O3, we currently utilize NPM and CDN (JSDeliver) to ensure that dApps can protect themselves from sudden changes in the interface. In this way, dApps can lock down their NPN dependencies, or reference a specific version of the CDN package to pull into their website.

NPM

CDN

Although this is how O3 choose to implement this, since the proposal is still just focused on the interface of the methods themselves, it is not currently a requirement.

Do others think that we should add this as a requirement to the proposal, or should it just be left as a general "best practice" for dApps and websites in general for importing JS dependencies?

@canesin

This comment has been minimized.

Copy link

commented Aug 7, 2019

I think that @corollari comment is on point. About an year ago I suggested that the provider should inject versioned libraries but if instead we can have a versioned provider object (as capabilities of the provider can change in time also) that the API library can consume instead is much better. Nash would be willing to support the standard if we adopt that model and ship a new version of neon-js that accept the provider object instead of current system.

@nickfujita

This comment has been minimized.

Copy link
Author

commented Aug 9, 2019

@corollari Added getBlockHeight

@nickfujita

This comment has been minimized.

Copy link
Author

commented Aug 9, 2019

@igormcoelho Thanks again for all the input. It was very helpful, and a bunch of improvements were made to the proposal per your feedback ^^

@nickfujita

This comment has been minimized.

Copy link
Author

commented Aug 9, 2019

@canesin We are all excited for the launch of your platform this month, and are glad you have circled back to this proposal. I've added to the proposal a spec which briefly describes how providers of client packages should be versioning and supporting this interface.

As for the integration into neon-js, we would be happy to provide any additional documentation required for the integration team.

@corollari

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

@nickfujita Looks good, sent a PR fixing several typos and grammar problems (all trivial changes).

Merge pull request #2 from corollari/patch-5
Fix grammar & vocab
@nickfujita

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

@corollari Thanks! Merged in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.