search

OpenBazaar / openbazaar-go

OpenBazaar 2.0 Server Daemon in Go
MIT License
994 stars 283 forks source link

ETH precision: openbazaar-go updates #1437

Open drwasho opened 5 years ago

drwasho commented 5 years ago

openbazaar-go updates

High-level overview

Protobuf Schema

proto CurrencyDefinition {
  string      code
  json.Number divisibility
}
proto CurrencyValue {
  CurrencyDefinition currency
  json.Number        value
}

*Note: I know there is redundant data with the currency type being expressed in every place where a value is represented. This was done intentionally so that knowledge of implicit relationships between fields aren't required to make sense of the data. Denormalizing data here also doesn't hurt us as we can ensure the implicit relationships are intact more easily with tests and validations.

API changes

A value of 0.12345678 BTC might be represented like this before:

{
    "data": "value",
    "amount": 12345678, // this key is inconsistent across calls
    "coin": "BTC", // this key is inconsistent across calls
    "otherdata": "othervalue"
}

This data would be changed to look like:

{
    "data": "value",
    "amount": { // this key would match the replaced key in the BEFORE case
        "value": "12345678",
        "currency": {
            "code": "BTC", // this key would always be `code`
            "divisibility": 8
        }
    },
    "otherdata": "othervalue"
}

Datastore migrations: SQLite data

[
  {
    "hash": "85499ec9b053772fc9ce95b695de1aa1fe83ea7daf44cda52ab27aac13a679d6",
    "value": 386091
  }
]
[
  {
    "Txid": "85499ec9b053772fc9ce95b695de1aa1fe83ea7daf44cda52ab27aac13a679d6",
    "Index": 0,
    "Value": 386091,
    "Address": "prv2kgx6m8ynestdmvhlndm237hhqgr4yce9q0a090",
    "Spent": true,
    "Timestamp": "2019-01-23T22:36:37.141428851-05:00"
  },
  {
    "Txid": "dde5a08c860faf7bd1b56d1528640fdbefd659ef798cfddb33e77b8b187fec4b",
    "Index": 0,
    "Value": -386091,
    "Address": "prv2kgx6m8ynestdmvhlndm237hhqgr4yce9q0a090",
    "Spent": true,
    "Timestamp": "2019-01-23T22:50:58.659452463-05:00"
  },
  {
    "Txid": "dde5a08c860faf7bd1b56d1528640fdbefd659ef798cfddb33e77b8b187fec4b",
    "Index": 0,
    "Value": -386091,
    "Address": "prv2kgx6m8ynestdmvhlndm237hhqgr4yce9q0a090",
    "Spent": false,
    "Timestamp": "2019-01-24T04:07:23.949652851Z"
  }
]
placer14 commented 5 years ago

Changed the spec to remove the magnitude key in favor of a simpler "one value" reprentation.

{
    "data": "value",
    "amount": { // this key would match the replaced key in the BEFORE case
        "value": "12345678",
      //  "magnitude": -8,    # omit this key
        "currency": {
            "code": "BTC", // this key would always be `code`
            "divisibility": 8
        }
    },
    "otherdata": "othervalue"
}
placer14 commented 5 years ago

@amangale Discussion has raised the following requirement:

NOTE: We need to ensure values lower than 0 in the value field when used in the JSON API are rejected with an error explaining that the value must be a positive integer.

Recommend a 400 Bad Request error response.

jjeffryes commented 5 years ago

@amangale you asked for some details of calls that use the amount field, here's what I turned up with a quick search that I know the desktop client uses. This may not include everything:

@amangale Just added three new endpoints ^^^ (rob)

rmisio commented 5 years ago

@amangale

Ashwan, just following up on my comment in the slack thread:

Keep in mind that a few of those endpoints include the full listing which does already have the coinDivisibility in it in the metadata object. It’s a required field when creating the listing.

So, for those api’s no change is needed.

I think these are the ones with the full listing:

rmisio commented 5 years ago

@amangale

Another thing is the metadata.coinDivisibility in the listing is in the old format (e.g. 100000000 instead of 8. Can you please make sure it uses the new format?

And I guess the server needs to be compatible with old listings using the old format.. or I guess maybe it just needs to migrate the data.

rmisio commented 5 years ago

@amangale @jjeffryes @mg

Another thing with that endpoint list is that a few of those endpoints aren't READs from the server. They are the client posting data back to the server.

For ob/purchase, the client would use the coinDiv from the listing which is referenced by a hash included in the POST data. I'm assuming the server when it receives the post could also get the coinDiv from the listing and use it to process any price fields?

For wallet/spend, should the client just use the coinDiv provided via wallet/balance or is it better for the client to explicitly send back the coinDiv it's using in the spend call?

For wallet/orderspend, should the client use the coinDiv from the listing the order is for or same question as above, should the client explicitly provide it in the orderspend call?

placer14 commented 5 years ago

@rmisio To respond to your old questions, I think posting the full definitions despite just getting them from a prior call makes the API more stateless. This means that a POST body might need to repeat the symbol and divisibility of the amount being sent. (The type and name can probably be omitted.) I think it's reasonable to accept just the string instead of the full definition, allow the server to assume the same divisibility as whats in the dictionary... but that would probably be a different key.