-
Notifications
You must be signed in to change notification settings - Fork 169
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
Fix import wallet account instead of creating account? #111 #173
Conversation
@@ -76,6 +76,19 @@ public class EthereumAccount: EthereumAccountProtocol { | |||
throw EthereumAccountError.createAccountError | |||
} | |||
} | |||
|
|||
public static func importAccount(keyStorage: EthereumKeyStorageProtocol, privateKey: String, keystorePassword password: String) throws -> EthereumAccount { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change privateKey
to be Data
, then you'll avoid the throw below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change
privateKey
to beData
, then you'll avoid the throw below.
About changing the privateKey as Data I dont agree with that. Its better to create an overloaded method to accept also Data as param instead of accepting only Data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, can be either way
Great thanks for the PR, see the comment above, plus could you please add a unit test for this new method? |
Added the unit test. About changing the privateKey as Data I dont agree with that. Its better to create an overloaded method to accept also Data as param instead of accepting only Data. |
func testImportAccount() { | ||
let account = try? EthereumAccount.importAccount(keyStorage: EthereumKeyLocalStorage(), privateKey: "0x2639f727ded571d584643895d43d02a7a190f8249748a2c32200cfc12dde7173", keystorePassword: "PASSWORD") | ||
|
||
XCTAssertNotNil(account, "Failed to import account.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for nitpick, but better to check account?.address.value == "0x..." instead of nil to be sure it was imported correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok perfect ;)
It would be nice to be able to import a wallet account! Fixes #111
We don't want to edit the package to add that functionality locally