Skip to content

Add Bitcoin Wallet to SegWit table. #257

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

Merged
merged 2 commits into from
Dec 18, 2019

Conversation

schildbach
Copy link
Contributor

The source code can be reviewed at
https://github.com/bitcoin-wallet/bitcoin-wallet

@schildbach
Copy link
Contributor Author

The errors don't seem to be related to my PR.

@jnewbery jnewbery added the compatibility Enhancements or bugs for the compatibility matrix label Nov 6, 2019
@jnewbery jnewbery requested a review from bitschmidty November 6, 2019 15:10
@harding
Copy link
Collaborator

harding commented Nov 6, 2019

The errors seem to be because your data file doesn't contain the RBF section.

@bitschmidty did we want to require every wallet have data in every section, or did we want to allow wallets to be part of (e.g.) the segwit section but not the RBF section? In either case, we can update our code---for the former case, I can ensure we provide a more descriptive failure message; for the later, I can stick some if statements in there to skip attempting to generate empty entries.

@schildbach
Copy link
Contributor Author

Just for clarification: we (the Bitcoin Wallet devs) favor CPFP over RBF, so it doesn't really make sense to add a row in the RBF table.

@bitschmidty
Copy link
Contributor

@schildbach Thanks for your contribution, much appreciated!

A couple things here:

  1. Contributing instructions were not clear in outlining that you need markdown file for each wallet as well. I have pushed a commit to your PR here (hope that is ok!) with that file. Additionally I have opened a PR to clarify that requirement.
  2. The template for listing all wallets didn’t have a check for missing RBF data. (there was such a check for segwit). I have opened a PR to add this conditional.
  3. The template for the wallet details page was missing a similar RBF data check (while also having the segwit data check). The PR mentioned above also resolves this.

Once the PR for the data checks is merged, we can look at merging this PR.

One thing you might consider adding, but is completely optional, are screenshots showing any applicable screenshots regarding segwit support. If you’re curious you can see other wallet pages for example screenshots.

Additionally, even though Bitcoin Wallet does not support RBF, that is still useful information to have in the matrix for people to be aware of. You could consider adding the RBF data to the compatibility file even if it rounds to "we dont support it".

Copy link
Contributor

@bitschmidty bitschmidty left a comment

Choose a reason for hiding this comment

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

Rebased on master which resolved the original issues around not having RBF data in the data file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Enhancements or bugs for the compatibility matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants