Skip to content
This repository was archived by the owner on Jan 18, 2023. It is now read-only.

Conversation

@asoong
Copy link
Contributor

@asoong asoong commented Jan 7, 2019

No description provided.

@asoong asoong requested review from bweick and felix2feng January 7, 2019 03:20
@asoong asoong force-pushed the alex/implement_whitelist_for_propose branch from b306ee9 to 0aaba9a Compare January 7, 2019 03:24
// to a propose that prohibit the set from carrying out an auction i.e. a token that only the manager possesses
IWhiteList rebalanceComponentWhitelist = IRebalancingSetFactory(_factory).rebalanceComponentWhitelist();
address[] memory components = ISetToken(_nextSet).getComponents();
for (uint256 i = 0; i < components.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could reduce the calls to whitelist by passing the array of components to the whitelist contract


// Check proposed components on whitelist. This is to ensure managers are unable to add contract addresses
// to a propose that prohibit the set from carrying out an auction i.e. a token that only the manager possesses
IWhiteList rebalanceComponentWhitelist = IRebalancingSetFactory(_factory).rebalanceComponentWhitelist();
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do not believe that the factory will change (which it shouldn't), we could cache the rebalanceComponentWhitelist in a state variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

@coveralls
Copy link

coveralls commented Jan 7, 2019

Pull Request Test Coverage Report for Build 3567

  • 7 of 7 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 3546: 0.0%
Covered Lines: 769
Relevant Lines: 769

💛 - Coveralls

@asoong asoong force-pushed the alex/implement_whitelist_for_propose branch from 42e3798 to a7882af Compare January 7, 2019 05:01
/**
* Validates address against white list
*
* @param _address Address to check
Copy link
Contributor

Choose a reason for hiding this comment

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

return javadoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

/**
* Verifies an array of addresses against the whitelist
*
* @return address[] Array of addresses to verify
Copy link
Contributor

Choose a reason for hiding this comment

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

return vs param javadoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

/**
* Verifies an array of addresses against the whitelist
*
* @return address[] Array of addresses to verify
Copy link
Contributor

Choose a reason for hiding this comment

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

should be param and add a return javadoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

/**
* Verifies an array of addresses against the whitelist
*
* @return address[] Array of addresses to verify
Copy link
Contributor

Choose a reason for hiding this comment

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

params javadoc

@asoong asoong force-pushed the alex/implement_whitelist_for_propose branch from a7882af to e1b88ea Compare January 7, 2019 05:20
@asoong asoong merged commit d7ab276 into master Jan 7, 2019
@asoong asoong deleted the alex/implement_whitelist_for_propose branch January 7, 2019 06:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants