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

fix: remove async for createProgramAddress and findProgramAddress (#2… #23185

Merged
merged 2 commits into from
Apr 18, 2022
Merged

fix: remove async for createProgramAddress and findProgramAddress (#2… #23185

merged 2 commits into from
Apr 18, 2022

Conversation

yangli-io
Copy link
Contributor

Fixes #23184

@mergify mergify bot added the community Community contribution label Feb 16, 2022
@mergify mergify bot requested a review from a team February 16, 2022 10:32
@codecov
Copy link

codecov bot commented Feb 16, 2022 •
edited
Loading

Codecov Report

Merging #23185 (8260d8d) into master (7939fdc) will decrease coverage by 11.5%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #23185       +/-   ##
===========================================
- Coverage    81.3%    69.7%    -11.6%     
===========================================
  Files         567       36      -531     
  Lines      154480     2217   -152263     
  Branches        0      314      +314     
===========================================
- Hits       125608     1546   -124062     
+ Misses      28872      559    -28313     
- Partials        0      112      +112

@t-nelson
Copy link
Contributor

Was the sha256 library changed to be sync at some point? That used to be what forced this family of methods to be async.

You can't break the API like this. Please add a sync variant instead.

@yangli-io
Copy link
Contributor Author

yangli-io commented Feb 17, 2022 •
edited
Loading

Thanks for the feedback, I forgot it will break people using promises. Updated to allow for a sync version.

I've also updated the tests to ensure backwards compatibility

@Arrowana
Copy link
Contributor

How breaking is this though? Code can await a non async method and awaiting with .then is still possible
Or is it just a matter of not touching the API types even if non technically "breaking"?

@yangli-io
Copy link
Contributor Author

it would be breaking if the developer has wrote their code as such

PublicKey.findProgramAddress([Buffer.from('', 'utf8')], programId).then();

I dont like the async versions but yes it would cause some issues. Probably be good make it completely sync in a major upgrade.

@stale
Copy link

stale bot commented Mar 2, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Mar 2, 2022
@joncinque joncinque mentioned this pull request Mar 31, 2022
Closed
joncinque
joncinque reviewed Mar 31, 2022
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

This looks good to me pending one tiny nit, and would be a useful addition. @t-nelson do you have any issues with this?

web3.js/src/publickey.ts
@@ -143,10 +143,10 @@ export class PublicKey extends Struct {
* Derive a program address from seeds and a program ID.
*/
/* eslint-disable require-await */
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Probably don't need this comment, right? Makes me wonder if this needed to be async for some reason

@stale stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Mar 31, 2022
t-nelson
t-nelson approved these changes Mar 31, 2022
View reviewed changes
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

@t-nelson do you have any issues with this?

Nope! I'm happy once you're happy

@stale
Copy link

stale bot commented Apr 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Apr 16, 2022
@joncinque joncinque merged commit a6742b5 into solana-labs:master Apr 18, 2022
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 30, 2022
@yangli-io @jeffwashington
fix: remove async for createProgramAddress and findProgramAddress (#2… (
8b62252 
solana-labs#23185)

* fix: remove async for createProgramAddress and findProgramAddress (solana-labs#23184)

make sync

* test: add test to ensure backwards compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joncinque joncinque joncinque left review comments

@t-nelson t-nelson t-nelson approved these changes

Assignees
No one assigned
Labels
community Community contribution stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

findProgramAddress does not need to be async
4 participants
@yangli-io @t-nelson @Arrowana @joncinque