-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deployment of MultiSig and TokenHolder master copy #53
Deployment of MultiSig and TokenHolder master copy #53
Conversation
Deployment of master copies and UserWalletFactory
Small cleanup changes of filenames
updated in requires deployContract => DeployContract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work.
Few inline comments.
lib/setup/UserSetup.js
Outdated
const jsonInterface = abiBinProvider.getABI(MultiSigMasterCopyContractName); | ||
const bin = abiBinProvider.getBIN(MultiSigMasterCopyContractName); | ||
|
||
let defaultOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are accepting txOptions in the params.So defaultOptions
is not needed.
Also, please remove it from _deployTokenHolderMasterCopyRawTx and _deployUserWalletFactoryRawTx
lib/setup/UserSetup.js
Outdated
const txObject = oThis._deployMultiSigMasterCopyRawTx(txOptions); | ||
|
||
let params = {}; | ||
params.contractName = MultiSigMasterCopyContractName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are constructing params in each deploy method.We can have private method for constructing params object.
What do you think ?
lib/setup/UserSetup.js
Outdated
return contract.deploy( | ||
{ | ||
data: bin, | ||
arguments: args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As there are no arguments for constructor of GnosisSafe
contract,we can remove arguments: args
.
Similar comments for UserWalletFactory
and TokenHolder
deployment
utils/DeployContract.js
Outdated
@@ -1,19 +1,21 @@ | |||
'use strict'; | |||
|
|||
// TODO Gulshan: Documentation | |||
// TODO Gulshan: class format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ticket created to address it :-
#54
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Great work 🥇
The PR covers below tasks: