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

Current mymonero API #272

Closed
wants to merge 1 commit into from
Closed

Conversation

vtnerd
Copy link
Contributor

@vtnerd vtnerd commented Sep 11, 2018

@moneroexamples @paulshapiro

This is a description of the current API as implemented by mymonero and the current PR for the light wallet server. I would like to make some changes for the open source light wallet server, but that would be done in a separate PR for discussion.

@moneroexamples
Copy link

Hi.
Thanks for that. Just quick question. Is it the same API as here: https://github.com/mymonero/hosted-monero-api-spec ?

If yes, then which one is the main repo where the discussions should be held?

@vtnerd
Copy link
Contributor Author

vtnerd commented Sep 12, 2018

Yes, the documents are identical. Only the commit message differs. The discussion should take place here.

@moneroexamples
Copy link

First thing I would like to clarify are the units for xmr amounts. Some are unit64-string and some are uint32 (e.g. get_random_outs; maybe mistake?). I think having everything in unit64, just like monero's codebase is using it, would be more natural and you would not need to keep converting between different units of xmr amounts. This has already been discussed in monero-project/monero#2109, but not sure what was the outcome of it.

@paulshapiro
Copy link

paulshapiro commented Sep 15, 2018

So one problem I didn't see raised is that JSON can't represent uint64s afaik

I wouldn't say we'd have any problems with strings parsing to ints by locale on the client-side because we'd never use any digit group separator character .. i.e. these strings (edit: generally) get fed directly into JSBigInt.

@moneroexamples
Copy link

I see. Thanks for clarification. But then why amounts in get_random_outs would be sent in uint32?

@paulshapiro
Copy link

Heya @luigi1111 could we get this merged?

The other discussion about number formats is really more relevant for the communal standards doc that we hoped we could arrive at after mymonero and openmonero each commit their existing API version (so we have things documented)

@paulshapiro
Copy link

paulshapiro commented Sep 17, 2018

@moneroexamples I believe the linked docs are incorrect there. Our API server code reads those values in as an array of strings. Plus, amounts should technically be uint64s rather than 32s. If they were 32s then numbers should work.

@paulshapiro
Copy link

Actually sorry, @vtnerd can you rename this document "existing_mymonero_lightwallet_api" or something? Thanks..

@moneroexamples
Copy link

@paulshapiro

I will modify openmonero to use uint64-string for all xmr amounts, as in mymonero.

@vtnerd
Copy link
Contributor Author

vtnerd commented Sep 19, 2018

@moneroexamples I flipped the field types by accident in the document. mixin is a uint32 whereas the amount array is a uint64-string. This has been corrected with a force push (trying to keep commit log "clean").

@paulshapiro I'm don't quite understand the purpose. Are there going to be multiple versions of the document stored in this repo? The document can always mark things as proposals still in discussion and/or mark as not yet implemented or not going-to-implement in various projects, etc.

@paulshapiro
Copy link

paulshapiro commented Sep 19, 2018

yeah @vtnerd we currently have separate APIs, so the idea we had with openmonero was to commit our own versions, then we commit a version we agree to communally that we use going forward as what we have been calling the standard. Obviously your document doesn't document openmonero's API so we can't exactly call it the light wallet server.

@vtnerd
Copy link
Contributor Author

vtnerd commented Sep 19, 2018

But isn't this PR the perfect place for the communal discussion? Both documents already exist in the mymonero repo already for viewing, and @moneroexamples has already requested a change (and then decided to alter the openmonero version instead). Additional change requests before merge are welcomed, if there is a concern about flexibility on changes in the future. I could mark this document as "draft" it that helps any, to make it clear that future PRs are welcomed (in a reasonable time frame). For instance, I intended to propose the removal of some fields, and possibly propose a pagination scheme into this document after merge.

More importantly - I don't see why monero-project/meta would want to store some specific document for mymonero or openmonero when this could be stored elsewhere. Unless @luigi1111 thinks any extensions/alterations to the API should be stored here too? I'd likely create a sub-folder to contain the multiple documents if that was desired.

@paulshapiro
Copy link

But isn't this PR the perfect place for the communal discussion?

Depends on how you name the file that this PR is making.

More importantly - I don't see why monero-project/meta would want to store some specific document for mymonero or openmonero when this could be stored elsewhere.

After the mutually agreed upon standard doc which merges the APIs gets added and after we update our own APIs then there'd be no reason to keep around the old API documents on this repository.

I'm just doing two things: 1) relaying the original suggestion in the conversations we all had about how this would proceed and 2) just making a suggestion based on project management experience which you're completely free to ignore. We're already talking, yes, but it's nice to have a record of separate APIs that are actually in play as well as actual versions of documents we come up with.

Whichever, let's just agree on something :)

@moneroexamples
Copy link

@vtnerd @paulshapiro

I will change openmonero xmr values to uint64-string because now I know that this is better than using uint64 after discussing them with you guys. There are many other things to discuss regarding the api though. For example I think get_address_info and get_address_txs could be merged into one api call. And this could be potentially much greater change than changing uint64 to uint64-string. Other example is the usage of legacy payment_ids for importing wallet in mymonero. I already implemented integrated_addresses for that in openmonero, but integrated_addresses are deprecated anyway these days. Probably would need to start thinking about subaddresses and api around them.

@paulshapiro
Copy link

@moneroexamples that is exactly why I suggest we split this up into stages where we first normalize our APIs then enhance them by agreeing on enhancements.

@moneroexamples
Copy link

@paulshapiro

Yes, fully agree.

@vtnerd
Copy link
Contributor Author

vtnerd commented Sep 22, 2018

Renamed.

For example I think get_address_info and get_address_txs could be merged into one api call.

It might be worth keeping for bandwidth purposes. With some tweaks the client can use it to poll for changes and then call the more bandwidth expensive get_address_txs when a change occurs. The client can mostly do this already, but a reorg (timestamp changes) wouldn't necessarily be detected. Pushing behavior is more desirable anyway, so might be a consideration if HTTP-only scenarios are needed (behind a firewall/proxy, no socket availability).

Other example is the usage of legacy payment_ids for importing wallet in mymonero. I already implemented integrated_addresses for that in openmonero, but integrated_addresses are deprecated anyway these days. Probably would need to start thinking about subaddresses and api around them.

Have both payment id types been deprecated, or just the unencrypted type? I've been hesitant to implement subaddresses on the mymonero backend.

@vtnerd
Copy link
Contributor Author

vtnerd commented Sep 22, 2018

For example I think get_address_info and get_address_txs could be merged into one api call.

It might be worth keeping for bandwidth purposes. With some tweaks the client can use it to poll for changes and then call the more bandwidth expensive get_address_txs when a change occurs. The client can mostly do this already, but a reorg (timestamp changes) wouldn't necessarily be detected. Pushing behavior is more desirable anyway, so might be a consideration if HTTP-only scenarios are needed (behind a firewall/proxy, no socket availability).

The other technique would be to combine polling / pagination, which is likely the best since you can have one endpoint with significant bandwidth reduction. get_address_info wouldn't have much of a use case, if clients implemented the poll from last-block-seen via pagination technique.

@moneroexamples
Copy link

@vtnerd
Pagination seems as a good choice. My understating in this case is that the client would be keeping all the get_address_txs data between requests to the backend, and was just getting new tx data, instead of eveyrthing each time?

subaddresses are expected to replace payments IDs as indicated monero-project/monero#2056 and here monero-project/monero#3679 (comment)

This won't happen anytime soon, but I think slowly have to start thinking about supporting them. And this probably will be a major resource hog for the backend. For now, I just added ability to send sub-addresses in openmonero frontend, but adding them to backend is major change I haven't looked into yet.

@paulshapiro
Copy link

paulshapiro commented Sep 23, 2018

@vtnerd if we rename the file, then imo it makes sense for record keeping purposes to rename the PR

If we do so, we get to merge this PR w/o having to do all the work to agree on the standard yet. Luckily it's probably not a long discussion. But technically we'll need another PR for the actual agreed upon API doc (which may incur the same conversation) and then additional PRs for the discussions you're having about new features.

So for clarity for other people who want to follow these developments plus record keeping purposes you may like to consider having these discussions on new PRs which indicate that they're about (a) normalizing the API and (b) new features on top of the normalized API instead of on the PR for the initial commit of the mymonero document.

@vtnerd vtnerd changed the title Adding light wallet server rest api v1 Current mymonero API Sep 24, 2018
@vtnerd
Copy link
Contributor Author

vtnerd commented Sep 25, 2018

Pagination seems as a good choice. My understating in this case is that the client would be keeping all the get_address_txs data between requests to the backend, and was just getting new tx data, instead of eveyrthing each time?

Yes, the client would send a height/hash pair to the server. The server can then filter below that height and report an error if that block reorg'ed off the primary chain. Somewhat similar to monerod.

@moneroexamples
Copy link

I don't know if this is the right place to ask, but are there any changes in API planned with regard to booletproofs? Also, is it known when mymonero.com will support making booletproof txs?

@paulshapiro
Copy link

paulshapiro commented Oct 6, 2018

I happened to see it so I guess I can answer here. I would suggest we could open an issue for it on this repo instead or even add a note to the future standards document about it.

We have a few things to clear off before jumping to bulletproofs support. I don't think any changes are necessary to the APIs for this but am not 100% sure yet. An earlier version of bulletproofs is ready on the client-side, but I need to update the C++ behind it given recent changes and then run the rebuild script on the JS lib so that the web wallet can use it. Once the fork rolls around, I can update the hardcoded fork version in the C++. (hard-coded since the server is not supplying it - normally the daemon does)

@paulshapiro
Copy link

By the way I'm not super clear on where we landed on this PR, but I'm pretty sure we can merge this so we can move on to PRing the shared API format. MyMonero is looking forward to PRing improvement proposals we've been cooking up!

@vtnerd
Copy link
Contributor Author

vtnerd commented Oct 7, 2018

Nothing needs to be changed for bulletproofs. The broadcast tx API takes the entire transaction as hex-ascii bytes - so the client needs to support computing the bulletproof values and encoding it in the monero specific format.

@moneroexamples
Copy link

@paulshapiro @vtnerd
Yep, i think it can be merged. For now I don't have further comments or questions.

Copy link

@stoffu stoffu left a comment

Choose a reason for hiding this comment

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

Minor comments regarding tx_pub_key

| tx_hash | binary | Bytes of tx hash |
| tx_prefix_hash | binary | Bytes of tx prefix hash |
| public_key | binary | Output public key |
| tx_pub_key | binary | Ephemeral ECDH key |
Copy link

Choose a reason for hiding this comment

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

Isn't this perhaps meant to be "Transaction public key"? I imagine an "ephemeral ECDH key" to be the output public key derived using the CryptoNote stealth addressing scheme P = Hs(a*R | i)*G + B.

|------------|---------------|--------------------------|
| amount | uint64-string | XMR possibly being spent |
| key_image | binary | Bytes of the key image |
| tx_pub_key | binary | Bytes of the tx public |
Copy link

Choose a reason for hiding this comment

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

The word "key" seems to be missing at the end?

@moneroexamples
Copy link

Quick question.
mymonero-app-js is using same api? Or this api is specific to only mymonero.com website?

@paulshapiro
Copy link

Same API (app-ios too)

@moneroexamples
Copy link

moneroexamples commented Oct 20, 2018

Was wondiring if it would be possible to extend mymonero api specs with actual examples of the requests and responses? For example, for some public stagenet account?


| Field | Type | Description |
|------------|-------------------|-----------------------------------------|
| per_kb_fee | uint64 | Estimated network fee |
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi. It seems that we'll want to deprecate this field in favor of a per_byte_fee, right?

If so, we've unfortunately already released a client using the existing field, which we don't want to break. So we'd like to do this in a backward compatible way.

Since ideally (IMO) we only want to send this old field to old clients, I would propose adding an optional field to this get_unspent_outs Request object for new clients to identify themselves. This could just be a feature flag, but I think it'd probably be better to just introduce an API version number here. In that case, the API exactly as documented here would constitute version 1, and a proposed version 2 would replace the Response field.

What are everyone's opinions about this?

Choose a reason for hiding this comment

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

I think it should be changed to per_byte_fee. Having second field, is just confusing and more code to maintain and data to transfer.

p.s. why per_kb_fee is not string type? Shouldn't all xmr amounts uint64_t be represented as strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi. It seems that we'll want to deprecate this field in favor of a per_byte_fee, right?

I recently proposed an update to the ZMQ call for fees that returns estimated_base_fee, size_scale, fee_mask, and hard_fork_version. The size scale returns 1024 when per-kb and 1 when per-byte. The fee mask is fixed for all versions, and seems unlikely to change anytime soon (ha!).

The per-byte is sufficient, but the client would have to know the fee mask locally or some fees will be slightly too low. Sending the hard_fork_version might be useful in future forks.

p.s. why per_kb_fee is not string type? Shouldn't all xmr amounts uint64_t be represented as strings?

I'm not certain why this is a uint64 field (before my time), but the per_kb_fee is unlikely to be larger than 2^53. I'm not opposed to changing it for consistency purposes if its somehow easier on someone.

Copy link
Contributor Author

@vtnerd vtnerd Oct 25, 2018

Choose a reason for hiding this comment

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

Since ideally (IMO) we only want to send this old field to old clients, I would propose adding an optional field to this get_unspent_outs Request object for new clients to identify themselves. This could just be a feature flag, but I think it'd probably be better to just introduce an API version number here. In that case, the API exactly as documented here would constitute version 1, and a proposed version 2 would replace the Response field.

The server could continue to send per_kb_fee for backwards compatibility. I can think of a reason to immediately remove this field.

EDIT: I meant newer clients would ignore this field, and older clients should ignore the newer fields - unless the existing clients are designed to error on unrecognized fields being returned.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would also work. I thought it'd be slightly better if the server didn't have to bother computing the old value for new clients, but that's obviously very minor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, just disregard the backwards-compatibility stuff. We've decided we don't want to support the old client after all (it has an unrelated issue when sending transactions). So, we can simply replace the field now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm mistaken, we also need to send the fee quantization mask to the client, right?

In wallet2, this mask is retrieved from the daemon (here) and applied (here via here) after multiplying by the final tx size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fee_quantization is a fixed value that has never changed (yet). When moo updated wallet2 RPC the field was now being sent to the wallet, so I followed with the ZMQ RPC update. The extra field is pretty simple at least. Anyway, you can do without for now if necessary.

@paulshapiro
Copy link

Wanted to share this as we get the process going for the lightwallet standard. Seems to be working pretty well for them..

https://github.com/WebAssembly/proposals

https://github.com/WebAssembly/design

@anonimal
Copy link
Contributor

This PR was sent to the wrong repo. MyMonero has nothing to do with the meta functionality of Monero Project. MyMonero is a private enterprise with a purported CEO @paulshapiro and hired guns who contribute to Monero code when it suits MyMonero's need.

This discussion should be in the Monero code repo or in the MyMonero portion of github, not here. Otherwise, this looks like a shift of the overton window toward accepting Monero Project privatization.

@paulshapiro
Copy link

This PR was sent to the wrong repo.

Well, @anonimal, aside from this PR being able to be marked as a "Do not merge" because its function (which has been fulfilled) was to facilitate establishing the details of standardizing the first common lightwallet API…

