Contract Architecture approach - separation of data and business concerns #11
Conversation
contract architecture for the project, should be viewed more as an example of architecture design than the prescriptive version of how the contracts should look
contract, which is assigned as a data contract to the fundFactory.
| fund.invest(_userAddress, _investmentAmounts[i]); | ||
| } | ||
|
|
||
| portfolioData.createNewUserAllocation(_userAddress, _pc, _investmentAmounts, _funds); |
There was a problem hiding this comment.
@Andy-Bell I'm not sure if this PR was meant to cover the business requirements but it seems that _investmentAmounts are not that useful for PortfolioData. The amounts are invested into the funds at line 37 so we have the history of investments on the blockchain and the sum of contributions in those funds. The portfolio would only ever store the initial contribution amounts because after creating a new allocation or by updating it we overwrite the old amounts.
There was a problem hiding this comment.
With how the DataContract is written (currently), the Functional contract would need to take existing investment, add the new investment to it, then send that on to the data contract as the number that should be stored as investment.
But this PR is more designed at an "architecture" level than business level, I tried to include business concerns from the user flows, but this is in no way meant to be a prescriptive "these are how the functions will work exactly".
| // they are just to demonstrate the separation of concerns | ||
| // into functional and data contracts. | ||
| function createNewUserPortfolio(address _userAddress, address[] _funds, uint[] _pc, uint[] _investmentAmounts) public { | ||
| require(msg.sender == owner || msg.sender == _userAddress); |
There was a problem hiding this comment.
@Andy-Bell Should every user create their own instance of the PortfolioFunctional contract? Because the require(msg.sender == owner || msg.sender == _userAddress); enforces that.
There was a problem hiding this comment.
My mistake, should be tx.origin not msg.sender, as msg sender gets reset on each external call
There was a problem hiding this comment.
Seems like using tx.origin is not recommended:
ethereum/solidity#683
There was a problem hiding this comment.
I forgot about the spoofing of it, probably will need a slight reshuffle:
- use msg.sender again
- functional contract needs a new function "reallocateDataContract" which is onlyOwner, and calls the change ownership of the data contract assigned
- after creation of the functional contract, the data contract owner should be updated (think it is called transferOwnership) to be the linked functional contract (called from initial owner)
So the flow goes:
Data contract made (owner akr)
Func contract made (owner akr)
Data contract transfered (using transferOwnership) (owner akr -> Func)
new Func contract made (owner akr)
Data contract transfered (using reallocateDataContract) (owner Func -> newFunc)
does that make sense?
There was a problem hiding this comment.
@Andy-Bell yes, thanks.
| function createNewFund(address _fundOwner, string _fundName) onlyOwner public returns(FundFunctional) { | ||
| FundData fundData = new FundData(); | ||
| FundFunctional fund = new FundFunctional(fundData, _fundName); | ||
| fundRegistry.createNewFund(_fundOwner, fund, _fundName); |
There was a problem hiding this comment.
fundRegistry.createNewFund fails because the function has onlyOwner modifier and the caller here is the FundFactory which is not the owner of the FundRegistry.
There was a problem hiding this comment.
I think a potential solution to this would be similar to my proposed solution in the other comment thread
This PR adds in several new contracts as an example of a 'best practice' approach to contract architecture.
The idea behind this approach is to give maximum preservation of data, even if business logic needs to be changed. This is achieved via creating separate contracts to contain the more 'app' focused logic (in the PR they are the xFunctional contracts) and the storing of data (the xData contracts).
Normally, by deploying a new contract you make the previous data unusable/unreachable for the new functionality (as the data would have been stored on the old contract) without a gas-expensive data migration between the two contracts, this approach allows the replacement of functional contracts with no loss of Data due to the Data contract changing.
At it's most basic level, each functional contract has a data contract as a contract-level public variable (set in the constructor), this can then allow it to be instantiated and have the public functions called upon it, allowing for basic CRUD operations on the stored data (in both examples, the stored data is a mapping). We can discuss this further on the call.