Skip to content

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.

@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.

4 participants