Add bip21 receive display support #1922
Conversation
benalleng
force-pushed
the
bip21-support
branch
2 times, most recently
from
a062d7b to
1e90273
Compare
liana-gui/src/app/state/receive.rs
Outdated
| #[derive(Debug, Default)] | ||
| pub struct Addresses { | ||
| list: Vec<Address>, | ||
| bip21s: HashMap<Address, String>, |
There was a problem hiding this comment.
I dont think we should store uri strings, they can be generated in the view, it's not ressources intensive
There was a problem hiding this comment.
If we don't store the uri string how do you think we should flag that the address being displayed should have some extra bip21 information, just a simple bool flag to indicate some additional method should be called?
There was a problem hiding this comment.
If we don't store the uri string how do you think we should flag that the address being displayed should have some extra, just a simple bool flag to indicate some additional method should be called?
I think so, but I'm not sure to have the whole context in mind, what additional infos can the uri potentially contains in our usage? I imagine:
- the amount user expect to receive (we do not yet support this feature, do we want it in the future @nondiremanuel?)
- information for receive payjoin (Directory url + auth metadata?), iiuc this is per-session based, do we store it in the DB? can several addresses share sames set of metadata? Can we have to deal with different set of metadata while the receive screen is still open? can an address have to been appended with different set of metatdat in some edge cases?
There was a problem hiding this comment.
You're right for all that. I was mostly asking in reference to just the physical display to keep the Qr code methods simple to just add some is_bip21 flag that just appends the relevant data on at the end since Message::ShowQrCode would need to either be passed or generate the bip21 data as it is built for a simple address index right now
There was a problem hiding this comment.
what about this:
do we store it in the DB? can several addresses share sames set of metadata? Can we have to deal with different set of metadata while the receive screen is still open? can an address have to been appended with different set of metatdat in some edge cases?
There was a problem hiding this comment.
Bullbitcoin wallet allow user to add amount & note for a PJ (receive), but I mean it can be another feature (not related to PJ) also
There was a problem hiding this comment.
do we store it in the DB? can several addresses share sames set of metadata? Can we have to deal with different set of metadata while the receive screen is still open? can an address have to been appended with different set of metatdat in some edge cases?
So no, each address should have a new pj session, reused addresses will cause problems as far as I know. There aren't really any practical edge cases where I could think of wanting to do that practically
Can we have to deal with different set of metadata while the receive screen is still open?
As for this I think the only 3 things that are really notable to happen during this stage
- the directory rejects / never receives our payjoin initialization
- the sender responds before we even exit this screen so we jump to the second signing stage immediately
- All other states are simply waiting for the sender
There was a problem hiding this comment.
The amount is not explicitly required in a pj, so for now I don't think we need to add it unless we think that an amount would dramatically change or improve the UX, especially with looking at the receive screen as it stands now it would likely require some design work to think about how to add a fixed amount seamlessly when it is not necessary for payjoins
There was a problem hiding this comment.
each address should have a new pj session,
where we store metadata about the PJ session? in RAM? in DB?
just to get back to the topic of this thread, I dont think it makes sense to store it here, as ReceivePanel is reset on reload:
liana/liana-gui/src/app/state/receive.rs
Line 313 in ca768d6
There was a problem hiding this comment.
A payjoin session data would ideally be stored in the DB but the bip21 associated with it would ideally not be recalled as a user really shouldn't be reusing it outside of the initial receive screen as it would be essentially restarting a sessions and could encourage some bad user patterns like giving a bip21 to 2 separate senders.
I think it might be helpful to perhaps open a more complete receiver PR to give some more context on some design decisions.
liana-gui/src/app/state/receive.rs
Outdated
| .map(|qr_code| Self { | ||
| pub fn new(address: &str, index: ChildNumber) -> Option<Self> { | ||
| if Address::from_str(address).is_ok() { | ||
| qr_code::Data::new(format!("bitcoin:{}?index={}", address, index)) |
There was a problem hiding this comment.
when an address is displayed as QR, its expected to be passed to a peer for payment, I dont see a good reason to let him know our derivation index.
The index can be usefull only for a signing device to verify the address, like #1040 or with Coldcard-Q/Krux devices, but in this case we should also pass the descriptor name. (Not related to this PR, but maybe we should open an issue more generic tan #1040 since we support more QR devices @nondiremanuel)
benalleng
force-pushed
the
bip21-support
branch
3 times, most recently
from
68a1dc7 to
c9e81ef
Compare
This adds the scaffolding for the receiver to display bip21 QR codes when a payjoin or other bip21 receive format is required.
benalleng
force-pushed
the
bip21-support
branch
from
c9e81ef to
e9d77a3
Compare
3 participants
This adds the scaffolding for the receiver to display bip21 QR codes when a payjoin or other bip21 receive format is required.
This is the first commit to begin support for #1806 by splitting it up into many smaller commits. This particular commit does not require any payjoin support.