Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add Developer Examples Page / New Example Transactions #442

Merged
merged 21 commits into from Jun 10, 2014

Conversation

Projects
None yet
2 participants
Contributor

harding commented Jun 7, 2014

As discussed on the docs mailing list, the developer docs are intended to have three main pages: the Developer Guide, the Developer Reference, and another page we provisionally called the Developer Cookbook (because it would be similar to the O'Reilly "[Programming Language] Cookbook"-style books.

I've added this page using the name Developer Examples. The new page features a newly-written set of example transactions with detailed explanations, which is something that's been TODO'd for a couple months now. It also features the extended BIP70 example from the Guide. A preview is available here:

http://dg0.dtrt.org/en/developer-examples

The extended BIP70 example from the Guide has been removed and replaced with an overview-level description of the Payment Protocol.

Now that we have three pages, I also gave each of the full pages their own icon on the portal page.

A new CSS class, multicode, has been added to allow multiple syntax highlighted examples to be included in a single code block. This lets us use bash highlighting for bash commands and json highlighting for JSON output, or any other combination. It can be seen in use in many of the example transactions.

Two one-line were made to the autocrossref plugin. The first change makes autocrossref ignore code blocks; the second change makes it remove HTML comments from autocrossref text so we can easily check to ensure it doesn't create broken links (see 61f0931 for details).

This is a large change, so I will not merge it until I receive at least one ACK.

harding and others added some commits May 30, 2014

Add Developer Examples; Transaction Tutorial
* Add new Developer Examples page

* Add a transaction tutorial describing in detail how to make various
  different transactions.

* Add a new "multicode" CSS class to allow combination of consecutive
  code blocks into a single code block. This lets us use pygments
  highlighting for multiple different types of code within the same
  aparent block of code.

* Get autocrossref to ignore code blocks so we don't need to endcrossref
  every time we encounter a code block. This makes the source much more
  readable and maintainable.
Move BIP70 Example To Dev Examples
* Move the extened BIP70 example from the guide to the developer
  examples page.

* Add new text introducing BIP70 to the guide
Add Icons; Update Links
* Add new icons for Developer Guide and Developer Examples so we don't
  keep reusing the book icon.

* Update payment protocol links to point to developer examples when
  appropriate.
Merge remote-tracking branch 'bitcoin.org/master' into generatingtxes
Conflicts:
	_includes/guide_payment_processing.md
	_less/screen.less
Updates Based On Feedback From Mailing List
* Remove the QR Code error corrections subsection.

* Remove the non-example Payment Protocol text from developer examples.

* Update reference links and autocrossrefs to reflect above deletions

* Fix CSS padding problems reported by @saivann

* Remove all HTML comments from autocrossref text to allow easy checking
  for broken link definitions: find _site -name '*.html' | xargs grep '\]\['

* Add PNG versions of guide and example SVG icons.  (optipng run)
Simplify multicode CSS styles
Add consistent border-radius
Simplify margins / paddings
Contributor

harding commented Jun 7, 2014

Note: the preview site has been updated with the changes @saivann made through to ca7b38d. (Thanks Saïvann!)

Contributor

saivann commented Jun 8, 2014

@harding I'm still slowly testing each part of the examples page and so far everything seems good!

However I'd like to suggest the "Simple Raw Transaction" example to use two outputs (one for the payment, one for the change). Although your warning text after the example is crystal clear, I think the vast majority of transactions have to spend part of the UTXO in change outputs, so handling change outputs where it's not automagically done by the API seems like a prudent approach to me given the risk involved.

Contributor

harding commented Jun 8, 2014

@saivann the example immediately following, "Complex Raw Transaction", has two outputs (and two inputs).

More importantly, it seems important to me to have a baseline simple-as-possible transaction before moving on to more complex examples. I explicitly make comparisons in several later parts of the text:

  • (Complex raw) "Create the raw transaction using createrawtransaction much the same as before, except now we have two inputs and two outputs."
  • (Offline signing) "Attempt to sign the raw transaction without any special arguments, the way we successfully signed the the raw transaction in the Simple Raw Transaction subsection."
  • (Multisig) "We generate the raw transaction the same way we did in the Simple Raw Transaction subsection."

If you think it's necessary, we can add a Simple Two-Output Raw Tx subsection immediately between the Simple and Complex Raw Txes. Let me know.

Thanks!,

-Dave

Contributor

saivann commented Jun 8, 2014

@harding Yes I saw that you tried to make this as simple as possible. Yet, my first impression is that we should accustom people with "Any createrawtransaction call must have a change output". If this complexity discourages someone from using this API Call and use sendtoaddress instead, then at a first glance I'd say this is probably a good thing, given the consequences of misusing this API Call.

Contributor

harding commented Jun 8, 2014

@saivann I'm not a big fan of that. I personally almost always use createrawtransaction (CRT) without change outputs. For example, I often use CRT to fully respend one or more outputs so I can add a locktime or move multisig outputs to my P2PKH cold wallet.

Although I'm not a fan of relying on it, it should be noted that sendrawtransaction will (by default) throw an error if you attempt to broadcast a transaction with what it considers an excessive fee. (That's in our RPC docs for sendrawtransaction.)

