Skip to content
This repository was archived by the owner on Oct 7, 2024. It is now read-only.

Use async/await instead of then - #148

Merged
darkwing merged 2 commits into
mainfrom
async-await
Aug 25, 2022
Merged

Use async/await instead of then#148
darkwing merged 2 commits into
mainfrom
async-await

Conversation

@darkwing

@darkwingdarkwing commented Aug 24, 2022

Copy link
Copy Markdown
Contributor

Much easier to read than the mess of then's.

@darkwing
darkwing requested a review from a team as a code ownerAugust 24, 2022 21:52
mcmire
mcmire reviewed Aug 24, 2022
View reviewed changes

@mcmiremcmire left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good idea! Had some suggestions.

Comment threadindex.js
Comment threadindex.js Outdated
Comment threadindex.js Outdated
Comment threadindex.js Outdated
Comment threadindex.js Outdated
Comment threadindex.js Outdated
Comment threadindex.js Outdated
Gudahtt
Gudahtt reviewed Aug 25, 2022
View reviewed changes
Comment threadindex.js
Gudahtt
Gudahtt reviewed Aug 25, 2022
View reviewed changes
Comment threadindex.js
@@ -692,35 +660,35 @@ class KeyringController extends EventEmitter {
* @param {string} address - An account address.

@GudahttGudahttAug 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: There is a .then a few lines up from here in getAccounts that we can remove as well

Gudahtt
Gudahtt reviewed Aug 25, 2022
View reviewed changes
Comment threadindex.js Outdated
Gudahtt
Gudahtt reviewed Aug 25, 2022
View reviewed changes

@GudahttGudahtt left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work @darkwing ! This has been on my wishlist for a long time. A few suggestions but everything looks good

@darkwing@mcmire
Address feedback about using awaits with returns and throwing errors
ee7a5c7
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
ryanml
ryanml approved these changes Aug 25, 2022
View reviewed changes

@ryanmlryanml left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

awesome!

Gudahtt
Gudahtt approved these changes Aug 25, 2022
View reviewed changes

@GudahttGudahtt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@darkwing
darkwing merged commit e797bdf into mainAug 25, 2022
@darkwing
darkwing deleted the async-await branch August 25, 2022 22:29
@nitindudeja2107nitindudeja2107 mentioned this pull request Aug 26, 2022
Merged
Gudahtt added a commit that referenced this pull request Nov 7, 2022
@Gudahtt
Ensure newly created vaults are unlocked
a2926d9
A new vault should be unlocked after being created, because the
password is submitted as part of creating the vault. This was
accidentally broken as part of #148. A unit test has been added to
ensure this doesn't happen again.
@GudahttGudahtt mentioned this pull request Nov 7, 2022
Merged
Gudahtt added a commit that referenced this pull request Nov 7, 2022
@Gudahtt
Ensure newly created vaults are unlocked (#155)
bc5716f
A new vault should be unlocked after being created, because the
password is submitted as part of creating the vault. This was
accidentally broken as part of #148. A unit test has been added to
ensure this doesn't happen again.
@legobeatlegobeat mentioned this pull request Sep 10, 2023
Closed
9 tasks
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers
@mcmiremcmiremcmire left review comments
@GudahttGudahttGudahtt approved these changes
@ryanmlryanmlryanml approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@darkwing@mcmire@Gudahtt@ryanml