Conversation
Now working! Needs merge and release of zeppelinos/zos-lib#6 before merging |
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.
Looks great! Left a few minor comments, but the code is much cleaner now. zOS starts looking awesome :-)
event Withdrawal(address indexed wallet, uint256 value); | ||
event NewDonation(address indexed donor, uint256 value, uint256 r, uint256 g, uint256 b); | ||
|
||
function initialize(address owner) public { |
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.
This behaviour needs to be abstracted into a reusable module (not now, though)
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.
+1
test/Basil.test.js
Outdated
}) | ||
|
||
it('sets the right registry', async function () { | ||
await this.basil.transferOwnership(proxyOwner, {from: owner}) |
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.
Why this call to transferOwnership
in this test?
truffle.js
Outdated
@@ -2,11 +2,20 @@ require('babel-register'); | |||
require('babel-polyfill'); | |||
|
|||
module.exports = { | |||
mocha: { | |||
bail: 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.
Remove this option, so the CI can show all errors
truffle.js
Outdated
networks: { | ||
ropsten: { | ||
host: "localhost", | ||
host: 'localhost', |
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.
Ropsten on localhost:8545
seems like a particular configuration on your host. Should we remove this entry?
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 didn't make that change, I just changed " for '
And please squash-and-merge this commit when merging :-P |
No description provided.