Support solidity 0.6 in Initializable contract #1413
Conversation
Modifies Initializable so it is compatible with Solidity 0.5 and 0.6. Adds a set of tests that run on the 0.6 version to verify its behaviour (note that the 0.6 compiled contracts are committed into source control to simplify the setup).
spalladino
force-pushed
the
feature/initializable-solidity-06
branch
from
d1a44d2
to
65993b0
Compare
Both mock-solc and mock-dependency where being copied instead of symlinked into packages/lib by yarn. Even worse, they were not re-copied if they were modified, requiring to rm-rf them, or update the cache in the CI. By defining mock-solc and mock-dependency as workspaces, yarn will symlink them from where they are requested, instead of copying them. This required moving them outside the packages/lib workspace, as yarn does not support nested workspaces.
There was a problem hiding this comment.
I love how you used the same code to test both 0.5 and 0.6 version of the same contract. I have a few questions but they are probably me missing something. Except that all seems good!
I can't compile mock-solc-0.6 with oz compile because of an error Could not find file /Users/iYalovoy/repo/sdk-review/tests/mocks/mock-solc-0.6/contracts/initializable/Initializable.sol in the project (imported from contracts). It seems symlink Initializable.sol is broken. Does it works for you?
I also couldn't compile it with Remix they have issue at that moment with solc 0.6 so no playing with code for me.
...b/test/mocks/mock-solc-0.6/contracts/initializable/MultipleInheritanceInitializableMocks.sol
Outdated
Show resolved
Hide resolved
...b/test/mocks/mock-solc-0.6/contracts/initializable/MultipleInheritanceInitializableMocks.sol
Outdated
Show resolved
Hide resolved
| function initialize(uint256 _mother, uint256 _gramps, uint256 _father, uint256 _child) initializer public { | ||
| SampleMother.initialize(_mother); | ||
| SampleFather.initialize(_gramps, _father); | ||
| child = _child; | ||
| } | ||
| } |
There was a problem hiding this comment.
Again, couldn't run the code so my question is why this doesn't cause to a duplicated call at SampleHuman. initialize? Since both SampleMother and SampleFather are children of SampleHuman.
There was a problem hiding this comment.
Actually, it does, and it's one of the things tested here (note that this test existed from before, I just moved it and migrated it to 0.6). There was a bug in Initializable that caused it to fail in these scenarios with an already initialized error. This test checks that it's ok to call a base initializer more than once during the initialization process, to accommodate for structures like this one.
There was a problem hiding this comment.
Hmm. Didn't know that our current approach is that your init method might be called twice. This is certainly tricky and not many devs would expect that. I like the direction we are moving with Transpiler.
Hmm it works for me, and also works on the CI. Do you know if OSX handles symlinks differently? Or could this be related to this git setting? |
|
OSX should handle them fine but if it works on CI and your machine it is fine. I've got all the answer I wanted. Thanks! |
Modifies Initializable so it is compatible with Solidity 0.5 and 0.6. Adds a set of tests that run on the 0.6 version to verify its behaviour (note that the 0.6 compiled contracts are committed into source control to simplify the setup).
I suggest reviewing commits separately. The 2nd is somewhat unrelated to this PR, but required for the tests to pass (it was either that, or manually killing the CircleCI cache).