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

CW-909-monero-background-sync-investigation #2041

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

Conversation

MrCyjaneK
Copy link
Collaborator

Description

  • Improve error handling and status checks during background sync
  • Modify zano init method to not conflict with default init()
  • Add dev tools for background sync monitoring and manual rescan

Pull Request - Checklist

  • Initial Manual Tests Passed
  • Double check modified code and verify it with the feature/task requirements
  • Format code
  • Look for code duplication
  • Clear naming for variables and methods

fossephate and others added 30 commits September 4, 2024 15:50
@MrCyjaneK MrCyjaneK changed the base branch from mweb-bg-sync-3 to main February 21, 2025 08:06
@MrCyjaneK MrCyjaneK force-pushed the CW-909-monero-background-sync-investigation branch from ce81a26 to fddbe7c Compare February 21, 2025 08:09
…let types

- Improve error handling and status checks during background sync
- Modify zano init method to not conflict with default init()
- Add dev tools for background sync monitoring and manual rescan
[DNM] Enable debug screen on release to test
Copy link
Contributor

@OmarHatem28 OmarHatem28 left a comment

Choose a reason for hiding this comment

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

@fossephate please continue on this branch and fix the comments here
also, please make the notifications and its settings only in debug mode
and make the whole background tile in the settings only show up on Android

Comment on lines +127 to +130
if (monero.Wallet_numSubaddressAccounts(wptr!) <= accountIndex) {
printV("accountIndex is out of bounds");
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why? wouldn't it create the account normally and then create the subaddress?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In background sync it is numSubaddressAccount is always zero.

Comment on lines +269 to +274
final firstWallet = litecoinWallets.first;
final wallet = await walletLoadingService.load(firstWallet.type, firstWallet.name);
await wallet.stopSync();
if (bitcoin!.getMwebEnabled(wallet)) {
syncingWallets.add(wallet);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what if the first wallet doesn't have mweb enabled,
loop through them until you find one that has mweb enabled then add it and break

Copy link
Contributor

Choose a reason for hiding this comment

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

ah right I forgot we changed mweb to be a per-wallet setting

Comment on lines +288 to +292
var node = settingsStore.getCurrentNode(WalletType.bitcoin);
await wallet.connectToNode(node: node);

bool nodeSupportsSP = await (wallet as ElectrumWallet).getNodeSupportsSilentPayments();
if (!nodeSupportsSP) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we first check on whether silent payment scanning is enabled or not

@@ -20,6 +20,7 @@ import workmanager
}

WorkmanagerPlugin.registerTask(withIdentifier: "com.fotolockr.cakewallet.monero_sync_task")
WorkmanagerPlugin.registerTask(withIdentifier: "com.fotolockr.cakewallet.mweb_sync_task")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a need for this, the feature is only android and this is not used

Comment on lines +24 to +25
// TODO: re-enable (need to figure out how to prevent current wallet from being loaded in the background service!)
// STILL TODO:!
Copy link
Contributor

Choose a reason for hiding this comment

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

still todo?!
I see you already added the loadWallet: false, but just need to make sure it's the only way the current wallet is loaded

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

Successfully merging this pull request may close these issues.

4 participants