-
Notifications
You must be signed in to change notification settings - Fork 129
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
Replace compatibility with new matrix #1871
base: master
Are you sure you want to change the base?
Conversation
560ad11
to
9bb645b
Compare
@@ -95,24 +95,18 @@ JEKYLL_ENV=local make build # Do a clean build of the site to the _site directo | |||
diff -ruN _site _site.bak # Compare the generated sites (-r for recursive, -u for unified, -N for new file) | |||
``` | |||
|
|||
## Compatibility Matrix Data | |||
## Bitcoin Feature Matrix Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TorokNRoll do you want to include the new process for user-submitted contributions for the matrix here?
b8c3461
to
04de591
Compare
8a08093
to
0696025
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, this is looking pretty good.
I had a few specific comments on items and fields that I left in the commits, but I was curious about a few more general things.
-
How do we deal with partial submissions? E.g. someone wants to contribute the bech32 support column and default receive type, but they don’t know about the software’s RBF behavior. Do we have a way of recording that we don’t have data for a field? Or do we just leave that field empty?
I would much prefer collecting "I’m not sure" for a field, instead of an incorrect value. -
I think it’s a good idea to get rid of the screenshots, that was way too much work to maintain, but how do reviewers check whether a submission is accurate?
-
How do we track how recent information is? Is our expectation that all entries in the table are always up-to-date or should we track when a wallet’s information was last updated? If so, would there be a way for us to track that the information might have only been verified partially?
--- | ||
name: "Bitkit" | ||
category: "Software Wallet (L1&L2)" | ||
keyfeatures: "Your ultimate Bitcoin toolkit. Self-custodial BTC and LN payments. Widgets, pay-to-contacts and more" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds more like the project’s self-description. How come the field name is "keyfeatures"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the Feature Matrix is only focused on a given set of features, I added the column Key Features / Use Cases. The objective here was to allow for product teams to highlight things which might differentiate their product, especially if any of their key features are not in the (current) scope of the matrix.
_data/matrix/Phoenix.yaml
Outdated
default_receive: "P2TR via swap-in-potentiam" | ||
bech32: "Send Support" | ||
bech32m: "Full Support (send & receive)" | ||
segwit_change: "No" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phoenix having "No Support" for native segwit change when they only use P2TR outputs for receiving surprises me. Is this meant in the context of splicing out to pay someone? Should this field then perhaps be "Not Applicable"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will confirm this with the Phoenix team.
As a reminder, all of the entires at this point were submitted by the respective product teams (outside Bitcoin Core Wallet).
_data/matrix/Envoy.yaml
Outdated
device: "Mobile" | ||
os: "Android, iOS" | ||
web: "No" | ||
hw_interface: "No" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn’t Envoy the software wallet that goes with the Passport? If so, how can it not have a hardware wallet interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will confirm with Envoy.
Likely a misunderstanding somewhere along the line.
_data/matrix/Passport.yaml
Outdated
device: "" | ||
os: "" | ||
web: "Not Applicable" | ||
hw_interface: "No" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this perhaps be "Not Applicable"? A hardware signing device wouldn’t be expected to have a HWI, would it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolute novice talking my way through here: If HWI is technically a "_Python library and command-line tool used to interface with hardware wallets_", then I would expect HWI to be "Not Applicable" for Signing Devices (as you suggest).
Would you agree, is that a good way to put this point to rest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. I probably misunderstood that column. I thought hw_interface
was generally referring to whether a software wallet can interact with a hardware signer, but now I get the sense that it specifically refers to whether something is compatible with the HWI library.
If that is the intended meaning, it might make sense to call the YAML field just hwi
, at least that threw me off. Perhaps the tooltip for that column could also be changed in that regard, I’ll suggest something there.
|
||
feature: | ||
device: "Mobile" | ||
os: "Android, iOS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the past we had the issue that e.g. Phoenix had some features that were only live on Android but not on iOS or vice versa. How could we handle that situation in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed a bit of a curveball, I will need to mull on this.
{% elsif include.state == "No Support" %} | ||
{% assign cell_emoji = "❌" %} <!--- No Support ---> | ||
{% assign cell_tooltip = "No Support" %} <!--- No Support ---> | ||
{% elsif include.state == "Not Applicable" %} | ||
{% assign cell_emoji = "➖" %} <!--- Not Applicable ---> | ||
{% assign cell_tooltip = "Not Applicable" %} <!--- Not Applicable ---> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide some guidance when to use "Not Applicable" vs "No Support"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will refine this, as mentioned above.
@@ -115,6 +115,6 @@ wallets and services.* | |||
[whatsat]: https://github.com/joostjager/whatsat | |||
[news72 sphinx]: /en/newsletters/2019/11/13/#possible-privacy-leak-in-the-ln-onion-format | |||
[river twitter thread]: https://twitter.com/philipglazman/status/1216849483184476165 | |||
[wasabi rbf notification]: /en/compatibility/wasabi/#receive-notification | |||
[wasabi rbf notification]: /en/compatibility/wasabi/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[wasabi rbf notification]: /en/compatibility/wasabi/ | |
[wasabi rbf notification]: /en/matrix/ |
@murchandamus - thanks for the feedback. Regarding your 3 general points:
As an additional note: all of the submissions in this commit were actually provided directly by the product owners. I will continue with your other comments on various files tomorrow..... |
Okay that sounds good. I hope that works out, but I think our experience in the past was that we were doing a lot of the submissions ourselves. After something is submitted, how would we get an update on just one field, e.g. after a service adds support for something?
Sure that should mostly work.
Yeah, I was thinking about whether it would be useful or necessary to track the last update per field, but generally, we might want to surface the date of the last update to a service. It would however be possible that a field would have been updated recently, but there are still other outdated fields on the entry.
Well, one problem with that might be that some of the product owners are not that technical and do not respond accurately, see e.g. the contradiction in the Casa data. |
@murchandamus - feedback to your latest comments: 1. Partial Submissions - "After something is submitted, how would we get an update on just one field, e.g. after a service adds support for something?" Updating just one field will also leverage the google forms / sheets. Each entry can be reaccessed via a unique link. The "Updater", would just need to access the link, change the specific field then resubmit. As a heads up, from there, a script is run with generates the updated YAML, which is then dropped in the associated site folder. here is an example of a unique form link to a test record: 2. Verifying submission accuracy w/o screenshots - "Well, one problem with that might be that some of the product owners are not that technical and do not respond accurately, see e.g. the contradiction in the Casa data." The general best case scenario is that representatives from the "product team" proactively manage their records, very simply via the google form. The google form should remove the friction for anyone to submit small updates, either from the "product team" (technical or not), bitcoin optech team, or just generally speaking. Regarding the Casa contradiction (I still need to review your feedback), once it is clarified, updating the matrix could technically be done in just a few minutes. 3. Tracking Information Freshness - regarding tracking record freshness or field freshness, of course both could be technically possible. My gut says the tracking on the record level should be sufficient (at least to start with), especially when I consider (1) the extra effort required to actually to set that up (2) and the increased complexity it will add to the UI. Just just adding an extra column "last update" could be done without too much effort. |
Ah that’s really nifty.
Don’t worry too much about it, if it is a lot of work. I’m not sure how much value add it would translate to. Also, we can already see in the git log when something was last changed. |
Once v1 of the new matrix is released, I will have the "last update" column at the top of my backlog. I should be able to do that without too much effort. The value would be for casual viewers who would not be accessing git logs, etc. I reviewed your detailed comments on the code above (which I will reply to now). I would summarise most of your feedback is related to the need to refine the No Support / Not Applicable concept (as you detected inconsistencies, related to that). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went over the column headers in the table again and added a few more suggestions. These are not urgent or blocking for a merge, but I anticipate that our readers would also bring up similar questions regarding what criteria specifically are tracked in the columns.
en/matrix.md
Outdated
<th rowspan="2">Category</th> | ||
<th rowspan="2">Key <br/> Features / <br/>Use Cases<br/><font size="1"><i class="italic">(hover)</i></font></th> | ||
<th class="col-platform" style="display: none;color:blue;" colspan="3">Platform</th> | ||
<th class="col-hwinterface" style="display: none;" rowspan="2"><a href="https://bitcoinops.org/en/topics/hwi/">HWI</a></th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
<th class="col-hwinterface" style="display: none;" rowspan="2"><a href="https://bitcoinops.org/en/topics/hwi/">HWI</a></th> | |
<th class="col-hwinterface" style="display: none;" rowspan="2">Implements hardware signer support via the <a href="https://bitcoinops.org/en/topics/hwi/">HWI</a> library.</th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! thanks for the suggestion.
en/matrix.md
Outdated
<a href="https://bitcoinops.org/en/topics/bech32/">Bech32m <br/><font color="grey">P2TR</font></a><span class="tooltip">BIP350</span> | ||
</div></th> | ||
|
||
<th style="vertical-align:top;">Native <br/>Segwit<br/> Change</th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be more concrete, P2WPKH, P2WSH, and P2TR all belong to the category of native segwit outputs, but sometimes people use "native segwit" only to refer to P2WPKH and P2WSH while they call P2TR a "taproot output". Sometimes people even think that only P2WPKH is called "native segwit". Therefore, it would be helpful to clarify what the content of this column refers to specifically.
en/matrix.md
Outdated
<th class="col-feebumping" style="display: none;"><div class="tooltip-container"> | ||
<a href="https://bitcoinops.org/en/topics/replace-by-fee/">Full RBF<br><font color="grey">Bump<br>Fee</font></a><span class="tooltip">full-RBF</span> | ||
</div></th> | ||
|
||
<th class="col-feebumping" style="display: none;"><div class="tooltip-container"> | ||
<a href="https://bitcoinops.org/en/topics/replace-by-fee/">Full RBF<br><font color="grey">Cancel<br>TXN</font></a><span class="tooltip">full-RBF</span> | ||
</div></th> | ||
|
||
<th class="col-feebumping" style="display: none;"><div class="tooltip-container"> | ||
<a href="https://bitcoinops.org/en/topics/replace-by-fee/">Full RBF<br><font color="grey">Notification</font></a><span class="tooltip">full-RBF</span> | ||
</div></th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these three columns it would also be helpful to learn what they exactly refer to.
I assume it is:
- Bump Fee: "Can use RBF to increase the feerate of a previously submitted transaction"
- Cancel TXN: "Can use RBF to change the outcome of previously submitted transactions, e.g. to cancel a payment"
- Notification: "Tracks when incoming transactions get replaced and notifies the user"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. As you remember, there is a google form which serves as intake for the entries. On that form, I do have questions similar to what you are proposing. It totally makes sense to have the same questions (via tooltip) on the resulting matrix.
en/matrix.md
Outdated
<a href="https://bitcoinops.org/en/topics/replace-by-fee/">Full RBF<br><font color="grey">Notification</font></a><span class="tooltip">full-RBF</span> | ||
</div></th> | ||
|
||
<th class="col-feebumping" style="display: none;"><a href="https://bitcoinops.org/en/topics/cpfp/">CPFP</a></th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, I am curious what criteria gives a wallet a check here:
<th class="col-feebumping" style="display: none;"><a href="https://bitcoinops.org/en/topics/cpfp/">CPFP</a></th> | |
<th class="col-feebumping" style="display: none;"><a href="https://bitcoinops.org/en/topics/cpfp/">CPFP</a><span class="tooltip">Can create child-pays-for-parent transaction to bump an [incoming transaction|outgoing transaction|any transaction that pays the wallet|at least one of the above]</span></th> |
Hey Steven, I chatted with a buddy from BitGo, he was open to having someone work on the form for the Matrix. Would that be something that we are interested in or should I wait until this is live? |
Hey Murch (@murchandamus)....apologies for the slow response, a massive time sinkhole had recently appeared in my life! I am back in the saddle now..... Regarding BitGo, I prefer to wait until we go live. With that, I will now start to work through your open comments.....which are the final steps before going live.... Once we go live, I would be grateful to have you get BitGo onboard asap! Rolling updates at that point should (theoretically) be a snap! Thanks |
Todo:
Add bluewallet data which appears to be missing@TorokNRoll