I also still think keeping the examples simple is the most important reason not to add additional outputs to the simplest example. If users ignore our warning about using raw txes on mainnet, and ignore our advice to only use spendtoaddress, and ignore our warning about CRT not creating change outputs---then making our examples more complicated is unlikely to dissuade them anyway.

Sorry to be stubborn about this.

-Dave

Contributor

harding commented Jun 8, 2014

@saivann do you think a reasonable compromise would be to add a short one-line warning to each use of CRT about change outputs? For example a shell comment above each call:

## NB: any satoshis from previous outputs not fully spent here will be LOST
Contributor

saivann commented Jun 8, 2014

sendrawtransaction will (by default) throw an error if you attempt to broadcast a transaction with what it considers an excessive fee

Didn't know that, that's great!

@harding OK I think it's fine, let's not make this a blocking issue. I didn't think my suggestion was making the example too complicate, but it is true that we have clear warning text about it (I will open a pull req to further increase visibility to these warning texts, maybe just this icon, but after this pull req is merged)

harding added some commits Jun 8, 2014

Add Tx Malleability Warning To Spending Unconfirmed Tx Example
Suggested and written by @saivann, with a revised version being
commited here. Thanks!
Add Note About Tx Fees To All Createrawtransaction Example Calls
Based on feedback from @saivann, this may help remind users that poor
use of createrawtransactions can cost them satoshis.
Contributor

harding commented Jun 8, 2014

Note: I updated the preview with commits 96bc22b (tx malleability warning) and abf8ac2 (inputs - outputs = tx fee warning).

@saivann saivann commented on an outdated diff Jun 9, 2014

_includes/guide_payment_processing.md
+* An optional memo Charlie can send to Bob. (There's no guarantee that
+ Bob will read it.)
+
+* A refund address (output script) which Bob can pay if he needs to
+ return some or all of Charlie's satoshis.
+
+Bob's server receives the Payment message, verifies the transaction pays
+the requested amount to the address provided, and then broadcasts the
+transaction to the network. It also replies to the HTTP POSTed Payment
+message with a PaymentACK message, which includes an optional memo
+from Bob's server thanking Charlie for his patronage and providing other
+information about the order, such as the expected arrival date.
+
+Charlie's wallet sees the PaymentACK and tells Charlie that the payment
+has been sent. The PaymentACK doesn't mean that Charlie has actually
+paid Bob---see the Verifying Payment subsection below---but it does mean
@saivann

saivann Jun 9, 2014

Contributor

This sentence seemed a little confusing to me on a first read. How about:

The PaymentACK doesn't mean that Charlie's order and payment is final
Contributor

saivann commented Jun 9, 2014

@harding Just sent you a couple of additional pull requests.

I noted that the Payment and PaymentACK texts disappeared. How about we add them back later in the "Reference" page?

...And I didn't test links, did you test them, per chance?

Otherwise, I've tested the whole Transaction examples subsection and this all LGTM. Really amazing work, which I'm sure will prove useful. Thanks!!

harding added some commits Jun 9, 2014

Merge remote-tracking branches 'saivann/generatingtxesbips', 'saivann…
…/generatingtxesresource' and 'saivann/generatingtxestypos' into generatingtxes
Clarification Of PaymentRequestss And Payment Verification
* Clarify a few places where payment request was used instead of
  PaymentRequest.

* Clarify that payment isn't necessarily verified until the merchant
  says so. Suggested by @saivann (thanks!)
Contributor

harding commented Jun 9, 2014

@saivann updated with your patches and a version of the clarification you suggested. I also fixed the merge problem and used the makefile (which, whoops, never got PR'd) to check all the links.

I think this addresses all known feedback, so if no critical feedback is received, I'll merge this around 18:00 UTC Tuesday.

Edit: Preview updated: http://dg0.dtrt.org/en/developer-examples

Contributor

saivann commented Jun 9, 2014

@harding Great! LGTM. Did you see my suggestion about the Payment and PaymentACK texts in my last comment?

Contributor

harding commented Jun 9, 2014

@saivann yes, I saw it. I'm not sure how I feel about that. I deleted them because I thought they basically repeat what BIP70 itself says---and because I think we could probably just expand the example we have to cover Payment and PaymentACK. In fact, I'll add that to the todo right now.

@harding harding merged commit ba1a33e into bitcoin-dot-org:master Jun 10, 2014

@harding harding deleted the harding:generatingtxes branch Feb 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment