Skip to content
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

Fix/oz30 #306

Merged
merged 18 commits into from
Apr 21, 2020
Merged

Fix/oz30 #306

merged 18 commits into from
Apr 21, 2020

Conversation

alcueca
Copy link
Contributor

@alcueca alcueca commented Apr 20, 2020

It took me a while to update everything to work with OpenZeppelin contracts 3.0, some tricks they threw in, including changing the ERC20 constructor signature.

I actually had to discard some 20 commits and branch from a branch to try a different approach.

And they broke one of my toys, I had to do some hard stuff to fix Democracy.sol :(

@alcueca
Copy link
Contributor Author

alcueca commented Apr 20, 2020

@obernardovieira, any idea why this fails? It works locally and I updated my packages using yarn.

@alcueca alcueca requested a review from vibern0 April 20, 2020 21:34
@vibern0
Copy link
Member

vibern0 commented Apr 21, 2020

The problem is around the erc165 checker, which I do not have much experience, but I've seen a couple of things.

  1. in ERC20Whitelisted you are extending ERC20 without calling its constructor. Any reason for that?
  2. WhitelistInterfaceId might be abstract.
  3. in WhitelistERC165 you are extending ERC165. How ERC20Whitelisted is different from WhitelistERC165 and why isn't one extending the other?
  4. In ERC20Whitelisted's constructor, how can you call _ERC165Checker.supportsInterface, since _supportsInterface is internal and ERC165Checker is a library? I'm a bit confused.

I had exactly the same problems as the CI and after adding ERC20 constructor (from 1.) and removing the "require" in ERC20Whitelisted constructor, it worked (with one exception). I just don't know how you want to fix it.

 274 passing
  1 failing

  1) Contract: ERC20Whitelisted
       constructor throws if called with an address not implementing IWhitelist.:
     AssertionError: expected to throw, did not: expected false to be truthy
      at Context.<anonymous> (test/utils.js:14:20)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

@alcueca
Copy link
Contributor Author

alcueca commented Apr 21, 2020

Next time CI gets different results than my local machine, I should remember to remove the build folder and recompile.

Once I got the errors in my machine it was easy to fix. OpenZeppelin just changed the constructor for ERC20 and renamed that function in ERC165Cheker.

@alcueca alcueca merged commit 1deec2a into master Apr 21, 2020
@alcueca alcueca deleted the fix/oz30 branch April 21, 2020 14:18
@vibern0
Copy link
Member

vibern0 commented Apr 22, 2020

I should remember to remove the build folder and recompile

ahah that usually helps 😂
awesome, congrats for this work 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants