First pass at updating contracts to modular format. Further testing r… #287
Conversation
Pull Request Test Coverage Report for Build 2872
💛 - Coveralls |
| address _newRebalanceAuctionModule | ||
| ) | ||
| external | ||
| onlyOwner |
There was a problem hiding this comment.
We may want to timelock this function too..
There was a problem hiding this comment.
Yeah we should...another PR though once yours is merged
| * | ||
| * @param _newIssuanceOrderModule Address of deployed issuanceOrderModule contract | ||
| */ | ||
| function setIssuanceOrderModule( |
There was a problem hiding this comment.
Do we need this function? If we upgrade the issuanceOrderModule, can we just redeploy the wrappers as well?
Then, we wouldn't need to manage ownership or have the update module functions in each of the wrappers.
There was a problem hiding this comment.
I think as little redeployment of contracts as possible is best. Not necessarily because its cheaper but because we should try to make the system as static as possible, people in theory will be watching this contracts quite a bit and moving them around with a redeploy feels less scalable.
There was a problem hiding this comment.
If we are trying to house contract tracking in Core, then why not have the Wrapper call Core to check what the issuanceOrder module is?
| * | ||
| * Extension used to expose internal accounting functions to module | ||
| */ | ||
|
|
There was a problem hiding this comment.
Connect comment with line 31?
| * @param _to Address to credit for deposit | ||
| * @param _quantity Amount of tokens to deposit | ||
| */ | ||
| function depositModule( |
There was a problem hiding this comment.
Maybe we just expose the batchDeposit version and the modules can format if they only want to pass a single deposit in.
There was a problem hiding this comment.
Yeah I think this might be a better approach
| address _to, | ||
| uint256 _quantity | ||
| ) | ||
| external |
There was a problem hiding this comment.
I'm sure this is WIP, but need a check that this is a module.
There was a problem hiding this comment.
Yup added in newest commit
| ICoreIssuance, | ||
| ICoreAccounting, | ||
| CoreState, | ||
| contract IssuanceOrderModule is |
There was a problem hiding this comment.
Have you thought about naming ModuleIssuanceOrder vs IssuanceOrderModule? @asoong any thoughts?
There was a problem hiding this comment.
I prefer the latter cause it rolls off the tongue better. It's also it's own contract so don't feel like it falls under the same category as our core extension type contracts when we prefix with Core.
| address public vault; | ||
|
|
||
| // Mapping of filled Issuance Orders | ||
| mapping(bytes32 => uint) public orderFills; |
| mapping(bytes32 => uint) public orderFills; | ||
|
|
||
| // Mapping of canceled Issuance Orders | ||
| mapping(bytes32 => uint) public orderCancels; |
|
|
||
| // Verify signature is authentic | ||
| ISignatureValidator(state.signatureValidator).validateSignature( | ||
| ISignatureValidator(ICore(core).signatureValidator()).validateSignature( |
There was a problem hiding this comment.
Does it make sense for Core to house the signatureValidator variable or for issuanceOrder?
There was a problem hiding this comment.
I think the contract tracking should still all run through Core which is why I kept it there.
| ); | ||
|
|
||
| // Call Exchange | ||
| // // Call Exchange |
…ess to module only functions in core.
bweick
force-pushed
the
brian/implement_modules
branch
from
0e88c4f to
19587f8
Compare
…th associated tests.
| // Make sure sender is rebalanceAuctionModule | ||
| require( | ||
| msg.sender == IRebalancingSetFactory(factory).core(), | ||
| msg.sender == IRebalancingSetFactory(factory).rebalanceAuctionModule(), |
There was a problem hiding this comment.
If we house contracts in Core, which ones make sense to house in the RebalancingSetFactory?
There was a problem hiding this comment.
Main reason I didn't add specific module names to the Core is because when we add new modules we won't be able to add their specific identifier to Core. As I think about it we don't actually need to specifically identify which module is calling these functions just that a module we approve of is, so I'd be fine just doing something like ICore(core).validModule(msg.sender) which I think accomplishes the same thing instead of specifying the module we need.
| * | ||
| * @param _newIssuanceOrderModule Address of deployed issuanceOrderModule contract | ||
| */ | ||
| function setIssuanceOrderModule( |
There was a problem hiding this comment.
If we are trying to house contract tracking in Core, then why not have the Wrapper call Core to check what the issuanceOrder module is?
| function addModule( | ||
| address _module | ||
| ) | ||
| external |
There was a problem hiding this comment.
Reminder that we will need to add timelock Modifier
| */ | ||
| function bid( | ||
| address _rebalancingSetToken, | ||
| function issueModule( |
There was a problem hiding this comment.
If we don't upgrade Core, we might want to expose other functions like redeemInternal as well
| * @param _module Factory address | ||
| * @return bool Boolean indicating if enabled factory | ||
| */ | ||
| function validModules( |
There was a problem hiding this comment.
validModule vs validModules?
There was a problem hiding this comment.
We pluralized it for the other functions was just going for consistency
| external | ||
| onlyOwner | ||
| { | ||
| state.validModules[_module] = true; |
There was a problem hiding this comment.
We should emit an event, so we can keep track of history
There was a problem hiding this comment.
Lets jsut add that to your PR since you have a standard for how you've been doing those.
| external | ||
| onlyOwner | ||
| { | ||
| state.validModules[_module] = false; |
There was a problem hiding this comment.
Same re: event. But we can do it later.
…tory and exchangeWrappers. Added redeemModule.
…dress and increment address to meet all the burning variations.
| returns (address[], uint256[], uint256[]) | ||
| { | ||
| // Make sure sender is Core | ||
| // Make sure sender is rebalanceAuctionModule |
There was a problem hiding this comment.
In actuality, we're checking that its a valid module
| */ | ||
| contract RebalancingSetTokenFactory { | ||
| contract RebalancingSetTokenFactory is | ||
| Ownable |
| */ | ||
| contract KyberNetworkWrapper { | ||
| contract KyberNetworkWrapper is | ||
| Ownable |
There was a problem hiding this comment.
Same - don't think we need it.
| */ | ||
| contract TakerWalletWrapper { | ||
| contract TakerWalletWrapper is | ||
| Ownable |
| require( | ||
| msg.sender == IRebalancingSetFactory(factory).core(), | ||
| ICore(IRebalancingSetFactory(factory).core()).validModules(msg.sender), | ||
| "RebalancingSetToken.placeBid: Sender must be core" |
| */ | ||
| contract ZeroExExchangeWrapper { | ||
| contract ZeroExExchangeWrapper is | ||
| Ownable |
…equired.