Deprecation: Add deprecation warnings on v1 algod API and old transaction format #381
Conversation
algosdk/transaction.py
Outdated
| Get the transaction's ID. | ||
|
|
||
| Returns: | ||
| str: transaction ID | ||
| """ | ||
| warnings.warn( |
There was a problem hiding this comment.
Adding warnings to each method might be pedantic (compared to just having a warning in the constructor and static methods) - in algod.py, only the constructor has a DeprecationWarning triggered.
There was a problem hiding this comment.
I agree warnings in each method is likely too much. If there's a warning in the constructor, that should be good enough in my opinion
| @@ -6,6 +6,9 @@ | |||
|
|
|||
|
|
|||
| class Template: | |||
| """ | |||
| NOTE: This class is deprecated | |||
There was a problem hiding this comment.
note: future.template already has a deprecation warning in the comments.
algochoi
changed the title
| @@ -383,7 +381,7 @@ def export_multisig(self, handle, address): | |||
| result = self.kmd_request("POST", req, data=query) | |||
| pks = result["pks"] | |||
| pks = [encoding.encode_address(base64.b64decode(p)) for p in pks] | |||
| msig = transaction.Multisig( | |||
| msig = future.transaction.Multisig( | |||
There was a problem hiding this comment.
The old and new Multisig should be the same - changed to used the new class in future here.
| @@ -273,6 +271,13 @@ def undictify(d): | |||
| def __eq__(self, other): | |||
| if not isinstance(other, (Transaction, transaction.Transaction)): | |||
| return False | |||
| if isinstance(other, transaction.Transaction): | |||
There was a problem hiding this comment.
Added a warning for people who might try to compare the old Transaction object to a future.Transaction object
algosdk/transaction.py
Outdated
| Get the transaction's ID. | ||
|
|
||
| Returns: | ||
| str: transaction ID | ||
| """ | ||
| warnings.warn( |
There was a problem hiding this comment.
I agree warnings in each method is likely too much. If there's a warning in the constructor, that should be good enough in my opinion
… functions have it
| warnings.warn( | ||
| "`msgpack_decode` is being deprecated. " | ||
| "Please use `future_msgpack_decode` instead.", | ||
| DeprecationWarning, | ||
| ) |
There was a problem hiding this comment.
I don't have much objection here, just a minor question: since encode-decode is coupled, do we want to mark encode as deprecated? Or it is because of Transaction had a structural change (i.e., some new fields)?
There was a problem hiding this comment.
Serializing into msgpack is okay - the old Transaction and new Transaction can both be serialized into msgpack as long as the fields are ordered and can be represented by a key-value map/dict. Decoding msgpack into the old Transaction does not seem preferable, so marked it with the deprecation warning here.
This PR adds deprecation warnings to v1 algod APIs and the old transaction format. In addition, there are warnings raised when the deprecated methods are called.
The PR also adds a deprecation comment to
template- previously, a similar comment was put in forfuture.template.The
DeprecationWarningthat is added to this PR and the langspec PR does trigger warnings on pytest (pytest tests/unit_tests). The corresponding tests should also be deleted when these features are removed from the SDK.Related issue: algorand/algorand-sdk-testing#218