TMP: include changes for BIMv2 in BIMv1to compare changes #1
Conversation
| { | ||
| require(_quantity > 0, "Redeem quantity must be > 0"); | ||
|
|
||
| address hookContract = _callManagerPreRedeemHooks(_setToken, _quantity, msg.sender, _to); |
There was a problem hiding this comment.
DIM doesn't have manager pre redeem hooks. not sure if we need that even if we add module hooks.
There was a problem hiding this comment.
Yeah, since neither DIM nor BIMv1 have this, I guess it wouldn't be needed. The only example of managerHooks I found in our tests is the max supply hook (see here) which only needs to be called at issuance.
| * @param _setToken Instance of the SetToken to update manager hook | ||
| * @param _newHook New manager hook contract address | ||
| */ | ||
| function updateManagerIssuanceHook( |
There was a problem hiding this comment.
this functionality is new. Not sure if really needed. Could be excluded because we have module hooks?
There was a problem hiding this comment.
We do use the supplyCapHook which is a manager issuance hook, so could be good to have a way to update it.
| require(!_setToken.hasExternalPosition(components[i]), "Only default positions are supported"); | ||
|
|
||
| notionalUnits[i] = _setToken.getDefaultPositionRealUnit(components[i]).toUint256().preciseMulCeil(_quantity); | ||
| uint256 units = _setToken.getDefaultPositionRealUnit(components[i]).toUint256(); |
There was a problem hiding this comment.
not exactly as in DIM, but almost
| * If a pre-redeem hook has been configured, call the external-protocol contract's pre-redeem function. | ||
| * Pre-issue hook logic can contain arbitrary logic including validations, external function calls, etc. | ||
| */ | ||
| function _callManagerPreRedeemHooks( |
There was a problem hiding this comment.
again, DIM doesn't call manager pre redeem hooks
There was a problem hiding this comment.
Generally LGTM.
ManagerPreredeemHook is probably not needed as you hinted at already.
If we do decide to go ahead with this we would probably want to implement the changes first in our set protocol fork though, and also test it there. (Also we probably want to add a new contract "BasicIssuanceModuleV2" instead of modifying the existing one).
| { | ||
| require(_quantity > 0, "Redeem quantity must be > 0"); | ||
|
|
||
| address hookContract = _callManagerPreRedeemHooks(_setToken, _quantity, msg.sender, _to); |
There was a problem hiding this comment.
Yeah, since neither DIM nor BIMv1 have this, I guess it wouldn't be needed. The only example of managerHooks I found in our tests is the max supply hook (see here) which only needs to be called at issuance.
| * @param _setToken Instance of the SetToken to update manager hook | ||
| * @param _newHook New manager hook contract address | ||
| */ | ||
| function updateManagerIssuanceHook( |
There was a problem hiding this comment.
We do use the supplyCapHook which is a manager issuance hook, so could be good to have a way to update it.
| * @return uint256[] List of component units required to issue the quantity of SetTokens | ||
| */ | ||
| function getRequiredComponentUnitsForIssue( | ||
| function getRequiredComponentIssuanceUnits( |
There was a problem hiding this comment.
This is nice. Having different names for this method between BIM and DIM was really annoying before :+1
There was a problem hiding this comment.
just noticed that they also differ in returned values...
ideally BIMv2 would have to return an empty array as return param 3 (for debt positions) I think
| * have a way to redeem their Sets | ||
| * SET TOKEN ONLY: Allows removal (and deletion of state) of BasicIssuanceModuleV2 | ||
| */ | ||
| function removeModule() external override { |
There was a problem hiding this comment.
This method is not protected right ? So anyone can call it ?
There was a problem hiding this comment.
Nevermind / False alarm: I just figured out that the method would fail if the caller is not a set token 👍
No description provided.