Skip to content
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

Store payment outputs in wallet database #9

Closed
wants to merge 10 commits into from

Conversation

garyyu
Copy link
Contributor

@garyyu garyyu commented Mar 7, 2019

Currently we only store self owned outputs in wallet database, i.e. our change output/s or received output.

The payment output (i.e. the one we send to others) is not stored on local wallet database.

But in some use cases, we need query the output which we sent.

So, in this PR, we add the following command:

  1. wallet payments list. for example:
$ grin-wallet --floonet payments

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Output Commitment                                                   Block Height  Locked Until  Status       # Confirms  Value        Shared Transaction Id 
============================================================================================================================================================================
 08623af49f7ca6180f626a2c6f4f23f994dbc5b5699d9d7c17aae67bf61d121d76  75747         0             Unconfirmed  0           2.400000000  388f604a-d67c-4b7d-95b9-1a8ae50a4586 
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 08792a45a9f41fa87afc1fd8b6ed671a4b42cf92c0e019511131af2847f83ab99d  75747         0             Confirmed    1           2.300000000  ce5d4a8b-94bc-4b1a-a1d7-d108aea6e4a0 
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 08b6fa0628393acec5ab84e7f1ae99464d919908a24d580c1f10757c7c1bbac264  75726         0             Confirmed    22          2.200000000  f7ecacf0-73eb-4957-86d0-404ba4d43e3b 
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  1. txs -i n will show the payment output also. for example:
$ ./grin-wallet --floonet txs -i 16

Transaction Log - Account 'default' - Block Height: 75747
----------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Id  Type     Shared Transaction Id                 Creation Time        Confirmed?  Confirmation Time    Num.    Num.     Amount    Amount   Fee    Net         Tx  
                                                                                                          Inputs  Outputs  Credited  Debited         Difference  Data 
======================================================================================================================================================================
 16  Sent Tx  ce5d4a8b-94bc-4b1a-a1d7-d108aea6e4a0  2019-03-08 09:03:56  true        2019-03-08 09:07:25  1       1        41.295    43.603   0.008  -2.308      Yes 
----------------------------------------------------------------------------------------------------------------------------------------------------------------------


Wallet Outputs - Account 'default' - Block Height: 75747
-------------------------------------------------------------------------------------------------------------------------------------------------------------
 Output Commitment                                                   MMR Index  Block Height  Locked Until  Status   Coinbase?  # Confirms  Value         Tx 
=============================================================================================================================================================
 0935fe8926aff61e6837188578cd1b45779ebaff3f237ef546c2c5b01b2836f3b0  None       75726         0             Spent    false      22          43.603000000  16 
-------------------------------------------------------------------------------------------------------------------------------------------------------------
 09815b2f7d67dcd6985ce349ecf9dcac2a260d9495e1162245745c63b4a056cd42  None       75747         0             Unspent  false      1           41.295000000  16 
-------------------------------------------------------------------------------------------------------------------------------------------------------------


Wallet Payments - Account 'default' - Block Height: 75747
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Output Commitment                                                   Block Height  Locked Until  Status     # Confirms  Value        Shared Transaction Id 
==========================================================================================================================================================================
 08792a45a9f41fa87afc1fd8b6ed671a4b42cf92c0e019511131af2847f83ab99d  75747         0             Confirmed  1           2.300000000  ce5d4a8b-94bc-4b1a-a1d7-d108aea6e4a0 
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------


Transaction Messages - Transaction '16'
--------------------------------------------------------------------------------------------------------
 Participant Id  Message  Public Key                                                          Signature 
========================================================================================================
 0               None     038bb25be3340b577857e3bc3b6d8e26aff1334438f5b06969a24b2790fa865af5  None 
--------------------------------------------------------------------------------------------------------
 1               None     036166f585700419c60e1a28f56d61c9eace8328ccda1bad71cdef044e37d97cdc  None 
--------------------------------------------------------------------------------------------------------

@garyyu garyyu marked this pull request as ready for review March 8, 2019 09:09
@garyyu garyyu added the enhancement New feature or request label Mar 8, 2019
@garyyu garyyu requested a review from yeastplume March 8, 2019 09:09
@yeastplume
Copy link
Member

yeastplume commented Mar 8, 2019

Technically it looks good, but a few points/concerns

  • How is the case handled where the recipient creates multiple outputs for themselves instead of just one? It won't be possible for the sender to know the exact amounts to associate with each payment output.
  • I have a few concerns about inflating the WalletBackend trait with even more methods, so there might need to be some refactoring there eventually.
  • I'd generally ensure that unit tests are in place for everything, particularly in refwallet/tests as well as src/bin/cmd/wallet_tests.rs.

But that aside, this is a significant feature that we haven't really discussed, so I'd like to have a wider discussion on the implications of allowing this behaviour by default. Also, since it's part of the backend trait it's encouraging wallet developers using our libs to include this functionality, so it would likely become a common feature in future wallets.

Obviously this info is available to all parties in a transaction (concerns about amounts aside,) but actively storing it by default may have some privacy concerns. It would also be good to have an idea of why you think this is necessary, and what use cases this addresses.

Perhaps we can get a few more people in on the review?

@garyyu
Copy link
Contributor Author

garyyu commented Mar 8, 2019

Thanks for the review 😄

How is the case handled where the recipient creates multiple outputs for themselves instead of just one? It won't be possible for the sender to know the exact amounts to associate with each payment output.

I can't see a practical use case for one recipient creating multiple outputs, since that will greatly increase the tx fee but paid by sender. So, IMO, we should strictly limit one recipient only have one output.

I have a few concerns about inflating the WalletBackend trait with even more methods, so there might need to be some refactoring there eventually.

fully agree :-) but with another PR.

I'd generally ensure that unit tests are in place for everything

👍 I will add the corresponding unit tests for those new functions.

Obviously this info is available to all parties in a transaction (concerns about amounts aside,) but actively storing it by default may have some privacy concerns. It would also be good to have an idea of why you think this is necessary, and what use cases this addresses.

It normally makes sense for sender keeping a record for what he/she paid. Since anyway this info is there already, I don't think storing it into database increase more privacy concerns.

Why you think this is necessary, and what use cases this addresses.

Not limited in this use case, but I saw an exchange (iirc, bittrex) use their received output as the unique id for a transaction, I want to address this output after I send to this exchange but I found we didn't store this :-) and then I thought it's a good requirement to use output as the unique ID instead of the wallet UUID (which is definitely not a real unique id, since anyone can reuse a UUID easily).

Anyway, the more sense is that I paid I recorded, a simple requirement.

@rentenmark
Copy link

Since anyway this info is there already, I don't think storing it into database increase more privacy concerns.

I completely agree with this sentiment. Any sufficiently sophisticated user will be maintaining their transaction slate archive which gives access to the same information. This feature just "democratizes" it in some sense.

@yeastplume
Copy link
Member

I can't see a practical use case for one recipient creating multiple outputs, since that will greatly increase the tx fee but paid by sender. So, IMO, we should strictly limit one recipient only have one output.

There's no way to enforce this at the protocol level and wallets can do as they choose, the case definitely needs to be handled.

@garyyu
Copy link
Contributor Author

garyyu commented Mar 8, 2019

I can't see a practical use case for one recipient creating multiple outputs, since that will greatly increase the tx fee but paid by sender. So, IMO, we should strictly limit one recipient only have one output.

There's no way to enforce this at the protocol level and wallets can do as they choose, the case definitely needs to be handled.

In case indeed one recipient creating multiple outputs (don't have if using this wallet software), we set the Value as the average value and put one more column to indicate Estimated / Accurate. Is this acceptable?

And we strictly limit one recipient only have one output in this official wallet software. can give another dedicated PR to this limitation.

@lehnberg
Copy link
Collaborator

lehnberg commented Mar 8, 2019

I can't see a practical use case for one recipient creating multiple outputs, since that will greatly increase the tx fee but paid by sender. So, IMO, we should strictly limit one recipient only have one output.

Here's an example. I have 1 x UTXO of 5,000 grins. I would like to split this into 10 x UTXOs of 500 grins each so that I can send multiple transactions in parallel to other users without having all my funds locked.

I don't see why we would want to limit a recipient to only be allowed to receive a transaction as one single UTXO.

@lehnberg
Copy link
Collaborator

lehnberg commented Mar 8, 2019

Not limited in this use case, but I saw an exchange (iirc, bittrex) use their received output as the unique id for a transaction, I want to address this output after I send to this exchange but I found we didn't store this :-) and then I thought it's a good requirement to use output as the unique ID instead of the wallet UUID (which is definitely not a real unique id, since anyone can reuse a UUID easily).

I might be missing something here, but what does this help with? And can the data be tampered with?

@garyyu
Copy link
Contributor Author

garyyu commented Mar 9, 2019

@lehnberg

Here's an example. I have 1 x UTXO of 5,000 grins. I would like to split this into 10 x UTXOs of 500 grins each so that I can send multiple transactions in parallel to other users without having all my funds locked.

I don't see why we would want to limit a recipient to only be allowed to receive a transaction as one single UTXO.

Look like you're mixing the recipient and changes. The splitting feature is done by the changes without any problem, and has nothing with the topic we're discussing.

I might be missing something here, but what does this help with? And can the data be tampered with?

^^^ please look above discussions:-)
"the more sense is that I paid I recorded, a simple requirement."

@lehnberg
Copy link
Collaborator

lehnberg commented Mar 9, 2019

Look like you're mixing the recipient and changes. The splitting feature is done by the changes without any problem, and has nothing with the topic we're discussing.

It might not be the same wallet that's to have the split outputs. Example, I have a cold storage wallet that I wish to withdraw 5,000 grins from into 10 x 500 grin outputs in a hot wallet. Could I do that using the change outputs?

Look I'm fully aware that I'm talking of quite advanced use cases here, it's just that... Handling a lot of transactions in parallel in Grin is very complicated for businesses, and I wouldn't want to make it more complicated. I am not sure how it makes sense to impose limits on basic protocol functionality in order for more data to be displayed in a data log of transactions.

I might be missing something here, but what does this help with? And can the data be tampered with?
^^^ please look above discussions:-)

Yes, I looked above at the discussions, I couldn't understand what problem this was addressing, so I thought I should ask instead in the hopes that someone could explain it to me. :)

"the more sense is that I paid I recorded, a simple requirement."

This is the problem this solves for, so that the user can keep track of what outputs they sent funds to? How does that help the user? Can they prove they sent funds there? Can the data be tampered with? How would they use this information exactly?

@garyyu
Copy link
Contributor Author

garyyu commented Mar 11, 2019

Regarding the multiple outputs on single receiver case, OK for no limitation, and it's not the part of this PR.

Regarding the use case of this PR: store payment output when paid, let's listen more voice from the users :-)

@JacobPlaster
Copy link

This feature will be extremely useful to us! Thanks for the good work @garyyu

@seibelj
Copy link

seibelj commented Mar 12, 2019

I support this feature, it will make diagnosing disagreements between participants easier to solve.

@lehnberg
Copy link
Collaborator

it will make diagnosing disagreements between participants easier to solve.

It would help my understanding if there could be a concrete example of a disagreement and how this feature would solve it.

Let's say for example:

  1. User sends Business a transaction.
  2. Business says they did not receive the transaction.
  3. User can now look up the tx in their wallet database and say "but I sent you it, output xyz!"

...What happens next?

@yeastplume
Copy link
Member

yeastplume commented Mar 13, 2019

Just to update with current thoughts, I'm still not convinced this functionality is worth the extra complexity and long-term maintenance, so it would be good to have some proper use cases spelled out. Also, how many users would use this? Sent transactions are stored at the moment, so if it's only a small minority of people who think they need this they could easily extract outputs from there.

@garyyu
Copy link
Contributor Author

garyyu commented Apr 25, 2019

Close obsoleted

@garyyu garyyu closed this Apr 25, 2019
@garyyu garyyu deleted the payments branch April 25, 2019 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants