Manged decimal #172
Manged decimal #172
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -0,0 +1,41 @@ | |||
| from decimal import Decimal | |||
|
|
|||
There was a problem hiding this comment.
Does this affect us?
>>> from decimal import *
>>> getcontext().prec
28
Maybe test the case with more decimals?
There was a problem hiding this comment.
I'd say 28 decimals is plenty given the fact that we don't compute anything using ManagedDecimal.
There was a problem hiding this comment.
Generally speaking, most operations (formatting, scaleb etc.) are influenced by this setting (as far as I know).
A double / triple check (research) is worth around this.
There was a problem hiding this comment.
did some reading, added a local context with 256 decimals
|
|
||
| assert isinstance(second_input, ManagedDecimalValue) | ||
| assert second_input.is_variable | ||
| assert second_input.scale == 0 |
There was a problem hiding this comment.
Thus, scale == 0 means variable, correct?
multiversx_sdk/abi/abi.py
Outdated
| if scale == "usize": | ||
| scale = 0 | ||
| is_variable = True | ||
| else: | ||
| scale = int(scale) | ||
| is_variable = False | ||
|
|
||
| return ManagedDecimalValue(scale=scale, is_variable=is_variable) |
There was a problem hiding this comment.
Can be a bit more compact: if that return object, else return object.
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
There was a problem hiding this comment.
In the PR description, please also reference the PRs that brought the implementation in JS.
| def set_payload(self, value: Any): | ||
| if isinstance(value, ManagedDecimalSignedValue): | ||
| if self.is_variable != value.is_variable: | ||
| raise Exception("Cannot set payload! Both ManagedDecimalValues should be variable.") |
There was a problem hiding this comment.
ManagedDecimalValues can be managed decimal values.
|
|
||
| if self.is_variable: | ||
| # read biguint value length in bytes | ||
| big_int_size = self._unsigned_from_bytes(data[:U32_SIZE_IN_BYTES]) |
There was a problem hiding this comment.
Is value_length a better name?
| return value_str | ||
| if len(value_str) <= self.scale: | ||
| # If the value is smaller than the scale, prepend zeros | ||
| value_str = "0" * (self.scale - len(value_str) + 1) + value_str |
There was a problem hiding this comment.
Does this help?
There was a problem hiding this comment.
it did. refactored the method
| def _unsigned_from_bytes(self, data: bytes) -> int: | ||
| return int.from_bytes(data, byteorder="big", signed=False) | ||
|
|
||
| def _convert_value_to_int(self) -> int: |
There was a problem hiding this comment.
Does this help?
>>> from decimal import Decimal
>>> int(Decimal("123.456").scaleb(3))
123456
There was a problem hiding this comment.
it did. refactored this method, as well.
| @@ -0,0 +1,103 @@ | |||
| import io | |||
There was a problem hiding this comment.
Same comments as for ManagedDecimalSignedValue.
There was a problem hiding this comment.
applied the same changes here, as well.
| from multiversx_sdk.abi.managed_decimal_value import ManagedDecimalValue | ||
|
|
||
|
|
||
| class TestManagedDecimalValueTest: |
There was a problem hiding this comment.
Maybe have a single test file to test both signed and unsigned values?
Also, maybe have some unit tests around encoding and decoding, as well?
There was a problem hiding this comment.
now having the tests in the same file. did some encoding and decoding tests, as well.
|
|
||
| def get_precision(self) -> int: | ||
| return len(str(self._convert_value_to_int()).lstrip("0")) | ||
| value_str = f"{self.value:.{self.scale}f}" | ||
| return len(value_str.replace(".", "")) |
There was a problem hiding this comment.
Does this work always / when using other locales (i.e. USA, German etc.)?
There was a problem hiding this comment.
Well yes, because it's locale independent since we explicitly format the string using a dot and then replacing the dot.
| value_str = "0" * (self.scale - len(value_str) + 1) + value_str | ||
| return f"{value_str[:-self.scale]}.{value_str[-self.scale:]}" | ||
| scaled_value = self._convert_value_to_int() | ||
| return f"{scaled_value / (10 ** self.scale):.{self.scale}f}" |
There was a problem hiding this comment.
This makes the assumption . is the decimals separator. Please double check if all right in all cases.
There was a problem hiding this comment.
as decided, to_string() was removed.
| scaled_value = self._convert_value_to_int() | ||
| return f"{scaled_value / (10 ** self.scale):.{self.scale}f}" | ||
|
|
||
| def get_precision(self) -> int: | ||
| return len(str(self._convert_value_to_int()).lstrip("0")) | ||
| value_str = f"{self.value:.{self.scale}f}" |
There was a problem hiding this comment.
A double check is necessary here, as well.
There was a problem hiding this comment.
This should be alright, as well. It behaves like the JS implementation.
| @@ -0,0 +1,41 @@ | |||
| from decimal import Decimal | |||
|
|
|||
There was a problem hiding this comment.
Generally speaking, most operations (formatting, scaleb etc.) are influenced by this setting (as far as I know).
A double / triple check (research) is worth around this.
| else: | ||
| return ManagedDecimalValue(scale=int(scale), is_variable=False) | ||
|
|
||
|
|
There was a problem hiding this comment.
maybe follow the same pattern as for ManagedDecimal
The integration test requires
localnet. To set up a localnet, check out the documentation.The implementation was inspired by this PR.