Mobile status.go: getRandomMnemonic #4712
Conversation
Jenkins BuildsClick to see older builds (41)
|
OmarBasem
force-pushed
the
feat/mobile-mnemonic
branch
from
b1da689
to
3726d71
Compare
mobile/status.go
Outdated
| @@ -1327,3 +1327,11 @@ func InitLogging(logSettingsJSON string) string { | |||
|
|
|||
| return makeJSONResponse(err) | |||
| } | |||
|
|
|||
| func GetRandomMnemonic() string { | |||
| resp, err := statusBackend.AccountManager().GetRandomMnemonic() | |||
There was a problem hiding this comment.
Hi @OmarBasem , sorry for the delay, just back from public holiday, overall LGTM. However, I'm confused about why GetRandomMnemonic was attached to DefaultManager, as it doesn't seem to relate to the DefaultManager instance. We don't use anything related to DefaultManager within GetRandomMnemonic.
Something like following would be more clearer for me:
func GetRandomMnemonic() (string, error) {
// generate mnemonic phrase
mn := extkeys.NewMnemonic()
mnemonic, err := mn.MnemonicPhrase(extkeys.EntropyStrength128, extkeys.EnglishLanguage)
if err != nil {
return "", fmt.Errorf("can not create mnemonic seed: %v", err)
}
return mnemonic, nil
}
WDYT?
There was a problem hiding this comment.
@qfrank why reimplement the method? I am calling GetRandomMnemonic from AccountManager similar for example to VerifyAccountPassword in the same file
There was a problem hiding this comment.
Hi @OmarBasem,
This is not relate to your PR, it's about refactor. The reason for the suggested change is to enhance code clarity and maintainability. By directly implementing GetRandomMnemonic without routing it through DefaultManager, we make the function's operation more transparent and reduce the dependency on external structures where it's not necessary.
The goal is to have GetRandomMnemonic as a standalone function that can be used independently of the AccountManager context. This can be beneficial for future use cases where we might need a mnemonic that doesn't rely on the account management logic, and also makes the function more testable in isolation.
The reference to VerifyAccountPassword seems to follow a similar context where it didn't use any fields or context with m (DefaultManager). If we find that this pattern doesn't serve a practical purpose or complicates the code unnecessarily, we should consider refactoring it as well.
Looking forward to your thoughts on this.
There was a problem hiding this comment.
Hi @qfrank, thanks for the clarification. I have updated the function 👍
There was a problem hiding this comment.
Thank you for the effort! But now there're repeat code(sorry for the confusion). Actually, what i expected was something like following in status.go 🙂:
func GetRandomMnemonic() string {
...
mnemonic, err := account.GetRandomMnemonic()
...
There was a problem hiding this comment.
@qfrank Btw, I don't think there was something wrong with my initial approach. We already have access to accountManager in that file, and it is used in multiple places in that file. So why not simply statusBackend.AccountManager().GetRandomMnemonic()? Cause right now using the suggested approach we are duplicating code.
If you are suggesting not to use accountManager at all in that file (I am not sure why) then that would be a different issue. A refactor for the whole file wherever AccountManager is being used.
There was a problem hiding this comment.
just made a branch to demo my thoughts:
9401093
maybe using program language to express would be more clearer @OmarBasem
There was a problem hiding this comment.
@qfrank we don't have access to
accounthere. I am using your suggested function:mn := extkeys.NewMnemonic() mnemonic, err := mn.MnemonicPhrase(extkeys.EntropyStrength128, extkeys.EnglishLanguage)
we have in my demo branch
There was a problem hiding this comment.
Btw, I don't think there was something wrong with my initial approach.
You're right, that's why i mentioned not relate to your PR, it's about refactor. The old implementation(not yours) for GetRandomMnemonic confused me, it's more like a util function, should not bind to AccountManager instance IMO
There was a problem hiding this comment.
It is more clear now @qfrank, thank you! I have updated it 👍
OmarBasem
force-pushed
the
feat/mobile-mnemonic
branch
from
85784e9
to
d924aeb
Compare
OmarBasem
force-pushed
the
feat/mobile-mnemonic
branch
from
4bd753f
to
2014cce
Compare
* feat: mobile status.go - getRandomMnemonic (#4712)
fixes: #4711
This PR adds the function
getRandomMnemonictomobile/status.go