Skip to content
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

brick doesn't allow checking return value of contract calls #104

Closed
aspiers opened this issue Feb 3, 2020 · 5 comments · Fixed by #174
Closed

brick doesn't allow checking return value of contract calls #104

aspiers opened this issue Feb 3, 2020 · 5 comments · Fixed by #174
Assignees
Labels
help wanted Extra attention is needed proposal UX Not bugs or features, but areas where the user/developer experience could be better

Comments

@aspiers
Copy link
Contributor

aspiers commented Feb 3, 2020

In brick, query allows an optional expected_query_result parameter for checking the return value from the query. However there is no equivalent parameter for checking the return value from a call. This would be a useful feature.

@hanlsin hanlsin added help wanted Extra attention is needed proposal labels Feb 4, 2020
@hanlsin
Copy link
Member

hanlsin commented Feb 4, 2020

It would be a great idea for the call tx.

I set this as a proposal.

Please look at this @sunpuyo :)

@sunpuyo
Copy link
Contributor

sunpuyo commented Feb 5, 2020

If call tx has a return value, it is not recommended because it will be charged more fee. It is good to use query to check the state change caused by call tx.

@aspiers
Copy link
Contributor Author

aspiers commented Feb 5, 2020

@sunpuyo Thanks a lot for the response. I know that @query is cheaper than @call, but still brick needs to support checking return values of contract calls for at least three reasons:

  1. Queries do not include a signature from the sender, so system.getSender() is not defined and therefore it is impossible to implement access control for queries.
  2. Sometimes it makes sense for @call to change the state and return a value associated with the result, e.g. some calculation. In this case it does not make sense to store the result separately so that it can be retrieved via a @query, because that would require a call followed by a query, which is more expensive than just a call.
  3. Ideally brick should be able to test any supported feature. Returning a value from a call is supported, therefore it should be testable by brick :-)

Thanks again!

@sunpuyo
Copy link
Contributor

sunpuyo commented Feb 6, 2020

@aspiers I agree with the need for this feature. However, to support this, the syntax of call and query must be identical. For e.g., If I modify the call syntax similar to query as follows:
call <sender_name> <amount> <contract_name> <func_name> <call_json_str> [expected_query_result] [expected_error]
Existing brick codes that use 'expected_error' will not work as expected.
Or if I change like this;
call <sender_name> <amount> <contract_name> <func_name> <call_json_str> [expected_error] [expected_query_result]
It is different order from the syntax of the query. And users will be confused.
query <contract_name> <func_name> <query_json_str> [expected_query_result] [expected_error]
So it's hard to change it.

@aspiers
Copy link
Contributor Author

aspiers commented Feb 8, 2020

I think it's OK if they are a different order. It's not ideal but it's better as a short-term fix than not being able to check the return value at all.

Thinking longer term, it should be possible to migrate both commands to a new syntax in which the order doesn't matter, by prefixing optional arguments with identifiers like error: and result:, e.g.:

call <sender_name> <amount> <contract_name> <func_name> <call_json_str> [error:<expected_error>] [result:<expected_query_result>]

but also:

call <sender_name> <amount> <contract_name> <func_name> <call_json_str> [result:<expected_query_result>] [error:<expected_error>]

@aspiers aspiers added the UX Not bugs or features, but areas where the user/developer experience could be better label Feb 13, 2020
@kroggen kroggen linked a pull request Jun 16, 2022 that will close this issue
@kroggen kroggen closed this as completed May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed proposal UX Not bugs or features, but areas where the user/developer experience could be better
Projects
None yet
4 participants