chore(Docs): Replaced instances if - throw to require() where applicable. by InfamousVague · Pull Request #2134 · argotorg/solidity

InfamousVague · 2017-04-19T18:34:27Z

chore(Docs): Replaced instances if - throw to require() where applicable.

InfamousVague · 2017-04-19T18:37:09Z

Looks like the test is failing because of a broken function trying to convert a line ending, is this my fault?

axic · 2017-04-19T23:36:01Z

docs/common-patterns.rst

-                if (!richest.send(msg.value)) {
-                    throw;
-                }
+                require(richest.send(msg.value));


I think this should be assert() here.

Perhaps we could also have a comment to say that the use of richest.transfer(msg.value) is preferred instead.

axic · 2017-04-19T23:36:41Z

docs/contracts.rst

            // "instance" will be the current contract.
-            if (!Set.insert(knownValues, value))
-                throw;
+            require(Set.insert(knownValues, value));


I think this should be assert.

axic · 2017-04-19T23:37:22Z

docs/security-considerations.rst

          shares[msg.sender] = 0;
-          if (!msg.sender.send(share))
-              throw;
+          require(msg.sender.send(share));


This is the withdraw pattern, in here probably we should prefer transfer() instead.

axic · 2017-04-19T23:37:41Z

docs/security-considerations.rst

-            if (tx.origin != owner) { throw; }
-            if (!dest.call.value(amount)()) throw;
+            require(tx.origin == owner);
+            require(dest.call.value(amount)());


Could be replaced with .transfer().

@axic 125 - 130 will need to be updated to reflect this change, I am not confident that I understand deeply enough to replace this paragraph; would you mind taking a look and if we still want to switch to transfer() let me know what you'd like me to replace that paragraph with.

Will make the change.

axic · 2017-04-19T23:39:07Z

I guess no point to repeat the same. Where the code explains why .send() or .call() is problematic, those should be used, but I think in any other case we should stick to .transfer().

Note, that by moving to .transfer() (and require() / assert(), the pragma solidity must be updated to reflect the version those features were added in.

axic · 2017-04-20T11:14:17Z

docs/types.rst

            c.amount = 0;
-            if (!c.beneficiary.send(amount))
-                throw;
+            require(c.beneficiary.transfer(amount));


transfer() will abort if it failed, no need for require() here

chriseth · 2017-04-20T11:40:19Z

docs/common-patterns.rst

@@ -86,9 +86,7 @@ This is as opposed to the more intuitive sending pattern:
                // Check if call succeeds to prevent an attacker


This comment is not relevant anymore. If you are not confident with making the relevant changes, then please just keep send and we will go over it in another iteration.

Okay I'll leave that in for now.

chriseth · 2017-04-20T11:40:49Z

docs/common-patterns.rst


        modifier onlyAfter(uint _time) {
-            if (now < _time) throw;
+            require(now > _time);


Should be >= to have identical semantics.

Changing now but wouldn't that signify it allows something to happen also at a given time and not just after a given time.

chriseth · 2017-04-20T11:41:01Z

docs/common-patterns.rst

        modifier costs(uint _amount) {
-            if (msg.value < _amount)
-                throw;
+            require(msg.value > _amount);


Also >= here.

chriseth · 2017-04-20T11:42:19Z

docs/contracts.rst

            // "instance" will be the current contract.
-            if (!Set.insert(knownValues, value))
-                throw;
+            assert(Set.insert(knownValues, value));


I think even here, we should use require because it is not an internal error if this fails - it is still invalid usage of contract C.

Wasn't the definition that require() is for input validation, while assert() is for everything else?

Let me know which is preferred and I'll update @chriseth @axic

The definition was that it is impossible to get a contract to trigger an assert unless it has a bug.

chriseth · 2017-05-02T12:17:17Z

Replaced this by #2207

Matt Wisniewski added 8 commits April 19, 2017 14:12

Update common-patterns.rst

ffe4211

Update contracts.rst

018c199

Update frequently-asked-questions.rst

2053f50

Update security-considerations.rst

3bdd9d8

Update structure-of-a-contract.rst

fa71aea

Update types.rst

5f97df1

Update solidity-by-example.rst

3359baa

Merge pull request #1 from wski/requireUpdates

e44efc8

chore(Docs): Replaced instances if - throw to require() where applicable.

LianaHus added the needs review label Apr 19, 2017

axic reviewed Apr 19, 2017

View reviewed changes

axic closed this Apr 19, 2017

axic removed the needs review label Apr 19, 2017

axic reopened this Apr 19, 2017

axic added the in progress label Apr 19, 2017

Matt Wisniewski added 10 commits April 19, 2017 19:44

Update common-patterns.rst

50864cd

Update contracts.rst

bee1d35

Update types.rst

e08f8a6

Update solidity-by-example.rst

9e1682f

Update types.rst

32a8676

Update solidity-by-example.rst

d4861e1

Update contracts.rst

8f155ff

Update security-considerations.rst

3dcbd62

Update structure-of-a-contract.rst

7126520

Update types.rst

999f6e6

axic reviewed Apr 20, 2017

View reviewed changes

chriseth reviewed Apr 20, 2017

View reviewed changes

Matt Wisniewski added 3 commits April 20, 2017 11:13

Update common-patterns.rst

3941df5

Update types.rst

230a272

Update solidity-by-example.rst

ef0c5d7

chriseth mentioned this pull request May 2, 2017

chore(Docs): Replaced instances if - throw to require() where applicable. #2207

Merged

chriseth self-assigned this May 2, 2017

chriseth closed this May 2, 2017

chriseth removed the in progress label May 2, 2017

		@@ -86,9 +86,7 @@ This is as opposed to the more intuitive sending pattern:
		// Check if call succeeds to prevent an attacker

Conversation

InfamousVague commented Apr 19, 2017

Uh oh!

InfamousVague commented Apr 19, 2017

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!

axic commented Apr 19, 2017

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 May 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants