Added maxGasPrice setting to SanityRates, and updated to solidity 6 #1053
Conversation
ayobuenavista
force-pushed
the
sanity/maxGasPrice
branch
from
b6affb6 to
1cbd443
Compare
ayobuenavista
force-pushed
the
sanity/maxGasPrice
branch
from
e706c9c to
934782e
Compare
ayobuenavista
force-pushed
the
sanity/maxGasPrice
branch
7 times, most recently
from
70e7f57 to
887422a
Compare
ayobuenavista
force-pushed
the
sanity/maxGasPrice
branch
from
887422a to
5d16b31
Compare
| setGasPrice(_maxGasPriceWei); | ||
| } | ||
|
|
||
| /// @dev set reasonableDiffInBps of a token to MAX_RATE if you want to ignore |
There was a problem hiding this comment.
would write:
set reasonableDiffInBps of a token to MAX_RATE to avoid handling the price feed for this token.
Hard to understand current sentence, "ignore sanity rate for it", seems the sentence is in reserve level perspective.
so better write it in this contract perspective.
easier to communicate it like this.
There was a problem hiding this comment.
Okay, changed it to this comment.
|
|
||
| contract SanityRatesGasPrice is IKyberSanity, Withdrawable3, Utils5 { | ||
| struct SanityData { | ||
| uint168 tokenRate; |
There was a problem hiding this comment.
lets use here:
uint128
uint128
those numbers look a little weird.
especially when later used for casting in the file.
| } | ||
| } | ||
|
|
||
| function getSanityRate(IERC20 src, IERC20 dest) external override view returns (uint256) { |
There was a problem hiding this comment.
please use the return value to declare 'rate' parameter.
it has 3 advantages:
- we use this parameter which in on the stack anyway.
- the function is clearer when we say name of return value (in this case not a big diff, in other examples more important)
- it saves another extra line of declaring another parameter.
test/sol6/sanityRatesGasPrice.js
Outdated
| sanityRates = await SanityRates.new(admin, gasPrice); | ||
| await sanityRates.addOperator(operator); | ||
|
|
||
| await sanityRates.setReasonableDiff(tokens, reasonableDiffs); |
There was a problem hiding this comment.
this is never called by non admin address
test/sol6/sanityRatesGasPrice.js
Outdated
| }); | ||
|
|
||
| it('should test setting sanity rates.', async function () { | ||
| await sanityRates.setSanityRates(tokens, rates, {from: operator}); |
There was a problem hiding this comment.
not this is never called by non operator address
|
|
||
|
|
||
| contract MockReserveSanity is IKyberReserve, Utils5 { | ||
| using SafeERC20 for IERC20; |
There was a problem hiding this comment.
could be better to just make this a mock reserve.
not only for sanity.
like mock reserve with fixed rates we use in Katalyst regression.
And the mock reserve can call sanity if address isn't 0.
but don't need to handle it now.
just name this mockReserve and add todo for trade function.
There was a problem hiding this comment.
There is already MockReserve.sol, but I forked it since I didn't know if modifying the existing one will affect the other existing tests. Anyway I can just use this instead. Just need to add more lines on sanity tests for reserve setup.
also fixing
#168