create random accounts in memory for onboarding #1464
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (45)
|
|
@flexsurfer @siphiuel We still need to finish and merge this PR but you can start using the artifact you can find in CI. |
|
@flexsurfer @siphiuel did you have time to try it? Just to understand if the API is fine for status-react so that I can continue with this and start the reviews. |
|
hey @flexsurfer @siphiuel @hesterbruikman @guylouis |
|
@gravityblast sorry for delayed response. Yes the API is good, I'm integrating it now into the setup flow. |
|
awesome! no problem thank you @siphiuel !! |
account/accounts.go
Outdated
| @@ -365,6 +370,53 @@ func (m *Manager) Accounts() ([]gethcommon.Address, error) { | |||
| return filtered, nil | |||
| } | |||
|
|
|||
| // NewOnboarding starts the onboarding process generating accountsCount accounts and returns a slice of OnboardingAccount. | |||
| func (m *Manager) NewOnboarding(accountsCount, mnemonicPhraseLength int) ([]*OnboardingAccount, error) { | |||
There was a problem hiding this comment.
Any reason not to do:
| func (m *Manager) NewOnboarding(accountsCount, mnemonicPhraseLength int) ([]*OnboardingAccount, error) { | |
| func (m *Manager) SetOnboarding(onboarding *Onboarding) { |
|
|
||
| m.onboarding = onboarding | ||
|
|
||
| return m.onboarding.Accounts(), nil |
There was a problem hiding this comment.
It's odd that NewOnboarding returns accounts.
account/accounts.go
Outdated
| } | ||
|
|
||
| // ResetOnboarding reset the current onboarding struct setting it to nil and deleting the accounts from memory. | ||
| func (m *Manager) ResetOnboarding() { |
There was a problem hiding this comment.
| func (m *Manager) ResetOnboarding() { | |
| func (m *Manager) RemoveOnboarding() { |
because reset suggest that this object is still available just its state got reset.
|
|
||
| // ImportOnboardingAccount imports the account specified by id and encrypts it with password. | ||
| func (m *Manager) ImportOnboardingAccount(id string, password string) (Info, string, error) { | ||
| m.mu.Lock() |
There was a problem hiding this comment.
In order to avoid repeating Info{} you can do var info Info{} at the top and return info.
| } | ||
|
|
||
| for i := 0; i < n; i++ { | ||
| account, err := onboarding.generateAccount(mnemonicPhraseLength) |
There was a problem hiding this comment.
I think generateAccount could assign an ID.
|
|
||
| // Accounts return the list of OnboardingAccount generated. | ||
| func (o *Onboarding) Accounts() []*OnboardingAccount { | ||
| accounts := make([]*OnboardingAccount, 0) |
There was a problem hiding this comment.
| accounts := make([]*OnboardingAccount, 0) | |
| accounts := make([]*OnboardingAccount, len(o.accounts)) |
There was a problem hiding this comment.
@adambabik I put it back as it was. I forgot I was transforming from hash to slice, so there wasn't a clean way to get the index.
account/onboarding.go
Outdated
|
|
||
| func mnemonicPhraseLengthToEntropyStrenght(length int) (extkeys.EntropyStrength, error) { | ||
| if length < 12 || length > 24 || length%3 != 0 { | ||
| return 0, errInvalidMnemonicPhraseLength |
There was a problem hiding this comment.
This error is finally returned to the caller so it should be exported. In the description of this error, there should be information when a mnemonic is valid.
| expectedStrength extkeys.EntropyStrength | ||
| expectedError error | ||
| }{ | ||
| {12, 128, nil}, |
There was a problem hiding this comment.
Recommend https://golang.org/pkg/testing/quick/ for this test.
|
thank you for the review @adambabik ! |
|
@adambabik there's a warning from code climate becuase we have duplicate code in lib and mobile, but I think it's better to keep it like this for now. Is that blocking the merge? |
| return info, "", err | ||
| } | ||
|
|
||
| m.onboarding = nil |
There was a problem hiding this comment.
@adambabik just to remove everything related to the keys and the keys themselves
| errString = err.Error() | ||
| } | ||
|
|
||
| out := struct { |
There was a problem hiding this comment.
In my opinion it's a bit confusing. I would declare out at the top of the function and if an error is detected, I would do out.Error = errString.
Finally, it would be something like this:
out := struct {
// ...
}
if err != nil {
fmt.Fprintln(os.Stderr, err)
out.Error = errString
} else {
for _, account := range accounts {
// ...
}
}
outBytes, _ := json.Marshal(out)
return C.CString(string(outBytes))There was a problem hiding this comment.
thank you @adambabik , it makes sense, I updated the code
gravityblast
changed the title
@adambabik , yes I'm migrating the account management to rpc call in the multi-account PR, still WIP but if you want you can start taking a look here: |
|
@siphiuel I'm merging it! |
Implementation of Onboarding Generate 5 accounts and store one with the password.
closes #1491
Important changes:
Closes #1456