Eliminate throw() per issue #44. #61

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

@@ -36,59 +36,50 @@ contract MultiSigWallet {
    }

    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

@@ -43,7 +39,7 @@ contract TestMigrationTarget is StandardToken, UpgradeAgent {
  }

  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

@@ -121,8 +116,7 @@ contract UpgradeableToken is StandardToken {
   * This allows us to set a new owner for the upgrade mechanism.
   */
  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

@@ -150,7 +144,7 @@ contract TokenTranchePricing is PricingStrategy, Ownable {
  }

  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

@@ -35,17 +35,10 @@ contract BonusFinalizeAgent is FinalizeAgent {
  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

@@ -163,7 +152,7 @@ contract Crowdsale is Haltable {
   * Don't expect to just send in money and get tokens.
   */
  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 if(getState() == State.Funding) {
      // Retail participants can only come in when the crowdsale is running
      // pass
    } 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


    assignTokens(receiver, tokenAmount);

    // 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

    investedAmountOf[msg.sender] = 0;
    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate throw() per issue #44. #61

Eliminate throw() per issue #44. #61

timloh-gtoken commented Sep 11, 2017

miohtama commented Sep 15, 2017

timloh-gtoken commented Sep 15, 2017

miohtama Sep 15, 2017

timloh-gtoken Sep 16, 2017

namanyayg commented Oct 10, 2017

miohtama commented Oct 10, 2017

villesundell left a comment

villesundell Oct 12, 2017

timloh-gtoken Oct 15, 2017

villesundell Oct 12, 2017

timloh-gtoken Oct 15, 2017

villesundell Oct 12, 2017

timloh-gtoken Oct 15, 2017

villesundell Oct 12, 2017

timloh-gtoken Oct 15, 2017

villesundell Oct 12, 2017

timloh-gtoken Oct 15, 2017

villesundell Oct 12, 2017

timloh-gtoken Oct 15, 2017

villesundell Oct 12, 2017

timloh-gtoken Oct 15, 2017

villesundell Oct 12, 2017

timloh-gtoken Oct 15, 2017

villesundell Oct 12, 2017

timloh-gtoken Oct 15, 2017

villesundell Oct 12, 2017

timloh-gtoken Oct 15, 2017

miohtama commented Nov 27, 2017 •

edited

Eliminate throw() per issue #44. #61

Are you sure you want to change the base?

Eliminate throw() per issue #44. #61

Conversation

timloh-gtoken commented Sep 11, 2017

miohtama commented Sep 15, 2017

timloh-gtoken commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namanyayg commented Oct 10, 2017

miohtama commented Oct 10, 2017

villesundell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miohtama commented Nov 27, 2017 • edited

miohtama commented Nov 27, 2017 •

edited