modular-network / ethereum-libraries-wallet

Wallet Library to create multisig wallets
7 stars 1 forks source link

Complex conditional paths #10

Open joshuahannan opened 6 years ago

joshuahannan commented 6 years ago

WalletMainLib's line 353 will throw if msg.sender is not a registered owner of the wallet. This line of code is used in several other places in the code. A good practice in Solidity is to extract such conditional checks into early return modifiers. In this particular case, an onlyOwner modifier would be appropriate. Similarly, other modifiers like transactionExists, transactionNotConfirmed, etc. could also be employed throughout. In the current state of the code, no modifiers are used. Solidity modifiers and early returns are fundamental tools of a programming pattern sometimes referred to as “Condition Oriented Programming” (COP), which makes code easier to follow, less susceptible to mistakes, and more robust. The idea behind COP is to avoid multiple level conditional trees in favor of a set of enumerable bail out conditions. See Gavin Wood’s great post describing this idea in more detail. Consider making use of Solidity’s modifiers and restructuring multi-level conditional structures into COP type early returns.

joshuahannan commented 6 years ago

Not sure what we want to do about this. I don't think we need modifiers because of reasons we've discussed before, but they make some good points