Support integer and decimal Nano amounts to raw conversion #3781

RickiNano · 2022-04-05T17:41:59Z

This adds support for decimal numbers when converting from Nano to raw. It is also backwards compatible with integer amounts both expressed like "35" or like "35.00000"
Decimals beyond 30 are cut to conform with the precision specifications
Invalid numbers will still show an error message
This pr fixes #3777

clemahieu · 2022-04-05T17:49:36Z

We should look at a real number parser rather than manually slicing the string up. https://www.boost.org/doc/libs/1_75_0/libs/spirit/doc/html/spirit/qi/reference/numeric/real.html

This also needs tests, specifically showing what it does to extreme values.

clemahieu · 2022-04-05T18:09:16Z

I like this strategy better than a separate RPC, we just need to make sure rounding errors can't happen.

RickiNano · 2022-04-06T12:07:28Z

I have added a bunch of testcases including all the edgecases I could think of. They run successfully with this implementation.
I'm hesitant to use the Boost float library. I don't have the required skills and I'm also worried if the library has the precision needed to store floating values from 0.00000000000000000000000000001 (one raw in Nano) and up to 13324829600000000000000000000000000000 (max supply as raw)
I feel more confident about handling the integer and integral parts as two separate numbers. This way the integer calculation is virtually unchanged compared to the current version, while only adding the integral part

nano2dev · 2022-08-29T18:30:39Z

@RickiNano Whats the status of this? Waiting on OK?

I need this more than ever and the bounty is still waiting to be claimed!

RickiNano · 2022-08-29T19:14:25Z

It's all up to @clemahieu if this gets approved. I've done a lot of testing on this and it's working correctly
I've also created the following testcases that runs successfully:
nano_to_raw_one_nano
nano_to_raw_zero_nano
nano_to_raw_one_tenth_nano
nano_to_raw_one_raw
nano_to_raw_value_less_than_one_raw
nano_to_raw_ten_million_nano
nano_to_raw_negative_value
nano_to_raw_invalid_input
nano_to_raw_zero_as_decimal
nano_to_raw_integer_as_decimal
If needed I can also code a tool that will do millions of random value requests to the old and the new api, and make sure that all integer numbers are handled the same to ensure 100% backward compatibility

nano2dev · 2022-08-29T19:58:38Z

Nice @RickiNano

@clemahieu Sorry for all the pings. Check this out when you get a chance.

Nano Node shipping 🚢 with Floating point conversion RPC makes for super fast prototyping w/ Nano!

dsiganos · 2022-08-30T10:53:37Z

I haven't looked at this closely but I agree with the idea that doing our own slicing of the string and parsing it ourselves is safer in short and medium term.
Floating number parsing is such a complicated task that if we left it to a library, it would be even harder to know exactly what the parsing rules were. Let's make our own very simple and very restrictive rules about what we parse.

dsiganos · 2022-08-30T10:57:36Z

nano/node/json_handler.cpp

+	nano::amount wholeNumber (0);
+	nano::amount decimalNumber (0);
+	nano::amount result (0);
+	if (!ec)


Let's make this a guard if and save a whole level of if nesting for better readability.

if (ec) return result;

dsiganos · 2022-08-30T10:58:39Z

nano/node/json_handler.cpp

@@ -283,6 +283,40 @@ nano::amount nano::json_handler::amount_impl ()
 	return result;
 }

+nano::amount nano::json_handler::decimal_amount_impl ()


Let's document the parsing rules. What are the parsing rules?
Let's make them very strict and very simple.

Should ".5" be legal value to parse?

Do we really want to parse negative numbers?

What is the largest number we support?
What is smallest number?
What happens when a value is too small or too large?

Do you want me to document the rules in the code as a comment or somewhere else?

The rules will need to be in the documentation to the RPC call. In the mean time, you could add them to the code as a comment.

What about whitespace? What are the rules, whitespace allowed?
I would lean on being strict on no whitespace allowed, for simplicity.

dsiganos · 2022-08-30T10:59:30Z

nano/node/json_handler.cpp

+	{
+		std::string amount_text (request.get<std::string> ("amount"));
+
+		int dotIndex = amount_text.find (".");


Let's make sure that there is either zero or one dots. And no other possibility.

dotIndex references the position of the dot. It's not the amount of dots. However if the input has multiple dots it will still output an error. Eg 5.3.6 will be converted into 5 as the wholenumber, but the remaining 3.6 can't be converted and the invalid_amount error is returned

Can we have a test case for it? We also need a test cases for:

empty string

negative zero

positive (with plus sign)

number too large

hex notation (should be rejected)

scientific notation (should be rejected)

use of comma character (should be rejected)

use of comma as fractional part separator (not all countries use dot as separator, should be rejected)

In general I would favour removing support for negative numbers and plus sign to reduce complexity.

nano/rpc_test/rpc.cpp

RickiNano · 2022-08-31T17:48:41Z

Hi @dsiganos
I have decided to use regex to validate the input. This makes validation strict and inputs are rejected early on. The required input format is either Digits only or if we have a dot then we require digits on both sides of the dot. All other inputs are rejected
The regex used is ^\d+(.\d+)?$
You can try it out with something like regex101.com if you want
I have also included all the testcases that you asked for and they run successfully
I've added documentation and the code should also be a lot more readable now. I have also addressed the points you've made in your review

dsiganos · 2022-09-01T08:12:38Z

I find the use of regex in this case over the top and a slight complication.
It would be extremely easy to perform the same check with pure C/C++.

This reverts commit c20ed43.

RickiNano · 2022-09-10T16:28:36Z

I have rolled back my attempt to convert the regex to c++ code
I lack the skills to do this myself so I hope someone can do this for me.

dsiganos · 2022-09-14T15:00:14Z

@RickiNano , @nano2dev you could write a specification for the allowed incoming string to be converted.
For example, what is the max number, what is the smallest number, etc.
so it can then be coded.

Changed maximum input to 340 million and changed unit tests to reflect this

RickiNano · 2022-09-14T17:47:01Z

@dsiganos
I had already written a summary for the method but I have updated it just now. It now says:

/// <summary>
/// Converts a Nano value with a maximum of 30 decimals into the corresponding raw value.
/// Accepted inputs are either digits only, or digits seperated by one dot. Any other format is rejected
/// Minimum value is 0 and maximum is 340 million
/// </summary>
/// <returns>raw value</returns>

I think this description is concise but still addresses all input validations
I have also updated the maximum amount to 340 million. Unit tests have also been updated to reflect this.
Btw, the current v24 implementation has no maximum amount and will return faulty values for inputs above 340 million caused by int128 overflow

nano2dev · 2022-09-14T18:22:40Z

Just commenting to say Thank You everyone involved.

I understand this PR may be controversial from an implementation standpoint.

My hope is that it gets shipped sooner than later - and it gets better with time.

I suspect, that I will be the only company to adopt this new feature immediately.

As most bring their own Big Num library.

My use case is Bash. Where no Big Num library exists.

On that note. This PR will make the Node much more "self-sufficient". Specially in Low level language ecosystems, where user input is Decimals.

nano2dev · 2022-09-15T03:55:37Z

@RickiNano Check this out when you get a chance:

RickiNano#1

Thanks @marcusmmmz

…-for-nano-to-raw-conversion convert regex to c++ code

RickiNano · 2022-09-15T15:02:13Z

@dsiganos
I have tested and merged @marcusmmmz 's changes. The validation is now done in c++ instead of using regex

dsiganos · 2022-09-15T17:35:24Z

Guys, I still do not see parsing rules. Where are the rules? What's allowed, what is not?
The rules need to answer all the questions I had posted at an earlier message.
then we can check the code against the rules.

marcusmmmz · 2022-09-15T18:04:33Z

@dsiganos
RickiNano already wrote out the parsing rules over here:
#3781 (comment)

They're intentionally very simple:

Minimum amount is 0
Maximum amount is 340 million
Maximum of 30 decimals
And it can be either:

Just a number sequence
Number sequence + "." + Number sequence

That's all the rules, no leading dots, no negative/positive signs, no hex, no scientific notation.

dsiganos · 2022-09-15T18:16:06Z

How many characters are allowed in the whole number section?
What is the max string length? Can we check for that explicitly before doing any other processing?

dsiganos · 2022-09-15T18:29:49Z

Is this legal? the current rules do not say.
000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001

marcusmmmz · 2022-09-15T22:38:13Z

How many characters are allowed in the whole number section? What is the max string length? Can we check for that explicitly before doing any other processing?

As 340 million is the max value (9 characters), and the maximum of decimals is 30, plus the "." character, that equals 40 characters, so that's the max string length. So yes, we could easily check that before doing other processing.

Is this legal? the current rules do not say. 000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001

Currently that's not being checked, but it can easily be done without adding too much complexity so i think we should add that check. I'll be working on this.

Edit: RickiNano's next commit supersedes this.

RickiNano · 2022-09-16T13:14:39Z

@dsiganos
I've added these extra validations:

The integer part is tested for a maximum length of 9
Leading zeros are now being removed

The validation rules are:

/// 9 characters [0-9] allowed
/// Optionally followed by up to one '.' character and then 30 characters [0-9]
/// Max value is 340 million

I have also added a test case with leading zeros

nano2dev · 2022-09-23T21:17:07Z

@dsiganos Please check this when possible :)

No rush on getting it shipped.

I intend on paying bounties when you simply give the 👍🏽

nano2dev · 2023-01-14T15:01:42Z

@RickiNano Do you mind providing nano address for reward. The 100 NANO prize is split 50/50 with @marcusmmmz .

Hope this PR makes in when adequate.

marcusmmmz · 2023-01-14T15:12:51Z

@dsiganos Have you reviewed what's left to be reviewed yet? it's been quite some time and we'd like at least some instructions on what to do next.

