Delayed Recovery Module Setup #107

pgev · 2019-02-19T12:08:23Z

Fixes #43

lib/helper/User.js

gulshanvasnani

Few inline comments.
Global comment :-
Provide documentation for the methods.Also,provide documentation for input parameters of the method.

lib/helper/User.js

gulshanvasnani · 2019-02-19T16:54:51Z

lib/helper/User.js

      sessionKeys,
      sessionKeysSpendingLimits,
      sessionKeysExpirationHeights
    );

-    const txReceipt = await new TxSender(txObject, oThis.auxiliaryWeb3, txOptions).execute();
+    const txSender1 = new TxSender(txObject, oThis.auxiliaryWeb3, txOptions);


txSender1 => txSender

package.json

gulshanvasnani · 2019-02-19T17:05:23Z

tools/initDevEnv.js

@@ -134,11 +140,11 @@ InitDevEnv.prototype = {
      `geth --datadir '${gethFolder}'` +
      ` --networkid ${setUpConfig.chain.networkId}` +
      ` --port ${setUpConfig.chain.geth.port}` +
-      ` --mine --minerthreads 1 --targetgaslimit ${setUpConfig.chain.gas} --gasprice 0x3B9ACA00` +
-      ` --rpc --rpcapi eth,net,web3,personal,txpool --rpcaddr ${setUpConfig.chain.geth.host} --rpcport ${
+      ` --fakepow --mine --minerthreads 16 --targetgaslimit ${setUpConfig.chain.gas} --gasprice 0x3B9ACA00` +


Why --fakepow is needed ?

if we actually use --mine then we should not actually mine on Travis nodes

--fakepow Disables proof-of-work verification

Two reasons:
- One has been also brought by Ben. We should not mine.
- Second was an attempt to cut tests running time. It cuts it by ~2. In this PR I've also set the difficulty to "0x0". Both together, cuts running time for me from 8mins to 3mins. If we want to have different setups, we should at least have configuration options and handle them in initDevEnv.js accordingly to have a faster running time during a development.

benjaminbollen

LGTM

benjaminbollen · 2019-02-19T18:04:47Z

test/integration/DelayedRecovery.js

+  let currentBlockNumber = await auxiliaryWeb3.eth.getBlockNumber();
+
+  while (currentBlockNumber < blockNumber) {
+    await sleep(2000);


this probably needs to be fixed later... ok for now to avoid creating a blocker.

Thanks. I've reduced time to 200 ms (was my original intention, did not notice that it's 2000). If you could share your proposal, maybe I could integrate into this PR also.

benjaminbollen

LGTM; missing documentation and other improvements can be done after alpha.1

benjaminbollen

LGTM

abhayks1

LGTM
There are few comments but I think that can be taken later as improvements.
However I have 1 question:

Can we know delayedRecoveryProxy address through event e.g. through UserWalletCreated or any other?
else we need to expose getModules() method in GnosisSafe interaction layer.
cc: @pgev

abhayks1 · 2019-02-20T11:07:48Z

test/integration/DelayedRecovery.js

+
+    const organizationOwnerAddress = deployerAddress;
+    const orgHelper = new OrganizationHelper(auxiliaryWeb3, null);
+    const orgConfig = {


OrganizationHelper integration needs to update with mosaic.js integration.
It's a note for me for the other PR #76

so #76 is not completed yet, correct? if so, can we add that as a comment on this issue?

Correct.
I have made above changes in PR #109

abhayks1 · 2019-02-20T11:07:55Z

test/integration/DelayedRecovery.js

+function signRecovery(recoveryModuleAddress, structTypeHash, prevOwner, oldOwner, newOwner, recoveryOwnerPrivateKey) {
+  const recoveryHash = hashRecoveryModuleRecovery(recoveryModuleAddress, structTypeHash, prevOwner, oldOwner, newOwner);
+
+  const signature = EthUtils.ecsign(EthUtils.toBuffer(recoveryHash), EthUtils.toBuffer(recoveryOwnerPrivateKey));


Signing could have been done with Mosaic.js EIP712Signer which other integration tests are consuming.
https://github.com/OpenSTFoundation/openst.js/blob/develop/test/integration/WalletOperations.js#L200
Improvement for later versions.

for now ok, but we should avoid having a logic function like hashRecoveryModuleRecovery l144 in the test files;

if there is not already a good EIP712 js library out there, then I am in favor of splitting the EIP712 signer out of mosaic.js

working on the JLP examples will gives us better clarity on the options for handling the signers outside of mosaic.js or openst.js

abhayks1 · 2019-02-20T11:17:09Z

test/integration/DelayedRecovery.js

+    gas: ConfigReader.gas
+  };
+
+  const modules = await gnosisSafeProxy.methods.getModules().call(txOptions);


Can we know delayedRecoveryProxy address through event e.g. through UserWalletCreated ?
else we need to expose getModules() method in GnosisSafe interaction layer.

there is value in exposing getModules for sure; let's open a ticket for that

the transaction receipt should include EnabledModule(module) as an event under the proxy contract address of GnosisSafe when setting up https://github.com/OpenSTFoundation/safe-contracts/blob/development/contracts/base/ModuleManager.sol#L43

Ticket created: #111

abhayks1 · 2019-02-20T15:09:52Z

lib/helper/User.js

    eip20Token,
    tokenRules,
-    userWalletFactoryAddress,
+    userWalletFactory,
+    proxyFactory,


Just realized since proxyFactory address are already being passed in constructor, we don't need to pass proxyFactory argument in createCompanyWallet method.

async createCompanyWallet( proxyFactory, owner, sessionKeys, sessionKeysSpendingLimits, sessionKeysExpirationHeights, txOptions )

We can avoid the duplication by removing proxyFactory argument from createCompanyWallet method.
I think it will be a quick change and can be fast forwarded to the release if everyone agrees.

Just want to make sure createCompanyWallet interface doesn't change much.

Paruyr Gevorgyan added 3 commits February 18, 2019 07:27

Recovery setup: deploy Recovery master copy and integrate with AddUser.

b3a5786

Cleaned up draft implementation and added more documentation.

bb7126c

Alpha implementation/test for delayed recovery.

8c53f60

pgev added the v0.10.0 label Feb 19, 2019

benjaminbollen reviewed Feb 19, 2019

View reviewed changes

lib/helper/User.js Show resolved Hide resolved

gulshanvasnani reviewed Feb 19, 2019

View reviewed changes

benjaminbollen approved these changes Feb 19, 2019

View reviewed changes

benjaminbollen reviewed Feb 19, 2019

View reviewed changes

benjaminbollen previously approved these changes Feb 19, 2019

View reviewed changes

Paruyr Gevorgyan added 3 commits February 20, 2019 08:12

Addressed review comments: more documentation.

15bdbc4

Merge with develop.

83b9cec

Merge with develop branch.

ec8d82b

pgev dismissed benjaminbollen’s stale review via ec8d82b February 20, 2019 09:02

benjaminbollen approved these changes Feb 20, 2019

View reviewed changes

benjaminbollen merged commit 402f865 into OpenST:develop Feb 20, 2019

abhayks1 reviewed Feb 20, 2019

View reviewed changes

abhayks1 mentioned this pull request Feb 20, 2019

Exposed getModules function in Gnosis Safe Interaction layer #111

Closed

abhayks1 reviewed Feb 20, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delayed Recovery Module Setup #107

Delayed Recovery Module Setup #107

pgev commented Feb 19, 2019

gulshanvasnani left a comment •

edited

Loading

gulshanvasnani Feb 19, 2019

pgev Feb 20, 2019

gulshanvasnani Feb 19, 2019

benjaminbollen Feb 19, 2019 •

edited

Loading

pgev Feb 20, 2019

benjaminbollen left a comment

benjaminbollen Feb 19, 2019

pgev Feb 20, 2019 •

edited

Loading

benjaminbollen left a comment •

edited

Loading

benjaminbollen left a comment

abhayks1 left a comment •

edited

Loading

abhayks1 Feb 20, 2019

benjaminbollen Feb 20, 2019 •

edited

Loading

abhayks1 Feb 20, 2019

abhayks1 Feb 20, 2019

benjaminbollen Feb 20, 2019 •

edited

Loading

abhayks1 Feb 20, 2019

benjaminbollen Feb 20, 2019

abhayks1 Feb 20, 2019

abhayks1 Feb 20, 2019 •

edited

Loading

Delayed Recovery Module Setup #107

Delayed Recovery Module Setup #107

Conversation

pgev commented Feb 19, 2019

gulshanvasnani left a comment • edited Loading

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

benjaminbollen Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgev Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

benjaminbollen left a comment • edited Loading

Choose a reason for hiding this comment

benjaminbollen left a comment

Choose a reason for hiding this comment

abhayks1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen Feb 20, 2019 • edited Loading

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

abhayks1 Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

gulshanvasnani left a comment •

edited

Loading

benjaminbollen Feb 19, 2019 •

edited

Loading

pgev Feb 20, 2019 •

edited

Loading

benjaminbollen left a comment •

edited

Loading

abhayks1 left a comment •

edited

Loading

benjaminbollen Feb 20, 2019 •

edited

Loading

benjaminbollen Feb 20, 2019 •

edited

Loading

abhayks1 Feb 20, 2019 •

edited

Loading