Add unit converter for transaction values about iota.py

[jinankjain]

@todofixthis I am looking forward to work on this bug.
I am thinking of making a separate function in transactions.py which will convert the value from string to int. Something like this:

def convert_value_to_standard_system_units(value):
  # type: string -> float
  """
  Return a floating point value in Standard System of Units from string
  'i'   :   1,
  'Ki'  :   1000,
  'Mi'  :   1000000,
  'Gi'  :   1000000000,
  'Ti'  :   1000000000000,
  'Pi'  :   1000000000000000
  """
  value_tuple = value.split()
  unit = 0
  if(value_tuple[1] == "i"):
    unit = 1
  elif(value_tuple[1] == "Ki"):
    unit = 1000
  elif(value_tuple[1] == "Mi"):
    unit = 1000000
  elif(value_tuple[1] == "Gi"):
    unit = 1000000000
  elif(value_tuple[1] == "Ti"):
    unit = 1000000000000
  elif(value_tuple[1] == "Pi"):
    unit = 1000000000000000
  
  return float(value_tuple[0])*unit

Is this the correct approach?

[todofixthis]

Hey @jinankjain. The approach you've laid out looks good. I think it could be improved with some validation (value must have correct type and format, must match a recognized value).

Looking forward to your pull request (:

[vynes]

Hi!

I've had a crack at it.
What return value is expected if validation fails? I've returned False.
I've also set the default unit prefix ('i') if none given. Not 100% sure if such implicit setting could cause problems?
Those three WARNING/ERROR print() statements are for testing purposes and should probably be replaced by logger methods, if such an approach will be used? (else just removed.)

def convert_value_to_standard_system_units(value):
    # type: string -> float
    """
    Return a floating point value in Standard System of Units from string
    """
    standard_units = {
        'i': 1,
        'Ki': 1000,
        'Mi': 1000000,
        'Gi': 1000000000,
        'Ti': 1000000000000,
        'Pi': 1000000000000000
    }

    default_unit_prefix = 'i'

    # Check value type
    if type(value) != str:
        print("[ERROR] Input(%s) is not valid." % type(value))
        return False

    value_tuple = value.split()

    # Check if input amount is valid
    try:
        value = float(value_tuple[0])
    except (ValueError, IndexError) as e:
        print("[ERROR] (%s) Invalid amount received: %s" % (e, value))
        return False

    # Check prefix and set unit value. If no prefix or prefix invalid, set to default. (Could cause problems?)
    try:
        unit_prefix = value_tuple[1]
        unit_value = standard_units[unit_prefix]
    except (IndexError, KeyError) as e:
        print("[WARNING] (%s) Invalid unit prefix given. Using default '%s'." % (e, default_unit_prefix))
        unit_value = standard_units[default_unit_prefix]

    return value * unit_value


########################
####### T E S T ########
########################
if __name__ == '__main__':
    print("Input value: '25.8 Ji'")
    print("Return value: {}\n\n".format(convert_value_to_standard_system_units('25.8 Ji')))
    print("Input value: ''")
    print("Return value: {}\n\n".format(convert_value_to_standard_system_units('')))
    print("Input value: '25.8 Ti'")
    print("Return value: {}\n\n".format(convert_value_to_standard_system_units('25.8 Ti')))
    print("Input value: '25.8 Pi'")
    print("Return value: {}\n\n".format(convert_value_to_standard_system_units('25.8 Pi')))
    print("Input value: '25.8 i'")
    print("Return value: {}\n\n".format(convert_value_to_standard_system_units('25.8 i')))
    print("Input value: '25.8Mi'")
    print("Return value: {}\n\n".format(convert_value_to_standard_system_units('25.8Mi')))
    print("Input value: 25.8 (float)")
    print("Return value: {}".format(convert_value_to_standard_system_units(25.8)))

TEST OUTPUT:

Input value: '25.8 Ji'
[WARNING] ('Ji') Invalid unit prefix given. Using default 'i'.
Return value: 25.8


Input value: ''
[ERROR] (list index out of range) Invalid amount received:
Return value: False


Input value: '25.8 Ti'
Return value: 25800000000000.0


Input value: '25.8 Pi'
Return value: 2.58e+16


Input value: '25.8 i'
Return value: 25.8


Input value: '25.8Mi'
[ERROR] (could not convert string to float: '25.8Mi') Invalid amount received: 25.8Mi
Return value: False


Input value: 25.8 (float)
[ERROR] Input(<class 'float'>) is not valid.
Return value: False

[jinankjain]

@vynes The main problem that I was facing was trying to integrate this conversion function into the system it caused a lot of error as I have to rewrite all the test.

[todofixthis]

@jinankjain @vynes Thanks for jumping on this!

Let me know if you have any questions or need assistance integrating it into the codebase. You can also find me on the IOTA slack (drop by the #iota-libs-pyota channel).

[vynes]

Am I right in saying that this should return int (not float), since iota (i) is the smallest unit?
Or left as float for lib to to do the rest?
If someone tries to send 1.4654168168181818 Pi should it be converted to the float 1465416816818181.8 and returned or rounded to 1465416816818182 and returned as int?

[todofixthis]

Interesting question!

I think returning a float makes the most sense in this case:

• Rounding is dangerous; it makes assumptions about what the caller is trying to do.
• Raising an exception makes some sense here, but again, this assumes that the caller only wants an int result.

Now, it's true that if the caller actually doesn't want a float value, we are putting the burden on the caller to validate the result... but odds are good that the caller will have to perform that validation anyway. Here's an example:

def create_txn(address, value):
  # type: (Address, Union[Text, int]) -> ProposedTransaction
  # If ``value`` is a formatted string, convert it into an integer amount.
  if isinstance(value, string_types):
    value = convert_units(value, 'i')

  # By this point, ``value`` must be an integer.
  # Either an integer was passed into the function,
  # or ``convert_units`` converted it into an int.
  if not isinstance(value, int):
    raise TypeError('``value`` must be an integer!')
  ...

In this example, create_txn expects either a string ('16.8 Ki') or integer (16800) for value. However, there's theoretically nothing preventing someone from passing a float instead, so even if convert_units doesn't get called, we still have to perform the type check directly in create_txn.

By allowing the convert_units function to return a float value, we enable applications that want to work with fractional IOTAs, without increasing the amount of work that int-only applications have to do.

[vynes]

Okay, that makes sense, thanks. I guess I was under the impression that IOTAs are not divisible.

[todofixthis]

You are correct that IOTAs are not divisible, according to the IOTA protocol.

However, there may be cases where an application might use fractional IOTAs internally (e.g., exchanges, computing interest/dividends, smart contracts, etc.).

In order to use those IOTAs in a transaction, it will have to convert the value into an integer, but until that point, it can do whatever it wants (:

[curVV]

Sorry to be a pain, but I think I might have misunderstood the context in which this converter will function.

In order to use those IOTAs in a transaction, it [the application] will have to convert the value into an integer, but until that point, it can do whatever it wants (:

Does that mean that this will only serve as a helper function which the application may or may not want to use? And not by the pyota lib receiving values from the application in the form of '1.2658 Mi'?

[todofixthis]

No worries. I'd like this to be integrated into PyOTA, but it should also exist as its own thing, so that application developers can take advantage of it, too.

E.g., developers should be able to do something like this:

iota.send_transfer([ProposedTransaction(..., value='1.2 Mi'), ...], ...)

However, I think it should be up to ProposedTransaction to reject fractional IOTAs, not the unit converter function.

E.g., if an application developer wants to do something like this:

iotas = convert_units('1.2345 Ki', 'i')
do_something_with(iotas)

We should also support that use case.

[curVV]

Good, makes sense.

Please have a look at: develop...vynes:unit_converter

Since the function needs to be accessible from both the api and also internally, I wasn't entirely sure where it should live. It is now just a standalone function inside of transaction.py but could maybe be a @staticmethod of ProposedTransaction?

I also thought of creating a IotaUnit type, but seemed a bit overkill for such simple thing.

The unit map (dict), I put in iota/__init__.py as "constant". Not sure if necessary as it might not ever be needed anywhere else?

And then also created a test which ran without error.

Let me know what you think.

[todofixthis]

Great, thanks @curVV ! I went ahead and created the pull request so that I could leave some comments:
#48

Overall, it looks really good; the convert_value_to_standard_unit implementation is really solid. There's a few minor items here and there that I've commented, and then it just needs some additional test coverage; then this is good to go!

[todofixthis]

Scheduled for release: 1.2.0