-
Notifications
You must be signed in to change notification settings - Fork 22
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
Feature/dev 100 adapt contracts 2.0 #130
Feature/dev 100 adapt contracts 2.0 #130
Conversation
giacomolicari
commented
Dec 5, 2018
- Switch pm-contracts to v1.2.0
- Use truffle-nice-tools on network-reset (previously using scripts/inject_network_info)
- Use WETH9 instead of EtherToken
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.
Some small improvements can be made, but this is fine too.
@@ -101,7 +101,7 @@ export async function buyOutcomeTokens() { | |||
buyTxOpts = Object.assign({}, opts, buyTxOpts) | |||
|
|||
const market = await this.contracts.Market.at(marketAddress) | |||
const collateralToken = await this.contracts.Token.at( | |||
const collateralToken = await this.contracts.WETH9.at( |
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.
Is there an IERC20 ABI? It would be more appropriate, as outcome tokens do not support deposit()
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.
Thank you @cag for reviewing it.
I noticed that Token is not being imported anymore on new pm-contracts.
Talking about WETH9, on its ABI taken from pm-contracts build directory, we have:
{
"constant": false,
"inputs": [],
"name": "deposit",
"outputs": [],
"payable": true,
"stateMutability": "payable",
"type": "function"
}
Other available Token contracts are:
- BasicToken
- OutcomeToken
- OutcomeTokenProxy
- StandardToken
should we go for one of the aforementioned tokens?
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.
Whoa, I'm sorry I didn't see this reply earlier! :o
This is a minor issue that it shouldn't get in the way of this getting merged. I'll take a look at pm-contracts packaging and see if there is something to be done there.
Also I've not heard of BasicToken
...
@cag can you review this to push it forward? thank you! |