Skip to content

Eliminate throw() per issue #44. #61

New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Eliminate throw() per issue #44. #61

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
53c72f5
Eliminate throw() per issue #44.
timloh-gtoken Sep 11, 2017
2e32e1c
Merge remote-tracking branch 'origin/master' into feat/eliminate-throw
timloh-gtoken Sep 15, 2017
af7f0f2
Eliminate throw from CrowdsaleToken.sol.
Sep 15, 2017
a6e94f1
Eliminate throw from GnosisWallet.sol.
Sep 15, 2017
6e5a19c
Changes as requested by @villesundell.
cheetimloh Oct 15, 2017
2c6668f
Merge commit '7d4a5ab837e23068f1678299dfa9d975f53397b9' into feat/eli…
cheetimloh Oct 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions contracts/CrowdsaleToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ contract CrowdsaleToken is ReleasableToken, MintableToken, UpgradeableToken {
function CrowdsaleToken(string _name, string _symbol, uint _initialSupply, uint _decimals, bool _mintable)
UpgradeableToken(msg.sender) {

// Cannot create a token without supply and no minting
require(_mintable || _initialSupply != 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should probably be moved to the same place the "throw" used to be? Minting should be optional, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The require() ensures that token is either mintable or is pre-allocated an initial supply so minting is optional as long as initial supply of tokens has been pre-allocated. Hence, I think start of function is an appropriate place for the require().


// Create any address, can be transferred
// to team multisig via changeOwner(),
// also remember to call setUpgradeMaster()
Expand All @@ -70,9 +73,6 @@ contract CrowdsaleToken is ReleasableToken, MintableToken, UpgradeableToken {
// No more new supply allowed after the token creation
if(!_mintable) {
mintingFinished = true;
if(totalSupply == 0) {
throw; // Cannot create a token without supply and no minting
}
}
}

Expand Down
36 changes: 13 additions & 23 deletions contracts/GnosisWallet.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,59 +36,50 @@ contract MultiSigWallet {
}

modifier onlyWallet() {
if (msg.sender != address(this))
throw;
require(msg.sender == address(this));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Gnosis, let's just use fresh upstream file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_;
}

modifier ownerDoesNotExist(address owner) {
if (isOwner[owner])
throw;
require(!isOwner[owner]);
_;
}

modifier ownerExists(address owner) {
if (!isOwner[owner])
throw;
require(isOwner[owner]);
_;
}

modifier transactionExists(uint transactionId) {
if (transactions[transactionId].destination == 0)
throw;
require(transactions[transactionId].destination != 0);
_;
}

modifier confirmed(uint transactionId, address owner) {
if (!confirmations[transactionId][owner])
throw;
require(confirmations[transactionId][owner]);
_;
}

modifier notConfirmed(uint transactionId, address owner) {
if (confirmations[transactionId][owner])
throw;
require(!confirmations[transactionId][owner]);
_;
}

modifier notExecuted(uint transactionId) {
if (transactions[transactionId].executed)
throw;
require(!transactions[transactionId].executed);
_;
}

modifier notNull(address _address) {
if (_address == 0)
throw;
require(_address != 0);
_;
}

modifier validRequirement(uint ownerCount, uint _required) {
if ( ownerCount > MAX_OWNER_COUNT
|| _required > ownerCount
|| _required == 0
|| ownerCount == 0)
throw;
require(ownerCount <= MAX_OWNER_COUNT
&& _required <= ownerCount
&& _required != 0
&& ownerCount != 0);
_;
}

Expand All @@ -111,8 +102,7 @@ contract MultiSigWallet {
validRequirement(_owners.length, _required)
{
for (uint i=0; i<_owners.length; i++) {
if (isOwner[_owners[i]] || _owners[i] == 0)
throw;
require(!isOwner[_owners[i]] && _owners[i] != 0);
isOwner[_owners[i]] = true;
}
owners = _owners;
Expand Down