Revamp Initializable and remove Migratable

[frangio]

Fixes #2.

Enhances Initializable to make it usable with multiple inheritance, with the purpose of removing Migratable (which was our current solution) because it leads to a confusing developer experience.

Marking in progress because I should update the docs to reflect the new usage, but the code and tests can already be reviewed.

• Fix contracts
• Fix rest of test suite
• Document changes

[spalladino]

Looks good! Can you move the PR to next instead of master, though?

[spalladino]

Can we add another suite where mother inherits from gramps as well, so we test a diamond-like inheritance graph?

[frangio]

Rebased on next.

[frangio]

@spalladino I changed the tested contracts into a diamond shaped inheritance as you suggested (kind of, as I didn't add a new suite, heh). I'm not sure how I feel about it though, because as we talked offline it will run the initializer twice, and although in this scenario the result ends up being correct, it might not always be the case. I suppose the warning on the Initializable contract coupled with the documentation will make it clear to be on the lookout for those problems...

[frangio]

@spalladino There's something we didn't think through: if we're removing Migratable, what should we do about upgradeAndCall? I apparently broke the tests for upgradeAndCall because they were using a Migratable contract as the mock implementation. This makes sense because the function you call via upgradeAndCall needs some mechanism to prevent it running more than once, and Migratable was that mechanism.

For the record, we had agreed on removing Migratable on the basis that the need for migrations really hasn't come up yet and we don't want to design it in advance of knowing the requirements of a real situation.

What do you suggest we do?

[spalladino]

@frangio upgradeAndCall still makes sense, the same way createAndCall makes sense even if we didn't provide any Initializable options. It now becomes the user's responsibility to ensure that a migration method is only called once, since we are not imposing any "best practice" to do so. We can keep the upgradeAndCall tests which call a regular migrate method, that simply relies on a hasBeenMigrated flag managed manually, instead of an isMigration(,,) modifier.

[frangio]

@spalladino I think the only error remaining is one in complex-example which I'm not sure how to fix. (To be honest I haven't looked into it yet but it looks kinda hairy.)

Error: Could not find zos-lib/contracts/migrations/Migratable.sol from any sources; imported from openzeppelin-zos/contracts/ownership/Ownable.sol

[frangio]

I added Migratable back in because due to Truffle's wrong handling of nested dependencies, there was no way to make the openzeppelin-zos package in lib-complex compile.

I removed the bump to 2.0.0-alpha because that wasn't going to fix anything.

One last thing. Currently Initializable is in the contracts/migrations/ directory. We cannot rename the directory because of the same reason we had to keep Migratable, but we could move Initializable somewhere else. @spalladino What do you think?

[spalladino]

+1 on moving Initializable to the root of Contracts, while keeping the deprecated Migratable in its old location

…

On Thu, Aug 23, 2018 at 1:37 PM Francisco Giordano ***@***.***> wrote: I added Migratable back in because due to Truffle's wrong handling of nested dependencies, there was no way to make the openzeppelin-zos package in lib-complex compile. I removed the bump to 2.0.0-alpha because that wasn't going to fix anything. One last thing. Currently Initializable is in the contracts/migrations/ directory. We cannot rename the directory because of the same reason we had to keep Migratable, but we could move Initializable somewhere else. @spalladino <https://github.com/spalladino> What do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaOJH1dFSYu3iybudOQ3gt2W8lW-3iLks5uTtpBgaJpZM4V28XP> .