-
Notifications
You must be signed in to change notification settings - Fork 69
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
Replace toWei/fromWei when base token conversion #1318
Conversation
@@ -366,10 +366,10 @@ export class FixedRateExchange { | |||
const estGas = await this.estSetRate( | |||
address, | |||
exchangeId, | |||
await this.unitsToAmount(exchange.baseToken, String(newRate)) | |||
await this.amountToUnits(exchange.baseToken, String(newRate)) |
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.
why noy toString()?
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.
this is what was used above so I used to be consistent
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.
strange, we always use toString, don't know how that stayed there.
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.
I will change it
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.
feel free to update all String(value) in other places if you find more of this 😃
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.
ok, I will
const estGas = await this.estSetRate( | ||
address, | ||
exchangeId, | ||
this.web3.utils.toWei(String(newRate)) | ||
await this.amountToUnits(exchange.baseToken, newRate.toString()) |
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.
this conversions should also take place in estSetRate()
just pass the raw string values to the estimate method and convert them there also
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.
right, makes sense
I run into an issue when using When sending the rate to the contract it contains 6 zeros so |
Code Climate has analyzed commit cee1bc2 and detected 8 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 63.4% (50% is the threshold). This pull request will bring the total coverage in the repository to 71.2% (-0.2% change). View more on Code Climate. |
Fixes #1279 .