Skip to content
This repository has been archived by the owner on Nov 6, 2018. It is now read-only.

Init library owner to 0x0 #3

Merged
merged 7 commits into from Apr 13, 2018
Merged

Init library owner to 0x0 #3

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
55f9718
Init library to 0x0 whatever you pass to the constructor
5chdn Apr 12, 2018
f75dae2
Disallow msg.sender to become owner
5chdn Apr 12, 2018
737dc64
Maintain consistent case
5chdn Apr 12, 2018
3b60313
Add a library constructor
5chdn Apr 12, 2018
6813dec
Simplyfy constructor and initWallet
5chdn Apr 12, 2018
0a549c1
Restore previous comment
5chdn Apr 12, 2018
aae3c2b
Make initWallet internal
5chdn Apr 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 17 additions & 10 deletions contracts/walletLibrary.sol
View file
Open in desktop
Expand Up @@ -93,6 +93,14 @@ contract WalletLibrary is WalletEvents {
_;
}

// LIBRARY CONSTRUCTOR
// calls the `initWallet` method
function WalletLibrary() {
address[] noOwners;
noOwners.push(address(0x0));
initWallet(noOwners, 1, 0);
}

// METHODS

// gets called when no other function matches
Expand All @@ -104,14 +112,14 @@ contract WalletLibrary is WalletEvents {

// constructor is given number of sigs required to do protected "onlymanyowners" transactions
// as well as the selection of addresses capable of confirming them.
function initMultiowned(address[] _owners, uint _required) only_uninitialized {
m_numOwners = _owners.length + 1;
m_owners[1] = uint(msg.sender);
m_ownerIndex[uint(msg.sender)] = 1;
function initMultiowned(address[] _owners, uint _required) only_uninitialized internal {
require(_required > 0);
require(_owners.length >= _required);
Copy link

Choose a reason for hiding this comment

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

IMHO we should also have require(_required < _owners.length) check here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-)

m_numOwners = _owners.length;
for (uint i = 0; i < _owners.length; ++i)
{
m_owners[2 + i] = uint(_owners[i]);
m_ownerIndex[uint(_owners[i])] = 2 + i;
m_owners[1 + i] = uint(_owners[i]);
Copy link

Choose a reason for hiding this comment

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

What's stored in m_owners[0] then?

Copy link

Choose a reason for hiding this comment

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

Oh, I see that ownerIndex=0 is reserved to determine if the owner is actually defined. Would be good to add a comment to m_owners and m_numOwners (something like: "owners are stored on indices [1, m_numOwners], 0 is reserved for uninitialized owner".)

m_ownerIndex[uint(_owners[i])] = 1 + i;
}
m_required = _required;
}
Expand Down Expand Up @@ -198,7 +206,7 @@ contract WalletLibrary is WalletEvents {
}

// constructor - stores initial daily limit and records the present day's index.
function initDaylimit(uint _limit) only_uninitialized {
function initDaylimit(uint _limit) only_uninitialized internal {
Copy link

Choose a reason for hiding this comment

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

TBH since both initDayLimit and initMultiOwned are used only in constructor and are internal I would be inclined to inline them instead.
Also if we inline only_uninitialized modifier, we'll end-up with single method that initializes the wallet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a single-purpose contract and I do not want to change any of the contract structure unless it's required security-wise. This is important for me to (A) keep the changes in the upcoming EIP as minimal as possible and (B) to maintain maximum compatibility with the already deployed wallet stubs.

If anyone else wants to use a Wallet + WalletLibrary, they should refer to the refactored and peer-reviewed version of https://github.com/paritytech/contracts/pull/74.

m_dailyLimit = _limit;
m_lastDay = today();
}
Expand All @@ -214,8 +222,7 @@ contract WalletLibrary is WalletEvents {
// throw unless the contract is not yet initialized.
modifier only_uninitialized { if (m_numOwners > 0) throw; _; }

// constructor - just pass on the owner array to the multiowned and
// the limit to daylimit
// set the wallet owner to 0x0 whatever you pass to the function
function initWallet(address[] _owners, uint _required, uint _daylimit) only_uninitialized {
initDaylimit(_daylimit);
initMultiowned(_owners, _required);
Expand Down Expand Up @@ -398,7 +405,7 @@ contract WalletLibrary is WalletEvents {
contract Wallet is WalletEvents {

// WALLET CONSTRUCTOR
// calls the `initWallet` method of the Library in this context
// calls the `initWallet` method of the Library in this context
function Wallet(address[] _owners, uint _required, uint _daylimit) {
// Signature of the Wallet Library's init function
bytes4 sig = bytes4(sha3("initWallet(address[],uint256,uint256)"));
Expand Down