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

[dapp-kit] Add ability to configure networks to SuiClientProvider #13342

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

hayes-mysten
Copy link
Contributor

Description

This is getting into react stuff where I am not very confident I know what I am doing. The goal here is to provide a Context provider that supports switching between networks, and allows providing either pre-configured SuiClients for each network, or a config object (with customizable options) that can be used to create a client.

Test Plan

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

@hayes-mysten hayes-mysten requested a review from a team as a code owner August 9, 2023 22:14
@vercel
Copy link

vercel bot commented Aug 9, 2023

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

Name Status Preview Comments Updated (UTC)
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2023 6:42pm
5 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Aug 14, 2023 6:42pm
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Aug 14, 2023 6:42pm
mysten-ui ⬜️ Ignored (Inspect) Visit Preview Aug 14, 2023 6:42pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Aug 14, 2023 6:42pm
sui-wallet-kit ⬜️ Ignored (Inspect) Visit Preview Aug 14, 2023 6:42pm

@github-actions
Copy link
Contributor

github-actions bot commented Aug 10, 2023

⚠️ 🦋 Changesets Warning: This PR has changes to public npm packages, but does not contain a changeset. You can create a changeset easily by running pnpm changeset in the root of the Sui repo, and following the prompts. If your change does not need a changeset (e.g. a documentation-only change), you can ignore this message. This warning will be removed when a changeset is added to this pull request.

Learn more about Changesets.

new SuiClient({
url: props.url ?? getFullnodeUrl('devnet'),
});
const DEFAULT_NETWORKS = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I said this in another comment but IMO this default either should be more comprehensive, or only be localnet.

Copy link
Contributor Author

@hayes-mysten hayes-mysten Aug 14, 2023

Choose a reason for hiding this comment

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

changed this to localnet for now

};

export function SuiClientProvider<T extends NetworkConfigs>(props: SuiClientProviderProps<T>) {
const networks = props.networks ?? (DEFAULT_NETWORKS as never);
Copy link
Contributor

Choose a reason for hiding this comment

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

as never casting here is weirding me out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change this to (props.networks ?? DEFAULT_NETWORKS) as T which is the same thing, since props.networks is T, this will throw away the DEFAULT_NETWORKS part of the union effectively making it never.

sdk/dapp-kit/vitest.config.ts Outdated Show resolved Hide resolved
@hayes-mysten hayes-mysten force-pushed the hayes/dapp-kit--provider-networks branch from 1e35807 to af2c4ad Compare August 14, 2023 18:41
@hayes-mysten hayes-mysten enabled auto-merge (squash) August 14, 2023 18:43
@hayes-mysten hayes-mysten merged commit 65ecb84 into main Aug 14, 2023
@hayes-mysten hayes-mysten deleted the hayes/dapp-kit--provider-networks branch August 14, 2023 18:52
damirka pushed a commit that referenced this pull request Aug 23, 2023
…3342)

## Description 

This is getting into react stuff where I am not very confident I know
what I am doing. The goal here is to provide a Context provider that
supports switching between networks, and allows providing either
pre-configured SuiClients for each network, or a config object (with
customizable options) that can be used to create a client.

## Test Plan 

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
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.

4 participants