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

0006-payment-proofs.md #31

Merged
merged 6 commits into from
Dec 3, 2019
Merged

0006-payment-proofs.md #31

merged 6 commits into from
Dec 3, 2019

Conversation

DavidBurkett
Copy link
Contributor

@DavidBurkett DavidBurkett commented Nov 5, 2019

@DavidBurkett DavidBurkett marked this pull request as ready for review November 12, 2019 10:41
@DavidBurkett DavidBurkett changed the title WIP: 0000-payment-proofs.md 0000-payment-proofs.md Nov 12, 2019
Copy link
Contributor

@lehnberg lehnberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job @DavidBurkett see minor questions and feedback points.

text/0000-payment-proofs.md Outdated Show resolved Hide resolved
text/0000-payment-proofs.md Outdated Show resolved Hide resolved
text/0000-payment-proofs.md Outdated Show resolved Hide resolved
text/0000-payment-proofs.md Outdated Show resolved Hide resolved
text/0000-payment-proofs.md Outdated Show resolved Hide resolved
Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice @DavidBurkett. See minor typos and question below.

text/0000-payment-proofs.md Outdated Show resolved Hide resolved
text/0000-payment-proofs.md Outdated Show resolved Hide resolved
text/0000-payment-proofs.md Outdated Show resolved Hide resolved
text/0000-payment-proofs.md Outdated Show resolved Hide resolved
text/0000-payment-proofs.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jaspervdm jaspervdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on the RFC! It looks good to me, just have a question:
I'm wondering if we should make the sender_address mandatory, to avoid the re-use of proofs for multiple transactions of the same amount

@DavidBurkett
Copy link
Contributor Author

I'm wondering if we should make the sender_address mandatory, to avoid the re-use of proofs for multiple transactions of the same amount

That was one of my open questions at the bottom. I wasn't real sure of the consequences of an optional sender address, so if you think it might be worth making it mandatory, I'm convinced that's probably the right decision.

@yeastplume
Copy link
Member

Yes, I think making it mandatory is the right approach. So fine to go ahead with this on the understanding that proofs won't be possible for invoice transactions just yet. (They are theoretically possible but would require another workflow involving extra back and forth where the payer is always the last to sign off on the TX.. not great from a UX perspective)

@lehnberg
Copy link
Contributor

+1 for making sender_address mandatory.

Should sender_address use (i.e. address re-use vs new sender address each time) be handled separately from this RFC, or does it make sense to set a default here as part of this effort?

@DavidBurkett
Copy link
Contributor Author

I prefer to leave that as an implementation detail, especially since it's easy and maybe even likely for different wallets to make different choices about that.

Copy link
Contributor

@lehnberg lehnberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on the changes @DavidBurkett - looks really good to me. 👍

@CircusDad
Copy link

CircusDad commented Nov 17, 2019

I fully support adding proof of payment to the grin transaction process but I caution providing an ability to add a message field to the signed portion of a grin transaction.

IF sender has the ability to put a message with a payment... AND prove I took the payment... AND prove the message was part of the payment when I took it,
THEN, I would be unwilling to blindly accept payment via a listening wallet.

"I have this signed receipt stating that CircusDad sold me -Insert Valuable Object Here- for 10 Grin"

For reference: Visa and Mastercard have been around the block a few times and, while their transactions do provide a proof of payment record between accounts, they stay out of the proof of message part of things.

I do see some value in messages but I suggest the "message" field be a separate PR subject to a separate discussion about implications and details. A solution might be for Grin to support putting a message in the signed portion of the slate... but only if both the sender and the receiver actively add the exact same message to the slate.

I strongly support proof of payment portion of this PR. Proof of payment is an important part of electronic transaction and would hate for this comment about the message field to prevent the payment-proofs portion of this PR from being added to the next Grin update.

@CircusDad
Copy link

CircusDad commented Nov 17, 2019

For completeness, I have thought of another reference that serves as a counterpoint: A bank check has a memo line to be filled out my the sender.  The receiver has choice to deposit, or not, or even to cross out/add to the memo field and then deposit.  In the latter case it is hopefully clear via hand writing what was written by the sender and what was added by the receiver (perhaps a separate sender and receiver message field for Grin?).  

I have no affiliation with this website apart from a web search, but for context: https://www.romanolaw.com/2015/11/17/can-writing-on-a-check-create-a-new-contract/   

@yeastplume
Copy link
Member

Thanks @CircusDad for your insights and your warnings about the message field makes sense to me at least.

Given this, I think we should leave the message field out for now and come back to it in a later RFC after giving the issue the appropriate consideration.

@DavidBurkett
Copy link
Contributor Author

Message field has been removed. Thanks @CircusDad

@antiochp antiochp changed the title 0000-payment-proofs.md 0006-payment-proofs.md Dec 2, 2019
@antiochp antiochp merged commit 5555b9c into mimblewimble:master Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants