convert balance to number before saving #105

2018-12-03T05:31:19+01:00

neb-b commented

2018-12-03 05:31:19 +01:00

(Migrated from github.com)

Any place that checks the balance might be checking against a string currently, this makes sure it is always saved as a number.

akinwale (Migrated from github.com) reviewed 2018-12-03 05:31:19 +01:00

skhameneh (Migrated from github.com) approved these changes 2018-12-03 07:00:05 +01:00

skhameneh (Migrated from github.com) left a comment

Please see my note on floats before merging.
I would advise against any calculations in JS w/ floats.

Please see my note on floats before merging. I would advise against any calculations in JS w/ floats.

src/redux/actions/wallet.js

					
				@ -12,1 +12,3 @@

				    Lbry.account_balance().then((balance) => {

				    Lbry.account_balance().then((balanceAsString) => {

				      const balance = parseFloat(balanceAsString);

skhameneh (Migrated from github.com) commented

2018-12-03 06:59:22 +01:00

I'd use extreme caution with floats, these are not guaranteed to be decimal accurate.

neb-b (Migrated from github.com) reviewed 2018-12-03 07:03:56 +01:00

src/redux/actions/wallet.js

					
				@ -12,1 +12,3 @@

				    Lbry.account_balance().then((balance) => {

				    Lbry.account_balance().then((balanceAsString) => {

				      const balance = parseFloat(balanceAsString);

neb-b (Migrated from github.com) commented

2018-12-03 07:03:56 +01:00

Recommendations for another approach?

kauffj (Migrated from github.com) requested changes 2018-12-03 16:22:04 +01:00

kauffj (Migrated from github.com) left a comment

I'm going to strengthen @skhameneh's comment and say storing LBC values as a floating point number should never be done.

Everyone on @lbryio/appsquad needs to be unified on approach here (e.g. always storing deweys so we stay in int land). Please discuss on upcoming meeting or in chat.

I'm going to strengthen @skhameneh's comment and say storing LBC values as a floating point number should never be done. Everyone on @lbryio/appsquad needs to be unified on approach here (e.g. always storing deweys so we stay in int land). Please discuss on upcoming meeting or in chat.

neb-b commented

2018-12-03 21:45:39 +01:00

(Migrated from github.com)

@kauffj based on discussion with @skhameneh and @akinwale we plan to continue with this approach (for now), mainly because it is only reverting back to how the code originally worked when the daemon was returning floats pre v0.30 (which seemed to work fine).

We agreed that a better solution would be to convert to deweys and always use those in comparisons, but don't want to introduce a change that big right before two app releases.

@kauffj based on discussion with @skhameneh and @akinwale we plan to continue with this approach (for now), mainly because it is only reverting back to how the code originally worked when the daemon was returning floats pre v0.30 (which seemed to work fine). We agreed that a better solution would be to convert to deweys and always use those in comparisons, but don't want to introduce a change that big right before two app releases.

👍 1

skhameneh commented

2018-12-04 17:54:08 +01:00

(Migrated from github.com)

@kauffj, @seanyesmunt summarized our discussion well, we talked about filing an issue to resolve concerns while merging this in to move forward for the release. We are not doing any critical operations on the floats at this time and the daemon will further enforce constraints.

I am okay with having this merged in while we take more time to reach a better solution - rather than push invasive, potentially breaking, changes when we're prepping for a release.

@kauffj, @seanyesmunt summarized our discussion well, we talked about filing an issue to resolve concerns while merging this in to move forward for the release. We are not doing any critical operations on the floats at this time and the daemon will further enforce constraints. I am okay with having this merged in while we take more time to reach a better solution - rather than push invasive, potentially breaking, changes when we're prepping for a release.

👍 1

Sign in to join this conversation.

No reviewers

akinwale

skhameneh

kauffj