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

[frontend dapp-kit] add useSuiClientQueries hook #14913

Merged
merged 13 commits into from
Nov 27, 2023

Conversation

jovicheng
Copy link
Contributor

@jovicheng jovicheng commented Nov 18, 2023

Description

In certain scenarios, dApp needs to utilize useQueries for data retrieval.

Here is an example:

const { data: coinBalanceList = [] } = useSuiClientQuery("getAllBalances", {
  owner: walletAddress,
});

// After obtaining the balance for each coin, 
// it is necessary to further retrieve the metadata for each individual coin.
const [coinMetadataQuery, allBalanceQuery, ownedObjectsQuery] =
  useSuiClientQueries(
    { method: "getCoinMetadata", params: { coinType: "exampleCoinType" } },
    {
      method: "getAllBalances",
      params: { owner: "exampleOwnerAddress" },
      options: {
        queryKey: ["123"],
      },
    },
    // error  'params' is declared here.
    { method: "getOwnedObjects" }
  );

const CoinMetadata = coinMetadataQuery.data;
const allBalance = allBalanceQuery.data;
const ownedObjects = ownedObjectsQuery.data;

With the useSuiClientQueries hook, retrieving the aforementioned data will be much more easy.

If there are any changes needed or if you have any suggestions, please feel free to contact me at any time!
Thanks!

Test Plan

  • Existing tests
  • CI

How did you test the new or updated feature?


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

@jovicheng jovicheng requested a review from a team as a code owner November 18, 2023 17:58
Copy link

vercel bot commented Nov 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 2:36am
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 2:36am
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 2:36am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Nov 27, 2023 2:36am
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Nov 27, 2023 2:36am
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Nov 27, 2023 2:36am

method: T,
params: SuiRpcMethods[T]['params'],
options?: UseSuiClientQueryOptions<T, TData>,
][]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make these objects?

This looks like it will require all methods to be the same, I think this would be more useful if we could allow each method to be different

Copy link
Contributor

@hayes-mysten hayes-mysten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution, this seems like a great feature to add.

I think we will need to make some improvements around the typing so that this correctly handles mapping results for multiple different RPC methods in the same call, and we'll want to add a few tests for the new hook.

I am happy to advise any questions you have about making those changes yourself, or if you prefer we can also help make these fixes ourselves

@jovicheng
Copy link
Contributor Author

jovicheng commented Nov 20, 2023

Thank you for the contribution, this seems like a great feature to add.

I think we will need to make some improvements around the typing so that this correctly handles mapping results for multiple different RPC methods in the same call, and we'll want to add a few tests for the new hook.

I am happy to advise any questions you have about making those changes yourself, or if you prefer we can also help make these fixes ourselves

I understand. I will make it compatible with different RPC methods. Please give me some time. @hayes-mysten

@hayes-mysten
Copy link
Contributor

The typing here can be a little tricky, something like this is probably what you want:
Screenshot 2023-11-20 at 10 33 16 AM

@hayes-mysten
Copy link
Contributor

hayes-mysten commented Nov 20, 2023

@jovicheng to clarify, the issue here is the typescript types, not the actual functionality. We want to ensure that the returned data is strictly typed

results should be strictly typed, so that the results have correct types for each queried method
Screenshot 2023-11-20 at 10 37 12 AM

Each query should be type checked independently, so that errors are relevant to the query being executed
Screenshot 2023-11-20 at 10 38 42 AM

@jovicheng
Copy link
Contributor Author

jovicheng commented Nov 20, 2023

@jovicheng to clarify, the issue here is the typescript types, not the actual functionality. We want to ensure that the returned data is strictly typed

results should be strictly typed, so that the results have correct types for each queried method Screenshot 2023-11-20 at 10 37 12 AM

Each query should be type checked independently, so that errors are relevant to the query being executed Screenshot 2023-11-20 at 10 38 42 AM

I completely understand what you mean, let me delve deeper into studying these types. 🙌
Also, thank you for the code snippets above! @hayes-mysten

@hayes-mysten
Copy link
Contributor

hayes-mysten commented Nov 20, 2023

@williamrobertson13 the types I shared here don't cover typing for select, I assume we probably need to fix that as well?

Yeah, I think for useQueries we want to make sure useQueries returns correctly typed data if one were to pass the combine argument in the options list (https://tanstack.com/query/v5/docs/react/reference/useQueries).

Also a test suite similar to our existing test suite for useSuiClientQuery would be great if you're up for it @jovicheng :)

Copy link
Contributor

@williamrobertson13 williamrobertson13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work and thanks so much for the contribution :)

I'll merge this shortly, just need to get CI passing and might update some of the documentation super quick!

Update the repo to be based on the latest main branch to fix CI error
@williamrobertson13 williamrobertson13 merged commit 47b137d into MystenLabs:main Nov 27, 2023
39 of 41 checks passed
tzakian pushed a commit that referenced this pull request Nov 29, 2023
## Description 

In certain scenarios, dApp needs to utilize useQueries for data
retrieval.

Here is an example:

```typescript
const { data: coinBalanceList = [] } = useSuiClientQuery("getAllBalances", {
  owner: walletAddress,
});

// After obtaining the balance for each coin, 
// it is necessary to further retrieve the metadata for each individual coin.
const [coinMetadataQuery, allBalanceQuery, ownedObjectsQuery] =
  useSuiClientQueries(
    { method: "getCoinMetadata", params: { coinType: "exampleCoinType" } },
    {
      method: "getAllBalances",
      params: { owner: "exampleOwnerAddress" },
      options: {
        queryKey: ["123"],
      },
    },
    // error  'params' is declared here.
    { method: "getOwnedObjects" }
  );

const CoinMetadata = coinMetadataQuery.data;
const allBalance = allBalanceQuery.data;
const ownedObjects = ownedObjectsQuery.data;

```
With the `useSuiClientQueries` hook, retrieving the aforementioned data
will be much more easy.

If there are any changes needed or if you have any suggestions, please
feel free to contact me at any time!
Thanks!

## Test Plan 

- Existing tests
- CI

How did you test the new or updated feature?

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

### Type of Change (Check all that apply)

- [ ] protocol change
- [ ] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes

---------

Co-authored-by: hayes-mysten <135670682+hayes-mysten@users.noreply.github.com>
Co-authored-by: William Robertson <williamrobertson@mystenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants