-
Notifications
You must be signed in to change notification settings - Fork 137
Compatibility data: opendime samourai trezor wasabi xapo #182
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
Compatibility data: opendime samourai trezor wasabi xapo #182
Conversation
6f1ccf1
to
bec042f
Compare
rebased |
bec042f
to
1e581a2
Compare
removed mycelium |
I'll review this for typos/formatting/etc this week. |
I was skimming this in order to maybe steal some of @bitschmidty's research and I wanted to suggest, for consideration, that Trezor might be better renamed as Wallet.Trezor.Com or something because the review is of the web frontend rather than the Trezor device. |
@harding Similar to the Ledger Live (vs Ledger), right? It seems that Trezor's wallet is called Trezor Wallet (https://wallet.trezor.io/#/) There is a similar consideration for Opendime. |
@bitschmidty yeah, Ledger Live vs Ledger too. For OpenDime, I only skimmed the listing here (I was looking for screenshots of Send screens) but it looked to me like you reviewed the hardware independent of a software wallet, so I think it's probably appropriately named. Bitcoin.org wallet maintainer @crwatkins has previously told me that he'd have preferred it if Bitcoin.org, instead of evaluating and listing hardware wallets by themselves, instead did them in pairs with compatible software wallets. E.g. instead of listing "Trezor" they'd list "Trezor+Trezor.com", "Trezor+Electrum", "Trezor+HWI+Core", etc... That way if the scoring said the wallet did X, the reader could be sure that it did X when used with the indicated software. |
At the risk of jumping in the middle of a PR that I haven't fully reviewed and carrying on a personal chat with @harding in the middle of it, I'll agree with what he said. As we know, most "hardware wallets" are really just key stores with signing engines (with the notable exception of the discontinued Case wallet, and the non-notable exceptions which are just software running on top of Android on some mobile-phone-like general purpose hardware). They are not fully functioning wallets by themselves. The issue at bitcoin.org currently is that most hardware cannot be reviewed as a wallet in isolation (perhaps as an engine, but not as a wallet) and we review "wallets". Another complication is that historically when a new "hardware wallet" comes out from a new vendor, their companion software is often fairly immature and does not pass recommended bitcoin.org wallet criteria, requiring the use of a third party app to meet all the criteria. |
I've pushed a commit that updates the image files. Ideally we want images that:
|
_data/compatibility/trezor.yaml
Outdated
shows_original_version: "false" | ||
send: | ||
signals_bip125: "false" | ||
list: "false" |
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 are these things testable (sent transaction labeled as RBF in summary and details screen), when the service doesn't support sending with RBF?
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.
You’re right. "Doesn’t flag RBF transactions (incoming)" doesn’t mean "Doesn’t flag RBF transactions outgoing".
I will make sure this is consistent across other data files as well.
Pushed a commit to address (most of) @jnewbery 's great feedback. |
Thanks @bitschmidty . Let me know when you've updated the captions (#183 (comment)) and I'll rereview. |
6336f33
to
3f951d6
Compare
|
Thanks Mike. Will rereview on Monday. |
c700ffe
to
8520996
Compare
default: "p2pkh" | ||
send: | ||
bech32: "true" | ||
change_bech32: "na" |
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 great @bitschmidty . I've pushed a branch that:
If you're happy with those changes, please go ahead and merge. |
@jnewbery looks great, thank you. |
No description provided.