Individuell proposer account #200

josojo · 2020-05-01T20:22:50Z

closes #158

I eliminated the standardized proposer account.

I think in this PR, we could eliminate the lightwallet integration completely, but then I think in the future we want to have keystore file system for sure.
I am not yet sure, which keystore filesystem we should use, since the one from the gnosis-safe team is no longer maintained:
https://github.com/ConsenSys/eth-lightwallet/blob/master/lib/signing.js

Hence, I have decided to keep the lightwallet in there, in case we switch back in the future.

fedgiac

This PR comes very close to "implementing our own cryptography".
I'd rather pick a different package to compute transaction hashes instead of manipulating private keys directly. Ideally there already is a helper function in the Gnosis Safe repo, in the very worst case I'd copy the code there 1-to-1.
In any case I thought the main motivation of this PR was to remove eth-lightwallet because of this comment. If we are willing to keep eth-lightwallet in then we could simply use a different mnemonic in signTransaction instead of the default one.
Also, would the complete liquidity provision script work for us with the current setup? We currently use Ganache's default account as the proposer account, which means that we need its private key when running the scripts. But it can't hold any funds, so it can't send out the transaction creating the new Safes.

scripts/utils/internals.js

fedgiac · 2020-05-04T11:28:15Z

scripts/utils/sign_and_send.js

-    console.log(`Signing and posting multi-send transaction request from proposer account ${account}`)
-    const sigs = signTransaction(lightWallet, [account], transactionHash)
+    const privateKey = process.env.PK
+    const account = web3.eth.accounts.privateKeyToAccount("0x" + privateKey)


Implementation question: does this line change the output of web3.eth.getAccounts()?

Also seems hackey to be doing string manipulation in the vicinity of a private key...

According to the documentation, is only creating an account object, which I am storing in the account variable:
https://web3js.readthedocs.io/en/v1.2.0/web3-eth-accounts.html#privatekeytoaccount

@bh2smith How would you propose to do it better?

I guess this is standard,

web3/web3.js#1072 (comment)

However, I wonder what would happen if I provided the "0x" along with the private when exporting it. For example, if I had copied my PK from ganache?

which includes the 0x.

bh2smith · 2020-05-04T12:33:15Z

we could eliminate the lightwallet integration completely

I would be very much in favour of this. It is a non-standard library to my understanding anyway and is probably also the reason for that silly bitcore-lib issue we have been having.

josojo · 2020-05-04T18:55:36Z

I'd rather pick a different package to compute transaction hashes instead of manipulating private keys directly

I don't manipulate private keys. I can see that one would like to argue for a vault, but I feel like I don't make anything worse, as we have the PK anyways already exported as env. I think the vault integration that does not depend on unmaintained repos should happen in another PR.

In any case I thought the main motivation of this PR was to remove eth-lightwallet because of this comment.

No. I think that it is not acceptable that anyone can propose transactions to our safe. Once the transaction is in the interface, there is a much higher risk that it gets signed accidentally.
With this PR, external parties can no longer propose solutions to our Gnosis safe.

Also, would the complete liquidity provision script work for us with the current setup?

Yes, we need to modify the setup for this of the current Master_safe, but it's worth it, for sure.

scripts/utils/internals.js

scripts/utils/sign_and_send.js

fedgiac · 2020-05-05T15:24:55Z

I think the changes in this PR are good and needed. I also think the changes in the implementation do not introduce any issue that was not already present before this PR, so overall the PR is approved.

I still think we should remove eth-lightwallet and clean up all the remaining unused code. The code of this PR is compatible with our current liquidity setup, it's sufficient to extract the default Ganache private key from the hardcoded mnemonic. Only exception is the complete liquidity provision script, since the account that creates the brackets is also the proposer account with the current changes, and must thus hold funds. Since we'll have new brackets only in the new multisig, I don't think we need this kind of compatibility.

josojo · 2020-05-06T06:15:56Z

Okay, then I will remove the light wallet before we merge this one.

package.json

bh2smith

Bravo! I have been dreaming of a day when this project no longer depends on lightwallet.

fedgiac

This PR is great!

fedgiac · 2020-05-06T09:13:45Z

test/dfusion_multi_safes.js

+        masterSafe,
+        safeOwner.privateKey,
+        requestWithdrawalTransaction,
+        "request withdrawal for all brackets"


I really wonder where this string came from. Maybe you can remove it, since you removed a similar one somewhere else in the code?

fedgiac · 2020-05-06T09:14:26Z

test/dfusion_multi_safes.js

+        masterSafe,
+        safeOwner.privateKey,
+        transferFundsToMasterTransaction,
+        "transfer funds to master for all brackets"


Candidate string to remove, as before.

fleupold

fleupold · 2020-05-06T12:06:30Z

scripts/utils/sign_and_send.js

-    console.log(`Signing and posting multi-send transaction request from proposer account ${account}`)
-    const sigs = signTransaction(lightWallet, [account], transactionHash)
+    const privateKey = process.env.PK
+    const account = web3.eth.accounts.privateKeyToAccount(privateKey)


This method is expecting a 0x prefixed string but the PK is exported without the 0x in truffle (https://web3js.readthedocs.io/en/v1.2.0/web3-eth-accounts.html#privatekeytoaccount)

fleupold · 2020-05-06T13:02:18Z

I see now that if you tested this change by exporting a PK with the 0x prefix it works.

I should have checked the discussion on this PR before commenting above as it was originally implemented in the way that I would have expected it (allowing to paste a PK from MM).

I might have further been mislead by the absence of a test plan.

Fixes #154 Instead of hardcoding an internal gas limit (which already turned out to be brittle, cf #209), we should use the Safe's functionality to estimate the internal call appropriately. This is also done in the [safe-proxy-kit](https://github.com/gnosis/contract-proxy-kit/pull/49/files) and an example of how to do it can be found in the safe contracts [unit tests](safe-global/safe-smart-account#188) There are two tiny unrelated changes in this PR. One is a hotfix for #200 and the other one is a formatting issue that made only printed as: > Error: Error while talking to the gnosis-interface: [object Object] ### Test Plan ``` export NETWORK_NAME=rinkeby export GAS_PRICE_GWEI=9 export MASTER_SAFE=your master safe export PK=... npx truffle exec scripts/complete_liquidity_provision.js --targetToken=0 --stableToken=7 --lowestLimit=0.10 --highestLimit=1 --currentPrice=0.70 --masterSafe=$MASTER_SAFE --investmentTargetToken=0.05 --investmentStableToken=0.05 --fleetSize=20 --network=$NETWORK_NAME ``` See txs go through. Or check https://rinkeby.etherscan.io/tx/0xbcf31282c2350f8e521123d76d243a3c8a995f21024a8b25776c98c12993b450#internal which shows the hardcoded value of 4M **before this PR** and https://rinkeby.etherscan.io/tx/0x37deb7cd729e52e2d29d195df6b4ac16299f1b6b90b14e6f53300e201d39d3e3#internal for the exact estimated value **after this PR** (2.9M)

Individuell proposer account

8d93a90

josojo requested review from fedgiac and bh2smith May 1, 2020 20:22

fedgiac reviewed May 4, 2020

View reviewed changes

fedgiac reviewed May 5, 2020

View reviewed changes

scripts/utils/internals.js Outdated Show resolved Hide resolved

bh2smith approved these changes May 5, 2020

View reviewed changes

fedgiac reviewed May 5, 2020

View reviewed changes

scripts/utils/sign_and_send.js Outdated Show resolved Hide resolved

bh2smith requested a review from fedgiac May 5, 2020 18:06

josojo added 4 commits May 6, 2020 08:17

merging master

9a5be50

removing lightwallet dependency

51ecc71

Bens and Federico's suggestions

e44991a

Make ganache-deterministic with -d instead of -g

9de3f6c

bh2smith reviewed May 6, 2020

View reviewed changes

package.json Show resolved Hide resolved

bh2smith approved these changes May 6, 2020

View reviewed changes

josojo added 2 commits May 6, 2020 10:18

removing deterministic requirement for travis

daf77e1

merging head

d891685

fedgiac approved these changes May 6, 2020

View reviewed changes

Federico's comment

c3f1a86

josojo merged commit c1a65b9 into master May 6, 2020

josojo deleted the individuellProposeAccount branch May 6, 2020 11:32

fleupold reviewed May 6, 2020

View reviewed changes

fleupold mentioned this pull request May 6, 2020

[GasEstimate] Use safe method to estimate gas for internal call #211

Merged

fleupold mentioned this pull request May 6, 2020

[Easy] allow PKs with both 0x and no prefix #212

Merged

bh2smith mentioned this pull request May 15, 2020

Sign with web3-accounts #258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Individuell proposer account #200

Individuell proposer account #200

josojo commented May 1, 2020

fedgiac left a comment

fedgiac May 4, 2020

bh2smith May 4, 2020

josojo May 4, 2020

josojo May 4, 2020

bh2smith May 5, 2020

bh2smith commented May 4, 2020

josojo commented May 4, 2020

fedgiac commented May 5, 2020

josojo commented May 6, 2020

bh2smith left a comment •

edited

fedgiac left a comment

fedgiac May 6, 2020

fedgiac May 6, 2020

fleupold left a comment

fleupold May 6, 2020

fleupold commented May 6, 2020

Individuell proposer account #200

Individuell proposer account #200

Conversation

josojo commented May 1, 2020

fedgiac 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

bh2smith commented May 4, 2020

josojo commented May 4, 2020

fedgiac commented May 5, 2020

josojo commented May 6, 2020

bh2smith left a comment • edited

Choose a reason for hiding this comment

fedgiac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold commented May 6, 2020

bh2smith left a comment •

edited