-
Notifications
You must be signed in to change notification settings - Fork 160
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
Support multiple module types #305
Conversation
# Conflicts: # contracts/modules/Checkpoint/ERC20DividendCheckpointFactory.sol # contracts/modules/Checkpoint/EtherDividendCheckpointFactory.sol # contracts/tokens/SecurityToken.sol
# Conflicts: # contracts/modules/Checkpoint/ERC20DividendCheckpointFactory.sol # contracts/modules/Checkpoint/EtherDividendCheckpointFactory.sol # contracts/tokens/SecurityToken.sol
Agreed. It doesn't affect CLI. |
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.
The logic seems sound to me but I don't really know the exact requirements. I have listed some minor possible optimizations below. @satyamakgec and @pabloruiz55 can take over from here.
contracts/tokens/SecurityToken.sol
Outdated
|
||
function _removeModuleWithIndex(uint8 _type, uint256 _index) internal { | ||
uint256 length = modules[_type].length; | ||
modules[_type][_index] = modules[_type][length - 1]; |
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.
modules[_type][_index] = modules[_type][length - 1];
can be moved to the if block below and length-- can be done after the if statement. Very minor change that can save some gas when length - 1 == index
.
Logic is good @adamdossa but we can also move the investor related functions into the library. It does make sense to move them as not many other functions depend on it. It will save up to I did some changes regarding that check here https://github.com/PolymathNetwork/polymath-core/blob/some-changes-adampr/contracts/libraries/TokenLib.sol If you think that these changes are worth to add then we can merge these changes into yours. |
Just some handy stats. |
Migrate the investors logic from ST to library
Support modules having multiple types.
To Do: Add proof-of-concept module for testing multiple types
Includes lots of refactoring in SecurityToken to manage contract size limitations.
@VictorVicente I checked the CLI code and couldn't see any impact.