-
Notifications
You must be signed in to change notification settings - Fork 59
Conversation
d07721f
to
9a4bee6
Compare
Pull Request Test Coverage Report for Build 2089
💛 - Coveralls |
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.
Took an initial look (need to look again at the mustache templates). See my comments. Major thing is if we can get a type re: contract instance
"sol-trace-set": "^0.0.1", | ||
"solium": "^1.1.7", | ||
"tiny-promisify": "^1.0.0", | ||
"truffle-hdwallet-provider": "^0.0.3", | ||
"truffle-hdwallet-provider": "^1.0.0-web3one.0", |
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.
That's cool this dep has a web3 1.0 version
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.
it is required as part of the truffle 5 migration efforts too i think
"node_modules/*" | ||
] | ||
}, | ||
"declaration": true, | ||
"declarationDir": "./dist/typings" | ||
"declarationDir": "./dist/typings", | ||
"esModuleInterop": true |
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.
What's the benefit / need for esModuleInterop
?
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.
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.
Required in order to allow for default imports (so we don't have to do require('...')
) for modules that don't explicitly have that
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.
in particular i think web3 and some other package had this problem that this was able to solve
@@ -14,14 +14,14 @@ export class BaseContract { | |||
public abi: any[]; | |||
// public abi: Web3.AbiDefinition[]; | |||
|
|||
public web3ContractInstance: Web3.ContractInstance; | |||
public web3ContractInstance: any; |
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.
I get really nervous when we're moving to an any
type. Is there another type we could use?
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.
If it doesn't exist, we should consider creating our own type
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.
There is no other type right now defined at least. web3.eth.Contract
is a class that has a constructor to generate classes, but I couldn't get it working as a type yet
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.
i guess same thing should be done for the // public abi: Web3.AbiDefinition[];
comment we have above. not sure why it was commented to be any
there as well
"node_modules/*" | ||
] | ||
}, | ||
"declaration": true, | ||
"declarationDir": "./dist/typings" | ||
"declarationDir": "./dist/typings", | ||
"esModuleInterop": true |
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.
@@ -38,7 +38,15 @@ module.exports = { | |||
gas: 0xfffffffffff, | |||
gasPrice: 0x01, | |||
}, | |||
} | |||
}, | |||
compilers: { |
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.
do we still need the top level solc:
option?
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.
i think we may be able to get rid of that
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.
still need it, or the lower level option is not getting picked up
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.
interesting, i know it picks up the compiler version, but maybe the solc is not somehow...or maybe its in your local truffle version not using it properly? Didn't you say you weren't on truffle 5 yet?
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 can also keep for backwards compatibility on older truffle versions
No description provided.