Product lines basic #70
Conversation
| abstract contract BaseProductLine { | ||
| // Constants | ||
|
|
||
| uint256 constant REENTRANCYLOCK__UNLOCKED = 1; |
There was a problem hiding this comment.
let's inherit from OZ maybe?
There was a problem hiding this comment.
Ideally we would decide on the import policy. We didn't use to do it at all, but now the synths are... but not the reentrancy lock.. Leave it for consistency? Or do it in the synths as well?
There was a problem hiding this comment.
I think we discussed that eventually synths will be moved to a separate repo. I think we decided to do the same for the product lines, right? I think in repos outside EVK we should be using external dependencies freely
src/ProductLines/BaseProductLine.sol
Outdated
| emit Genesis(); | ||
| } | ||
|
|
||
| function makeNewVaultInternal(address asset, bool upgradeable, address oracle, address unitOfAccount) |
There was a problem hiding this comment.
make upgradable the first param
|
|
||
| uint256 private reentrancyLock; | ||
|
|
||
| mapping(address vault => bool created) public vaultLookup; |
There was a problem hiding this comment.
this can also be taken from OZ - enumerable set
There was a problem hiding this comment.
same as reentrancy test - either use the imports everywhere or not. The set is an overkill for the needs here and we'll still need the getters..
| vault.setSymbol(string.concat("e", getTokenSymbol(asset))); | ||
|
|
||
| vault.setFeeReceiver(feeReceiver); | ||
| vault.setGovernorAdmin(governor); |
There was a problem hiding this comment.
what about IRM? we need to set something default which should probably be stored as immutable.
btw, I noticed that we have inconsistency in Governance module:
getter is called interestRateModel and setter is called setIRM
There was a problem hiding this comment.
the default value for IRM is address(0) which is handled as 0% interest set. Good point about the setter
There was a problem hiding this comment.
I'll fix the setter straight on master
There was a problem hiding this comment.
the default value for IRM is address(0) which is handled as 0% interest set.
I know and I think it's problematic. when a new borrowable vault enabled, it should at least have a default interest rate installed. similar to what we had in Euler v1
There was a problem hiding this comment.
A new Core vault won't have any LTV's either, so its not usable without further config anyway.
There was a problem hiding this comment.
we'll probably need some default set of collaterals that gets set up of every new core vault
| IEVault vault = makeNewVaultInternal(asset, UPGRADEABLE, oracle, unitOfAccount); | ||
|
|
||
| vault.setName(string.concat("Core vault: ", getTokenName(asset))); | ||
| vault.setSymbol(string.concat("e", getTokenSymbol(asset))); |
There was a problem hiding this comment.
symbol must also encode the product line I think. otherwise there's no way to distinguish between two vaults of the same asset by looking at the symbol
There was a problem hiding this comment.
even within a product line there could be duplicate assets. In core, the governor can simply change the name.
Escrow does enforce single asset per line. How about escrUSDC @hoytech ?
But what about Edge vaults? edgUSDCn? where n is a counter of USDC vaults?
There was a problem hiding this comment.
good point re multiple vaults of the same asset. the UI assumes that vaults names and symbols also have numbers. I think without them, it will be harder to handle for FE team. why don't we have a counter for each asset and add a number to the name and symbol? i.e. coreUSDC3?
There was a problem hiding this comment.
Product lines will be developed further for sure. Let's maybe keep these improvements in mind for further work? For now we want yAudit to get a sense of what these things are
dglowinski
force-pushed
the
product-lines-basic
branch
from
56b114c
to
d2aede1
Compare
|
|
||
| if (isEVCCompatible(asset)) { | ||
| uint32 flags = IEVault(newVault).configFlags(); | ||
| IEVault(newVault).setConfigFlags(flags | CFG_EVC_COMPATIBLE_ASSET); |
There was a problem hiding this comment.
Will this BaseProductLine.sol be the governor address always? or should we transfer governance at the end of the creation?
There was a problem hiding this comment.
Ah I see it is handled in the functions that inherits this one 👍🏽
| address public governor; | ||
| address public feeReceiver; |
There was a problem hiding this comment.
Should we have functions that allow to set those addresses by governor?
Core and Escrow product lines included.
Needs testing