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

fix: subscribing on wallets #44

Closed
wants to merge 4 commits into from
Closed

fix: subscribing on wallets #44

wants to merge 4 commits into from

Conversation

MaxSKuzin
Copy link

No description provided.

@Alex-A4 Alex-A4 self-requested a review August 1, 2023 08:55
@@ -197,8 +197,6 @@ mixin TonWalletRepositoryImpl implements TonWalletRepository {
unsubscribe(asset.address);
}

lastUpdatedAssets = assets;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't updatelastUpdatedAssets, then you mustn't use last != null expression. Imho, it's better to remove lastUpdatedAssets, trying to use wallets for all checks. Look through the same behaviour in TokenWalletrepositoryImpl

@Alex-A4 Alex-A4 changed the title Fixed subscribing on wallets fix: subscribing on wallets Aug 1, 2023
@Alex-A4 Alex-A4 self-requested a review August 2, 2023 06:50
@@ -225,15 +211,21 @@ mixin TonWalletRepositoryImpl implements TonWalletRepository {

@override
Future<void> updateTransportSubscriptions() async {
closeAllSubscriptions();
final assets = wallets
Copy link
Contributor

@Alex-A4 Alex-A4 Aug 2, 2023

Choose a reason for hiding this comment

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

I think, this place is a bottle neck, because if you change transport until all subscriptions were completed, you will lose some part of wallets, so imho for this place you must use some cache list as it was lastUpdatedAddress and you won't lose necessary wallets. But lastUpdatedAddress must be used only here, so when you will revert this, leave a comment with description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also waiting for TokenWalletRepositoryImpl with the same logic

@Alex-A4
Copy link
Contributor

Alex-A4 commented Aug 21, 2023

Closed, because this logic implemented in #53

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.

2 participants