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.

Sign up for GitHub

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

Jump to bottom

Migrate data of GTM #538

Merged
merged 15 commits into from
Feb 6, 2019
Merged

Migrate data of GTM #538

merged 15 commits into from
Feb 6, 2019

Conversation

satyamakgec
Copy link
Contributor

No description provided.

@satyamakgec satyamakgec changed the title migrate data of gtm [WIP]: Migrate data of GTM Feb 1, 2019
@satyamakgec satyamakgec changed the title [WIP]: Migrate data of GTM Migrate data of GTM Feb 5, 2019
maxsam4
maxsam4 suggested changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@maxsam4 maxsam4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only partial review completed yet.

contracts/libraries/VersionUtils.sol Outdated Show resolved Hide resolved
contracts/libraries/VersionUtils.sol Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Feb 6, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 96.86% when pulling eff3390 on migrate-gtm-data into 25e8f0e on dev-3.0.0.

maxsam4
maxsam4 reviewed Feb 6, 2019
View reviewed changes
Copy link
Contributor

@maxsam4 maxsam4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left some minor comments regarding optimization.

contracts/modules/TransferManager/GeneralTransferManager.sol Outdated Show resolved Hide resolved
contracts/modules/TransferManager/GeneralTransferManager.sol Show resolved Hide resolved
@adamdossa
Copy link
Contributor

I modified a couple of the function names and added a _validLockTime function to make code a bit more readable.

@adamdossa
Copy link
Contributor

adamdossa commented Feb 6, 2019 •
edited
Loading

I also wonder if we could do something more sensible with the investors data. We currently have a list of investors (append-only) in the token, and the list of investors (KYC data individuals) in the GTM. We could think about consolidating this in the dataStore possibly in a single data set. This could be a separate PR though.

@satyamakgec satyamakgec merged commit 22c48b3 into dev-3.0.0 Feb 6, 2019
@satyamakgec satyamakgec deleted the migrate-gtm-data branch October 30, 2019 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxsam4 maxsam4 maxsam4 approved these changes

@adamdossa adamdossa Awaiting requested review from adamdossa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@satyamakgec @coveralls @adamdossa @maxsam4 @SatyamSB