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

Optimise STR #294

Merged
merged 25 commits into from
Oct 2, 2018
Merged

Optimise STR #294

merged 25 commits into from
Oct 2, 2018

Conversation

adamdossa
Copy link
Contributor

Optimise STR code size & fix modifyTicker bug

@adamdossa
Copy link
Contributor Author

@bakii

Please could you carefully review this PR as it fixes some bugs and makes some changes.

Previous bug was in modifyTicker - if you passed in a new ticker reference, it wouldn’t record this in userToTickers due to the logic in _modifyTicker

This was missed in the test cases, specifically the test case “Should add the new custom ticker” did not correctly record ownership. When we then transferred ownership in “Should change the details of the existing ticker” we actually then transfer the ownership of the wrong token since “ETH” does not have an index in “tickerIndex”.

We didn’t catch this in the getter test case “Should get the tickers by owner” as it checked that account_temp should have 2 tokens instead of 3. For this test case we should have:

token_owner: [DET, AAA, ETH, POLY]
account_temp: [DET2, LOG, LOG2]

@adamdossa
Copy link
Contributor Author

@satyamakgec Also fixed a few cases where we weren't using the correct upper case ticker value - please could you double check these as well.

@satyamakgec
Copy link
Contributor

Ah! I miss it, actually the cause of bug was that I focused on solving the modifySecurityToken() case in which we are calling _modifyTicker() but those actions also affect the real functionality of modifyTicker(). Thanks for spotting this bug.

contracts/SecurityTokenRegistry.sol Outdated Show resolved Hide resolved
contracts/SecurityTokenRegistry.sol Show resolved Hide resolved
@satyamakgec
Copy link
Contributor

Also did a small commit to optimize more.

@adamdossa adamdossa merged commit c06beaa into development-1.5.0 Oct 2, 2018
@adamdossa adamdossa deleted the optimise_str_size branch October 2, 2018 13:00
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

4 participants