This repository has been archived by the owner. It is now read-only.

Init library owner to 0x0 #3

Merged
merged 7 commits into from Apr 13, 2018

Conversation

Projects
None yet
4 participants
@5chdn
Copy link
Contributor

5chdn commented Apr 12, 2018

  • adds a constructor for the library that sets owner always to 0x0
  • msg.sender is not allowed to become an owner by default anymore
  • make initWallet, initMultiowned, and initDaylimit internal functions to disallow direct access

ref paritytech/contracts#74

5chdn added some commits Apr 12, 2018

@ngotchac
Copy link

ngotchac left a comment

LGTM, but you might want to call initWallet directly in the WalletLibrary contructor.

@5chdn

This comment has been minimized.

Copy link
Contributor Author

5chdn commented Apr 12, 2018

There is no WalletLibrary constructor, or am I blind? Shall I add one?

@5chdn

This comment has been minimized.

Copy link
Contributor Author

5chdn commented Apr 12, 2018

Also does resetting initWallet to 0x0 break Wallet functionality? Shouldn't, right?

@ngotchac

This comment has been minimized.

Copy link

ngotchac commented Apr 12, 2018

Yep you should create it. And no it shouldn't, as it will only write to the Library's storage, so the other wallets will just use the logic but their own storage.

@5chdn

This comment has been minimized.

Copy link
Contributor Author

5chdn commented Apr 12, 2018

All right, added a constructor that calls initWallet().

address[] noOwners;
noOwners.push(address(0x0));
initDaylimit(0);
initMultiowned(noOwners, 1);

This comment has been minimized.

@ngotchac

ngotchac Apr 12, 2018

I would actually put this bit in the constructor, and keep initWallet as it was.

This comment has been minimized.

@5chdn

5chdn Apr 12, 2018

Author Contributor

Makes sense.

5chdn added some commits Apr 12, 2018

@@ -216,7 +224,7 @@ contract WalletLibrary is WalletEvents {

// constructor - just pass on the owner array to the multiowned and
// the limit to daylimit
function initWallet(address[] _owners, uint _required, uint _daylimit) only_uninitialized {
function initWallet(address[] _owners, uint _required, uint _daylimit) only_uninitialized internal {

This comment has been minimized.

@andresilva

andresilva Apr 12, 2018

I think this will make it impossible for the Wallet constructor to run, which means it will be impossible to deploy new Wallets. Not sure if this was intended but I guess it's actually a good thing.

This comment has been minimized.

@tomusdrw

tomusdrw Apr 13, 2018

We should remove the Wallet constructor then or rather change it to require(false) otherwise it's really misleading.

This comment has been minimized.

@5chdn

5chdn Apr 13, 2018

Author Contributor

Yes, we do not want anyone to deploy new wallets, at least not with this code.

This comment has been minimized.

@5chdn

5chdn Apr 13, 2018

Author Contributor

Addressed in #5

@@ -216,7 +224,7 @@ contract WalletLibrary is WalletEvents {

// constructor - just pass on the owner array to the multiowned and
// the limit to daylimit
function initWallet(address[] _owners, uint _required, uint _daylimit) only_uninitialized {
function initWallet(address[] _owners, uint _required, uint _daylimit) only_uninitialized internal {

This comment has been minimized.

@tomusdrw

tomusdrw Apr 13, 2018

We should remove the Wallet constructor then or rather change it to require(false) otherwise it's really misleading.

@@ -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 {

This comment has been minimized.

@tomusdrw

tomusdrw Apr 13, 2018

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.

This comment has been minimized.

@5chdn

5chdn Apr 13, 2018

Author Contributor

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 paritytech/contracts#74.

m_ownerIndex[uint(msg.sender)] = 1;
function initMultiowned(address[] _owners, uint _required) only_uninitialized internal {
require(_required > 0);
require(_owners.length >= _required);

This comment has been minimized.

@tomusdrw

tomusdrw Apr 13, 2018

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

This comment has been minimized.

@5chdn

5chdn Apr 13, 2018

Author Contributor

:-)

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]);

This comment has been minimized.

@tomusdrw

tomusdrw Apr 13, 2018

What's stored in m_owners[0] then?

This comment has been minimized.

@tomusdrw

tomusdrw Apr 13, 2018

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".)

@5chdn

This comment has been minimized.

Copy link
Contributor Author

5chdn commented Apr 13, 2018

Remove Wallet in #5, left comments on the other suggestions.

@5chdn 5chdn merged commit 507fc5a into master Apr 13, 2018

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
buildkite/parity-contracts-slash-0x863df6bfa4 Build #9 passed (3 seconds)
Details

@5chdn 5chdn deleted the a5-init-library branch Apr 13, 2018

@maurelian maurelian referenced this pull request Apr 18, 2018

Open

Add tests #6

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.