-
Notifications
You must be signed in to change notification settings - Fork 53
Fix syncing races between periodic and change-server syncing #670
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 fixes the race between periodic syncing and change-server subscriptions by introducing a `usesChangeServer` flag on `EdgeCurrencyInfo` which tells the core to not invoke periodic syncing. In addition, we introduce a `resubscribing` state to the subscription status to differentiate initial `subscription` with intermittent subscription issues from the change server.
This is indeed true, however this is because most wallets will have a single subscription |
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.
Bug: Wallet Status Overwritten by Last Subscription
The subscription status update logic incorrectly applies a single determined status to all subscriptions within a wallet, rather than updating each individually. This bug was introduced by removing a filter that previously limited updates to 'subscribing' statuses. Now, the status
derived from a single subscription's outcome is mapped to all subscriptions in the wallet, overwriting correct statuses (e.g., 'listening') with potentially incorrect ones (e.g., 'avoiding', 'resubscribing'). If a wallet has multiple subscriptions, the final status for all is determined by the last processed subscription's result.
src/core/currency/currency-pixie.ts#L302-L311
edge-core-js/src/core/currency/currency-pixie.ts
Lines 302 to 311 in e39f343
// Update the status for the subscription: | |
const subscriptions = wallet.changeServiceSubscriptions.map( | |
subscription => ({ | |
...subscription, | |
status | |
}) | |
) | |
subscriptionUpdates.set(walletId, subscriptions) | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
This fixes the race between periodic syncing and change-server subscriptions by introducing a
usesChangeServer
flag onEdgeCurrencyInfo
which tells the core to not invoke periodic syncing.In addition, we introduce a
resubscribing
state to the subscription status to differentiate initialsubscription
with intermittent subscription issues from the change server.CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none