-
Notifications
You must be signed in to change notification settings - Fork 300
refactor: code optimisation for erc20 #7553
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
Conversation
0bbc64f to
c0491b5
Compare
Taseen08
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm from WP, coins team review would be more relevant
c0491b5 to
b6a8478
Compare
pritam-gembali
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good effort to simplify. Let's do it the right way please
5c25d02 to
e58fc9e
Compare
e58fc9e to
ccd5a2f
Compare
3dda2b1 to
d325b85
Compare
mmcshinsky-bitgo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests to make sure there aren't any regressions to these changes?
Taseen08
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we address the comment regarding test cov?
d325b85 to
644708d
Compare
644708d to
d8693ab
Compare
d8693ab to
b76e247
Compare
Taseen08
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
concerns addressed - EthLikeErc20Token is used for evm coins and ERC20Token instance is already being used by eth coin

Ticket: WIN-7914
Optimisation:
coinFactory.ts - Added map-based routing for EthLike chains
Adding to the above map would reduce code bloat by decreasing the size of the switch case. Around 6 chains from the switch case would be removed and added to the map, and future coins would only be added to the map, reducing multiple lines of code and code bloat for each chain.
tokenConfig.ts - Currently has repetitive token config functions
the above generic method reduces around 120 lines by removing all the token specific methods:
This avoids code repetition, keeping the code clean and preventing code bloat.
Also extracted TokenNetwork interface and created reusable type definition for token network structure, created getFormattedTokensByNetwork helper and consolidated mainnet/testnet token formatting logic
Before & After

Before (Token Configuration)
After (Token Configuration)

Test case results: