importAccountWithStrategy shouldn't select address
#1309
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mikesposito
force-pushed
the
feat/1249-importaccountwithstrategy
branch
from
4644392
to
d6a88a0
Compare
Gudahtt
reviewed
May 3, 2023
mikesposito
changed the title
importAccountWithStrategy shouldn't select address
Gudahtt
reviewed
May 3, 2023
| ).rejects.toThrow( | ||
| 'Expected private key to be an Uint8Array with length 32', | ||
| ); | ||
| it('should not select imported account', async () => { |
There was a problem hiding this comment.
Nit: Should we repeat this test for the JSON input as well? Or maybe bring it to the top-level of the importAccountWithStrategy block to show that it's not expected to differ between strategies?
There was a problem hiding this comment.
Uhm, good point - I think it would make more sense to add it for the JSON input, rather than moving it to the top-level group because otherwise it would go along with:
- using strategy privateKey
- using strategy json
- using unrecognized strategy
EDIT: Also, the actual test would be different, as we would use a different strategy - so I think it makes even more sense to repeat it for JSON
mikesposito
force-pushed
the
feat/1249-importaccountwithstrategy
branch
from
d6a88a0
to
78d3c40
Compare
mikesposito
force-pushed
the
feat/1249-importaccountwithstrategy
branch
from
bee319d
to
edd0460
Compare
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
* feat: importAccountWithStrategy doesn't select new address * test: add test case for json import
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
* feat: importAccountWithStrategy doesn't select new address * test: add test case for json import
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR refactors the
importAccountWithStrategymethod from KeyringController to not select the imported account.As a new test case has been added, I took the chance to refactor tests groups for the
importAccountWithStrategy.Changes
KeyringController.importAccountWithStrategyto not select imported addressReferences
importAccountWithStrategyshould not change selected account #1249Checklist