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

BUG: asset or metadata fields need to be stringified (no float) #88

Open
diminator opened this issue Aug 16, 2017 · 9 comments
Open

BUG: asset or metadata fields need to be stringified (no float) #88

diminator opened this issue Aug 16, 2017 · 9 comments
Assignees
Labels

Comments

@diminator
Copy link
Contributor

Using float values might lead to InvalidHash errors on the server

ie using the following metadata:

{ time: 1502839313139, value: 0.26841099995038165 }   
@TimDaub
Copy link
Contributor

TimDaub commented Aug 24, 2017

Mhh, I wonder if the stringification of an asset's float should be the user's responsibility.
I see a few options here:

  1. stringify in the client (then there is also no InvalidHash error)
  2. error in the client as soon as as a float value in asset or meta data is detected
  3. just document the behavior but don't change the code

/cc @vrde @r-marques

@michielmulders
Copy link
Contributor

What's the status on this one? I think it's not the responsibility of the js-driver. I choose for option 3?

@TimDaub
Copy link
Contributor

TimDaub commented Sep 26, 2017

It's not the responsibility of the driver, except for maybe its documentation.

@ttmc Where do you think this information should be documented?

@ttmc
Copy link
Contributor

ttmc commented Sep 27, 2017

Is the general rule:

RULE: In the JSON contents of "asset" and "metadata", each value must be either a string or another JSON object (not an integer, float or other type).

If so, then this is something that can be checked by BigchainDB Server as one of the transaction validity conditions. Drivers and other tools can do whatever they like, but if they want to be useful, then they shouldn't build transactions with non-strings in the leaf values of "asset" or "metadata".

If we implement this check in BigchainDB Server, and we probably should, then it should be documented in the BigchainDB Server docs about "asset" and "metadata".

I will create an issue for this on the BigchainDB Server repo.

@diminator
Copy link
Contributor Author

If so, then please make sure that the error returned is appropriately labeled.

This wasnt the case before

@future-is-present
Copy link
Contributor

From https://jsfiddle.net/mdq41f16/ I still get the same error InvalidHash:

{
  "message": "Invalid transaction (InvalidHash): The transaction's id '19103d69d9983672782e7be437ec28e2e43db37be296bf029d8b78be9b693155' isn't equal to the hash of its body, i.e. it's not valid.", 
  "status": 400
}

Are we planning to change this error?

@ttmc
Copy link
Contributor

ttmc commented Jan 4, 2018

The current thinking about how to handle floats in asset.data and metadata is summarized in the issue bigchaindb/bigchaindb#1900

It will mean changing the transaction model, and there are many proposed changes to the transaction model. We probably won't be making any of those transaction model changes, except for the ones required by Tendermint integration, until after Tendermint integration.

@agwego
Copy link

agwego commented Jan 4, 2018

There are some subtle issues around floats, for instance I'm working on the java-bigchaindb-driver and the scale of BigDecimal isn't being respected. For instance sending a value 16.00 is being truncated to 16.0 as an example, this of course leads to an incorrect hash being generated (also hampered by the lack of debug logging in the server)

@ttmc
Copy link
Contributor

ttmc commented Jan 4, 2018

@agwego That's a great example of why floats are problematic. Big integers, Infinity and NaN have similar issues. That's why part of the proposal in bigchaindb/bigchaindb#1900 is to disallow JSON numbers completely in asset.data and metadata.

Until we change the transaction model (on the server side), you can enforce non-usage of JSON numbers in your application code or driver code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants