UkoeHB / monero

Monero: the secure, private, untraceable cryptocurrency
https://getmonero.org
Other
7 stars 4 forks source link

Legacy payment ID's #46

Open j-berman opened 6 months ago

j-berman commented 6 months ago

Would be nice if the scanner picked up payment ID's for users who use and have used the feature in the past.

Looks like it could be well suited for LegacyBasicEnoteRecord, LegacyIntermediateEnoteRecord, and LegacyEnoteRecord

Here's the logic wallet2 uses to identify and decrypt payment ID's: https://github.com/monero-project/monero/blob/c8214782fb2a769c57382a999eaf099691c836e7/src/wallet/wallet2.cpp#L2699-L2761

UkoeHB commented 6 months ago

It's part of the tx memo in the enote origin context already.

j-berman commented 6 months ago

It's in ciphertext there. Plus it relies on other data in the tx in order to decrypt and utilize the same as wallet2.

It could be done at a different time than scanning but scanning feels like a good time to do it. We'll want that logic from wallet2 implemented somewhere.

UkoeHB commented 6 months ago

Hmm I would prefer for this to be handled by downstream consumers of enote store events. The payment ID is not strictly part of the enote, it is extra information passed along with a tx. Jamming that handling into the existing scan/enote store flow just feels like spaghettifying an already very complex design. I realize it's very tempting to just compromise a little and shove this in, but clean design like in the Seraphis library comes from being painfully strict. Otherwise you suffer death by a thousand cuts (aka wallet2).

j-berman commented 6 months ago

It's moving the spaghetti somewhere else, but ok

UkoeHB commented 6 months ago

The enote store was never designed to be the 'complete source of all information you can ever want', because that task is simply too big. It's already quite complex just handling enote management.

The idea was always for there to be downstream consumers of enote store events that manage more user-facing information (such as payment IDs).

UkoeHB commented 6 months ago

It's moving the spaghetti somewhere else, but ok

This implies there is no solution without spaghetti, which surely isn't true. Decouple and isolate

j-berman commented 6 months ago

I don't agree that it would spaghettify the scanner any more so than it would spagettify any other place it's placed, but will see how it goes implementing downstream

UkoeHB commented 6 months ago

That's fair. I will leave this issue open. If it is truly not a good fit for downstream, then we can revisit.

jeffro256 commented 1 month ago

Payment IDs were the only official way to discriminate senders of received payments until subaddresses, and are still used today for integrated addresses. As such, I think that payment IDs are in scope at least for the enote store which needs to maintain balance recovery information.

jeffro256 commented 1 month ago

They're encrypted inside tx_extra which would mean you would need to bring your private key key online to see stored payment IDs, even after the enotes are already scanned which isn't ideal IMO. I also don't like the messiness of it, but since payment IDs was an official feature of addressing, I believe it should be supported

UkoeHB commented 1 month ago

I also don't like the messiness of it, but since payment IDs was an official feature of addressing, I believe it should be supported

Architecturally the issue is don't you need a bunch of other information for the enote store to be useful to a wallet? Information the enote store can't feasibly handle without becoming overburdened. If that's the case, then you need to be thinking how to build a companion for the enote store to manage that extra data, and I assume payment IDs would fall in place there.

I forget the whole workflow, but IIRC the scanning scaffolding would let you stick more than an enote store in there.

Of course, the greatest danger is bugs from having multiple potentially unsynchronized representations of wallet state at once. It's a challenging design space and I won't claim to have it solved. But I'm definitely reluctant to expand the enote store, since it was a huge pain to get right.