Skip to content
This repository was archived by the owner on Sep 12, 2023. It is now read-only.

Allow exporting seed in taker UI #2988

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Allow exporting seed in taker UI #2988

merged 1 commit into from
Sep 27, 2022

Conversation

klochowicz
Copy link
Contributor

@klochowicz klochowicz commented Sep 20, 2022

TODO:

@klochowicz klochowicz marked this pull request as draft September 20, 2022 05:28
@klochowicz klochowicz self-assigned this Sep 20, 2022
@klochowicz klochowicz linked an issue Sep 20, 2022 that may be closed by this pull request
@klochowicz klochowicz force-pushed the allow-exporting-seed branch 3 times, most recently from e292047 to a2c5907 Compare September 20, 2022 06:08
@holzeis holzeis force-pushed the allow-exporting-seed branch from a2c5907 to cd302a2 Compare September 26, 2022 08:03
@holzeis
Copy link
Contributor

holzeis commented Sep 26, 2022

Added a toast about the successful outcome. file-saver will not produce any errors as it is based on the native html 5 download href, hence no error message in case of no success.

@holzeis
Copy link
Contributor

holzeis commented Sep 26, 2022

Opted against adding regression tests, as there is not much logic to test anyway.

@holzeis holzeis force-pushed the allow-exporting-seed branch from 18c7a33 to 3e6c019 Compare September 26, 2022 12:33
@holzeis holzeis marked this pull request as ready for review September 26, 2022 12:34
@holzeis holzeis force-pushed the allow-exporting-seed branch 2 times, most recently from cdb18bf to 324b4d4 Compare September 26, 2022 12:38
Copy link
Collaborator

@bonomat bonomat left a comment

Choose a reason for hiding this comment

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

Haven't tested it but looks good to me. Can you squash the commits please?

@bonomat bonomat requested a review from luckysori September 26, 2022 21:57
@klochowicz
Copy link
Contributor Author

klochowicz commented Sep 27, 2022

I'm not sure whether this should go in as-is (hence the draft status), especially without prior to testing this. I have kept it as draft as I noticed on Friday that the saved file is not the same binary size as the original one; and worked on a test that would ensure we backup the right thing.

I suspect RocketResponderPro is adding headers / screwing up encoding - might be easier to go with serde and then deserialise into a blob to ensure the file is as it's supposed to be.

The other way (perhaps simpler altogether) that I came up with during the weekend (but haven't tried) would be just serving taker_seed as a NamedFile in Rocket. Of course then we can't backup from --app-seed, but that's not a requirement.

@klochowicz
Copy link
Contributor Author

klochowicz commented Sep 27, 2022

Opted against adding regression tests, as there is not much logic to test anyway.

As this is one of the paths where the user might lose money if it doesn't work properly, I would certainly test this to prove that it works.
However, manual testing should be enough for this PR, don't worry about it here - I already started working on the test, so I will submit it separately for review 😊

@holzeis
Copy link
Contributor

holzeis commented Sep 27, 2022

I'm not sure whether this should go in as-is (hence the draft status), especially without prior to testing this. I have kept it as draft as I noticed on Friday that the saved file is not the same binary size as the original one; and worked on a test that would ensure we backup the right thing.

The downloaded seed file works properly.. I've tested it manually.

  1. startup itchysats
  2. backup seed
  3. stop itchysats
  4. replace seed file with downloaded one
  5. startup itchysats

Everything is working ;)

Opted against adding regression tests, as there is not much logic to test anyway.

As this is one of the paths where the user might lose money if it doesn't work properly, I would certainly test this to prove that it works.

I think the regressions tests are better covered including the restore feature.

The other way (perhaps simpler altogether) that I came up with during the weekend (but haven't tried) would be just serving taker_seed as a NamedFile in Rocket. Of course then we can't backup from --app-seed, but that's not a requirement.

I guess that would also work, for the app-seed argument I was thinking if we could store that also to a taker_seed file if we get it, then we have the same behavior. wdyt?

@holzeis holzeis force-pushed the allow-exporting-seed branch from 324b4d4 to 5f24afa Compare September 27, 2022 15:59
@holzeis
Copy link
Contributor

holzeis commented Sep 27, 2022

bors r+

@bors bors bot merged commit 778b956 into master Sep 27, 2022
@bors bors bot deleted the allow-exporting-seed branch September 27, 2022 18:36
@klochowicz
Copy link
Contributor Author

The downloaded seed file works properly.. I've tested it manually.

  1. startup itchysats
  2. backup seed
  3. stop itchysats
  4. replace seed file with downloaded one
  5. startup itchysats

Everything is working ;)

awesome! 🤘

@klochowicz
Copy link
Contributor Author

I guess that would also work, for the app-seed argument I was thinking if we could store that also to a taker_seed file if we get it, then we have the same behavior. wdyt?

that would also work, yes! however it might be a too niche feature to even bother. :-)

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

Successfully merging this pull request may close these issues.

Make it easier to back up the seed-file
3 participants