Transaction Converter & refactoring and reorganizing #17
Conversation
| @@ -314,7 +314,7 @@ | |||
| ], | |||
| "source": [ | |||
| "from multiversx_sdk.core import TokenComputer\n", | |||
| "from multiversx_sdk.core.transaction_factories import \\\n", | |||
| "from multiversx_sdk.core.transactions_factories import \\\n", | |||
There was a problem hiding this comment.
Minor, easy to welcome breaking change.
examples/Cookbook.ipynb
Outdated
| "\n", | ||
| "provider = ProxyNetworkProvider(\"https://devnet-gateway.multiversx.com\")\n", | ||
| "provider = ProxyNetworkProvider(\"https://devnet-gateway.multiversx.com\", TransactionConverter())\n", |
There was a problem hiding this comment.
Additional breaking change. Very good from a design point of view. Perhaps less welcomed by some developers.
Let's brainstorm an eventual facade package - first, in the specs.
There was a problem hiding this comment.
Perhaps we can provide also the default value?
| def __init__( | ||
| self, | ||
| url: str, | ||
| transaction_converter: ITransactionConverter, |
There was a problem hiding this comment.
As mentioned above, it's a breaking change.
In the specs, we use the plural variant: https://github.com/multiversx/mx-sdk-specs/blob/main/converters/transactionsConverter.md.
There was a problem hiding this comment.
Renamed to ITransactionsConverter.
| ITransaction, TransactionOnNetwork) | ||
|
|
||
|
|
||
| class ITransactionConverter(Protocol): |
There was a problem hiding this comment.
Should be plural, I think.
| transaction.options = transaction.options | TRANSACTION_OPTIONS_TX_GUARDED | ||
| transaction.guardian = guardian | ||
|
|
||
| def _to_dictionary(self, transaction: ITransaction) -> Dict[str, Any]: |
There was a problem hiding this comment.
Correct, this should not use the new converter 👍
(it has other reasons for change / wrt. SRP).
| def __init__( | ||
| self, | ||
| url: str, | ||
| transaction_converter: ITransactionConverter, |
There was a problem hiding this comment.
It's a breaking change (we can postpone it via some workarounds, but perhaps we should not).
Also, can use plural variant.
There was a problem hiding this comment.
Using the plural variant
| def __init__(self, transaction: Transaction) -> None: | ||
| self.expected = transaction | ||
|
|
||
| def __eq__(self, actual: object) -> bool: |
There was a problem hiding this comment.
Interesting. Maybe we can also promote this as a core functionality (just a question)?
There was a problem hiding this comment.
Well, I think this is more for testing purposes, but we can think about it.
| return Transaction( | ||
| nonce=dictionary.get("nonce", None), | ||
| value=int(dictionary.get("value", None)), | ||
| receiver=dictionary["receiver"], |
There was a problem hiding this comment.
Indeed, sender, receiver, gas limit, chain ID are required - but if the input dictionary is missing them, the resulting exception will not be very intelligible (JS implementation would suffer from the same thing, but there we have the legacy IPlainTransaction interface).
Maybe validate the input dictionary around the required fields and throw exceptions with explicit messages?
Maybe can also stay as it is.
There was a problem hiding this comment.
added a method to validate that the fields sender, receiver, chainID and gasLimit are not missing
| def _value_to_b64_or_empty(self, value: str | bytes) -> str: | ||
| value_as_bytes = value.encode() if isinstance(value, str) else value | ||
|
|
||
| if value and len(value): |
There was a problem hiding this comment.
Using len(value) only would be all right? Below as well?
| return b"" | ||
|
|
||
| def _bytes_from_hex(self, value: str) -> bytes: | ||
| print(value) |
There was a problem hiding this comment.
Rogue print statement.
…ncy from network providers constructors
| from multiversx_sdk.core.transaction import Transaction | ||
|
|
||
|
|
||
| class TransactionsConverter: |
There was a problem hiding this comment.
in the specs there is also transaction_on_network_to_outcome(transaction: TransactionOnNetwork): TransactionOutcome; do we implement this one also? or not yet?
There was a problem hiding this comment.
That has been added later. Will be implemented in a future PR.
Implemented the
TransactionConverterclass according to the sdk-specs. Also moved theadapterspackage outside of thecorepackage and theTransactionComputerclass out of thetransactionmodule in thetransaction_computermodule.