feat: add option to require currency pairs to be pre-authorized before initialization #1
Conversation
| function setPrice(bytes32 _currencyPair, uint128 _price) external onlyOracle { | ||
| latestBlockNumber = block.number; | ||
|
|
||
| if (!currencyPairInfo[_currencyPair].initialized) { | ||
| revert UninitializedCurrencyPair(0); | ||
| } | ||
|
|
||
| priceData[latestBlockNumber][_currencyPair] = PriceData(_price, block.timestamp); | ||
| emit PriceDataUpdated(_currencyPair, _price); | ||
| } |
There was a problem hiding this comment.
Is this functionality used only in case of single price pair? IIRC gas with the loop on anything more than 1 price pair is cheaper?
There was a problem hiding this comment.
it's not actually used in geth right now - it's cheaper to use to loop in anything >1 pair. I added this in a backup in case the loop has an issue such as out of gas, but in that case we could still use the loop function with less pairs. I can remove this for now
| function setRequireCurrencyPairAuthorization(bool _requireCurrencyPairAuthorization) external onlyOwner { | ||
| requireCurrencyPairAuthorization = _requireCurrencyPairAuthorization; | ||
| } |
There was a problem hiding this comment.
Should we only allow disabling this property? There are some odd edge cases if you don't, ie authorization is only a moving forward property, only applies to ability to initialize. See my comment on if price pairing decimals change.
There was a problem hiding this comment.
it might make sense to only allow it to be disabled since we don't have the ability to un-initialize pairs. if we add the ability to un-initialize, we should probably allow this to be toggled
| } | ||
|
|
||
| function authorizeCurrencyPair(bytes32 _currencyPair) external onlyOwner { | ||
| authorizedCurrencyPairs[_currencyPair] = true; | ||
| } | ||
|
|
||
| function initializeCurrencyPair(bytes32 _currencyPair, uint8 _decimals) external onlyOracle { |
There was a problem hiding this comment.
Is there any reason to have this have an early return if the currency pair is already initialized?
Should it be possible to ever change a currency pair decimals? This would serve as a guard against that?
If there is a valid pathway where we would change currency pairs:
- I don't think this would work right now in the contract setup?
- If you had switched to requiring whitelist (maybe we shouldn't allow this), then the update would fail to make it through.
I suppose if the execution client code never calls once initialized it's not a real issue and would cost gas to have the check. It is free gas though as our executions are configured, and would make it more safe against code issues in execution client?
There was a problem hiding this comment.
Should it be possible to ever change a currency pair decimals? This would serve as a guard against that?
theoretically, the decimals could change, as they're defined in a Market within the market map, which can be updated via an action. so we should allow the contract to change its decimals as well. however this is not currently supported in geth, although it could be as geth receives the decimals with each price. the current behaviour is that geth will never call this again once initialized.
If you had switched to requiring whitelist (maybe we shouldn't allow this), then the update would fail to make it through.
yes, in this case we should not update the price until the pair is re-initialized (after being authorized)
| currencyPairInfo[_currencyPair] = CurrencyPairInfo(true, _decimals); | ||
| emit CurrencyPairInitialized(_currencyPair, _decimals); | ||
| } | ||
|
|
||
| function updatePriceData(bytes32[] memory _currencyPairs, uint128[] memory _prices) external onlyOracle { | ||
| function setPrices(bytes32[] memory _currencyPairs, uint128[] memory _prices) external onlyOracle { |
There was a problem hiding this comment.
This already seems to be the case, but we should maybe document somewhere on how authorization works.
Authorization is only for a currencyPair to be initialized, once initialized it always exists.
## Summary update geth chart and genesis bytecode to reflect changes from astriaorg/astria-oracle-contracts#1. geth chart is now on latest commit from astriaorg/astria-geth#62. ## Background required for oracle contract updates ## Changes - update geth image dev tag - update geth genesis AstriaOracle deployed bytecode ## Testing I ran the dev cluster and ensured oracle prices were still being updated.
AstriaOracleto be ownablerequireCurrencyPairAuthorizationis false, the contract/rollup behave the same as previously