search

gnosis / mock-contract

Simple Solidity contract to mock dependent contracts in truffle tests.
94 stars 24 forks source link

Upgrade solidity to 0.5.0 #3

Closed s1na closed 5 years ago

s1na commented 5 years ago

I was trying to upgrade TransferLimitModule to use solidity 0.5.0 and faced some difficulties with compiling MockContract, and had to upgrade the method signatures for my purposes locally. I thought I'd also make a PR upstream :)

If you're not planning to upgrade to 0.5.0, please feel free to reject this PR.

On a different note: When running the tests I get new BigNumber() not a base 16 number for 3 of the cases (both before and after upgrade). The other tests pass after the upgrade.

Note: I had to run cd node_modules/truffle && npm install solc@0.5.0 && cd ../.. locally to make it work, as pointed out by Richard.

fleupold commented 5 years ago

We are happy to merge your contribution. It looks like @denisgranha is working on something similar (https://github.com/gnosis/mock-contract/compare/feature/solidity-0.5.0).

I checked current master on my local machine and it is passing all tests (also Travis is green on master). Would you be able to get the tests to pass?

s1na commented 5 years ago

Oh, sorry I saw there was no open PR, but didn't think about checking branches! Now I see in addition to the work you mentioned, there's also this.

Hm, it might be somehow related to the environment then (a dependency version maybe). Master also seems to be failing due to the same error.

I tried to pinpoint the issue. It seems to happen when return type is string and no return value has been set. This issue suggests similar issues in web3js. I'm wondering if web3js is failing to parse an empty bytes returned as string.

denisgranha commented 5 years ago

yeah, its failing even with the master branch now, checked truffle and it’s dependencies and seems that they don’t accept empty string as valid value

rmeissner commented 5 years ago

I would propose closing this in favour of #4