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

[WIP] API for dApps (dAPI) #8

Open
wants to merge 60 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@backslash47
Copy link
Contributor

commented Aug 6, 2018

No description provided.

@nickfujita
Copy link

left a comment

Amazing work on the proposal. Left my feedback and requests for clarification. Excited to get this finalized :)

function getBalance(address: string): Promise<Balance>
</pre>

For further explanation about the wrapped method consult https://ontio.github.io/documentation/restful_api_en.html . The types '''Transaction''', '''Block''', '''MerkleProof''' and '''Balance''' corresponds to the exact object returned from Ontology blockchain.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

Could we please migrate the exact definitions into this document? I think it will benefit from being as detailed as possible, providing sample input and output values, with explanations.

For example, getGenerateBlockTime, I tried looking up an equivalent function in the rest api docs, but there doesn't seem to be an exact one. My best guess is that this is the timestamp of the latest block?

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 22, 2018

Author Contributor

I agree. I will note it to add it later.

A network API consists of:

<pre>
type Network = 'MAIN' | 'TEST' | 'PRIVATE';

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

How do you envision the Network type PRIVATE will work? Since there can be many different private networks, is there some config that needs to be set before hand to ensure that the PRIVATE type is handled correctly? What is this is not sent beforehand?

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 22, 2018

Author Contributor

For now, the getNetwork() method will provide only the information that wallet is not connected to MAIN-NET or TEST-NET. I might add method getNetworkAddress() which will return the specific url of the network (in case of main/test net the corresponding address of the network = used Node address to be precise)

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 23, 2018

Author Contributor

I refactored it a little different. The getNetwork now returns an object with type and address property.

type Network = 'MAIN' | 'TEST' | 'PRIVATE';
type Asset = 'ONT' | 'ONG';

function getGenerateBlockTime(): Promise<number | null>

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

Some of these functions appear to be beyond the scope of what will be required for a dApp. For example:

  • getNodeCount
  • getMerkleProof

Should the dApp be concerned with this level of detail, or should this only be the concern of the dAPI provider?

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 22, 2018

Author Contributor

Those method are only a wrapper around calls to the Node. No special logic behind them and in case of merke proofs, those can be handy in future when more functionality around verifiable claims are added into the api.

I will keep them as I want to provide as much from underlying RPC calls as possible.


====getName====
<pre>
function getName(): Promise<string>

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

Can we consolidate isInstalled, getName, and getVersion to be a single function called getProvvider?

It can have a method signature similar to the following:

interface Provider {
  name: string;
  version: string;
}

function getProvider(): Promise<Provider>

In the case that there is no provider will will Rejects with NO_PROVIDER in case there is no dAPI provider installed.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 22, 2018

Author Contributor

Good idea. Will do that.


====getOwnAccounts====
<pre>
function getOwnAccounts(): Promise<string[]>

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

Is this something that we want to provide dApps with the power to retrieve from a user? It allows a dApp to essentially crawl a users entire wallet for all the addresses they hold.

Would it not be safer to only have getAccount, which will return a single account? This can be selected by the user in the dAPI provider interface. Can be a default address if the dAPI provider allows such functionality, or prompt the user with a request to provide a selected address to use with the dApp?

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 22, 2018

Author Contributor

Very good point indeed. I will think about it. A function to request the user to select (UI) an address to use is also a good idea.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 26, 2018

Author Contributor

I removed all the getting of user accounts and identities. And I stated in the OEP, that it's up to dAPI provider to allow the user the selection of the account/identity. I also added the Caller mechanism for every call, so dAPI provider can implement blacklisting or custom access logic

====addAttributes====
<pre>
function addAttributes(identity: string, attributes: OntIdAttribute[]): Promise<void>

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

What is the expected behavior if the the attribute already exists? Is it overridden?

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 22, 2018

Author Contributor

It depends on the Ontology ONT ID smart contract implementation. I can find out and put the information into this OEP for clarity.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 23, 2018

Author Contributor

The attributes are overwritten according to SC implementation.


====signMessage====
<pre>
function signMessage(address: string, message: string): Promise<string>

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

Should we remove the address argument, as the user should be the one deciding with which address to sign the message with?

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 22, 2018

Author Contributor

Will decide after #8 (comment)

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 26, 2018

Author Contributor

I removed it.

* Rejects with '''MALFORMED_IDENTITY''' in case the <code>identity</code> is not a valid identity
===Message===
This API deals with arbitrary message signing and verification.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

I am assuming that Message refers to a raw transaction or possibly an arbitrary string, for things like encrypted messaging? Could not find this in the Ontology documentation (could you please point me in the right direction?)

Assuming this is for raw transactions, is this functionality really something that we want to provide dApps with the power to have a user potentially sign? Even if the Ontology message is included in the message string, it still seems risky for users.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 22, 2018

Author Contributor

Actually this is not about raw transaction. Signing raw transaction is very dangerous and should not be allowed at all.

This is about encrypted messaging. There is nothing in the Ontology documentation, but it is modelled after Ethereum message signing.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 23, 2018

Thanks for the clarification on this. After reviewing more documentation of related protocols for secure message signing techniques, I think we should remove this from OEP-6, and create a new OEP specifically relating to message signing. Reason being is that message signing is can expand beyond the scope of dAPI providers, and there may be others looking to implement message signing. Because of that, it would be good to have a standard set for the different types of message signing formats that will be present in the Ontology ecosystem.

At some later point, once the OEP for message signing is accepted, we can file a new OEP to add the message signing functions to the dAPI standards.

value: any;
}

function invoke(account: string, contract: string, method: string, parameters: Parameter[],

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

Should the responsibility of the gasPrice and gasLimit be left up to the dAPI provider to allow users to override defaults on their end? Should dApps even be concerned with the fees to execute network transactions?

Nice job on Parameters :)

Should account be address?

How will addresses be used? I understand that it will be a list of addresses or identities required to sign the transaction, but should this be allowed in a single transaction? What are some real world use cases for such a transaction within the same dAPI provider wallet?

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 22, 2018

Author Contributor

Account should be address.

The gaslimit and gasprice are tricky. Every instruction in SC call costs something. So only the dApp developer know how costly is the call. On the other hand we have the gasprice value which user can increase in times when the network is congested. So his transaction get processed sooner.

Therefore we should let user decide what unit price he is willing to pay, but let the dApp developer set the limit. But because he does not know what unit price the user will choose, it should be stated relative to the unit price.

And after all, user is gonna pay for the transaction, so he should have the option to override the limit. DApp developer should only tell: "this operation is costly, you should use this limit"

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 23, 2018

Author Contributor

About the account and addresses.

  1. I was wrong, the account should not be address. Address means account/identity. Invoking a SC needs to be signed by an account.

  2. addresses is as you said. A list of accounts/identities to sign the transaction with. There is a for example a use case when you want to prove that you own an identity during the SC invocation but you also need to pay for the transaction. In some cases you might need to prove you own 2, 3, ... identities/accounts.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 28, 2018

Author Contributor

The addresses was replaced by requireIdentity, which tells the dAPI provider, that the request must also be signed by the identity not only account.


