Better sanity checks when creating/encoding transactions

fabrice102 commented 3 years ago

Currently, if arguments of transactions are of the wrong type, most of the time the SDK will not throw any exception and will even allow to sign and send the transactions. But then, the node will reject the transaction with a relatively cryptic error message.

For example, if you make the asset ID a string:

from algosdk import mnemonic
from algosdk.v2client import algod
from algosdk.future.transaction import AssetTransferTxn

passphrase = "patrol dog clean ancient negative despair pulp right false figure faint ethics film garment absorb gold capable busy juice tape arrange smooth twin able supply"
# address = TD5MTOHW7LE7RVIQW52WNQDIHUIJFLKTIAB5BGYLB3ZYHQAN4NU3MYNC4M
pk = mnemonic.to_public_key(passphrase) 
sk = mnemonic.to_private_key(passphrase)

algod_client = algod.AlgodClient(algod_token='', algod_address='https://api.testnet.algoexplorer.io', headers={ 'User-Agent': 'DoYouLoveMe?' })

params = algod_client.suggested_params()

asset_id = "14075399" # error: asset_id should be an int, not a string
txn = AssetTransferTxn(sender=pk, sp=params, receiver=pk, amt=0, index=asset_id)
stxn = txn.sign(sk)

txid = algod_client.send_transaction(stxn)

you get the following error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/algosdk/v2client/algod.py", line 72, in algod_request
    resp = urlopen(req)
  File "/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 214, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 523, in open
    response = meth(req, response)
  File "/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 632, in http_response
    response = self.parent.error(
  File "/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 561, in error
    return self._call_chain(*args)
  File "/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 494, in _call_chain
    result = func(*args)
  File "/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/lib/python3.9/urllib/request.py", line 641, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 400: Bad Request

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/algosdk/v2client/algod.py", line 77, in algod_request
    raise error.AlgodHTTPError(json.loads(e)["message"], code)
algosdk.error.AlgodHTTPError: msgpack decode error [pos 247]: cannot decode unsigned integer: unrecognized descriptor byte: a8/string|bytes

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/fabrice/tmp/ad.py", line 18, in <module>
    txid = algod_client.send_transaction(stxn)
  File "/usr/local/lib/python3.9/site-packages/algosdk/v2client/algod.py", line 172, in send_transaction
    return self.send_raw_transaction(encoding.msgpack_encode(txn),
  File "/usr/local/lib/python3.9/site-packages/algosdk/v2client/algod.py", line 189, in send_raw_transaction
    return self.algod_request("POST", req, data=txn, headers=headers, **kwargs)["txId"]
  File "/usr/local/lib/python3.9/site-packages/algosdk/v2client/algod.py", line 79, in algod_request
    raise error.AlgodHTTPError(e, code)
algosdk.error.AlgodHTTPError: {"message":"msgpack decode error [pos 247]: cannot decode unsigned integer: unrecognized descriptor byte: a8/string|bytes"}

I am thinking of two complementary solutions:

Add sanity checks in constructors of algosdk.future.* and/or when encoding/decoding, similar to what was done in https://github.com/algorand/py-algorand-sdk/pull/117 and https://github.com/algorand/py-algorand-sdk/pull/107 but in a systematic way.
Use type hinting. Unfortunately types are not checked by default. This requires either to use a type checker such as mypy, pyright, or an IDE such as PyCharm or VS Code with the PyLance extension and the PyLance setting "Type Checking Mode" set to "basic" or "strict". However, this is very convenient for users using such tools.

jkschin commented 3 years ago

@fabrice102 and @ian-algorand is the team taking PRs for this? Happy to follow #117 and #107 and implement something along these lines.

michielmulders commented 3 years ago

@jkschin Samuel, I'm sorry, @Niraj-Kamdar has requested to tackle this issue yesterday. If he can't finish the issue, I'm happy for you to tackle it and submit it to the bounty program. Thanks for understanding!

Niraj-Kamdar commented 3 years ago

Nice, I will add sanity checks for other data-structures as well then. This library looks more promising and I will check it out if it works better: https://typeguard.readthedocs.io/en/latest/userguide.html

Niraj-Kamdar commented 3 years ago

I have added sanity type check for all the transaction classes. Many tests also broke so it's working as expected 😉

algorand / py-algorand-sdk

Better sanity checks when creating/encoding transactions #179