Skip to content
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

Add account manager #37

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add account manager #37

wants to merge 2 commits into from

Conversation

rscripnic
Copy link
Contributor

No description provided.

import 'package:fuelet_secure_layer/src/features/wallet_import/entity/wallet_import_typedef.dart';

class AccountsManagerImpl extends AccountsManager {
final int _loadingDerevativeAccountCount = 20;
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: derivative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

(Account account, String seedPhrase) accountSeedPhraseItem =
accountSeedPhrases[i];

(String phrase, List<Account> accout) derivativeAccounts =
Copy link
Contributor

Choose a reason for hiding this comment

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

typo. Can we defined it like (String, List<Account>) or just final or var?

Copy link
Contributor Author

@rscripnic rscripnic Dec 2, 2024

Choose a reason for hiding this comment

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

it's for you, more readable

await _defineAccountSeedPhrases(currentAccounts);

for (int i = 0; i < accountSeedPhrases.length; i++) {
(Account account, String seedPhrase) accountSeedPhraseItem =
Copy link
Contributor

Choose a reason for hiding this comment

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

same: do we need variable names here in defintion? Why not just (Account, String) accountSeedPhraseItem =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not problem, but, if Account is obvious, then String, what is it

Comment on lines +47 to +48
element.fuelAddress.bech32Address ==
currentAccountItem.fuelAddress.bech32Address)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we compare not bech32Address, but just addresses? element.fuelAddress == currentAccountItem.fuelAddress?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think, comparing by primitive type is better, because Account object is not finished yet

return resultMap;
}

Future<List<(Account, String)>> _defineAccountSeedPhrases(
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible that several accounts will have the same seed phrase, we should filter them out here at return just distinct phrases. Also, I don't think that we need to return an account from this function, just List<String> should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

further we use this result, and need Account

Copy link
Contributor

Choose a reason for hiding this comment

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

you can just use the first account from the list of derivative accounts, that'd be more accurate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants