Eliminate throw() per issue #44. by timloh-gtoken · Pull Request #61 · TokenMarketNet/smart-contracts

timloh-gtoken · 2017-09-11T09:02:05Z

No description provided.

miohtama · 2017-09-15T14:20:50Z

Thanks. Is this ready to merge?

timloh-gtoken · 2017-09-15T17:47:37Z

Yes, this is ready to merge, thanks.

miohtama · 2017-09-15T21:21:09Z

contracts/GnosisWallet.sol

    modifier onlyWallet() {
-        if (msg.sender != address(this))
-            throw;
+        require(msg.sender == address(this));


For Gnosis, let's just use fresh upstream file

Will use upstream file once upstream has merged my pull request gnosis/MultiSigWallet#12 that eliminates throw and other warnings, thanks.

namanyayg · 2017-10-10T15:32:13Z

Hi, any reason this has not been merged? Looking to launch an ICO and would prefer not using deprecated features. Are there any issues with the pull request?

miohtama · 2017-10-10T15:48:14Z

No particular reason besides lack of capacity in review pipeline. I'll have mycoworker @villesundell to review this, merge tomorrow. Please if you need anything, @namanyayg, join our Gitter chat.

villesundell

The overall quality is high, thank you for contributing this!

villesundell · 2017-10-12T11:19:24Z

contracts/test/TestMigrationTarget.sol


  function() public payable {
-    throw;
+    require(false);


I think that we should use revert().. ...if this would be a normal function. However, in this case I think we can just leave an empty function and remove the "payable" modifier?

Removed "payable" modifier and require() to leave an empty function.

villesundell · 2017-10-12T11:21:17Z

contracts/UpgradeableToken.sol

  function setUpgradeMaster(address master) public {
-      if (master == 0x0) throw;
-      if (msg.sender != upgradeMaster) throw;
+      require(master != 0x0 && msg.sender == upgradeMaster);


I think "semantically" these should be two separate require()s?

Changed to two separate require()s.

villesundell · 2017-10-12T11:25:04Z

contracts/UpgradeableToken.sol

-        // Called in a bad state
-        throw;
-      }
+      require(state == UpgradeState.ReadyToUpgrade || state == UpgradeState.Upgrading); // Called in a bad state


I am not sure should these be two separate require()s, afterall those are dealing with the same variable "state".

Since they are dealing with the same data, think better to use same require().

villesundell · 2017-10-12T11:57:44Z

contracts/TokenTranchePricing.sol

  }

  function() payable {
-    throw; // No money on this contract


Same as with TestMigrationTarget.sol: I suppose we can have this as an empty function, and remove the "payable" modifier.

Removed "payable" modifier and require() resulting in empty function.

villesundell · 2017-10-12T11:59:50Z

contracts/TokenTranchePricing.sol

-    if(_tranches.length % 2 == 1 || _tranches.length >= MAX_TRANCHES*2) {
-      throw;
-    }
+    require((_tranches.length % 2 == 0) && (_tranches.length < MAX_TRANCHES*2));


Should we have two require()s here? (I am not sure, since we are working with the same variable)

Since they are dealing with the same data, think better to use same require(). Have changed condition to _tranches.length <= MAX_TRANCHES * 2 similar to EthTranchePricing.sol.

villesundell · 2017-10-12T20:34:34Z

contracts/BonusFinalizeAgent.sol

  uint public allocatedBonus;

  function BonusFinalizeAgent(CrowdsaleToken _token, Crowdsale _crowdsale, uint _bonusBasePoints, address _teamMultisig) {
+    require(address(_crowdsale) != 0 && address(_teamMultisig) != 0);


Should these be two separate require()s?

Thanks for the compliments, changed to two separate require()s as requested.

villesundell · 2017-10-12T20:38:27Z

contracts/Crowdsale.sol

   */
  function() payable {
-    throw;
+    require(false);


We could remove the "payable" modifier and the require() to make an empty function (In my opinion the function should be stay there anyway to signal that this is intentional)

Removed "payable" modifier and require() to make empty function as requested.

villesundell · 2017-10-12T20:39:29Z

contracts/Crowdsale.sol

    } else {
      // Unwanted state
-      throw;
+      assert(false);


This should be probably revert()?

Changed to revert() as requested.

villesundell · 2017-10-12T20:40:57Z

contracts/Crowdsale.sol


    // Pocket the money
-    if(!multisigWallet.send(weiAmount)) throw;
+    require(multisigWallet.send(weiAmount));


We could use .transfer() instead of .send(), then require() would not be needed.

Changed to use .transfer() as requested.

villesundell · 2017-10-12T20:42:29Z

contracts/Crowdsale.sol

    weiRefunded = weiRefunded.plus(weiValue);
    Refund(msg.sender, weiValue);
-    if (!msg.sender.send(weiValue)) throw;
+    require(msg.sender.send(weiValue));


We could use .transfer() instead of .send(), eliminating the need for require()

Changed to use .transfer() as requested.

…minate-throw # Conflicts: # contracts/MilestonePricing.sol

miohtama · 2017-11-27T20:50:33Z

This is still pending, but slowly being merged to the master bit by bit.

timloh-gtoken added 2 commits September 11, 2017 11:51

Eliminate throw() per issue TokenMarketNet#44.

53c72f5

Merge remote-tracking branch 'origin/master' into feat/eliminate-throw

2e32e1c

Tim Loh added 2 commits September 16, 2017 02:15

Eliminate throw from CrowdsaleToken.sol.

af7f0f2

Eliminate throw from GnosisWallet.sol.

a6e94f1

miohtama reviewed Sep 15, 2017

View reviewed changes

villesundell suggested changes Oct 12, 2017

View reviewed changes

cheetimloh added 2 commits October 16, 2017 01:20

Changes as requested by @villesundell.

6e5a19c

Merge commit '7d4a5ab837e23068f1678299dfa9d975f53397b9' into feat/eli…

2c6668f

…minate-throw # Conflicts: # contracts/MilestonePricing.sol

Conversation

timloh-gtoken commented Sep 11, 2017

Uh oh!

miohtama commented Sep 15, 2017

Uh oh!

timloh-gtoken commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namanyayg commented Oct 10, 2017

Uh oh!

miohtama commented Oct 10, 2017

Uh oh!

villesundell left a comment

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!

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!

miohtama commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

miohtama commented Nov 27, 2017 •

edited

Loading