-
Notifications
You must be signed in to change notification settings - Fork 137
Compatibility data: abra binance bitgo bitmex #178
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: abra binance bitgo bitmex #178
Conversation
7bbc6a6
to
fb5f4ba
Compare
rebased |
@harding - please review for typos/formatting/etc. Let me know if you can't review this week. Thanks! |
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.
A few small notes on these wallets inline. I posted comments on the framework in general elsewhere. Thanks for all your investigating and information curating @bitschmidty!
_data/compatibility/abra.yaml
Outdated
examples: | ||
- image: /img/compatibility/abra/send-screen-default.png | ||
caption: > | ||
Sending RBF Transaction - Default send transaction screen |
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 find it disconcerting that this and other descriptions on this wallet start with "Sending RBF Transaction" when the categorization text says, "Does not signal BIP125 replacability when sending transactions".
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.
The "Sending RBF Transaction" part was supposed to designate the "scenario/test". So "Sending RBF Transaction - " screenshots were workflow as a part of try to achieve the goal of sending an RBF transaction.
John gave some good feedback on Bitcoin Core and reworded those before-the-hash scenarios. Can you let me know what you think of his wording on the usability sections: https://bitcoinops.org/en/compatibility/bitcoin-core/ @harding
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 agree that these 'scenario' strings are confusing and should be changed (eg to "Sending transaction" instead of "Sending RBF transaction")
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 think the ones on Bitcoin Core make sense. Maybe they should all be phrased as uncompleted actions "Attempting to fee bump - fee bump menu option", "Attempting to fee bump - accepting new fee," etc...
Alternatively, we could maybe use numbers to tie related images together and reduce the duplication of words:
(1) Attempting to fee bump - foo bar baz | (2) quux qux widget
(3) blah blah blah | (4) eh meh neh
_data/compatibility/binance.yaml
Outdated
send: | ||
bech32: "true" | ||
change_bech32: "untested" | ||
segwit_v1: "Does not pass validation" |
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 don't know what "Does not pass validation" means. Since this is being passed to the wallet details page as text, I think it should be more descriptive about the problem and possibly about what the wallet author should do to fix the problem.
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 brought up the valid concern of whether we should even show segwit v1 results. Im going to leave this for now until we decide on that.
Edit: actually added in a bit more info for this in the meantime.
_data/compatibility/binance.yaml
Outdated
the two-factor authentication code, an address error message appears. | ||
- image: /img/compatibility/binance/segwit/address-book-add.png | ||
caption: > | ||
Bech32 addresses can be successfully added using the address book functionality. |
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.
Did you happen to get any screenshots of the P2WSH addresses not working? I think that'd be a useful addition here.
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 back and tested this again to get screenshots and added one in. Also clarified some of the bech32 p2wsh/p2wpkh distinctions.
fb5f4ba
to
1f98616
Compare
Pushed a commit to address @harding 's feedback (thank you sir!) with the exception of #178 (comment) pending more feedback |
pushed commits with:
|
9ef9c7c
to
3e2cc35
Compare
3e2cc35
to
99ad7a0
Compare
No description provided.