-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Smart Contract queries controller #16
Conversation
@@ -1,2 +1,2 @@ | |||
[flake8] | |||
ignore = E501 | |||
ignore = E501, E722 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference (for reviewers):
|
||
If you want to skip network interaction tests run: | ||
``` | ||
pytest -m "not networkInteraction" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should have it the reversed way? That is, by default all except networkInteraction
? Just thinking out loud.
Fine this way, as well - tests of mx-chain-go
is the same: all tests by default, and long tests excluded on demand 👍
only: only run a specific test (run using: pytest -m "only") | ||
networkInteraction: only run API and Proxy network providers tests (run using `pytest -m networkInteraction` or run all tests excepting these using: `pytest -m "not networkInteraction"`) | ||
|
||
log_cli = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
|
||
class AccountOnNetwork(IAccountOnNetwork): | ||
def __init__(self, nonce: int, balance: str | int) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, in PY, I think we can switch to only have the int
variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now only supports int
multiversx_sdk/testutils/utils.py
Outdated
EGLD_NUM_DECIMALS = 18 | ||
|
||
|
||
def create_account_egld_balance(egld: int | str) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, in PY, I think we can switch to only have the int
variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now only supports int
caller=caller | ||
) | ||
|
||
legacy_query_response = self.network_provider.query_contract(legacy_query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we can do breaking changes in the network providers, I think we'll have them return core's SmartContractQueryResponse
- or, anyway, have this "adapter" logic in some other place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a special adapter for it.
parsed = controller.parse_query_response(response) | ||
assert parsed == ["abba".encode()] | ||
|
||
@pytest.mark.skip("Depends on the actual contract deployed on Devnet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always skip or only when networkInteraction
marker is missing - maybe have an adder
deployed beforehand (manually deployed)? Or query a system VM contract.
In the future, perhaps we can add some tests based on localnet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the pytes.mark.networkInteraction
marker. Will do some tests using the localnet in the future.
from multiversx_sdk.core.address import Address | ||
from multiversx_sdk.core.transaction import Transaction | ||
from multiversx_sdk.network_providers.proxy_network_provider import ( | ||
ContractQuery, ProxyNetworkProvider) | ||
|
||
|
||
@pytest.mark.networkInteraction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
from multiversx_sdk.testutils.utils import create_account_egld_balance | ||
|
||
|
||
class AccountOnNetwork(IAccountOnNetwork): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In JS, we've defined the class since, back then, the network providers package wasn't referenced as a dev dependency (for tests), but here, I think we can use this class:
https://github.com/multiversx/mx-sdk-py/blob/main/multiversx_sdk/network_providers/accounts.py#L8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now using the above class.
from typing import Protocol | ||
|
||
|
||
class IAccountOnNetwork(Protocol): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-check if can be dropped, if we use the existing AccountOnNetwork
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted the file, now using AccountOnNetwork
.
... | ||
|
||
|
||
class ContractQuery(IQuery): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the existing one usable here?
https://github.com/multiversx/mx-sdk-py/blob/main/multiversx_sdk/core/contract_query.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that one isn't used in core
anymore, but only by network providers
and adapters
, then add a TODO to be moved to network providers
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the implementation of ContractQuery
from the adapter, now using the ContractQuery
class from core
.
self.network_provider = network_provider | ||
|
||
def run_query(self, query: SmartContractQuery) -> SmartContractQueryResponse: | ||
legacy_query = ContractQuery( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the prefix "legacy", as in JS. Ultimately, they aren't legacy, it's just that they are the concept that the network provider uses, not the concepts that the query controller uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed it to adapted_query
here and in JS, as well.
class TestSmartContractQueriesController: | ||
provider = ProxyNetworkProvider("https://devnet-api.multiversx.com") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use @pytest.mark.networkInteraction
here also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no point in using the annotation for the class because there is no actual network interaction. In the first two tests i just create a query, then in the other tests i use a mock network provider and the last test actually interacts with the network but that one is annotated with @pytest.mark.networkInteraction
Code snippet: