feat(backup)_: local user data backups #6722
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (154)
|
jrainville
commented
Jun 27, 2025
| if rowsAffected == 0 { | ||
| // If no rows were affected, it means the setting does not exist | ||
| return errors.New("settings not initalized, please call CreateSettings first") | ||
| } |
There was a problem hiding this comment.
"Bug" we found with @osmaczko . If the settings are not created at the start (of a test for example), then saveSetting still "works", ie doesn't send an error. It makes debugging the failing test very annoying.
| backup := &protobuf.Backup{} // TODO: fill me in more efficient way than waku backup | ||
|
|
||
| // TODO is using this for the clock ok? | ||
| settings, err := s.prepareSyncSettingsMessages(uint64(time.Now().UnixMilli()), true) |
There was a problem hiding this comment.
The old code used the clock from clock, chat := m.getLastClockWithRelatedChat().
I think using the time for Now should be ok, since we're exporting Now.
|
|
||
| for _, setting := range backup.Settings { | ||
| // TODO is it ok to use the messenger here? Otherwise, I have to duplicate a lot of code | ||
| err = s.messenger.HandleBackedUpSettings(setting) |
There was a problem hiding this comment.
I'm using the messenger's HandleBackedUpSettings here because I'd need to duplicate a lot of code.
In theory HandleBackedUpSettings I could just move, because once we remove the old backup code, only the local backup will use it.
However, HandleBackedUpSettings calls extractAndSaveSyncSetting, which is used fro Sync messages too (paired devices), so the Messenger needs it too.
Thankfully, extractAndSaveSyncSetting doesn't rely on anything Messenger related, so maybe we could extract it to a common place where we only have to pass the needed function to save the setting?
Finally, I'll also be lacking the messengerSignalsHandler. There is probably another way to do it?
There was a problem hiding this comment.
I'll create a new issue to modify the code to not use the messenger here.
I'll also address the duplication there
| } | ||
|
|
||
| // TODO this is a duplicate of the code in messenger_handler. Should it be moved to a common place? | ||
| func (s *Service) handleSyncWatchOnlyAccount(message *protobuf.SyncAccount) (*accounts.Account, error) { |
There was a problem hiding this comment.
Similar to the Account Service, I had the issue of having to duplicate code. This Service doesn't have access to the Messenger, so I guess creating a share function would do the trick?
Let me know where and how is the best way to do it
There was a problem hiding this comment.
I'll create a separate issue to remove the duplication
|
I think it will be simpler to review the full final PR, though it will be a lot indeed. |
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
4 times, most recently
from
a86c42c to
6cc985b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6722 +/- ##
============================================
- Coverage 59.06% 28.79% -30.28%
============================================
Files 828 796 -32
Lines 101562 98874 -2688
============================================
- Hits 59991 28469 -31522
- Misses 33998 66047 +32049
+ Partials 7573 4358 -3215
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
from
6cc985b to
b54e6a5
Compare
jrainville
changed the title
|
@igor-sirotin @osmaczko ready for review. One PR to rule them all 😅 |
This was referenced Jul 9, 2025
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
from
b54e6a5 to
96899be
Compare
3 tasks
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
3 times, most recently
from
9d9322c to
5bacf9d
Compare
Fixes #18248 #18107 #18120 #18122 #18141 #18121 Implements all the backend for the local user data backups. Uses the scaffold @osmaczko created to make it more extendable. Instead of using the old Messenger code for backups, we now use a new controller that let's other modules register to be backed up. This includes the messenger for chats, contacts and communities, the accounts service for settings and the wallet service for watch-only accounts. I also created new Protobufs for those backups, so that we no longer user the super generic Waku Backup protobuf. Finally, I added some integration tests and a functional test that does the whole flow. A lot of cleanups can still be done to reduce duplication, but they can be done in another commit/issue, since the internal of the services can be modified without the backup process being affected, since the protobufs are in place already.
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
from
5bacf9d to
c16b32a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #18248 #18107 #18120 #18122 #18141 #18121
This is the big PR replacing all the smaller ones, since a lot of the early PRs are now semi-obsolete.
Implements all the backend for the local user data backups.
Uses the scaffold @osmaczko created to make it more extendable. Instead of using the old Messenger code for backups, we now use a new controller that let's other modules register to be backed up. This includes the messenger for chats, contacts and communities, the accounts service for settings and the wallet service for watch-only accounts.
I also created new Protobufs for those backups, so that we no longer user the super generic Waku Backup protobuf.
Finally, I added some integration tests and a functional test that does the whole flow.
A lot of cleanups can still be done to reduce duplication, but they can be done in another commit/issue, since the internal of the services can be modified without the backup process being affected, since the protobufs are in place already.