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

Specification for standard lightwallet client/server API #287

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ndorf

ndorf commented Nov 10, 2018

As implemented by OpenMonero, MyMonero and the Monero Project.

@ndorf

Heya @moneroexamples, any thoughts on this proposed final change to the earlier document (edit: and its filename)? 1252e44

@moneroexamples

This comment has been minimized.

moneroexamples commented Nov 10, 2018

Are there updates regarding ringct conbase txs in rct field and making all xmr amounts string?

@ndorf ndorf force-pushed the ndorf:lightwallet_api branch 4 times, most recently from 957963c to e08a086 Nov 10, 2018

@ndorf

This comment has been minimized.

ndorf commented Nov 11, 2018

Sure thing. I also renamed the per_kb_field as discussed earlier, fixed the field descriptions as suggested by stoffu, and added a new field fee_mask to pass the fee quantization mask to the client.

The changes are smaller with whitespace ignored. You can also see them as individual commits on this otherwise-identical branch.

@moneroexamples

This comment has been minimized.

moneroexamples commented Nov 11, 2018

Thanks. But I was referring to this issue: mymonero/mymonero-app-js#240

Ringct coinbase txs require special treatment in the backend. The api should explain how coinbase txs should be treated. Otherwise issues as the one linked will still be raised.

Also what is fee_mask? I don't understand what "Fee quantization mask" means? What is wrong with current way of handling fee? Sorry, maybe it was explained in some thread somewhere and I missed that.

Also I don't understand why in the api dock says "should be used to locally select outputs using a triangular distribution". I think we already moved to gamma distribution (open monero still uses triangular one, but this is going to change, and mymonero is also working on that)?

@ndorf

This comment has been minimized.

ndorf commented Nov 12, 2018

mymonero/mymonero-app-js#240

Yes, the is_rct flag is meant to fix that issue, as suggested by luigi therein. When is_rct == true && rct.length == 0, the output is from a ringct coinbase tx and the unencrypted identity mask must be used. That should be all that's needed, unless I'm misunderstanding something?

Also what is fee_mask?

I asked about that a few days ago. Basically, the final fee should be rounded to be a multiple of this value when constructing the tx. Historically, it has been hardcoded and has never changed; however, with mooo's v8 changes, the mainline wallet has been updated to request it from the daemon. Since that means that, at least theoretically, it can now change at any time, it seems to make sense to support that for the lightwallet client as well. The value should be retrieved from monerod using the get_fee_quantization_mask RPC call. What do you think?

I think we already moved to gamma distribution

Good catch, I'll change this. Thanks.

@ndorf

This comment has been minimized.

ndorf commented Nov 12, 2018

Oops, I seem to have missed the other half of that discussion. Ignore the is_rct portion for a moment, please

@ndorf ndorf force-pushed the ndorf:lightwallet_api branch from e08a086 to 54d2f4c Nov 12, 2018

@ndorf

This comment has been minimized.

ndorf commented Nov 12, 2018

I've now replaced the is_rct field with a note stating stating that "Coinbase ringct transactions shall have the identity mask and a zero amount in [the rct field]," in commit ndorf@16aa96d and squashed into this PR.

I don't think anything else is required to correctly handle these txes, is it? Does the usage need to be clarified further?

edit: squashed without whitespace

@ndorf ndorf force-pushed the ndorf:lightwallet_api branch from 54d2f4c to 00e5976 Nov 12, 2018

@ndorf

This comment has been minimized.

ndorf commented Nov 12, 2018

Actually, it looks like the rct field description wasn't correct; it stated that it contained "decrypted" mask and amount values, but in reality both openmonero and mymonero appear to send the raw, encrypted values (except for coinbase txes).

So I replaced it with a new one (in commit 37689ce, and squashed) that reads as follows:

rct is, for ringct outputs, a 96-byte blob containing the concatenation of
the public commitment, then the ringct mask value, and finally the ringct
amount value. For coinbase transactions, the mask is always the identity mask
and the amount is zero; for other transactions, the mask and amount are the
respective raw encrypted values, which must be decrypted by the client using
the view secret key. For non-ringct outputs, this field is nil.

@moneroexamples

This comment has been minimized.

moneroexamples commented Nov 13, 2018

Thanks for explanations and changes.

For coinbase transactions should be For ringct coinbase transactions . Non-ringct coinbase txs would be treated normally as other non-ringct txs.

Thx for info about and links and quantization_mask. I see it is also in the official monero wallet. Missed that before.

For now I don't have any other comments. I'm sure something new will pop up as time progresses:-). Probably the only other comment I would have would be to have some actual examples of all these api queries, rct fields, quantization masks, error responses, etc. But this can be done later, or viewed through openmonero implementation.

Anyway, maybe @vtnerd or @paulshapiro want to comment more, especially about the rct field.

@paulshapiro

This comment has been minimized.

paulshapiro commented Nov 13, 2018

Yep, that looks good to me.

@ndorf ndorf force-pushed the ndorf:lightwallet_api branch from 00e5976 to e432cc6 Nov 13, 2018

@ndorf

This comment has been minimized.

ndorf commented Nov 13, 2018

Thanks for the feedback. Also, "outputs" makes more sense here than "transactions," right? We now have, in bc1d1bf, the following:

rct is, for ringct outputs, a 96-byte blob containing the concatenation of
the public commitment, then the ringct mask value, and finally the ringct
amount value. For ringct coinbase outputs, the mask is always the identity
mask and the amount is zero; for other ringct outputs, the mask and amount
are the respective raw encrypted values, which must be decrypted by the
client using the view secret key. For non-ringct outputs, this field is nil.