MyMonero is a private enterprise with a purported CEO @paulshapiro and hired guns who contribute to Monero code when it suits MyMonero's need.

Are you saying that OpenMonero doesn't exist, that wallet2 does not contain lightwallet functionality to this very API, and that there is not an apparently valid lightwallet server PR to the Monero project at the moment? The "privatization" you accuse us of consists of other people dumping a lot of money into open sourcing a bunch of software that we had virtually zero short or medium term incentive to open source. What is your deal, man?

@paulshapiro
Copy link

I mean if the issue is that you're getting spammed with email notifications then there are solutions for that.

@anonimal
Copy link
Contributor

Hey Shappy, try reading what people actually write and take more than a few minutes to think of a response. You'll find that the answers are already there.

@HIPERLENDARIO
Copy link

HIPERLENDARIO commented Nov 18, 2018

Hi, I'm trying to create a Miner of Monero with JavaScript and Ajax, and HTML5, I wanted to know if it's okay to use 32-bit variables either for the signed integer or for the unsigned integer, and how they will be sent by JSON to the Network. Not has problem right????

| address | base58-address | Address to retrieve |
| view_key | binary | View key bytes for authorization |

> If `address` is not authorized, the server must return a HTTP 405
Copy link

@serhack serhack Nov 18, 2018

Choose a reason for hiding this comment

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

The HTTP code for Forbidden error is 403. (405 - Method not allowed). Should we keep 405 or 403?

> error must be returned.

> If approval process is manual, a successful HTTP 200 OK and response object
> must be returned. Subsequent requests shall be HTTP 405 "Forbidden" until
Copy link

Choose a reason for hiding this comment

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

I think it could be more useful HTTP 403.

Details about HTTP 403 RFC 2616 :

The server understood the request, but is refusing to fulfill it. Authorization will not help and the request SHOULD NOT be repeated. If the request method was not HEAD and the server wishes to make public why the request has not been fulfilled, it SHOULD describe the reason for the refusal in the entity. If the server does not wish to make this information available to the client, the status code 404 (Not Found) can be used instead.

> If clients are creating multiple rings with the same amount, they must set
> `count` to the mixin level and add the value to `amounts` multiple times.
> Server must respond to each value in `amounts`, even if the value appears
> multiple times.gi
Copy link

Choose a reason for hiding this comment

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

Typo?

Copy link

Choose a reason for hiding this comment

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

Fixed in #287

Copy link

@jonathancross jonathancross left a comment

Choose a reason for hiding this comment

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

A few typos.

| HKD * | float32 | HKD/XMR exchange rate |
| INR * | float32 | INR/XMR exchange rate |
| JPY * | float32 | JPY/XMR exchange rate |
| KRW * | float32 | KRW/XMR exhcnage rate |

Choose a reason for hiding this comment

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

"exhcnage" => "exchange"

| tx_prefix_hash | binary | Bytes of tx prefix hash |
| public_key | binary | Output public key |
| tx_pub_key | binary | Ephemeral ECDH key |
| spend_key_images | array of binary's | Bytes of key images |

Choose a reason for hiding this comment

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

binary's would not be the plural form of binary
Instead, should just be array of binary values, array of binaries or array of binary types, etc.

| total_sent | uint64-string | XMR possibly being spent |
| unlock_time | uint64 | Tx unlock time field |
| height * | uint64 | Block height |
| spent_outputs | array of spend's | List of possible spends |

Choose a reason for hiding this comment

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

RE: "spend's": see comment above about "binary's".

| Field | Type | Description |
|------------|---------------------------|----------------------------------|
| count | uint32 | Mixin (name is historical) |
| amounts | array of uint64-strings's | XMR amounts that need mixing |

Choose a reason for hiding this comment

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

RE: "uint64-strings's": see comment above about "binary's".

@paulshapiro
Copy link

Can we close this PR or …? @vtnerd @ndorf

@vtnerd vtnerd closed this Jul 26, 2019
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

Successfully merging this pull request may close these issues.

9 participants