Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Maximum integer part and scale #75

Closed
nesquikm opened this issue Mar 7, 2023 · 17 comments
Closed

Maximum integer part and scale #75

nesquikm opened this issue Mar 7, 2023 · 17 comments

Comments

@nesquikm
Copy link

nesquikm commented Mar 7, 2023

I couldn't find any mention of any restrictions, but as far as I can see the integer and fractional parts are represented by the BigInt. However, the simple test below breaks when maxScale>18 or maxInts>18:

import 'package:test/test.dart';
import 'package:money2/money2.dart';

void main() {
  const maxScale = 18;
  const maxInts = 18;

  setUp(() {
    for (var scale = 0; scale <= maxScale; scale++) {
      final c = Currency.create('C$scale', scale, symbol: '=$scale=');
      Currencies().register(c);
    }
  });

  test('scale 0-$maxScale test', () {
    for (var scale = 0; scale <= maxScale; scale++) {
      final c = Currencies().find('C$scale');
      expect(c, isNotNull);
      final str = scale == 0 ? '1' : '1.${'0' * (scale - 1)}1';
      final fmt = scale == 0 ? 'S#' : 'S#.${'#' * scale}';
      expect(Money.parseWithCurrency(str, c!).format(fmt), '=$scale=$str', reason: 'Failed with $scale scale');
    }
  });

  test('integers 0-$maxInts test', () {
    for (var ints = 0; ints <= maxInts; ints++) {
      final c = Currencies().find('C0');
      expect(c, isNotNull);
      final str = ints == 0 ? '0' : '9' * ints;
      final fmt = 'S#';
      expect(Money.parseWithCurrency(str, c!).format(fmt), '=0=$str', reason: 'Failed with $ints ints');
    }
  });

  test('scale 0-$maxScale and integers 0-$maxInts test', () {
    for (var scale = 0; scale <= maxScale; scale++) {
      for (var ints = 0; ints <= maxInts; ints++) {
        final c = Currencies().find('C$scale');
        expect(c, isNotNull);
        final intsStr = ints == 0 ? '0' : '9' * ints;
        final str = scale == 0 ? intsStr : '$intsStr.${'0' * (scale - 1)}1';
        final fmt = scale == 0 ? 'S#' : 'S#.${'#' * scale}';
        expect(Money.parseWithCurrency(str, c!).format(fmt), '=$scale=$str',
            reason: 'Failed with $scale scale, $ints ints');
      }
    }
  });
}
@bsutton
Copy link
Collaborator

bsutton commented Mar 7, 2023

So I'm uncertain what action your are looking to be taken?

@nesquikm
Copy link
Author

nesquikm commented Mar 7, 2023

There is no mention of border conditions in the documentation except AmountTooLargeException which is confusing because the library does not work correctly even at smaller scale and without num. First of all, I was looking for an answer to the question at what point does the library stopping working in my test, assuming that I missed something.

@nesquikm
Copy link
Author

nesquikm commented Mar 7, 2023

And it is not Fixed: problem, I just wrote similar test.

@bsutton
Copy link
Collaborator

bsutton commented Apr 16, 2023

@nesquikm so I ran your unit tests and they all pass?

@bsutton
Copy link
Collaborator

bsutton commented Apr 16, 2023

What is your test environment?
I'm running 64bit linux - ubuntu 22.04
dart 2.19.5

@nesquikm
Copy link
Author

nesquikm commented Apr 16, 2023

As I mention before, I offered Fixed.parse() as a fix for Money.parseWithCurrency() bug. You just added test for Fixed.parse(), so it should pass :)

@bsutton
Copy link
Collaborator

bsutton commented Apr 16, 2023 via email

@nesquikm
Copy link
Author

Yeah, there is test for money (also you can find it in the first comment), try to increase maxScale or maxInts to see the problem.

And this is my temporary fix.

@bsutton
Copy link
Collaborator

bsutton commented Apr 29, 2023

do you have a specific requirement?
Otherwise, I'm inclined to just document the limit.

@bsutton
Copy link
Collaborator

bsutton commented Feb 17, 2024

I've documented the limits here:
https://money2.onepub.dev/

and I've opened an enhancement request to increase the limits.

@bsutton bsutton closed this as completed Feb 17, 2024
@nesquikm
Copy link
Author

I just found that Money.fromJson and toJson have similar problems (they use toInt()). So I added some tests and updated my package that fixes all of these problems: https://pub.dev/packages/money2_fixer

For context you can run example:

Test with: integers: 3, decimals: 18
String: 999.999999999999999999
Pattern: 0.################## S
Parse and format:
  with Money.parse: 999.999999999999999999 SV
  with improved method: 999.999999999999999999 SV
To and from json:
 parsing, integer part: 999, decimal part: 999999999999999999
  to and from Money.json: integer part: 999  decimal part: 999999999999999999
  to and from json improved: integer part: 999  decimal part: 999999999999999999

Test with: integers: 3, decimals: 19
String: 999.9999999999999999999
Pattern: 0.################### S
Parse and format:
  with Money.parse: -842.8297329635842064385 SV
  with improved method: 999.9999999999999999999 SV
To and from json:
 parsing, integer part: 999, decimal part: 9999999999999999999
  to and from Money.json: integer part: 999  decimal part: 9223372036854775807
  to and from json improved: integer part: 999  decimal part: 9999999999999999999

Test with: integers: 19, decimals: 3
String: 9999999999999999999.999
Pattern: 0.### S
Parse and format:
  with Money.parse: 9223372036854775807.999 SV
  with improved method: 9999999999999999999.999 SV
To and from json:
 parsing, integer part: 9999999999999999999, decimal part: 999
  to and from Money.json: integer part: 9223372036854775807  decimal part: 999
  to and from json improved: integer part: 9999999999999999999  decimal part: 999

@bsutton
Copy link
Collaborator

bsutton commented Feb 14, 2025 via email

@nesquikm
Copy link
Author

You are of course right, I will try to make a PR as soon as I have time.
According to statistics, many users are already using money2_fixer, so I need to fix it as soon as possible.

@bsutton
Copy link
Collaborator

bsutton commented Feb 14, 2025

can I suggest that we merge the two projects and I add you as a contributor to money2 and then mark money2_fixer as deprecated.

I'm always happy to have help.

@nesquikm
Copy link
Author

Yes, of course. It would probably be nice to add tests from money2_fixer too.

@bsutton
Copy link
Collaborator

bsutton commented Feb 15, 2025 via email

@nesquikm
Copy link
Author

So, I created two PR:

onepub-dev/fixed#22
#91

But I think we should eliminate all the rest toInt()/fromInt(), and check math operations for big int support.

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

No branches or pull requests

2 participants