-
Notifications
You must be signed in to change notification settings - Fork 279
Background Sync via View-Key #934
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
Conversation
thanks! @anhdres check this out please |
@J0J0XMR can you please add a commit which removes all the demo code (prefs, toggles, lockscreen) only using view-only mode for syncing. ad point 3: monerujo runs as a foreground service and so does not get killed by the system - this may change in the future due to android constraints |
@m2049r "Only using view-only mode for sync" - that's what this PR does already, essentially letting the user turn their wallet into a view-only wallet when syncing (by wiping the spend-key) at the push of a button. This PR has no benefit to view-only wallets, since there's no spend-key to wipe. |
Does this pull request (combined with the one made to @m2049r 's monero core fork add background sync via view keys as oer the requirement of this bounty @m2049r https://bounties.monero.social/posts/10/5-100m-implement-background-wallet-sync-via-view-keys-on-monerujo |
just built from source and can confirm that:
when testing for ANONERO we also left spend UI open at first to see if itll fail when spending and j0j0s PR shows the same behavior seeing as i worked with jberman to make sure our implementation was correct, and this PR is based on that, LGTM |
Do any buttons introduced here have labels for screen readers? That way they dont just read as "unlabeled"? |
The buttons were added to demo the new functions - @m2049r will decide if they are needed in the final UX. |
The wallet can't definitively determine the user's balance when background syncing since without the spend key, it can't identify all new spends. Because of that, I expected the feature to be used when the user isn't actively using the wallet. I don't think users should be able to see their txs or their balance while background syncing, otherwise they could get pretty confused at unexpected values (exposing newbs to the view-only wallet quirks seems undesirable). Here's what I made background syncing look like when the user explicitly locks their GUI wallet (or goes inactive and their GUI locks) as an example: Here's what I would suggest: use Note: the above suggested flow doesn't change how Monerujo keeps a foreground service open so the wallet stays syncing. One issue to keep in mind is that if the foreground service is killed, then the syncing stops and the user has to reopen their wallet in order to continue syncing again. This would/should be avoidable by going with Option 2 which I discussed in monero-project/monero#8619 (comment), which as mentioned there, would grant the OS perpetual access to the user's view key (read the "How it works" and "Mobile wallets" sections). |
This PR designed to implement background wallet sync via view keys on Monerujo.
To enable background syncing, tap the 3-dot menu, select Background Sync, and tap the Enable Background Sync toggle.
In order to demonstrate the feature most effectively, I created a simple lockscreen triggered by the new lock icon in the top right of the main screen. I have purposefully left the UX as simple as possible so that the Monerjo team can package it as they see fit.
With background sync enabled, the lock icon acts as the trigger: locking the wallet enables background sync, and unlocking disables it.
I also have left the Receive/Give buttons intact on the lockscreen to demonstrate that once background sync is enabled, the wallet's spend key is wiped from memory, which will lead to failure when trying to construct a transaction. These buttons can easily be removed in the final UX if Monerujo team decides to keep the lockscreen in place.
Street Mode is unaffected when background sync is enabled.
My personal feedback:
The on/off toggle and menu option seems unnecessary, as view-key syncing should just be the default/only way to sync.
I've left out "Sync only over WiFi" option mentioned in the bounty, as its already an option available in Android Settings (Monerujo > App Info > Mobile data & Wi-Fi > Background data). Monerujo already syncs in the background regardless of what connection is used, and since this PR doesn't affect the amount of data being synced, I kept the default behaviour. If Monerujo devs want to add an in-app WiFi only option, it should be a global setting, not specific to this feature.
Monerujo should add a prompt to set the app to "Unoptimized" in battery settings, to avoid Android killing the app while its in the background.
Donations:
866SAS9afc7GMQAQLz5KbyCBqP38K1MUb4jeGCwCnqALFz4pcXKGfYFAaP8txom4JjPxzEUsinVQERd7tsRTgNKXSovyQrg
APK Download: https://we.tl/t-6eE0UCbsQZ
APK SHA256: 7783ae19ac2993d292c18758d3b6d9e58096245fde917646332d2c306ae18257
Time lapse video of the feature:
monerujo.mp4