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

WIP: Coin Selection Page #90

Merged
merged 41 commits into from May 28, 2021

Conversation

barefoot-88
Copy link
Contributor

@barefoot-88 barefoot-88 commented Jan 4, 2021

type: page
title: coin-selection
permalink: TBD
file: TBD

Problem

Coin selection is an integral feature of privacy in sending bitcoin payments. During bitcoin transactions, we can unknowingly expose the private data of senders, receivers, and their various wallet balances along the way. Coin selection, whether an automated or manual process, will most likely result in the breach of privacy for both you as well as your previous senders of bitcoin. However, there are various ways of mitigating how much data is exposed during each transaction. We can reduce this security risk in a few different ways, however, we are left with a design challenge.

Solution

To become aware of the trade-offs when using different coin selection strategies (manual vs automatic), as well as choosing what to optimise for (speed, cost, privacy).

Assumptions about the reader

  1. Knows what a UTXO is...
  2. Knows about contact creation and UTXO labelling

Result

@johnsBeharry johnsBeharry linked an issue Jan 4, 2021 that may be closed by this pull request
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
@johnsBeharry johnsBeharry added this to 🛠 In Progress in Payments Jan 11, 2021
Copy link
Contributor

@pavlenex pavlenex left a comment

Choose a reason for hiding this comment

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

Great work.

I reviewed your PR CoinSelection page up until Automatic Coin Selection will continue tomorrow.

General feedback is that your sentences are quite lengthy sometimes. I suggest you try to keep concise and short sentences when explaining complex terms, as it's easy to get lost to in what you were trying to tell us. I mentioned wordiness as a complaint, that usually refers to the sentence structure and means it's very easy to get lost on the point.

I also notices quite a lot of inconsistencies for the perspective. You start with I, then use we and in the end user, it would be good to be consistent here and use third-person writing.

I suggest that instead of using tons of names, you illustrate what you wanted to tell. You go quite in-depth when making an example, which can be easily presented with a graphic, text is not essential, designers are visual people 😄

One more suggestion is to look at the Private key management section. We can all learn from there on how to structure our chapters. Daniel there utalized nice structure

Best practice
Do's
Dont's
When to use
When not to use

I think you can bullet list a lots of the content. Don't forget designers are visual people. We don't need to explain, we can show.

Don't let my review discourage you in any way. Take it as a suggestion to further improve what already is a solid base. 🚀

guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/UTXOs.md Outdated Show resolved Hide resolved
guide/payments/UTXOs.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
@GBKS
Copy link
Contributor

GBKS commented Jan 18, 2021

Whenever you're getting ready for review, could you please review the illustration guidelines and adjust your images accordingly? It's a brand-new page as we're just getting those contribution guidelines going, please let me know if you have questions or feedback.

@pavlenex
Copy link
Contributor

pavlenex commented Feb 5, 2021

Can a maintainer re-run the tests for this one? The reason tests are failing I think is too many images being compressed. Locally it works but takes quite a bit of time.

@GBKS
Copy link
Contributor

GBKS commented Feb 5, 2021

@pavlenex yes, the submitted images have not been compressed. They are also way too big (pixel size), that's why it it takes so long and ends up timing out. My computer also takes forever.

Re-running tests will also fail. Better to:

  • Resize images
  • Delete duplicate images (e.g. funding-tx.png exists twice)
  • Delete unused images (e.g. coin-selection-header.png)
  • Compress images (run site locally to let the compression script run, then recommit)

Additionally, the following need to be done at some point before merging:

  • Rename images and image folders to lowercase and remove spaces (e.g. Hidden Balance.png)
  • Reorganize images to a single folder (currently spread across two folders)
  • Add retina images
  • Add mobile layout variations (most images are not legible on mobile)
  • Switch to picture include for proper markup

@pavlenex
Copy link
Contributor

pavlenex commented Feb 6, 2021

Thanks for giving the pointers @GBKS and @barefoot-88 for quickly addressing the issue. I'll proceed with reviewing in a bit now that tests are passing.

Copy link
Contributor

@pavlenex pavlenex left a comment

Choose a reason for hiding this comment

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

Reviewed up until line 166. Will continue tomorrow.

guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
@pavlenex
Copy link
Contributor

pavlenex commented May 27, 2021

@barefoot-88 Really awesome work. I enjoyed while reading through this one so far. I've done a few things:

  1. I've resolved the conflicts that this PR had
  2. I've merged the master branch into this one since it was a bit lacking behind
  3. I've reviewed the PR up to line 166, but I also batch committed my suggestions. I've done this to immediately see if the formatting changes make sense and get a better feeling on how content works together before I continue to review the rest.
  4. Optimized images and merged them back in.

guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
guide/payments/coin-selection.md Outdated Show resolved Hide resolved
@pavlenex pavlenex merged commit bcc4a15 into BitcoinDesign:master May 28, 2021
@pavlenex pavlenex mentioned this pull request May 28, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Payments
🛠 In Progress
Development

Successfully merging this pull request may close these issues.

Develop coin control section
4 participants