dsiganos · 2023-01-14T15:48:59Z

This change is controversial, Colin for example doesn't want it (because he regards it to be beyond the scope of nano node), but he will reluctantly accept it, if it is demonstrated that it is safe.

To finish this PR we need:

better specification of the parsing rules
better C++ coding
to demonstrate that it is safe for the node and for the programmer using the feature
documented well

In article the below, I say:
"During the process of contributing, a relationship, trust and a common understanding is being built between the core team and the contributor. This takes time but it is the most important element of the process. Many people forget this and do not give it the care and attention it deserves."
https://nano.org/en/blog/how-to-make-c-contributions-to-nano-node-development--cfc11ce8
A common understanding never got established here. This (and it being a low priority) are the primary reasons this PR did not progress.

I am currently trying to clean up existing PRs. Let's revisit this on Monday.
I will work on this PR Monday and get it over the line.

dsiganos · 2023-01-18T20:25:52Z

Apparently this was closed by mistake so I am reopening.

EmirLogas · 2024-09-14T20:58:41Z

Is there any progress on this issue? The answer should be 0.5 nano, but it is 0.

Support integer and decimal Nano amounts to raw conversion

0be4675

zhyatt added the rpc Changes related to Remote Procedure Calls label Apr 5, 2022

RickiNano added 2 commits April 6, 2022 13:47

Added testcases

683e23c

Fixed clang error

74c938b

dsiganos reviewed Aug 30, 2022

View reviewed changes

nano/rpc_test/rpc.cpp Show resolved Hide resolved

RickiNano added 5 commits August 30, 2022 22:49

More input checks and improved readability

02e4a66

Added more testcases for nano_to_raw

44e5bf2

Fix clang

98f2b5b

Use regex to validate input

24b2412

Fix clang

a8f6c71

RickiNano added 2 commits August 31, 2022 20:52

Merge branch 'develop' into pr/3781

a02c608

Fix unit tests

45affd6

RickiNano added 2 commits September 1, 2022 17:00

changed regex to c++

c20ed43

Revert "changed regex to c++"

f3dc91c

This reverts commit c20ed43.

Updated summary

adc9bb2

Changed maximum input to 340 million and changed unit tests to reflect this

marcusmmmz added 2 commits September 15, 2022 00:45

convert regex to c++ code

9ad5fcb

fix indentation

d15bdb0

RickiNano added 2 commits September 15, 2022 06:54

Merge pull request #1 from marcusmmmz/feature/support-decimal-numbers…

c8512f2

…-for-nano-to-raw-conversion convert regex to c++ code

Fixed clang

21debf2

Remove leading zeros. Limit string length

16cf944

Merge branch 'develop' into pr/3781

ff40dec

RickiNano closed this by deleting the head repository Jan 18, 2023

dsiganos reopened this Jan 18, 2023

Support integer and decimal Nano amounts to raw conversion #3781

Are you sure you want to change the base?

Support integer and decimal Nano amounts to raw conversion #3781

Conversation

RickiNano commented Apr 5, 2022

clemahieu commented Apr 5, 2022

clemahieu commented Apr 5, 2022

RickiNano commented Apr 6, 2022 • edited Loading

nano2dev commented Aug 29, 2022 • edited Loading

RickiNano commented Aug 29, 2022

nano2dev commented Aug 29, 2022

dsiganos commented Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsiganos Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

RickiNano commented Aug 31, 2022

dsiganos commented Sep 1, 2022

RickiNano commented Sep 10, 2022

dsiganos commented Sep 14, 2022

RickiNano commented Sep 14, 2022 • edited Loading

nano2dev commented Sep 14, 2022 • edited Loading

nano2dev commented Sep 15, 2022

RickiNano commented Sep 15, 2022

dsiganos commented Sep 15, 2022 • edited Loading

marcusmmmz commented Sep 15, 2022

dsiganos commented Sep 15, 2022

dsiganos commented Sep 15, 2022

marcusmmmz commented Sep 15, 2022 • edited Loading

RickiNano commented Sep 16, 2022

nano2dev commented Sep 23, 2022

nano2dev commented Jan 14, 2023

marcusmmmz commented Jan 14, 2023

dsiganos commented Jan 14, 2023

dsiganos commented Jan 18, 2023

EmirLogas commented Sep 14, 2024 • edited Loading

RickiNano commented Apr 6, 2022 •

edited

Loading

nano2dev commented Aug 29, 2022 •

edited

Loading

dsiganos commented Aug 30, 2022 •

edited

Loading

dsiganos Aug 30, 2022 •

edited

Loading

RickiNano commented Sep 14, 2022 •

edited

Loading

nano2dev commented Sep 14, 2022 •

edited

Loading

dsiganos commented Sep 15, 2022 •

edited

Loading

marcusmmmz commented Sep 15, 2022 •

edited

Loading

EmirLogas commented Sep 14, 2024 •

edited

Loading