solace-fi / solace-core

solace.fi smart contracts
GNU General Public License v3.0
6 stars 3 forks source link

Refactor/soteria (WIP) #239

Closed kyzooghost closed 2 years ago

kyzooghost commented 2 years ago

TODO


COMMIT LOG

Soteria refactor 6

Realised needed to split _maxRate into _maxRateNum and _maxRateDenom, because 10% annual rate => _maxRate = 1/315360000

Soteria refactor 5

Renamed _closePolicy to _deactivatePolicy, implemented calls to _deactivatePolicy() in both deactivatePolicy() and chargePremiums()

Soteria refactor 4

Added sanity check to withdraw() and _canPurchaseNewCover()

Renamed _rewardPoints mapping to _rewardPointsOf

Changed giftRewardPoints() to setRewardPoints()

Soteria refactor 3

Added _beforeTokenTransfer(…) hook to ensure

Renamed totalCoverLimit to activeCoverLimit (as per specs)

Added chargeCycle variable, as well as related public getter and governance-only setter functions.

Changed maxChargeableRate to maxRate.

require (minAccountBalance = maxRate chargeCycle coverLimit) is incorporated into activatePolicy(), updatePolicy(), and withdraw()

Made sure withdraw() function follows these specs:

Renamed _soteriaAccountBalance mapping to _accountBalanceOf

Made sure activatePolicy(…) follows these specs:

Soteria refactor 2

Soteria refactor 1

DISCUSSED CHANGES THAT I’VE DECIDED TO KEEP IN THIS PR

leonardishere commented 2 years ago

Removing _totalPolicyCount in favour of using ERC721.totalSupply(). OpenZeppelin implementation of the ERC721 standard does not have a totalSupply() method - https://docs.openzeppelin.com/contracts/2.x/api/token/erc721. To get a totalSupply() method, we need to inherit ERC721Enumerable.sol, or the method (saves 70% gas on NFT mint compared to inheriting ERC721Enumerable.sol) described here https://shiny.mirror.xyz/OUampBbIz9ebEicfGnQf5At_ReMHlZy0tB4glb9xQ0E. Given that both these methods involve bloating the code considerably, in comparison to what we already have with _totalPolicyCount, I propose to keep _totalPolicyCount as it is.

I just read the mirror article. _totalPolicyCount acts as a counter as described in the article so this implementation should be more efficient but I haven't gone that deep into testing gas costs.

Since Soteria uses 1 account - 1 policy it might not be necessary to inherit ERC721Enumerable but it would be nice to have some of the functions in case they need to be indexed.

function totalSupply() external view returns (uint256) { return _totalPolicyCount; }
function tokenByIndex(uint256 index) external view returns (uint256) { require(index < _totalPolicyCount); return index+1 }
function tokenOfOwnerByIndex(address owner, uint256 index) external view returns (uint256) { require(index == 0); uint256 policyID = _policyOf[owner]; require(policyID != 0); return policyID; }

It might be possible to cut down on the size of the code by overwriting the transfer functionality like https://github.com/solace-fi/solace-core/blob/feat/xSOLACEv2/contracts/xSOLACE.sol#L120-L163. maybe not

function transferFrom(address _from, address _to, uint256 _tokenId) external payable;
function safeTransferFrom(address _from, address _to, uint256 _tokenId) external payable;
function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external payable;
function approve(address _approved, uint256 _tokenId) external payable;
function setApprovalForAll(address _operator, bool _approved) external;
function getApproved(uint256 _tokenId) external view returns (address);
function isApprovedForAll(address _owner, address _operator) external view returns (bool);
leonardishere commented 2 years ago

I also recommend something like this for tokenURI https://github.com/solace-fi/solace-core/blob/feat/xSOLACEv2/contracts/ERC721Enhanced2.sol#L144-L173 https://github.com/solace-fi/solace-core/blob/df60c301e4dce9c2465d0f660e1488d82e9fb3c8/contracts/xsLocker.sol#L388-L396