Add price lib tracking in Core #246
Conversation
felix2feng
changed the title
Pull Request Test Coverage Report for Build 2373
💛 - Coveralls |
| * | ||
| * @param _priceLibrary The address of the Price Library to disable | ||
| */ | ||
| function disablePriceLibrary( |
There was a problem hiding this comment.
Why do we also need a disable method? Can we instead have one setter method where we set whether a price library is enabled?
There was a problem hiding this comment.
We could. I was just following the pattern we already have in place.
There was a problem hiding this comment.
Yeah it doesn't make sense to have two methods, lets add a card to get the other ones changed as well (also making disableSet bi directional). I'm also not sure we need the array and the map.
There was a problem hiding this comment.
There was a problem hiding this comment.
Can you make the changes for price library for this PR? No point merging this in and changing it later
contracts/core/interfaces/ICore.sol
Outdated
| * | ||
| * @param _priceLibrary The address of the PriceLibrary to enable/disable | ||
| */ | ||
| function setPriceLibrary( |
There was a problem hiding this comment.
Why are we exposing this/other setters again? Not sure we ever need to expose the owner-only setters. I think this adds to the runtime gas cost by having them all in a single core interface. @bweick, we should do an exercise on that and confirm. If true, we should definitely have logic specific interfaces that expose 1-2 methods at a time.
| * | ||
| * @param _priceLibrary The address of the Price Library to enable or disable | ||
| */ | ||
| function setPriceLibrary( |
There was a problem hiding this comment.
setPriceLibraryValid(address _priceLibrary, boolean _valid) or setPriceLibraryEnabled(address _priceLibrary, boolean _enabled)?
| // Mark as true or false in validPriceLibraries mapping | ||
| state.validPriceLibraries[_priceLibrary] = !state.validPriceLibraries[_priceLibrary]; | ||
|
|
||
| if (state.validPriceLibraries[_priceLibrary]) { |
There was a problem hiding this comment.
Then you can save on a lot of lookups here, not that this is a function we're going to be calling often.
| onlyOwner | ||
| { | ||
| // Mark as true or false in validPriceLibraries mapping | ||
| state.validPriceLibraries[_priceLibrary] = !state.validPriceLibraries[_priceLibrary]; |
There was a problem hiding this comment.
Same, a ton of lookups there. I'll be using one of the interfaces above for the other methods.
Completes this card.
Add new functions: addPriceLibrary and removePriceLibrary so that only approved price libraries can be used.