-
Notifications
You must be signed in to change notification settings - Fork 36
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
Added suport for contract queries using abi file #436
Conversation
multiversx_sdk_cli/contracts.py
Outdated
|
||
response = sc_query_controller.run_query(query) | ||
|
||
if self._abi: |
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.
This if-else is not consistent.
a)
With ABI you return the data-parts
Without ABI your return other stuff + data-parts as Dict
b) errors for bad queries
With ABI you raise SmartContractQueryError
Without ABI you return the Dict with error message
1/ What is the reason you return different types?
To make it consistent you can just return sc_query_controller.parse_query_response(response)
but for NO abi case return parts as hex.
2/ Please add some unit tests for bad queries (wrong --function or whatever)
3/ I can't find test for contract query with abi
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, the if-else
is gone. The querying is done in a try-except
block so all the errors thrown are of type
QueryContractError
. Did a small "fix" for the json encoder to convert all bytes to hex. Also added tests.
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.
Looks good to me
multiversx_sdk_cli/cli_contracts.py
Outdated
@@ -443,17 +444,28 @@ def upgrade(args: Any): | |||
def query(args: Any): | |||
logger.debug("query") | |||
|
|||
# workaround so we can use the function bellow | |||
# workaround so we can use the function bellow to set chainID |
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.
Typo below
.
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.
fixed
multiversx_sdk_cli/cli_wallet.py
Outdated
@@ -134,6 +134,9 @@ def wallet_new(args: Any): | |||
else: | |||
mnemonic = Mnemonic.generate() | |||
|
|||
# this is done to get rid of the Pylance error: possibly unbound | |||
mnemonic = cast(Mnemonic, mnemonic) # type: ignore |
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.
Can also fix the warning by extracting the above logic to a separate function e.g. generate_mnemonic_with_shard_constraint()
or something like that.
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.
indeed, extracted to a separate function.
return_code = response["returnCode"] | ||
if return_code == "ok": | ||
print(f"name [{name}] is valid") | ||
else: | ||
print(f"name [{name}] is invalid") | ||
|
||
print(response) |
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.
Can be considered a breaking change by some (due to changing the stdout) - but this shouldn't be an issue anyway - we should just document it in the release notes.
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.
indeed, it will be documented in the release notes
multiversx_sdk_cli/dns.py
Outdated
chain_id = proxy.get_network_config().chain_id | ||
config = TransactionsFactoryConfig(chain_id) | ||
contract = SmartContract(config) |
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.
Can define a small function in this module e.g. query_contract(contract_address, proxy, function, args)
- to extract the common functionality - getting chain ID, creating config etc.
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.
done
@@ -63,6 +65,10 @@ class IContractQueryResponse(Protocol): | |||
return_data: List[str] | |||
return_code: str | |||
return_message: str | |||
gas_used: 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.
If we reference the latest & greatest feat/next of sdk-py, maybe we can drop this interface (or maybe not yet)?
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.
I don't think we can drop it.
if isinstance(o, ISerializable): | ||
return o.to_dictionary() | ||
if isinstance(o, bytes): |
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.
Generally speaking, is not a breaking change (e.g. it does not alter the output for existing flows that use --outfile or so).
multiversx_sdk_cli/utils.py
Outdated
if isinstance(o, ISerializable): | ||
return o.to_dictionary() | ||
if isinstance(o, bytes): | ||
return o.hex() | ||
if isinstance(o, list): |
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 this necessary? (I did not do an extra check).
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 what I've tested it's not. deleted it.
multiversx_sdk_cli/contracts.py
Outdated
@@ -202,7 +201,7 @@ def query_contract(self, | |||
contract_address: IAddress, | |||
proxy: INetworkProvider, | |||
function: str, | |||
arguments: List[Any], | |||
arguments: Union[List[Any], 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.
Can also be Optional
- or maybe not yet (older Python)?
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.
changed to Optional[List[Any]]
.
else: | ||
return [self._query_response_to_dict(response)] | ||
try: | ||
response = sc_query_controller.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.
So we don't support the caller
and callValue
parameters? But we didn't support them before anyway. I think we should, in the future.
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.
we can do that in the future, we didn't support them before.
multiversx_sdk_cli/cli_wallet.py
Outdated
|
||
# this is done to get rid of the Pylance error: possibly unbound | ||
mnemonic = cast(Mnemonic, mnemonic) # type: ignore | ||
mnemonic = _generate_mnemonic_with_shard_constraint(shard) |
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.
Could be: if shard is not none, then call the new function, else call Mnemonic.generate()
.
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.
done
multiversx_sdk_cli/dns.py
Outdated
arguments=[], | ||
args_from_file=False | ||
)[0] | ||
function="versgetRegistrationCostion", |
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.
Please fix this function :D
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.
thanks:) fixed
The same arguments as for contract calls have been added. The args are
--abi
and--arguments-file
. Keep in mind the args should be placed inside alist
like such:When performing queries without the abi file the response looks like this:
[ "0e" ]
But when using the abi file the response is parsed, and it looks like this:
There are small differences in how values are returned. For example, if an endpoint returns
variadic<Address>
, when querying without abi, the response looks like this:But when the abi is used, the response looks like this: