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

ADD: Border Wallets #5690

Open
wants to merge 51 commits into
base: master
Choose a base branch
from
Open

ADD: Border Wallets #5690

wants to merge 51 commits into from

Conversation

3bitcoins
Copy link
Contributor

@3bitcoins 3bitcoins commented Sep 17, 2023

Replaces #5669

This is an independently developed implementation of Border Wallets which brings similar features, but solves numerous key issues with the initial PR:

  • No codebase overhaul (This implementation does not change any existing features, compared to 5,521 LOC changed in the other implementation. Additionally, it does not implement any new wallet types, as all features can be done without doing so.)
  • Decreased LOC in general: 11,070 additions -> 1,362 additions
  • Improved UX (More straightforward, and explains to the user the process of creating a border wallet. There are no screens with multiple ambiguous buttons, and you are not expected to know how to use a border wallet beforehand.)
  • Less dependencies (Only 1 new dependency, for pdfs, compared to 10?? in the other implementation)
  • No unnecessary files or changes

@marcosrdz
Copy link
Member

Conflicts

@3bitcoins
Copy link
Contributor Author

I'll fix this when #5722 is merged, because it reuses that logic

@3bitcoins
Copy link
Contributor Author

@limpbrains @Overtorment This is now ready for review

@3bitcoins
Copy link
Contributor Author

@Overtorment is there any chance you could review this?

Copy link
Member

@Overtorment Overtorment left a comment

Choose a reason for hiding this comment

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

i reviewed it a bit, not deeply.
first, of all, i dont really like the feature, and i dont think it will do any good to anyone, ever.
but, we can consider adding it to "tools" section, so it wont occupy precious real-estate on already UI-heavy add/import screens.

also, some issues:

  • extra dep. i think allowing backups in pure html is good enough, no need for a pdf conversion. luckily we already have everyting to create html, save it fo file (via fs) and export for user
  • zero tests in this pr. at the very least id like to know that our implementation is compatible with the reference one via shared test vectors
  • new components should be in components/

@3bitcoins
Copy link
Contributor Author

@Overtorment Hey, thanks for your review

i reviewed it a bit, not deeply. first, of all, i dont really like the feature, and i dont think it will do any good to anyone, ever. but, we can consider adding it to "tools" section, so it wont occupy precious real-estate on already UI-heavy add/import screens.

Ever since I discovered it i've been using it to backup my seeds, and it's been extremely useful at least to me, anecdotally

  • extra dep. i think allowing backups in pure html is good enough, no need for a pdf conversion. luckily we already have everyting to create html, save it fo file (via fs) and export for user

the PDF feature is in consistent in the reference implementation as well as Sparrow Wallet's implementation; it's for compatibility

  • zero tests in this pr. at the very least id like to know that our implementation is compatible with the reference one via shared test vectors
  • new components should be in components/

I will resolve these issues if you get back to me on whether or not the PDF export is a hard no or not

@Overtorment
Copy link
Member

if we can get rid of pdf dependency we can add it to tools section, but this thing wont help people cross real dangerous borders (you know, the ones where guards beat the shit out of you if they find anything resembling crypto backups on you)

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.

None yet

3 participants