ADD: AOPP #2915
ADD: AOPP #2915
Conversation
limpbrains
force-pushed
the
limpbrains-aopp
branch
from
8577f84
to
c57f9b4
Compare
limpbrains
force-pushed
the
limpbrains-aopp
branch
from
c57f9b4
to
f2a25e6
Compare
limpbrains
force-pushed
the
limpbrains-aopp
branch
2 times, most recently
from
d41ab06
to
4529fc1
Compare
|
Two tiny things:
|
fixed, thanks for testing! |
screen/wallets/signVerify.js
Outdated
| @@ -60,6 +62,16 @@ const SignVerify = () => { | |||
| try { | |||
| const newSignature = wallet.signMessage(message, address); | |||
There was a problem hiding this comment.
IIUC to make this work with AOPP segwit signatures the options variable needs to be set to undefined
BlueWallet/class/wallets/legacy-wallet.js
Line 516 in 4b7081c
as described in https://github.com/bitcoinjs/bitcoinjs-message#about-electrum-segwit-signature-support
There was a problem hiding this comment.
Correct. But I'm not sure what type of signature should BlueWallet generate by default.
I think since we are using bitcoinjs-message we should stick with their defaults.
Anyway to increase AOPP adoption, you need to try both variants on a backend.
There was a problem hiding this comment.
The "electrum signature type" is the one bitcoin core uses too. So currently, BlueWallet signatures are not compatible with bitcoin core's.
I read that trezor supports both signature types by now because they wanted to be compatible with bitcoin core. Which means that everyone is supporting "electrum signature types" but not the trezor one.
Because AOPP is brand new I would really like to keep this ambiguity out of the protocol and just stick with the "electrum signature types". I hope that you can see the value of having a clean protocol. I'm aware that the spec is not clear on that but I'm happy to propose a commit for that.
(I'm using "electrum signature types" here to be consistent with the bticoinjs-message terminology. To me, those are just bitcoin core message signatures)
There was a problem hiding this comment.
@limpbrains maybe we should introduce "electrum/bitcoincore compatible format" checkbox on sign/verify screen so user could choose..? and ofcourse when signature is requested by aopp - only produce aopp compatible signature (don't even show the checkbox)
There was a problem hiding this comment.
@Overtorment
I feel like we should switch to "electrum" format by default.
what do you think ?
There was a problem hiding this comment.
@limpbrains whatever you feel is right, I'm not good with sign/verify topic. id say default should be whats the majority of software supports..?
screen/wallets/signVerify.js
Outdated
| @@ -60,6 +62,16 @@ const SignVerify = () => { | |||
| try { | |||
| const newSignature = wallet.signMessage(message, address); | |||
There was a problem hiding this comment.
@limpbrains maybe we should introduce "electrum/bitcoincore compatible format" checkbox on sign/verify screen so user could choose..? and ofcourse when signature is requested by aopp - only produce aopp compatible signature (don't even show the checkbox)
screen/wallets/aopp.js
Outdated
| }, []); // eslint-disable-line react-hooks/exhaustive-deps | ||
|
|
||
| const handleChoose = address => | ||
| navigation.navigate('SignVerify', { |
There was a problem hiding this comment.
why navigate to signVerify when we have the wallet, we have the address, we could sign right on the spot? no changes to screen/wallets/signVerify.js necessary
There was a problem hiding this comment.
Because we need to show user what is he actually signing.
|
|
||
| await a.send({ address, signature }); | ||
| }); | ||
| }); |
|
i missed this when reading the spec. is there a scenario when server already knows our address, and we just need to find appropriate wallet that owns this address and do the signature with it..? |
|
@Overtorment wrote:
@limpbrains had some interesting ideas for such scenarios that I think are worthwhile taking a closer look. However, for the initial version the goal is to have a narrow scope such that the effort is minimized on the wallet side to allow for quick adoption. Hence, those scenarios are candidates for an upcoming version. |
limpbrains
force-pushed
the
limpbrains-aopp
branch
from
8ac9bbf
to
a599543
Compare
|
Let's merge it. |
limpbrains
force-pushed
the
limpbrains-aopp
branch
from
e5eb3d0
to
d210b9d
Compare
|
♫ This was a triumph. I'm making a note here: HUGE SUCCESS ♫ [android in browser] https://appetize.io/app/0f3z34g1d6cc52n8xdbkhu6xcw |
aopp.mov
closes #2861