Skip to content
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

Remove max modules #259

Merged
merged 9 commits into from
Sep 17, 2018
Merged

Remove max modules #259

merged 9 commits into from
Sep 17, 2018

Conversation

adamdossa
Copy link
Contributor

@adamdossa adamdossa commented Sep 16, 2018

Fix #260

@VictorVicente to make corresponding CLI changes

Uses an internal function for verifyTransfer rather than signature matching on msg.data

Removes MAX_MODULES and changes the onlyModule modifier to be constant time.

Modules can be:
archived: makes a module not valid for the purposes of onlyModule. Can be unarchived
unarchived: reverts archive
remove: permanently remove module (can't be re-added)

NB: We still have a dependency on the number of TM modules attached as we loop over these in verifyTransfer.

@adamdossa adamdossa changed the title Remove max modules WIP: Remove max modules Sep 16, 2018
@adamdossa adamdossa changed the title WIP: Remove max modules Remove max modules Sep 16, 2018
@pabloruiz55
Copy link
Contributor

Left a few small comments. Otherwise, looks good.
Only wondering if it we should really allow them to remove a module vs only just allowing them to archive them

contracts/interfaces/ISecurityToken.sol Outdated Show resolved Hide resolved
contracts/tokens/SecurityToken.sol Outdated Show resolved Hide resolved
contracts/tokens/SecurityToken.sol Outdated Show resolved Hide resolved
contracts/tokens/SecurityToken.sol Show resolved Hide resolved
@adamdossa
Copy link
Contributor Author

Thanks @satyamakgec @pabloruiz55 - have resolved comments.

@pabloruiz55 pabloruiz55 merged commit 63ca912 into development-1.5.0 Sep 17, 2018
@VictorVicente VictorVicente mentioned this pull request Sep 17, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants