-
Notifications
You must be signed in to change notification settings - Fork 1
fix: avoid showing sync failed immediately when app comes to foreground #359
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ovitrif
left a comment
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.
utAck
maybe 1 little nit:
could use init instead of initialize/ initialization in fn names 🙏🏻
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…m/synonymdev/bitkit-ios into fix/sync-failed-coming-foreground
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ovitrif
left a comment
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.
re utAck
Bitkit/MainNavView.swift
Outdated
| settings.enableNotifications = true | ||
| notificationManager.requestPermission() | ||
| } else { | ||
| if newStatus != .authorized && settings.enableNotifications { |
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.
How is this change related to the rest of this PR? I fear that this change will break something. Can we split this up so it is easier to test/review?
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.
It's not directly related the PR is mostly multiple bug fixes, we could separate it if you prefer
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.
yes please, move at least this change out. I'm fairly certain this is there for a reason, and the whole notification registration flow has a lot of edge cases that need testing
Bitkit/Managers/ScannerManager.swift
Outdated
| if let wallet { | ||
| _ = await wallet.waitForNodeToRun() | ||
| try? await Task.sleep(nanoseconds: Self.nodeReadyDelayNanoseconds) | ||
| } | ||
|
|
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.
We don't need to wait for the node to parse scanning data. I have a working branch that moves some logic around to handle this. Can we move this fix out of the PR as well?
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.
Reverted the 2 commits mentioned
Code reviewFound 1 issue: Race Condition in
|
| // Guard against concurrent setup calls | |
| guard !isSettingUp && node == nil else { | |
| Logger.debug("Node already setting up or already set up, skipping") | |
| return | |
| } | |
| isSettingUp = true | |
| defer { isSettingUp = false } |
Two concurrent calls to setup() could both pass the guard check (both seeing isSettingUp == false) before either sets it to true, defeating the purpose of the guard.
While StateLocker.lock() at line 52 would prevent both setups from running simultaneously, this happens after the race window, meaning:
- The second call will unnecessarily wait 30 seconds before timing out
- The
isSettingUpflag's purpose (quick early return) is completely defeated
Suggested fixes:
- Make
LightningServicean actor, OR - Mark
isSettingUpas@MainActorisolated and ensure all calls tosetup()happen on the main actor, OR - Use a proper synchronization primitive like
NSLockor an atomic operation
Prevent node from stopping immediately when going to background and fix app getting stuck on app status
Fix scanning of address to send while node is initializing
Fix turning off payment notifications from settings is not saved