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

feat: new ldk backup server restore #1294

Merged
merged 8 commits into from
Oct 10, 2023
Merged

feat: new ldk backup server restore #1294

merged 8 commits into from
Oct 10, 2023

Conversation

Jasonvdb
Copy link
Collaborator

@Jasonvdb Jasonvdb commented Oct 6, 2023

Description

  • Enables backing up and restoring from new backup server.
  • Reverts back to deprecated backup server if no backup found on new server

Linked Issues/Tasks

Does not resolve existing restore issues it should prevent further ones from occurring.
#713
#770

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (improving code without creating new functionality)

Tests

  • Detox test
  • Unit test
  • No test

QA Notes

Backup and restoring lightning funds should be reliable now

@Jasonvdb Jasonvdb marked this pull request as ready for review October 6, 2023 14:50
@limpbrains
Copy link
Collaborator

should performRemoteLdkBackup be removed?

Also list in Settings -> Backup and Restore screen doesn't represent actual state now. We need a way to get backup info from ldk to update it

Comment on lines +234 to +236
if (fileListRes.isErr()) {
return err(fileListRes.error);
}
Copy link
Collaborator

@limpbrains limpbrains Oct 9, 2023

Choose a reason for hiding this comment

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

this throws "Error: Request failed. Missing response from backup server."
I believe we should return { backupExists: false } in this case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Might be a server issue here, if there is no backup then the list is just empty. I don't want to assume an error means there's no backup. I'll get back to you on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm testing with server from development.template. I've created a wallet using master branch, then I'm trying to restore in using new-ldk-backups and I'm getting this error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the env with new backup server. SSL was required as well as a backend update.

@Jasonvdb
Copy link
Collaborator Author

Jasonvdb commented Oct 9, 2023

should performRemoteLdkBackup be removed?

I was thinking let me remove it right after we've done a few rounds of live testing. Just in case.

Also list in Settings -> Backup and Restore screen doesn't represent actual state now. We need a way to get backup info from ldk to update it

True. Now we don't really have a last backup time to display. I do have a self check function for the backups maybe we can run this when this view is loaded to show the state.

@Jasonvdb Jasonvdb merged commit e8fe5a4 into master Oct 10, 2023
@Jasonvdb Jasonvdb deleted the new-ldk-backups branch October 10, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants