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.

Sign up for GitHub

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

Jump to bottom

Delete async when new wallet context in sui sdk #16459

Merged
merged 5 commits into from
Mar 13, 2024
Merged

Delete async when new wallet context in sui sdk #16459

merged 5 commits into from
Mar 13, 2024

Conversation

ccbond
Copy link
Contributor

@ccbond ccbond commented Feb 29, 2024 •
edited by ronny-mysten
Loading

Description

Fix #16458

Test Plan

If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.

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

The WalletContext::new method does not have any asynchronous dependencies, and thus, it does not need to be defined as an asynchronous function. This PR is a breaking change that modifies the signature of WalletContext::new into a synchronous function. Users that depend on WalletContext::new need to update callsites accordingly by dropping the .await.

@vercel Vercel
Copy link

vercel bot commented Feb 29, 2024 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 3:57am
multisig-toolkit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 3:57am
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 3:57am
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 3:57am
sui-kiosk ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 3:57am
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 3:57am

@wlmyng
Copy link
Contributor

wlmyng commented Mar 5, 2024

@ccbond thanks for taking a stab at this. Now that WalletContext::new is no longer async, we also need to remove all instances of .await from the codebase. cc: @stefan-mysten

@vercel vercel bot temporarily deployed to Preview – mysten-ui March 8, 2024 08:29 Inactive
@ccbond
Copy link
Contributor Author

ccbond commented Mar 8, 2024 •
edited
Loading

@wlmyng i use global find all functions used WalletContext::new(), judgment and delete all useless async define. Maybe you can help me review this pr.

@wlmyng
Copy link
Contributor

wlmyng commented Mar 8, 2024

@ccbond thanks for cleaning up some of the issues! It should be mostly clerical work at this point - do you mind taking a look at the errors from ci and cleaning those up as well?

@ccbond
Copy link
Contributor Author

ccbond commented Mar 8, 2024

of course. @wlmyng thanks for your review.

@vercel vercel bot temporarily deployed to Preview – mysten-ui March 8, 2024 19:39 Inactive
wlmyng
wlmyng reviewed Mar 8, 2024
View reviewed changes
Copy link
Contributor

@wlmyng wlmyng left a comment

Choose a reason for hiding this comment

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

I believe there's still a few spots to clean up. Would you also be able to pull from main to pick up the pnpm audit fix?

crates/sui-cluster-test/src/faucet.rs Outdated Show resolved Hide resolved
crates/sui-cluster-test/src/wallet_client.rs Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – mysten-ui March 8, 2024 19:58 Inactive
@ccbond
Copy link
Contributor Author

ccbond commented Mar 8, 2024 •
edited
Loading

I believe there's still a few spots to clean up. Would you also be able to pull from main to pick up the pnpm audit fix?

When i run pnpm audit, i got

image image image

i can't get any valid info. It seems that there is no information related to my PR, did I miss anything?

@vercel vercel bot temporarily deployed to Preview – mysten-ui March 8, 2024 21:19 Inactive
@ccbond ccbond requested a review from wlmyng March 8, 2024 21:54
@vercel vercel bot temporarily deployed to Preview – mysten-ui March 11, 2024 02:45 Inactive
@ccbond
Copy link
Contributor Author

ccbond commented Mar 11, 2024

All the previous tests passed, so I pull rebase the latest code but its needed to approve the tests again. @wlmyng

wlmyng
wlmyng approved these changes Mar 11, 2024
View reviewed changes
Copy link
Contributor

@wlmyng wlmyng left a comment

Choose a reason for hiding this comment

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

looks good!

wlmyng
wlmyng requested changes Mar 11, 2024
View reviewed changes
Copy link
Contributor

@wlmyng wlmyng left a comment

Choose a reason for hiding this comment

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

wanted to call out a slight change

crates/sui-cluster-test/src/faucet.rs Show resolved Hide resolved
crates/sui-cluster-test/src/wallet_client.rs Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – mysten-ui March 12, 2024 03:57 Inactive
@ccbond ccbond requested a review from wlmyng March 12, 2024 07:45
@ccbond
Copy link
Contributor Author

ccbond commented Mar 13, 2024

@wlmyng Is there anything else I can change？

wlmyng
wlmyng approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@wlmyng wlmyng left a comment

Choose a reason for hiding this comment

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

Looks good - let's ship this

@stefan-mysten
Copy link
Contributor

Thanks for being persistent with this change @ccbond!
@wlmyng feel free to merge it!

@wlmyng wlmyng merged commit 58e3a06 into MystenLabs:main Mar 13, 2024
41 checks passed
tx-tomcat pushed a commit to tx-tomcat/sui-network that referenced this pull request May 30, 2024
@ccbond @tx-tomcat
Delete async when new wallet context in sui sdk (MystenLabs#16459)
87b9baa 
## Description 

Fix MystenLabs#16458 

## Test Plan 



---
If your changes are not user-facing and do not break anything, you can
skip the following section. Otherwise, please briefly describe what has
changed under the Release Notes section.

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

- [ ] protocol change
- [ ] user-visible impact
- [x] 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
The `WalletContext::new` method does not have any asynchronous
dependencies, and thus it does not need to be defined as an asynchronous
function. This PR is a breaking change that modifies the signature of
`WalletContext::new` into a synchronous function. Users that depend on
`WalletContext::new` will need to update callsites accordingly by
dropping the `.await`.

---------

Co-authored-by: Hgamiui9 <jinlong@cplus.zone>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wlmyng wlmyng wlmyng approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Please delete async when new wallet context in sui-sdk
3 participants
@ccbond @wlmyng @stefan-mysten