Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

revise query API on client #4892

Closed
mversic opened this issue Jul 25, 2024 · 3 comments · Fixed by #5184
Closed

revise query API on client #4892

mversic opened this issue Jul 25, 2024 · 3 comments · Fixed by #5184
Assignees
Labels
api-changes Changes in the API for client libraries Enhancement New feature or request good first issue Good for newcomers queries

Comments

@mversic
Copy link
Contributor

mversic commented Jul 25, 2024

          I don't like it that on the http client we have API like `asset::all_definitions()`. Because it differs from what we have in smart contracts. IMO there is nothing wrong in constructing a query like this:
    let vec = client
        .query(FindAllAssetDefinitions::new())

Definitely not intended for this PR, but would like to get an opinion on this

Originally posted by @mversic in #4833 (comment)

@mversic
Copy link
Contributor Author

mversic commented Jul 25, 2024

in any case API should be the same in client and smart contract

@mversic mversic mentioned this issue Jul 25, 2024
6 tasks
@mversic mversic added the good first issue Good for newcomers label Aug 5, 2024
@DCNick3 DCNick3 added Enhancement New feature or request api-changes Changes in the API for client libraries queries labels Aug 6, 2024
@nxsaken nxsaken added this to Iroha Sep 4, 2024
@nxsaken nxsaken moved this to Backlog in Iroha Sep 4, 2024
@nxsaken nxsaken moved this from Backlog to Work in Progress in Iroha Sep 4, 2024
@nxsaken nxsaken moved this from Work in Progress to Backlog in Iroha Sep 4, 2024
@0x009922
Copy link
Contributor

0x009922 commented Oct 7, 2024

I don't see much usability in this module-based client construction, to be honest. Using structures like FindPeers is direct and easy enough, all names are unified, and IDE suggests them straight away.

Apart from that, I want to emphasise again the importance of having an async client (#3130). Current design of QueryBuilder is limited to sync QueryExecutor trait. Due to that, I had to re-implement a slice of client from scratch (https://github.com/soramitsu/iroha2-block-explorer-backend/blob/9d0a77161bf17336b307ee83fe77e8f91bb8704d/src/iroha.rs).

@aoyako
Copy link
Contributor

aoyako commented Oct 22, 2024

I would like to work on this issue.

@mversic mversic moved this from Backlog to Required for MVP in Iroha Oct 22, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
api-changes Changes in the API for client libraries Enhancement New feature or request good first issue Good for newcomers queries
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants