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

Double to string for #44 #45

Merged
merged 2 commits into from Feb 28, 2019
Merged

Conversation

ammaratef45
Copy link
Collaborator

PR for #44
Changed double to string when referring to balance.

@0crat 0crat added the scope label Feb 27, 2019
@0crat
Copy link
Collaborator

0crat commented Feb 27, 2019

Job #45 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented Feb 27, 2019

This pull request #45 is assigned to @g4s8/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @amihaiemil/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

Copy link

@g4s8 g4s8 left a comment

Choose a reason for hiding this comment

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

@ammaratef45 please see few comments

src/main/java/com/amihaiemil/zold/RtWallet.java Outdated Show resolved Hide resolved
@@ -113,7 +114,7 @@ public double balance() throws IOException{
@Override
public void pay(
final String keygap, final String user,
final double amount, final String details
final String amount, final String details
Copy link

Choose a reason for hiding this comment

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

@ammaratef45 this change will affect JSON payload format, type amount changed from JSON-number to JSON-string:
before:

{
  "amount": 1234.56
}

after:

{
  "amount": "1234.56"
}

I'm not sure here (because I didn't find API docs for JSON payload), but most probably API won't accept such format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@g4s8 reverting it makes the issue meaningless, what do you think? we may need to rediscuss the issue, right?

Copy link

Choose a reason for hiding this comment

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

@ammaratef45 you can use json.add("amount", new BigDecimal(amount)) - that code adds amount as json number, but with arbitrary precision

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@g4s8 The problem is that this change meant to be used for avoiding any casting or precision problems and let the higher layers handle this.

Copy link

Choose a reason for hiding this comment

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

@ammaratef45 but BigDecimal doesn't limit the precision, so I think it's safe to use here. In any case we should conform to wts API protocol, so if you have any doubts you can submit a ticket to wts repo, e.g. to expand documentation for JSON apis.
// cc @amihaiemil

Copy link
Contributor

Choose a reason for hiding this comment

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

@ammaratef45 , @g4s8 , @amihaiemil , the following conversion is performed in wts.rb here:

  amount = Zold::Amount.new(zld: params[:amount].to_f)

I am not a Ruby expert but my guess, which is based on the documentation:

"123.45e1".to_f        #=> 1234.5
"45.67 degrees".to_f   #=> 45.67
"thx1138".to_f         #=> 0.0

is that

"amount": "1234.56"

would be interpretted as 1234.56 (Float).

Hope, that would help us out here. By the way, there has been opened a related to accurate decimal operations issue #718 in Zold project.

src/main/java/com/amihaiemil/zold/Wallet.java Outdated Show resolved Hide resolved
@ammaratef45
Copy link
Collaborator Author

@g4s8 I've merged the master and the irrelevant changes went away
Also, according to @denis-konovalyenko comment, looks like the string payload of amount won't make a problem (or we need to wait for zold-io/zold#718 )
What do you think?

@g4s8
Copy link

g4s8 commented Feb 28, 2019

@ammaratef45 thanks

@g4s8
Copy link

g4s8 commented Feb 28, 2019

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 28, 2019

@rultor merge

@g4s8 Thanks for your request. @amihaiemil Please confirm this.

@amihaiemil
Copy link
Owner

@rultor merge it

@rultor
Copy link
Collaborator

rultor commented Feb 28, 2019

@rultor merge it

@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit ac2c37a into amihaiemil:master Feb 28, 2019
@rultor
Copy link
Collaborator

rultor commented Feb 28, 2019

@rultor merge it

@amihaiemil Done! FYI, the full log is here (took me 2min)

@ammaratef45 ammaratef45 deleted the double_to_string branch February 28, 2019 08:40
@0crat
Copy link
Collaborator

0crat commented Feb 28, 2019

Job was finished in 24 hours, bonus for fast delivery is possible (see §36)

@0crat 0crat removed the scope label Feb 28, 2019
@0crat
Copy link
Collaborator

0crat commented Feb 28, 2019

The job #45 is now out of scope

@0crat
Copy link
Collaborator

0crat commented Feb 28, 2019

Order was finished: +20 point(s) just awarded to @g4s8/z

@0crat
Copy link
Collaborator

0crat commented Feb 28, 2019

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @amihaiemil/z

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.

None yet

6 participants