Micropayment channel example polish #4523

ChrisChinchilla · 2018-07-18T14:21:02Z

I think this was the reference PR - #4212

I tidied up some language, changed some of the code examples that were using wrong/outdated namespaces, or changed them to use namespaces that the docs of the project use.

chriseth · 2018-07-18T14:24:41Z

docs/solidity-by-example.rst

-In this section we will learn how to build a simple implementation
-of a payment channel. It use cryptographics signatures to make
+In this section we will learn how to build an example implementation
+of a payment channel. It use cryptographic signatures to make


Typo: uses

chriseth · 2018-07-18T14:25:21Z

docs/solidity-by-example.rst

 sign and verify signatures, and setup the payment channel.

 Creating and verifying signatures
 =================================

 Imagine Alice wants to send a quantity of Ether to Bob, i.e.
 Alice is the sender and the Bob is the recipient.
+.. TODO: What does the below mean?


it means she does not have to send an actual transaction to the blockchain - is that what you mean?

@chriseth I didn't write this, so it's not what 'I' mean, I was trying to understand what the original writer meant,

Alice only needs to send cryptographically signed messages off-chain
(e.g. via email) to Bob and it is similar to writing checks.

Just sounds kind of strange, but if it makes sense, then.

chriseth · 2018-07-18T14:26:05Z

docs/solidity-by-example.rst

-in a unusual way, instead of calling a function herself
-to initiate a payment, she will let Bob
-do that, and therefore pay the transaction fee.
+Alice and Bob use signatures to authorise transactions, which are a feature of smart contracts.


Hm, verifying signatures is something that can be done in smart contracts on Ethereum, but I would not call it a feature of smart contracts.

chriseth · 2018-07-18T14:27:22Z

docs/solidity-by-example.rst

-In this tutorial, we will sign messages in the browser
-using ``web3.js`` and ``MetaMask``.
-In particular, we will use the standard way described in `EIP-762 <https://github.com/ethereum/EIPs/pull/712>`_,
+Alice does not need to interact with the Ethereum network to sign the transaction, the process is completely offline.


Please try to change the newlines as little as possible (at least in such a large PR) as it makes the review much more complicated.

chriseth · 2018-07-18T14:28:23Z

docs/solidity-by-example.rst

+    /// Hashing first makes things easier
+    var hash = web3.utils.sha3("message to sign");
+    web3.eth.personal.sign(hash, web3.eth.defaultAccount, function () {
+      console.log("Signed")


Perhaps this should better also output the signature.

@chriseth In the original there was nothing inside the function and I added this mostly to know if it had worked. As the example adds to it later anyway, I'm thinking maybe just leave the function empty for now?

chriseth · 2018-07-18T14:30:11Z

docs/solidity-by-example.rst

-themselves, a so-called nonce, which is the number of transactions sent by an 
-account.
-The smart contract will check if a nonce is used multiple times.
+A replay attack is when a signed message is reused to claim authorisation for a second action. To avoid replay attacks we will use the same technique as in Ethereum transactions themselves, 'nonce', which is the number of transactions sent by an account. The smart contract will check if a nonce is used multiple times.


a so-called "nonce" sounds better to me.

@chriseth not too keen on that phrase, try this as an alternative.

chriseth · 2018-07-18T14:33:42Z

docs/solidity-by-example.rst

-`ecrecover <mathematical-and-cryptographic-functions>`_
-that accepts a message along with the ``r``, ``s`` and ``v`` parameters and
-returns the address that was used to sign the message.
+In general, ECDSA signatures consist of two parameters, ``r`` and ``s``. Signatures in Ethereum include a third parameter called ``v``, that you can use to recover which account's private key was used to sign the message, and the transaction's sender. Solidity provides a built-in function `ecrecover <mathematical-and-cryptographic-functions>`_ that accepts a message along with the ``r``, ``s`` and ``v`` parameters and returns the address that was used to sign the message.


This sounds a little like you would be able to recover the private key which is of course not possible. Also the transaction's sender can be derived from the public key, it is not something you could also recover using v.

chriseth · 2018-07-18T14:34:21Z

docs/solidity-by-example.rst

-We will use `inline assembly <assembly>`_ to do the job
-in the ``splitSignature`` function (the third function in the full contract
-at the end of this chapter).
+Signatures produced by web3.js are the concatenation of ``r``, ``s`` and ``v``, so the first step is to split these parameters apart. You can do this on the client-side, but doing it inside the smart contract means only one need to send one signature parameter needs rather than three. Splitting apart a byte array into component parts is a messy, so we use `inline assembly <assembly>`_ to do the job in the ``splitSignature`` function (the third function in the full contract at the end of this section).


"means only one need" reads a little weird

typo: "a messy"

chriseth · 2018-08-02T11:57:16Z

Please rebase.

ChrisChinchilla · 2018-08-03T07:37:13Z

@chriseth squashed and rebased

codecov · 2018-08-03T08:19:06Z

Codecov Report

Merging #4523 into develop will decrease coverage by 0.08%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4523      +/-   ##
===========================================
- Coverage     88.1%   88.02%   -0.09%     
===========================================
  Files          308      314       +6     
  Lines        31177    31782     +605     
  Branches      3744     3748       +4     
===========================================
+ Hits         27469    27976     +507     
- Misses        2455     2537      +82     
- Partials      1253     1269      +16

Flag	Coverage Δ
#all	`88.02% <ø> (-0.09%)`	⬇️
#syntax	`28.77% <ø> (-0.28%)`	⬇️

axic · 2018-08-03T13:14:40Z

docs/solidity-by-example.rst


 ::

    // this mimics the prefixing behavior of the eth_sign JSON-RPC method.
    function prefixed(hash) {
-        return ethereumjs.ABI.soliditySHA3(
+        return abi.soliditySHA3(


Why change this? The initial version is how the browser builds of ethereumjs work.

@axic Ah, because when I looked at the project docs here - https://github.com/ethereumjs/ethereumjs-abi that's how they showed it and I don't see any other examples showing the original syntax. I was aiming for consistency among the projects to reduce confusion. Unless I'm missing something here (which is highly possible), I can't find any reference to the original syntax. This also could just mean that the original syntax should also be documented better.

The original syntax is when using the browser builds from https://github.com/ethereumjs/browser-builds

axic · 2018-08-03T13:14:48Z

docs/solidity-by-example.rst

-        var split = ethereumjs.Util.fromRpcSig(signature);
-        var publicKey = ethereumjs.Util.ecrecover(message, split.v, split.r, split.s);
-        var signer = ethereumjs.Util.pubToAddress(publicKey).toString("hex");
+        var split = ethereumjsutil.fromRpcSig(signature);


Why change this? The initial version is how the browser builds of ethereumjs work.

@axic Pretty much the same as the comment above.

axic · 2018-08-03T13:15:35Z

docs/solidity-by-example.rst

-
-
+.. note::
+  The function ``splitSignature`` is very simple and does not use all security checks. A real implementation should use a more rigorously tested library, such as openzepplin's `version <https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/ECRecovery.sol>`_ of this code.


Why change it from manually broken lines?

There are two problems with this:

diffs are harder to see

it is painful in some editors

@axic The whole line break issue is a big conversation between writers and coders, but in all honestly it's mostly because I haven't worked on a code base for a while that hard enforced line breaks in docs and had just got out of the habit, fixed.

axic · 2018-09-19T13:01:27Z

docs/solidity-by-example.rst

-    web3.personal.sign(hash, web3.eth.defaultAccount, function () {...});
+    /// Hashing first makes things easier
+    var hash = web3.utils.sha3("message to sign");
+    web3.eth.personal.sign(hash, web3.eth.defaultAccount, function () {


I believe this is web3 1.x syntax. Should mention that above.

ChrisChinchilla · 2018-09-20T06:52:14Z

@axic Ahh, I was using Node (which made more sense to me). OK, I'll check the code in a browser example as I'd like to understand how it all works anyway.

ChrisChinchilla · 2018-09-20T07:16:42Z

OK @axic I reverted the code, I think I'll revisit this page soone though and make some of the examples clearer, or to be more precise, make it clearer how to write and run them.

ChrisChinchilla · 2018-09-20T07:16:42Z

OK @axic I reverted the code, I think I'll revisit this page soone though and make some of the examples clearer, or to be more precise, make it clearer how to write and run them.

axic · 2018-09-20T10:21:43Z

Please split the web3.js change into a separate PR. You are moving from the web3.js 0.x API to the 1.x API, and I believe the 0.x is still more common, given 1.x is not even released yet.

ChrisChinchilla · 2018-09-21T09:22:54Z

@axic I reverted to the original text submitted by the original poster, so I'm not sure what the web3.js change you're referring to is? Unless I missed something somewhere or misunderstand you?

ChrisChinchilla · 2018-10-05T10:03:18Z

Rebased and squashed. @chriseth I missed your comment (#4523 (comment)) and attempted to address it, but after some searching, I wasn't necessarily any clearer, and it seems that there are other issues to document this better anyway (#681). So I may address that separately when I look at that issue.

@axic still waiting for clarity on your comment (#4523 (comment))

Language tidy, add correct method and package namespaces and make more consistent with each project docs First changes from review Further fixes after review Fix line breaks Revert code changes Update

chriseth

I'm still not sure about the use of web3.js, but it's certainly better than before.

ChrisChinchilla requested review from chriseth and axic July 18, 2018 14:21

ChrisChinchilla added documentation 📖 in progress labels Jul 18, 2018

chriseth reviewed Jul 18, 2018

View reviewed changes

chriseth suggested changes Jul 18, 2018

View reviewed changes

axic removed the in progress label Aug 2, 2018

ChrisChinchilla force-pushed the micropayment-channel-example-polish branch 2 times, most recently from 6f3efa8 to d7e0193 Compare August 3, 2018 07:36

axic reviewed Aug 3, 2018

View reviewed changes

axic reviewed Sep 19, 2018

View reviewed changes

ChrisChinchilla force-pushed the micropayment-channel-example-polish branch from 45c1868 to 8a65d79 Compare October 5, 2018 10:01

Begin review of micropayments example

121d733

Language tidy, add correct method and package namespaces and make more consistent with each project docs First changes from review Further fixes after review Fix line breaks Revert code changes Update

ChrisChinchilla force-pushed the micropayment-channel-example-polish branch from 8a65d79 to 121d733 Compare November 14, 2018 16:14

chriseth approved these changes Nov 14, 2018

View reviewed changes

chriseth merged commit aaa5018 into develop Nov 14, 2018

axic deleted the micropayment-channel-example-polish branch November 14, 2018 16:52



		.. note::
		The function ``splitSignature`` is very simple and does not use all security checks. A real implementation should use a more rigorously tested library, such as openzepplin's `version <https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/ECRecovery.sol>`_ of this code.

Micropayment channel example polish #4523

Micropayment channel example polish #4523

Uh oh!

Conversation

ChrisChinchilla commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Aug 2, 2018

Uh oh!

ChrisChinchilla commented Aug 3, 2018

Uh oh!

codecov bot commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

axic Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisChinchilla commented Sep 20, 2018

Uh oh!

ChrisChinchilla commented Sep 20, 2018

Uh oh!

ChrisChinchilla commented Sep 20, 2018

Uh oh!

axic commented Sep 20, 2018

Uh oh!

ChrisChinchilla commented Sep 21, 2018

Uh oh!

ChrisChinchilla commented Oct 5, 2018

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Aug 3, 2018 •

edited

Loading

axic Aug 3, 2018 •

edited

Loading