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

Invoiced Transactions API Support #90

Merged
merged 18 commits into from May 1, 2019

Conversation

Projects
None yet
4 participants
@yeastplume
Copy link
Member

commented Apr 24, 2019

First attempt at supporting an invoiced transaction workflow, whereby the recipient generates a slate with an output for a certain amount, then sends the slate to a payer to fill in inputs and return. The aim of this PR is to add the API support (with some community feedback,) and implement the command line wallet functionality later. Much of the internal functionality to support this is already in place, so it's really just a case of getting the API structured correctly.

The way it's currently implemented here is:

  • Add 2 functions to the owner api, init_invoice_tx and receive_invoice_tx. Receiving in the invoice case is being the person to add inputs to the slate, so it's an owner api function. It's assumed this will always be sent to the payer to view/verify before applying it to their wallet, and parameters need to be chosen similar to the regular init_send_tx function.
  • Add 1 function to the foreign api, finalize_invoice_tx used to accept and finalize the returned slate. This can be a foreign API function as it's assumed invoicers should be able to accept a returned invoice from the payer without the invoicer needing to intervene

It might be possible to present this functionality using parameters to existing functions, but I'm leaning towards the notion that it's clearer from a client perspective to keep the functionality for both workflows distinct and a separate set of functions. Happy to hear opinions here.

Code works, with a working API example in controller/tests/invoice.rs

Outstanding tasks

  • - Review and community feedback
  • - Implementation of JSONRPC API stubs and tests
  • - Documentation of new API methods
  • - Test transaction / output state more rigorously in integration tests

yeastplume added some commits Apr 24, 2019

@DavidBurkett

This comment has been minimized.

Copy link

commented Apr 24, 2019

The code looks good, but receive_invoice_tx doesn't seem like the right wording to me. In the physical world, one would typically "pay" an invoice. I get that there's still an additional step (finalize) in the grin world before it's considered paid. Even so, I personally think something like pay_invoice is more clear. To me, hearing receive_invoice_tx makes me think I'm receiving invoice payment (finalize), not receiving the invoice to pay it.

@lehnberg

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

Yes, @DavidBurkett has a point. You also don't really init an invoice, you're making a payment request.

How about
issue_invoice_tx >> process_invoice_tx >> finalize_invoice_tx

@kargakis

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

It's assumed this will always be sent to the payer to view/verify before applying it to their wallet

How is the verification going to happen in practice? GUI wallets can hide most of the complexity found in a slate but with grin-wallet, we are left with reviewing the slate itself? Maybe there is room for dry-running process_invoice_tx before actually applying.

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

I'm happy with issue >> process >> finalize, will make the changes there

@kargakis Absolutely that is a usability issue, particularly with the command line wallet, which at the very least will have to output a summary of the slate contents before prompting to continue processing. Will have more thoughts on that in an upcoming future PR that will add the functionality to the command line wallet (this PR is just about getting the API in place)

yeastplume added some commits Apr 25, 2019

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

Happy with where this is now, going to merge shortly if no objections.

@yeastplume yeastplume merged commit 3a3057d into mimblewimble:master May 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yeastplume yeastplume referenced this pull request May 2, 2019

Merged

Command line implementation of invoice commands #96

4 of 5 tasks complete

@yeastplume yeastplume changed the title [WIP] Invoiced Transactions API Support Invoiced Transactions API Support May 2, 2019

@yeastplume yeastplume deleted the yeastplume:invoiced_tx branch May 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.