-
Notifications
You must be signed in to change notification settings - Fork 31
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
Use npm and git submodules #728
Conversation
Code deduplication by getting dependencies from npm and git submodules. * `UtilityToken` contracts are now imported as a git submodule. * `Organization` contracst are now imported as a git submodule inside the `UtilityToken` submodule. * `SafeMath` is now imported as open zeppelin npm package. Note that inside the utility token repository, `token` has not been renamed to `value token`, and therefore some tests needed to be updated. Also, some tests required updating as `SafeMath` does not give an error message anymore. `Organiation` contracts cannot be imported as a separate git submodule dependency, as then the solidity compiler will complain that there are multiple definitions of the same symbol (e.g. `contract Organization`). For now it is fine to import organization from inside utility tokens, but that will lead to an error latest when there is a diamond dependency: ``` Organization / \ UtilityToken SomethingElse \ / NewRepository ``` In this case, it couldn't be avoided that `Organization` was defined multiple times. To fix this, I created follow-up ticket: OpenST/developer-guidelines#27 Deduplicating EIP20Token is out-of-scope, as it requires changing UtilityToken contracts (which has an EIP20Token) and all consumers of UtilityToken, which currently may or may not rely on the EIP20Token from there. Simply adding the npm package does not work, as there will be an error about multiple definitions of the same Symbol (`contract EIP20Token`). To fix this, I created a follow-up ticket: OpenST/developer-guidelines#28 Fixes OpenST#712
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 good to me 👍
I just have one question. Why 2.1.1 and not 2.2.0 for openzeppelin-solidity?
I used 2.1.1, because that is what was used in the other repositories. For now, I would rather use the same version across all repositories. 2.2 also has no messages and 2.3 is still a pre-release... |
@deepesh-kn what do you think? |
The same version across all repositories sounds good to me 👍 |
Code deduplication by getting dependencies from npm and git submodules.
UtilityToken
contracts are now imported as a git submodule.Organization
contracst are now imported as a git submodule insidethe
UtilityToken
submodule.SafeMath
is now imported as open zeppelin npm package.Note that inside the utility token repository,
token
has not beenrenamed to
value token
, and therefore some tests needed to be updated.Also, some tests required updating as
SafeMath
does not give an errormessage anymore.
Organiation
contracts cannot be imported as a separate git submoduledependency, as then the solidity compiler will complain that there are
multiple definitions of the same symbol (e.g.
contract Organization
).For now it is fine to import organization from inside utility tokens,
but that will lead to an error latest when there is a diamond
dependency:
In this case, it couldn't be avoided that
Organization
was definedmultiple times.
To fix this, I created follow-up ticket:
OpenST/developer-guidelines#27
Deduplicating EIP20Token is out-of-scope, as it requires changing
UtilityToken contracts (which has an EIP20Token) and all consumers of
UtilityToken, which currently may or may not rely on the EIP20Token from
there.
Simply adding the npm package does not work, as there will be an error
about multiple definitions of the same Symbol (
contract EIP20Token
).To fix this, I created a follow-up ticket:
OpenST/developer-guidelines#28
Fixes #712