@moneroexamples

This comment has been minimized.

moneroexamples commented Nov 13, 2018

@ndorf

Sounds good to me.

| Field | Type | Description |
|--------------|-------------------|-----------------------------------------|
| per_byte_fee | uint64-string | Estimated network fee |
| fee_mask | uint64-string | Fee quantization mask |

This comment has been minimized.

@vtnerd

vtnerd Nov 13, 2018

This can probably be a uint64 field, because it is unlikely that this mask will exceed 2^53. This would be masking 1000 XMR or so.

| outputs | array of output's | Outputs possibly available for spending |
| Field | Type | Description |
|--------------|-------------------|-----------------------------------------|
| per_byte_fee | uint64-string | Estimated network fee |

This comment has been minimized.

@vtnerd

vtnerd Nov 13, 2018

This field is currently a uint64 for the same reason, this was not a typo.

This comment has been minimized.

@ndorf

ndorf Nov 13, 2018

Yeah, @moneroexamples requested all XMR amount fields to be uint64-string for consistency. It seemed reasonable enough, assuming the costs of parsing the string are negligible. Is that not the case?

> is returned as a single 96-byte binary blob.
> `rct` is, for ringct outputs, a 96-byte blob containing the concatenation of
> the public commitment, then the ringct mask value, and finally the ringct
> amount value. For ringct coinbase outputs, the mask is always the identity

This comment has been minimized.

@vtnerd

vtnerd Nov 13, 2018

This comment got lost I think ...

There are no "other ringct outputs", this object is strictly for spending an output. The amount field would have to be updated otherwise; ringct would always be zero for "other ringct outputs".

Also, the ringct values are always encrypted.

This comment has been minimized.

@ndorf

ndorf Nov 13, 2018

"Other ringct outputs" here means other than the "ringct coinbase outputs" mentioned earlier, i.e. non-coinbase ringct outputs. I guess I should just write exactly that, for clarity.

For the coinbase txes, the server is now to construct a special rct object consisting of the
unencrypted values rct::identity() for the mask and 0 for the amount. Previously, the client was unable to spend ringct coinbase outputs at all. This was the solution chosen therein, not least because the OpenMonero server already does it this way.

Does this make sense now, or do you think something (other than s/other/non-coinbase/) still needs to be changed?

> 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

This comment has been minimized.

@vtnerd

vtnerd Nov 13, 2018

Some weird typo I left (gi).

This comment has been minimized.

@ndorf

ndorf Nov 13, 2018

Found the vi user :)

@ndorf ndorf force-pushed the ndorf:lightwallet_api branch from e432cc6 to bb8dfb9 Nov 13, 2018

@ndorf

This comment has been minimized.

ndorf commented Nov 13, 2018

Updated with typo fix (1018025) and "non-coinbase" clarification (4a7ef32).

@serhack serhack referenced this pull request Nov 18, 2018

Open

Current mymonero API #272

| RUB * | float32 | RUB/XMR exchange rate |
| ZAR * | float32 | ZAR/XMR exchange rate |
> If an exchange rate is unavailable, the server field shall omit the field

This comment has been minimized.

@serhack

serhack Nov 18, 2018

I think it could be useful and helpful to mention from where Monero software will take the exchange rate.

This comment has been minimized.

@vtnerd

vtnerd Nov 19, 2018

Does it need to be from a consistent place for all implementations? This seemed restrictive.

This comment has been minimized.

@serhack

serhack Nov 19, 2018

I was trying to ask: is there any central source for retrieving prices? I saw you put cryptocompare API in your pull request on monero-project/monero.

This comment has been minimized.

@ndorf

ndorf Nov 19, 2018

I think the source of exchange rates has to remain up to the implementation. There doesn't exist a universally canonical source, and I don't see any reason to force implementations to use the same ones (or even provide them at all -- note, these fields are marked as optional.)

This comment has been minimized.

@serhack

serhack Nov 20, 2018

I agree with you.

Show resolved Hide resolved api/lightwallet_rest.md Outdated
Show resolved Hide resolved api/lightwallet_rest.md Outdated
Show resolved Hide resolved api/lightwallet_rest.md Outdated
| Field | Type | Description |
|---------------------|---------|------------------------------------|
| new_address | boolean | Whether account was just created |
| generated_locally * | boolean | Flag from initial account creation |

This comment has been minimized.

@serhack

serhack Nov 18, 2018

What does * mean in this context?

This comment has been minimized.

@vtnerd

vtnerd Nov 19, 2018

As described above, if used next to the field then the field is optional.

This comment has been minimized.

@serhack

serhack Nov 19, 2018

What is the purpose of having a response field as optional?

This comment has been minimized.

@vtnerd

vtnerd Nov 21, 2018

It is not required for the server to implement the feature. Not sure if this is the best approach, but that was the idea (this was added very recently to mymonero and openmonero has (had?) no such concept).

This comment has been minimized.

@moneroexamples

moneroexamples Nov 21, 2018

There is no generated_locally in openmonero. It might be added later, but at present there is no such plans.

@ndorf ndorf force-pushed the ndorf:lightwallet_api branch from bb8dfb9 to c139cb3 Nov 19, 2018

@ndorf

This comment has been minimized.

ndorf commented Nov 19, 2018

Replaced "Forbidden" code with the correct 403.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment