Tips Wallet #35

jksf · 2017-11-22T21:59:05Z

Closes #23
Replaces #29 with more concise and future-proof implementation.

Checklist:

initial setup of owners
cheap fallback payable function
emit event on deposit
arbitrary transaction execution
change owners (required approval of all existing owners)
reset owners (approval of one of the owners with possibility to cancel request in a configured time window)
write security tests
write nonce duplication tests
run tests in modularized way

I smuggled also some typing and utils refactoring.

jksf · 2017-12-07T22:24:49Z

That was a lot of work. Added 2210 and removed 401 lines of code (excluding JSON ABI files), 237 unit/integration tests, not counting 192 skipped tests due to base class tests duplication.
I hope the funds received on this wallet will be worth it ;-).

Commits are decently organized, however I suggest reviewing it in final form.

biern

Impressive work!

I couldn't find anything wrong with the contracts. I will take a look at them once again tomorrow to double check.
It looks like you have found a nice solution to tests with parametrized contexts. Also the tests are very comprehensive.
Naming of the classes / instances related to the commands was a bit confusing for me on the first read, I've suggested some improvements (hopefully), although the general concept is neat.
Most of the comments are just minor remarks related mostly to naming, I couldn't find anything wrong here.

biern · 2017-12-09T17:18:22Z

contracts/MultiSig/MultiSig.sol

+    }
+
+    /* solhint-disable no-empty-blocks */
+    function() public payable {}


Maybe it makes sense to have a Deposit event here?

Good suggestion. I had it initially but removed it due to doubts about "Deposit" naming in context of TipsWallet. Nonetheless, it should exist in the MultiSig base contract if we want it to be practical and used in other contexts.
Added it in commit 6b9525a.

biern · 2017-12-09T17:19:12Z

contracts/MultiSig/MultiSig.sol

+    /* solhint-disable no-empty-blocks */
+
+    function listOwners() public view returns (address[]) {
+        return owners;


address is public but it has a getter. It should be made private than.

I did it consciously. owners default getter works with indices and returns a single address at given index. So it is a separate use case. I am not sure whether the default getter is useful, I have mixed feelings about default getters for arrays in Solidity. But I thought it wouldn't hurt to leave it as it is.

biern · 2017-12-09T17:21:48Z

contracts/MultiSig/MultiSig.sol

+        bytes data
+    )
+        public
+        onlySigned(v, r, s, keccak256(byte(0x19), byte(0), this, destination, value, data, nonce))


So, where does 0x19 and byte(0) come from?

It follows ERC191 specification (ethereum/EIPs#191).
0x19 <1 byte version> <version specific data> <data to sign>

Good to know that, thanks

biern · 2017-12-09T17:27:31Z

contracts/MultiSig/RecoverableMultiSig.sol

+
+contract RecoverableMultiSig is TransferableMultiSig {
+
+    uint256 public recoveryConfirmations;


This name is a bit misleading, maybe recoveryBlockOffset?

Fixed in 5bbc32b

biern · 2017-12-09T17:31:44Z

contracts/MultiSig/RecoverableMultiSig.sol

+import "./TransferableMultiSig.sol";
+
+
+contract RecoverableMultiSig is TransferableMultiSig {


Am I correct that RecoverableMultiSig does not depend on TransferableMultiSig but just MultiSig?
I'm just asking, this is still just fine for our use case.

Yes, you are correct. I flattened it in e3af2c4.
Subconsciously I was worried about Diamond Problem in multiple base classes inheritance, but it is non-existent in Solidity.

biern · 2017-12-09T18:14:06Z

test/MultiSig/transferablemultisig.test.ts

+);
+
+contract('TransferableMultiSigContract', accounts => {
+  describe.skip('#ctor', () => {


Skip usage already commented in other place

biern · 2017-12-09T18:14:18Z

test/MultiSig/transferablemultisig.test.ts

+
+  const ownerSets = [[accounts[2]], accounts.slice(1, 3), accounts.slice(2, 6)];
+  ownerSets.map(owners => {
+    context.skip(`When wallet has ${owners.length} owners`, () => {


Skip usage already commented in other place

biern · 2017-12-09T18:15:14Z

test/MultiSig/transferablemultisig.test.ts

+      });
+
+      describe('#ctor', () => {
+        it('should set owners', async () => {


set -> list

Fixed in 748887e

biern · 2017-12-09T18:17:44Z

test/tipswallet.test.ts

+      });
+
+      describe('#ctor', () => {
+        it('should set owners', async () => {


list. This is duplicated, it can be extracted to a testListOwners(ctx) method

Fixed in 748887e

biern · 2017-12-09T18:18:15Z

types/truffle-global.d.ts

-  const artifacts: SignHashArtifacts;
-  const assert: Chai.AssertStatic;
-  const contract: ContractContextDefinition;
-}


I'm not sure why have you removed that.

There are two problems with this approach:

Not in all contexts these variables are defined (e.g. seed)
WebStorm does not recognize them (pretty lousy argument, but I heavily rely on this IDE)

jksf · 2017-12-09T23:12:29Z

@biern Thanks for a thorough review. I fixed all the things you pointed out except the declaration of global variables. I will reconsider changing it when WebStorm supports it. But still, there is an issue that not in all context these variables are available (for example in custom scripts, seed).
Let's keep it as it is (for now), it's more verbose but does not hurt.

biern · 2017-12-10T12:54:17Z

I've took a look at the contracts again today and still couldn't find any issues. Outstanding job, I'm merging this.

Jakub Stefanski added 2 commits November 22, 2017 22:48

Wallet sets owners and accepts deposits

7030436

I smuggled also some typing and utils refactoring.

Implement multisig using ecrecover

8bebf81

jksf added the work in progress label Nov 22, 2017

jksf requested a review from biern November 22, 2017 21:59

jksf mentioned this pull request Nov 22, 2017

Tips wallet #29

Closed

9 tasks

Jakub Stefanski added 22 commits November 22, 2017 23:19

Fix code inspection issues

476538b

Refactor multisig lib

0ba611c

Transfer ERC20 tokens from wallet

9c472fe

Refactor MultiSig TypeScript libs

f04f302

Test multiple wallet transactions at once

23915e1

Add indices to Wallet events

76ac01c

Test repeated Wallet transactions

d2102c5

Clean Wallet tests

7a410c1

Test Wallet security

5233291

Implement changing owners of Wallet

2f63c1a

Test replay and nonce reuse attacks on wallet

56a818f

Refactor TipsWallet and reorganize files

df5e4af

Implement Recoverable MultiSig

86faacc

Reorganize MultiSig files

9a9a5f9

Make recovery confirmations configurable

61c67d4

Write RecoverableMultiSig tests and add events

a434fb9

Modularize RecoverableMultiSigTests

8b9841f

Run recovery tests for TipsWallet

fa7b8af

Move multisig test context to separate file

ee67b15

Modularize TransferableMultiSig

ff5045a

Modularize MultiSig tests

7006b9a

Skip duplicated base classes tests

e6ac358

jksf removed the work in progress label Dec 7, 2017

Remove test context duplication

bec4fbe

biern reviewed Dec 9, 2017

View reviewed changes

Jakub Stefanski added 8 commits December 9, 2017 21:45

Add Deposited event to MultiSig

6b9525a

Do not skip MultiSig base class tests

e9b9b1f

Flatten RecoverableMultiSig inheritance

e3af2c4

Add scripts for testing selected contracts

0c61614

Rename MultiSigCommand to MultiSigTransaction

193df12

Refactor multisig and tests

d4ce6a3

Rename recoveryConfirmations to recoveryBlockOffset

5bbc32b

Remove listOwners test duplication

748887e

biern merged commit ced0929 into SignHash:master Dec 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tips Wallet #35

Tips Wallet #35

jksf commented Nov 22, 2017 •

edited

Loading

jksf commented Dec 7, 2017 •

edited

Loading

biern left a comment

biern Dec 9, 2017

jksf Dec 9, 2017

biern Dec 9, 2017

jksf Dec 9, 2017

biern Dec 9, 2017

jksf Dec 9, 2017

biern Dec 10, 2017

biern Dec 9, 2017

jksf Dec 9, 2017

biern Dec 9, 2017

jksf Dec 9, 2017

biern Dec 9, 2017

jksf Dec 9, 2017

biern Dec 9, 2017

jksf Dec 9, 2017

biern Dec 9, 2017

jksf Dec 9, 2017

biern Dec 9, 2017

jksf Dec 9, 2017

biern Dec 9, 2017

jksf Dec 9, 2017

jksf commented Dec 9, 2017

biern commented Dec 10, 2017


		contract RecoverableMultiSig is TransferableMultiSig {

		uint256 public recoveryConfirmations;

		import "./TransferableMultiSig.sol";


		contract RecoverableMultiSig is TransferableMultiSig {

Tips Wallet #35

Tips Wallet #35

Conversation

jksf commented Nov 22, 2017 • edited Loading

jksf commented Dec 7, 2017 • edited Loading

biern 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

Choose a reason for hiding this comment

jksf commented Dec 9, 2017

biern commented Dec 10, 2017

jksf commented Nov 22, 2017 •

edited

Loading

jksf commented Dec 7, 2017 •

edited

Loading