====deploy====
<pre>
function deploy(account: string, code: string, name: string, version: string, author: string,

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

Shouldn't the gas price and limit be handled by the dAPI provider?


====removeEventListener====
<pre>
function addEventListener(listener: EventListener): void

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 22, 2018

Typo. Should be removeEventListener

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 22, 2018

Author Contributor

Yup. Typo.
I am still deciding about the notify events and how to return the data to dApp.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 23, 2018

Author Contributor

I decided to remove the listener functionality altogether and further explain the return value from invoke and invokeRead.


interface Provider {
name: string;
version: string;

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 23, 2018

Let's please also add a field called compatibility. This will be used to inform the dApp about the OEP standards that have been implemented by this version of the providers dAPI.

interface Provider {
    name: string;
    version: string;
    compatibility: string[];
}

Example:

{
    name: 'cool wallet',
    version: 'v0.1.2'
    compatability: [
        'OEP-6',
        'OEP-10',
        'OEP-29'
    ]
}

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 23, 2018

This will be useful in the case where we have, for example, OEPs 6,10, & 29 related to dAPI, but a provider only implements 6 & 29.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 23, 2018

Author Contributor

Agreeed.

====dAPI access restriction====
Using dAPI any dApp is able to call the dAPI provider and initiate an interaction with the user (e.g.: <code>makeTransfer</code>). Only prerequisite is, that the user visits the dApp page. Although the user will need to confirm such an action, bothering him with this action, if he has no intention to confirm it, will hinder the experience.

Therefore the dAPI will forward with every request the <code>Caller</code> object containing the <code>url</code> of the dApp page or <code>id</code> of another extension trying to communicate with dAPI provider. It is upto the dAPI provider to implement custom permission granting workflow or automatic blacklisting.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 28, 2018

Couple things on this approach:

  • The dApp should not be the one telling the provider what it's own URL is, but rather, the provider should be responsible for knowing the domain of the window from which the dApp is broadcasting from. The reason for this is that if there is a popular dApp that a lot of users are using, then any other dApp can just send that URL and spoof being that dApp.
  • What did you have in mind for id of another extension? I think this will suffer from the same vulnerability as the previous.

We also need to confirm if this connection will be made on a per address or per wallet basis. If the user chooses to connect with a dapp, based on it's url domain, should we continue to permit requests of a user changes their account on the provider side? If a user changes their address on the wallet side, should this expire the connection with the dapp, and the dapp will have to reconnect for this address?

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 28, 2018

If the connection is on a per account basis, it would be nice to return the address and private key for the account upon connection.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 28, 2018

Author Contributor

About the first point. This was a misunderstanding. Your point is right and it was never my intention that the dApp will supply the url. The url is retrieved from the communication channel and can not be changed by the dApp.

Second point: Sometimes the dApp itself won't be a web page, but rather another extension. In this case the ID will be the ID of this extension and can be used to distinguish the dApp. As in the previous point, the ID is not supplied from dApp, rather from the communication channel.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 28, 2018

will these be provided from the communication channel on the dApp side, or the provider side?

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 28, 2018

Author Contributor

the url should be provided by the channel, not by the dApp. It will depend on the specific channel implementation, but it should not be in the gesture of dApp to provide whatever values it wants. In the reference channel implementation, the background script of the dAPI provider will be the source of the information.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 28, 2018

Author Contributor

In the reference channel implementation and reference dAPI provider implementation the connection will be confirmed at the wallet level, because it won't make sense to bind it either to account or identity. Different dAPI provider can choose to invalidate the confirmation upon account/identity change. And different channel implementation can choose to implement it differently based on the requirements.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 31, 2018

i'm good with this.


====getDefaultAccount====
<pre>
function getDefaultAccount(): Promise<string>

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 28, 2018

Since we have now removed the ability to get all addresses for a wallet, can we rename this to just getAccount()?

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 28, 2018

Author Contributor

Was thinking about that. To be orthogonal with getIdentity, I need to make some changes to that part. Because in identity case, one can query also the blockchain for foreign identities and it won't make sense to have getIdentity(identity: string) and getAccount(), where the former one will query the blockchain for arbitrary identity and the second one will get only user account.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 28, 2018

Where is the getIdentity(identity: string) method? It is currently getDefaultIdentity(): Promise<string> right?

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 28, 2018

Author Contributor

Sorry for misunderstanding. Changed to getAccount and getIdentity.

* Rejects with '''NO_IDENTITY''' in case the user is not signed in or has no identity

====getPublicKeys====
<pre>

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 28, 2018

Is this method required, as I believe the the public keys are a part of the DDO returned by getDDO.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 28, 2018

Author Contributor

It's just that, those methods are separate on the ONT ID SC. But you are right, that DDO will also contain the public keys. In time, there might be many attributes (the other thing in the ddo) which will slow down the query, when you just want to get the public keys (e.g: message verification?)

This comment has been minimized.

Copy link
@nickfujita

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 28, 2018

Author Contributor

Yes you can, but there are also the methods for separate request attributes and public keys in the SC definition:
getAttributes, getDDO, getPublicKeys

But for now a simple non-redundant API for dApps will definitely be more helpful than trying to have all the methods here.

value: string;
}

function getAttributes(identity: string): Promise<OntIdAttribute[]>

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 28, 2018

Is this method required, as I believe the the attributes are a part of the DDO returned by getDDO.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 28, 2018

Author Contributor

Solved in #8 (comment)

Will keep only the DDO method

@nickfujita

This comment has been minimized.

Copy link

commented Aug 31, 2018

Recent round of changes look good. I think the remaining changes are:

  • Add more details to the network section methods
  • Remove message signing out of this OEP, start new OEP for messaging
function getAllowance({ asset: Asset, fromAddress: string, toAddress: string }): Promise<number>
function getBlock({ block: number | string }): Promise<Block>
function getTransaction({ txHash: string }): Promise<Transaction>
function getNetwork(): Network

This comment has been minimized.

Copy link
@nickfujita

nickfujita Aug 31, 2018

Should there also be a method for requesting that the wallet change networks? Lets say that the dApp has a setting to run on testnet. Would be good to allow the dApp to request that the provider use the testnet, as it would be terrible if transactions that were meant to be sent to testnet by the dApp, were instead sent to mainnet by the provider.

This comment has been minimized.

Copy link
@backslash47

backslash47 Aug 31, 2018

Author Contributor

I don't see much point in requesting dAPI provider to change network. dApp has information about used network. It could warn (or even forbid the action) the user that he is calling the sc/transfer on MainNet or instruct the user to change the network.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Sep 11, 2018

Then how should dApps coordinate using the test net over main net for their transactions? Are we assuming that this protocol will always be used with main net? Will users have to make sure themselves that their provider wallet and the dApp are both set to test net or both set to main net?

This comment has been minimized.

Copy link
@backslash47

backslash47 Sep 13, 2018

Author Contributor

I don't think a dApp should communicate something on MainNet and something on TestNet for single user account. A single dApp should communicate on one network only.

But it does not mean, it can work on both Nets. It just means, that the user account on TestNet and MainNet are two different accounts.

About coordinating the dApp and Provider net. I think the Provider is the one here, who will decide on which network are the dApp currently running. dApp should detect the Net from Provider, and act accordingly (there are more options):

  1. show information about the network and inform the user about running on specific Net (e.g: a yellow bar informing the user, that he is on testNet)

  2. don't allow user to do anything and inform him about necessity of switching network, because the dApp SC is deployed only to specific Net.

  3. don't care at all, and only distinguishing the user accounts between Nets.

@trueinsider

This comment has been minimized.

Copy link

commented Sep 5, 2018

I was thinking about seamlessness that I think is important for mass adoption of dApps. Let's take EOS Knights for example, it's a game on EOS blockchain where you can hire heroes, fight monsters, buy swords, etc.
If we think how to implement something similar then:

  1. Game would simulate some actions locally (buy sword, kill monster, etc)
  2. Game would periodically sync those actions (and their results) to blockchain e.g. by invoking contract operation sync with parameters ["buyBigSword", "attackThatGoblin"]

From user perspective constant transaction confirmations would ruin experience and he would just stop playing.
Maybe there could be some kind of permission model (like in Android/iOS/Facebook) where you allow dApp to do without confirmation only some actions on some resources so he can't steal user's funds but experience will be seamless?
Or maybe some decentralized registry of trusted dApps?
Of course this should be thought about very carefully as to not compromise security.

@backslash47

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

Hi,
thanks for the input.

I know what you mean. I am planing to implement automatic confirmation for SC calls in Cyano wallet. You will be able to check if SC calls to the same SC and method can be automatically confirmed. Although you can not do this if your key is on Ledger HW for obvious reasons.

But I think this is something, what is in the responsibility of the dAPI provider. There is no need to define it in standard. At least not in the dAPI OEP.

@trueinsider

This comment has been minimized.

Copy link

commented Sep 6, 2018

Hi,

I'm worrying that without standard everyone would end up implementing it differently or not implementing at all. Also I think we need best practices here, security-wise. Maybe not in this particular OEP.


Initiates a transfer of <code>amount asset</code> to <code>recipient</code> account.

The amount should always be an integer value. Therefore is is represented in the smallest, further non divisible units for the asset. E.g.: If the <code>asset</code> is ONG then the value is multiplied by 1000000000.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Sep 11, 2018

After reviewing this method, wanted to see if we could make 2 updates.

  • Update method name to send. It simply states that we are making an intent to send an asset.
  • Update the amount field to be a string value rather than an integer. This is a brief discussion that we had in the past, but after further consideration I think that using a string will be best for dApp developers. Inline with the responses from explorer.ont.on, I think that it would be best if we can abstract as much of the parsing logic as possible from dApp developers. As far as users and possibly even dApp developers are concerned, all amounts of ONT, ONG, and any future assets exist in their readable format. If we go with the format of requiring dApp developers to always perform the conversion from their input and display fields to and from it's readable decimal format from the fixed decimal integer format, this is unnecessary overhead, and creates more room for error on their part. For these reasons, I think that this method, along with getBalance should update their input and return values to be the parsed string representations of the values rather than the fixed decimal integer representations. (eg. For 0.0001 ONG, pass "0.0001" instead of 100000.

This comment has been minimized.

Copy link
@nickfujita

nickfujita Sep 11, 2018

Can we also update the input variable name from recipient to receiver?

This comment has been minimized.

Copy link
@backslash47

backslash47 Sep 13, 2018

Author Contributor

makeTransfer -> send , OK
integer -> string, will think about it more
recipient -> receiver, what about changing it to "to" ?

This comment has been minimized.

Copy link
@nickfujita

nickfujita Sep 13, 2018

recipient => to

+1


* Rejects with '''NO_ACCOUNT''' in case the user is not signed in or has no accounts
* Rejects with '''MALFORMED_ACCOUNT''' in case the <code>recipient</code> is not a valid account
* Rejects with '''CANCELED''' in case the user cancels the request

This comment has been minimized.

Copy link
@nickfujita

nickfujita Sep 11, 2018

Will we also need an error in the case that the transaction is rejected?

This comment has been minimized.

Copy link
@backslash47

backslash47 Sep 13, 2018

Author Contributor

It depends. I can add it here, but I don't want to go any further as to distinguishing "not enough amount", ... because it will reveal too much information. But a simple FAILED/REJECTED will do.

results: Result[];
}

function invoke({ contract: string, method: string, parameters?: Parameter[],

This comment has been minimized.

Copy link
@nickfujita

nickfujita Sep 11, 2018

Can we please update all references to a contract script hash from contract, to scriptHash to have a more specific description of this argument?

This comment has been minimized.

Copy link
@nickfujita

nickfujita Sep 11, 2018

Could we please also update the arguments:

  • method => operation
  • parameters => args

The reasoning for this is that in many of the smart contract examples provided both by the NEO community and by Ontology, the common nomenclature for these inputs are operation and args. So it makes sense that they would have a more direct mapping in interface of this protocol.

This comment has been minimized.

Copy link
@backslash47

backslash47 Sep 13, 2018

Author Contributor

I agree.

backslash47 added some commits Aug 3, 2018

backslash47 added some commits Aug 17, 2018

@backslash47 backslash47 force-pushed the backslash47:oep-dapp-api branch from 88a2bad to 43e4816 Oct 31, 2018

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