Conversation
brandoniles
left a comment
brandoniles
left a comment
There was a problem hiding this comment.
LGTM, but please what for @ahnaguib or @aalavandhan 's approval before merging.
test/unit/MedianOracle_gas_cost.ts
Outdated
| @@ -0,0 +1,49 @@ | |||
| // TODO(naguib): Fail tests if gas utilization changes | |||
| import { ethers } from 'hardhat' | |||
| import { BigNumber, Contract, ContractFactory, Signer } from 'ethers' | |||
There was a problem hiding this comment.
I think we can just get rid of this. We wrote this when there wasn't a lot of tooling for solidity development.
We now have a hardhat plugin built in which does this out of the box ,,
https://www.npmjs.com/package/hardhat-gas-reporter
contracts/MedianOracle.sol
Outdated
| @@ -0,0 +1,223 @@ | |||
| pragma solidity 0.4.24; | |||
|
|
|||
| import "openzeppelin-solidity/contracts/math/SafeMath.sol"; | |||
There was a problem hiding this comment.
I wonder if you can specify the version explicitly as i've done here ..
There was a problem hiding this comment.
so I tried a few things here. I could yarn add from the github path as you had, in the case "https://github.com/OpenZeppelin/openzeppelin-solidity.git#2.0.0" actually resolves to "https://github.com/OpenZeppelin/openzeppelin-contracts.git#2.0.0", but the package name remains "openzeppelin-solidity" for purposes of importing from the contract code
contracts/MedianOracle.sol
Outdated
| @@ -0,0 +1,223 @@ | |||
| pragma solidity 0.4.24; | |||
|
|
|||
| import "openzeppelin-solidity/contracts/math/SafeMath.sol"; | |||
There was a problem hiding this comment.
| import "openzeppelin-solidity/contracts/math/SafeMath.sol"; | |
| import {SafeMath} from "openzeppelin-solidity/contracts/math/SafeMath.sol"; |
contracts/MedianOracle.sol
Outdated
| pragma solidity 0.4.24; | ||
|
|
||
| import "openzeppelin-solidity/contracts/math/SafeMath.sol"; | ||
| import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; |
There was a problem hiding this comment.
| import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; | |
| import {Ownable} from "openzeppelin-solidity/contracts/ownership/Ownable.sol"; |
|
I wonder if we can just use the latest solidity versions (and oz dependencies) for the market oracle contracts you've imported? If we redeploy we'll bump them up anyway. We could add an issue and get back to it in a later PR, no rush. |
|
Open to doing this. Of course, would not match the existing deployed oracle, so maybe better for a separate PR, possibly including any feature update proposals |
3 participants
Migrates MedianOracle contract to this repository, also migrating the testing from truffle/js to hardhat/ts.
Open questions: